From a8a27d74fef05fda99960866bbb63f02329f44ec Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Thu, 7 Mar 2019 00:58:36 +0000
Subject: [PATCH 1/6] A clearer message for props destructuring where
 applicable

---
 .../ESLintRuleExhaustiveDeps-test.js          | 20 ++++++++++++++-----
 .../src/ExhaustiveDeps.js                     |  5 +++--
 2 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
index a1c7932368507..067adb6148aff 100644
--- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -2447,7 +2447,9 @@ const tests = {
       errors: [
         "React Hook useEffect has a missing dependency: 'props'. " +
           'Either include it or remove the dependency array. ' +
-          'Alternatively, destructure the necessary props outside the callback.',
+          `However, the preferred fix is to destructure the 'props' ` +
+          `object outside of the useEffect call and refer to specific ` +
+          `props directly by their names.`,
       ],
     },
     {
@@ -2478,7 +2480,9 @@ const tests = {
       errors: [
         "React Hook useEffect has a missing dependency: 'props'. " +
           'Either include it or remove the dependency array. ' +
-          'Alternatively, destructure the necessary props outside the callback.',
+          `However, the preferred fix is to destructure the 'props' ` +
+          `object outside of the useEffect call and refer to specific ` +
+          `props directly by their names.`,
       ],
     },
     {
@@ -2529,7 +2533,9 @@ const tests = {
       errors: [
         "React Hook useEffect has a missing dependency: 'props'. " +
           'Either include it or remove the dependency array. ' +
-          'Alternatively, destructure the necessary props outside the callback.',
+          `However, the preferred fix is to destructure the 'props' ` +
+          `object outside of the useEffect call and refer to specific ` +
+          `props directly by their names.`,
       ],
     },
     {
@@ -2556,7 +2562,9 @@ const tests = {
       errors: [
         "React Hook useEffect has a missing dependency: 'props'. " +
           'Either include it or remove the dependency array. ' +
-          'Alternatively, destructure the necessary props outside the callback.',
+          `However, the preferred fix is to destructure the 'props' ` +
+          `object outside of the useEffect call and refer to specific ` +
+          `props directly by their names.`,
       ],
     },
     {
@@ -2583,7 +2591,9 @@ const tests = {
       errors: [
         "React Hook useEffect has missing dependencies: 'props' and 'skillsCount'. " +
           'Either include them or remove the dependency array. ' +
-          'Alternatively, destructure the necessary props outside the callback.',
+          `However, the preferred fix is to destructure the 'props' ` +
+          `object outside of the useEffect call and refer to specific ` +
+          `props directly by their names.`,
       ],
     },
     {
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
index 2f8b18c0069a7..f18bc8a8f1bb6 100644
--- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -724,8 +724,9 @@ export default {
         }
         if (isPropsOnlyUsedInMembers) {
           extraWarning =
-            ' Alternatively, destructure the necessary props ' +
-            'outside the callback.';
+            ` However, the preferred fix is to destructure the 'props' ` +
+            `object outside of the ${reactiveHookName} call and ` +
+            `refer to specific props directly by their names.`;
         }
       }
 

From 9045c2b5e201b4db140117da4fdf1988bb3a4b79 Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Thu, 7 Mar 2019 01:07:18 +0000
Subject: [PATCH 2/6] Add line number to the "move function" message

---
 .../__tests__/ESLintRuleExhaustiveDeps-test.js | 18 +++++++++---------
 .../src/ExhaustiveDeps.js                      |  6 ++++--
 2 files changed, 13 insertions(+), 11 deletions(-)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
index 067adb6148aff..20d9078d15089 100644
--- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -3349,7 +3349,7 @@ const tests = {
         `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, ` +
+          `inside the useEffect callback (at line 9). Alternatively, ` +
           `wrap the 'handleNext' definition into its own useCallback() Hook.`,
       ],
     },
@@ -3388,7 +3388,7 @@ const tests = {
         `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, ` +
+          `inside the useEffect callback (at line 9). Alternatively, ` +
           `wrap the 'handleNext' definition into its own useCallback() Hook.`,
       ],
     },
@@ -3486,15 +3486,15 @@ const tests = {
       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 ' +
+          'function inside the useEffect callback (at line 12). 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 ' +
+          'function inside the useLayoutEffect callback (at line 15). 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 ' +
+          'function inside the useMemo callback (at line 18). Alternatively, wrap the ' +
           "'handleNext3' definition into its own useCallback() Hook.",
       ],
     },
@@ -3554,15 +3554,15 @@ const tests = {
       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 ' +
+          'function inside the useEffect callback (at line 12). 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 ' +
+          'function inside the useLayoutEffect callback (at line 16). 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 ' +
+          'function inside the useMemo callback (at line 20). Alternatively, wrap the ' +
           "'handleNext3' definition into its own useCallback() Hook.",
       ],
     },
@@ -3747,7 +3747,7 @@ const tests = {
         `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 ` +
+          `the useEffect callback (at line 12). Alternatively, wrap the ` +
           `'handleNext' definition into its own useCallback() Hook.`,
       ],
     },
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
index f18bc8a8f1bb6..7b3ce83fc6afa 100644
--- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -588,8 +588,10 @@ export default {
           } else {
             message +=
               ` To fix this, move the '${fn.name.name}' function ` +
-              `inside the ${reactiveHookName} callback. Alternatively, ` +
-              `wrap the '${
+              `inside the ${reactiveHookName} callback (at line ${
+                node.loc.start.line
+              }). ` +
+              `Alternatively, wrap the '${
                 fn.name.name
               }' definition into its own useCallback() Hook.`;
           }

From bb16f21d275ac7396d07dc8213d7035494aff5b5 Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Thu, 7 Mar 2019 01:41:55 +0000
Subject: [PATCH 3/6] Add a hint for how to fix callbacks from props

---
 .../ESLintRuleExhaustiveDeps-test.js          | 48 +++++++++++++++++++
 .../src/ExhaustiveDeps.js                     | 42 ++++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
index 20d9078d15089..9d55cd14e8c75 100644
--- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -3859,6 +3859,54 @@ const tests = {
           `Either include it or remove the dependency array.`,
       ],
     },
+    {
+      code: `
+        function Podcasts({ fetchPodcasts, id }) {
+          let [podcasts, setPodcasts] = useState(null);
+          useEffect(() => {
+            fetchPodcasts(id).then(setPodcasts);
+          }, [id]);
+        }
+      `,
+      output: `
+        function Podcasts({ fetchPodcasts, id }) {
+          let [podcasts, setPodcasts] = useState(null);
+          useEffect(() => {
+            fetchPodcasts(id).then(setPodcasts);
+          }, [fetchPodcasts, id]);
+        }
+      `,
+      errors: [
+        `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` +
+          `Either include it or remove the dependency array. ` +
+          `If specifying 'fetchPodcasts' makes the dependencies change too often, ` +
+          `find the parent component that defines it and wrap that definition in useCallback.`,
+      ],
+    },
+    {
+      code: `
+        function Podcasts({ api: { fetchPodcasts }, id }) {
+          let [podcasts, setPodcasts] = useState(null);
+          useEffect(() => {
+            fetchPodcasts(id).then(setPodcasts);
+          }, [id]);
+        }
+      `,
+      output: `
+        function Podcasts({ api: { fetchPodcasts }, id }) {
+          let [podcasts, setPodcasts] = useState(null);
+          useEffect(() => {
+            fetchPodcasts(id).then(setPodcasts);
+          }, [fetchPodcasts, id]);
+        }
+      `,
+      errors: [
+        `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` +
+          `Either include it or remove the dependency array. ` +
+          `If specifying 'fetchPodcasts' makes the dependencies change too often, ` +
+          `find the parent component that defines it and wrap that definition in useCallback.`,
+      ],
+    },
   ],
 };
 
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
index 7b3ce83fc6afa..1b405d349a3a1 100644
--- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -732,6 +732,48 @@ export default {
         }
       }
 
