Enhancement: Improve lint error report location #8543

yeonjuan · 2024-02-23T18:12:13Z

Before You File a Proposal Please Confirm You Have Done The Following...

I have searched for related issues and found none that match my proposal.
I have searched the current rule list and found no rules that match my proposal.
I have read the FAQ and my problem is not listed.

Relevant Package

typescript-eslint

My proposal is suitable for this project

I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).

Description

Some rule's error report location seems too wide. I think it could be improved to report more accurate location where user should care about.For example,

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

// AS-IS
let unusedVar: { foo: number } | undefined = 1;
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// TO-BE
let unusedVar: { foo: number } | undefined = 1;
     ^^^^^^^^

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

// AS-IS
class Animal {
   get name(): string {  return this.animalName;  }
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
}
// TO-BE
 class Animal {
   get name(): string {  return this.animalName;  }
   ^^^^^^^^^
}

no-for-in-array fix(eslint-plugin): [no-for-in-array] refine report location #8874

// AS-IS
declare const array: string[];
for (const i in array) { console.log(array[i]); }
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// TO-BE
declare const array: string[];
for (const i in array) { console.log(array[i]); }
^^^^^^^^^^^^^^^^^^^^^

init-declarations fix(eslint-plugin): [init-declarations] refine report locations #8893

// AS-IS
let bar: number | undefined | string;
   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
bar = 2;
// TO-BE
let bar: number | undefined | string;
   ^^^^^
bar = 2;

prefer-readonly fix(eslint-plugin): [prefer-readonly] refine report locations #8894

// AS-IS
class Container {
  private onClick = () => { /* ... */ };
 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
}
// TO-BE
class Container {
  private onClick = () => { /* ... */ };
  ^^^^^^^^^^^^^^^
}

~~space-before-blocks~~ (formatting/layout rule)

... maybe more

Additional Info

No response

Josh-Cena · 2024-02-23T19:37:31Z

Some may be harder than others (for example, the no-for-in-array one—for (const i in array) is not an AST node) but definitely welcoming PRs for all of them

kirkwaiblinger · 2024-04-10T03:48:03Z

related: #8597

kirkwaiblinger · 2024-04-10T06:29:23Z

I think no-unused-vars could be added to the list, with the same pattern as init-declarations:

// current report
let unusedVar: Really & Long & { Type: 'Annotation' }; 
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// proposed
let unusedVar: Really & Long & { Type: 'Annotation' }; 
    ^^^^^^^^^

More generally, this is probably a pattern that applies often (always?) if there are other cases when a VariableDeclarator is reported by a base rule (without an init), since the type annotation is probably never relevant to the reason for the report coming from a base rule. I wonder if we could handle this automagically across all the rules rather than thinking of them one at a time.

(example for why the absence of init is relevant)

/* eslint init-declarations: ["error", "never"] */
let unusedVar: Really & Long & { Type: 'Annotation' } = 'this configuration forbids initialization.'; 
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

bradzacher · 2024-04-10T08:15:13Z

Note that we don't particularly want to break from eslint core too much in the case of extension rules.

If eslint reports on the variable and the init - we should too. If they report on just the variable name - then sure it makes sense for us to limit it to reduce noise!

kirkwaiblinger · 2024-04-10T15:40:39Z

Note that we don't particularly want to break from eslint core too much in the case of extension rules.

If eslint reports on the variable and the init - we should too. If they report on just the variable name - then sure it makes sense for us to limit it to reduce noise!

Yeah, good point, agreed. We could posit a guiding principle for dealing with the base rules that "we will generally only modify the report loc if there are multiple ways that it could be mapped onto TS syntax". Examples being, as above, highlighting the name of a variable could be construed to include or not include the type annotation, whereas it's unambiguous when the init is present and included in the report. Another example might be that class members could be refined to include or not include accessibility modifiers.

kirkwaiblinger · 2024-04-12T06:30:16Z

Another one:

