fix(eslint-plugin): [no-unused-vars] clear error report range #8640

developer-bandi · 2024-03-10T09:01:49Z

PR Checklist

Addresses an existing open issue: fixes (partially) Enhancement: Improve lint error report location #8543
That issue was marked as accepting prs
Steps in Contributing were taken

Overview

clear error range in no-unused-vars rule

typescript-eslint · 2024-03-10T09:01:52Z

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 · 2024-03-10T09:02:19Z

✅ 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 · 2024-03-10T09:03:09Z

☁️ 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

✅ Successfully ran 31 targets

Sent with 💌 from NxCloud.

codecov · 2024-03-10T09:15:20Z

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

Additional details and impacted files

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

Flag	Coverage Δ
unittest	`87.21% <100.00%> (-0.19%)`	⬇️

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

Files	Coverage Δ
packages/eslint-plugin/src/rules/no-unused-vars.ts	`98.74% <100.00%> (+0.02%)`	⬆️

... and 64 files with indirect coverage changes

yeonjuan · 2024-03-10T09:33:44Z

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: '',
          },
        },

developer-bandi · 2024-03-10T10:06:39Z

@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,
        },
      ],
    },

yeonjuan · 2024-04-06T11:47:23Z

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

+            const node = writeReferences.length
+              ? writeReferences[writeReferences.length - 1].identifier
+              : unusedVar.identifiers[0];
+
+            const { start } = node.loc;
+            const nodeVariableLength = node.name.length;


👍 What do you think about changing the variable names a bit?

Suggested change

const node = writeReferences.length

? writeReferences[writeReferences.length - 1].identifier

: unusedVar.identifiers[0];

const { start } = node.loc;

const nodeVariableLength = node.name.length;

const id = writeReferences.length

? writeReferences[writeReferences.length - 1].identifier

: unusedVar.identifiers[0];

const { start } = node.loc;

const idLength = node.name.length;

it make sense, so i change variable name. thank you!

developer-bandi · 2024-04-07T03:18:16Z

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

-              node: writeReferences.length
-                ? writeReferences[writeReferences.length - 1].identifier
-                : unusedVar.identifiers[0],
+              node,


i delete node. thank you!

Ah, looking closer at this... I may have led you astray, at least partially. Sorry about that! The test failures in CI are due to having removed the node from the report, since some of the tests specifically check for the type of the node
(the following function generates error assertions that demand an Identifier be reported on via the type field)

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

Lines 56 to 77 in 584db29

/**

* Returns an expected error for assigned-but-not-used variables.

* @param varName The name of the variable

* @param [additional] The additional text for the message data

* @param [type] The node type (defaults to "Identifier")

* @returns An expected error object

*/

function assignedError(

varName: string,

additional = '',

type = AST_NODE_TYPES.Identifier,

): TSESLint.TestCaseError<MessageIds> {

return {

messageId: 'unusedVar',

data: {

varName,

action: 'assigned a value',

additional,

},

type,

};

}

Now, it's still not clear to me that the value of the node is actually used in any way other than being tested, though, when the loc is also provided. I ran into the same problem in #8874 and just removed the type from the test cases, but I'm not totally sure if that is sound.

My thought is
if it has no effect => we should remove the type from the test, since it's not testing anything at all, but it looks like it is
if it does have an effect => just put node back.

Maybe @JoshuaKGoldberg or @bradzacher will know for sure? Would either of you know whether there's any reason to provide a report node if loc is being provided?

Yeah you only need to provide one or the other.

kirkwaiblinger

The code looks good to me! Just requesting a bit more test coverage of the edge cases.

kirkwaiblinger · 2024-04-19T04:30:56Z

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

