Repository navigation
fix(64231): avoid parsing async() calls as typed arrows in conditionals - #64233
Oleksandr Tarasiuk (a-tarasyuk) wants to merge 12 commits into
Conversation
typescript-automation
Bot
added
the
For Uncommitted Bug
There was a problem hiding this comment.
🟢 Approval recommended
The parser change is narrowly scoped and is backed by targeted regression tests plus updated baselines covering the reported ambiguity and key variants.
Pull request overview
Fixes a long-standing parsing ambiguity where async() inside conditional (?:) expressions could be misread as the start of an async arrow with a return type annotation, producing spurious syntax errors in valid code.
Changes:
- Adjusts the parser’s parenthesized-arrow lookahead so
async () : ...is treated as ambiguous (and therefore speculatively parsed) instead of being assumed to be an arrow. - Adds regression coverage for
a ? async() : ...patterns in both TS and JS (allowJs/checkJs) test suites, including nested conditionals and mixed arrow branches. - Updates the corresponding reference baselines (types/symbols/errors and JS emit) to reflect the corrected parse.
File summaries
| File | Description |
|---|---|
| tsc/internal/parser/parser.go | Makes async () : return TSUnknown in lookahead to avoid misclassifying ternary : as an arrow return-type :. |
| tsc/testdata/tests/cases/conformance/async/es2017/asyncArrowFunction/asyncArrowFunction_allowJs.ts | Adds allowJs regression cases for conditional expressions involving async() calls. |
| tsc/testdata/tests/cases/compiler/parenthesizedAsyncArrowFunction.ts | Adds compiler regression cases covering the reported conditional parsing scenarios and related edge cases. |
| tsc/testdata/baselines/reference/conformance/asyncArrowFunction_allowJs.types | Baseline updates for the new allowJs conditional parsing cases. |
| tsc/testdata/baselines/reference/conformance/asyncArrowFunction_allowJs.symbols | Symbol baseline updates for the new allowJs cases. |
| tsc/testdata/baselines/reference/conformance/asyncArrowFunction_allowJs.errors.txt | Confirms the new allowJs cases introduce no additional diagnostics. |
| tsc/testdata/baselines/reference/compiler/parenthesizedAsyncArrowFunction.types | Type baseline updates for the new compiler regression cases. |
| tsc/testdata/baselines/reference/compiler/parenthesizedAsyncArrowFunction.symbols | Symbol baseline updates for the new compiler regression cases. |
| tsc/testdata/baselines/reference/compiler/parenthesizedAsyncArrowFunction.js | Emit baseline updates demonstrating correct parsing/emit around the new conditional cases. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
|
TypeScript Bot (@typescript-bot) test top800 |
|
Daniel Rosenwasser (@DanielRosenwasser) Here are the results of running the top 800 repos with tsc comparing Everything looks good! |
typescript-automation
Bot
added
For Milestone Bug
|
I had copilot look at this and it caught some cases I missed: This looks correct for the reported examples. Could we also cover spread arguments? The const xs = [1, 2];
const spread1 = a ? async(...xs) : 0;
const spread2 = a ? async(...xs) : x => x;Both still fail on this branch. It looks like the spread lookahead could similarly return const restArrow = a
? async (...xs: number[]): Promise<number> => xs.length
: () => 0;There is also a remaining nested case worth covering specifically in the JavaScript test: const nested1 = a ? a ? async() : x => x : 0;
const nested2 = a ? a ? async(1) : x => x : 0;Node and esbuild's JS parser accept both, but this branch reports a spurious type annotation and These are pre-existing failures, not regressions introduced here, so the nested-JS issue could reasonably be a follow-up. |
472e30e to
3458355
Compare
ef5955a to
b647349
Compare
b647349 to
a1110c5
Compare
Jake Bailey (jakebailey)
left a comment
Jake Bailey (jakebailey)
left a comment
There was a problem hiding this comment.
I had Copilot do a bunch of cross checking, and there are some interesting edge cases to think about here.
- No regressions: everything under
testdata/testsplusnode_moduleswas parsed withmainand with this PR (~80k parses across TS/TSX/JS). ASTs and diagnostics were identical. - No backtracking blowup: deeply nested
a ? async() : x => …chains (up to 40 levels) parse in the same time as onmain. - A bonus fix:
async(...a.b),async(...a())andasync(...[1])fail to parse onmaineven outside conditionals, and this PR fixes them. That might be worth a test and a mention in the description.
Here's how other parsers handle the same inputs, with r = in front of each expression:
| Input | TS 6.0 | main |
this PR | esbuild | swc | oxc | bun |
|---|---|---|---|---|---|---|---|
a ? async() : 0 |
❌ | ❌ | call | call | call | call | call |
a ? async() : x => x |
❌ | ❌ | call | ❌ | call | call | call |
a ? async(...c) : x => x |
❌ | ❌ | call | ❌ | call | ❌ | call* |
a ? async(): number => 1 : 2 |
arrow | arrow | arrow | arrow | arrow | arrow | arrow |
a ? (c): T => (d) : x => x |
arrow | arrow | arrow | call | arrow | arrow | call |
a ? async(): T => (d) : x => x |
❌ | ❌ | call | ❌ | arrow | arrow | call |
a ? async (): T => async() : x => x |
❌ | ❌ | call | ❌ | arrow | arrow | call |
a ? async(): x => async() : y |
❌ | ❌ | ❌ | arrow | arrow | arrow | arrow |
async(...a.b) |
❌ | ❌ | ok | ok | ok | ❌ | ok |
(* Bun drops the spread here and emits async(c). That's a Bun bug.)
"call" means async(...) was parsed as a call in the true branch; "arrow" means a typed async arrow. Versions: TypeScript 6.0.3, main at 98add1bbed1, esbuild 0.28.2, swc 1.16.18, oxc-transform 0.155.0, bun 1.4.3.
The cases from the issue are fixed. The ambiguous typed cases have no consensus between tools, but a couple of them are inconsistent with how this PR treats the parenthesized form. See the inline comments.
We need to think about these edge cases and decide what behavior we actually want, ideally captured in tests, rather than leaving it to whatever falls out of the current heuristics.
Nit: could the description mention the async(...a.b) fix outside conditionals, and the jsdocDiagnostics/reparseList mark/rewind change? Both are behavior changes beyond the issue.
| parameters = p.createMissingList() | ||
| } else { | ||
| if isAsync && typeParameters == nil && (p.token == ast.KindCloseParenToken || p.token == ast.KindDotDotDotToken) { | ||
| bodyFlags = flags & ExpressionParseFlagsAllowAmbiguousAsyncArrow |
There was a problem hiding this comment.
This removes DisallowArrowReturnType from the body of async()/async(...) arrows, so they resolve differently from the parenthesized equivalent:
a ? (c): T => (d) : x => x; // arrow in true branch: a ? ((c): T => (d)) : (x => x)
a ? async(): T => (d) : x => x; // call: a ? async() : (T => (d) => x)Both readings are valid TS. Still, the existing rule for parenthesized arrows prefers the arrow in the true branch when a : follows, and swc and oxc pick the arrow for both. Is this difference intentional, perhaps to get f17 in the test to parse as a call?
Also, this assignment only has an effect when requiresColon is true, because the => branch below overwrites bodyFlags otherwise. Moving it into that branch would make that clearer.
| return result | ||
| } | ||
|
|
||
| func (p *Parser) nextTokenIsStartOfArrowFunction() bool { |
There was a problem hiding this comment.
Since this is a lookahead heuristic, the result depends on how the return type is spelled:
a ? async (): Promise<number> => async() : x => x; // arrow (matches f27)
a ? async (): T => async() : x => x; // call: a ? async() : (T => async(): x => x)Both are valid TS, but changing the type name shouldn't change which reading we pick.
| if lastToken == ast.KindEqualsGreaterThanToken || lastToken == ast.KindOpenBraceToken { | ||
| body = p.parseArrowFunctionExpressionBody(isAsync, allowReturnTypeInArrowFunction) | ||
| if requiresColon { | ||
| bodyFlags |= ExpressionParseFlagsAllowAmbiguousAsyncArrow |
There was a problem hiding this comment.
This input still fails to parse, although it's valid TS:
a ? async(): x => async() : y;
// error TS1005: ';' expectedesbuild, swc, oxc and bun all parse it as a ? (async(): x => async()) : y. The parenthesized version a ? (c): x => (d) : y already works. The cause seems to be the flag on the outer body: it makes the inner async() : y count as a definite arrow, the outer arrow then finds no : after its body, and gets rejected. main fails on this too, so it's not a regression. It might be worth a test, even if it's only recorded as a known limitation.
| assert.Equal(t, missingImplementsDecl.HeritageClauses.Nodes[0].AsHeritageClause().Types.Nodes[0].Kind, ast.KindExpressionWithTypeArguments) | ||
| } | ||
|
|
||
| func TestConditionalAsyncCall(t *testing.T) { |
There was a problem hiding this comment.
Most of these ~590 lines of unit tests repeat what parenthesizedAsyncArrowFunction{,Js}.ts already covers. Could some of them become baseline cases instead? That's the usual pattern here and easier to review. Keeping only the ones that check things the baselines can't show (e.g. YieldContext flags, JSDoc rewind) seems enough.
| if p.parsingFlags&ParsingFlagsTentative != 0 { | ||
| panic(tentativeParseFailure{}) |
Fixes #64231