Skip to content

Fix issue with multiple code branches in hooks linter #14661

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 25, 2019
Merged

Fix issue with multiple code branches in hooks linter #14661

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
cb1e8a5
Fix issue with multiple code branches
Yurickh Jan 23, 2019
d3ad6e5
Add solution by @calebmer
Yurickh Jan 24, 2019
8928f8a
Add performance test
Yurickh Jan 24, 2019
0ad2940
Undo unrelated change
gaearon Jan 25, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,6 +270,17 @@ eslintTester.run('react-hooks', ReactHooksESLintRule, {
useState();
}
`,
`
// Valid because the loop doesn't change the order of hooks calls.
function RegressionTest() {
const res = [];
const additionalCond = true;
for (let i = 0; i !== 10 && additionalCond; ++i ) {
res.push(i);
}
React.useLayoutEffect(() => {});
}
`,
],
invalid: [
{
Expand Down
47 changes: 14 additions & 33 deletions packages/eslint-plugin-react-hooks/src/RulesOfHooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,47 +110,29 @@ export default {
* Segments 1 and 2 have one path to the beginning of `MyComponent` and
* segment 3 has two paths to the beginning of `MyComponent` since we
* could have either taken the path of segment 1 or segment 2.
*
* Populates `cyclic` with cyclic segments.
*/

function countPathsFromStart(segment) {
const {cache} = countPathsFromStart;
Copy link
Contributor

@calebmer calebmer Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi 👋
I’m the original author of this ESLint rule popping in for a review.

The reason I used a cache here is that in complex components, time complexity can really start to spike up. Consider:

function MyComponent() {
  if (a) {} else {}
  if (b) {} else {}
  if (c) {} else {}
  useHook();
}

Here we have 23 = 8 paths from useHook() to the start of MyComponent. Representing the following combinations of a, b, and c.

a b c
true true true
false true true
true false true
false true false
true false false
false true false
false false true
false false false

So we have a 2n exponential relationship, fun. Now remember that every && and || (in the future perhaps also ??) introduces a condition since false && expensive() will not execute expensive().

Let’s say we have a complex component that has 5 conditions placed in the component before 6 hooks all in the same segment. Without a cache we have to call countPathsFromStart() on 25 paths 6 times. With a cache, we only need to call countPathsFromStart() on 2 × 5 segments because we cache the value for every segment so we only need to visit each segment once.

In big-O notation where “n” is the number of conditions and “h” is the number of hooks, we have O(2n) time complexity with a cache and O(2n × h) without a cache.

To see what I mean in practice try adding the following component to the test suite. On this branch, I became impatient after waiting about 10s for the test to finish. When I switched back to master the entire ESLint test suite finished in about 3s.

It’s up to the React team (cc @gaearon) to determine whether or not this performance regression is acceptable. 20 conditions and 10 hooks were fine for me on this branch, but 40 conditions and 10 hooks were not. If this performance regression is not acceptable then I recommend adding the below component to the test suite.

function MyComponent() {
  // 40 conditions
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}
  if (c) {} else {}

  // 10 hooks
  useHook();
  useHook();
  useHook();
  useHook();
  useHook();
  useHook();
  useHook();
  useHook();
  useHook();
  useHook();
}

Copy link
Contributor Author

@Yurickh Yurickh Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Thanks for the chime in, explaining the original rationale behind this piece of code, that was really enlightening.

Unfortunately, I'm not sure there's a way to work around this performance regression. Using the cache as it is proved itself faulty and could lead to false negatives, as issue proved.
I could just remove the item from the cache once we finished visiting it, but the complexity of the overall algorithm would be the same.
I guess the complexity of the problem of finding the number of paths between two nodes on a graph can't really be reduced here.

I'll think about how we could improve it for the case of multiple hooks, so the complexity is not so high.

Copy link
Contributor

@calebmer calebmer Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments below with a possible solution and the rationale behind that solution. TL;DR when countPathsFromStart() breaks a cycle it gives a temporary result of 0 to avoid looping forever.

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;
}
function countPathsFromStart(segment, visited = new Set()) {
if (codePath.thrownSegments.includes(segment)) {
Copy link

@hg-pyun hg-pyun Jan 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it okay includes method without polyfill?
https://developer.mozilla.org/ko/docs/Web/JavaScript/Reference/Global_Objects/Array/includes

Copy link
Contributor Author

@Yurickh Yurickh Jan 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess so, since the linter always runs in a node environmemt.

Copy link
Contributor Author

@Yurickh Yurickh Jan 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, IIRC, thrownSegments isn’t an array here, it’s just array-like.

Copy link
Contributor

@Jessidhia Jessidhia Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming this is an eslint@5 plugin, then yes as it's supported since node 6.

(Unless there are specific rules against it here?)

Copy link
Contributor Author

@Yurickh Yurickh Jan 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it is a direct extract from the original code, this shouldn't really be an issue.

Copy link

@hg-pyun hg-pyun Jan 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, It looks good.

return 0;
}

// We have a cached `paths`. Return it.
if (paths !== undefined) {
return paths;
// We reached the destination
if (segment.prevSegments.length === 0) {
return 1;
}

// 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);
let paths = 0;
visited.add(segment.id);

for (const prevSegment of segment.prevSegments) {
// Check if we already visited the segment so we don't fall into a cycle.
if (!visited.has(prevSegment.id)) {
paths += countPathsFromStart(prevSegment, visited);
}
}
cache.set(segment.id, paths);

visited.delete(segment.id);
return paths;
}

Expand Down Expand Up @@ -271,7 +253,6 @@ export default {
return length;
}

countPathsFromStart.cache = new Map();
countPathsToEnd.cache = new Map();
shortestPathLengthToStart.cache = new Map();

Expand Down
12 changes: 6 additions & 6 deletions ...s/react-reconciler/src/__tests__/__snapshots__/ReactIncrementalPerf-test.internal.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,30 +396,30 @@ exports[`ReactDebugFiberPerf supports Suspense and lazy 2`] = `
"
`;

exports[`ReactDebugFiberPerf supports portals 1`] = `
exports[`ReactDebugFiberPerf supports memo 1`] = `
Copy link
Contributor Author

@Yurickh Yurickh Jan 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure why this changed, as I just run the tests, and didn't ask for snapshot updating at any time.

"⚛ (Waiting for async callback... will force flush in 5250 ms)

⚛ (React Tree Reconciliation: Completed Root)
⚛ Parent [mount]
⚛ Child [mount]
⚛ Foo [mount]

⚛ (Committing Changes)
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 2 Total)
⚛ (Committing Host Effects: 1 Total)
⚛ (Calling Lifecycle Methods: 0 Total)
"
`;

exports[`ReactDebugFiberPerf supports memo 1`] = `
exports[`ReactDebugFiberPerf supports portals 1`] = `
"⚛ (Waiting for async callback... will force flush in 5250 ms)

⚛ (React Tree Reconciliation: Completed Root)
⚛ Parent [mount]
⚛ Foo [mount]
⚛ Child [mount]

⚛ (Committing Changes)
⚛ (Committing Snapshot Effects: 0 Total)
⚛ (Committing Host Effects: 1 Total)
⚛ (Committing Host Effects: 2 Total)
⚛ (Calling Lifecycle Methods: 0 Total)
"
`;
Expand Down