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

[M-Izadmehr]

This MR Fixes a bug (#16832) in the eslint-plugin-react-hooks, where caching of cycles is performed wrong in some cases.

Example: https://codesandbox.io/s/exciting-bhabha-mqj7q

Description
If the user uses an if statement inside a for ... of obj statement, and uses a correct hook afterwards, the eslint shows an error that:
React Hook "${hook}" is called conditionally. React Hooks must be called in the exact same order in every component render.

Reports
I guess that it is already known issue because, inside the ESLintRulesOfHooks.js - L562, it is already known that in some cases caching of cyclic paths in code path, might result in the wrong errors.

Also (#16832) reports this case too.

Reason
Eslint uses CodePathSegment which is similar to doubly linked list. Based on the CodePathSegment AST different types of cyclic segments might happen. In the special case of ForInStatement there are two paths from obj identifier to the next segment. In such a graph, in order to traverse all the paths from start to end, we need to not only remember the current segment, but also the whole path.

The current code uses a simple caching just by name of segments (and not the list of segment history) as a result, whenever it faces a cycle, it breaks the cycle by returning 0. As a result, there might be several conditions, where the segment might have cyclic and not cyclic paths to end. In the current code, if the first path is cyclic all the non-cyclic ones are thrown away. And depending on the starting point and endpoint of AST, we might have different results.

For example for the demo code, the result of RulesOfHooks.js - L404, the value of all allPathsFromStartToEnd is calculated as 2 instead of 3

Solution
We need caching of responses and also saving cyclic paths for later logic, In the new code, I added a different caching mechanism, which also remembers the path history too. The effect of the performance is still negligible because even for the most difficult test including 1099511627776 paths from start to end of the code, it only increases the time about 0.3s on MacBookPro.

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

[sizebot]

No significant bundle size changes to report.

Generated by 🚫 dangerJS against 7e28144

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[ghost]

I think unnecessary diff in here.

[M-Izadmehr]

I am not sure why, but I am not sure why it considers this line as changed:)

[stale]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

[stale]

Closing this pull request after a prolonged period of inactivity. If this issue is still present in the latest release, please ask for this pull request to be reopened. Thank you!

[TrySound]

@gaearon This problem is still relevant.

[gaearon]

Thanks!

[M-Izadmehr]

I almost forgot about this :D