[@typescript-eslint/no-unused-vars] Rule should not fail on definition files

[astorije]

Repro

{
  "rules": {
    "@typescript-eslint/no-unused-vars": "error"
  }
}

We are using the flora-sql-parser package which has no type definitions in its repo, nor is there a @types/flora-sql-parser on DefinitelyTyped.

At the moment, we are maintaining a flora-sql-parser.d.ts file locally (which is highly not exhaustive, which is why it has not been submitted to DefinitelyTyped). Part of our definition file has:

declare module 'flora-sql-parser' {
  // ...

  interface SqlAst {
    // ...
  }

  class Parser {
    parse(input: string): SqlAst;
  }

  // ...
}

to express the typing of the Parser class.

Expected Result

Classes defined in definition files should not fail the @typescript-eslint/no-unused-vars rule: it's expected that they are not exported nor used in the current file.

Actual Result

We get the following:

[...]/src/flora-sql-parser.d.ts
  92:9  error  'Parser' is defined but never used  @typescript-eslint/no-unused-vars

Additional Info

I am filing this as a bug report, but I do not have a lot of experience writing definition files so I would totally understand if I'm doing something incorrect in the definition file above (i.e. using a class instead of another construct, e.g. namespace, etc.).

Versions

package	version
@typescript-eslint/eslint-plugin	2.24.0
@typescript-eslint/parser	2.24.0
TypeScript	3.8.3
ESLint	6.8.0
node	12.14.0
npm	6.13.4

[astorije]

Apologies if this is a duplicate of #1596. It seems to me to be slightly different so I preferred to open a ticket and see what you think.

[bradzacher]

It's all the same bug under the hood. We don't have good scope analysis support due to the complexity of the dual-scope nature of TS, and it never being a high priority.

There's the no-unused-vars-experimental rule, which leverages TS's internal unused vars algorithm, but it requires type information.

Ultimately though, we just recommend that people use typescript's no unused compiler option.

[bradzacher]

Merging into #1856

[astorije]

Thank you! I'll follow the ticket there!

[astorije]

@bradzacher, regarding no-unused-vars and no-unused-vars-experimental, a few questions:

• When using noUnusedLocals and noUnusedParameters, what do those rules bring extra or?
• Depending on the answer to the question above, should docs/rules/no-unused-vars.md#when-not-to-use-it contain a note about those compiler options?
• Same question about adding a "When not to use it" section to docs/rules/no-unused-vars-experimental.md
• Should the docs of no-unused-vars-experimental mention that linting time might take a toll?

To be clear, I would love to add such notes myself, but I prefer asking first to make sure I have the right understanding before opening PRs.

[bradzacher]

The difference is that noUnusedLocals and noUnusedParameters are compiler options, so they will prevent your build from passing (unless you have noEmitOnError: false in your compiler options).

Many people don't like the options because they want noEmitOnError: true, so that type errors won't be built. But when you're developing, you don't care if unused variables exist, you only want them cut from the production build.

This is the tradeoff, and one of the reasons that the creators of TS now sort of regret building noUnusedLocals and noUnusedParameters into the compiler.

no-unused-vars-experimental was an experiment in relying upon the compiler (under the hood it just turns on noUnusedLocals and noUnusedParameters, then asks TS for the errors produced). By using TS's underlying feature, it means it's got 100% correctness. Unfortunately, there is a huge performance difference between a lint rule and TS's diagnostics. So it's a good stop gap rule for people that really want the correctness, but don't care about lint times, but it's not a good solution for most.

[astorije]

That's a great explanation, thanks @bradzacher!

So, when using noUnusedLocals and noUnusedParameters, no-unused-vars-experimental is completely unnecessary, noted.

However, do you think there is a benefit in using no-unused-vars also when using noUnusedLocals and noUnusedParameters?

I'm currently seeing a case where this rule flags an unused parameter while TS doesn't:

setStart: (_node: Node, _offset: number) => {},

(it's an unfortunate mock that we have no control over, sadly, and not including those parameters fails the type checker).
Currently debating if I want to use argsIgnorePattern: "^_" or to disable this rule entirely.

[bradzacher]

setting argsIgnorePattern: "^_" (and the vars pattern) will make the rule work the same as typescript works as standard.

There's not really any point in having both on - they report the same things. And also right now (as per the above issue) no-unused-vars is likely to false-positive a lot too.