From bf13d3e3c6632acad4e7fce1bc93df336cb57acc Mon Sep 17 00:00:00 2001 From: Moji Izadmehr Date: Tue, 25 Feb 2020 12:38:23 +0100 Subject: [PATCH] =?UTF-8?q?[eslint-plugin-react-hooks]=20Fix=20cyclic=20ca?= =?UTF-8?q?ching=20for=20loops=20containing=20a=E2=80=A6=20(#16853)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * [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 Co-authored-by: Luke Kang --- .../__tests__/ESLintRulesOfHooks-test.js | 21 +++--- .../src/RulesOfHooks.js | 70 ++++++++++--------- 2 files changed, 50 insertions(+), 41 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index 662e9cc39b963..ad932fe4d792b 100644 --- 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 index dbcc9c3de59bb..0e5dac014611b 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -152,31 +152,33 @@ 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) { @@ -184,7 +186,7 @@ export default { } else { paths = 0; for (const prevSegment of segment.prevSegments) { - paths += countPathsFromStart(prevSegment); + paths += countPathsFromStart(prevSegment, pathList); } } @@ -221,31 +223,33 @@ 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) { @@ -253,11 +257,11 @@ export default { } 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; }