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

feat(eslint-plugin): add no-unused-vars-experimental #688

Merged
merged 4 commits into from
Nov 24, 2019

Conversation

bradzacher
Copy link
Member

@bradzacher bradzacher commented Jul 9, 2019

Creates a new, experimental version of no-unused-vars.
This version leverages typescript's checker to report the unused vars, which saves us from having to build and maintain

Changes from no-unused-vars:

  • Rule will only work inside a file that typescript deems to be either a commonjs or es6 module file.
    • This is because a lot of typescript's logic gates the unused variable checking behind this function. I'd link to the ts source, but it's in checker.ts which is too big for github to render.
  • Completely changed the rule options.
    • I think supporting all of the old options will be hard because Typescript has hardcoded support for ignoring underscore prefixed imports, function arguments, type parameter declarations, and object destructuring variables.
    • I ultimately decided on minimal changes to the underlying typescript checker functionality for simplicity, and because most people seem to like it.

Fixes #54
Fixes #111
Fixes #316
Fixes #363
Fixes #557
Fixes #571
Fixes #696
Fixes #937
Fixes #965
Fixes #1011
Fixes #1062

@bradzacher bradzacher added DO NOT MERGE PRs which should not be merged yet breaking change This change will require a new major version to be released labels Jul 9, 2019
@Jamesernator

This comment has been minimized.

@bradzacher

This comment has been minimized.

@uniqueiniquity
Copy link
Contributor

@bradzacher This looks really good. I'll wait to actually approve until it's left WIP status, but I definitely support keeping around the node maps regardless of the project option. Your usage of the TS API looks fine to me, though I only really saw the point where you get semantic diagnostics from the program.

@bradzacher bradzacher changed the base branch from master to expose-parserServices-always July 18, 2019 00:21
@bradzacher bradzacher force-pushed the expose-parserServices-always branch 2 times, most recently from 89c81a5 to 01de581 Compare July 18, 2019 00:28
@bradzacher bradzacher added enhancement New feature or request and removed DO NOT MERGE PRs which should not be merged yet labels Jul 18, 2019
@ark120202
Copy link

