Skip to content
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.

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

feat(linter): no-unused-vars auto-fix plan and progress #84

Open
7 of 9 tasks
DonIsaac opened this issue Aug 7, 2024 · 0 comments
Open
7 of 9 tasks

feat(linter): no-unused-vars auto-fix plan and progress #84

DonIsaac opened this issue Aug 7, 2024 · 0 comments
Assignees
Labels
A-linter Area - Linter enhancement New feature or request

Comments

@DonIsaac
Copy link

DonIsaac commented Aug 7, 2024

This issue tracks the progress of providing auto-fix suggestions for eslint/no-unused-vars. All auto-fixes will be "dangerous suggestions", so oxlint will need to be run with --fix-suggestions --fix-dangerously for them to be applied.

Prerequisites

Tasks that adding auto-fixing depends on but that aren't directly related to its implementation

  • fix(linter/no-unused-vars): symbol spans should not include type annotations
  • test(linter): test expected fixes by FixKind
  • feat(linter): add maybeAsiHazard to RuleFixer

    NOTE: this one is also needed for no-console

Unsolved Problems

1. Symbols to Never Remove

IMO, there are some symbols that should never be removed. A (non-comprehensive) list would be

  1. Classes
  2. Functional React components
  3. Interfaces
  4. Namespaces
  5. Function statements

Are there symbol kinds that should be added to or removed from this list? Would love some opinions.

2. Declaration Lists

How should we handle removing unused VariableDeclarations and ImportStatements that declare multiple symbols?
Right now fixers are a fixes are applied independently of each other. However, if all imports are unused, and each diagnostic only deletes the symbol its reporting, then this:

import { A, B } from './a'

Would be fixed into:

import {  } from './a'

Making this fix delete the import statement entirely would change program behavior if ./a has side effects. I would love to hear opinions on this.

3. Unused Reference Chains

A read reference is considered a usage even if it's being used by a variable that is itself unused. This means that when this code gets auto-fixed:

type T = number // used by a
const a: T = 4  // not used anywhere

it will become

type T = number // now T is unused!

I'd expect T to also be removed here, but it remains.

4. JSDoc-only Imports

Sometimes people import things for use in JSDoc comments. Removing them would break their documentation

import { Relevant } from './foo'
/**
 * @see {@link Relevant}
 */
export function foo() {}
  1. We could force this to be a type-only import, and then never remove them
import type { Relevant } from './foo'
  1. We could add JSDoc references to Semantic and consider that a usage
  2. We could update no-unused-vars to look through extracted JSDoc @links and derive usage manually

Suggestions

1. Variable Declarations

  • Remove variable declarations with no references of any kind
  • If a varsIgnorePattern is provided, try to rename the variable to match it
  • Do not delete declarators whose initializers contain assignment expressions to other symbols
    js // x is unused, but we don't wanna delete the whole statement since it removes the export const foo = module.exports = function foo() {}

2. Imports

Sometimes people import things for use in JSDoc comments. Removing them

  • Delete unused named imports
  • Delete unused default imports
  • Delete import * as x imports

CC: @oxc-project/linter

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linter Area - Linter enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

1 participant