+      if (!extraWarning && missingDependencies.size > 0) {
+        // See if the user is trying to avoid specifying a callable prop.
+        // This usually means they're unaware of useCallback.
+        let missingCallbackDep = null;
+        missingDependencies.forEach(missingDep => {
+          if (missingCallbackDep) {
+            return;
+          }
+          // Is this a variable from top scope?
+          const topScopeRef = componentScope.set.get(missingDep);
+          const usedDep = dependencies.get(missingDep);
+          if (usedDep.reference.resolved !== topScopeRef) {
+            return;
+          }
+          // Was it called?
+          const id = usedDep.reference.identifier;
+          if (
+            id == null ||
+            id.parent == null ||
+            id.parent.type !== 'CallExpression' ||
+            id.parent.callee !== id
+          ) {
+            return;
+          }
+          // Is this a destructured prop?
+          const def = topScopeRef.defs[0];
+          if (def == null || def.name == null || def.type !== 'Parameter') {
+            return;
+          }
+          if (isAncestorNodeOf(componentScope.block.params[0], def.name)) {
+            missingCallbackDep = missingDep;
+          }
+        });
+        if (missingCallbackDep !== null) {
+          extraWarning =
+            ` If specifying '${missingCallbackDep}'` +
+            ` makes the dependencies change too often, ` +
+            `find the parent component that defines it ` +
+            `and wrap that definition in useCallback.`;
+        }
+      }
+
       context.report({
         node: declaredDependenciesNode,
         message:

From 2cf8d49814b93f3a3a3b66b8c961a50da745d7c8 Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Thu, 7 Mar 2019 08:50:04 +0000
Subject: [PATCH 4/6] Simplify code and harden tests

---
 .../ESLintRuleExhaustiveDeps-test.js          | 98 +++++++++++++++++++
 .../src/ExhaustiveDeps.js                     | 11 +--
 2 files changed, 103 insertions(+), 6 deletions(-)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
index 9d55cd14e8c75..ebbb15b0f5414 100644
--- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -849,6 +849,31 @@ const tests = {
         }
       `,
     },
+    {
+      code: `
+        function withFetch(fetchPodcasts) {
+          return function Podcasts({ id }) {
+            let [podcasts, setPodcasts] = useState(null);
+            useEffect(() => {
+              fetchPodcasts(id).then(setPodcasts);
+            }, [id]);
+          }
+        }
+      `,
+    },
+    {
+      code: `
+        function Podcasts({ id }) {
+          let [podcasts, setPodcasts] = useState(null);
+          useEffect(() => {
+            function doFetch({ fetchPodcasts }) {
+              fetchPodcasts(id).then(setPodcasts);
+            }
+            doFetch({ fetchPodcasts: API.fetchPodcasts });
+          }, [id]);
+        }
+      `,
+    },
   ],
   invalid: [
     {
@@ -3710,6 +3735,47 @@ const tests = {
           "'handleNext2' definition into its own useCallback() Hook.",
       ],
     },
+    {
+      code: `
+        function MyComponent(props) {
+          let handleNext = () => {
+            console.log('hello');
+          };
+          if (props.foo) {
+            handleNext = () => {
+              console.log('hello');
+            };
+          }
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+        }
+      `,
+      // Normally we'd suggest moving handleNext inside an
+      // effect. But it's used more than once.
+      // TODO: our autofix here isn't quite sufficient because
+      // it only wraps the first definition. But seems ok.
+      output: `
+        function MyComponent(props) {
+          let handleNext = useCallback(() => {
+            console.log('hello');
+          });
+          if (props.foo) {
+            handleNext = () => {
+              console.log('hello');
+            };
+          }
+          useEffect(() => {
+            return Store.subscribe(handleNext);
+          }, [handleNext]);
+        }
+      `,
+      errors: [
+        "The 'handleNext' function makes the dependencies of useEffect Hook " +
+          '(at line 13) change on every render. To fix this, wrap the ' +
+          "'handleNext' definition into its own useCallback() Hook.",
+      ],
+    },
     {
       code: `
         function MyComponent(props) {
@@ -3907,6 +3973,38 @@ const tests = {
           `find the parent component that defines it and wrap that definition in useCallback.`,
       ],
     },
+    {
+      code: `
+        function Podcasts({ fetchPodcasts, fetchPodcasts2, id }) {
+          let [podcasts, setPodcasts] = useState(null);
+          useEffect(() => {
+            setTimeout(() => {
+              console.log(id);
+              fetchPodcasts(id).then(setPodcasts);
+              fetchPodcasts2(id).then(setPodcasts);
+            });
+          }, [id]);
+        }
+      `,
+      output: `
+        function Podcasts({ fetchPodcasts, fetchPodcasts2, id }) {
+          let [podcasts, setPodcasts] = useState(null);
+          useEffect(() => {
+            setTimeout(() => {
+              console.log(id);
+              fetchPodcasts(id).then(setPodcasts);
+              fetchPodcasts2(id).then(setPodcasts);
+            });
+          }, [fetchPodcasts, fetchPodcasts2, id]);
+        }
+      `,
+      errors: [
+        `React Hook useEffect has missing dependencies: 'fetchPodcasts' and 'fetchPodcasts2'. ` +
+          `Either include them or remove the dependency array. ` +
+          `If specifying 'fetchPodcasts' makes the dependencies change too often, ` +
+          `find the parent component that defines it and wrap that definition in useCallback.`,
+      ],
+    },
   ],
 };
 
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
index 1b405d349a3a1..5e83b408cef04 100644
--- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -761,9 +761,10 @@ export default {
           if (def == null || def.name == null || def.type !== 'Parameter') {
             return;
           }
-          if (isAncestorNodeOf(componentScope.block.params[0], def.name)) {
-            missingCallbackDep = missingDep;
-          }
+          // If it's missing (i.e. in component scope) *and* it's a parameter
+          // then it is definitely coming from props destructuring.
+          // (It could also be props itself but we wouldn't be calling it then.)
+          missingCallbackDep = missingDep;
         });
         if (missingCallbackDep !== null) {
           extraWarning =
@@ -1026,9 +1027,7 @@ function scanForDeclaredBareFunctions({
       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 {
+        if (!isAncestorNodeOf(declaredDependenciesNode, reference.identifier)) {
           return true;
         }
       }

From 530dcc046da5a9eb82288e35adfa38a40c971955 Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Thu, 7 Mar 2019 09:52:01 +0000
Subject: [PATCH 5/6] Collect all dependency references for better warnings

---
 .../ESLintRuleExhaustiveDeps-test.js          | 48 ++++++++++++++--
 .../src/ExhaustiveDeps.js                     | 55 ++++++++++++-------
 2 files changed, 79 insertions(+), 24 deletions(-)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
index ebbb15b0f5414..2a553effe5a75 100644
--- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -2673,11 +2673,15 @@ const tests = {
           let value;
           let value2;
           let value3;
+          let value4;
           let asyncValue;
           useEffect(() => {
-            value = {};
+            if (value4) {
+              value = {};
+            }
             value2 = 100;
             value = 43;
+            value4 = true;
             console.log(value2);
             console.log(value3);
             setTimeout(() => {
@@ -2694,11 +2698,15 @@ const tests = {
           let value;
           let value2;
           let value3;
+          let value4;
           let asyncValue;
           useEffect(() => {
-            value = {};
+            if (value4) {
+              value = {};
+            }
             value2 = 100;
             value = 43;
+            value4 = true;
             console.log(value2);
             console.log(value3);
             setTimeout(() => {
@@ -2708,14 +2716,20 @@ const tests = {
         }
       `,
       errors: [
+        // value2
+        `Assignments to the 'value2' variable from inside a React useEffect Hook ` +
+          `will not persist between re-renders. ` +
+          `If it's only needed by this Hook, move the variable inside it. ` +
+          `Alternatively, declare a ref with the useRef Hook, ` +
+          `and keep the mutable value in its 'current' property.`,
         // value
         `Assignments to the 'value' variable from inside a React useEffect Hook ` +
           `will not persist between re-renders. ` +
           `If it's only needed by this Hook, move the variable inside it. ` +
           `Alternatively, declare a ref with the useRef Hook, ` +
           `and keep the mutable value in its 'current' property.`,
-        // value2
-        `Assignments to the 'value2' variable from inside a React useEffect Hook ` +
+        // value4
+        `Assignments to the 'value4' variable from inside a React useEffect Hook ` +
           `will not persist between re-renders. ` +
           `If it's only needed by this Hook, move the variable inside it. ` +
           `Alternatively, declare a ref with the useRef Hook, ` +
@@ -4005,6 +4019,32 @@ const tests = {
           `find the parent component that defines it and wrap that definition in useCallback.`,
       ],
     },
+    {
+      code: `
+        function Podcasts({ fetchPodcasts, id }) {
+          let [podcasts, setPodcasts] = useState(null);
+          useEffect(() => {
+            console.log(fetchPodcasts);
+            fetchPodcasts(id).then(setPodcasts);
+          }, [id]);
+        }
+      `,
+      output: `
+        function Podcasts({ fetchPodcasts, id }) {
+          let [podcasts, setPodcasts] = useState(null);
+          useEffect(() => {
+            console.log(fetchPodcasts);
+            fetchPodcasts(id).then(setPodcasts);
+          }, [fetchPodcasts, id]);
+        }
+      `,
+      errors: [
+        `React Hook useEffect has a missing dependency: 'fetchPodcasts'. ` +
+          `Either include it or remove the dependency array. ` +
+          `If specifying 'fetchPodcasts' makes the dependencies change too often, ` +
+          `find the parent component that defines it and wrap that definition in useCallback.`,
+      ],
+    },
   ],
 };
 
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
index 5e83b408cef04..a56c299736813 100644
--- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -365,8 +365,10 @@ export default {
               memoizedIsFunctionWithoutCapturedValues(resolved);
             dependencies.set(dependency, {
               isStatic,
-              reference,
+              references: [reference],
             });
+          } else {
+            dependencies.get(dependency).references.push(reference);
           }
         }
         for (const childScope of currentScope.childScopes) {
@@ -514,9 +516,12 @@ export default {
 
       // Warn about assigning to variables in the outer scope.
       // Those are usually bugs.
-      let foundStaleAssignments = false;
+      let staleAssignments = new Set();
       function reportStaleAssignment(writeExpr, key) {
-        foundStaleAssignments = true;
+        if (staleAssignments.has(key)) {
+          return;
+        }
+        staleAssignments.add(key);
         context.report({
           node: writeExpr,
           message:
@@ -532,15 +537,17 @@ export default {
 
       // Remember which deps are optional and report bad usage first.
       const optionalDependencies = new Set();
-      dependencies.forEach(({isStatic, reference}, key) => {
+      dependencies.forEach(({isStatic, references}, key) => {
         if (isStatic) {
           optionalDependencies.add(key);
         }
-        if (reference.writeExpr) {
-          reportStaleAssignment(reference.writeExpr, key);
-        }
+        references.forEach(reference => {
+          if (reference.writeExpr) {
+            reportStaleAssignment(reference.writeExpr, key);
+          }
+        });
       });
-      if (foundStaleAssignments) {
+      if (staleAssignments.size > 0) {
         // The intent isn't clear so we'll wait until you fix those first.
         return;
       }
@@ -699,7 +706,7 @@ export default {
         if (propDep == null) {
           return;
         }
-        const refs = propDep.reference.resolved.references;
+        const refs = propDep.references;
         if (!Array.isArray(refs)) {
           return;
         }
@@ -743,17 +750,7 @@ export default {
           // Is this a variable from top scope?
           const topScopeRef = componentScope.set.get(missingDep);
           const usedDep = dependencies.get(missingDep);
-          if (usedDep.reference.resolved !== topScopeRef) {
-            return;
-          }
-          // Was it called?
-          const id = usedDep.reference.identifier;
-          if (
-            id == null ||
-            id.parent == null ||
-            id.parent.type !== 'CallExpression' ||
-            id.parent.callee !== id
-          ) {
+          if (usedDep.references[0].resolved !== topScopeRef) {
             return;
           }
           // Is this a destructured prop?
@@ -761,6 +758,24 @@ export default {
           if (def == null || def.name == null || def.type !== 'Parameter') {
             return;
           }
+          // Was it called in at least one case? Then it's a function.
+          let isFunctionCall = false;
+          let id;
+          for (let i = 0; i < usedDep.references.length; i++) {
+            id = usedDep.references[i].identifier;
+            if (
+              id != null &&
+              id.parent != null &&
+              id.parent.type === 'CallExpression' &&
+              id.parent.callee === id
+            ) {
+              isFunctionCall = true;
+              break;
+            }
+          }
+          if (!isFunctionCall) {
+            return;
+          }
           // If it's missing (i.e. in component scope) *and* it's a parameter
           // then it is definitely coming from props destructuring.
           // (It could also be props itself but we wouldn't be calling it then.)

From 80169c9c9537203b2e4e9c364485ddc786a74554 Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Thu, 7 Mar 2019 12:21:00 +0000
Subject: [PATCH 6/6] Suggest updater or reducer where appropriate

---
 .../ESLintRuleExhaustiveDeps-test.js          | 336 +++++++++++++++++-
 .../src/ExhaustiveDeps.js                     | 129 ++++++-
 2 files changed, 455 insertions(+), 10 deletions(-)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
index 2a553effe5a75..c30146cac3635 100644
--- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -874,6 +874,81 @@ const tests = {
         }
       `,
     },
+    {
+      code: `
+        function Counter() {
+          let [count, setCount] = useState(0);
+
+          function increment(x) {
+            return x + 1;
+          }
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, []);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+    },
+    {
+      code: `
+        function Counter() {
+          let [count, setCount] = useState(0);
+
+          function increment(x) {
+            return x + 1;
+          }
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => increment(count));
+            }, 1000);
+            return () => clearInterval(id);
+          }, []);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+    },
+    {
+      code: `
+        import increment from './increment';
+        function Counter() {
+          let [count, setCount] = useState(0);
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => count + increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, []);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+    },
+    {
+      code: `
+        function withStuff(increment) {
+          return function Counter() {
+            let [count, setCount] = useState(0);
+
+            useEffect(() => {
+              let id = setInterval(() => {
+                setCount(count => count + increment);
+              }, 1000);
+              return () => clearInterval(id);
+            }, []);
+
+            return <h1>{count}</h1>;
+          }
+        }
+      `,
+    },
   ],
   invalid: [
     {
@@ -2228,7 +2303,10 @@ const tests = {
       `,
       errors: [
         "React Hook useEffect has a missing dependency: 'state'. " +
-          'Either include it or remove the dependency array.',
+          'Either include it or remove the dependency array. ' +
+          `If 'state' is only necessary for calculating the next state, ` +
+          `consider refactoring to the setState(state => ...) form which ` +
+          `doesn't need to depend on the state from outside.`,
       ],
     },
     {
@@ -2257,7 +2335,10 @@ const tests = {
       `,
       errors: [
         "React Hook useEffect has a missing dependency: 'state'. " +
-          'Either include it or remove the dependency array.',
+          'Either include it or remove the dependency array. ' +
+          `If 'state' is only necessary for calculating the next state, ` +
+          `consider refactoring to the setState(state => ...) form which ` +
+          `doesn't need to depend on the state from outside.`,
       ],
     },
     {
@@ -3860,13 +3941,260 @@ const tests = {
           return <h1>{count}</h1>;
         }
       `,
-      // TODO: ideally this should suggest useState updater form
-      // since this code doesn't actually work.
       errors: [
         "React Hook useEffect has a missing dependency: 'count'. " +
+          'Either include it or remove the dependency array. ' +
+          `If 'count' is only necessary for calculating the next state, ` +
+          `consider refactoring to the setCount(count => ...) form which ` +
+          `doesn't need to depend on the state from outside.`,
+      ],
+    },
+    {
+      code: `
+        function Counter() {
+          let [count, setCount] = useState(0);
+          let [increment, setIncrement] = useState(0);
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count + increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, []);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      output: `
+        function Counter() {
+          let [count, setCount] = useState(0);
+          let [increment, setIncrement] = useState(0);
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count + increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, [count, increment]);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      errors: [
+        "React Hook useEffect has missing dependencies: 'count' and 'increment'. " +
+          'Either include them or remove the dependency array. ' +
+          `If 'count' is only necessary for calculating the next state, ` +
+          `consider refactoring to the setCount(count => ...) form which ` +
+          `doesn't need to depend on the state from outside.`,
+      ],
+    },
+    {
+      code: `
+        function Counter() {
+          let [count, setCount] = useState(0);
+          let [increment, setIncrement] = useState(0);
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => count + increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, []);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      output: `
+        function Counter() {
+          let [count, setCount] = useState(0);
+          let [increment, setIncrement] = useState(0);
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => count + increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, [increment]);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      errors: [
+        "React Hook useEffect has a missing dependency: 'increment'. " +
+          'Either include it or remove the dependency array. ' +
+          `If 'increment' is only necessary for calculating the next state, ` +
+          `consider refactoring to the useReducer Hook. This ` +
+          `lets you move the calculation of next state outside the effect.`,
+      ],
+    },
+    {
+      code: `
+        function Counter() {
+          let [count, setCount] = useState(0);
+          let increment = useCustomHook();
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => count + increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, []);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      output: `
+        function Counter() {
+          let [count, setCount] = useState(0);
+          let increment = useCustomHook();
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => count + increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, [increment]);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      // This intentionally doesn't show the reducer message
+      // because we don't know if it's safe for it to close over a value.
+      // We only show it for state variables (and possibly props).
+      errors: [
+        "React Hook useEffect has a missing dependency: 'increment'. " +
+          'Either include it or remove the dependency array.',
+      ],
+    },
+    {
+      code: `
+        function Counter({ step }) {
+          let [count, setCount] = useState(0);
+
+          function increment(x) {
+            return x + step;
+          }
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => increment(count));
+            }, 1000);
+            return () => clearInterval(id);
+          }, []);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      output: `
+        function Counter({ step }) {
+          let [count, setCount] = useState(0);
+
+          function increment(x) {
+            return x + step;
+          }
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => increment(count));
+            }, 1000);
+            return () => clearInterval(id);
+          }, [increment]);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      // This intentionally doesn't show the reducer message
+      // because we don't know if it's safe for it to close over a value.
+      // We only show it for state variables (and possibly props).
+      errors: [
+        "React Hook useEffect has a missing dependency: 'increment'. " +
           'Either include it or remove the dependency array.',
       ],
     },
+    {
+      code: `
+        function Counter({ step }) {
+          let [count, setCount] = useState(0);
+
+          function increment(x) {
+            return x + step;
+          }
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => increment(count));
+            }, 1000);
+            return () => clearInterval(id);
+          }, [increment]);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      output: `
+        function Counter({ step }) {
+          let [count, setCount] = useState(0);
+
+          function increment(x) {
+            return x + step;
+          }
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => increment(count));
+            }, 1000);
+            return () => clearInterval(id);
+          }, [increment]);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      errors: [
+        `The 'increment' function makes the dependencies of useEffect Hook ` +
+          `(at line 14) change on every render. To fix this, move the ` +
+          `'increment' function inside the useEffect callback (at line 9). ` +
+          `Alternatively, wrap the \'increment\' definition into its own ` +
+          `useCallback() Hook.`,
+      ],
+    },
+    {
+      code: `
+        function Counter({ increment }) {
+          let [count, setCount] = useState(0);
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => count + increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, []);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      output: `
+        function Counter({ increment }) {
+          let [count, setCount] = useState(0);
+
+          useEffect(() => {
+            let id = setInterval(() => {
+              setCount(count => count + increment);
+            }, 1000);
+            return () => clearInterval(id);
+          }, [increment]);
+
+          return <h1>{count}</h1>;
+        }
+      `,
+      errors: [
+        "React Hook useEffect has a missing dependency: 'increment'. " +
+          'Either include it or remove the dependency array. ' +
+          `If 'increment' is only necessary for calculating the next state, ` +
+          `consider refactoring to the useReducer Hook. This lets you move ` +
+          `the calculation of next state outside the effect. ` +
+          `You can then read 'increment' from the reducer ` +
+          `by putting it directly in your component.`,
+      ],
+    },
     {
       code: `
         function Counter() {
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
index a56c299736813..512f71db07500 100644
--- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -35,6 +35,8 @@ export default {
     const options = {additionalHooks};
 
     // Should be shared between visitors.
+    let setStateCallSites = new WeakMap();
+    let stateVariables = new WeakSet();
     let staticKnownValueCache = new WeakMap();
     let functionWithoutCapturedValueCache = new WeakMap();
     function memoizeWithWeakMap(fn, map) {
@@ -204,19 +206,40 @@ export default {
           return false;
         }
         const id = def.node.id;
-        if (callee.name === 'useRef' && id.type === 'Identifier') {
+        const {name} = callee;
+        if (name === 'useRef' && id.type === 'Identifier') {
           // useRef() return value is static.
           return true;
-        } else if (callee.name === 'useState' || callee.name === 'useReducer') {
+        } else if (name === 'useState' || name === 'useReducer') {
           // Only consider second value in initializing tuple static.
           if (
             id.type === 'ArrayPattern' &&
             id.elements.length === 2 &&
-            Array.isArray(resolved.identifiers) &&
-            // Is second tuple value the same reference we're checking?
-            id.elements[1] === resolved.identifiers[0]
+            Array.isArray(resolved.identifiers)
           ) {
-            return true;
+            // Is second tuple value the same reference we're checking?
+            if (id.elements[1] === resolved.identifiers[0]) {
+              if (name === 'useState') {
+                const references = resolved.references;
+                for (let i = 0; i < references.length; i++) {
+                  setStateCallSites.set(
+                    references[i].identifier,
+                    id.elements[0],
+                  );
+                }
+              }
+              // Setter is static.
+              return true;
+            } else if (id.elements[0] === resolved.identifiers[0]) {
+              if (name === 'useState') {
+                const references = resolved.references;
+                for (let i = 0; i < references.length; i++) {
+                  stateVariables.add(references[i].identifier);
+                }
+              }
+              // State variable itself is dynamic.
+              return false;
+            }
           }
         }
         // By default assume it's dynamic.
@@ -790,6 +813,100 @@ export default {
         }
       }
 
+      if (!extraWarning && missingDependencies.size > 0) {
+        let setStateRecommendation = null;
+        missingDependencies.forEach(missingDep => {
+          if (setStateRecommendation !== null) {
+            return;
+          }
+          const usedDep = dependencies.get(missingDep);
+          const references = usedDep.references;
+          let id;
+          let maybeCall;
+          for (let i = 0; i < references.length; i++) {
+            id = references[i].identifier;
+            maybeCall = id.parent;
+            // Try to see if we have setState(someExpr(missingDep)).
+            while (maybeCall != null && maybeCall !== componentScope.block) {
+              if (maybeCall.type === 'CallExpression') {
+                const correspondingStateVariable = setStateCallSites.get(
+                  maybeCall.callee,
+                );
+                if (correspondingStateVariable != null) {
+                  if (correspondingStateVariable.name === missingDep) {
+                    // setCount(count + 1)
+                    setStateRecommendation = {
+                      missingDep,
+                      setter: maybeCall.callee.name,
+                      form: 'updater',
+                    };
+                  } else if (stateVariables.has(id)) {
+                    // setCount(count + increment)
+                    setStateRecommendation = {
+                      missingDep,
+                      setter: maybeCall.callee.name,
+                      form: 'reducer',
+                    };
+                  } else {
+                    const resolved = references[i].resolved;
+                    if (resolved != null) {
+                      // If it's a parameter *and* a missing dep,
+                      // it must be a prop or something inside a prop.
+                      // Therefore, recommend an inline reducer.
+                      const def = resolved.defs[0];
+                      if (def != null && def.type === 'Parameter') {
+                        setStateRecommendation = {
+                          missingDep,
+                          setter: maybeCall.callee.name,
+                          form: 'inlineReducer',
+                        };
+                      }
+                    }
+                  }
+                  break;
+                }
+              }
+              maybeCall = maybeCall.parent;
+            }
+            if (setStateRecommendation !== null) {
+              break;
+            }
+          }
+        });
+        if (setStateRecommendation !== null) {
+          let suggestion;
+          switch (setStateRecommendation.form) {
+            case 'reducer':
+              suggestion =
+                'useReducer Hook. This lets you move the calculation ' +
+                'of next state outside the effect.';
+              break;
+            case 'inlineReducer':
+              suggestion =
+                'useReducer Hook. This lets you move the calculation ' +
+                'of next state outside the effect. You can then ' +
+                `read '${
+                  setStateRecommendation.missingDep
+                }' from the reducer ` +
+                `by putting it directly in your component.`;
+              break;
+            case 'updater':
+              suggestion =
+                `${setStateRecommendation.setter}(${
+                  setStateRecommendation.missingDep
+                } => ...) form ` +
+                `which doesn't need to depend on the state from outside.`;
+              break;
+            default:
+              throw new Error('Unknown case.');
+          }
+          extraWarning =
+            ` If '${setStateRecommendation.missingDep}'` +
+            ` is only necessary for calculating the next state, ` +
+            `consider refactoring to the ${suggestion}`;
+        }
+      }
+
       context.report({
         node: declaredDependenciesNode,
         message: