Skip to content

Fix panic on anonymous export default class with an auto-accessor field - #31331

Merged
Jarred-Sumner merged 2 commits into
mainfrom
farm/80720bc3/fix-export-default-accessor-name
May 24, 2026
Merged

Fix panic on anonymous export default class with an auto-accessor field#31331
Jarred-Sumner merged 2 commits into
mainfrom
farm/80720bc3/fix-export-default-accessor-name

Conversation

@robobun

@robobun robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator

Fixes a fuzzer-found panic: called 'Option::unwrap()' on a 'None' value when transpiling or bundling an anonymous export default class that contains an accessor (auto-accessor) field and no decorators.

Repro

// e.ts (or .js — both panic)
export default class {
  accessor op;
}
bun e.ts          # panic
bun build e.ts    # panic
Bun.build({ entrypoints: ["./e.ts"], target: "bun", minify: true, sourcemap: "external" })  # panic (original fuzz case)
panic: called `Option::unwrap()` on a `None` value
  <bun_js_parser::p::P<true, false>>::lower_impl   src/js_parser/lower/lower_decorators.rs:1062
  lower_standard_decorators_stmt → lower_class → s_export_default

The minify/sourcemap flags from the fuzz report are incidental — any parse of the input hits it.

Root cause

An accessor field marks the class should_lower_standard_decorators, so lower_class takes the standard-decorator lowering path, which reads the class name ref unconditionally for class statements (lower_decorators.rs:1062).

For export default class { ... } the class statement is anonymous, and s_export_default only injected the generated <file>_default name when the class has_decorators. A class whose only reason for lowering is an auto-accessor has has_decorators == false, so no name was injected and the lowering unwrapped None.

The expression path already keys this off should_lower_standard_decorators; the statement path didn't.

Fix

In s_export_default, inject the default name whenever the class has decorators or will go through standard-decorator lowering. Output now matches the already-working decorated case:

var _op = new WeakMap;
var _init = __decoratorStart(undefined);
class e_default {
  constructor() { __privateAdd(this, _op, undefined); }
  get op() { return __privateGet(this, _op); }
  set op(v) { __privateSet(this, _op, v); }
}
__decoratorMetadata(_init, e_default);
let _e_default = e_default;
export { e_default as default };

Verification

  • New tests in test/bundler/transpiler/es-decorators.test.ts (runtime JS, runtime TS, and a Bun.build case mirroring the fuzz config) fail with the panic before the fix and pass after.
  • Full es-decorators.test.ts, es-decorators-esbuild.test.ts, decorators.test.ts, decorator-metadata.test.ts, export-default.test.js, bundler_decorator_metadata.test.ts, and transpiler.test.js pass with the fix.
  • cargo clippy -p bun_js_parser clean.

Standard-decorator lowering requires the class to have a name ref, but
s_export_default only injected the generated default name when the class
had decorators. A class with only `accessor` fields (no decorators) still
takes the standard-decorator lowering path, so the missing name hit an
unwrap on None in lower_decorators.

Inject the default name whenever the class will go through standard
decorator lowering, matching what the expression path already does.
@coderabbitai

coderabbitai Bot commented May 24, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, we couldn't start this review because you've used your available PR reviews for now.

Your plan currently allows 1 review/hour. Refill in 50 minutes and 25 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 @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8aaaba5d-eda6-4989-aec4-52c3c279aa6c

📥 Commits

Reviewing files that changed from the base of the PR and between 49c97de and cc7efb5.

📒 Files selected for processing (2)
  • src/js_parser/visit/visit_stmt.rs
  • test/bundler/transpiler/es-decorators.test.ts

Comment @coderabbitai help to get the list of available commands and usage tips.

@robobun

robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 7:04 AM PT - May 24th, 2026

@robobun, your commit cc7efb5a39553027293711c8cb27d7ad6c2d9918 passed in Build #57570! 🎉


🧪   To try this PR locally:

bunx bun-pr 31331

That installs a local version of the PR into your bun-31331 executable, so you can run:

bun-31331 --bun

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. Parse and lower accessor fields under experimentalDecorators #29201 - Also fixes the anonymous export default class panic by widening the needs_default_name condition in visit_stmt.rs to cover should_lower_standard_decorators, preventing the null class_name crash in lower_decorators.rs

🤖 Generated with Claude Code

@robobun

robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator Author

Partial overlap, worth spelling out:

#29201's primary change is making accessor parse and lower under experimentalDecorators: true (issues #29197/#27335). As part of that it also widens the same name-injection condition in s_export_default (via a property scan for auto-accessors), which would cover this panic too.

This PR is only the minimal fix for the panic that currently reproduces on main (export default class { accessor op; } in standard-decorators mode — the fuzzer hit it through Bun.build), using the should_lower_standard_decorators flag the parser already computes, plus tests for that case.

So: if #29201 lands first, this PR is redundant and can be closed; if this lands first, #29201 only needs a trivial rebase of its visit_stmt.rs hunk (its broader condition is still wanted there because it routes accessor through standard lowering under experimentalDecorators as well).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — minimal, well-reasoned fix that aligns the statement path with the expression path's existing should_lower_standard_decorators check, with regression tests covering the fuzz repro.

Extended reasoning...

Overview

This PR fixes a fuzzer-found panic when transpiling/bundling export default class { accessor x; } (anonymous class with an auto-accessor field and no decorators). The change is a one-line condition widening in src/js_parser/visit/visit_stmt.rs (s_export_default handling), plus three regression tests in test/bundler/transpiler/es-decorators.test.ts.

Verification of root cause

I traced the code path and confirmed the analysis:

  • parse/mod.rs:274-275 sets should_lower_standard_decorators = standard_decorators && (has_any_decorators || has_auto_accessor), so an accessor-only class has has_decorators=false but should_lower_standard_decorators=true.
  • p.rs:6935 routes lower_class into lower_standard_decorators_stmt based on should_lower_standard_decorators.
  • lower_decorators.rs:1059-1062 unconditionally unwraps class.class_name in the statement branch.
  • The old guard at visit_stmt.rs:833 only injected a name when has_decorators, leaving the accessor-only case nameless → panic.

The fix adds || should_lower_standard_decorators so the guard now covers exactly the set of classes that will hit the unwrap. This mirrors how the expression path (visit_stmt.rs:418) and other name-propagation sites already key off should_lower_standard_decorators.

Security risks

None. This is parser/transpiler AST-lowering logic with no auth, crypto, filesystem, or network surface. The change only affects whether a synthetic class name is assigned before lowering.

Level of scrutiny

Low-to-moderate. The Rust change is a single boolean disjunct in a guard, strictly widening when an already-existing fallback (inject <file>_default name) fires. It cannot regress the previously-working decorated case (that branch was already taken via has_decorators). The new tests follow the established patterns in the file (tempDir + spawn + stdout/stderr/exitCode assertions) and cover JS runtime, TS runtime, and the original Bun.build fuzz config.

Other factors

  • No CODEOWNERS cover the modified files.
  • No prior reviewer comments to address.
  • The PR description reports the full decorator/export-default test suites pass and clippy is clean.
  • Minor note: the parallel Zig reference at visit_stmt.zig:456 retains the old has_decorators-only check, but the active Rust parser (per the panic trace) is what's fixed here; not a blocker for this PR.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — minimal, well-targeted fix that widens the name-injection condition to match the lowering dispatch, with solid regression tests.

Extended reasoning...

Overview

This PR fixes a fuzzer-found panic in the JS parser when transpiling export default class { accessor op; } (anonymous default-exported class with an auto-accessor and no decorators). The change is a 2-line condition widening in src/js_parser/visit/visit_stmt.rs:833 plus three regression tests in test/bundler/transpiler/es-decorators.test.ts.

Correctness

I verified the root cause: should_lower_standard_decorators is set whenever standard_decorators && (has_any_decorators || has_auto_accessor) (parse/mod.rs:274), and lower_class dispatches to lower_standard_decorators_stmt based on that flag (p.rs:6935), which then unconditionally unwraps class.class_name for statements (lower_decorators.rs:1062). The old s_export_default guard only injected a name when has_decorators was true, missing the auto-accessor-only case. The new condition has_decorators || should_lower_standard_decorators exactly matches the set of cases that reach the unwrap. The change is strictly additive — it only injects a name in more cases, and only when class_name is already None/unbound, so it cannot regress named or previously-working cases. The expression path (visit_stmt.rs:418, visit/mod.rs:303/645/684/988) already keys off should_lower_standard_decorators, so this brings the statement path into line with the established pattern.

