Skip to content
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

Merged

Conversation

matwilko
Copy link
Contributor

@matwilko matwilko commented Jul 12, 2023

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.

const arr = [];

for (let i = 0; i < 10; i++) {
    arr.push(() => a);
             ^^^^^^^ Function declared in a loop contains unsafe references to variable(s) 'a'. eslint(no-loop-func)
                   ^ 'a' is not defined. eslint(no-undef)
}

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 by no-undef.

What actually happened? Please include the actual, raw output from ESLint.

ESLint Output
npm run lint

> eslint@8.44.0 lint
> node Makefile.js lint

Validating JavaScript files

C:\Code\eslint\tests\lib\rules\no-loop-func.js
  24:14  error  Function declared in a loop contains unsafe references to variable(s) 'a'  no-loop-func
  24:20  error  'a' is not defined                                                         no-undef

✖ 2 problems (2 errors, 0 warnings)

C:\Code\eslint\node_modules\shelljs\src\common.js:401
      if (config.fatal) throw e;
                        ^

Error: exec:
    at Object.error (C:\Code\eslint\node_modules\shelljs\src\common.js:110:27)
    at execSync (C:\Code\eslint\node_modules\shelljs\src\exec.js:120:12)
    at _exec (C:\Code\eslint\node_modules\shelljs\src\exec.js:223:12)
    at C:\Code\eslint\node_modules\shelljs\src\common.js:335:23
    at target.lint (C:\Code\eslint\Makefile.js:537:18)
    at global.target.<computed> [as lint] (C:\Code\eslint\node_modules\shelljs\make.js:36:40)
    at C:\Code\eslint\node_modules\shelljs\make.js:48:27
    at Array.forEach (<anonymous>)
    at Timeout._onTimeout (C:\Code\eslint\node_modules\shelljs\make.js:46:10)
    at listOnTimeout (node:internal/timers:569:17)

Node.js v18.16.0
NativeCommandExitException: Program "npm.cmd" ended with non-zero exit code: 1.

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 the while).

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 by no-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 and no-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.

@matwilko matwilko requested a review from a team as a code owner July 12, 2023 18:33
@eslint-github-bot eslint-github-bot bot added the bug ESLint is working incorrectly label Jul 12, 2023
@netlify
Copy link

netlify bot commented Jul 12, 2023

Deploy Preview for docs-eslint ready!

Name Link
🔨 Latest commit ada2f40
🔍 Latest deploy log https://app.netlify.com/sites/docs-eslint/deploys/64b5ab16196b790007fd6af8
😎 Deploy Preview https://deploy-preview-17358--docs-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mdjermanovic mdjermanovic added rule Relates to ESLint's core rules evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 14, 2023
tests/lib/rules/no-loop-func.js Outdated Show resolved Hide resolved
tests/lib/rules/no-loop-func.js Outdated Show resolved Hide resolved
tests/lib/rules/no-loop-func.js Outdated Show resolved Hide resolved
tests/lib/rules/no-loop-func.js Outdated Show resolved Hide resolved
@mdjermanovic
Copy link
Member

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.

@nzakas
Copy link
Member

nzakas commented Jul 14, 2023

@mdjermanovic I also agree.

@mdjermanovic mdjermanovic added accepted There is consensus among the team that this change meets the criteria for inclusion and removed evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion labels Jul 15, 2023
@matwilko matwilko force-pushed the fix-no-loop-func-no-undef-overlap branch from 6cd7aae to ada2f40 Compare July 17, 2023 20:56
@matwilko
Copy link
Contributor Author

Comment on tests updated to reflect how the rule operates at the moment.

Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@mdjermanovic mdjermanovic merged commit 42faa17 into eslint:main Jul 18, 2023
22 checks passed
@matwilko matwilko deleted the fix-no-loop-func-no-undef-overlap branch July 18, 2023 23:20
bmish added a commit to bmish/eslint that referenced this pull request Jul 23, 2023
* 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)
  ...
@eslint-github-bot eslint-github-bot bot locked and limited conversation to collaborators Jan 15, 2024
@eslint-github-bot eslint-github-bot bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Jan 15, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly contributor pool rule Relates to ESLint's core rules
Projects
Status: Complete
Development

Successfully merging this pull request may close these issues.

None yet

3 participants