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

ESLint 6.2.0 + babel-eslint + no-unused-vars false positive with for-in loop #12117

Closed
feross opened this issue Aug 18, 2019 · 55 comments
Closed
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion blocked This change can't be completed until another issue is resolved bug ESLint is working incorrectly rule Relates to ESLint's core rules

Comments

@feross
Copy link
Contributor

feross commented Aug 18, 2019

There's a new issue in ESLint 6.2.0 just caught by the standard test suite. Issue did not exist in ESLint 6.1.0.

Tell us about your environment

  • ESLint Version: 6.2.0
  • Node Version: 10.16.3
  • npm Version: 6.9.0

What parser (default, Babel-ESLint, etc.) are you using? babel-eslint

Please show your full configuration:

Configuration
{
  "parserOptions": {
    "ecmaVersion": 2019,
    "ecmaFeatures": {
      "jsx": true
    },
    "sourceType": "module"
  },
  "parser": "babel-eslint",
  "rules": {
    "no-unused-vars": ["error", { "vars": "all", "args": "none", "ignoreRestSiblings": true }]
  }
}

What did you do? Please include the actual source code causing the issue, as well as the command that you used to run ESLint.

function scoreAudioCoverFile (imgFile) {
  const fileName = path.basename(imgFile.name, path.extname(imgFile.name)).toLowerCase()
  const relevanceScore = {
    cover: 80,
    folder: 80,
    album: 80,
    front: 80,
    back: 20,
    spectrogram: -80
  }

  for (const keyword in relevanceScore) {
    if (fileName === keyword) {
      return relevanceScore[keyword]
    }
    if (fileName.indexOf(keyword) !== -1) {
      return relevanceScore[keyword]
    }
  }
  return 0
}
npx eslint --config eslintrc.json t.js

What did you expect to happen?

Just one error:

  1:10  error  'scoreAudioCoverFile' is defined but never used  no-unused-vars

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

Two errors:

   1:10  error  'scoreAudioCoverFile' is defined but never used  no-unused-vars
  12:14  error  'keyword' is defined but never used              no-unused-vars

The variable keyword from the for-in loop is seen as not used.

This may be a bug in babel-eslint since the issue does not happen when the default parser is used. However, this issue did not exist with babel-eslint + 6.1.0 and does exist with babel-eslint + 6.2.0.

Issue opened on babel-eslint: babel/babel-eslint#791

Are you willing to submit a pull request to fix this bug?
Yes

@h4emp3
Copy link

h4emp3 commented Aug 18, 2019

I just noticed this as well, maybe a second example might help in finding the problem.

test.js

export const foo = bar => {
    const baz = {};
    for (const key of bar) {
        baz[key] = [];
    }
    return baz;
};
eslint --parser babel-eslint --rule '{"no-unused-vars": ["error"]}' --no-eslintrc test.js

@feross
Copy link
Contributor Author

feross commented Aug 18, 2019

This may have been caused by the change to the no-unused-vars rule introduced in this PR: #12055 cc @mdjermanovic

@h4emp3
Copy link

h4emp3 commented Aug 18, 2019

Well, at least the rule doesn't result in a false positive for my example when I just add the additional condition again in a otherwise unchanged v6.2.0.

@tmorehouse
Copy link

We are getting man false positives with 6.2.0 as well:

6:14 error 'arg' is defined but never used no-unused-vars

  for (const arg of args) {
    if (arg !== undefined) {
      return arg
    }
  }

