Repository navigation
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 (typescript-bot)
added
the
For Uncommitted Bug
| 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
| 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
| } | ||
| 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.
| 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 (@typescript-bot) pack this |
|
Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the extended test suite on this PR at d234453. You can monitor the build here. |
|
Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the tarball bundle task on this PR at d234453. You can monitor the build here. |
|
Heya Daniel Rosenwasser (@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 Daniel Rosenwasser (@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 Daniel Rosenwasser (@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 Daniel Rosenwasser (@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 |
|
Daniel Rosenwasser (@DanielRosenwasser) Here are the results of running the user test suite comparing Everything looks good! |
|
Daniel Rosenwasser (@DanielRosenwasser) Here they are:Comparison Report - main..51126
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
TypeScript Bot (typescript-bot)
added
For Backlog Bug
Daniel Rosenwasser (DanielRosenwasser)
left a comment
Daniel Rosenwasser (DanielRosenwasser)
left a comment
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.
|
Daniel Rosenwasser (@DanielRosenwasser) Here are the results of running the top-repos suite comparing Everything looks good! |
|
Daniel Rosenwasser (@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 |
Daniel Rosenwasser (DanielRosenwasser)
added
the
Merge/Review for Next Release
|
Daniel Rosenwasser (@DanielRosenwasser) is this ready to merge now? (Once it's up-to-date with main) |
|
You probably want to get Anders Hejlsberg (@ahejlsberg) to take a look at it in case I missed anything. |
# Conflicts: # src/compiler/checker.ts
|
Nathan Shively-Sanders (@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 |
| } | ||
| 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.
|
Ron Buckton (@rbuckton) reverted those unrelated bits |
|
Awesome to see this fixed and merged, thank you Mateusz Burzyński (@Andarist)! |
7 participants
fixes #51115
cc Jeremy Rifkin (@jeremy-rifkin) Gary Oberbrunner (@garyo) Benoit Vallée (@bvallee-thefork)