Don't panic when export default interface starts an expression or labeled statement#31263
Conversation
…n or label "export default interface=2" parsed the inner statement as an expression statement and wrapped it in S.ExportDefault, an AST shape the printer rejects with "Internal error: unexpected export default stmt data". Handle an SExpr result by keeping the statement (matching esbuild) and report a syntax error for a labeled statement instead of panicking.
|
Warning Review limit reached
Your plan currently allows 2 reviews/hour. Refill in 12 minutes and 52 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Comment |
|
Updated 5:31 AM PT - May 23rd, 2026
✅ @robobun, your commit 1199f5d72f499645a0ca0f5a6f4742832011fa8a passed in 🧪 To try this PR locally: bunx bun-pr 31263That installs a local version of the PR into your bun-31263 --bun |
|
The parser fuzzer hit a second signature from this same root cause, this time through // e.ts: export default interface=7
await Bun.build({ entrypoints: ["./e.ts"], target: "browser", throw: false })In the bundler, the Verified on
So this PR resolves both fuzz signatures; no separate change is needed for the bundler path. (Side note for anyone adding a |
…er export default interface An expression statement result relied on the visit pass flagging "interface" as a strict-mode reserved word, but that check only covers identifier expressions, not arrow parameter bindings, so "export default interface => 1" would have silently dropped the export. Route every non-function/class/interface result through the same Unexpected "interface" error instead.
There was a problem hiding this comment.
LGTM — the follow-up in f680f99 routes every non-declaration result through the syntax-error arm as suggested, and the new tests cover the arrow-param shape.
Extended reasoning...
Overview
This PR changes one match arm in src/js_parser/parse/parse_stmt.rs (the export default → function/class/interface statement path) and adds a regression test in test/bundler/transpiler/transpiler.test.js. The previous _ => {} fall-through, which built an S.ExportDefault { value: Stmt(...) } that the printer/visit pass cannot handle, now emits Unexpected "interface" and returns a SyntaxError. This branch is only reachable when the token after export default is the contextual keyword interface and the nested parse did not yield STypeScript/SFunction/SClass — i.e., exclusively spec-invalid inputs that previously panicked.
Security risks
None. This is parser error-recovery on already-invalid source; no auth, crypto, FS, or network surface is touched. The new code path strictly narrows behavior from "panic" to "logged syntax error", and range_of_identifier is bounds-checked so the error formatting can't read out of range.
Level of scrutiny
Low–medium. The diff is ~20 lines in a single match arm plus tests. The only valid-code paths through this branch (export default function, export default class, export default interface Foo {}) are matched by the explicit arms above and remain unchanged — confirmed by the exp("export default interface Foo {}", "") assertions in the new test. Every input that reaches the new _ arm uses interface as an identifier in a strict-mode module, which is invalid regardless, so erroring here cannot reject any program that should compile.
Other factors
I previously flagged (inline, on the first revision) that returning SExpr as-is would silently drop export default interface => 1 because the strict-mode reserved-word check doesn't cover arrow-param bindings. The author addressed this in f680f99 by collapsing SExpr into the catch-all error arm, and the test now asserts that exact shape throws on both ts and js loaders. The bug-hunting system found nothing on the updated revision, full transpiler/TS test suites pass per the PR description, and there is no CODEOWNERS entry for src/js_parser/.
|
CI triage for build 57181: the only failing job was Updated the branch with main so the size comparison is apples-to-apples; this also folds in #31257, which touches the adjacent |
There was a problem hiding this comment.
LGTM — the follow-up in f680f99 routes every non-function/class/interface-declaration shape through the Unexpected "interface" error, closing the arrow-param hole I flagged.
Extended reasoning...
Overview
This PR changes one match arm in src/js_parser/parse/parse_stmt.rs (the export default → function/class/interface branch of t_export) and adds a regression test in test/bundler/transpiler/transpiler.test.js. Previously the catch-all _ => {} arm let any non-SFunction/SClass/STypeScript statement fall through and get wrapped in S::ExportDefault { value: Stmt(...) }, which the printer/renamer cannot handle and which panics. The new arm reports Unexpected "interface" at stmt.loc and returns Err(SyntaxError) instead.
The branch is only entered when the token after default is function, class, or the contextual identifier interface. parse_fn_stmt only yields SFunction/STypeScript and parse_class_stmt only yields SClass, so the new error arm is reachable solely from the interface case, and stmt.loc there is always the interface token — range_of_identifier produces the right span. Because the arm returns Err, parsing aborts before the visit pass, so the Label scope recorded by the interface: 0 shape never needs to be reconciled.
Security risks
None. This is a parser error-reporting change on already-invalid input; it narrows behavior from "panic" to "syntax error" and cannot affect codegen for valid programs.
Level of scrutiny
Low–medium. The diff is ~20 lines in a single match arm plus a test. The only valid-code path through this match (export default interface Foo {} → STypeScript) is unchanged and is asserted by the new test. The author addressed my earlier inline comment exactly as suggested, and the test now explicitly covers the arrow form (interface => 1) on both ts and js loaders.
Other factors
- My prior inline comment is resolved; the
SExprearly-return that silently dropped theexport defaultis gone. - Transpiler suite passes per the PR description;
cargo check/clippy/fmtclean. - The Buildkite "binary size" failure (+11 MB on Linux targets only, ~0 on every other platform) is clearly a baseline/infra artifact unrelated to this 20-line change and should not block.
What does this PR do?
Fixes a transpiler/parser panic found by fuzzing (
panic: Internal error: unexpected export default stmt data):The same crash is reachable with the plain
js/jsxloaders (loader: "js"panics on the released canary too), and through other shapes likeexport default interface: 2(labeled statement) andexport default interface => 1(arrow) on every loader.Cause
In
parse_stmt.rs, theexport defaulthandling enters a statement path whenever the next token isfunction,class, or the contextual keywordinterface, then wraps whatever statement comes back inS.ExportDefault { value: Stmt(...) }unless it is a bare type annotation.Since #31239,
interface=2correctly comes back from the nestedparse_stmtcall as a plain expression statement (the TS contextual-keyword path only applies when the expression is exactly the bare identifier), andinterface: 2comes back as a labeled statement. Neither is one of the two statement kinds (S.Function/S.Class) that the visit pass and printer support insideS.ExportDefault, so the printer hits itsInternal error: unexpected export default stmt datapanic.Fix
In the
export defaultstatement path ofparse_stmt.rs, any nested statement that isn't a function declaration, class declaration, or erased TypeScript declaration now reports a syntax error at theinterfacetoken instead of being wrapped into anS.ExportDefaultthe later passes can't handle:This covers the expression forms (
interface = 2,interface + 1), the arrow form (interface => 1, whereinterfacebecomes a parameter binding that no later reserved-word check would flag), and the labeled-statement form (interface: 0). All of these are invalid inputs — esbuild and tsc reject every one of them (esbuild itself crashes withpanic: Internal erroron the labeled/arrow shapes), so no valid code changes behavior.Real
export default interface Foo {}declarations,export default function/class, and theasync/abstractpaths are unchanged.How did you verify your code works?
New test
export default interface that is not an interface declaration does not crashintest/bundler/transpiler/transpiler.test.jscovering the fuzz repro (tsx), thetsandjsloaders, the expression/arrow/labeled variants, and the still-working interface-declaration forms.panic: Internal error: unexpected export default stmt data.bun bd test test/bundler/transpiler/transpiler.test.js— 127 pass, 0 failbun bd test test/bundler/transpiler/scope-mismatch-panic.test.ts test/bundler/esbuild/ts.test.ts— 65 pass, 0 failcargo check -p bun_js_parser,cargo clippy -p bun_js_parser,cargo fmt --checkpass