[eslint-plugin-react-hooks] Fix cyclic caching for loops containing a… (

#16853) * [eslint-plugin-react-hooks] Fix cyclic caching for loops containing a condition * [eslint-plugin-react-hooks] prettier write * [eslint-plugin-react-hooks] Fix set for tests * Update packages/eslint-plugin-react-hooks/src/RulesOfHooks.js Co-Authored-By: Luke Kang <[email protected]> Co-authored-by: Luke Kang <[email protected]>
facebook · Feb 25, 2020 · bf13d3e · bf13d3e
1 parent 0e49074
commit bf13d3e
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 41 deletions.
diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
@@ -405,6 +405,18 @@ const tests = {
         useHook();
       }
     `,
+    `
+      // Valid because the neither the condition nor the loop affect the hook call.
+      function App(props) {
+        const someObject = {propA: true};
+        for (const propName in someObject) {
+          if (propName === true) {
+          } else {
+          }
+        }
+        const [myState, setMyState] = useState(null);
+      }
+    `,
   ],
   invalid: [
     {
@@ -640,14 +652,7 @@ const tests = {
           }
         }
       `,
-      errors: [
-        loopError('useHook1'),
-
-        // NOTE: Small imprecision in error reporting due to caching means we
-        // have a conditional error here instead of a loop error. However,
-        // we will always get an error so this is acceptable.
-        conditionalError('useHook2', true),
-      ],
+      errors: [loopError('useHook1'), loopError('useHook2', true)],
     },
     {
       code: `

diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
@@ -152,39 +152,41 @@ export default {
          * Populates `cyclic` with cyclic segments.
          */
 
-        function countPathsFromStart(segment) {
+        function countPathsFromStart(segment, pathHistory) {
           const {cache} = countPathsFromStart;
           let paths = cache.get(segment.id);
-
-          // If `paths` is null then we've found a cycle! Add it to `cyclic` and
-          // any other segments which are a part of this cycle.
-          if (paths === null) {
-            if (cyclic.has(segment.id)) {
-              return 0;
-            } else {
-              cyclic.add(segment.id);
-              for (const prevSegment of segment.prevSegments) {
-                countPathsFromStart(prevSegment);
-              }
-              return 0;
+          const pathList = new Set(pathHistory);
+
+          // If `pathList` includes the current segment then we've found a cycle!
+          // We need to fill `cyclic` with all segments inside cycle
+          if (pathList.has(segment.id)) {
+            const pathArray = [...pathList];
+            const cyclicSegments = pathArray.slice(
+              pathArray.indexOf(segment.id) + 1,
+            );
+            for (const cyclicSegment of cyclicSegments) {
+              cyclic.add(cyclicSegment);
             }
+
+            return 0;
           }
 
+          // add the current segment to pathList
+          pathList.add(segment.id);
+
           // We have a cached `paths`. Return it.
           if (paths !== undefined) {
             return paths;
           }
 
-          // Compute `paths` and cache it. Guarding against cycles.
-          cache.set(segment.id, null);
           if (codePath.thrownSegments.includes(segment)) {
             paths = 0;
           } else if (segment.prevSegments.length === 0) {
             paths = 1;
           } else {
             paths = 0;
             for (const prevSegment of segment.prevSegments) {
-              paths += countPathsFromStart(prevSegment);
+              paths += countPathsFromStart(prevSegment, pathList);
             }
           }
 
@@ -221,43 +223,45 @@ export default {
          * Populates `cyclic` with cyclic segments.
          */
 
-        function countPathsToEnd(segment) {
+        function countPathsToEnd(segment, pathHistory) {
           const {cache} = countPathsToEnd;
           let paths = cache.get(segment.id);
-
-          // If `paths` is null then we've found a cycle! Add it to `cyclic` and
-          // any other segments which are a part of this cycle.
-          if (paths === null) {
-            if (cyclic.has(segment.id)) {
-              return 0;
-            } else {
-              cyclic.add(segment.id);
-              for (const nextSegment of segment.nextSegments) {
-                countPathsToEnd(nextSegment);
-              }
-              return 0;
+          let pathList = new Set(pathHistory);
+
+          // If `pathList` includes the current segment then we've found a cycle!
+          // We need to fill `cyclic` with all segments inside cycle
+          if (pathList.has(segment.id)) {
+            const pathArray = Array.from(pathList);
+            const cyclicSegments = pathArray.slice(
+              pathArray.indexOf(segment.id) + 1,
+            );
+            for (const cyclicSegment of cyclicSegments) {
+              cyclic.add(cyclicSegment);
             }
+
+            return 0;
           }
 
+          // add the current segment to pathList
+          pathList.add(segment.id);
+
           // We have a cached `paths`. Return it.
           if (paths !== undefined) {
             return paths;
           }
 
-          // Compute `paths` and cache it. Guarding against cycles.
-          cache.set(segment.id, null);
           if (codePath.thrownSegments.includes(segment)) {
             paths = 0;
           } else if (segment.nextSegments.length === 0) {
             paths = 1;
           } else {
             paths = 0;
             for (const nextSegment of segment.nextSegments) {
-              paths += countPathsToEnd(nextSegment);
+              paths += countPathsToEnd(nextSegment, pathList);
             }
           }
-          cache.set(segment.id, paths);
 
+          cache.set(segment.id, paths);
           return paths;
         }