/* eslint @typescript-eslint/no-non-null-assertion: "error" */
declare const long: any;
// current
long.long.nested.thing.that.ends.in.a.non.null.assertion!
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
// proposed
long.long.nested.thing.that.ends.in.a.non.null.assertion!
                                                        ^

bradzacher · 2024-04-12T07:03:17Z

The complicated thing with that one is that it means you have a single-character report range. That can sometimes be hard to spot as a user and thus can be frustrating.

kirkwaiblinger · 2024-04-15T04:52:19Z

how about this one?

// eslint @typescript-eslint/consistent-type-assertions: ['error', { assertionStyle: 'as' } ]
<T>x;
^^^
// eslint @typescript-eslint/consistent-type-assertions: ['error', { assertionStyle: 'angle-bracket' } ]
x as T;
  ^^^^
// or, another option
x as T;
  ^^
  
// (but no change to the report loc for the objectLiteralTypeAssertions reports, i.e. keep the following)
{} as T;
^^^^^^^

bradzacher · 2024-04-15T14:09:03Z

One thing I am personally a fan of is over-highlighting if it highlights the code that would be fixed by the rule.

For example if you highlight

x as T;
  ^^

Then one might be fooled into thinking that the autofixer / suggestion fixers might solely act on the as. However that's not the case because the fix removes the as and relocates and wraps the T.

If you're ADDING code then the highlight doesn't matter - but if you're changing existing code then often times highlighting the fixable range can provide good signal to the user as to what the fixer acts on.

There's the trade-off, of course, but I think that it can have value as long as there's not too much visual noise.

kirkwaiblinger · 2024-04-15T14:32:05Z

One thing I am personally a fan of is over-highlighting if it highlights the code that would be fixed by the rule.

For example if you highlight
x as T;
  ^^
Then one might be fooled into thinking that the autofixer / suggestion fixers might solely act on the as. However that's not the case because the fix removes the as and relocates and wraps the T.

Well, in this case there's no autofixer at all for the as T -> <T> case 😅 But yeah I agree with you here.

Also, while I was thinking about these, i realized that it doesn't really matter too too much what the report loc is for autofixable rules, or, more generally, reports that are likely to be short-lived, since they'll just be seen once and addressed. In my experience, just a small subset of rules end up being left around long-term as a warning that people are too afraid to fix or to ignore, and those are the ones that are probably the most impactful to refine. I'm thinking:

no-floating-promises
no-misused-promises
require-await (this one already has pretty good report locations from what I've tested)
no-unnecessary-condition
prefer-nullish-coalescing

the promise ones specifically would probably be the most impactful i 8B0D n the codebases I've worked in, but, also, unfortunately, the least obvious IMO how to shorten 🤔

yeonjuan added enhancement New feature or request triage Waiting for team members to take a look labels Feb 23, 2024

Josh-Cena added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for team members to take a look labels Feb 23, 2024

developer-bandi mentioned this issue Mar 10, 2024

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

Merged

3 tasks

This was referenced Apr 7, 2024

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

Merged

fix(eslint-plugin): [no-for-in-array] refine report location #8874

Merged

This was referenced Apr 10, 2024

fix(eslint-plugin): [init-declarations] refine report locations #8893

Merged

fix(eslint-plugin): [prefer-readonly] refine report locations #8894

Merged

kirkwaiblinger mentioned this issue Apr 15, 2024

Rule proposal: break up consistent-type-assertions into consistent-type-assertion-syntax and no-unsafe-object-literal-type-assertion #8921

Closed

6 tasks

Josh-Cena added the package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin label Jun 2, 2024

JoshuaKGoldberg closed this as completed in #8894 Jun 17, 2024

github-actions bot added the locked due to age Please open a new issue if you'd like to say more. See https://typescript-eslint.io/contributing. label Jun 25, 2024

github-actions bot locked as resolved and limited conversation to collaborators Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: Improve lint error report location #8543

Enhancement: Improve lint error report location #8543

Enhancement: Improve lint error report location #8543

Enhancement: Improve lint error report location #8543

Comments

Before You File a Proposal Please Confirm You Have Done The Following...

Relevant Package

My proposal is suitable for this project

Description

Additional Info