[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 <kidkkr@icloud.com> Co-authored-by: Luke Kang <kidkkr@icloud.com>
Moji Izadmehr committed
Feb 25, 2020 at 12:38 UTC
bf13d3e3c6632acad4e7fce1bc93df336cb57acc
2 files changed
+50
-41
packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
+13
-8
@@ -405,6 +405,18 @@ const tests = {
405
useHook();
406
}
407
`,
408
+ `
409
+ // Valid because the neither the condition nor the loop affect the hook call.
410
+ function App(props) {
411
+ const someObject = {propA: true};
412
+ for (const propName in someObject) {
413
+ if (propName === true) {
414
+ } else {
415
+ }
416
+ }
417
+ const [myState, setMyState] = useState(null);
418
+ }
419
+ `,
420
],
421
invalid: [
422
{
@@ -640,14 +652,7 @@ const tests = {
652
}
653
}
654
`,
643
- errors: [
644
- loopError('useHook1'),
645
-
646
- // NOTE: Small imprecision in error reporting due to caching means we
647
- // have a conditional error here instead of a loop error. However,
648
- // we will always get an error so this is acceptable.
649
- conditionalError('useHook2', true),
650
- ],
655
+ errors: [loopError('useHook1'), loopError('useHook2', true)],
656
},
657
{
658
code: `
packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
+37
-33
@@ -152,31 +152,33 @@ export default {
152
* Populates `cyclic` with cyclic segments.
153
*/
154
155
- function countPathsFromStart(segment) {
155
+ function countPathsFromStart(segment, pathHistory) {
156
const {cache} = countPathsFromStart;
157
let paths = cache.get(segment.id);
158
-
159
- // If `paths` is null then we've found a cycle! Add it to `cyclic` and
160
- // any other segments which are a part of this cycle.
161
- if (paths === null) {
162
- if (cyclic.has(segment.id)) {
163
- return 0;
164
- } else {
165
- cyclic.add(segment.id);
166
- for (const prevSegment of segment.prevSegments) {
167
- countPathsFromStart(prevSegment);
168
- }
169
- return 0;
158
+ const pathList = new Set(pathHistory);
159
+
160
+ // If `pathList` includes the current segment then we've found a cycle!
161
+ // We need to fill `cyclic` with all segments inside cycle
162
+ if (pathList.has(segment.id)) {
163
+ const pathArray = [...pathList];
164
+ const cyclicSegments = pathArray.slice(
165
+ pathArray.indexOf(segment.id) + 1,
166
+ );
167
+ for (const cyclicSegment of cyclicSegments) {
168
+ cyclic.add(cyclicSegment);
169
}
170
+
171
+ return 0;
172
}
173
174
+ // add the current segment to pathList
175
+ pathList.add(segment.id);
176
+
177
// We have a cached `paths`. Return it.
178
if (paths !== undefined) {
179
return paths;
180
}
181
178
- // Compute `paths` and cache it. Guarding against cycles.
179
- cache.set(segment.id, null);
182
if (codePath.thrownSegments.includes(segment)) {
183
paths = 0;
184
} else if (segment.prevSegments.length === 0) {
@@ -184,7 +186,7 @@ export default {
186
} else {
187
paths = 0;
188
for (const prevSegment of segment.prevSegments) {
187
- paths += countPathsFromStart(prevSegment);
189
+ paths += countPathsFromStart(prevSegment, pathList);
190
}
191
}
192
@@ -221,31 +223,33 @@ export default {
223
* Populates `cyclic` with cyclic segments.
224
*/
225
224
- function countPathsToEnd(segment) {
226
+ function countPathsToEnd(segment, pathHistory) {
227
const {cache} = countPathsToEnd;
228
let paths = cache.get(segment.id);
227
-
228
- // If `paths` is null then we've found a cycle! Add it to `cyclic` and
229
- // any other segments which are a part of this cycle.
230
- if (paths === null) {
231
- if (cyclic.has(segment.id)) {
232
- return 0;
233
- } else {
234
- cyclic.add(segment.id);
235
- for (const nextSegment of segment.nextSegments) {
236
- countPathsToEnd(nextSegment);
237
- }
238
- return 0;
229
+ let pathList = new Set(pathHistory);
230
+
231
+ // If `pathList` includes the current segment then we've found a cycle!
232
+ // We need to fill `cyclic` with all segments inside cycle
233
+ if (pathList.has(segment.id)) {
234
+ const pathArray = Array.from(pathList);
235
+ const cyclicSegments = pathArray.slice(
236
+ pathArray.indexOf(segment.id) + 1,
237
+ );
238
+ for (const cyclicSegment of cyclicSegments) {
239
+ cyclic.add(cyclicSegment);
240
}
241
+
242
+ return 0;
243
}
244
245
+ // add the current segment to pathList
246
+ pathList.add(segment.id);
247
+
248
// We have a cached `paths`. Return it.
249
if (paths !== undefined) {
250
return paths;
251
}
252
247
- // Compute `paths` and cache it. Guarding against cycles.
248
- cache.set(segment.id, null);
253
if (codePath.thrownSegments.includes(segment)) {
254
paths = 0;
255
} else if (segment.nextSegments.length === 0) {
@@ -253,11 +257,11 @@ export default {
257
} else {
258
paths = 0;
259
for (const nextSegment of segment.nextSegments) {
256
- paths += countPathsToEnd(nextSegment);
260
+ paths += countPathsToEnd(nextSegment, pathList);
261
}
262
}
259
- cache.set(segment.id, paths);
263
264
+ cache.set(segment.id, paths);
265
return paths;
266
}
267