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

[bradzacher]

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

[uniqueiniquity]

@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.

[ark120202]

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]

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]

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]

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

[codecov]

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%> (ø)

[akoidan]

@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]

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]

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]

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

[lsotoangeldonis]

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)