Skip to content

feat(eslint-plugin): add a default-off option to autofix remove unused imports #11243

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

Open
wants to merge 24 commits into
base: main
Choose a base branch
from

Conversation

nayounsang
Copy link

@nayounsang nayounsang commented May 23, 2025

PR Checklist

Overview

Auto fix unused import statements

  • Test cases that correspond to the issue can be fixed

examples

import Unused1 from 'foo';
import Unused2, { Used } from 'bar';
export { Used };

import { Used } from 'bar';
export { Used };

// -------------------
import { Unused1 } from 'foo';
import Used1, { Unused2 } from 'bar';
import { Used2, Unused3 } from 'baz';
import Used3, { Unused4, Used4 } from 'foobar';
export { Used1, Used2, Used3, Used4 };

import Used1 from 'bar';
import { Used2 } from 'baz';
import Used3, { Used4 } from 'foobar';
export { Used1, Used2, Used3, Used4 };

// -----------------
import { /* cmt */ Unused1, Used1 } from 'foo';
export { Used1 };

import { /* cmt */ Used1 } from 'foo';
export { Used1 };

// ------------------
import type { UnusedType } from 'foo';
import { Used1, Unused1 } from 'foo';
export { Used1 };

import { Used1 } from 'foo';
export { Used1 };

// -----------------
import { Unused1 as u1, Used1 as u2 } from 'foo';
export { u2 };

import { Used1 as u2 } from 'foo';
export { u2 };

Currently logically impossible (commented test cases)

  • write an import statement in multiple lines
  • If there are multiple unused specifiers in an import statement

examples

import { Unused1, Unused2, Used1 } from 'foo';
import { Unused3, Unused4 } from 'bar';
export { Used1 };

// expect
import { Used1 } from 'foo';
export { Used1 };

// received
import { Unused2, Used1 } from 'foo';
import { Unused4 } from 'bar';
export { Used1 };

// ----------------------
import {
  Unused1,
  Unused2,
  Unused3,
  Unused4,
  Used1,
  /* cmt */
  Unused5,
  Unused6,
  Used2,
} from 'foo';
export { Used1, Used2 };

// expect (currently skip this case)
import {
  Used1,
  /* cmt */
  Used2,
} from 'foo';
export { Used1, Used2 };

Guessing about the failed(commented) test case: remove multiple unused vars in one-line

  • Conflict between fixer
  • To solve this, I need to remove all specifiers on the same line. (return fixer.remove as an array?)
  • Therefore, we need to check whether each specifer is unused before calling the fixer function.

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @nayounsang!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!


🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint.

Copy link

netlify bot commented May 23, 2025

👷 Deploy Preview for typescript-eslint processing.

Name Link
🔨 Latest commit 90c2d9a
🔍 Latest deploy log https://app.netlify.com/projects/typescript-eslint/deploys/6849768bc9ef6d000895a682

@nayounsang nayounsang marked this pull request as draft May 23, 2025 15:53
Copy link

nx-cloud bot commented May 23, 2025

View your CI Pipeline Execution ↗ for commit 5662583.

Command Status Duration Result
nx test eslint-plugin --coverage=false ❌ Failed 5m 2s View ↗
nx test eslint-plugin ❌ Failed 4m 59s View ↗
nx run-many -t typecheck ✅ Succeeded 2m 8s View ↗
nx run-many -t lint ✅ Succeeded 12s View ↗
nx test typescript-estree --coverage=false ✅ Succeeded <1s View ↗
nx run types:build ✅ Succeeded 1s View ↗
nx test eslint-plugin-internal --coverage=false ✅ Succeeded <1s View ↗
nx run integration-tests:test ✅ Succeeded <1s View ↗
Additional runs (26) ✅ Succeeded ... View ↗

☁️ Nx Cloud last updated this comment at 2025-06-11 09:23:29 UTC

@nayounsang
Copy link
Author

Wow, when adding a new feature, a follow-up development is needed. I'll work on it tomorrow.

Copy link

codecov bot commented May 24, 2025

Codecov Report

Attention: Patch coverage is 94.59459% with 4 lines in your changes missing coverage. Please review.

Project coverage is 90.92%. Comparing base (445514a) to head (6de9050).

Current head 6de9050 differs from pull request most recent head 90c2d9a

Please upload reports for the commit 90c2d9a to get more accurate results.