@@ -419,10 +419,23 @@ export default createRule<Options, MessageIds>({
                ref.from.variableScope === unusedVar.scope.variableScope,
            );

+            const id = writeReferences.length


Even though these branches collapse to do the same thing, I think it could be good to test both branches, in case one day they get separated (for example, to report different error messages)

What do you think about adding test cases with all 4 report loc fields for some cases like the following?

let foo: number;

let foo: number = 1; foo = 2; foo = 3;

kirkwaiblinger · 2024-04-19T04:36:00Z

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

(testing)
Could you also add a test case that uses the declare modifier with the full loc data?

FYI - here and in other comments, there's probably no need to add new test cases; modifying existing ones to have the full loc data would work too 🙂

kirkwaiblinger

In great shape - only thing remaining blocking merge is resolution of #8640 (comment) (by removing type from test cases). Thanks!

kirkwaiblinger

cpojer · 2024-04-30T02:02:19Z

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?

bradzacher · 2024-04-30T03:29:20Z

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:

duplicating the rule by wrapping its create function in a new rule
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?

cpojer · 2024-04-30T03:34:57Z

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?

timtucker-dte · 2024-04-30T04:06:54Z

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

DigitalLeonPro · 2024-04-30T07:59:15Z

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.

bradzacher · 2024-04-30T09:00:28Z

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.

HitkoDev · 2024-04-30T11:45:17Z

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?

JacobZyy · 2024-04-30T12:13:05Z

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.

JoshuaKGoldberg · 2024-04-30T14:29:03Z

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:

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

JoshuaKGoldberg · 2024-04-30T14:39:49Z

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:

JoshuaKGoldberg · 2024-04-30T14:50:05Z

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

JoshuaKGoldberg · 2024-04-30T14:52:10Z

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!

bradzacher · 2024-04-30T14:59:03Z

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

So imagine this:

you add import foo from "foo" to your file
you save the file
you attempt to use the variable foo
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.

fix: clear error code range

74944e5

test: add test code that cover end line

d95260e

bradzacher added the bug Something isn't working label Apr 4, 2024

yeonjuan reviewed Apr 6, 2024

View reviewed changes

kirkwaiblinger reviewed Apr 6, 2024

View reviewed changes

kirkwaiblinger mentioned this pull request Apr 7, 2024

fix(eslint-plugin): [explicit-member-accessibility] refine report locations #8869

Open

3 tasks

yeonjuan mentioned this pull request Apr 7, 2024

Enhancement: Improve lint error report location #8543

Open

9 tasks

refactor: rename variable name and delete not effect property

56ff4f2

kirkwaiblinger requested changes Apr 19, 2024

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Apr 19, 2024

test: add test case

4062305

developer-bandi requested a review from kirkwaiblinger April 19, 2024 14:03

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 19, 2024

kirkwaiblinger approved these changes Apr 19, 2024

View reviewed changes

kirkwaiblinger added the 1 approval PR that a maintainer has LGTM'd - any maintainer can merge this when ready label Apr 19, 2024

armano2 previously approved these changes Apr 22, 2024

View reviewed changes

bradzacher previously approved these changes Apr 22, 2024

View reviewed changes

kirkwaiblinger added the awaiting response Issues waiting for a reply from the OP or another party label Apr 23, 2024

This comment was marked as duplicate.

Sign in to view

kirkwaiblinger requested changes Apr 23, 2024

View reviewed changes

fix: remove type property

8819bd3

developer-bandi dismissed stale reviews from bradzacher and armano2 via 8819bd3 April 28, 2024 08:27

fix: remove type property

00c75a4

fix: remove type property

164798e

developer-bandi requested a review from kirkwaiblinger April 28, 2024 10:56

github-actions bot removed the awaiting response Issues waiting for a reply from the OP or another party label Apr 28, 2024

kirkwaiblinger approved these changes Apr 28, 2024

View reviewed changes

Merge branch 'main' into feat/no-unused-var-patch

702b828

kirkwaiblinger merged commit 8127873 into typescript-eslint:main Apr 28, 2024
57 of 58 checks passed

bradzacher mentioned this pull request Apr 29, 2024

chore: fix v8 branch snapshots for flat-config-types #9029

Merged

3 tasks

cpojer mentioned this pull request Apr 30, 2024

This plugin breaks when using typescript-eslint 7.8.0. sweepline/eslint-plugin-unused-imports#77

Closed

Bernankez mentioned this pull request Apr 30, 2024

Cannot destructure property 'parent' of 'node' as it is undefined. antfu/eslint-config#464

Closed

5 tasks

This comment has been minimized.

Sign in to view

auvred mentioned this pull request Apr 30, 2024

chore: add knip #8192

Merged

26 tasks

typescript-eslint deleted a comment from github-actions bot Apr 30, 2024

typescript-eslint locked as resolved and limited conversation to collaborators Apr 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(eslint-plugin): [no-unused-vars] clear error report range #8640

fix(eslint-plugin): [no-unused-vars] clear error report range #8640

developer-bandi commented Mar 10, 2024 •

edited by auvred

typescript-eslint bot commented Mar 10, 2024

netlify bot commented Mar 10, 2024 •

edited

nx-cloud bot commented Mar 10, 2024 •

edited

codecov bot commented Mar 10, 2024 •

edited

yeonjuan commented Mar 10, 2024 •

edited

developer-bandi commented Mar 10, 2024 •

edited

yeonjuan Apr 6, 2024 •

edited

developer-bandi Apr 7, 2024

This comment was marked as resolved.

developer-bandi Apr 7, 2024

kirkwaiblinger Apr 19, 2024

JoshuaKGoldberg Apr 22, 2024

kirkwaiblinger left a comment

kirkwaiblinger Apr 19, 2024

kirkwaiblinger Apr 19, 2024

This comment was marked as duplicate.

kirkwaiblinger left a comment

kirkwaiblinger left a comment

cpojer commented Apr 30, 2024

bradzacher commented Apr 30, 2024

cpojer commented Apr 30, 2024

timtucker-dte commented Apr 30, 2024

This comment has been minimized.

DigitalLeonPro commented Apr 30, 2024 •

edited

bradzacher commented Apr 30, 2024

HitkoDev commented Apr 30, 2024 •

edited

JacobZyy commented Apr 30, 2024

JoshuaKGoldberg commented Apr 30, 2024

JoshuaKGoldberg commented Apr 30, 2024

JoshuaKGoldberg commented Apr 30, 2024 •

edited

JoshuaKGoldberg commented Apr 30, 2024

bradzacher commented Apr 30, 2024

	/**
	* Returns an expected error for assigned-but-not-used variables.
	* @param varName The name of the variable
	* @param [additional] The additional text for the message data
	* @param [type] The node type (defaults to "Identifier")
	* @returns An expected error object
	*/
	function assignedError(
	varName: string,
	additional = '',
	type = AST_NODE_TYPES.Identifier,
	): TSESLint.TestCaseError<MessageIds> {
	return {
	messageId: 'unusedVar',
	data: {
	varName,
	action: 'assigned a value',
	additional,
	},
	type,
	};
	}

fix(eslint-plugin): [no-unused-vars] clear error report range #8640

fix(eslint-plugin): [no-unused-vars] clear error report range #8640

Conversation

developer-bandi commented Mar 10, 2024 • edited by auvred

PR Checklist

Overview

typescript-eslint bot commented Mar 10, 2024

netlify bot commented Mar 10, 2024 • edited

✅ Deploy Preview for typescript-eslint ready!

nx-cloud bot commented Mar 10, 2024 • edited

☁️ Nx Cloud Report

codecov bot commented Mar 10, 2024 • edited

Codecov Report

yeonjuan commented Mar 10, 2024 • edited

developer-bandi commented Mar 10, 2024 • edited

yeonjuan Apr 6, 2024 • edited

Choose a reason for hiding this comment

developer-bandi Apr 7, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

developer-bandi Apr 7, 2024

Choose a reason for hiding this comment

kirkwaiblinger Apr 19, 2024

Choose a reason for hiding this comment

JoshuaKGoldberg Apr 22, 2024

Choose a reason for hiding this comment

kirkwaiblinger left a comment

Choose a reason for hiding this comment

kirkwaiblinger Apr 19, 2024

Choose a reason for hiding this comment

kirkwaiblinger Apr 19, 2024

Choose a reason for hiding this comment

This comment was marked as duplicate.

kirkwaiblinger left a comment

Choose a reason for hiding this comment

kirkwaiblinger left a comment

Choose a reason for hiding this comment

cpojer commented Apr 30, 2024

bradzacher commented Apr 30, 2024

cpojer commented Apr 30, 2024

timtucker-dte commented Apr 30, 2024

This comment has been minimized.

DigitalLeonPro commented Apr 30, 2024 • edited

bradzacher commented Apr 30, 2024

HitkoDev commented Apr 30, 2024 • edited

JacobZyy commented Apr 30, 2024

JoshuaKGoldberg commented Apr 30, 2024

JoshuaKGoldberg commented Apr 30, 2024

JoshuaKGoldberg commented Apr 30, 2024 • edited

JoshuaKGoldberg commented Apr 30, 2024

bradzacher commented Apr 30, 2024

developer-bandi commented Mar 10, 2024 •

edited by auvred

netlify bot commented Mar 10, 2024 •

edited

nx-cloud bot commented Mar 10, 2024 •

edited

codecov bot commented Mar 10, 2024 •

edited

yeonjuan commented Mar 10, 2024 •

edited

developer-bandi commented Mar 10, 2024 •

edited

yeonjuan Apr 6, 2024 •

edited

DigitalLeonPro commented Apr 30, 2024 •

edited

HitkoDev commented Apr 30, 2024 •

edited

JoshuaKGoldberg commented Apr 30, 2024 •

edited