[ESLint] Suggest moving inside a Hook or useCallback when bare function is a dependency (#15026)

* Warn about bare function deps and suggest moving or useCallback

* Clearer wording

packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js

@@ -313,7 +313,7 @@ const tests = {
     },
     {
       code: `
-        function MyComponent({ maybeRef2 }) {
+        function MyComponent({ maybeRef2, foo }) {
           const definitelyRef1 = useRef();
           const definitelyRef2 = useRef();
           const maybeRef1 = useSomeOtherRefyThing();
@@ -323,8 +323,8 @@ const tests = {
           const [state4, dispatch2] = React.useReducer();
           const [state5, maybeSetState] = useFunnyState();
           const [state6, maybeDispatch] = useFunnyReducer();
-          function mySetState() {}
-          function myDispatch() {}
+          const mySetState = useCallback(() => {}, []);
+          let myDispatch = useCallback(() => {}, []);
 
           useEffect(() => {
             // Known to be static
@@ -380,8 +380,8 @@ const tests = {
           const [state5, maybeSetState] = useFunnyState();
           const [state6, maybeDispatch] = useFunnyReducer();
 
-          function mySetState() {}
-          function myDispatch() {}
+          const mySetState = useCallback(() => {}, []);
+          let myDispatch = useCallback(() => {}, []);
 
           useEffect(() => {
             // Known to be static
@@ -662,30 +662,6 @@ const tests = {
         }
       `,
     },
-    {
-      code: `
-        function MyComponent(props) {
-          function handleNext1() {
-            console.log('hello');
-          }
-          const handleNext2 = () => {
-            console.log('hello');
-          };
-          let handleNext3 = function() {
-            console.log('hello');
-          };
-          useEffect(() => {
-            return Store.subscribe(handleNext1);
-          }, [handleNext1]);
-          useLayoutEffect(() => {
-            return Store.subscribe(handleNext2);
-          }, [handleNext2]);
-          useMemo(() => {
-            return Store.subscribe(handleNext3);
-          }, [handleNext3]);
-        }
-      `,
-    },
     {
       // Declaring handleNext is optional because
       // it doesn't use anything in the function scope.
@@ -950,8 +926,12 @@ const tests = {
         }
       `,
       errors: [
-        "React Hook useMemo doesn't serve any purpose without a dependency array as a second argument.",
-        "React Hook useCallback doesn't serve any purpose without a dependency array as a second argument.",
+        "React Hook useMemo doesn't serve any purpose without a dependency array. " +
+          'To enable this optimization, pass an array of values used by the inner ' +
+          'function as the second argument to useMemo.',
+        "React Hook useCallback doesn't serve any purpose without a dependency array. " +
+          'To enable this optimization, pass an array of values used by the inner ' +
+          'function as the second argument to useCallback.',
       ],
     },
     {
@@ -3313,6 +3293,443 @@ const tests = {
           'Either include it or remove the dependency array.',
       ],
     },
+    {
+      // Even if the function only references static values,
+      // once you specify it in deps, it will invalidate them.
+      code: `
+        function MyComponent(props) {
+          let [, setState] = useState();
+
+          function handleNext(value) {
+            setState(value);
+          }
+
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+        }
+      `,
+      // Not gonna autofix a function definition
+      // because it's not always safe due to hoisting.
+      output: `
+        function MyComponent(props) {
+          let [, setState] = useState();
+
+          function handleNext(value) {
+            setState(value);
+          }
+
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+        }
+      `,
+      errors: [
+        `The 'handleNext' function makes the dependencies of ` +
+          `useEffect Hook (at line 11) change on every render. ` +
+          `To fix this, move the 'handleNext' function ` +
+          `inside the useEffect callback. Alternatively, ` +
+          `wrap the 'handleNext' definition into its own useCallback() Hook.`,
+      ],
+    },
+    {
+      // Even if the function only references static values,
+      // once you specify it in deps, it will invalidate them.
+      code: `
+        function MyComponent(props) {
+          let [, setState] = useState();
+
+          const handleNext = (value) => {
+            setState(value);
+          };
+
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+        }
+      `,
+      // We don't autofix moving (too invasive). But that's the suggested fix
+      // when only effect uses this function. Otherwise, we'd useCallback.
+      output: `
+        function MyComponent(props) {
+          let [, setState] = useState();
+
+          const handleNext = (value) => {
+            setState(value);
+          };
+
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+        }
+      `,
+      errors: [
+        `The 'handleNext' function makes the dependencies of ` +
+          `useEffect Hook (at line 11) change on every render. ` +
+          `To fix this, move the 'handleNext' function ` +
+          `inside the useEffect callback. Alternatively, ` +
+          `wrap the 'handleNext' definition into its own useCallback() Hook.`,
+      ],
+    },
+    {
+      // Even if the function only references static values,
+      // once you specify it in deps, it will invalidate them.
+      // However, we can't suggest moving handleNext into the
+      // effect because it is *also* used outside of it.
+      // So our suggestion is useCallback().
+      code: `
+        function MyComponent(props) {
+          let [, setState] = useState();
+
+          const handleNext = (value) => {
+            setState(value);
+          };
+
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+
+          return <div onClick={handleNext} />;
+        }
+      `,
+      // We autofix this one with useCallback since it's
+      // the easy fix and you can't just move it into effect.
+      output: `
+        function MyComponent(props) {
+          let [, setState] = useState();
+
+          const handleNext = useCallback((value) => {
+            setState(value);
+          });
+
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+
+          return <div onClick={handleNext} />;
+        }
+      `,
+      errors: [
+        `The 'handleNext' function makes the dependencies of ` +
+          `useEffect Hook (at line 11) change on every render. ` +
+          `To fix this, wrap the 'handleNext' definition into its own useCallback() Hook.`,
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          function handleNext1() {
+            console.log('hello');
+          }
+          const handleNext2 = () => {
+            console.log('hello');
+          };
+          let handleNext3 = function() {
+            console.log('hello');
+          };
+          useEffect(() => {
+            return Store.subscribe(handleNext1);
+          }, [handleNext1]);
+          useLayoutEffect(() => {
+            return Store.subscribe(handleNext2);
+          }, [handleNext2]);
+          useMemo(() => {
+            return Store.subscribe(handleNext3);
+          }, [handleNext3]);
+        }
+      `,
+      // Autofix doesn't wrap into useCallback here
+      // because they are only referenced by effect itself.
+      output: `
+        function MyComponent(props) {
+          function handleNext1() {
+            console.log('hello');
+          }
+          const handleNext2 = () => {
+            console.log('hello');
+          };
+          let handleNext3 = function() {
+            console.log('hello');
+          };
+          useEffect(() => {
+            return Store.subscribe(handleNext1);
+          }, [handleNext1]);
+          useLayoutEffect(() => {
+            return Store.subscribe(handleNext2);
+          }, [handleNext2]);
+          useMemo(() => {
+            return Store.subscribe(handleNext3);
+          }, [handleNext3]);
+        }
+      `,
+      errors: [
+        "The 'handleNext1' function makes the dependencies of useEffect Hook " +
+          "(at line 14) change on every render. To fix this, move the 'handleNext1' " +
+          'function inside the useEffect callback. Alternatively, wrap the ' +
+          "'handleNext1' definition into its own useCallback() Hook.",
+        "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +
+          "(at line 17) change on every render. To fix this, move the 'handleNext2' " +
+          'function inside the useLayoutEffect callback. Alternatively, wrap the ' +
+          "'handleNext2' definition into its own useCallback() Hook.",
+        "The 'handleNext3' function makes the dependencies of useMemo Hook " +
+          "(at line 20) change on every render. To fix this, move the 'handleNext3' " +
+          'function inside the useMemo callback. Alternatively, wrap the ' +
+          "'handleNext3' definition into its own useCallback() Hook.",
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          function handleNext1() {
+            console.log('hello');
+          }
+          const handleNext2 = () => {
+            console.log('hello');
+          };
+          let handleNext3 = function() {
+            console.log('hello');
+          };
+          useEffect(() => {
+            handleNext1();
+            return Store.subscribe(() => handleNext1());
+          }, [handleNext1]);
+          useLayoutEffect(() => {
+            handleNext2();
+            return Store.subscribe(() => handleNext2());
+          }, [handleNext2]);
+          useMemo(() => {
+            handleNext3();
+            return Store.subscribe(() => handleNext3());
+          }, [handleNext3]);
+        }
+      `,
+      // Autofix doesn't wrap into useCallback here
+      // because they are only referenced by effect itself.
+      output: `
+        function MyComponent(props) {
+          function handleNext1() {
+            console.log('hello');
+          }
+          const handleNext2 = () => {
+            console.log('hello');
+          };
+          let handleNext3 = function() {
+            console.log('hello');
+          };
+          useEffect(() => {
+            handleNext1();
+            return Store.subscribe(() => handleNext1());
+          }, [handleNext1]);
+          useLayoutEffect(() => {
+            handleNext2();
+            return Store.subscribe(() => handleNext2());
+          }, [handleNext2]);
+          useMemo(() => {
+            handleNext3();
+            return Store.subscribe(() => handleNext3());
+          }, [handleNext3]);
+        }
+      `,
+      errors: [
+        "The 'handleNext1' function makes the dependencies of useEffect Hook " +
+          "(at line 15) change on every render. To fix this, move the 'handleNext1' " +
+          'function inside the useEffect callback. Alternatively, wrap the ' +
+          "'handleNext1' definition into its own useCallback() Hook.",
+        "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +
+          "(at line 19) change on every render. To fix this, move the 'handleNext2' " +
+          'function inside the useLayoutEffect callback. Alternatively, wrap the ' +
+          "'handleNext2' definition into its own useCallback() Hook.",
+        "The 'handleNext3' function makes the dependencies of useMemo Hook " +
+          "(at line 23) change on every render. To fix this, move the 'handleNext3' " +
+          'function inside the useMemo callback. Alternatively, wrap the ' +
+          "'handleNext3' definition into its own useCallback() Hook.",
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          function handleNext1() {
+            console.log('hello');
+          }
+          const handleNext2 = () => {
+            console.log('hello');
+          };
+          let handleNext3 = function() {
+            console.log('hello');
+          };
+          useEffect(() => {
+            handleNext1();
+            return Store.subscribe(() => handleNext1());
+          }, [handleNext1]);
+          useLayoutEffect(() => {
+            handleNext2();
+            return Store.subscribe(() => handleNext2());
+          }, [handleNext2]);
+          useMemo(() => {
+            handleNext3();
+            return Store.subscribe(() => handleNext3());
+          }, [handleNext3]);
+          return (
+            <div
+              onClick={() => {
+                handleNext1();
+                setTimeout(handleNext2);
+                setTimeout(() => {
+                  handleNext3();
+                });
+              }}
+            />
+          );
+        }
+      `,
+      // Autofix wraps into useCallback where possible (variables only)
+      // because they are only referenced outside the effect.
+      output: `
+        function MyComponent(props) {
+          function handleNext1() {
+            console.log('hello');
+          }
+          const handleNext2 = useCallback(() => {
+            console.log('hello');
+          });
+          let handleNext3 = useCallback(function() {
+            console.log('hello');
+          });
+          useEffect(() => {
+            handleNext1();
+            return Store.subscribe(() => handleNext1());
+          }, [handleNext1]);
+          useLayoutEffect(() => {
+            handleNext2();
+            return Store.subscribe(() => handleNext2());
+          }, [handleNext2]);
+          useMemo(() => {
+            handleNext3();
+            return Store.subscribe(() => handleNext3());
+          }, [handleNext3]);
+          return (
+            <div
+              onClick={() => {
+                handleNext1();
+                setTimeout(handleNext2);
+                setTimeout(() => {
+                  handleNext3();
+                });
+              }}
+            />
+          );
+        }
+      `,
+      errors: [
+        "The 'handleNext1' function makes the dependencies of useEffect Hook " +
+          '(at line 15) change on every render. To fix this, wrap the ' +
+          "'handleNext1' definition into its own useCallback() Hook.",
+        "The 'handleNext2' function makes the dependencies of useLayoutEffect Hook " +
+          '(at line 19) change on every render. To fix this, wrap the ' +
+          "'handleNext2' definition into its own useCallback() Hook.",
+        "The 'handleNext3' function makes the dependencies of useMemo Hook " +
+          '(at line 23) change on every render. To fix this, wrap the ' +
+          "'handleNext3' definition into its own useCallback() Hook.",
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          const handleNext1 = () => {
+            console.log('hello');
+          };
+          function handleNext2() {
+            console.log('hello');
+          }
+          useEffect(() => {
+            return Store.subscribe(handleNext1);
+            return Store.subscribe(handleNext2);
+          }, [handleNext1, handleNext2]);
+          useEffect(() => {
+            return Store.subscribe(handleNext1);
+            return Store.subscribe(handleNext2);
+          }, [handleNext1, handleNext2]);
+        }
+      `,
+      // Normally we'd suggest moving handleNext inside an
+      // effect. But it's used by more than one. So we
+      // suggest useCallback() and use it for the autofix
+      // where possible (variable but not declaration).
+      output: `
+        function MyComponent(props) {
+          const handleNext1 = useCallback(() => {
+            console.log('hello');
+          });
+          function handleNext2() {
+            console.log('hello');
+          }
+          useEffect(() => {
+            return Store.subscribe(handleNext1);
+            return Store.subscribe(handleNext2);
+          }, [handleNext1, handleNext2]);
+          useEffect(() => {
+            return Store.subscribe(handleNext1);
+            return Store.subscribe(handleNext2);
+          }, [handleNext1, handleNext2]);
+        }
+      `,
+      // TODO: we could coalesce messages for the same function if it affects multiple Hooks.
+      errors: [
+        "The 'handleNext1' function makes the dependencies of useEffect Hook " +
+          '(at line 12) change on every render. To fix this, wrap the ' +
+          "'handleNext1' definition into its own useCallback() Hook.",
+        "The 'handleNext1' function makes the dependencies of useEffect Hook " +
+          '(at line 16) change on every render. To fix this, wrap the ' +
+          "'handleNext1' definition into its own useCallback() Hook.",
+        "The 'handleNext2' function makes the dependencies of useEffect Hook " +
+          '(at line 12) change on every render. To fix this, wrap the ' +
+          "'handleNext2' definition into its own useCallback() Hook.",
+        "The 'handleNext2' function makes the dependencies of useEffect Hook " +
+          '(at line 16) change on every render. To fix this, wrap the ' +
+          "'handleNext2' definition into its own useCallback() Hook.",
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          let [, setState] = useState();
+          let taint = props.foo;
+
+          function handleNext(value) {
+            let value2 = value * taint;
+            setState(value2);
+            console.log('hello');
+          }
+
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          let [, setState] = useState();
+          let taint = props.foo;
+
+          function handleNext(value) {
+            let value2 = value * taint;
+            setState(value2);
+            console.log('hello');
+          }
+
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+        }
+      `,
+      errors: [
+        `The 'handleNext' function makes the dependencies of ` +
+          `useEffect Hook (at line 14) change on every render. ` +
+          `To fix this, move the 'handleNext' function inside ` +
+          `the useEffect callback. Alternatively, wrap the ` +
+          `'handleNext' definition into its own useCallback() Hook.`,
+      ],
+    },
     {
       code: `
         function Counter() {

packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js

@@ -93,10 +93,12 @@ export default {
           reactiveHookName === 'useCallback'
         ) {
           context.report({
-            node: node,
+            node: node.parent.callee,
             message:
               `React Hook ${reactiveHookName} doesn't serve any purpose ` +
-              `without a dependency array as a second argument.`,
+              `without a dependency array. To enable ` +
+              `this optimization, pass an array of values used by the ` +
+              `inner function as the second argument to ${reactiveHookName}.`,
           });
         }
         return;