Security risks

None. This is a parser/transpiler crash fix with no auth, crypto, permissions, or untrusted-input-handling implications beyond making the parser not panic on valid syntax.

Level of scrutiny

Low-to-moderate. It's a 2-line logic change in a hot parser path, but the change is mechanical (OR-ing in a flag that's already used identically elsewhere for the same purpose), the root cause is well-explained and verifiable, and the three new tests (JS runtime, TS runtime, Bun.build mirroring the fuzz config) follow the exact patterns of adjacent tests in the same describe block.

Other factors

  • No bugs found by the bug-hunting system.
  • The overlap with #29201 is already clearly explained in the thread; that's a sequencing decision for maintainers, not a correctness concern with this PR.
  • CI showed a failure on 21f59ad and was retriggered via cc7efb5; the code change itself is sound regardless.
  • The Zig mirror at visit_stmt.zig:456 still uses the narrower has_decorators check — if the Zig path is still active, it may have the same latent bug, but that's out of scope for this Rust-side fix.

@robobun

robobun commented May 24, 2026

Copy link
Copy Markdown
Collaborator Author

CI status for maintainers: the change is green on every lane that actually ran — twice.

Both builds (57528 on 21f59ad, and the re-run 57570 on cc7efb5) have zero test failures. The only red checks are darwin-14-aarch64-test-bun and darwin-26-aarch64-test-bun, which show "Expired" — the jobs never executed because no macOS aarch64 agent picked them up before the scheduling timeout. The same expiry is hitting other open PRs at the same timestamps, so it's an agent-capacity issue, not something in this diff (macOS x64 test lanes pass; 70/72 statuses green on the latest build).

Retrying those two jobs on build 57570 once aarch64 agents are available should turn the PR fully green. The fix itself is a two-line condition change in visit_stmt.rs plus regression tests; see the PR description and the #29201 overlap note above for context.

@Jarred-Sumner
Jarred-Sumner merged commit 24e94ad into main May 24, 2026
78 checks passed
@Jarred-Sumner
Jarred-Sumner deleted the farm/80720bc3/fix-export-default-accessor-name branch May 24, 2026 20:46
springmin pushed a commit to springmin/bun that referenced this pull request May 25, 2026
* oven/main (20 new commits):
  webcore: free Blob's owned content type on drop (oven-sh#31358)
  Support cross-compiling macOS binaries from Linux (oven-sh#31303)
  test: forward keep-alive requests in proxy.test.ts's mock proxy (oven-sh#31352)
  Port Bun.stringWidth to C++ with explicit SIMD (oven-sh#31351)
  Fix quadratic hang reporting duplicate-binding parse errors in the transpiler (oven-sh#31341)
  shell: don't abort when a glob's directory prefix doesn't exist (oven-sh#31367)
  Error instead of crashing on deeply nested statements in the transpiler (oven-sh#31333)
  Fix JSX transform panic when a bare `key` prop precedes `key` with a value (oven-sh#31350)
  Cap ANSI markdown indentation so deeply nested lists render in linear time (oven-sh#31366)
  css: bound selector-list expansion when compiling nesting for older targets (oven-sh#31277)
  node:http2: reassemble HEADERS+CONTINUATION before HPACK decoding (oven-sh#31323)
  Fix `await using` expression printing `using` as `await` (oven-sh#31324)
  Parenthesize `async` when it starts a for-of loop initializer (oven-sh#31326)
  Print Infinity and negative numeric property keys as computed properties (oven-sh#31328)
  css: keep required grouping parens in @container conditions when minifying (oven-sh#31330)
  Fix panic on anonymous export default class with an auto-accessor field (oven-sh#31331)
  node:http2: send GOAWAY frames on stream 0 (oven-sh#31353)
  parser: fix Scope mismatch while visiting panic from decorators on dropped class members (oven-sh#31340)
  webcrypto: reject oversized BufferSource inputs instead of aborting (oven-sh#31356)
  Error instead of crashing on deeply nested TypeScript types in the transpiler (oven-sh#31361)

Resolved conflicts:
  - scripts/build.ts: kept both OHOS and macOS-cross argv entries
  - scripts/build/config.ts: kept both OHOS and macOS-cross config fields
  - scripts/build/deps/webkit.ts: kept OHOS fno-pic exclusion, adopted upstream -flto=thin
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants