In fact, the rules do a much worse job and seem to be very flaky. Why would we recommend these instead of built-in compiler features?

I think the reasoning was that these rules can be enabled without breaking your build.

If that's the case, fair enough, but i'll raise another issue for the unused-vars bug if I can reproduce again.

Have you tried the canary version? The issue should’ve been fixed by #102.

Hi.

I'm not sure if @typescript-eslint/no-unused-vars rule is valuable because tsc covers it.
I'm not familiar with eslint-plugin-typescript, but why does the rule exist? Because maintainance cost is not zero, I don't think that keeping redundant rules is good.

How about the follwoing setting in the recommended preset, instead of a substitute rule for both JS and TS?

{
    "overrides": [
        {
            "files": ["*.ts", "*.tsx"],
            "rules": { "no-unused-vars": "off" }
        }
    ]
}

I personally strongly agree, but I was not involved in the setting up of the recommended rules.

I will let those folks comment who were, but I’m happy for us to consider changing this.

IMO the bar to be recommended should be exceptionally high (in all tools)

Indeed, it definitely should not be recommended IMO.

It will always do a worse job than the compiler as it has less context around the type system and inference.

By having it in the recommended set, we are covering up or clashing with the compiler features which will lead to confusion.

i can see only one use case, vue class components in *.vue files

I personally turn rules that the compiler catches off for my TypeScript projects. Would be in favor of not recommending and possibly deprecating @typescript-eslint/no-unused-vars, since I don't think it's worth the maintenance burden.

i have small idea, but i'm don't know if ts provides api for that:

retrieve errors about used vars and map them to eslintUsed, this will allow us to delegate detection to tsc and give plugins way to extend functionality

@typescript-eslint/explicit-function-return-type
noImplicitReturns

These are not the same and serve completely different purposes.
noImplicitReturns enforces that every code path returns any value at all.
explicit-function-return-type enforces that every function has a return value annotation.

The former is ensuring you haven't forgotten a return, the latter is ensuring you're writing strictly typed function contracts.

@typescript-eslint/no-unused-vars
noUnusedLocals / noUnusedParameters

I brought this up when I took over the plugin for the exact reason that it's got a non-zero maintenance cost and is flakey, and the response from the community was this:

The rule gives a developer the ability to have non-dev-build blocking warning of unused code.

noUnusedLocals + noUnusedParameters are compiler options, which cause hard fails at compile time. If you use these in conjunction with noEmitOnError, then it means you can have a harder time writing/prototyping code, because you can never leave in anything unused.

The use case is that I see the local builds are done with no-unused-vars on, and noUnusedLocals + noUnusedParameters off, then a production/ci/git hook build is done with the inverse.

What I was hoping, was that we could leverage the TS parser services for this, so that we don't have to worry about manually supporting every use case, and then there would be a near zero maintenance cost due to reusing the tsc logic.

@typescript-eslint/explicit-function-return-type
noImplicitReturns

These are not the same and serve completely different purposes.
noImplicitReturns enforces that every code path returns any value at all.
explicit-function-return-type enforces that every function has a return value annotation.

The former is ensuring you haven't forgotten a return, the latter is ensuring you're writing strictly typed function contracts.

Yup i'm aware, my mistake. This means there's yet another bug since the monorepo and/or last version then as inherited types were taken into account before. Either that or another tsc option was handling those checks.

@typescript-eslint/no-unused-vars
noUnusedLocals / noUnusedParameters

I brought this up when I took over the plugin for the exact reason that it's got a non-zero maintenance cost and is flakey, and the response from the community was this:

The rule gives a developer the ability to have non-dev-build blocking warning of unused code.

I really don't think it is worth it. The cost of maintenance of an already flaky rule outweighs the need for someone to change a compilation option into a warning. Such people should be fixing their builds rather than placing a burden on this repo's maintainers for a very redundant feature.

The fact that we're trying to do something which the compiler will always do better is questionable by its self.

in vue class component projects we can't use those options noUnusedLocals / noUnusedParameters

<template>
   <div>{{ bar }}</div>
</template>
<script lang="ts">
import { Component, Vue } from 'vue-property-decorator'
@Component
export default class Foo extends Vue {
  get bar() : string { return 'example' }
}
</script>

in this case typescript is going to warn me that bar is not used, but i'm using it in template.

This means there's yet another bug since the monorepo and/or last version then as inherited types were taken into account before.

If you can get a solid repro case, log it and we'll gladly look into it!

The fact that we're trying to do something which the compiler will always do better is questionable by its self.

