Fix false positive lint error with large number of branches

[scyron6]

…trc.js to use es2020.

Summary

Fixes #24279. JavaScript could not handle the large integer when there were numerous conditional statements before and after the hook. This was causing rounding errors which resulted in a conditional being evaluated incorrectly. I switched the functions to use BigInts instead and updated the eslintrc.js. If using BigInts is not acceptable, I'll be happy to try and find a different solution.

How did you test this change?

The user who created the issue linked to a repository that showed the bug. I copied that exact code and pasted into the tests file. This test now passes.
Link to repository: https://github.com/SanderRonde/eslint-hook-bug

Now when I open that file in my local instance, there is no error message for the hook.

When I run all tests in ESLintRulesOfHooks-test.js, everything passes.

Unfortunately, when I ran yarn test --prod, I got several errors that were unrelated to ESLintRulesOfHooks. I am not sure if this is normal.

[sizebot]

Comparing: af73043...8797f17

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	131.40 kB	131.40 kB	=	41.99 kB	41.99 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	136.45 kB	136.45 kB	=	43.43 kB	43.43 kB
facebook-www/ReactDOM-prod.classic.js	=	433.07 kB	433.07 kB	=	79.60 kB	79.60 kB
facebook-www/ReactDOM-prod.modern.js	=	418.07 kB	418.07 kB	=	77.24 kB	77.24 kB
facebook-www/ReactDOMForked-prod.classic.js	=	433.07 kB	433.07 kB	=	79.61 kB	79.61 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+0.35%	25.54 kB	25.63 kB	+0.24%	8.76 kB	8.78 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+0.35%	25.54 kB	25.63 kB	+0.24%	8.76 kB	8.78 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+0.35%	25.54 kB	25.63 kB	+0.24%	8.76 kB	8.78 kB

Generated by 🚫 dangerJS against 8797f17

[gaearon]

Fascinating.

[gaearon]

It seems worrying that we get to such high numbers. Does this also mean we're doing a ton of work and are slow in this case? Is this case pathological in some particular way? Can we think of any way to do less work for it?

[gaearon]

cc @calebmer you might find this interesting

[scyron6]

Work Context

Adding some screenshots from when I was working just to add some context. This first screenshot is before I added BigInts. The pathsFromStartToEnd and allPathsFromStartToEnd are the values that are being compared in the conditional for the error.

These are the values from the same code but with BigInts.

The code is using a recursive algorithm with time O(log n). I added a console log to the function to see the value at each step of recursion for countPathsFromStart.

Concluding Thoughts

I do not see any way to get around having very large integers if a user has complex code with a lot of conditionals. I have not done a deep dive into the recursive algorithm, but it seems well designed and runs in O(log n). As we have to make sure the hook is reachable from the initial segment and every final segment, I believe we will have to traverse the code somehow.

I'm wondering if there is a way to do this with simple multiplication. If there are 4 simple if else statements before a hook, then there are 2^4 pathsFromStart. We'd have to distinguish the different types of conditionals somehow, which the current recursive algorithm already takes care of.

[gaearon]

Can you do some kind of a stress test to see how this change affects performance for the common cases? I'm not sure if there are any perf pitfalls to using BigInts.

[scyron6]

Setup

I took the eslint-hook-bug repository and made some adjustments. I copied app/foo.tsx (the file that previously failed because the ints were too big) and copied it to fail.tsx. I then removed a couple of the conditionals from app/foo.tsx so there would be no errors when run with the current version. Finally, I copied app/foo.tsx 100 times into the app folder for the stress test.

Stress Test

I checked out the main branch and ran yarn build eslint-plugin-react-hooks to simulate the current environment.

Eslint comes with feature that shows you how long each run takes. The command 'TIMING=1 yarn eslint app/*' will run the rule on every file in app and give us the total time eslint took to run. I ran the command 10 times and sent the output to main_runs.txt. I then ran the same command but with fail.tsx and also sent that output to main_runs.txt just so we can see the failure.
main_runs.txt

I then switched over to the branch that with BigInts and reran yarn build.

I then ran the same TIMING=1 yarn eslint app/* command 10 times and sent the output to big_int_runs.txt. I then ran it with fail.tsx to watch it pass this time.
big_int_runs.txt

Concluding Thoughts

The runtimes seemed very similar to me. If you would also like me to check the memory usage difference, let me know. That might take longer though as I'd have to do a lot of research on how to do that.

[gaearon]

Thanks for checking. Would you mind fixing the lint issue? I think you need to find the lint config we use for builds and add BigInt to known globals.

[gaearon]

Looks like your commits aren't attributed to your GH user (maybe different email). Would you like to fix them up so that you appear as a contributor after merge? I'm not sure how this works.

[scyron6]

You were right I had an old email in there. Don't worry about it though.

[gaearon]

Thanks!