Thanks for the PR, @developer-bandi!

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.

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	702b828
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/662e656a32de340008a0189f
😎 Deploy Preview	https://deploy-preview-8640--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.
Lighthouse	1 paths audited Performance: 99 (🟢 up 6 from production) Accessibility: 100 (no change from production) Best Practices: 92 (no change from production) SEO: 98 (no change from production) PWA: 80 (no change from production) View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 702b828. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this CI Pipeline Execution

Sent with 💌 from NxCloud.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.21%. Comparing base (216d1b0) to head (d95260e).

❗ Current head d95260e differs from pull request most recent head 702b828. Consider uploading reports for the commit 702b828 to get more accurate results

@@            Coverage Diff             @@
##             main    #8640      +/-   ##
==========================================
- Coverage   87.40%   87.21%   -0.19%     
==========================================
  Files         260      251       -9     
  Lines       12605    12308     -297     
  Branches     3937     3880      -57     
==========================================
- Hits        11017    10735     -282     
+ Misses       1313     1303      -10     
+ Partials      275      270       -5     

Hi @developer-bandi , I think it would be nice to explicitly specify endColumn in some invalid test cases to guarantee the behavior and to prevent regressions! 👍

• no-unused-vars.test.ts

{
          messageId: 'unusedVar',
          line: 3,
          column: 1,
          endColumn:  3, <<<<<<< add `endColumn` on some test cases
          data: {
            varName: 'foo',
            action: 'assigned a value',
            additional: '',
          },
        },

@yeonjuan thank you, i add test case with endline and endColumn that cover shorten error range

    {
      code: `
const foo: number = 1;
      `,
      errors: [
        {
          messageId: 'unusedVar',
          data: {
            varName: 'foo',
            action: 'assigned a value',
            additional: '',
          },
          line: 2,
          column: 7,
          endLine: 2,
          endColumn:10,
        },
      ],
    },

I believe this change broke eslint-plugin-unused-imports, which relies on no-unused-vars from this plugin to pull the node and amend the error. See https://github.com/sweepline/eslint-plugin-unused-imports/blob/master/lib/rules/load-rule.js for the code that loads this rule.

It now crashes here because node is undefined: https://github.com/sweepline/eslint-plugin-unused-imports/blob/master/lib/rules/predicates.js#L28

I don't see a good reason why node has to be removed. Would you mind putting node back into the context.report call?

It does seem a very brittle that that plugin is depending on our private, internal implementation details like that.

For example if we were to change the rule to report on a different node then the rule would similarly break.

From the looks of it that's a horridly inefficient plugin too.
It utilises eslint-rule-composer.
That util works by:

1. duplicating the rule by wrapping its create function in a new rule
2. monkey-patches the context object that ESLint passes so that it can intercept the reports generated by the wrapped rule.

(1) is especially bad because it means you have multiple copies of the rule. i.e. if you use both unused-imports/no-unused-imports and unused-imports/no-unused-vars then you're running the no-unused-vars rule TWICE.

This is a really unstable setup and I'm really not sure if we should be supporting usage of our private internals like that.

I'm not sure I understand the point of the plugin - what is the usecase for separating out the imports from the regular variables? What's the benefit from working this way?

Yup, I get your argument. In this case you could argue it keeps API shape compatibility with the corresponding rule in eslint, if you need a reason.

The plugin comes with a fixer, which I run when hitting cmd+s in VS Code, and it removes all unused imports automatically. If you hoisted this feature into your version of no-unused-vars, I can imagine dropping eslint-plugin-unused-imports.

Note that the ESLint plugin has almost 2m downloads per week, and I'm sure many are using it while also using typescript-eslint, so you'll likely hear from more people soon. I'm just the one who lives on the bleeding edge :D

cc @sweepline – would you be open to porting this rule into eslint-plugin-unused-imports? If yes, @bradzacher would you be open to making a patch release that adds back node until that happens?

@bradzacher we're using the plugin for auto-removing unused imports on save / build as well

I ran into the same issue, and now it's just messed up the whole plugin. I don't think we should be doing these breaking updates.

Let me be very clear:
We changed a private, explicitly not exposed to the public, internal implementation detail.

The other plugin monkey patches eslint's API so it can hook into our internals.

There is no breaking change here.

If something builds on top of private implementation details and breaks when we change those details - that is not a breaking change.

That fact is not up for debate.

How to best remedy this is an open question that we shall discuss amongst the maintenance team and make a decision.

To be clear: the plugin in question is used by almost 10% of typescript-eslint userbase. The plugin itself adds ~100 lines of code to automatically remove unused variables and imports, which is something those users need.

Is there any good reason such functionality shouldn't be a part of typescript-eslint?

mabe typescript-eslint can set a new rule named no-unused-imports and make it fixable. the fact is, I have three plugin have a rule named 'no-unused-vars', and I also really need the auto-fixable rule unused-imports
Besides, sometimes no-unused-imports is to noisy in editor, and that can refer to the solution of @antfu 's config, turn off the rule in editor and turn it on in the cli.

