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

nayounsang · 2025-05-23T15:53:14Z

PR Checklist

Addresses an existing open issue: fixes Enhancement: [no-unused-vars] add a default-off option to autofix remove unused imports #11223
That issue was marked as accepting prs
Steps in Contributing were taken

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 · 2025-05-23T15:53:17Z

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.

netlify · 2025-05-23T15:53:18Z

👷 Deploy Preview for typescript-eslint processing.

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

nx-cloud · 2025-05-23T15:54:12Z

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 · 2025-05-23T16:07:04Z

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

codecov · 2025-05-24T13:13:55Z

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.

packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts

This reverts commit e5394b2.

…ture

kirkwaiblinger

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!

kirkwaiblinger · 2025-06-06T00:12:48Z

packages/eslint-plugin/src/rules/no-unused-vars.ts

+
+                    const source = context.sourceCode;
+                    const node = def.node;
+                    const decl = node.parent as TSESTree.ImportDeclaration;


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.

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

kirkwaiblinger · 2025-06-06T00:22:58Z

packages/eslint-plugin/src/rules/no-unused-vars.ts

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


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

typescript-eslint/packages/eslint-plugin/src/util/getFixOrSuggest.ts

Lines 3 to 21 in 5b2ebcd

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] };

}

}

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

kirkwaiblinger · 2025-06-06T00:27:21Z

packages/eslint-plugin/src/rules/no-unused-vars.ts

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


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).

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

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

related commit: 2c8b740 & 9a15d49

packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts

nayounsang · 2025-06-11T12:31:16Z

@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.

nayounsang added 2 commits May 24, 2025 00:49

test: add testcase

e5394b2

feat: test successful logic

e339973

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

chore: generate schema

7fc5d69

nayounsang commented May 24, 2025

View reviewed changes

packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts Outdated Show resolved Hide resolved

nayounsang and others added 9 commits June 3, 2025 17:42

chore: Revert "test: add testcase"

7b06e7c

This reverts commit e5394b2.

chore: remove uninteded code changes

a2dce5f

refactor: simplify conditional statements to increase converage

855acdf

test: add testacase autofixer should do nothing in import-autofix fea…

233fb1e

…ture

test: add more testcases

622aaf0

refactor: remove duplicate condition validate

122509f

test: add more test case

556ae92

chore: logic that is currently difficult to implement is left as TODO

6d35a71

Merge branch 'main' into auto-unused

440d112

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

chore: add line for test category

d9f1609

kirkwaiblinger requested changes Jun 6, 2025

View reviewed changes

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

nayounsang added 3 commits June 6, 2025 18:20

fix: remove type casting and support TSImportEqulasDeclaration node

11ac4fa

test: simplify test case with noFormat

633d2c8

fix: remove format-related logic

9f76441

nayounsang commented Jun 6, 2025

View reviewed changes

packages/eslint-plugin/tests/rules/no-unused-vars/no-unused-vars.test.ts Outdated Show resolved Hide resolved

nayounsang and others added 3 commits June 8, 2025 21:53

Merge branch 'main' into auto-unused

99699fc

feat: add suggestion for enableAUtofixRemoval

2c8b740

test: add suggestion effected by import statement

9a15d49

nayounsang and others added 5 commits June 8, 2025 23:43

test: add suggestion for no-unused-vars-eslint test

6de9050

feat: remove all unused specifers at once if no used specifers in decl

a177e5c

test: add test

1a05465

Merge branch 'main' into auto-unused

5662583

chore: resolve real conflict

90c2d9a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

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

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

nayounsang commented May 23, 2025 •

edited

Loading

Uh oh!

typescript-eslint bot commented May 23, 2025

Uh oh!

netlify bot commented May 23, 2025 •

edited

Loading

Uh oh!

nx-cloud bot commented May 23, 2025 •

edited

Loading

Uh oh!

nayounsang commented May 23, 2025

Uh oh!

codecov bot commented May 24, 2025 •

edited

Loading

Uh oh!

Uh oh!

kirkwaiblinger left a comment

Uh oh!

kirkwaiblinger Jun 6, 2025

Uh oh!

nayounsang Jun 6, 2025 •

edited

Loading

Uh oh!

kirkwaiblinger Jun 6, 2025

Uh oh!

nayounsang Jun 6, 2025

Uh oh!

kirkwaiblinger Jun 6, 2025

Uh oh!

nayounsang Jun 6, 2025 •

edited

Loading

Uh oh!

nayounsang Jun 8, 2025

Uh oh!

Uh oh!

nayounsang commented Jun 11, 2025

Uh oh!

Uh oh!

	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] };
	}
	}

Uh oh!

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

Are you sure you want to change the base?

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

Conversation

nayounsang commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Checklist

Overview

Auto fix unused import statements

Currently logically impossible (commented test cases)

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

Uh oh!

typescript-eslint bot commented May 23, 2025

Uh oh!

netlify bot commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

👷 Deploy Preview for typescript-eslint processing.

Uh oh!

nx-cloud bot commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nayounsang commented May 23, 2025

Uh oh!

codecov bot commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

kirkwaiblinger left a comment

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

nayounsang Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

nayounsang Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

kirkwaiblinger Jun 6, 2025

Choose a reason for hiding this comment

Uh oh!

nayounsang Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nayounsang Jun 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nayounsang commented Jun 11, 2025

Uh oh!

Uh oh!

nayounsang commented May 23, 2025 •

edited

Loading

netlify bot commented May 23, 2025 •

edited

Loading

nx-cloud bot commented May 23, 2025 •

edited

Loading

codecov bot commented May 24, 2025 •

edited

Loading

nayounsang Jun 6, 2025 •

edited

Loading

nayounsang Jun 6, 2025 •

edited

Loading