@@ -550,7 +552,57 @@ export default {
         duplicateDependencies.size +
         missingDependencies.size +
         unnecessaryDependencies.size;
+
       if (problemCount === 0) {
+        // If nothing else to report, check if some callbacks
+        // are bare and would invalidate on every render.
+        const bareFunctions = scanForDeclaredBareFunctions({
+          declaredDependencies,
+          declaredDependenciesNode,
+          componentScope,
+          scope,
+        });
+        bareFunctions.forEach(({fn, suggestUseCallback}) => {
+          let message =
+            `The '${fn.name.name}' function makes the dependencies of ` +
+            `${reactiveHookName} Hook (at line ${
+              declaredDependenciesNode.loc.start.line
+            }) ` +
+            `change on every render.`;
+          if (suggestUseCallback) {
+            message +=
+              ` To fix this, ` +
+              `wrap the '${
+                fn.name.name
+              }' definition into its own useCallback() Hook.`;
+          } else {
+            message +=
+              ` To fix this, move the '${fn.name.name}' function ` +
+              `inside the ${reactiveHookName} callback. Alternatively, ` +
+              `wrap the '${
+                fn.name.name
+              }' definition into its own useCallback() Hook.`;
+          }
+          context.report({
+            node: fn.node,
+            message,
+            fix(fixer) {
+              // Only handle the simple case: arrow functions.
+              // Wrapping function declarations can mess up hoisting.
+              if (suggestUseCallback && fn.type === 'Variable') {
+                return [
+                  // TODO: also add an import?
+                  fixer.insertTextBefore(fn.node.init, 'useCallback('),
+                  // TODO: ideally we'd gather deps here but it would
+                  // require restructuring the rule code. For now,
+                  // this is fine. Note we're intentionally not adding
+                  // [] because that changes semantics.
+                  fixer.insertTextAfter(fn.node.init, ')'),
+                ];
+              }
+            },
+          });
+        });
         return;
       }
 
@@ -858,6 +910,83 @@ function collectRecommendations({
   };
 }
 
+// Finds functions declared as dependencies
+// that would invalidate on every render.
+function scanForDeclaredBareFunctions({
+  declaredDependencies,
+  declaredDependenciesNode,
+  componentScope,
+  scope,
+}) {
+  const bareFunctions = declaredDependencies
+    .map(({key}) => {
+      const fnRef = componentScope.set.get(key);
+      if (fnRef == null) {
+        return null;
+      }
+      let fnNode = fnRef.defs[0];
+      if (fnNode == null) {
+        return null;
+      }
+      // const handleChange = function () {}
+      // const handleChange = () => {}
+      if (
+        fnNode.type === 'Variable' &&
+        fnNode.node.type === 'VariableDeclarator' &&
+        fnNode.node.init != null &&
+        (fnNode.node.init.type === 'ArrowFunctionExpression' ||
+          fnNode.node.init.type === 'FunctionExpression')
+      ) {
+        return fnRef;
+      }
+      // function handleChange() {}
+      if (
+        fnNode.type === 'FunctionName' &&
+        fnNode.node.type === 'FunctionDeclaration'
+      ) {
+        return fnRef;
+      }
+      return null;
+    })
+    .filter(Boolean);
+
+  function isUsedOutsideOfHook(fnRef) {
+    let foundWriteExpr = false;
+    for (let i = 0; i < fnRef.references.length; i++) {
+      const reference = fnRef.references[i];
+      if (reference.writeExpr) {
+        if (foundWriteExpr) {
+          // Two writes to the same function.
+          return true;
+        } else {
+          // Ignore first write as it's not usage.
+          foundWriteExpr = true;
+          continue;
+        }
+      }
+      let currentScope = reference.from;
+      while (currentScope !== scope && currentScope != null) {
+        currentScope = currentScope.upper;
+      }
+      if (currentScope !== scope) {
+        // This reference is outside the Hook callback.
+        // It can only be legit if it's the deps array.
+        if (isAncestorNodeOf(declaredDependenciesNode, reference.identifier)) {
+          continue;
+        } else {
+          return true;
+        }
+      }
+    }
+    return false;
+  }
+
+  return bareFunctions.map(fnRef => ({
+    fn: fnRef.defs[0],
+    suggestUseCallback: isUsedOutsideOfHook(fnRef),
+  }));
+}
+
 /**
  * Assuming () means the passed/returned node:
  * (props) => (props)