Skip to content

Possibility of writing "no-useless-ts-ignore-comments" rule #236

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

Closed
mohsen1 opened this issue Feb 9, 2019 · 10 comments
Closed

Possibility of writing "no-useless-ts-ignore-comments" rule #236

mohsen1 opened this issue Feb 9, 2019 · 10 comments
Labels
external This issue is with another package, not typescript-eslint itself package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin

Comments

@mohsen1
Copy link
Contributor

mohsen1 commented Feb 9, 2019

I'm exploring if it's possible to write a rule that warns against // @ts-ignore comments that have become stale as result of changes in other places of even on the line that tsc is being ignored. My approach is to get all diagnostics and cross reference them with // @ts-ignore comments and see which comment does not actually ignoring an error. But if the // @ts-ignore comment is still there TypeScript will not report any diagnostics for the next line.

Is there any way of getting diagnostics if we remove the // @ts-ignore comment?

One solution is to remove all // @ts-ignore comments from source files of existing program, construct a new program with those source files and get diagnostics. This seems very very expensive

Any thoughts on this?

Versions

package version
@typescript-eslint/eslint-plugin 1.3.0
TypeScript 3.0
@mohsen1 mohsen1 added package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin triage Waiting for team members to take a look labels Feb 9, 2019
@bradzacher bradzacher added question Questions! (i.e. not a bug / enhancment / documentation) enhancement: new plugin rule New rule request for eslint-plugin and removed triage Waiting for team members to take a look labels Feb 9, 2019
@bradzacher
Copy link
Member

it might be worth looking at https://github.com/mysticatea/eslint-plugin-eslint-comments
It does a similar thing except with eslint comments.

Glancing at that code makes me suspect this is something we can't handle purely within the plugin?

@platinumazure
Copy link
Contributor

Just spitballing: It seems like it could be possible if TSC includes all errors/warnings, including the ignored warnings under a different object property. Then it would be easy to check those warnings' locations against the TS ignore comments and see if any of the ignore comments have no corresponding warnings. But not sure if that's the intended/ideal direction, or if TSC should be handling this.

@bradzacher
Copy link
Member

The problem there is that we don't attach comments to nodes, and eslint deprecated the functionality to do that. #158 (comment)

So whilst we know about the comments, we don't know what node they're attached to (unless tsc exposes that information). This means we don't know which node to check for errors (even if tsc did surface the ignored errors for us).

For that reason alone, I would say that this rule would be non-trivial to implement.

@platinumazure
Copy link
Contributor

@bradzacher You wouldn't need to use comment attachment- you would just need to compare range or location information. That said, I agree that this would, in general, not be trivial to implement.

@mohsen1
Copy link
Contributor Author

mohsen1 commented Feb 10, 2019

//@ts-ignore comments ignore all type errors on next line. Something like this is possible:

// @ts-ignore
let a: string = 1; let b: number = 'str'

I agree with @platinumazure that TSC should provide information about ignored diagnostics so we can cross check them with the comments. Made an issue for that in TypeScript:
microsoft/TypeScript#29843

kaicataldo pushed a commit to kaicataldo/typescript-eslint that referenced this issue Aug 27, 2019
@JoshuaKGoldberg
Copy link
Member

JoshuaKGoldberg commented Jan 25, 2020

Relevant: microsoft/TypeScript#36014 will add a // @ts-expect-error to TypeScript. We could make this rule instead just ban // @ts-ignore altogether.

Edit: I just noticed ban-ts-comment already does that! 😄

@bradzacher
Copy link
Member

It's already a rule (as of a week or so)! Just missed this issue.
https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/docs/rules/ban-ts-comment.md

@mohsen1
Copy link
Contributor Author

mohsen1 commented Jan 25, 2020

This is different than what I suggest. I wanted to make sure we don’t have dead ts-ignore directives. Banning it altogether is not the same.

This is useful when you have an issue that you can’t resolve and you use ts ignore to surpass it but then later via some mechanism like dependency update the type error is no longer there but the ts-ignore comment is there and it doesn’t allow ts to check the next line.

@armano2 armano2 reopened this Jan 25, 2020
@bradzacher
Copy link
Member

bradzacher commented Jan 26, 2020

@bradzacher
Copy link
Member

This can be closed now, as microsoft/TypeScript#36014 has been merged, and will be released as part of the next TS version.

You'll have to migrate your codebases to use ts-expect-error when it's released, and then any comments that don't cause a TS error will themselves cause a TS error.

@bradzacher bradzacher added external This issue is with another package, not typescript-eslint itself and removed enhancement: new plugin rule New rule request for eslint-plugin question Questions! (i.e. not a bug / enhancment / documentation) labels Mar 6, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
external This issue is with another package, not typescript-eslint itself package: eslint-plugin Issues related to @typescript-eslint/eslint-plugin
Projects
None yet
Development

No branches or pull requests

5 participants