fix: arrow/async in conditional expressions for TS#17572
fix: arrow/async in conditional expressions for TS#17572liuxingbaoyu wants to merge 2 commits intobabel:mainfrom
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/60120 |
| if (!this.eat(tt.colon)) { | ||
| state.noArrowAt = this.state.noArrowAt; | ||
| if (state.noArrowAt.length <= 3) { | ||
| state.noArrowAt = [state.noArrowAt[0]]; | ||
| } else { | ||
| state.noArrowAt = state.noArrowAt.slice(3); | ||
| } | ||
| this.state = state; | ||
| node.consequent = this.parseMaybeAssignAllowIn(); | ||
| this.expect(tt.colon); | ||
| } |
There was a problem hiding this comment.
This was inspired by the Flow plugin, thanks to @nicolo-ribaudo.
The current implementation doesn't look reliable at all, but it passes all the tests.🤦♂️
If we try them one by one, it will be more reliable, but slower.
|
commit: |
| @@ -0,0 +1 @@ | |||
| true ? async (): true; | |||
There was a problem hiding this comment.
Although it is valid JavaScript, tsc does not support it either:
https://www.typescriptlang.org/play/?#code/C4JwrgpgBA-FCGBnAngOwMZQBQEoBcUokA3EA
Could we skip this test temporarily and enable it when we align to the tsc behaviour?
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
I don;t love this, but it looks ok. It'd be great to see some benchmarks, but we should prioritize correctness over performance anyway.
…s from babel#17572 Since the `?` token is already verified at the start of parseConditional, use `this.next()` to consume it instead of redundantly checking with `this.expect()`. Also add additional conditional expression test fixtures ported from babel#17572 to improve coverage of arrow/async ambiguity edge cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
These test cases document known limitations of the current fix: - arrow-ambiguity, arrow-like, arrow-param: Non-async arrow disambiguation in ternary expressions (out of scope for this PR) - async-call, async-call-3: Edge cases where async call parsing interacts with nested arrow/paren contexts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…17799) * Fix TypeScript parser failing on async calls inside ternary consequent When the TypeScript plugin encounters `async(a)` followed by `:` inside a ternary consequent, it incorrectly treats `:` as a return type annotation for an async arrow function, rather than the ternary separator. For example, `true ? async(a) : b => b` should parse as a ternary with `async(a)` as the consequent (a call expression) and `b => b` as the alternate (an arrow function). Instead, babel tried to parse `async(a): b => b` as an async arrow with return type `b`. This commit: - Adds an `inConditionalConsequent` state flag set during ternary consequent parsing - When the flag is active, `shouldParseAsyncArrow` no longer treats `:` as a return type annotation start - Resets the flag inside parenthesized expressions, so `a ? (async(b): T => body) : c` still works correctly Fixes #17547 * Address review: use this.next() instead of this.expect() and add tests from #17572 Since the `?` token is already verified at the start of parseConditional, use `this.next()` to consume it instead of redundantly checking with `this.expect()`. Also add additional conditional expression test fixtures ported from #17572 to improve coverage of arrow/async ambiguity edge cases. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * Add expected output files for test fixtures from #17572 These test cases document known limitations of the current fix: - arrow-ambiguity, arrow-like, arrow-param: Non-async arrow disambiguation in ternary expressions (out of scope for this PR) - async-call, async-call-3: Edge cases where async call parsing interacts with nested arrow/paren contexts Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: replace throws expectations with output.json for async-call fixtures The async-call and async-call-3 test fixtures had "throws" directives in options.json expecting parse errors, but the parser fix now correctly handles these cases. Replace with output.json files containing expected ASTs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
I initially tried to port the TS implementation, but failed.
Maybe I should try again.