Skip to content
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

Enhancement: Improve lint error report location #8543

Open
6 of 9 tasks
yeonjuan opened this issue Feb 23, 2024 · 10 comments · May be fixed by #8894
Open
6 of 9 tasks

Enhancement: Improve lint error report location #8543

yeonjuan opened this issue Feb 23, 2024 · 10 comments · May be fixed by #8894
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request

Comments

@yeonjuan
Copy link
Contributor

yeonjuan commented Feb 23, 2024

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

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,

... maybe more

Additional Info

No response

@yeonjuan yeonjuan added enhancement New feature or request triage Waiting for maintainers to take a look labels Feb 23, 2024
@Josh-Cena Josh-Cena added accepting prs Go ahead, send a pull request that resolves this issue and removed triage Waiting for maintainers to take a look labels Feb 23, 2024
@Josh-Cena
Copy link
Member

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
Copy link
Member

related: #8597

@kirkwaiblinger
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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
Copy link
Member

kirkwaiblinger commented Apr 15, 2024

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
Copy link
Member

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
Copy link
Member

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 in the codebases I've worked in, but, also, unfortunately, the least obvious IMO how to shorten 🤔

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accepting prs Go ahead, send a pull request that resolves this issue enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants