Repository navigation
fix(64231): avoid parsing async() calls as typed arrows in conditionals - #64233
Oleksandr Tarasiuk (a-tarasyuk) wants to merge 2 commits into
Conversation
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! |
|
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
| } | ||
| state := p.mark() | ||
| result := p.parsePossibleParenthesizedArrowFunctionExpression(allowReturnTypeInArrowFunction) | ||
| result := p.parsePossibleParenthesizedArrowFunctionExpression(flags) |
There was a problem hiding this comment.
Maybe I'm missing something, but isn't flags here only being used to pass two bools, and this has nothing to do with the existing meaning of parse flags? Should this be a different set of flags? Or not passed as an arg at all? (I remember when I added this bool arg I did an arg on purpose because it was challenging to remember to clear at the right time but maybe that time has passed)


Fixes #64231