-
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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.
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
fix: Update no-loop-func to not overlap with no-undef #17358
fix: Update no-loop-func to not overlap with no-undef #17358
Conversation
✅ Deploy Preview for docs-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
I agree that this rule should ignore undefined variables since their usage cannot be analyzed, and they're themselves a problem reported by another rule. |
@mdjermanovic I also agree. |
6cd7aae
to
ada2f40
Compare
Comment on tests updated to reflect how the rule operates at the moment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
* main: (101 commits) docs: Migrating `eslint-env` configuration comments (eslint#17390) Merge pull request from GHSA-qwh7-v8hg-w8rh feat: adds option for allowing empty object patterns as parameter (eslint#17365) fix: FlatESLint#getRulesMetaForResults shouldn't throw on unknown rules (eslint#17393) docs: fix Ignoring Files section in config migration guide (eslint#17392) docs: Update README feat: Improve config error messages (eslint#17385) fix: Update no-loop-func to not overlap with no-undef (eslint#17358) docs: Update README docs: Update README 8.45.0 Build: changelog update for 8.45.0 chore: package.json update for @eslint/js release docs: add playground links to correct and incorrect code blocks (eslint#17306) docs: Expand rule option schema docs (eslint#17198) docs: Config Migration Guide (eslint#17230) docs: Update README fix: Fix suggestion message in `no-useless-escape` (eslint#17339) docs: Update README chore: update eslint-config-eslint exports (eslint#17336) ...
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Tell us about your environment (
npx eslint --env-info
):Node version: v18.16.0
npm version: v9.5.1
Local ESLint version: v8.44.0 (Currently used)
Global ESLint version: Not found
Operating System: win32 10.0.23493
What parser are you using (place an "X" next to just one item)?
[X]
Default (Espree)
[ ]
@typescript-eslint/parser
[ ]
@babel/eslint-parser
[ ]
vue-eslint-parser
[ ]
@angular-eslint/template-parser
[ ]
Other
Please show your full configuration:
eslint
repo's configuration.What did you do? Please include the actual source code causing the issue.
Any loop form with a function declaration in it, where there is a reference to an undeclared variable in the function body.
e.g.
What did you expect to happen?
no-loop-func
shouldn't report on this variable reference, it's not an unsafe loop variable capture, but rather an undeclared variable access that should solely be picked up byno-undef
.What actually happened? Please include the actual, raw output from ESLint.
ESLint Output
What changes did you make? (Give an overview)
Moved a couple of
while
/do..while
loop tests from invalid to valid (the variable they test is undeclared, but looks unsafe because it's the condition of thewhile
).Added some new valid test cases for loops where the function accesses an undeclared variable.
Updated
no-loop-func
to ignore any variable references that are unresolved. These will then be solely picked up byno-undef
if enabled.Why?
My understanding from the Core Rule Guidelines is that no two rules should overlap, and in this case, both
no-loop-func
andno-undef
are warning for the same issue (though with different interpretations of the problem.)Given that
no-loop-func
as documented is intended to find captures of loop variables that are shared by all iterations of the loop, it seems like it shouldn't warn on a variable reference that isn't a loop variable at all.