From 3125b4088d2e39c932f10f52396791dcf98a8ee2 Mon Sep 17 00:00:00 2001 From: Mojtaba Izadmehr Date: Sun, 22 Sep 2019 19:40:34 +0200 Subject: [PATCH 1/4] [eslint-plugin-react-hooks] Fix cyclic caching for loops containing a condition --- .../__tests__/ESLintRulesOfHooks-test.js | 18 +++-- .../src/RulesOfHooks.js | 70 ++++++++++--------- 2 files changed, 50 insertions(+), 38 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index c6fc1746e13..5002029bb92 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -347,6 +347,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: [ { @@ -558,11 +570,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), + loopError('useHook2', true), ], }, { diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js index a4a4063a8b0..6669da2946a 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -114,31 +114,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; + 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 = [...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) { @@ -146,7 +148,7 @@ export default { } else { paths = 0; for (const prevSegment of segment.prevSegments) { - paths += countPathsFromStart(prevSegment); + paths += countPathsFromStart(prevSegment, pathList); } } @@ -183,31 +185,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 = [...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) { @@ -215,11 +219,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; } From 916a3dc7c3a1ceba809f51d0c4345d46d5cc67b1 Mon Sep 17 00:00:00 2001 From: Mojtaba Izadmehr Date: Sun, 22 Sep 2019 20:45:15 +0200 Subject: [PATCH 2/4] [eslint-plugin-react-hooks] prettier write --- .../__tests__/ESLintRulesOfHooks-test.js | 5 +---- packages/eslint-plugin-react-hooks/src/RulesOfHooks.js | 4 ++-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js index 5002029bb92..096cebba6df 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js @@ -568,10 +568,7 @@ const tests = { } } `, - errors: [ - loopError('useHook1'), - loopError('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 6669da2946a..9763aa859b1 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -127,7 +127,7 @@ export default { pathArray.indexOf(segment.id) + 1, ); for (const cyclicSegment of cyclicSegments) { - cyclic.add(cyclicSegment) + cyclic.add(cyclicSegment); } return 0; @@ -198,7 +198,7 @@ export default { pathArray.indexOf(segment.id) + 1, ); for (const cyclicSegment of cyclicSegments) { - cyclic.add(cyclicSegment) + cyclic.add(cyclicSegment); } return 0; From 5587b8c9ac3e42485118845f02abcdbd36842761 Mon Sep 17 00:00:00 2001 From: Mojtaba Izadmehr Date: Sun, 22 Sep 2019 22:34:14 +0200 Subject: [PATCH 3/4] [eslint-plugin-react-hooks] Fix set for tests --- packages/eslint-plugin-react-hooks/src/RulesOfHooks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js index 9763aa859b1..55399d6f4d7 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -193,7 +193,7 @@ export default { // 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 pathArray = Array.from(pathList); const cyclicSegments = pathArray.slice( pathArray.indexOf(segment.id) + 1, ); From 7e28144a691886cedd4988a82fe2d54ca8afe4e6 Mon Sep 17 00:00:00 2001 From: Mojtaba Izadmehr Date: Thu, 3 Oct 2019 16:25:06 +0200 Subject: [PATCH 4/4] Update packages/eslint-plugin-react-hooks/src/RulesOfHooks.js Co-Authored-By: Luke Kang --- packages/eslint-plugin-react-hooks/src/RulesOfHooks.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js index 55399d6f4d7..51eb3502e42 100644 --- a/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js +++ b/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js @@ -117,7 +117,7 @@ export default { function countPathsFromStart(segment, pathHistory) { const {cache} = countPathsFromStart; let paths = cache.get(segment.id); - let pathList = new Set(pathHistory); + 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