Fixed false positive circular errors for await expressions with simple non-generic calls in CFA loops #51126
Conversation
…e non-generic calls in CFA loops
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| if (isAwaitExpression(expr)) { | ||
| const type = getQuickTypeOfExpression(expr.expression); | ||
| return type ? getAwaitedType(type) : undefined; | ||
| } |
There was a problem hiding this comment.
this is the core of the fix
src/compiler/checker.ts
Outdated
| if (type) { | ||
| return type; | ||
| } |
There was a problem hiding this comment.
just removed a somewhat redundant lines, replaced that with direct return in the statement above this
src/compiler/checker.ts
Outdated
| } | ||
| else if (isAssertionExpression(expr) && !isConstTypeReference(expr.type)) { | ||
| return getTypeFromTypeNode((expr as TypeAssertion).type); | ||
| } | ||
| else if (node.kind === SyntaxKind.NumericLiteral || node.kind === SyntaxKind.StringLiteral || | ||
| node.kind === SyntaxKind.TrueKeyword || node.kind === SyntaxKind.FalseKeyword) { | ||
| else if (isLiteralExpression(node)) { |
There was a problem hiding this comment.
a bonus optimization~ (?), we now handle here all literal expressions - instead of just a subset of them
| @@ -0,0 +1,137 @@ | |||
| // @strict: true | |||
There was a problem hiding this comment.
I basically just copy-pasted existing controlFlowIterationErrors test case and I converted all of the functions to be async.
In reality, only the last two cases changed behavior with this PR so maybe I should limit what had been added to this test case? Either way, happy to clean up if needed - for now I've left those alone.
| } | ||
| } | ||
|
|
||
| // repro #51115 |
There was a problem hiding this comment.
Here, we have the new test cases - based on the reported issues. They are very same-ish, but I've figured out that it still might be worthwhile to commit all of them.
| @@ -39,7 +39,6 @@ interface A { | |||
| function foo(x: A): string { return "abc"; } | |||
|
|
|||
| class C { | |||
| // Error expected | |||
There was a problem hiding this comment.
this was outdated, no error is being reported here (and no error has to be reported here)
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at d234453. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at d234453. You can monitor the build here. |
|
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at d234453. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the abridged perf test suite on this PR at d234453. You can monitor the build here. Update: The results are in! |
|
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at d234453. You can monitor the build here. Update: The results are in! |
|
Hey @DanielRosenwasser, 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 and an npm module you can use via |
|
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
|
@DanielRosenwasser Here they are:Comparison Report - main..51126
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
DanielRosenwasser
requested review from
ahejlsberg,
andrewbranch and
rbuckton
typescript-bot
added
For Backlog Bug
There was a problem hiding this comment.
Without knowing much about this code path, I think this is reasonable. The only risk is the call to getAwaitedType which I think has re-entrance issues - but that issue is open at #42948.
With another sign-off, I would be okay with merging for 4.9, as it feels like a bug-fix.
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
|
@DanielRosenwasser huh, luckily... I've fixed that particular issue a couple of months ago in this PR :) I've created a PR with an extra test case that will close that issue: #51167 |
DanielRosenwasser
added
the
Merge/Review for Next Release
|
@DanielRosenwasser is this ready to merge now? (Once it's up-to-date with main) |
|
You probably want to get @ahejlsberg to take a look at it in case I missed anything. |
# Conflicts: # src/compiler/checker.ts
|
@sandersn it's up to date now :) |
|
friendly 🏓 since this has been labeled with "Merge/Review for Next Release" and the 5.0 Beta is going to be published in 2 weeks |
src/compiler/checker.ts
Outdated
| } | ||
| else if (isAssertionExpression(expr) && !isConstTypeReference(expr.type)) { | ||
| return getTypeFromTypeNode((expr as TypeAssertion).type); | ||
| } | ||
| else if (node.kind === SyntaxKind.NumericLiteral || node.kind === SyntaxKind.StringLiteral || | ||
| node.kind === SyntaxKind.TrueKeyword || node.kind === SyntaxKind.FalseKeyword) { | ||
| else if (isLiteralExpression(node)) { |
There was a problem hiding this comment.
Why did this change? isLiteralExpression doesn't cover TrueKeyword/FalseKeyword so this isn't an equivalent change.
There was a problem hiding this comment.
Sorry for this - I probably assumed that this already covers TrueKeyword/FalseKeyword. Tests pass without this else if branch - so this was probably an optimization or something (it's not an unused code).
I brought back the check for those keywords.
I don't exactly recall why I changed it in the first place - perhaps just to cover for all literal types as that seemed like the right thing to do at the time. If you want me to revert this change entirely, let me know.
There was a problem hiding this comment.
If it's not relevant to the bug you're fixing, I'd revert the change. The fact that changing it didn't affect any tests makes me more reticent to take the change, since we don't know what we'd be breaking.
Andarist
requested review from
rbuckton
and removed request for
andrewbranch and
ahejlsberg
|
@rbuckton reverted those unrelated bits |
|
Awesome to see this fixed and merged, thank you @Andarist! |
fixes #51115
cc @jeremy-rifkin @garyo @bvallee-thefork