[eslint-plugin-react-hooks] Include cycle-entry segment in cyclic set#36324
[eslint-plugin-react-hooks] Include cycle-entry segment in cyclic set#36324thomasbuilds wants to merge 1 commit intofacebook:mainfrom
Conversation
|
Hi @thomasbuilds! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign 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 to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks! |
|
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
countPathsFromStart/countPathsToEndinRulesOfHookspopulate acyclicset of code-path segments so that hooks inside loops can be reported as "may be executed more than once". When a cycle is detected, the code slices thepathListstarting atindexOf(segment.id) + 1— which excludes the repeated segment itself (the cycle-entry / loop-header segment) from thecyclicset.As a result, a hook that lives directly in the test expression of a
whileorforloop is not reported, even though it is called on every iteration:The
do { … } while (useHook())case happened to be reported, but only because of a separateisInsideDoWhileLoopfallback check on the hook node — there was no equivalent for plainwhile/for, so the bug was invisible there.The fix slices from the repeated segment itself (
indexOf(segment.id)instead of+ 1), so every segment participating in the cycle — including the cycle head — is recorded. The change is applied symmetrically to bothcountPathsFromStartandcountPathsToEnd.How did you test this change?
Added two new invalid cases to ESLintRulesOfHooks-test.js asserting
loopError('useHookInsideLoop')for:while (useHookInsideLoop()) { foo(); }for (; useHookInsideLoop(); ) { foo(); }Both cases fail on
main(no error reported) and pass with the fix.Reproduced the underlying off-by-one with a standalone simulation of the cycle-detection logic over the CFG shape ESLint produces for
while (useHook()) { … }(segments:start → test → body → test,test → end):The old behavior drops the
testsegment, which is exactly where the hook in the condition lives — matching the observed false negative.Existing tests (including the
do { … } while (useHook())case, and loop-body cases likewhile (a) { useHook1(); … }) are unaffected: their reported hooks sit in non-header segments that were already captured by the old slice.