eslint / typescript-eslint-parser

An ESLint custom parser which leverages TypeScript ESTree to allow for ESLint to lint TypeScript source code.
Other
915 stars 92 forks source link

Fix: make no-unused-vars not reporting enum members #558

Closed mysticatea closed 5 years ago

mysticatea commented 5 years ago

This is a small followup for #553.

Currently, no-unused-vars rule reports enum members.

enum E {
    FOO //→ 'FOO' is assigned a value but never used.
}

This behavior is not nice.

This PR makes to set eslintUsed flag to the enum member variables, as similar to /* exported */ directive comments. As a result, no-unused-vars knows the enum members are exported (used on other places).

platinumazure commented 5 years ago

I wonder if this is the right approach?

My thinking: Enums are sort of like objects, and enum values are sort of like object properties. I don't think we check object properties in no-unused-vars. So I feel that enum values also shouldn't be considered variables and shouldn't be added to a variable scope.

Maybe this isn't possible for other reasons? Let me know.

mysticatea commented 5 years ago

It's better if the enum members behaved like properties. In fact, those close to variables. See also: https://github.com/eslint/typescript-eslint-parser/issues/552#issuecomment-439276282

armano2 commented 5 years ago

i'm unsure if this is good way to do it, it will be better if this will be supported by eslint-plugin-typescript?

with actual checking if they are used?

with that we are leaving users with possibility to turn on/off this

mysticatea commented 5 years ago

I think that this approach is fair enough because:

  1. The enum members are exported from the scope of the enum body.
  2. This is the very similar issue to that global variables are exported from the file.
  3. We handle the exported variables by the eslintUsed property.
mysticatea commented 5 years ago

The typescript/use-enum-members rule or something like (as similar to react/jsx-uses-vars) work for me. But I think nice if it's supported in the parser because the similar /* exported */ directive is supported in core.

eventualbuddha commented 5 years ago

This issue is causing my team to sprinkle /* eslint-disable no-unused-vars */ in our codebase whenever we have an enum. I'm happy to help fix this, or to augment typescript/no-unused-vars to handle this. What do y'all think should be done here?

armano2 commented 5 years ago

there is PR in eslint-plugin-typescript with this: https://github.com/bradzacher/eslint-plugin-typescript/pull/236

JamesHenry commented 5 years ago

@armano2 @bradzacher Will merging this PR interfere with the plugin's implementation in any way?

bradzacher commented 5 years ago

@JamesHenry - I don't think so. In fact, it means we can delete some code!

https://github.com/bradzacher/eslint-plugin-typescript/blob/c9d2227db2bea041bcd3dc21c639b05b0732d61e/lib/rules/no-unused-vars.js#L63-L65

kaicataldo commented 5 years ago

Closing this PR since the project has been moved to the TypeScript ESLint organization. Feel free to reopen the PR there.