Files with missing lines Patch % Lines
packages/eslint-plugin/src/rules/no-unused-vars.ts 94.59% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11243      +/-   ##
==========================================
+ Coverage   90.85%   90.92%   +0.06%     
==========================================
  Files         501      501              
  Lines       50901    50943      +42     
  Branches     8383     8403      +20     
==========================================
+ Hits        46248    46318      +70     
+ Misses       4638     4610      -28     
  Partials       15       15              
Flag Coverage Δ
unittest 90.92% <94.59%> (+0.06%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
packages/eslint-plugin/src/rules/no-unused-vars.ts 99.41% <94.59%> (-0.59%) ⬇️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nayounsang nayounsang marked this pull request as ready for review June 5, 2025 06:13
Copy link
Member

@kirkwaiblinger kirkwaiblinger left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A great start!

I'm leaving a bit of a drive-by review since I see that there are several major TODOs in the code that need to get resolved. Feel free to reach out if you're looking for help with those! In the meantime, I'm going to leave a Changes Requested review so that this isn't in our ready-to-review queue.

Thanks!


const source = context.sourceCode;
const node = def.node;
const decl = node.parent as TSESTree.ImportDeclaration;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that this will be unsound for import = require-style imports, for example

import x = require('foo')

Let's be sure to include some test coverage with that style of import.

Copy link
Author

@nayounsang nayounsang Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, nice catch. This is TSImportEqualsDeclaration type.
I test with this code:

    {
      code: `
import x = require('foo');
import y = require('bar');
export { y };
      `,
      errors: [{ messageId: 'unusedVar' }],
      options: [{ enableAutofixRemoval: { imports: true } }],
      output: `
import y = require('bar');
export { y };
      `,
    }

In the existing logic, an error occurs and a more accurate logic is used rather than type casting.
related commit: 11ac4fa

@@ -687,6 +708,80 @@ export default createRule<Options, MessageIds>({
data: unusedVar.references.some(ref => ref.isWrite())
? getAssignedMessageData(unusedVar)
: getDefinedMessageData(unusedVar),
fix: options.enableAutofixRemoval?.imports
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems useful as a suggestion, even if the autofix option is disabled. Consider using getFixOrSuggest to conditionally provide this fix as a suggestion?

export function getFixOrSuggest<MessageId extends string>({
fixOrSuggest,
suggestion,
}: {
fixOrSuggest: 'fix' | 'none' | 'suggest';
suggestion: TSESLint.SuggestionReportDescriptor<MessageId>;
}):
| { fix: TSESLint.ReportFixFunction }
| { suggest: TSESLint.SuggestionReportDescriptor<MessageId>[] }
| undefined {
switch (fixOrSuggest) {
case 'fix':
return { fix: suggestion.fix };
case 'none':
return undefined;
case 'suggest':
return { suggest: [suggestion] };
}
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. However, it would be better to provide suggestions to existing test cases and then resolve this comment before processing.

if (decl.specifiers.length === 1) {
return fixer.removeRange([
decl.range[0],
decl.range[1] + 1, // +1 to include "\n"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to include "\n"

Why do we want to do this exactly? This causes a bug with this technically valid code (please include as a test case - hint, you'll need to use noFormat):

import x from 'foo';import y from 'bar'

My instinct is to say we're better off just leaving a blank line and letting the user decide how to format it afterwards (whether by hand or with prettier or similar).

Copy link
Author

@nayounsang nayounsang Jun 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we're better off just leaving a blank line and letting the user decide how to format it afterwards (whether by hand or with prettier or similar).

Wow, that's right. That should have been it. Plus, it lowers the difficulty of solving the problem and the code becomes simpler.
related commit: 9f76441 & 633d2c8

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add new message: unusedVarSuggestion. However, I'm concerned about the proposed message: Remove unused variable.. Any thoughts?

related commit: 2c8b740 & 9a15d49

@kirkwaiblinger kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Jun 6, 2025
@nayounsang
Copy link
Author

@kirkwaiblinger Hi. I committed my code about your review and TODOs! Please make sure I'm on the right track.
The problem to be solved now is: Problem with unformatted code.

// test case
import {A,B,C,D,E,F,G,Used1,H,I,J,K} from 'foo';
export {Used1};

// expect
import {Used1} from 'foo';
export {Used1};

// received
import {B,D,F,Used1,I,K} from 'foo'
export {Used1};

How can I solve this problem? My idea is to merge the contiguous segments and remove multiple specifiers at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting response Issues waiting for a reply from the OP or another party
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: [no-unused-vars] add a default-off option to autofix remove unused imports
2 participants