Bug: [no-unused-vars] false positive when using decorators on class

[weareoutman]

Before You File a Bug Report Please Confirm You Have Done The Following...

• I have tried restarting my IDE and the issue persists.
• I have updated to the latest version of the packages.
• I have searched for related issues and found none that matched my issue.
• I have read the FAQ and my problem is not listed.

Playground Link

https://typescript-eslint.io/play/#ts=4.9.3&sourceType=module&code=GYVwdgxgLglg9mABAEwKYQBQEoBciCGYAnogN4BQAvueQAJqZbkQA2+Azu4gEJk2VA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQHYHsBaWXWZRAE0IDcBDaVDSfAMxcgBpxtIABAFwCeABxQBjaAEth-QiniTc-APQFipclToN0UAO71ckbgF8QJoA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEk6AHolsvOkXgIaoAi6cmHPLExlKeTAFd04MAF8QsoA

Repro Code

function dec(): any {
}

@dec()
class B {

}

ESLint Config

module.exports = {
  "rules": {
    "no-unused-vars": "off",
    "@typescript-eslint/no-unused-vars": "warn"
  }
}

tsconfig

{
  "compilerOptions": {
    "experimentalDecorators": true
  }
}

Expected Result

No warnings.

Actual Result

'B' is defined but never used.

Additional Info

No response

[JoshuaKGoldberg]

🤔 I would think most users would consider this to be an unused class. Anecdotally speaking, most class decorators I've seen in use today only modify the class - they don't send it somewhere else.

Perhaps this needs to be an opt-in option using the format in discussion in #6017? Marking as blocked pending that discussion's resolution (waiting on eslint/eslint#16540).

[weareoutman]

Actually my case is using decorators to define custom elements, just like the introduction example of the tc39/proposal-decorators

@defineElement("my-class")
class C extends HTMLElement {
  @reactive accessor clicked = false;
}

[weareoutman]

I agree that we can't tell a decorator whether would use the decorated target, just by AST.

I didn't fully understand your proposal in that discussion, but I think we can provide an opt-in option to mark some decorators as having side effects on the decorated target?

[JoshuaKGoldberg]

an opt-in option to mark some decorators as having side effects!

Yup! The tricky part is specifying the format for that opt-in option. Enough rules have this need that we're trying to make one unified format. That discussion is talking about how we want that format to look.

[weareoutman]

👍 Looking forward to it

[bradzacher]

The rule is correct here based purely on semantics.

Why? Because decorators are just syntactic sugar that does something like this:

@deco
class Foo {}

// same as

const Foo = deco(class Foo {})

Notice how regardless of what side-effects the decorator may or may not do, there is still an unused variable left defined?

To put this another way, would you consider this to be an unused variable:

function sideEffect(arg) {
  window.store.add(arg);
  return arg;
}

const foo = sideEffect(1);

I'd hope you'd answer yes, as the variable is clearly unused.

Given that the rule is correct here, the question is whether or not there should be an option to ignore this sort of case?
I'd personally be leaning towards no for two reasons:

1. this is the first time this has been asked for ever, so it's not a common want for the community (I.e. This currently is a niche request).
2. unless I'm mistaken, you could rewrite your code to remove the unused variable by not using a decorator (defineElement('my-class') (class C {})).

(1) may change over time for sure as decorators are standardised fully and more use cases are explored by the community. If it does I think we could definitely revisit this discussion (though at that point it would be in the context of the core rule).

[weareoutman]

@bradzacher Thanks for your patient explaining, but I guess I'll keep using it as decorator and ignore such warnings, : (

[weareoutman]

As you mentioned above of the de-sugared code is like:

const Foo = deco(class Foo {})

Even though only the latter Foo is used, but since they both share the name of Foo, we can't just say that Foo (in source code, there is only one Foo) is not used. I think the only unused thing is the class name, while the class itself (or say the class body) has been used. It's a bit like class expression with or without a name.

Currently in vscode, if I click "Quick Fix", the editor will remove my whole code, which is of course wrong I think (but it maybe the problem of typescript)

[bradzacher]

@weareoutman no-unused-vars does not include a fixer, so any quick fix you're using is provided by another tool.

[weareoutman]

@bradzacher Yes, it's provided by TypeScript, I also fired an issue there, since TS reports an unused error for the same use case, too. It seems that someone else has the same opinion as you mentioned above, however, I don't agree with that. I explained with more of my thoughts on that issue.

[oceanofmaya]

This affects a lot of projects I work on where every entity I create for use with TypeORM has decorators. Refer https://github.com/typeorm/typeorm/blob/master/docs/entities.md

[bradzacher]

We do not encode semantics for framework-specific side-effects in our lint rules.
We purely work on the code you provide.
If the class is not referenced, but is indirectly used by a side-effect in the generator - then it's unused.

If you'd like to encode information about your specific framework's side-effects into the rule - you can freely do that by creating a new rule that marks the variable as used using ESLint's standard API, context.markVariableAsUsed:
https://eslint.org/docs/latest/extend/custom-rules#:~:text=markVariableAsUsed(name)%20%2D%20marks%20a%20variable%20with%20the%20given%20name%20in%20the%20current%20scope%20as%20used.%20This%20affects%20the%20no%2Dunused%2Dvars%20rule.%20Returns%20true%20if%20a%20variable%20with%20the%20given%20name%20was%20found%20and%20marked%20as%20used%2C%20otherwise%20false.

Here are two example rules that do exactly that:

• https://github.com/jsx-eslint/eslint-plugin-react/blob/master/lib/rules/jsx-uses-react.js
• https://github.com/jsx-eslint/eslint-plugin-react/blob/master/lib/rules/jsx-uses-vars.js

[weareoutman]

Thanks for your advice of custom rules, that seems to be the right way