feat(linter): add eslint/no-unused-vars (⭐ attempt 3.2)

[DonIsaac]

Re-creation of #4427 due to rebasing issues. Original attempt: #642

---

Third time's the charm?

Each time I attempt this rule, I find a bunch of bugs in Semantic, and I expect this attempt to be no different. Expect sidecar issues+PRs stemming from this PR here.

Not Supported

These are cases supported in the original eslint rule, but that I'm intentionally deciding not to support

• export comments in scripts

  /* exported a */ var a;

• global comments

  /* global a */ var a;

Behavior Changes

These are intentional deviations from the original rule's behavior:

• logical re-assignments are not considered usages

  // passes in eslint/no-unused-vars, fails in this implementation
  let a = 0; a ||= 1;
  let b = 0; b &&= 2;
  let c = undefined; c ??= []

Known Limitations

• Lint rules do not have babel or tsconfig information, meaning we can't determine if React imports are being used or not. The relevant tsconfig settings here are jsx, jsxPragma, and jsxFragmentName. To accommodate this, all imports to symbols named React or h are ignored in JSX files.
• References to symbols used in JSDoc {@link} tags are not created, so symbols that are only used in doc comments will be reported as unused. See: (proposal) feat(semantic): add ReferenceFlag::JSDoc #4443
• .vue files are skipped completely, since variables can be used in templates in ways we cannot detect

  note: .d.ts files are skipped as well.

Todo

• Skip unused TS enum members on used enums
• Skip unused parameters followed by used variables in object/array spreads
• Re-assignments to array/object spreads do not respect destructuredArrayIgnorePattern (related to: bug(semantic): incorrect parents for ArrayAssignmentTarget #4435)
• fix(semantic): non-exported namespace member symbols flagged as exported #4493
• References inside a nested scope are not considered usages (bug(semantic): Reference has incorrect span, node_id, and symbol_id #4447)
• Port over typescript-eslint test cases (wip, they've been copied and I'm slowly enabling them)
• Handle constructor properties

  class Foo {
    constructor(public a) {} // `a` should be allowed
  }

• Read references in sequence expressions (that are not in the last position) should not count as a usage

  let a = 0; let b = (a++, 0); console.log(b)

  Honestly, is anyone even writing code like this?

• function overload signatures should not be reported
• Named functions returned from other functions get incorrectly reported as unused (found by @camc314)

  function foo() {
    return function bar() { }
  }
  Foo()()

• false positive for TS modules within ambient modules

  declare global {
    // incorrectly marked as unused
    namespace jest { }
  }

Blockers

• bug(semantic): AstNode has incorrect ScopeId #4436
• bug(semantic): non-last expression in sequence counted as a read #4437
• bug(semantic): references on MemberExpression not recorded #4446
• bug(semantic): Reference has incorrect span, node_id, and symbol_id #4447
• bug(semantic): typeof operator on type imports not counted as a reference #4494
• bug(semantic): generic passed to typeof not counted as a reference #4495

Non-Blocking Issues

• (proposal) feat(semantic): add ReferenceFlag::JSDoc #4443
• bug(semantic): params in export default (function() {}) flagged as SymbolFlags::Export #4475 (prevents checks on exported symbols from namespaces)

[codspeed-hq]

CodSpeed Performance Report

Merging #4445 will not alter performance

_{Comparing don/linter/feat/no-unused-vars-3.2 (b952942) with main (6543958)}

Summary

✅ 32 untouched benchmarks

[Dunqing]

Nice performance! I'll review this PR this weekend.

[camc314]

this is great!

i get a false positive with:

function test() {
    return function Baz() {
        return 'Baz';
    };
}

test()();

output:

  × eslint(no-unused-vars): Function 'Baz' is declared but never used.
   ╭─[./test.ts:2:21]
 1 │ function test() {
 2 │     return function Baz() {
   ·                     ─┬─
   ·                      ╰── 'Baz' is declared here
 3 │         return 'Baz';
   ╰────
  help: Consider removing this declaration.

Finished in 7ms on 1 file with 1 rules using 12 threads.
Found 0 warnings and 1 error.

also:

input:

import type { mySchema } from './my-schema';

function test(arg: ReturnType<typeof mySchema>) {
    arg;
}

test('');

output:

  × eslint(no-unused-vars): Type 'mySchema' is imported but never used.
   ╭─[./test.tsx:1:15]
 1 │ import type { mySchema } from './my-schema';
   ·               ────┬───
   ·                   ╰── 'mySchema' is imported here
 2 │ 
   ╰────
  help: Consider removing this import.

Finished in 7ms on 1 file with 1 rules using 12 threads.
Found 0 warnings and 1 error.

[DonIsaac]

@camc314 thank you so much for the feedback! I'll add those cases and fix it up.

[mysteryven]

This is impressive work!!

---

Found several cases should fail:

1. https://github.com/preactjs/preact/blob/f7f9d9bf4309d09bcd0a432acd72787b7d9baecb/compat/src/index.d.ts#L674-L680

// SS is unused
export abstract class PureComponent<P = {}, S = {}, SS = any> extends preact.Component<P, S> {
    isPureReactComponent: boolean;
}

2. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/87ffcec0275bcded1270f737deb0bc675129a7f6/types/angular-material/index.d.ts#L1

import * as angular from "angular";

3. https://github.com/DefinitelyTyped/DefinitelyTyped/blob/87ffcec0275bcded1270f737deb0bc675129a7f6/types/antlr4/CommonTokenStream.d.ts#L3-L4

4. Looks like some test files are not lint, they are in diff.txt: https://stackblitz.com/edit/stackblitz-starters-7d4upf?file=diff.txt.

[DonIsaac]

@mysteryven those examples are located in .d.ts files, which I am currently skipping checks on completely.

[Dunqing]

@mysteryven those examples are located in .d.ts files, which I am currently skipping checks on completely.

You are right. The semantic doesn't handle .d.ts files either currently.

[Dunqing]

Awesome! Let's merge it after all requested changes are resolved!

[DonIsaac]

@Dunqing I think we're good to go, should we send it?

NOTE: I'm pretty confident this will cause failures in Preact's lint CI job. Lemme make a PR into their codebase first.

EDIT: PRs have been made into offending codebases. Depending on how much time we have between merging this PR and our next release, we should be ok to merge.

[graphite-app]

Merge activity

• Jul 30, 11:20 PM EDT: Dunqing added this pull request to the Graphite merge queue.
• Jul 30, 11:27 PM EDT: Dunqing merged this pull request with the Graphite merge queue.

[Dunqing]

@DonIsaac Can you take a look at https://github.com/oxc-project/oxlint-ecosystem-ci/actions/runs/10173564561/job/28137965651. This case seems a false positive

  x eslint(no-unused-vars): Function 'onMessage' is declared but never used.
    ,-[wasm-runtime/fs-proxy.cjs:78:51]
 77 |  */
 78 | module.exports.createOnMessage = (fs) => function onMessage(e) {
    :                                                   ^^^^|^^^^
    :                                                       `-- 'onMessage' is declared here
 79 |   if (e.data.__fs__) {
    `----
  help: Consider removing this declaration.

[trigaten]

Congrats!