typescript-eslint / typescript-eslint

:sparkles: Monorepo for all the tooling which enables ESLint to support TypeScript
https://typescript-eslint.io
Other
15.32k stars 2.74k forks source link

Bug: [no-unused-vars] Add option to allow variables to be used only as types or allow 'declare' #10266

Open InExtremaRes opened 3 weeks ago

InExtremaRes commented 3 weeks ago

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

Playground Link

https://typescript-eslint.io/play/#ts=5.6.2&fileType=.ts&code=CYUwxgNghgTiAEYD2A7AzgF3gfWwIxihWAC54BXFASwEdyE0BPAWzyQgG4AoEADwAckMLBkb8EAIULEQwAMoYYVFAHN4AXniYlq%2BADJ4Abxz5ppeKPFIAZiYJFg8AL7cgA&eslintrc=N4KABGBEBOCuA2BTAzpAXGUEKQAIBcBPABxQGNoBLY-AWhXkoDt8B6Jge1tidmUQAmtAG4BDaKgyRE0aB2iRwYAL4hlQA&tsconfig=N4KABGBEDGD2C2AHAlgGwKYCcDyiAuysAdgM6QBcYoEEkJemy0eFYDAruuGAL4g9A&tokens=false

Repro Code

declare const __brand: unique symbol;
type BrandedString = string & { __brand: typeof __brand };

ESLint Config

module.exports = {
  parser: "@typescript-eslint/parser",
  rules: {
    "@typescript-eslint/no-unused-vars": "error",
  },
};

tsconfig

No response

Expected Result

Have an option to either allow variables declared using declare or to allow variables as types in general.

Actual Result

Variable __brand is flagged as unused without much options (besides disabling the rule or use a pattern).

Additional Info

In #9330 the rule now detects and flags variables that are used only as types. I propose to add an option to disable this behavior, which can be enabled by default.

I've seen some discussion about cases where it's desirable to support such a thing in #9275 and #10184; the latter was closed since the particular requirement in that issue can be achieved by other means. A particular case where I encounter the issue was trying to brand a type with an unique type. I would like to do something similar to this:

type BrandedString = string & { __brand: unique symbol };

However that's not possible since an unique symbol cannot be used that way, but I can do this:

declare const __brand: unique symbol;
type BrandedString = string & { __brand: typeof __brand };

This way the BrandedString will be different to any other BrandedString, even if it is declared the same way since every symbol is actually unique. However this case is flagged by the rule.

(In the actual code I use a declared private field so the #__brand property and its type cannot be accessed from anyone else, but my point still remains)

I propose to either allow cases where the variable is declared using declare or adding a new option to allow variables to be used in type positions. The first case has been touched in the mentioned issues and will satisfy this particular use case, but I think adding an option is the simplest and more universal solution.

Josh-Cena commented 3 weeks ago

Agreed, I think we should find some way to make this use case work.

bradzacher commented 3 weeks ago

My 2c - this usecase isn't broadly common enough to warrant the need for an entire option to handle it, nor is it worth us expanding bandwidth/maintenance burden on it.

Across the entirety of public github repos there are only 8.3k hits.

IMO document, disable comment and move on is all we should fo..

Josh-Cena commented 3 weeks ago

My 2c - this usecase isn't broadly common enough to warrant the need for an entire option to handle it, nor is it worth us expanding bandwidth/maintenance burden on it.

It's a totally valid use case though, most people just never found the need to use branded types. You would probably find similarly much usage of Reflect—doesn't mean these users are simply ignored.

In general, declare itself is a type-only construct, and more often than not actually cannot be used at runtime (unless you are linting .d.ts files).

bradzacher commented 3 weeks ago

I'm not saying ignore. Documenting the usecase as something we know exists but won't add logic for is not ignoring them.

We have limited bandwidth for building, reviewing and maintaining -- and we need to be realistic about that limitation. In an ideal world we would handle all cases but in reality we physically cannot.

8.3k instances is truly minuscule in the grand scheme of things -- hence I say it's definitely not worth thinking too much about.

jakubmazanec commented 1 week ago

Another data point: ability to turn off error "XYX is assigned a value but only used as a type" is definitely something that I would use need now.