Added @ts-expect-error to @ts-ignore directives #36014
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoshuaKGoldberg
force-pushed
the
ts-expect-error
branch
from
152655c
to
9918231
Compare
Similar to `// @ts-ignore`, but will itself cause a new error diagnostic if it does not cause an existing diagnostic to be ignored. Technical summary: 1. The scanner will now keep track of `CommentDirective`s it comes across: both `@ts-expect-error` and `@ts-ignore` 2. During type checking, the program will turn those directives into a map keying them by line number 3. For each diagnostic, if it's preceded by a directive, that directive is marked as "used" 4. All `@ts-expect-error` directives not marked as used generate a new diagnostic error
JoshuaKGoldberg
force-pushed
the
ts-expect-error
branch
from
9918231
to
eb5610a
Compare
|
I'd like to try this in a playground: @typescript-bot perf test |
|
Heya @orta, I've started to run the perf test suite on this PR at eb5610a. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
|
@orta Here they are:Comparison Report - master..36014
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Re that On-Demand Benchmark test failure: Monaco check time went up 7.14% (line link). but emit time decreased by almost as much (31s out of 36s), and the total time went up only by 0.57%:
...so this feels like it's not a serious performance degradation, and this PR is still valid? 😁 |
|
This is great! @JoshuaKGoldberg I've also been toying around with implementing this. However, I was making I added the And then I parse it in ( While I couldn't find a formal definition of "pragmas" in programming languages, in some programming languages, a "pragma" can be in places other than just the top of the file (https://dlang.org/spec/pragma.html). Here's my in progress diff: https://gist.github.com/davidgomes/5cb71342b6d2cf95ffa05a767eb802c7 |
|
I’d love to have something more powerful: //@ts-expect-error 2322 "Type 'number' is not assignable to type 'never'."Maybe with the option to omit trailing characters of the message and/or the error number. The former could be achieved via regular expressions: //@ts-expect-error 2322 /^Type 'number' is not assignable to type 'never'\.$/Rationale: It’s not only a problem to expect errors where there aren’t any, but also to expect the wrong kind of error. |
|
Re-running perf tests, I'm not really convinced this could add 7% after reading the PR @typescript-bot perf test |
|
Heya @orta, I've started to run the perf test suite on this PR at 7a5ef8a. You can monitor the build here. Update: The results are in! |
|
Hey @orta, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build. |
|
@orta Here they are:Comparison Report - master..36014
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
These numbers make more sense to me - basically everything is varying in the 0.x range on a plus or minus which is basically just normal noise in perf tests. |
orta
approved these changes
Mar 4, 2020
|
This would make it possible to deprecate It would also reduce noise in IDEs, as |
|
OK, this looks good to go - thanks! I don't know if it's in TypeScript, or in vscode, but we should also get The playground doesn't show any of those options. |
|
👍 It's in vscode, in a list of directives: microsoft/vscode#92093 |
|
I love this! Does this mean we can use |
|
I think so, yep |
This was referenced Mar 5, 2020
|
As someone else already mentioned, I second that it would be nice to be able to specify exactly what error to expect (by its id/code etc). I've only used ts-ignore a few times but have already ran into the problem where I think I'm ignoring one error just to have it transform into another completely unrelated error however, unknowingly, since this transformation happened in "the meantime" while the ts-ignore directive was still active/in-place the whole time. So the new error that the old error "transformed into" went completely unnoticed from what I remember (until I removed the directive of course, which was when I found out about the change). Now I try to just avoid ts-ignore altogether because of such potential situations that may arise because it's just not worth taking that kind of a risk imo. |
|
related to @Tyler1205 's comment I would like to be able to specify number of errors, and that might be easier to handle. |
I agree, this is really needed as you can never know if you are blocking more than 1 error, or the wrong kind of error |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Similar to
// @ts-ignore, but will itself cause a new error diagnostic if it does not cause an existing diagnostic to be ignored.Technical summary:
CommentDirectives it comes across: both@ts-expect-errorand@ts-ignore@ts-expect-errordirectives not marked as used generate a new diagnostic errorFixes #29394