feat(linter): add no-unused-vars (attempt #3) #4427

DonIsaac · 2024-07-23T13:58:27Z

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;
```

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)
Port over typescript-eslint test cases

Blockers

graphite-app · 2024-07-23T13:58:36Z

Your org has enabled the Graphite merge queue for merging into main

Add the label “merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix.

You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link.

DonIsaac · 2024-07-23T13:58:39Z

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @DonIsaac and the rest of your teammates on Graphite

codspeed-hq · 2024-07-23T14:10:17Z

CodSpeed Performance Report

Merging #4427 will not alter performance

_{Comparing don/07-22-feat_linter_add_no-unused-vars_take_3_ (30d56b2) with main (46cf717)}

Summary

✅ 32 untouched benchmarks

Dunqing · 2024-07-23T14:43:24Z

I'm glad to see you attempt this rule again. After I've done most of the changes to the semantic scopes/symbols I've also thought about implementing some linter rules to verify that the changes are correct.

DonIsaac · 2024-07-23T22:45:13Z

^ Rebase hell, my apologies

Reverts #4414

1. This looks like caused by rust-lang/cargo#4242 1. rust-lang/futures-rs#2305

- 85a7cea semantic: [**BREAKING**] Remove name from `reference` (#4329) (Dunqing) - f68b659 ast: [**BREAKING**] Reorder fields of `ArrowFunctionExpression` (#4364) (Dunqing) - d345b84 ast: Add `#[ast]` attribute to non-visited AST types. (#4309) (rzvxa) - 3c0c709 linter: Add typescript-eslint/no-extraneous-class (#4357) (Jaden Rodriguez) - 68efcd4 linter/react-perf: Handle new objects and arrays in prop assignment patterns (#4396) (DonIsaac) - 0deb027 minfier: Dce `if (xxx) else if (false) { REMOVE }` (#4407) (Boshen) - e33ec18 minifier: Compress `typeof foo == "undefined"` into `typeof foo > "u"` (#4412) (Boshen)- 6068e6b Add error codes to OxcDiagnostic (#4334) (DonIsaac) - aece1df ast: Visit `Program`s `hashbang` field first (#4368) (overlookmotel) - 44a10c4 codegen: Object shorthand with parens `({x: (x)})` -> `({ x })` (#4391) (Boshen) - 3d88f20 codegen: Print shorthand for all `{ x }` variants (#4374) (Boshen) - e624dff codegen,mangler: Do not print shorthand for `ObjectProperty` (#4350) (Boshen) - ac08de8 linter/react_perf: Allow new objects, array, fns, etc in top scope (#4395) (DonIsaac) - 267f7c4 minifier: Skip `Object.defineProperty(exports, ...)` for `cjs-module-lexer` (#4409) (Boshen) - bc8d4e5 semantic: Correct comment (#4410) (overlookmotel) - 6ffce86 semantic: Align `visit_arrow_function_expression` field visit order with ast (#4366) (Dunqing) - 4cd5df0 sourcemap: Avoid negative line if token_chunks has same prev_dst_line (#4348) (underfin) - f8565ae transformer/typescript: Unexpectedly removed class binding from ExportNamedDeclaration (#4351) (Dunqing)- ea33f94 Impl PartialEq<str> for CompactStr (#4352) (DonIsaac) - 1b51511 semantic: Use `Atom` instead of `CompactStr` for `UnresolvedReferencesStack` (#4401) (Dunqing) - 40f9356 semantic: Calculate number of nodes, scopes, symbols, references before visiting AST (#4367) (Dunqing) - da13d93 semantic: Remove bounds checks on unresolved references stack (#4390) (overlookmotel) - e70c67b semantic: Remove a branch from `add_scope` (#4384) (overlookmotel) - 402006f semantic: Simplify logic in `enter_scope` + `leave_scope` (#4383) (overlookmotel) - 7469e01 semantic: Remove branch from `Nodes::add_node` (#4361) (overlookmotel) - 7eb2864 traverse: Speed up finding UID binding name (#4356) (overlookmotel)- a207923 Replace some CompactStr usages with Cows (#4377) (DonIsaac) - d213773 ast: Replace serde rename "lowercase" with "camelCase" (#4376) (overlookmotel) - abfccbd ast: Reduce `#[cfg_attr]` boilerplate in AST type defs (#4375) (overlookmotel) - 5f1c7ec ast: Rename the `visited_node` marker to `ast`. (#4289) (rzvxa) - 58f6ec2 ast: Enter node before scope (#4347) (Dunqing) - 59aea73 ast: Scope is created only if CatchClause has param (#4346) (Dunqing) - 7a3e925 ast_codegen: Better visit marker parsing. (#4371) (rzvxa) - 7a75e0f linter: Use diagnostic codes in lint rules (#4349) (DonIsaac) - a2eabe1 parser: Use error codes for ts diagnostics (#4335) (DonIsaac) - 5d77b36 semantic: `visit_program` visit `hashbang` field (#4370) (overlookmotel) - f7b9ada semantic: `Program` visitor leave scope before node (#4369) (overlookmotel) - 729b288 semantic: Shorten code (#4358) (overlookmotel) - 21d0eee semantic: Use error codes for ts diagnostics (#4336) (DonIsaac) Co-authored-by: Boshen <Boshen@users.noreply.github.com>

Still release failed. Reverts #4417

…#4419) Reverts #4416 This trick doesn't work for crates that have circular dependencies on each other that all need to be published!

DonIsaac · 2024-07-24T16:02:26Z

I seem to have completely fallen into rebase hell, I think I'll re-create this PR

DonIsaac · 2024-07-24T16:36:42Z

Re-created this PR in #4445

Before: ```ts namespace N {} // --------------- ``` After: ```ts namespace N {} // - ``` Found while working on #4427

@camc314

> 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 ```js /* exported a */ var a; ``` - global comments ```js /* global a */ var a; ``` ## Behavior Changes These are intentional deviations from the original rule's behavior: - logical re-assignments are not considered usages ```js // 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: #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 - [x] Skip unused TS enum members on used enums - [x] Skip unused parameters followed by used variables in object/array spreads - [x] Re-assignments to array/object spreads do not respect `destructuredArrayIgnorePattern` (related to: #4435) - [x] #4493 - [x] References inside a nested scope are not considered usages (#4447) - [x] Port over typescript-eslint test cases _(wip, they've been copied and I'm slowly enabling them)_ - [x] Handle constructor properties ```ts class Foo { constructor(public a) {} // `a` should be allowed } ``` - [x] Read references in sequence expressions (that are not in the last position) should not count as a usage ```js let a = 0; let b = (a++, 0); console.log(b) ``` > Honestly, is anyone even writing code like this? - [x] function overload signatures should not be reported - [x] Named functions returned from other functions get incorrectly reported as unused (found by @camc314) ```js function foo() { return function bar() { } } Foo()() ``` - [x] false positive for TS modules within ambient modules ```ts declare global { // incorrectly marked as unused namespace jest { } } ``` ## Blockers - #4436 - #4437 - #4446 - #4447 - #4494 - #4495 ## Non-Blocking Issues - #4443 - #4475 (prevents checks on exported symbols from namespaces)

DonIsaac mentioned this pull request Jul 23, 2024

feat(syntax): add boolean check methods for typescript-related symbol flags #4426

Merged

github-actions bot added A-linter Area - Linter A-semantic Area - Semantic labels Jul 23, 2024

DonIsaac changed the title ~~wip: testing and is_used fix~~ feat(linter): add no-unused-vars (attempt #3) Jul 23, 2024

DonIsaac force-pushed the don/07-22-feat_linter_add_no-unused-vars_take_3_ branch 2 times, most recently from e5f30a9 to eea8737 Compare July 23, 2024 21:29

DonIsaac added the C-enhancement Category - New feature or request label Jul 23, 2024

DonIsaac force-pushed the don/07-22-feat_linter_add_no-unused-vars_take_3_ branch 3 times, most recently from be32177 to e423a36 Compare July 23, 2024 22:05

DonIsaac marked this pull request as ready for review July 23, 2024 22:14

DonIsaac force-pushed the don/07-22-feat_syntax_add_boolean_check_methods_for_typescript-related_symbol_flags branch from ec26c9e to 2e32df2 Compare July 23, 2024 22:15

DonIsaac force-pushed the don/07-22-feat_linter_add_no-unused-vars_take_3_ branch 2 times, most recently from 65ed3b0 to c8f63fe Compare July 23, 2024 22:17

DonIsaac force-pushed the don/07-22-feat_syntax_add_boolean_check_methods_for_typescript-related_symbol_flags branch from f4e577a to e8071e3 Compare July 23, 2024 22:23

DonIsaac changed the base branch from don/07-22-feat_syntax_add_boolean_check_methods_for_typescript-related_symbol_flags to main July 23, 2024 22:26

DonIsaac force-pushed the don/07-22-feat_linter_add_no-unused-vars_take_3_ branch 3 times, most recently from f891ed3 to 2799e1b Compare July 23, 2024 22:44

DonIsaac force-pushed the don/07-22-feat_linter_add_no-unused-vars_take_3_ branch 3 times, most recently from 11deac0 to dfbae81 Compare July 23, 2024 22:53

DonIsaac added 3 commits July 24, 2024 11:01

wip: testing and is_used fix

d5ad8a5

wip: unused parameters

1faf3b6

wip: binding patterns, IIFEs, and fn/variable declarations

f52a970

DonIsaac and others added 11 commits July 24, 2024 11:01

feat: unused spreads in function parameters

daa2d76

cleanup

311fcf4

Revert "Release crates v0.22.0" (#4415)

e13e58a

Reverts #4414

chore: fix failed publish caused by cyclic dev dependencies (#4416)

e0c04fc

1. This looks like caused by rust-lang/cargo#4242 1. rust-lang/futures-rs#2305

Revert "Release crates v0.22.0" (#4418)

99458f4

Still release failed. Reverts #4417

Revert "chore: fix failed publish caused by cyclic dev dependencies " (…

0ea8ea2

…#4419) Reverts #4416 This trick doesn't work for crates that have circular dependencies on each other that all need to be published!

chore: fix cyclic dependencies by moving mangler tests to oxc_minifier

d6d1714

wip: testing and is_used fix

68d1a6e

wip: unused parameters

59a950c

feat: unused spreads in function parameters

687fac9

DonIsaac force-pushed the don/07-22-feat_linter_add_no-unused-vars_take_3_ branch from dfbae81 to 0427efa Compare July 24, 2024 15:24

github-actions bot added A-parser Area - Parser A-minifier Area - Minifier A-ast Area - AST A-transformer Area - Transformer / Transpiler A-codegen Area - Code Generation A-cfg Area - Control Flow Graph A-isolated-declarations Isolated Declarations labels Jul 24, 2024

DonIsaac force-pushed the don/07-22-feat_linter_add_no-unused-vars_take_3_ branch from 0427efa to d51435a Compare July 24, 2024 15:51

test: more ts eslint fail cases

30d56b2

DonIsaac force-pushed the don/07-22-feat_linter_add_no-unused-vars_take_3_ branch from d51435a to 30d56b2 Compare July 24, 2024 16:01

DonIsaac mentioned this pull request Jul 24, 2024

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

Merged

11 tasks

DonIsaac closed this Jul 24, 2024

DonIsaac mentioned this pull request Jul 24, 2024

fix(semantic): use correct span for namespace symbols #4448

Merged

Dunqing pushed a commit that referenced this pull request Jul 25, 2024

fix(semantic): use correct span for namespace symbols (#4448)

77bd5f1

Before: ```ts namespace N {} // --------------- ``` After: ```ts namespace N {} // - ``` Found while working on #4427

Boshen deleted the don/07-22-feat_linter_add_no-unused-vars_take_3_ branch November 21, 2024 13:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(linter): add no-unused-vars (attempt #3) #4427

feat(linter): add no-unused-vars (attempt #3) #4427

DonIsaac commented Jul 23, 2024 •

edited

Loading

graphite-app bot commented Jul 23, 2024

DonIsaac commented Jul 23, 2024 •

edited

Loading

codspeed-hq bot commented Jul 23, 2024 •

edited

Loading

Dunqing commented Jul 23, 2024

DonIsaac commented Jul 23, 2024

DonIsaac commented Jul 24, 2024

DonIsaac commented Jul 24, 2024

feat(linter): add no-unused-vars (attempt #3) #4427

feat(linter): add no-unused-vars (attempt #3) #4427

Conversation

DonIsaac commented Jul 23, 2024 • edited Loading

Not Supported

Todo

Blockers

graphite-app bot commented Jul 23, 2024

Your org has enabled the Graphite merge queue for merging into main

DonIsaac commented Jul 23, 2024 • edited Loading

codspeed-hq bot commented Jul 23, 2024 • edited Loading

CodSpeed Performance Report

Merging #4427 will not alter performance

Summary

Dunqing commented Jul 23, 2024

DonIsaac commented Jul 23, 2024

DonIsaac commented Jul 24, 2024

DonIsaac commented Jul 24, 2024

DonIsaac commented Jul 23, 2024 •

edited

Loading

DonIsaac commented Jul 23, 2024 •

edited

Loading

codspeed-hq bot commented Jul 23, 2024 •

edited

Loading