Hence my second comment - we're hoping that we can at some point leverage the TS compiler/parser.
This was actually one of the motivations behind absorbing the rule into the plugin, as opposed to extending it like we used to do (see bradzacher/eslint-plugin-typescript#260)

I really don't think it is worth it. The cost of maintenance of an already flaky rule outweighs the need for someone to change a compilation option into a warning.
Such people should be fixing their builds rather than placing a burden on this repo's maintainers for a very redundant feature.

I don't use the rule myself, but many, many users have asked for this. There's a reason this rule was implemented almost 3 years ago, and there's a reason that it's still here 3 years later - users have asked for this, users want this.

I can understand why - it's a very nice workflow when rapidly prototyping code. It's very simple to delete a reference to something with the intention of cleaning it up before pushing/releasing.
Without this, you have to go find the definition, and either comment it out, delete it, or prefix the name with an underscore.

Also there are cases (like @armano2 mentioned), that users cannot use the default TS compiler options.

The problem here is that you're blaming the users for wanting this rule and reporting issues on this rule, when in actuality the problem is more with us (the package maintainers); we have implemented the rule, and we have missed cases. The maintenance burden exists because of how the rule has been written/implemented, and how we missed cases in our tests.
It's our problem to fix our buggy rule in a sustainable way, which is why we are looking for better ways to do this via the compiler.

As an aside, at this point in time I think we've gotten the majority of cases.
v21 of the parser fixed the majority of the issues by providing the correct listings for visitorKeys, saving us from manually iterating over typeAnnotations.

Now we are getting into mainly weird edge cases where there's deeply nested type usages that require special handling.
i.e. #102 from @armano2 which fixed heritage cases:

export interface Bar extends foo.i18n<bar> {} // `bar` was previously marked as unused

I think the problem is more that you're going to be forever playing catch up if you keep this rule implemented yourself. If it can't make use of the compiler's implementation, it'll never be in sync and will essentially be a reinvention of the wheel (which is why it has bugs and "missing cases").

Also regarding the original issue overall, i think we should at least move to get them out of the recommended rule set as they definitely don't belong there IMO.

There's a reason this rule was implemented almost 3 years ago

I got it.

noUnusedLocals option has been introduced since about 2 years ago (microsoft/TypeScript@a0a9666). I think the situation is changed. And the fact that no-unused-vars rule exists may prevent people to find the change of situation.

hard fails

Since 2.9, tsc suggests unused variables even if you don't configure noUnusedLocals option ( microsoft/TypeScript#22361)

I believe that rule is completely duplicated and we can deprecate that rule in favor of tsc.

Since 2.9, tsc suggests unused variables even if you don't configure noUnusedLocals option

I didn't know that @mysticatea!
I haven't ever actually run tsc directly in the CLI without the noUnusedXX options turned on.

Do you know what this looks like in the console?
More importantly - how does it interact with build tools like webpack?

If it doesn't play nice with webpack/etc, then there is still a valid case for the rule.

Also regarding the original issue overall, i think we should at least move to get them out of the recommended rule set as they definitely don't belong there IMO.

We turned it on in recommended because 2/4 config sets we looked at had it turned on (bradzacher/eslint-plugin-typescript#144), and nobody replied saying they were against it being turned on.

We're more than happy to change anything in the recommended set - if there is the community engagement to do so.

I haven't ever actually run tsc directly in the CLI without the noUnusedXX options turned on.

Me too :)
I have never disabled noUnusedXX on both local and CI in the past.

One big benefit of having the rules is that linting runs live in the editor, while tsc only runs during build-time. Having the rules help you catch issues faster.

@sindresorhus TypeScript's type checking is also running in editors real-time. For example, vscode colors the unused variables gray. I guess that other editors have similar feature because TypeScript has the language server.

Unfortunately noUnusedParameters warns for cases like this:

function test(arg1: number, arg2: number) {
  console.log(arg2);
}

src/components/Filter/SelectionValueFilter.tsx:12:15 - error TS6133: 'arg1' is declared but its value is never read.

12 function test(arg1: number, arg2: number) {
                 ~~~~

And will only ignore it if prefixed with an underscore, which you may or may not like:

function test(_arg1: number, arg2: number) {
  console.log(arg2);
}

So I like that no-unused-vars allows to catch unused args while allowing the ones before the last used parameter.

The two rules should be removed from the recommended set either way, as we all seem to have agreed.

Keeping them around is no loss to any of us as long as they're disabled by default. It is more a question of if the maintainers want to maintain them (and if so, they should definitely use the typescript api instead of always playing catch up).

Since 2.9, tsc suggests unused variables even if you don't configure noUnusedLocals option ( microsoft/TypeScript#22361)

i was looking for this, we can use diagnostics to change logic of this rule to report suggestions even if user does not provide noUnusedLocals

i did tests with noUnusedLocals against current version of typescript with vue files and its seems to work fine.
use case there is no longer valid

We've had this debate play out a few times in TSLint. In the latest TSLint, no-unused-variable is deprecated because it became very difficult to maintain while supporting multiple tsc versions. Since then, lots of users have asked for it to be un-deprecated: palantir/tslint#4100. Yes, you will always be playing catch-up with the compiler even if you are leveraging the functionality provided by the TS language services. It could still be a useful rule for typescript-eslint users. I would suggest removing it from the recommended ruleset though, if it has bugs in its current implementation.

Even if it had no bugs and was flawless, it doesn't belong in the recommended config.

We should begin by removing it from there and suggesting to users that they enable the noUnused* compilation options.

a lot of eslint rules relies on scope analysis and if we are going to support any of them we can support all of them

if we will decide to drop support for no-unused-vars it will means that its going to be hard to make sure that all of rules are going to work correctly.

scope analysis is used in:

• no-misleading-character-class
• no-lone-blocks
• no-unused-vars
• prefer-rest-params
• no-implicit-globals
• symbol-description
• no-use-before-define
• no-eval
• consistent-this
• no-unmodified-loop-condition
• no-shadow
• no-invalid-this
• no-undef-init
• no-native-reassign
• prefer-object-spread
• global-require
• radix
• no-undefined
• no-restricted-globals
• object-shorthand
• no-console
• require-unicode-regexp
• no-undef
• no-label-var
• no-extend-native
• no-new-symbol
• handle-callback-err
• no-loop-func
• no-catch-shadow
• no-global-assign
• no-alert
• no-redeclare
• prefer-arrow-callback
• no-regex-spaces

most of them will work if we will use default scope analysis from eslint, but support for additional syntax will not be added, example modules.

for sure there is going some setback and issues, but if its going to work for one rule its will be reused in others

We should begin by removing it from there and suggesting to users that they enable the noUnused* compilation options.

I am currently using the noUnused* compiler options because I haven't upgraded to the alpha that fixed my bug.

These options absolutely suck!

They generate hard failures during development simply because you haven't used a variable yet. Which is 100% reasonable during development, you regularly setup a partially implemented function and want to confirm the partial behaviour before implementing the rest of it. In the last few work days I have regularly dropped or commented out something unfinished, waited for my app to compile, then had to go and comment out an import that is no longer used simply because the incomplete code using it was commented out. All this does is make development slower without providing me any benefit during development.

But having no-unused-vars at lint time is very useful. You get good warnings about clutter you need to cleanup, and you can use commit hooks to make sure you don't commit partial code forgetting you removed something or didn't finish something.

noUnused* compiler options should not be recommended to anyone.

I am not saying remove the rule, it just does not belong in the recommended rule set.

We are currently misleading people by recommending a rule rather than the compilation options. This is the wrong thing to do as they should be exposed to the compilation options and make their own choice.

Keep the rule, remove it from the recommended config.

I am not saying remove the rule, it just does not belong in the recommended rule set.

We are currently misleading people by recommending a rule rather than the compilation options. This is the wrong thing to do as they should be exposed to the compilation options and make their own choice.

I know. I'm saying that these options are so bad that the compiler options should not be "recommended" to anyone. If we give the user a choice between the two and the rule comes with a "there are some bugs we're working on fixing" asterisk, then the compiler options should come with a huge "these compiler options will introduce a hard failure on perfectly functional code and actively impede your work during development" warning and shouldn't be a "recommended" option for anyone.

I'm saying that these options are so bad that the compiler options should not be "recommended" to anyone.

This is a pretty aggressive stance IMO. TS compiler errors do not prohibit JS emit, so you should still be able to easily run your code in development even when the noUnusedVariables check is reporting errors.

This is a pretty aggressive stance IMO. TS compiler errors do not prohibit JS emit, so you should still be able to easily run your code in development even when the noUnusedVariables check is reporting errors.

You'd better tell that to ts-node then, because ts-node throws a fatal error and exits when you enable noUnusedVariables and stick a let foo; into your code. Meaning you cannot run anything when any part of your code contains a type error. And I would assume that NestJS is not the only NodeJS framework that comes preconfigured with ts-node.

ts-node -r tsconfig-paths/register src/main.ts

.../node_modules/ts-node/src/index.ts:261
    return new TSError(diagnosticText, diagnosticCodes)
           ^
TSError: ⨯ Unable to compile TypeScript:
foo.ts(5,5): error TS6133: 'foo' is declared but its value is never read.

I mean I have strict compilation options on and have done for as long as they've existed, with a workflow im happy with.

It really depends on your own personal use case, setup and what not.

Keeping this rule definitely is fine because some people want a "soft" error rather than the hard compilation error. However, by recommending it we are covering up the fact that such a compilation option exists and potentially misleading consumers by doing so (especially since the implementations will always differ).

@dantman without getting too much into the defaults enforced by various tooling in the JS ecosystem right here in this thread (of which ts-node is just one): in my experience with TS over the years, the compiler team has always treated type checking as an additional "add-on" check detached from transpilation, thereby preserving the quick, iterative nature of most JS development workflows. This stance is best illustrated in the default value of the --noEmitOnError compiler option, which is false. So if there's a problem with a downstream tool like ts-node, perhaps you should look into that tool's config options or suggest a change in their repo.

in my experience with TS over the years, the compiler team has always treated type checking as an additional "add-on" check detached from transpilation, thereby preserving the quick, iterative nature of most JS development workflows. [...] perhaps you should look into that tool's config options or suggest a change in their repo.

Do you have a citation for this stance I can quote to those downstream tools? Besides your experience and the default of one compiler option? Because I doubt I can convince any widely used tooling that is the case and get them to change their default based on just that.