Repository navigation
parser: keep legal comments across the speculative arrow parse in a conditional - #40985
Conversation
…onditional The attempt in is_type_script_arrow_return_type_after_question_and_before_colon parses the arrow body. A block body or import() drains lexer.comments_to_preserve_before. The lexer snapshot records only a length, so the restore could not bring the comments back. The debug build hit the assertion in Lexer::restore. The release build dropped the legal comment from the output. Move the buffer into ParserSnapshot at snapshot time and put it back on restore.
|
Status: ready for review. Reproduced on main (ceef547) with a debug build: The build aborts with Found while I checked #34252 against #40818. The three new test cases come from the test block of #34252. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughThe parser snapshot now preserves pending lexer comments during speculative parsing. Transpiler regression tests cover comments in conditional expressions with arrow-function and ChangesComment preservation
Suggested reviewers: Merge Risk: ⚪ Minimal · up to This localized parser change preserves legal comments during speculative conditional-arrow parsing and prevents the reported debug assertion and release-output omission; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — targeted snapshot fix with matching regression tests.
Checked that mem::take runs before self.lexer.snapshot(), so the captured comments_to_preserve_before_len is 0 and the debug assertion in Lexer::restore holds regardless of what the attempt drains or appends.
Verified the single caller (parse_skip_typescript.rs:1598) always reaches restore_parser_snapshot — the attempt closure's Result is captured, not ?-propagated, so the moved-out buffer cannot be lost.
ParserSnapshot is consumed by value on restore, so the saved Vec cannot be reused twice; the &self → &mut self change is safe with one &mut self-context caller.
New exp() cases sit alongside the existing conditional-arrow assertions and follow the same pattern.
Extended reasoning...
Overview
This PR fixes a snapshot/restore asymmetry in the JS parser's speculative-parse machinery for TypeScript conditional-arrow disambiguation (a ? (b) : T => body : c). ParserSnapshot previously relied on LexerSnapshot capturing comments_to_preserve_before as a length, but a block body or import() inside the attempt drains that buffer entirely — a length cannot restore a shrunk Vec, tripping a debug assertion and dropping the legal comment in release. The fix moves the buffer out with core::mem::take into the snapshot (leaving the lexer empty for the attempt) and writes it back on restore. Two files touched: src/js_parser/p.rs (~10 lines) and three new exp() assertions in test/bundler/transpiler/transpiler.test.js.
Security risks
None. This is purely internal parser state bookkeeping for legal-comment preservation during a speculative parse whose output is discarded. No user-facing API, no untrusted-input validation path, no allocation sizing, no unsafe code.
Level of scrutiny
Low-to-moderate. The change is small and mechanical, with a single caller. I traced the ordering carefully: mem::take executes before self.lexer.snapshot(), so LexerSnapshot.comments_to_preserve_before_len is captured as 0; on restore, Lexer::restore truncates whatever the attempt accumulated back to 0 (its debug_assert!(len >= 0) is trivially satisfied), and then restore_parser_snapshot overwrites with the saved Vec. The sole call site at parse_skip_typescript.rs:1598-1611 unconditionally calls restore_parser_snapshot(snapshot) before any early return, so the taken buffer can never be dropped without being reinstated. restore_parser_snapshot takes the snapshot by value, so double-restore is impossible. The &self → &mut self signature change is safe given the one caller is already in a &mut self method.
Other factors
Tests are placed in the existing describe block for TS conditional-arrow parsing, immediately after the related snapshot-restore coverage from the earlier PR, using the same exp() helper — exactly where CLAUDE.md says they belong. The three cases cover both attempt outcomes (arrow accepted, arrow rejected → plain conditional) plus the import() drain path. The added doc-comment lines and test comment are concise and explain the non-obvious "why a length cannot restore this." No CODEOWNERS entry covers these paths. Exit reason was dry_streak.
Problem
x = a ? /*! L */ (b) : T => { let y = 1 } : caborts a debug build:panic: assertion failed: self.comments_to_preserve_before.len() >= original.comments_to_preserve_before_leninLexer::restore(src/js_parser/lexer.rs:396). A release build drops the legal comment from the output.export typebefore a newline, andimport type from#40818 (src/js_parser/parse/parse_skip_typescript.rs:1598). It parses the arrow body, then restores a snapshot that recordscomments_to_preserve_beforeas a length. A block body (src/js_parser/parse/mod.rs:1473) or animport()drains that buffer. A length cannot restore it.Fix
P::parser_snapshotmoves the buffer out of the lexer withcore::mem::take. The attempt runs with an empty buffer.restore_parser_snapshotputs the original back.test/bundler/transpiler/transpiler.test.js(three new cases, each aborts the debug build without the fix). Alsobundler_comments,bundler_edgecase,esbuild/ts.Background
/*! ... */,//! ..., or a comment with@licenseor@preserve. The lexer collects them incomments_to_preserve_before. The next statement takes them and prints them before itself.(b) : c => daftera ?is an arrow function with return typeconly when another:follows its body. Otherwise the:pairs with the?. parser: accept TypeScript arrow return types inside conditionals,export typebefore a newline, andimport type from#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.Notes
a ? (b) : c => dbetween ternary and arrow return type #34252 is superseded by parser: accept TypeScript arrow return types inside conditionals,export typebefore a newline, andimport type from#40818. The test block of js_parser: disambiguatea ? (b) : c => dbetween ternary and arrow return type #34252 has these three cases. With this change, all 32 of its assertions pass on main.import()case loses the comment with and without this change, becauseimport()parsing (parse_import_export.rs:49) clears the buffer. That case covers only the assertion.x = a ? (b) => {\n let y = 1;\n} : c;(the comment is gone).parser_snapshothas one caller. It now takes&mut self.