parser: apply ASI when TS contextual keywords precede a newline - #34258
Conversation
…newline Bun's TS parser treated declare, abstract, interface (statement level and class-body level) and accessor (class body) as modifiers even when a newline followed, silently deleting the declaration that came after. esbuild and tsc both apply ASI and treat the word as a standalone identifier/field. Also fixes class-body get/set followed by '*' (esbuild parses the word as a field and '*' as a generator; bun rejected with 'Unexpected *'). Statement keywords now gate on !has_newline_before to match esbuild: TsStmtDeclare, TsStmtInterface, TsStmtAbstract, and the export-default abstract class path. Class-body declare/abstract/accessor gate on !has_newline_before. could_be_modifier_keyword now excludes '*' after get/set (when not async), matching esbuild.
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Not a duplicate of #29201. That PR is about lowering |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…f emitting live code When the newline falls after interface/abstract inside an ambient "declare" context, falling through to SExpr leaves the remaining tokens to be parsed as live runtime statements (the declare wrapper only consumes the keyword). esbuild rejects these shapes. Extend the error checks to cover is_typescript_declare and point the error at the keyword range to match esbuild, replacing the self-contradictory "Expected class but found class" wording for the decorator variants.
…lag checks The per-arm is_typescript_declare checks over-rejected inside ambient namespace/global bodies (where the flag is propagated for body semantics) and for "export declare\n" (where t_export pre-set the flag redundantly), and under-covered the type/namespace/module arms. Replace with esbuild's approach: after the recursive parse inside TsStmtDeclare, reject any result that is not STypeScript/SLocal/SEmpty, pointing at the token immediately after "declare". This uniformly catches "declare interface\n", "declare abstract\n", "declare type\n", "declare namespace\n", "declare module\n", "declare declare\n", and "declare foo" while accepting all valid ambient forms and all shapes inside ambient bodies. Also drop the redundant is_typescript_declare pre-set in t_export's SDeclare arm (TsStmtDeclare sets it itself on the no-newline path).
"declare foo: bar" and "declare module : es2015" are now rejected with Unexpected "foo"/"module" (matching esbuild) instead of being silently dropped, so they no longer exercise the scope-discard codepath. Move the error assertions to transpiler.test.js alongside the other declare-body validation cases.
…icitly The JSX fixture runs from a tempDir with no node_modules of its own, so react/jsx-dev-runtime resolution depends on NODE_PATH being set in the ambient environment. Set it explicitly so the test passes in environments that do not pre-seed NODE_PATH.
There was a problem hiding this comment.
No new findings this pass. All four earlier review rounds have been addressed — the final shape (post-check in TsStmtDeclare accepting only STypeScript/SLocal/SEmpty, per-arm is_typescript_declare checks removed, redundant t_export pre-set dropped) matches esbuild's approach and the 50-assertion test block covers every variant I'd raised. Deferring to a human for final sign-off since this is core parser control flow that went through several non-trivial correctness iterations.
What was reviewed:
- Re-checked the
declarepost-check against every valid ambient form in the tests (function/class/enum/namespace/abstract class/const/let/var) — each returnsSTypeScriptorSLocalunderis_typescript_declare, so none are falsely rejected. - Confirmed the ambient-body cases (
declare namespace N { abstract\n... },declare global { ... },declare module "m" { ... }) andexport declare\n...no longer trip the removed per-armis_typescript_declareguard. - Checked the
get/set+*gate is scoped byopts.is_asyncsoasync * x()still parses; the two removedscope-mismatch-paniccases are covered as parse errors in the new test block.
Extended reasoning...
Overview
This PR adds !has_newline_before gates to seven TS contextual-keyword parse sites (statement-level declare/abstract/interface, export default abstract, class-body declare/abstract/accessor, and get/set before *) so that a newline between the keyword and what follows applies ASI instead of silently consuming the following declaration as an ambient/modifier body. It also replaces the previous "unconditionally wrap whatever the recursive parse returned in S::TypeScript{}" behaviour in TsStmtDeclare with a post-check that rejects any result other than STypeScript/SLocal/SEmpty, matching esbuild's case "declare": handler. Files: parse_stmt.rs (+77/-8), parse_property.rs (+8/-2), transpiler.test.js (+90), scope-mismatch-panic.test.ts (2 cases moved to the new test block).
Security risks
None. Pure parser control-flow / ASI correctness; no I/O, auth, crypto, or untrusted-length arithmetic. The new error paths return SyntaxError rather than panicking.
Level of scrutiny
High — this is core JS/TS parser logic that runs on every .ts file Bun touches. The change is not mechanical: it went through four review rounds here, three of which surfaced real correctness issues (self-contradictory diagnostic wording; a regression where declare interface\n/declare abstract\n started emitting live code; an over-broad is_typescript_declare guard that falsely rejected export declare\n and ambient-body statements; and incomplete coverage of sibling type/namespace/module arms). Each was fixed, and the final design converged on esbuild's post-check approach — but the number of iterations to get there is itself a signal that a maintainer familiar with the parser should confirm the final shape.
Other factors
- The 50-assertion test block is comprehensive: ASI cases, no-newline controls, every valid
declare X, every rejecteddeclare X, ambient-body acceptance,export {abstract,declare}\nfall-through, and the three decorator+newline error paths.bun bd testpasses on both debug-ASAN and release per the evidence block; the test fails on the unfixed build. - The two removed
scope-mismatch-panic.test.tscases (declare foo: bar,declare module : es2015) were previously accepted but are now parse errors — a behaviour change, but one that matches esbuild and is covered in the new test block. The unrelatedNODE_PATHtweak in that file (resolvingreactfromtest/node_modules) is a hermeticity fix. - The dropped
opts.is_typescript_declare = trueint_export'sSDeclarearm is genuinely redundant now thatTsStmtDeclaresets it itself, and removing it is what makesexport declare\nclass Foo {}fall through correctly.
Given the parser-critical scope and the iteration history, I'm deferring rather than approving, but I have no outstanding concerns with the current diff.
There was a problem hiding this comment.
No new issues found after the latest revision — all prior feedback (the declare <keyword>\n live-code leak, the over-broad is_typescript_declare guard, and both comment-length nits) has been addressed. Deferring to a human for final sign-off since this touches core TS parser control flow.
Checked: the newline gates in parse_property.rs (PDeclare/PAbstract/PAccessor, get/set + *) and parse_stmt.rs (TsStmtInterface/Abstract/Declare, export default abstract) against esbuild's shapes.
Checked: the TsStmtDeclare post-check accepts every valid ambient form (SLocal/STypeScript/SEmpty) including inside declare namespace/declare global/declare module bodies — no false rejections found.
Ruled out: SEmpty arm diverging from esbuild (declare; — unreachable, the newline guard returns first); accessor newline test being vacuous (it asserts the field-named-accessor output either way).
Extended reasoning...
Overview
This PR fixes Bun's TypeScript parser to apply ASI when the contextual keywords declare, abstract, interface, and accessor are followed by a newline, instead of treating them as modifiers that silently consume (and erase) the following declaration. It touches src/js_parser/parse/parse_stmt.rs (~75 lines: newline gates on TsStmtInterface/TsStmtAbstract/TsStmtDeclare and the export default abstract path, plus a post-recursive-parse validation in TsStmtDeclare that rejects any non-STypeScript/SLocal/SEmpty result) and src/js_parser/parse/parse_property.rs (~8 lines: newline gates on class-body PDeclare/PAbstract/PAccessor, and excluding * from the modifier-follow set after get/set). It adds ~90 lines of tests in transpiler.test.js covering 50 assertions across every shape, and removes two now-rejected cases from scope-mismatch-panic.test.ts.
Security risks
None. This is parser control-flow for pathological TypeScript inputs; there is no untrusted-length arithmetic, allocation sizing, or FFI involved. The change tightens acceptance (some previously-accepted inputs now error) and loosens erasure (some previously-erased declarations now survive), both matching esbuild/tsc.
Level of scrutiny
High — the JS/TS parser is a critical, hot code path, and the review history bears this out: three prior review rounds on this PR found real behavioral regressions (declare interface\n leaking live code, an over-broad is_typescript_declare guard rejecting valid ambient bodies, a self-contradictory Expected "class" but found "class" diagnostic). All have been fixed, and the final approach — esbuild's post-check in TsStmtDeclare rather than per-arm is_typescript_declare guards — is the structurally correct one and eliminates the false-positive class entirely.
Other factors
Test coverage is thorough: the new test block exercises the full variant matrix (statement-level and class-body, with and without export/export default/decorators, every valid declare X form as a control, ambient-body cases). The two candidate issues raised this run were both refuted on inspection. The one behavioral divergence from esbuild I'm aware of (declare; — esbuild's SEmpty post-check arm) is unreachable in Bun's implementation because the newline guard fires first and returns Ok(None) before the recursive parse. Given the parser's criticality and the number of iterations this took to converge, a human familiar with the esbuild port should give it a final look.
|
The diff is green: CI builds #73421 and #73448 both failed only on unrelated flakes (net-connect-memleak, install-registry/patch, webview, bake/deinitialization, spawn, repl, solc, valkey, fetch.stream); none touch the parser. Ready for review. |
### What `#34249` changed TypeScript enum lowering so that only **module-scope** enums emit `var`; an enum in a function, method, or block body now emits `let`. `#34250` merged four minutes earlier and added `it("rejects yield/await/this/super in enum initializers")`, whose expectations still assert `var x` for **block-scoped** enums. Each PR was green against a `main` that lacked the other's change, so the collision only appeared once both had landed — and since `main` pushes run no test shards, nothing caught it. The result is that `main` asserts output its own parser no longer produces. `test/bundler/transpiler/transpiler.test.js` currently fails on every PR that merges `main`. ### The fix Updates the five stale expectations to `let`. All five are enums nested in a function or method body: | Line | Case | |---|---| | 771 | `function *f() { enum x { y = (function*() { yield 1 })() } }` | | 775 | `async function f() { enum x { y = (async () => await 1)() } }` | | 785 | `function *f() { enum x { y = 1 } yield 1; }` | | 789 | `async function f() { enum x { y = 1 } await 1; }` | | 793 | `class C extends B { m() { enum x { y = 1 } super.foo(); } }` | Deliberately unchanged: - **Line 779** — `enum x { y = (function() { return this })() }` is top-level, so `var` is still correct. - **Namespace expectations** — namespaces only appear at module scope or nested in another namespace, where both the old and new predicates agree. ### Note for reviewers Only the line-771 failure is visible in CI: `expectPrinted_` throws at the first mismatch, masking the other four. A fix touching only the reported line would go red again on the next one, so all five are updated together. ### Verification Ran `test/bundler/transpiler/transpiler.test.js` against a build containing `#34249`: - pristine `main`: 176 pass / 4 fail - with this change: 177 pass / 3 fail The change flips exactly the one enum test and touches nothing else. The 3 remaining failures are unrelated to this diff — they are skew between that build and two commits that landed after it (`#34254`, `#34258`), and build from source in CI. Also swept the repo for any other assertion of enum-lowering text (the closure IIFE shape `(x ||= {})` / `(x = x || {})`). Only two files assert it: this one, and `test/js/node/module/require-extensions.test.ts:129`, whose fixture declares a top-level enum and is correctly `var`.
2 participants
Problem
Bun's TS parser treats
declare,abstract,interface, andaccessoras modifiers even when a newline follows them, silently deleting the declaration that comes after. esbuild and tsc both apply ASI and treat the keyword as a standalone identifier expression (statement level) or class field (class body).Sibling shapes with the same bug:
abstract\nclass A {},interface\nA\n{ sideEffect() },abstract class A { abstract\nfoo() {} },export default abstract\nclass A {},class A { accessor\n x = 1 }. Alsoclass A { get\n*x() {} }was rejected withUnexpected *; esbuild/tsc parse a fieldgetfollowed by a generator*x.Cause
esbuild (
internal/js_parser/js_parser.go) gates each of these paths on!p.lexer.HasNewlineBeforebefore committing to the modifier interpretation, and validates the body of everydeclarestatement after the recursive parse. Bun's port lost both:TsStmtDeclare,TsStmtInterface,TsStmtAbstractinparse_stmt_fallthrough_ts_keywordhad no newline check.export default abstractclass path had no newline check.PDeclare,PAbstract,PAccessorinparse_propertyhad no newline check.could_be_modifier_keywordcounted*afterget/setas a modifier follow-up; esbuild excludes it.TsStmtDeclareunconditionally wrapped whatever the recursive parse returned inS::TypeScript{}, so a newline-split keyword that fell through toSExprsilently left the rest of the input as live runtime statements (declare type\nFoo = numberemittedFoo = number;).Fix
!p.lexer.has_newline_beforegates at each site above.TsStmtDeclare, after the recursiveparse_stmt, reject any result that is notSTypeScript/SLocal/SEmptywithUnexpected "<token>"pointing at the token captured before recursion. This uniformly catchesdeclare {interface,abstract,type,namespace,module,declare}\n,declare foo, anddeclare foo: barwhile accepting every valid ambient form and everything inside ambient bodies (declare namespace { ... },declare module "m" { ... },declare global { ... }).export default interface \n Foo {}stays accepted (esbuild explicitly allows a newline there) via the existingis_name_optionalflag.export interface \n Foo {}now reportsUnexpected "interface"like esbuild.@decoratorfollowed bydeclare/abstractsplit by a newline reportsUnexpected "declare"/"abstract"instead of the self-contradictoryExpected "class" but found "class".is_typescript_declare = truepre-set int_export'sSDeclarearm (thedeclarearm sets it itself).Verification
New test in
test/bundler/transpiler/transpiler.test.js(contextual keywords followed by a newline apply ASI instead of acting as modifiers) covers 50 assertions across every shape above: the ASI cases, no-newline control cases, every validdeclare Xform, every rejecteddeclare Xform, ambient-body cases (accepted),export abstract\n/export declare\n(accepted), and the decorator variants. Fails at the first assertion on the unfixed build.Two cases in
test/bundler/transpiler/scope-mismatch-panic.test.ts(declare foo: baranddeclare module : es2015) were previously accepted and are now parse errors matching esbuild; their assertions moved to the new test block.bun bd test test/bundler/transpiler/transpiler.test.js— 172 pass, 0 failbun bd test test/bundler/esbuild/ts.test.ts— 57 pass, 0 failbun bd test test/bundler/transpiler/{decorators,decorator-metadata,es-decorators}.test.ts test/bundler/bundler_decorator_metadata.test.ts— 78 pass, 0 failRelated: #29201 independently adds the
accessornewline check while lifting thestandard_decoratorsgate; whichever lands second has a one-hunk rebase on that branch.[review] gate passed · iteration 3 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 4 passed · 0 rejected · iteration 3
evidence per changed file