One feature of no-unused-vars that was useful even with noUnusedLocals and noUnusedParameters enabled is caughtErrors: 'all' (to enforce use of optional catch binding). TypeScript intentionally decided not to make a diagnostic there, leaving it to a linter (see microsoft/TypeScript#19601).

Apparently standard no-unused-vars with { varsIgnorePattern: '^(?!error$)', caughtErrors: 'all' } options works there (assuming catch binding is always named error).

I think it would be good to do one of these things:

  • Add a new rule to enforce it
  • Add a special case in @typescript-eslint/no-unused-vars, but it would require to add a way to disable all diagnostic-based warnings (for people using compiler options)
  • Add a note in docs recommending to use a standard rule instead

@bradzacher
Copy link
Member Author

Add a note in docs recommending to use a standard rule instead
This isn't an option because there are so many false positives.

I would go with implementing a new rule to check unhandled exception
I don't really want to mix logic within this new implementation - i.e. I'd want the trigger for all errors reported by this implementation to be from typescript diagnostics.


The usages that I've seen within the TS community is that they love the noUnused* compiler options, but they hate that they are build blocking errors. This implementation fixes that problem.

I thought about continuing the work to complete the TS specific scope analyser (which would fix the base impl of rule, and a few other rules), but I figured that this was significantly less work, and a lot more interesting because it's aligned with what typescript does by default.

@ark120202
Copy link

ark120202 commented Jul 18, 2019

I would go with implementing a new rule to check unhandled exception

👍, that's my preferred option as well

The usages that I've seen within the TS community is that they love the noUnused* compiler options, but they hate that they are build blocking errors.

One thing I'd like to say there is that I've personally always seen TypeScript as a linter. Ignoring any errors to run tests is really convenient during a refactor. There has been a discussion in #122 (comment), but since then ts-node got a --disableExit--log-error option. Maybe it would make sense to get some more feedback on the reason why people have to use noEmitOnError now?

@bradzacher
Copy link
Member Author

Maybe it would make sense to get some more feedback on the reason why people have to use noEmitOnError now?

A lot of the time people have noEmitOnError on by default because it prevents emitting broken code during watch rebuild cycles. Otherwise things like webpack will happily hot reload broken code to your browser causing hard failures instead of nice reloaded screens.

It's the same philosophy that create-react-app's typescript integration has - they don't use the noUnused* options because they are build blocking non-errors, so instead they use our version of no-unused-vars (which has false positives).

As another point - it's also generally easier to have a single tsconfig instead of having both a production one, and a development one. Because you don't want to output broken code to prod, you have noEmitOnError on in your single config.

This philosophy probably stems from the ambiguity of what tsconfig extensions do (it merges compilerOptions objects together, but it feels like it should overwrite because include/exclude/files overwrite).

@bradzacher bradzacher changed the title feat(eslint-plugin): [no-unused-vars] rewrite rule to use ts diagnostics feat(eslint-plugin)!: [no-unused-vars] rewrite rule to use ts diagnostics Jul 21, 2019
@bradzacher bradzacher added this to the 3.0.0 milestone Aug 7, 2019
@Lonli-Lokli

This comment has been minimized.

@bradzacher bradzacher changed the title feat(eslint-plugin)!: [no-unused-vars] rewrite rule to use ts diagnostics feat(eslint-plugin): [no-unused-vars] rewrite rule to use ts diagnostics Oct 8, 2019
@bradzacher bradzacher added enhancement: new plugin rule New rule request for eslint-plugin and removed breaking change This change will require a new major version to be released enhancement New feature or request labels Nov 22, 2019
@bradzacher bradzacher removed this from the 3.0.0 milestone Nov 22, 2019
@bradzacher bradzacher changed the title feat(eslint-plugin): [no-unused-vars] rewrite rule to use ts diagnostics feat(eslint-plugin): add no-unused-vars-experimental Nov 22, 2019
@bradzacher bradzacher changed the base branch from expose-parserServices-always to master November 22, 2019 03:56
@codecov
Copy link

codecov bot commented Nov 24, 2019

Codecov Report

Merging #688 into master will decrease coverage by 0.06%.
The diff coverage is 91.07%.

@@            Coverage Diff             @@
##           master     #688      +/-   ##
==========================================
- Coverage   94.32%   94.25%   -0.07%     
==========================================
  Files         127      128       +1     
  Lines        5443     5555     +112     
  Branches     1534     1556      +22     
==========================================
+ Hits         5134     5236     +102     
- Misses        176      180       +4     
- Partials      133      139       +6
Impacted Files Coverage Δ
...ges/typescript-estree/src/create-program/shared.ts 80% <ø> (ø) ⬆️
packages/eslint-plugin/src/rules/index.ts 100% <100%> (ø) ⬆️
packages/eslint-plugin/src/util/types.ts 82.47% <83.33%> (+0.19%) ⬆️
...nt-plugin/src/rules/no-unused-vars-experimental.ts 92.47% <92.47%> (ø)

@bradzacher bradzacher merged commit 05ebea5 into master Nov 24, 2019
@bradzacher bradzacher deleted the no-unused-vars-rewrite branch November 24, 2019 22:54
@akoidan
Copy link

akoidan commented Nov 25, 2019

@bradzacher is there any way to make it work with vue-tempaltes? When a variables is used in the <template with vue-property-decorator

@bradzacher
Copy link
Member Author

Uhh.. not easily, no.
I didn't design this with vue's custom build chain in mind (as I wrote this before I knew anything about how ridiculously custom vue's build chain is).

This rule would have to be updated to support the eslintUsed property that rules like vue/no-unused-vars rely upon to function.

@mik-jozef
Copy link

This rule would have to be updated to support the eslintUsed property that rules like vue/no-unused-vars rely upon to function.

Is this planned to be solved/is there an issue somewhere for this? I couldn't find any.

@bradzacher
Copy link
Member Author

nope. please file an issue if it's something important to you.
happy to accept a PR as well.

@lsotoangeldonis
Copy link

lsotoangeldonis commented Jan 12, 2020

This rule would have to be updated to support the eslintUsed property that rules like vue/no-unused-vars rely upon to function.

Is this planned to be solved/is there an issue somewhere for this? I couldn't find any.

Don't know if helps, but this worked for me using vue and typescript:

#46 (comment)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.