From df7b4768c7931504338ee1093ebcfda3d5974650 Mon Sep 17 00:00:00 2001
From: Dan Abramov <dan.abramov@gmail.com>
Date: Fri, 1 Mar 2019 16:10:22 +0000
Subject: [PATCH] [ESLint] Deduplicate suggested dependencies (#14982)

* Deduplicate suggested dependencies

* Tweak test cases
---
 .../ESLintRuleExhaustiveDeps-test.js          | 783 ++++++++++++------
 .../src/ExhaustiveDeps.js                     | 298 +++++--
 2 files changed, 765 insertions(+), 316 deletions(-)

diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
index 75bcfd0778d95..947a97c458dd9 100644
--- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
+++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js
@@ -33,33 +33,33 @@ const tests = {
   valid: [
     {
       code: `
-      function MyComponent() {
-        const local = {};
-        useEffect(() => {
-          console.log(local);
-        });
-      }
-    `,
+        function MyComponent() {
+          const local = {};
+          useEffect(() => {
+            console.log(local);
+          });
+        }
+      `,
     },
     {
       code: `
-      function MyComponent() {
-        useEffect(() => {
-          const local = {};
-          console.log(local);
-        }, []);
-      }
-    `,
+        function MyComponent() {
+          useEffect(() => {
+            const local = {};
+            console.log(local);
+          }, []);
+        }
+      `,
     },
     {
       code: `
-      function MyComponent() {
-        const local = {};
-        useEffect(() => {
-          console.log(local);
-        }, [local]);
-      }
-    `,
+        function MyComponent() {
+          const local = {};
+          useEffect(() => {
+            console.log(local);
+          }, [local]);
+        }
+      `,
     },
     {
       // OK because `props` wasn't defined.
@@ -68,164 +68,161 @@ const tests = {
       // a component-level variable. Ignore it until it
       //  gets defined (a different rule would flag it anyway).
       code: `
-      function MyComponent() {
-        useEffect(() => {
-          console.log(props.foo);
-        }, []);
-      }
-    `,
+        function MyComponent() {
+          useEffect(() => {
+            console.log(props.foo);
+          }, []);
+        }
+      `,
     },
     {
       code: `
-      function MyComponent() {
-        const local1 = {};
-        {
-          const local2 = {};
-          useEffect(() => {
-            console.log(local1);
-            console.log(local2);
-          });
+        function MyComponent() {
+          const local1 = {};
+          {
+            const local2 = {};
+            useEffect(() => {
+              console.log(local1);
+              console.log(local2);
+            });
+          }
         }
-      }
-    `,
+      `,
     },
     {
       code: `
-      function MyComponent() {
-        const local1 = {};
-        {
-          const local2 = {};
-          useCallback(() => {
-            console.log(local1);
-            console.log(local2);
-          }, [local1, local2]);
+        function MyComponent() {
+          const local1 = {};
+          {
+            const local2 = {};
+            useCallback(() => {
+              console.log(local1);
+              console.log(local2);
+            }, [local1, local2]);
+          }
         }
-      }
-    `,
+      `,
     },
     {
       code: `
-      function MyComponent() {
-        const local1 = {};
-        function MyNestedComponent() {
-          const local2 = {};
-          useCallback(() => {
-            console.log(local1);
-            console.log(local2);
-          }, [local2]);
+        function MyComponent() {
+          const local1 = {};
+          function MyNestedComponent() {
+            const local2 = {};
+            useCallback(() => {
+              console.log(local1);
+              console.log(local2);
+            }, [local2]);
+          }
         }
-      }
-    `,
+      `,
     },
     {
       code: `
-      function MyComponent() {
-        const local = {};
-        useEffect(() => {
-          console.log(local);
-          console.log(local);
-        }, [local]);
-      }
-    `,
+        function MyComponent() {
+          const local = {};
+          useEffect(() => {
+            console.log(local);
+            console.log(local);
+          }, [local]);
+        }
+      `,
     },
     {
       code: `
-      function MyComponent() {
-        useEffect(() => {
-          console.log(unresolved);
-        }, []);
-      }
-    `,
+        function MyComponent() {
+          useEffect(() => {
+            console.log(unresolved);
+          }, []);
+        }
+      `,
     },
     {
       code: `
-      function MyComponent() {
-        const local = {};
-        useEffect(() => {
-          console.log(local);
-        }, [,,,local,,,]);
-      }
-    `,
+        function MyComponent() {
+          const local = {};
+          useEffect(() => {
+            console.log(local);
+          }, [,,,local,,,]);
+        }
+      `,
     },
     {
-      code: `
       // Regression test
-      function MyComponent({ foo }) {
-        useEffect(() => {
-          console.log(foo.length);
-        }, [foo]);
-      }
-    `,
+      code: `
+        function MyComponent({ foo }) {
+          useEffect(() => {
+            console.log(foo.length);
+          }, [foo]);
+        }
+      `,
     },
     {
-      code: `
       // Regression test
-      function MyComponent({ foo }) {
-        useEffect(() => {
-          console.log(foo.length);
-          console.log(foo.slice(0));
-        }, [foo]);
-      }
-    `,
+      code: `
+        function MyComponent({ foo }) {
+          useEffect(() => {
+            console.log(foo.length);
+            console.log(foo.slice(0));
+          }, [foo]);
+        }
+      `,
     },
     {
-      code: `
       // Regression test
-      function MyComponent({ history }) {
-        useEffect(() => {
-          return history.listen();
-        }, [history]);
-      }
-    `,
+      code: `
+        function MyComponent({ history }) {
+          useEffect(() => {
+            return history.listen();
+          }, [history]);
+        }
+      `,
     },
     {
-      // TODO: we might want to forbid dot-access in deps.
       code: `
-      function MyComponent(props) {
-        useEffect(() => {
-          console.log(props.foo);
-        }, [props.foo]);
-      }
-    `,
+        function MyComponent(props) {
+          useEffect(() => {
+            console.log(props.foo);
+          }, [props.foo]);
+        }
+      `,
     },
     {
-      // TODO: we might want to forbid dot-access in deps.
       code: `
-      function MyComponent(props) {
-        useEffect(() => {
-          console.log(props.foo);
-          console.log(props.bar);
-        }, [props.bar, props.foo]);
-      }
-    `,
+        function MyComponent(props) {
+          useEffect(() => {
+            console.log(props.foo);
+            console.log(props.bar);
+          }, [props.bar, props.foo]);
+        }
+      `,
     },
     {
-      // TODO: we might want to forbid dot-access in deps.
       code: `
-      function MyComponent(props) {
-        useEffect(() => {
-          console.log(props.foo);
-          console.log(props.bar);
-        }, [props.foo, props.bar]);
-      }
-    `,
+        function MyComponent(props) {
+          useEffect(() => {
+            console.log(props.foo);
+            console.log(props.bar);
+          }, [props.foo, props.bar]);
+        }
+      `,
     },
     {
-      // TODO: we might want to forbid dot-access in deps.
       code: `
-      function MyComponent(props) {
-        const local = {};
-        useEffect(() => {
-          console.log(props.foo);
-          console.log(props.bar);
-          console.log(local);
-        }, [props.foo, props.bar, local]);
-      }
-    `,
+        function MyComponent(props) {
+          const local = {};
+          useEffect(() => {
+            console.log(props.foo);
+            console.log(props.bar);
+            console.log(local);
+          }, [props.foo, props.bar, local]);
+        }
+      `,
     },
     {
-      // TODO: we might want to warn "props.foo"
-      // is extraneous because we already have "props".
+      // [props, props.foo] is technically unnecessary ('props' covers 'props.foo').
+      // However, it's valid for effects to over-specify their deps.
+      // So we don't warn about this. We *would* warn about useMemo/useCallback.
       code: `
         function MyComponent(props) {
           const local = {};
@@ -233,6 +230,12 @@ const tests = {
             console.log(props.foo);
             console.log(props.bar);
           }, [props, props.foo]);
+
+          let color = {}
+          useEffect(() => {
+            console.log(props.foo.bar.baz);
+            console.log(color);
+          }, [props.foo, props.foo.bar.baz, color]);
         }
       `,
     },
@@ -247,7 +250,6 @@ const tests = {
       options: [{additionalHooks: 'useCustomEffect'}],
     },
     {
-      // TODO: we might want to forbid dot-access in deps.
       code: `
         function MyComponent(props) {
           useCustomEffect(() => {
@@ -258,46 +260,46 @@ const tests = {
       options: [{additionalHooks: 'useCustomEffect'}],
     },
     {
-      code: `
       // Valid because we don't care about hooks outside of components.
-      const local = {};
-      useEffect(() => {
-        console.log(local);
-      }, []);
-    `,
-    },
-    {
       code: `
-      // Valid because we don't care about hooks outside of components.
-      const local1 = {};
-      {
-        const local2 = {};
+        const local = {};
         useEffect(() => {
-          console.log(local1);
-          console.log(local2);
+          console.log(local);
         }, []);
-      }
-    `,
+      `,
     },
     {
+      // Valid because we don't care about hooks outside of components.
       code: `
-      function MyComponent() {
-        const ref = useRef();
-        useEffect(() => {
-          console.log(ref.current);
-        }, [ref]);
-      }
-    `,
+        const local1 = {};
+        {
+          const local2 = {};
+          useEffect(() => {
+            console.log(local1);
+            console.log(local2);
+          }, []);
+        }
+      `,
     },
     {
       code: `
-      function MyComponent() {
-        const ref = useRef();
-        useEffect(() => {
-          console.log(ref.current);
-        }, []);
-      }
-    `,
+        function MyComponent() {
+          const ref = useRef();
+          useEffect(() => {
+            console.log(ref.current);
+          }, [ref]);
+        }
+      `,
+    },
+    {
+      code: `
+        function MyComponent() {
+          const ref = useRef();
+          useEffect(() => {
+            console.log(ref.current);
+          }, []);
+        }
+      `,
     },
     {
       code: `
@@ -561,38 +563,56 @@ const tests = {
       `,
     },
     {
-      // Valid because it's a primitive constant
+      // Valid because it's a primitive constant.
       code: `
-      function MyComponent() {
-        const local1 = 42;
-        const local2 = '42';
-        const local3 = null;
-        useEffect(() => {
-          console.log(local1);
-          console.log(local2);
-          console.log(local3);
-        }, []);
-      }
-    `,
+        function MyComponent() {
+          const local1 = 42;
+          const local2 = '42';
+          const local3 = null;
+          useEffect(() => {
+            console.log(local1);
+            console.log(local2);
+            console.log(local3);
+          }, []);
+        }
+      `,
     },
     {
       // It's not a mistake to specify constant values though.
       code: `
-      function MyComponent() {
-        const local1 = 42;
-        const local2 = '42';
-        const local3 = null;
-        useEffect(() => {
-          console.log(local1);
-          console.log(local2);
-          console.log(local3);
-        }, [local1, local2, local3]);
-      }
-    `,
+        function MyComponent() {
+          const local1 = 42;
+          const local2 = '42';
+          const local3 = null;
+          useEffect(() => {
+            console.log(local1);
+            console.log(local2);
+            console.log(local3);
+          }, [local1, local2, local3]);
+        }
+      `,
+    },
+    {
+      // It is valid for effects to over-specify their deps.
+      // TODO: maybe only allow local ones, and disallow this example.
+      code: `
+        function MyComponent() {
+          useEffect(() => {}, [window]);
+        }
+      `,
+    },
+    {
+      // It is valid for effects to over-specify their deps.
+      code: `
+        function MyComponent(props) {
+          const local = props.local;
+          useEffect(() => {}, [local]);
+        }
+      `,
     },
     {
       // Valid even though activeTab is "unused".
-      // We allow that in effects, but not callbacks or memo.
+      // We allow over-specifying deps for effects, but not callbacks or memo.
       code: `
         function Foo({ activeTab }) {
           useEffect(() => {
@@ -601,6 +621,46 @@ const tests = {
         }
       `,
     },
+    {
+      // It is valid to specify broader effect deps than strictly necessary.
+      // Don't warn for this.
+      code: `
+        function MyComponent(props) {
+          useEffect(() => {
+            console.log(props.foo.bar.baz);
+          }, [props]);
+          useEffect(() => {
+            console.log(props.foo.bar.baz);
+          }, [props.foo]);
+          useEffect(() => {
+            console.log(props.foo.bar.baz);
+          }, [props.foo.bar]);
+          useEffect(() => {
+            console.log(props.foo.bar.baz);
+          }, [props.foo.bar.baz]);
+        }
+      `,
+    },
+    {
+      // It is *also* valid to specify broader memo/callback deps than strictly necessary.
+      // Don't warn for this either.
+      code: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props.foo.bar.baz);
+          }, [props]);
+          const fn2 = useCallback(() => {
+            console.log(props.foo.bar.baz);
+          }, [props.foo]);
+          const fn3 = useMemo(() => {
+            console.log(props.foo.bar.baz);
+          }, [props.foo.bar]);
+          const fn4 = useMemo(() => {
+            console.log(props.foo.bar.baz);
+          }, [props.foo.bar.baz]);
+        }
+      `,
+    },
   ],
   invalid: [
     {
@@ -674,8 +734,8 @@ const tests = {
       ],
     },
     {
+      // Regression test
       code: `
-        // Regression test
         function MyComponent() {
           const local = {};
           useEffect(() => {
@@ -686,7 +746,6 @@ const tests = {
         }
       `,
       output: `
-        // Regression test
         function MyComponent() {
           const local = {};
           useEffect(() => {
@@ -702,8 +761,8 @@ const tests = {
       ],
     },
     {
+      // Regression test
       code: `
-        // Regression test
         function MyComponent() {
           const local = {};
           useEffect(() => {
@@ -714,7 +773,6 @@ const tests = {
         }
       `,
       output: `
-        // Regression test
         function MyComponent() {
           const local = {};
           useEffect(() => {
@@ -730,8 +788,8 @@ const tests = {
       ],
     },
     {
+      // Regression test
       code: `
-        // Regression test
         function MyComponent() {
           const local = {};
           useEffect(() => {
@@ -743,7 +801,6 @@ const tests = {
         }
       `,
       output: `
-        // Regression test
         function MyComponent() {
           const local = {};
           useEffect(() => {
@@ -923,7 +980,7 @@ const tests = {
     {
       code: `
         function MyComponent() {
-          useCallback(() => {}, [local]);
+          useCallback(() => {}, [window]);
         }
       `,
       output: `
@@ -931,6 +988,26 @@ const tests = {
           useCallback(() => {}, []);
         }
       `,
+      errors: [
+        "React Hook useCallback has an unnecessary dependency: 'window'. " +
+          'Either exclude it or remove the dependency array.',
+      ],
+    },
+    {
+      // It is not valid for useCallback to specify extraneous deps
+      // because it doesn't serve as a side effect trigger unlike useEffect.
+      code: `
+        function MyComponent(props) {
+          let local = props.foo;
+          useCallback(() => {}, [local]);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          let local = props.foo;
+          useCallback(() => {}, []);
+        }
+      `,
       errors: [
         "React Hook useCallback has an unnecessary dependency: 'local'. " +
           'Either exclude it or remove the dependency array.',
@@ -938,18 +1015,18 @@ const tests = {
     },
     {
       code: `
-      function MyComponent({ history }) {
-        useEffect(() => {
-          return history.listen();
-        }, []);
-      }
+        function MyComponent({ history }) {
+          useEffect(() => {
+            return history.listen();
+          }, []);
+        }
       `,
       output: `
-      function MyComponent({ history }) {
-        useEffect(() => {
-          return history.listen();
-        }, [history]);
-      }
+        function MyComponent({ history }) {
+          useEffect(() => {
+            return history.listen();
+          }, [history]);
+        }
       `,
       errors: [
         "React Hook useEffect has a missing dependency: 'history'. " +
@@ -958,24 +1035,24 @@ const tests = {
     },
     {
       code: `
-      function MyComponent({ history }) {
-        useEffect(() => {
-          return [
-            history.foo.bar[2].dobedo.listen(),
-            history.foo.bar().dobedo.listen[2]
-          ];
-        }, []);
-      }
+        function MyComponent({ history }) {
+          useEffect(() => {
+            return [
+              history.foo.bar[2].dobedo.listen(),
+              history.foo.bar().dobedo.listen[2]
+            ];
+          }, []);
+        }
       `,
       output: `
-      function MyComponent({ history }) {
-        useEffect(() => {
-          return [
-            history.foo.bar[2].dobedo.listen(),
-            history.foo.bar().dobedo.listen[2]
-          ];
-        }, [history.foo]);
-      }
+        function MyComponent({ history }) {
+          useEffect(() => {
+            return [
+              history.foo.bar[2].dobedo.listen(),
+              history.foo.bar().dobedo.listen[2]
+            ];
+          }, [history.foo]);
+        }
       `,
       errors: [
         "React Hook useEffect has a missing dependency: 'history.foo'. " +
@@ -1240,6 +1317,62 @@ const tests = {
       ],
     },
     {
+      // It is not valid for useCallback to specify extraneous deps
+      // because it doesn't serve as a side effect trigger unlike useEffect.
+      // However, we generally allow specifying *broader* deps as escape hatch.
+      // So while [props, props.foo] is unnecessary, 'props' wins here as the
+      // broader one, and this is why 'props.foo' is reported as unnecessary.
+      code: `
+        function MyComponent(props) {
+          const local = {};
+          useCallback(() => {
+            console.log(props.foo);
+            console.log(props.bar);
+          }, [props, props.foo]);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          const local = {};
+          useCallback(() => {
+            console.log(props.foo);
+            console.log(props.bar);
+          }, [props]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has an unnecessary dependency: 'props.foo'. " +
+          'Either exclude it or remove the dependency array.',
+      ],
+    },
+    {
+      // Since we don't have 'props' in the list, we'll suggest narrow dependencies.
+      code: `
+        function MyComponent(props) {
+          const local = {};
+          useCallback(() => {
+            console.log(props.foo);
+            console.log(props.bar);
+          }, []);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          const local = {};
+          useCallback(() => {
+            console.log(props.foo);
+            console.log(props.bar);
+          }, [props.bar, props.foo]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has missing dependencies: 'props.bar' and 'props.foo'. " +
+          'Either include them or remove the dependency array.',
+      ],
+    },
+    {
+      // Effects are allowed to over-specify deps. We'll complain about missing
+      // 'local', but we won't remove the already-specified 'local.id' from your list.
       code: `
         function MyComponent() {
           const local = {id: 42};
@@ -1248,8 +1381,6 @@ const tests = {
           }, [local.id]);
         }
       `,
-      // TODO: autofix should be smart enough
-      // to remove local.id from the list.
       output: `
         function MyComponent() {
           const local = {id: 42};
@@ -1263,6 +1394,193 @@ const tests = {
           'Either include it or remove the dependency array.',
       ],
     },
+    {
+      // Callbacks are not allowed to over-specify deps. So we'll complain about missing
+      // 'local' and we will also *remove* 'local.id' from your list.
+      code: `
+        function MyComponent() {
+          const local = {id: 42};
+          const fn = useCallback(() => {
+            console.log(local);
+          }, [local.id]);
+        }
+      `,
+      output: `
+        function MyComponent() {
+          const local = {id: 42};
+          const fn = useCallback(() => {
+            console.log(local);
+          }, [local]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has a missing dependency: 'local'. " +
+          'Either include it or remove the dependency array.',
+      ],
+    },
+    {
+      // Callbacks are not allowed to over-specify deps. So we'll complain about
+      // the unnecessary 'local.id'.
+      code: `
+        function MyComponent() {
+          const local = {id: 42};
+          const fn = useCallback(() => {
+            console.log(local);
+          }, [local.id, local]);
+        }
+      `,
+      output: `
+        function MyComponent() {
+          const local = {id: 42};
+          const fn = useCallback(() => {
+            console.log(local);
+          }, [local]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has an unnecessary dependency: 'local.id'. " +
+          'Either exclude it or remove the dependency array.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props.foo.bar.baz);
+          }, []);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props.foo.bar.baz);
+          }, [props.foo.bar.baz]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has a missing dependency: 'props.foo.bar.baz'. " +
+          'Either include it or remove the dependency array.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          let color = {}
+          const fn = useCallback(() => {
+            console.log(props.foo.bar.baz);
+            console.log(color);
+          }, [props.foo, props.foo.bar.baz]);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          let color = {}
+          const fn = useCallback(() => {
+            console.log(props.foo.bar.baz);
+            console.log(color);
+          }, [color, props.foo.bar.baz]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has a missing dependency: 'color'. " +
+          'Either include it or remove the dependency array.',
+      ],
+    },
+    {
+      // Callbacks are not allowed to over-specify deps. So one of these is extra.
+      // However, it *is* allowed to specify broader deps then strictly necessary.
+      // So in this case we ask you to remove 'props.foo.bar.baz' because 'props.foo'
+      // already covers it, and having both is unnecessary.
+      // TODO: maybe consider suggesting a narrower one by default in these cases.
+      code: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props.foo.bar.baz);
+          }, [props.foo.bar.baz, props.foo]);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props.foo.bar.baz);
+          }, [props.foo]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has an unnecessary dependency: 'props.foo.bar.baz'. " +
+          'Either exclude it or remove the dependency array.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props.foo.bar.baz);
+            console.log(props.foo.fizz.bizz);
+          }, []);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props.foo.bar.baz);
+            console.log(props.foo.fizz.bizz);
+          }, [props.foo.bar.baz, props.foo.fizz.bizz]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has missing dependencies: 'props.foo.bar.baz' and 'props.foo.fizz.bizz'. " +
+          'Either include them or remove the dependency array.',
+      ],
+    },
+    {
+      // Normally we allow specifying deps too broadly.
+      // So we'd be okay if 'props.foo.bar' was there rather than 'props.foo.bar.baz'.
+      // However, 'props.foo.bar.baz' is missing. So we know there is a mistake.
+      // When we're sure there is a mistake, for callbacks we will rebuild the list
+      // from scratch. This will set the user on a better path by default.
+      // This is why we end up with just 'props.foo.bar', and not them both.
+      code: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props.foo.bar);
+          }, [props.foo.bar.baz]);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props.foo.bar);
+          }, [props.foo.bar]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has a missing dependency: 'props.foo.bar'. " +
+          'Either include it or remove the dependency array.',
+      ],
+    },
+    {
+      code: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props);
+            console.log(props.hello);
+          }, [props.foo.bar.baz]);
+        }
+      `,
+      output: `
+        function MyComponent(props) {
+          const fn = useCallback(() => {
+            console.log(props);
+            console.log(props.hello);
+          }, [props]);
+        }
+      `,
+      errors: [
+        "React Hook useCallback has a missing dependency: 'props'. " +
+          'Either include it or remove the dependency array.',
+      ],
+    },
     {
       code: `
         function MyComponent() {
@@ -1335,8 +1653,6 @@ const tests = {
           }, []);
         }
       `,
-      // TODO: we need to think about ideal output here.
-      // Should it capture by default?
       output: `
         function MyComponent(props) {
           useEffect(() => {
@@ -1358,8 +1674,6 @@ const tests = {
           }, []);
         }
       `,
-      // TODO: we need to think about ideal output here.
-      // Should it capture by default?
       output: `
         function MyComponent(props) {
           useEffect(() => {
@@ -1453,8 +1767,6 @@ const tests = {
           }, []);
         }
       `,
-      // TODO: we need to think about ideal output here.
-      // Should it capture by default?
       output: `
         function MyComponent(props) {
           const local = {};
@@ -1910,20 +2222,19 @@ const tests = {
           }, []);
         }
       `,
-      // TODO: [props.onChange] is superfluous. Fix to just [props.onChange].
       output: `
         function MyComponent(props) {
           useEffect(() => {
             if (props.onChange) {
               props.onChange();
             }
-          }, [props, props.onChange]);
+          }, [props]);
         }
       `,
       errors: [
-        // TODO: reporting props separately is superfluous. Fix to just props.onChange.
-        "React Hook useEffect has missing dependencies: 'props' and 'props.onChange'. " +
-          'Either include them or remove the dependency array.',
+        // TODO: make this message clearer since it's not obvious why.
+        "React Hook useEffect has a missing dependency: 'props'. " +
+          'Either include it or remove the dependency array.',
       ],
     },
     {
diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
index 5d5cabc2e7ea9..45539d875c069 100644
--- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
+++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js
@@ -306,77 +306,75 @@ export default {
         });
       }
 
-      // TODO: we can do a pass at this code and pick more appropriate
-      // data structures to avoid nested loops if we can.
-      let suggestedDependencies = [];
-      let duplicateDependencies = new Set();
-      let unnecessaryDependencies = new Set();
-      let missingDependencies = new Set();
-      let actualDependencies = Array.from(dependencies.keys());
+      // Warn about assigning to variables in the outer scope.
+      // Those are usually bugs.
       let foundStaleAssignments = false;
-
-      function satisfies(actualDep, dep) {
-        return actualDep === dep || actualDep.startsWith(dep + '.');
+      function reportStaleAssignment(writeExpr, key) {
+        foundStaleAssignments = true;
+        context.report({
+          node: writeExpr,
+          message:
+            `Assignments to the '${key}' variable from inside a React ${context.getSource(
+              reactiveHook,
+            )} 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.`,
+        });
       }
 
-      // First, ensure what user specified makes sense.
-      declaredDependencies.forEach(({key}) => {
-        if (actualDependencies.some(actualDep => satisfies(actualDep, key))) {
-          // Legit dependency.
-          if (suggestedDependencies.indexOf(key) === -1) {
-            suggestedDependencies.push(key);
-          } else {
-            // Duplicate. Do nothing.
-            duplicateDependencies.add(key);
-          }
-        } else {
-          if (isEffect && !key.endsWith('.current')) {
-            // Effects may have extra "unnecessary" deps.
-            // Such as resetting scroll when ID changes.
-            // The exception is ref.current which is always wrong.
-            // Consider them legit.
-            if (suggestedDependencies.indexOf(key) === -1) {
-              suggestedDependencies.push(key);
-            }
-          } else {
-            // Unnecessary dependency. Remember to report it.
-            unnecessaryDependencies.add(key);
-          }
-        }
-      });
-
-      // Then fill in the missing ones.
+      // Remember which deps are optional and report bad usage first.
+      const optionalDependencies = new Set();
       dependencies.forEach(({isStatic, reference}, key) => {
+        if (isStatic) {
+          optionalDependencies.add(key);
+        }
         if (reference.writeExpr) {
-          foundStaleAssignments = true;
-          context.report({
-            node: reference.writeExpr,
-            message:
-              `Assignments to the '${key}' variable from inside a React ${context.getSource(
-                reactiveHook,
-              )} 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.`,
-          });
+          reportStaleAssignment(reference.writeExpr, key);
         }
+      });
+      if (foundStaleAssignments) {
+        // The intent isn't clear so we'll wait until you fix those first.
+        return;
+      }
 
-        if (
-          !suggestedDependencies.some(suggestedDep =>
-            satisfies(key, suggestedDep),
-          )
-        ) {
-          if (!isStatic) {
-            // Legit missing.
-            suggestedDependencies.push(key);
-            missingDependencies.add(key);
-          }
-        } else {
-          // Already did that. Do nothing.
-        }
+      let {
+        suggestedDependencies,
+        unnecessaryDependencies,
+        missingDependencies,
+        duplicateDependencies,
+      } = collectRecommendations({
+        dependencies,
+        declaredDependencies,
+        optionalDependencies,
+        isEffect,
       });
 
+      const problemCount =
+        duplicateDependencies.size +
+        missingDependencies.size +
+        unnecessaryDependencies.size;
+      if (problemCount === 0) {
+        return;
+      }
+
+      // If we're going to report a missing dependency,
+      // we might as well recalculate the list ignoring
+      // the currently specified deps. This can result
+      // in some extra deduplication. We can't do this
+      // for effects though because those have legit
+      // use cases for over-specifying deps.
+      if (!isEffect && missingDependencies.size > 0) {
+        suggestedDependencies = collectRecommendations({
+          dependencies,
+          declaredDependencies: [], // Pretend we don't know
+          optionalDependencies,
+          isEffect,
+        }).suggestedDependencies;
+      }
+
+      // Alphabetize the suggestions, but only if deps were already alphabetized.
       function areDeclaredDepsAlphabetized() {
         if (declaredDependencies.length === 0) {
           return true;
@@ -385,26 +383,10 @@ export default {
         const sortedDeclaredDepKeys = declaredDepKeys.slice().sort();
         return declaredDepKeys.join(',') === sortedDeclaredDepKeys.join(',');
       }
-
       if (areDeclaredDepsAlphabetized()) {
-        // Alphabetize the autofix, but only if deps were already alphabetized.
         suggestedDependencies.sort();
       }
 
-      const problemCount =
-        duplicateDependencies.size +
-        missingDependencies.size +
-        unnecessaryDependencies.size;
-
-      if (problemCount === 0) {
-        return;
-      }
-
-      if (foundStaleAssignments) {
-        // The intent isn't clear so we'll wait until you fix those first.
-        return;
-      }
-
       function getWarningMessage(deps, singlePrefix, label, fixVerb) {
         if (deps.size === 0) {
           return null;
@@ -475,11 +457,167 @@ export default {
   },
 };
 
+// The meat of the logic.
+function collectRecommendations({
+  dependencies,
+  declaredDependencies,
+  optionalDependencies,
+  isEffect,
+}) {
+  // Our primary data structure.
+  // It is a logical representation of property chains:
+  // `props` -> `props.foo` -> `props.foo.bar` -> `props.foo.bar.baz`
+  //         -> `props.lol`
+  //         -> `props.huh` -> `props.huh.okay`
+  //         -> `props.wow`
+  // We'll use it to mark nodes that are *used* by the programmer,
+  // and the nodes that were *declared* as deps. Then we will
+  // traverse it to learn which deps are missing or unnecessary.
+  const depTree = createDepTree();
+  function createDepTree() {
+    return {
+      isRequired: false, // True if used in code
+      isSatisfiedRecursively: false, // True if specified in deps
+      hasRequiredNodesBelow: false, // True if something deeper is used by code
+      children: new Map(), // Nodes for properties
+    };
+  }
+
+  // Mark all required nodes first.
+  // Imagine exclamation marks next to each used deep property.
+  dependencies.forEach((_, key) => {
+    const node = getOrCreateNodeByPath(depTree, key);
+    node.isRequired = true;
+    markAllParentsByPath(depTree, key, parent => {
+      parent.hasRequiredNodesBelow = true;
+    });
+  });
+
+  // Mark all satisfied nodes.
+  // Imagine checkmarks next to each declared dependency.
+  declaredDependencies.forEach(({key}) => {
+    const node = getOrCreateNodeByPath(depTree, key);
+    node.isSatisfiedRecursively = true;
+  });
+  optionalDependencies.forEach(key => {
+    const node = getOrCreateNodeByPath(depTree, key);
+    node.isSatisfiedRecursively = true;
+  });
+
+  // Tree manipulation helpers.
+  function getOrCreateNodeByPath(rootNode, path) {
+    let keys = path.split('.');
+    let node = rootNode;
+    for (let key of keys) {
+      let child = node.children.get(key);
+      if (!child) {
+        child = createDepTree();
+        node.children.set(key, child);
+      }
+      node = child;
+    }
+    return node;
+  }
+  function markAllParentsByPath(rootNode, path, fn) {
+    let keys = path.split('.');
+    let node = rootNode;
+    for (let key of keys) {
+      let child = node.children.get(key);
+      if (!child) {
+        return;
+      }
+      fn(child);
+      node = child;
+    }
+  }
+
+  // Now we can learn which dependencies are missing or necessary.
+  let missingDependencies = new Set();
+  let satisfyingDependencies = new Set();
+  scanTreeRecursively(
+    depTree,
+    missingDependencies,
+    satisfyingDependencies,
+    key => key,
+  );
+  function scanTreeRecursively(node, missingPaths, satisfyingPaths, keyToPath) {
+    node.children.forEach((child, key) => {
+      const path = keyToPath(key);
+      if (child.isSatisfiedRecursively) {
+        if (child.hasRequiredNodesBelow) {
+          // Remember this dep actually satisfied something.
+          satisfyingPaths.add(path);
+        }
+        // It doesn't matter if there's something deeper.
+        // It would be transitively satisfied since we assume immutability.
+        // `props.foo` is enough if you read `props.foo.id`.
+        return;
+      }
+      if (child.isRequired) {
+        // Remember that no declared deps satisfied this node.
+        missingPaths.add(path);
+        // If we got here, nothing in its subtree was satisfied.
+        // No need to search further.
+        return;
+      }
+      scanTreeRecursively(
+        child,
+        missingPaths,
+        satisfyingPaths,
+        childKey => path + '.' + childKey,
+      );
+    });
+  }
+
+  // Collect suggestions in the order they were originally specified.
+  let suggestedDependencies = [];
+  let unnecessaryDependencies = new Set();
+  let duplicateDependencies = new Set();
+  declaredDependencies.forEach(({key}) => {
+    // Does this declared dep satsify a real need?
+    if (satisfyingDependencies.has(key)) {
+      if (suggestedDependencies.indexOf(key) === -1) {
+        // Good one.
+        suggestedDependencies.push(key);
+      } else {
+        // Duplicate.
+        duplicateDependencies.add(key);
+      }
+    } else {
+      if (isEffect && !key.endsWith('.current')) {
+        // Effects are allowed extra "unnecessary" deps.
+        // Such as resetting scroll when ID changes.
+        // Consider them legit.
+        // The exception is ref.current which is always wrong.
+        if (suggestedDependencies.indexOf(key) === -1) {
+          suggestedDependencies.push(key);
+        }
+      } else {
+        // It's definitely not needed.
+        unnecessaryDependencies.add(key);
+      }
+    }
+  });
+
+  // Then add the missing ones at the end.
+  missingDependencies.forEach(key => {
+    suggestedDependencies.push(key);
+  });
+
+  return {
+    suggestedDependencies,
+    unnecessaryDependencies,
+    duplicateDependencies,
+    missingDependencies,
+  };
+}
+
 /**
  * Assuming () means the passed/returned node:
  * (props) => (props)
  * props.(foo) => (props.foo)
- * props.foo.(bar) => (props.foo).bar
+ * props.foo.(bar) => (props).foo.bar
+ * props.foo.bar.(baz) => (props).foo.bar.baz
  */
 function getDependency(node) {
   if (
@@ -493,7 +631,7 @@ function getDependency(node) {
       node.parent.parent.callee === node.parent
     )
   ) {
-    return node.parent;
+    return getDependency(node.parent);
   } else {
     return node;
   }