Repository navigation
Conversation
WalkthroughChangesThe parser now propagates ternary context through nested expressions, uses speculative state-restoring checks to distinguish TypeScript arrow return types, and adds regression tests for ambiguous and malformed syntax. TypeScript ternary parsing
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 9:58 PM PT - Jul 16th, 2026
❌ @autofix-ci[bot], your commit c8eb496 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34252That installs a local version of the PR into your bun-34252 --bun |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js_parser/parse/mod.rs`:
- Around line 661-728: Update
is_typescript_arrow_return_type_after_question_and_before_colon to snapshot the
symbol table length before speculative parsing and truncate/restore it alongside
the existing lexer, log, and scope state. Ensure all bindings created by
parse_arrow_body are removed on both success and failure before the helper
returns.
In `@test/bundler/transpiler/transpiler.test.js`:
- Around line 156-198: Refactor the independent expectPrinted_ and
expectParseError cases in the disambiguates test into describe.each()-based
parameterized tables, with separate tables or case markers for printing and
parse-error expectations. Preserve the existing inputs, expected outputs, error
messages, and explanatory setup/comments while giving each case an individual
test result.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 50cc2889-8d23-45fe-a960-684596291f30
📒 Files selected for processing (6)
src/ast/expr.rssrc/js_parser/parse/mod.rssrc/js_parser/parse/parse_prefix.rssrc/js_parser/parse/parse_suffix.rssrc/js_parser/parser.rstest/bundler/transpiler/transpiler.test.js
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No new issues found after 5211ca3 — the fixes for the comments-buffer invariant, exponential backtracking memo, and scopes_in_order_for_enum rollback all look correct, and the decision to keep symbols un-truncated so stale Ref-keyed map entries stay unreachable is sound. That said, this is the first backtracking helper in the parser that speculatively parses a full arrow body (arbitrary statements) rather than just a TypeScript type, and the save/restore list took three review rounds to converge; a human should sign off on whether that list is now complete.
What was reviewed:
- Flag threading (
EFlags::AfterQuestionAndBeforeColon→sfx_t_question→pfx_t_open_paren→parse_paren_expr) and nested-ternary propagation to the alternate branch. - The speculative helper's save/restore against every parser field that
parse_arrow_body→parse_stmts_up_to→parse_stmtcan reach;scopes_in_order_for_enumis an insertion-orderedArrayHashMapso the.pop()rollback is correct. - Memoization: verdict keyed by the
:byte offset, re-searched after nested inserts, matches thets_infer_constraint_backtrackspattern. - Test matrix vs. esbuild's cases, including the legal-comment, enum-in-body, for-init
in, private-identifier, and n=25 nesting cases.
Extended reasoning...
Overview
Ports esbuild's fix for evanw/esbuild#4241: in TypeScript, a ? (b) : c => d is ambiguous between a ternary whose alternate is c => d and an arrow (b): c => d as the consequent. tsc picks the arrow only when another : follows the body. The PR threads a new EFlags::AfterQuestionAndBeforeColon from sfx_t_question through parse_prefix into parse_paren_expr, and when set, replaces the cheap lexer-only try_skip_type_script_arrow_return_type_with_backtracking with a new helper that speculatively parses ': Type => body' in full, checks for a trailing :, and unconditionally rewinds. Touches src/ast/expr.rs (new enum variant), src/js_parser/{p.rs,parser.rs} (new memo field / opts field), src/js_parser/parse/{mod.rs,parse_prefix.rs,parse_suffix.rs} (~130 net lines), and adds ~70 lines of test cases to transpiler.test.js.
Security risks
None. Parser-only change with no I/O, network, auth, or FFI surface. The one DoS-class concern (exponential backtracking on nested a ? (b) : T => ... : c) was found in an earlier review round and fixed with a byte-offset-keyed memo (ts_arrow_after_question_memo), covered by an n=25 test case.
Level of scrutiny
High. src/js_parser/parse/ is the core JS/TS parser — every file Bun transpiles goes through it. More importantly, this is the first backtracking helper that speculatively runs parse_arrow_body → parse_stmts_up_to, i.e. full statement parsing, and then rewinds. Every prior lexer_backtracker_* / try_skip_* helper only skips TypeScript types, so their save/restore lists never had to cover state that only statement parsing mutates. This PR's helper does, and the review history bears that out: three separate rounds found gaps (comments_to_preserve_before grow-only invariant, exponential re-parse, scopes_in_order_for_enum Loc-keyed debug_assert). Each was fixed and now has test coverage. The final design intentionally does not truncate symbols/ts_use_counts so speculatively-allocated Ref indices are never recycled, which keeps stale entries in Ref-keyed maps (ref_to_ts_namespace_member etc.) unreachable rather than requiring them all to be rolled back individually — a reasonable tradeoff, but one a maintainer should confirm.
Other factors
All prior review threads (CodeRabbit's symbol-table hygiene note, my three inline findings, and the dead is_async branch nit) are resolved. The is_async branch was intentionally kept to match esbuild verbatim. Test coverage is thorough and mirrors esbuild's matrix plus the regression cases from each review round. The memo insertion correctly re-searches for the insertion point after nested attempts. No new bugs surfaced in this run's bug-hunting pass. Given the criticality of the parser and the demonstrated subtlety of getting the save/restore list right, I'm deferring rather than approving.
|
Ran an adversarial self-review on this diff (36 concerns probed, 33 dismissed). Three survived and are addressed in 2797d6f:
The final state-restore list for the speculative body parse covers: lexer snapshot, log counts, |
There was a problem hiding this comment.
All four earlier inline findings (comments_to_preserve_before, exponential backtrack, scopes_in_order_for_enum, dead is_async branch) are addressed, and the self-review pass caught the import_records duplicate too. I didn't find anything new this round, but this is the first backtracker in the parser that speculatively parses a full arrow body (not just a type), so the hand-maintained save/restore list in is_typescript_arrow_return_type_after_question_and_before_colon is worth a human pass — three gaps in it were found across review iterations, and completeness rests on the "Ref-keyed maps stay unreachable because symbols isn't truncated" invariant.
Checked this run:
- Memo key uses
lexer.startat the:, re-searched after nested inserts so the sorted-vec invariant holds; both success and failure verdicts covered by the n=25 tests. EFlags::AfterQuestionAndBeforeColononly reachesparse_paren_exprviapfx_t_open_parenand theTLessThancast recursion; otherParenExprOptscall sites (async, force-arrow) default the flag to false — matches esbuild.ImportRecordList::truncateon theBorrowedarm just shortens aVec, no arena provenance issue.
Extended reasoning...
Overview
Ports esbuild's fix for evanw/esbuild#4241: disambiguating a ? (b) : c => d in TypeScript between a ternary and an arrow with a return type annotation. Touches src/ast/expr.rs (new EFlags variant), src/js_parser/p.rs (new memo field + ImportRecordList::truncate), src/js_parser/parser.rs (new ParenExprOpts field), src/js_parser/parse/parse_prefix.rs and parse_suffix.rs (flag threading), and src/js_parser/parse/mod.rs (the ~115-line speculative helper). ~100 lines of new test coverage in transpiler.test.js.
Security risks
None. Pure parser correctness on user source; no I/O, no auth, no external data. The one DoS-shaped concern (exponential backtrack on nested a ? (b) : T => ... : c) was flagged in an earlier round and is now memoized by byte offset with an n=25 regression test.
Level of scrutiny
High. js_parser runs on every TypeScript file Bun touches, and this introduces a qualitatively new backtracking shape: every prior lexer_backtracker_*/try_skip_* helper only skips TypeScript types, so their save/restore lists were bounded. This one calls parse_arrow_body(), which reaches parse_stmts_up_to() → parse_stmt() — i.e. arbitrary statement parsing. The restore block must therefore cover every piece of parser state that statement parsing mutates and that is either (a) keyed by source position or (b) observed after the rewind. Three such fields were missed in the initial revision and caught across two review rounds plus a self-review (comments_to_preserve_before, scopes_in_order_for_enum, import_records). The current list is documented in robobun's 19:58 comment, and the "don't truncate symbols so Ref-keyed maps stay keyed by unreachable indices" reasoning is sound as far as I can trace it — but the fact that the list is hand-curated and was incomplete three times makes me want a maintainer to sign off on it rather than a bot.
Other factors
- All prior inline findings (mine and CodeRabbit's) are resolved with test coverage for each.
- Test coverage is thorough: esbuild's full matrix, nested propagation, block bodies, legal comments, enum-in-body,
<Type>cast path, for-initin, private identifiers, both memo verdicts, andscanImportsdedup. - The
opts.is_asyncbranch inside the new check is dead in practice (onlypfx_t_open_parensets the flag, never withis_async); kept intentionally to mirror esbuild — reasonable. - The change to
sfx_t_questionthreadsflagsthrough, which also affects thenobranch propagation; verified this only carriesAfterQuestionAndBeforeColon(otherEFlagsvalues are stripped toNone), soTsDecoratorcan't accidentally leak into a ternary alternate.
|
CI on the rebased c8eb496 (build 74325): 284 jobs passed, 2 failed on tests unrelated to this diff:
Both are already being tracked separately. |
Jarred-Sumner
left a comment
Jarred-Sumner
left a comment
There was a problem hiding this comment.
Rewrite this PR. This PR is not good.
- It does not match the other TypeScript backtracking errors
- It likely can be simplified to ~10 lines of code instead of ~200
|
@Jarred-Sumner I'd like to make it smaller but I'm not seeing the ~10-line path yet, so I want to check what you have in mind before I rewrite in the wrong direction. Both reference implementations parse the whole arrow body speculatively and then look for a trailing
tsc's Options I can see:
Happy to go with whichever you prefer. |
|
@robobun there should be an existing helper for lexer backtracking. can we use that/ |
|
@Jarred-Sumner What I can do is make it look like the other backtrackers: a single Pushing that now; if you see a way to get it smaller I'm happy to revise again. |
|
@robobun fix conflicts & rebase |
…return type In TypeScript, `(b) : c => d` after `?` is ambiguous between an arrow function with return type `c` and a parenthesized `b` followed by the ternary's `:` and `c => d`. The TypeScript compiler picks the arrow only when another `:` follows the body to pair with the leading `?`. Bun previously used the cheap lexer-only check (skip `: Type`, require `=>`) regardless of context, which greedily committed to the arrow and then failed to find the ternary's `:`. `sfx_t_question` now passes `EFlags::AfterQuestionAndBeforeColon` when parsing the consequent (and propagates it to the alternate when already inside an outer ternary). `parse_paren_expr` uses it to pick the new `try_skip_type_script_arrow_return_type_after_question_with_backtracking` in `parse_skip_typescript.rs` instead of the cheap check. The new helper wraps `skip_type_script_arrow_return_type_with_backtracking` + `parse_arrow_body` + a trailing-`:` check, and on either outcome rewinds the lexer plus the parser state a body parse touches: scopes via `discard_scopes_up_to`, log counts, `comments_to_preserve_before`, `scopes_in_order_for_enum`, `import_records`, `fn_or_arrow_data_parse`, `after_arrow_body_loc`. `symbols` is left un-truncated so stale entries in the `Ref`-keyed maps statement parsing populates stay keyed by unique indices and are never looked up. The verdict is memoized by the byte offset of the leading `:` (same guard as `ts_infer_constraint_backtracks`) so deeply-nested inputs are O(n^2) instead of O(2^n).
debd179 to
5c3a418
Compare
|
Rebased onto main (b69e3ca) and squashed to one commit (5c3a418). The only conflict was in |
|
I checked this PR against main after #40818 (38f35d4) landed. I applied only the test block from this PR on top of main and ran it with a debug build. #40818 implements the same rule: parse the arrow return type and body speculatively, keep the arrow only when another Three inputs fail on main. Each has a legal comment between
A debug build aborts with #40985 fixes this on top of #40818 and adds the three cases to the test block from #40818. With that change, all 32 assertions from this PR pass on main. Once it lands, this PR can be closed. The rest of this diff conflicts with #40818 and is no longer needed. |
…onditional (#40985) ### Problem - `x = a ? /*! L */ (b) : T => { let y = 1 } : c` aborts a debug build: `panic: assertion failed: self.comments_to_preserve_before.len() >= original.comments_to_preserve_before_len` in `Lexer::restore` (`src/js_parser/lexer.rs:396`). A release build drops the legal comment from the output. - The cause is the speculative parse from #40818 (`src/js_parser/parse/parse_skip_typescript.rs:1598`). It parses the arrow body, then restores a snapshot that records `comments_to_preserve_before` as a length. A block body (`src/js_parser/parse/mod.rs:1473`) or an `import()` drains that buffer. A length cannot restore it. ### Fix - `P::parser_snapshot` moves the buffer out of the lexer with `core::mem::take`. The attempt runs with an empty buffer. `restore_parser_snapshot` puts the original back. - Correct because the attempt's output is discarded. The real parse scans the same tokens again and attaches the comment to the first statement of the arrow body. - Verified: `test/bundler/transpiler/transpiler.test.js` (three new cases, each aborts the debug build without the fix). Also `bundler_comments`, `bundler_edgecase`, `esbuild/ts`. ### Background - A legal comment is `/*! ... */`, `//! ...`, or a comment with `@license` or `@preserve`. The lexer collects them in `comments_to_preserve_before`. The next statement takes them and prints them before itself. - In TypeScript, `(b) : c => d` after `a ?` is an arrow function with return type `c` only when another `:` follows its body. Otherwise the `:` pairs with the `?`. #40818 decides this with a speculative parse: snapshot, parse the body, restore. - `ParserSnapshot` (`src/js_parser/p.rs`) holds everything a parse of an expression mutates. Lists are stored as lengths and truncated on restore. <details><summary>Notes</summary> - Found while I checked whether #34252 is superseded by #40818. The test block of #34252 has these three cases. With this change, all 32 of its assertions pass on main. - The `import()` case loses the comment with and without this change, because `import()` parsing (`parse_import_export.rs:49`) clears the buffer. That case covers only the assertion. - Release output without this change: `x = a ? (b) => {\n let y = 1;\n} : c;` (the comment is gone). - `parser_snapshot` has one caller. It now takes `&mut self`. </details>
Repro
esbuild and tsc both accept this and print
let x = a ? b : (c) => d;.Cause
In TypeScript,
(b) : c => dafter?is ambiguous between an arrow function with return typecand a parenthesizedbfollowed by the ternary's:andc => d. The TypeScript compiler picks the arrow only when another:follows the arrow body to pair with the leading?; otherwise the first:belongs to the ternary.Bun always used the cheap lexer-only check (skip
: Type, require=>) regardless of context, so ina ? (b) : c => dit greedily committed to the arrow(b): c => dand then failed to find the ternary's:.Both tsc (
parseParenthesizedArrowFunctionExpressioninsidetryParse) and esbuild (isTypeScriptArrowReturnTypeAfterQuestionAndBeforeColon) decide this by speculatively parsing the arrow body and checking whether a:follows; see evanw/esbuild#4241.Fix
sfx_t_questionnow passesEFlags::AfterQuestionAndBeforeColonwhen parsing the consequent, and propagates it to the alternate when already between an outer?and:(so nested ternaries likea ? b ? c : (d) : e => fwork).pfx_t_open_parenforwards it toparse_paren_exprasParenExprOpts::is_after_question_and_before_colon.When that flag is set and the token after
)is:,parse_paren_exprcalls the newtry_skip_type_script_arrow_return_type_after_question_with_backtrackinginparse_skip_typescript.rs(alongside the other TypeScript backtrackers) instead of the cheap check. The helper wrapsskip_type_script_arrow_return_type_with_backtracking()+parse_arrow_body()+ a trailing-:check, and on either outcome rewinds the lexer plus the parser state a body parse touches:discard_scopes_up_to, log counts,comments_to_preserve_before,scopes_in_order_for_enum,import_records,fn_or_arrow_data_parse,after_arrow_body_loc.symbolsis left un-truncated so stale entries in theRef-keyed maps statement parsing populates stay keyed by unique indices and are never looked up.The verdict is memoized by the byte offset of the leading
:(same guard asts_infer_constraint_backtracks) so deeply-nested inputs are O(n²) instead of O(2^n).Verification
New test in
test/bundler/transpiler/transpiler.test.jscovers esbuild's full matrix for this case plus the regression cases surfaced during review: nested-propagation, block bodies,<Type>(b)cast-before-paren, legal comments before(,enumin the body, private identifiers,ininside a for-init,scanImports()dedup, both n=25 memo verdicts, and the error cases. Fails on main, passes with this change. Full file: 172 pass, 0 fail.[review] gate passed · iteration 3 · 8 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 3
evidence per changed file