111:16 error 'type' is defined but never used no-unused-vars

    for (const type of ['a', 'b']) {
      const x = Array.isArray(options[type]) ? options[type] : []

@mdjermanovic
Copy link
Member

babel-eslint is using "eslint-scope": "3.7.1"

@mdjermanovic
Copy link
Member

This may have been caused by the change to the no-unused-vars rule introduced in this PR: #12055 cc @mdjermanovic

Most likely it is that commit, with babel-eslint in the keyword example there is a TDZ scope between function and for, probably because it's using the old version of eslint-scope

@kaicataldo kaicataldo added evaluating The team will evaluate this issue to decide whether it meets the criteria for inclusion rule Relates to ESLint's core rules and removed triage An ESLint team member will look at this issue soon labels Aug 19, 2019
@kaicataldo
Copy link
Member

@feross Any chance you can test the same code out with the default parser to determine if this issue should be fixed in babel-eslint or here? @mdjermanovic's assessment is most likely what's going on here.

@hellomrbigshot
Copy link

The same question! I use 6.1.0 instead and it works.

@feross
Copy link
Contributor Author

feross commented Aug 19, 2019

@kaicataldo The issue doesn't exist with the default parser.

bor3ham added a commit to uptick/react-object-list that referenced this issue Oct 7, 2019
michaelelliot added a commit to OasisDEX/mcd-cdp-portal that referenced this issue Nov 11, 2019
levity added a commit to OasisDEX/mcd-cdp-portal that referenced this issue Nov 11, 2019
levity added a commit to OasisDEX/mcd-cdp-portal that referenced this issue Nov 11, 2019
because it depends on babel-eslint 10.0.3, which gets us the fix for eslint/eslint#12117. upgrading only babel-eslint was causing CI to fail because react-scripts noticed the mismatch
@xsf0105
Copy link

xsf0105 commented Nov 26, 2019

It's a bug in eslint, just upgrade eslint to v6.7.1.

@MarkPare
Copy link

MarkPare commented Dec 4, 2019

I upgraded to eslint v6.7.2 and still seeing this.

@platinumazure
Copy link
Member

It's a bug in eslint, just upgrade eslint to v6.7.1.

@allan2coder This is not accurate, or at least not fully accurate. babel-eslint did have to write a fix on their side as well. Please ensure you have latest babel-eslint.

I upgraded to eslint v6.7.2 and still seeing this.

@MarkPare See above: You do need to upgrade babel-eslint to latest as well. If you upgrade babel-eslint and still run into this issue, please open a new issue and fill out our issue template so we can take a look at your configuration and help you more effectively. Thanks!

@jgreen210
Copy link

...i.e. I also had to remove the .eslintcache directory.

@lffg
Copy link

lffg commented Dec 7, 2019

I am using ESLint v6.7.2 and this error is happening when I use a for..of loop.

image

@RobTheFiveNine
Copy link

If anyone is still running into this issue with React Native due to @react-native-community/eslint-config having a hard reference to version 10.0.1 of babel-eslint and you're using yarn, add this to your package.json, remove node_modules and re-run yarn:

  "resolutions": {
    "@react-native-community/eslint-config/babel-eslint": "^10.0.3"
  }

wincent added a commit to liferay/liferay-npm-tools that referenced this issue Feb 21, 2020
In liferay-portal right now, we've resolved our babel-eslint dependency
to v10.0.2:

https://github.com/liferay/liferay-portal/blob/52b997396c42fa102a7d850d5f3521d63377423c/modules/yarn.lock#L3637

But that has a bug in it causing false positives of the no-unused-vars
rule in relation with `for` loops, as explained here:

eslint/eslint#12117

By tightening the requirement here, we can force liferay-portal to use
the fixed version (we're already on the latest, fixed version of ESLint
itself, so no need to worry about that).

Test plan:

Remove this suppression from the flags-taglib project:

https://github.com/liferay/liferay-portal/blob/52b997396c42fa102a7d850d5f3521d63377423c/modules/apps/flags/flags-taglib/src/main/resources/META-INF/resources/flags/soy/Flags.es.js#L113

See the spurious lint warning:

```
flags-taglib ❯ portool yarn checkFormat
info: using /Users/greghurrell/code/portal/liferay-portal/build/node/bin/node
yarn run v1.17.3
$ liferay-npm-scripts check
Prettier checked 12 files
/Users/greghurrell/code/portal/liferay-portal/modules/apps/flags/flags-taglib/src/main/resources/META-INF/resources/flags/soy/Flags.es.js
  113:14  error  'name' is defined but never used.  no-unused-vars

✖ 1 problem (1 error, 0 warnings)
```

Force usage of babel-eslint 10.0.3 (with `yarn add`, confirming with
`yarn why` that it really is the only version in use in liferay-portal)
and then retest:

```
flags-taglib! ❯ portool yarn checkFormat
info: using /Users/greghurrell/code/portal/liferay-portal/build/node/bin/node
yarn run v1.17.3
$ liferay-npm-scripts check
Prettier checked 12 files

✨  Done in 2.02s.
```
@eslint-deprecated eslint-deprecated bot locked and limited conversation to collaborators Feb 23, 2020
@eslint-deprecated eslint-deprecated bot added the archived due to age This issue has been archived; please open a new issue for any further discussion label Feb 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
archived due to age This issue has been archived; please open a new issue for any further discussion blocked This change can't be completed until another issue is resolved bug ESLint is working incorrectly rule Relates to ESLint's core rules
Projects
None yet
Development

No branches or pull requests