plugin in question is used by almost 10% of typescript-eslint userbase

No, it's very unlikely that 1 in 10 of our users are using eslint-plugin-unused-imports. npm download numbers are an inaccurate metric (reference: 2.3m for the plugin, 27m for us today). Another inaccurate measurement would be GitHub stars, 420 vs 14.6k today.

A more reliable metric would be something like Sourcegraph searches of open source repositories' package.json files today:

• 3.3k results for eslint-plugin-unused-imports
• 111.3k results for @typescript-eslint/eslint-plugin
  • 841 results for the new typescript-eslint

At least in open source land, it seems more likely that the no-unused-imports plugin is floating around 3% of our users.

Is there any good reason such functionality shouldn't be a part of typescript-eslint?

Two reasons. The first is that @typescript-eslint/no-unused-vars acts as an "extension rule" on top of the base eslint/no-unused-vars rule. We don't add any major new functionality on top of base rules here. If you want that added, it'd be an issue for ESLint core.

The reason why ESLint core isn't enthusiastically adding in the automatic removal of values is that it's actually a tricky thing to get right. Seemingly unused imports and variables can actually have side effects that change your code's behavior - which is very dangerous to have in an automatic rule fixer!

Example of an import with a side effect:

// a.js
let count = 0;
console.log("Gotcha!");
export const getCount = () => count;

// b.js

// Unused. Should we auto fix to...
//  import {} from "./a.js" ?
//  import "./a.js" ?
//  nothing at all?
import { getCount } from "./a.js";

console.log("Hi!");

Example of a variable with a side effect:

let count = 0;

export function getNextCount() {
  return (count += 1);
}

// Unused. Should we auto-fix to...
//  getNextCount()
//  nothing at all?
let unusedIsh = getNextCount();

Example of a variable without:

function hasNoSideEffect() {
  for (let i = 0; i < 9001 ** 7; i += 1) {}
}

// Unused. Should we auto-fix to...
//  hasNoSideEffect()
//  nothing at all?
let alsoUnusedIsh = hasNoSideEffect()

By using a plugin like eslint-plugin-unused-imports, you explicitly are taking on the risk of changing code behavior due to side effects like that. Which, hey, might be a great sign about how you've structured your code 😄 nice job avoiding side effects! But lint rules can't make that assumption for the general case.

One thing lint rules can do is add suggestions: which are like fixes but won't be applied automatically. Rule Change: Add support for suggestions for unused var tracks adding that on the ESLint side. That issue was accepted, assigned, and has an open pull request now! 🙌

For more info on the difference between a fix and a suggestion, see:

• https://eslint.org/docs/latest/use/core-concepts/#rule-fixes
• https://eslint.org/docs/latest/use/core-concepts/#rule-suggestions

Given that:

• Reports from our rules are an implementation detail not covered by our public API contract (see typescript-eslint.io > Versioning > eslint-plugin and (I'll file an issue in a moment) Docs: Consider explicitly mentioning rule report properties as not covered by semver #9075 to make this more clear)
• eslint-plugin-unused-imports is stuck having to rely on the specifics of what the rule reports (there's nothing better it could use)

...the "right" solution would be for eslint-plugin-unused-imports to work with what it's given: either the loc or node depending on the version of @typescript-eslint/eslint-plugin. That's being tracked by sweepline/eslint-plugin-unused-imports#77, and has a PR open already in sweepline/eslint-plugin-unused-imports#78.

Edit: that PR was merged the same minute as I posted this comment. Nice!
Edit 2: eslint-plugin-unused-imports@3.2.0 includes the fix. 👏

Anyway, per our PR contributing guide, a previously-merged PR isn't the right place to discuss changes. We'd generally ask that bug reports and feature requests be reported as a standalone new issue so they're more discoverable. We get that this was a blocker for everyone using eslint-plugin-unused-imports so it's reasonable that the discussion ended up happening here. Thanks for the reports, all! ❤️

Locking this issue as this specific typescript-eslint PR doesn't need any more discussion. If more issues on the topic get filed I'll make sure they show up here. Cheers!

The issue with an autofixer is that a lot of people run them on save.

So imagine this:

1. you add import foo from "foo" to your file
2. you save the file
3. you attempt to use the variable foo
4. you get an error because the import was deleted in (2)

There's many variations of this situation - the development loop is an inherently messy thing and there's no way for a static analysis tool to delineate between "unused and it should be deleted" and "unused but it should not be deleted".

There's also the issue of side-effects that Josh mentioned. If you autofix delete an import or a variable - you can change runtime behaviour because side-effects are lost.

There actually used to be a fixer for no-unused-vars a long, long time ago but it was removed because people found it both annoying and dangerous.

There's always scope to introduce an optional fixer for people to opt-in to the danger / annoyance. But as Josh mentioned the rule is intended to be a TS-compatible replacement for the base rule. Whilst it is a complete rewrite - the options, features and reports of the rule are the same.
We don't like to break too far from the base rules for our extension rules because it places a heavy maintenance burden on us. The more forking we do; the harder it is to merge upstream changes.