Skip to content

[WIP] feat(linter): add eslint/no-unused-vars #642

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 32 commits into from
Closed

[WIP] feat(linter): add eslint/no-unused-vars #642

wants to merge 32 commits into from

Conversation

@DonIsaac
Copy link
Contributor

@DonIsaac DonIsaac commented Jul 27, 2023 •
edited
Loading

This PR is a work in progress and not ready to be merged

Closes #595

Feature Todos

vars

  • support vars: "local"
  • ignore vars by name using regex option
  • Ignore object patterns based on same name
  • Ignore array spreads based on both ignored vars regex and restructured array regex
  • support ignoreRestSiblings
  • support caughtErrors
  • support ignoring unused caught errors with caughtErrorsIgnorePattern option

functions and classes

  • report unused functions
  • report unused classes
  • report functions that only get called recursively

function args

  • report unused args
  • ignore unused args that match argsIgnorePattern
  • support after-used option
  • report unused args from obj/arr spreads that occur "after" a used option when after-used is set

Bugs + Other Todos

In Rule

  • vars that are used as iterators in for in loops should count as a valid read, e.g.
// this is valid
(function(obj) {
  var name;
  for ( name in obj ) return;
})({});
  • bindings declared within the rhs of an assignment should count as a usage, e.g.
//           vvv used b/c exported 
export const Foo = class Foo {}
//                       ^^^ used b/c assigned

(Possibly) in Semantic Analysis

  • variables used in the rhs of an assignment that are also mutated are (incorrectly) not flagged as a read. e.g.
/// this should count as a read usage, but it doesn't
var a = 1; a ||= 1;
// x is incorrectly reported as not read
var x = 1; var y = x++; console.log(y)

@DonIsaac DonIsaac added C-enhancement Category - New feature or request A-linter Area - Linter labels Jul 27, 2023
@github-actions github-actions bot added the A-semantic Area - Semantic label Jul 27, 2023
@Boshen
Copy link
Member

Boshen commented Jul 27, 2023

You may draft this to reduce some noise.

@DonIsaac DonIsaac marked this pull request as draft July 27, 2023 03:54
@DonIsaac DonIsaac self-assigned this Jul 27, 2023
@DonIsaac
Copy link
Contributor Author

blocked by #660

@DonIsaac DonIsaac mentioned this pull request Jul 30, 2023
Merged
DonIsaac added a commit that referenced this pull request Jul 31, 2023
@DonIsaac
feat(ast): AstKind::debug_name() (#665)
4754133 
Adds `AstKind::debug_name()`, which returns the name of an AstKind
struct with a few minor details as applicable. I intentionally did not
make `AstKind` implement `Display`, as this method returns minimal
information. Additionally, this method only exists in debug builds. I've
hidden it behind a `#[cfg(debug_assertions)]`, so it won't affect
release builds.

This code was extracted from #642, where it as created while debugging
iteration over ancestor nodes. I figured it belonged in its own PR and
may be useful to other devs.
@Boshen
Copy link
Member

Boshen commented Oct 21, 2023

Note to self: merge some of this and close this PR.

@Boshen Boshen self-requested a review October 21, 2023 01:29
@Boshen
Copy link
Member

Boshen commented Oct 21, 2023

After inspecting this PR closely, it seems like this is in a unmergable state, so I'll close this for now. Thank you for working on this @DonIsaac!

I think we have the semantic data for reading unused vars now.

@Boshen Boshen closed this Oct 21, 2023
@DonIsaac DonIsaac mentioned this pull request Jul 26, 2024
Merged
11 tasks
Dunqing pushed a commit that referenced this pull request Jul 31, 2024
@DonIsaac
feat(linter): add eslint/no-unused-vars (⭐ attempt 3.2) (#4445)
b952942 
> 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Boshen Boshen Awaiting requested review from Boshen

@Dunqing Dunqing Awaiting requested review from Dunqing Dunqing will be requested when the pull request is marked ready for review Dunqing is a code owner

@camc314 camc314 Awaiting requested review from camc314 camc314 will be requested when the pull request is marked ready for review camc314 is a code owner

Copilot code review Copilot Awaiting requested review from Copilot Copilot will automatically review once the pull request is marked ready for review

Assignees

@DonIsaac DonIsaac

Labels

A-linter Area - Linter A-semantic Area - Semantic C-enhancement Category - New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

feat(linter): no-unused-vars

2 participants

@DonIsaac @Boshen