Skip to content

js_parser: name an anonymous decorated class assigned with ??=, ||= or &&= after the target - #38924

Open
robobun wants to merge 1 commit into
farm/80aecbe5/decorator-inferred-class-namefrom
farm/4055ec3e/decorated-class-logical-assign-name
Open

js_parser: name an anonymous decorated class assigned with ??=, ||= or &&= after the target#38924
robobun wants to merge 1 commit into
farm/80aecbe5/decorator-inferred-class-namefrom
farm/4055ec3e/decorated-class-logical-assign-name

Conversation

@robobun

@robobun robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator

Stacked on #38757 (base branch farm/80aecbe5/decorator-inferred-class-name); see Background for why. The diff of this PR is one hunk in visit_binary.rs plus tests.

Problem

  • With standard (TC39) decorators, an anonymous class expression assigned with ??=, ||= or &&= gets the wrong .name:
    function dec() {}
    let a; a ??= class { @dec m() {} };
    let b = null; b ||= class { @dec m() {} };
    let c = 1; c &&= class { @dec m() {} };
    let d; d = class { @dec m() {} };
    console.log(JSON.stringify([a.name, b.name, c.name, d.name]));
    bun 1.4.0 prints ["_class","_class","_class","d"]; node prints ["a","b","c","d"] for the same file with the @dec removed. With a class decorator instead (a ??= @dec class {}) both a.name and the decorator's context.name are "". On js_parser: keep the inferred name of lowered anonymous decorated class expressions #38757 alone the three names become "".
  • Cause: visit_right_and_finish in src/js_parser/visit/visit_binary.rs:124 only records the assignment target as the class's context name when the operator is =. The logical assignment operators visit their right operand through the same function but skipped that block, so the lowering never sees a name for them.
  • The spec gives ??=, ||= and &&= the same NamedEvaluation rule as = (an anonymous function or class on the right is named after an identifier target); the arithmetic and bitwise compound assignments have no such rule.

Fix

  • Record the context name for BinNullishCoalescingAssign, BinLogicalOrAssign and BinLogicalAndAssign as well as BinAssign. The existing conditions still apply: only when the right operand itself is an anonymous class that will be lowered, and only when the target is an identifier. The name is saved and restored around the visit of the right operand exactly as before.
  • Correct because the lowered output now names the class in exactly the cases the engine would have named it had the class not been rewritten: the three operators the spec lists, and no others. += and friends (no NamedEvaluation) and member targets such as o.p ??= class {} (not an identifier reference) stay excluded. The class decorator's context.name comes from the same recorded name, so it now matches what a = @dec class {} already reports.
  • This is the only visitor site involved: the parser never lowers the logical assignment operators, and the separate assignment-pattern paths ([x = class {}] = ..., ({ x = class {} } = ...)) only ever see =, since a pattern default cannot use a logical operator.
  • Verification, test/bundler/transpiler/es-decorators.test.ts, new block anonymous decorated class assigned with a logical assignment operator (3 of its 4 tests fail on the base branch without the hunk; the fourth is a guard):
    • the three operators with a method decorator, plus =, a chained Outer = Inner ??= class {} (named Inner), an assignment inside a function, and an accessor-only class (lowered without decorators).
    • the three operators with a class decorator, checking both context.name and .name.
    • guards that pass before and after: an explicit class name wins, a member target, a class nested inside the right operand, and += all still report no context name.
    • the class body still sees the variable being assigned: a static initializer reads the value the variable had during definition (null under ||=, 1 under &&=), a method sees a later reassignment, the body of a class replaced by a class decorator sees the replacement, and a method can assign to the variable. This is the interaction with js_parser: keep the inferred name of lowered anonymous decorated class expressions #38757 described in Background; it passes on this stack and fails (TypeError) with this hunk applied to main.
    • .name and scoping expectations were cross-checked against node on the same programs with the decorators removed.
    • es-decorators.test.ts (including the tests js_parser: keep the inferred name of lowered anonymous decorated class expressions #38757 adds), es-decorators-esbuild.test.ts, decorators.test.ts and decorator-metadata.test.ts pass on a debug build of this branch; cargo clippy -p bun_js_parser and cargo fmt are clean.

Background

Earlier revision of this PR (based on main)

The first revision carried the same visit_binary.rs hunk and the first three tests, based on main. Review turned up the scoping interaction above: with the hunk on main, let b; b ??= class { @dec m() { b = 5 } }; new b().m() throws a TypeError (it works on 1.4.0 and on this stack), a static initializer reading b sees the class instead of the variable, and a replacing class decorator's replacement is not what the body sees. The PR was rebased onto #38757, where those programs behave as in node, and the fourth test was added to cover them.

@coderabbitai

coderabbitai Bot commented Aug 15, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 2 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d9999572-f418-41f3-975e-d53025a04d31

📥 Commits

Reviewing files that changed from the base of the PR and between 7d276b9 and 659065b.

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

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

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 12:05 PM PT - Aug 15th, 2026

@robobun, your commit 5727b0f has some failures in Build #98123 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 38924

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

bun-38924 --bun

@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 — extends the existing decorator_class_name propagation from = to the three logical assignment operators, matching the spec's NamedEvaluation rule.

What was reviewed:

  • Confirmed ??=/||=/&&= perform NamedEvaluation per ECMA-262 §13.15.2 while arithmetic compound assigns do not — the operator set is exactly right.
  • The save/restore of prev_decorator_class_name at lines 121/182 already brackets all right-operand visits, so the new arms are covered without further changes.
  • Checked the sibling decorator_class_name sites in visit/mod.rs, visit_stmt.rs and visit_expr.rs — they cover declarators, destructuring defaults, property values and export default, none of which can carry a logical operator, so this is the only site that needed changing.
  • Tests cover all three operators (method + class decorator paths), the chained Outer = Inner ??= … case, and negative guards for named classes, member targets, nested classes, and +=.
Extended reasoning...

Overview

The PR extends one condition in src/js_parser/visit/visit_binary.rs: the block that records the assignment target's identifier name into p.decorator_class_name before visiting the right operand now fires for BinNullishCoalescingAssign, BinLogicalOrAssign and BinLogicalAndAssign in addition to BinAssign. Three new test.concurrent blocks are added to test/bundler/transpiler/es-decorators.test.ts.

Security risks

None. This is compile-time transpiler metadata (the .name given to a lowered anonymous class expression and the context.name seen by a class decorator). There is no user-input parsing, allocation sizing, or FFI change.

Level of scrutiny

Low-to-medium. The Rust change is a mechanical widening of an equality check into a four-arm matches!. Everything around it — the was_anonymous_named_expr gate, the EClass + should_lower_standard_decorators check, the EIdentifier left-side check, and the prev_decorator_class_name save/restore around the visit — is unchanged and already exercised by the existing BinAssign path. I verified that the restore at line 182 runs after every branch of the operator-dispatch match, so setting the name for the three new operators cannot leak into unrelated visits.

The spec claim checks out: ECMA-262 §13.15.2 defines the runtime semantics for &&=, ||= and ??= to call NamedEvaluation of the RHS with the LHS reference name when the RHS is an anonymous function/class definition, exactly as = does; the arithmetic/bitwise compound assignments do not. So the four-operator set is complete and does not over-apply.

Other factors

  • The chained-assignment test (Outer = Inner ??= class {…} → name Inner) confirms the inner-most NamedEvaluation position wins, which is what the save/restore stacking already produces.
  • Negative guards prove the change doesn't over-apply: an explicit class name still wins (is_anonymous_named is false), a member-expression target is still excluded (EIdentifier gate), a class not directly on the RHS is still excluded (EClass gate), and += is still excluded (operator gate).
  • I checked the other decorator_class_name write sites (visit/mod.rs for let x = … and destructuring/parameter defaults, visit_stmt.rs for export default, visit_expr.rs for object property values and array-pattern defaults). None of those grammatical positions accept a logical assignment operator, so visit_binary.rs is indeed the only site that needed this change, as the PR description states.
  • Tests use test.concurrent per repo convention for independent subprocess-spawning tests, assert stderr before exitCode, and use exact toEqual on structured JSON output.

Comment thread src/js_parser/visit/visit_binary.rs Outdated

@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 — extends the existing decorator_class_name propagation from = to ??=/||=/&&=, which are exactly the assignment operators the spec defines to perform NamedEvaluation.

What was reviewed:

  • Confirmed the four-operator set matches the ECMAScript NamedEvaluation rule; compound arithmetic/bitwise assigns are correctly excluded.
  • The save/restore of prev_decorator_class_name was already in place, so the chained Outer = Inner ??= class {} case and nested visits behave the same as for =.
  • New tests cover all three operators (method + class decorator paths), plus negative guards for explicit names, member targets, nested classes, and +=.
  • The comment-cop bot flag on line 124 looks like a false positive — the added line is a one-sentence spec citation, not a workaround justification.
Extended reasoning...

Overview

This PR widens a single condition in src/js_parser/visit/visit_binary.rs:124 so that p.decorator_class_name is recorded not only for BinAssign but also for BinNullishCoalescingAssign, BinLogicalOrAssign, and BinLogicalAndAssign. The rest of the block (anonymous-class check, should_lower_standard_decorators gate, identifier-only LHS, save/restore of the previous value) is unchanged. Three new test.concurrent blocks are added to test/bundler/transpiler/es-decorators.test.ts.

Security risks

None. This is a transpiler naming fix affecting the .name property and context.name of lowered decorated class expressions. No untrusted input parsing, no memory management, no FFI.

Level of scrutiny

Low-to-medium. The change is a mechanical widening of an existing, already-correct pattern to three sibling operators. I verified against the ECMAScript spec that exactly =, &&=, ||=, and ??= perform NamedEvaluation of an anonymous RHS with an identifier LHS — the arithmetic/bitwise compound assignments do not, and the code correctly excludes them. The consumer side in visit_expr.rs (decorator_name_from_context = p.decorator_class_name) reads the value the same way regardless of which operator set it, so no downstream change is needed.

Other factors

  • Test coverage is thorough for a change this size: positive cases for each operator via both the method-decorator path (.name was _class) and the class-decorator path (context.name was ""), plus negative guards (explicitly named class, member-expression target, class nested inside the RHS, +=) that pass before and after — so the change is scoped precisely to the spec-mandated cases.
  • No CODEOWNERS entry covers these files.
  • The github-actions "comment-cop" inline flag on the two-line comment appears to be a false positive: the added second line ("Only these four operators perform NamedEvaluation of the right side") is a concise spec citation explaining a non-obvious operator set, not a paragraph justifying a workaround. Per the repo's own guidance, that's exactly the kind of comment that belongs.
  • No prior human review comments to address; the bug-hunting system found nothing.

@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 — small, spec-aligned extension of the existing = NamedEvaluation path to cover ??=, ||= and &&=.

What was reviewed:

  • Verified the four operators listed are exactly the assignment operators the spec defines NamedEvaluation for; += and other compound ops are correctly excluded.
  • Confirmed the save/restore of prev_decorator_class_name and the EClass/should_lower_standard_decorators/EIdentifier guards are unchanged, so member targets and non-direct RHS classes stay excluded.
  • Checked the new tests cover method-decorator, class-decorator (context.name), chained assignment, accessor-only lowering, and negative guards (explicit name, member target, nested class, +=).
  • The comment-cop bot's feedback was addressed in 659065b (comment is now one line).
Extended reasoning...

Overview

This PR extends the decorator_class_name propagation in visit_right_and_finish (src/js_parser/visit/visit_binary.rs) from BinAssign alone to also include the three logical assignment operators. This makes lowered anonymous decorated classes assigned via ??=/||=/&&= receive the correct .name and class-decorator context.name, matching what the engine would produce natively per the NamedEvaluation rule. Three new test.concurrent blocks in es-decorators.test.ts cover the positive cases and the negative guards.

Security risks

None. This is a transpiler correctness fix that only affects the name string threaded through the standard-decorator lowering. No parsing of untrusted lengths, no allocation changes, no new code paths.

Level of scrutiny

Low-to-medium. The Rust diff is a single condition change — replacing an equality check with a four-arm matches! — inside an existing well-guarded block. The surrounding save/restore of p.decorator_class_name and the inner EClassshould_lower_standard_decoratorsEIdentifier gates are untouched, so the new operators inherit exactly the same restrictions as = already had. The spec claim (only = and the three logical assignments perform NamedEvaluation, not arithmetic/bitwise compound assignments) is accurate.

Other factors

  • The github-actions comment-cop flagged a long comment on an earlier commit; commit 659065b shortened it to a single line, which is what the diff now shows.
  • Test coverage is thorough: all three operators for both method and class decorators, the chained Outer = Inner ??= class {} case (correctly names Inner), an accessor-only class (lowered without decorators), and four guard cases confirming explicit names, member targets, nested classes, and += are still excluded. Tests use test.concurrent and the existing runDecorator helper, matching file conventions.
  • The PR description confirms cross-checking against Node's native behavior and passing the full decorator test suite plus clippy/fmt.

@robobun

robobun commented Aug 15, 2026

Copy link
Copy Markdown
Collaborator Author

Status: stacked on #38757, rebased onto its current head, waiting on CI.

Reproduced on bun 1.4.0 with the snippet in the description: ["_class","_class","_class","d"] (node on the same file without the decorators: ["a","b","c","d"]). The new tests in test/bundler/transpiler/es-decorators.test.ts fail on the base branch without the visit_binary.rs hunk (3 of 4; the fourth is a guard for the cases that must stay unnamed) and pass with it.

Why it is stacked: on main the recorded name is applied as a class binding that shadows the variable inside the class body, so recording it for these operators there would make b ??= class { @dec m() { b = 5 } } throw. On #38757 the name is applied with __name, and the fourth test pins that the body still sees the outer variable. Details in the description.

CI on the previous head had no test failures; the build was only marked failed because the macOS 14 aarch64 test lane expired without running.

@robobun
robobun changed the base branch from main to farm/80aecbe5/decorator-inferred-class-name August 15, 2026 12:58
@robobun
robobun force-pushed the farm/4055ec3e/decorated-class-logical-assign-name branch from 659065b to d33bb32 Compare August 15, 2026 12:58

@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 — extends the existing decorator_class_name propagation from = to the three logical assignment operators, matching the spec's NamedEvaluation rule.

What was reviewed:

  • Confirmed =, ??=, ||=, &&= are exactly the assignment operators that perform NamedEvaluation per ECMA-262; compound arithmetic/bitwise ops are correctly excluded.
  • The save/restore of prev_decorator_class_name around visit_expr is unchanged, so no leakage into unrelated right-hand expressions.
  • Traced decorator_class_name to its consumer in visit_expr.rs (e_class) — the new operators reach it via the same p.visit_expr(&mut e_.right) path as BinAssign already did.
  • Tests cover the positive matrix, context.name for class decorators, negative guards (named class, member target, nested, +=), and that no class binding is introduced.
Extended reasoning...

Overview

The PR widens one condition in src/js_parser/visit/visit_binary.rs: the block that records p.decorator_class_name before visiting the right operand of an assignment now fires for BinNullishCoalescingAssign, BinLogicalOrAssign and BinLogicalAndAssign in addition to BinAssign. Everything else — the was_anonymous_named_expr guard, the EClass+should_lower_standard_decorators guard, the EIdentifier left-hand guard, and the save/restore of the previous value — is byte-identical to before. Four new test.concurrent blocks in test/bundler/transpiler/es-decorators.test.ts exercise the fix and pin the negative cases.

Security risks

None. This is a parser-visitor change that only affects what string is threaded through to the standard-decorator lowering as the class's inferred .name. No untrusted input handling, no memory management, no FFI/JSC surface.

Level of scrutiny

Low-to-medium. The Rust change is a one-condition widening at a site that already handled BinAssign; the mechanism (decorator_class_namee_classname_from_context in the lowering) is pre-existing and unchanged. I checked the spec claim: ECMA-262 gives =, &&=, ||= and ??= the NamedEvaluation step for an anonymous function/class RHS with an identifier LHS, and no other assignment operators do, so the operator list is exactly right. The consumer side in visit_expr.rs reads p.decorator_class_name and clears it before visiting the class body, so the existing scoping still holds.

Other factors

  • The comment-cop bot flagged a paragraph-long comment on the first push; the author trimmed it to a single line in 659065b, which is what the current diff shows and which reads fine.
  • No CODEOWNERS cover src/js_parser/ or test/bundler/.
  • Tests follow the file's existing runDecorator harness pattern, use test.concurrent, assert stderr/stdout/exitCode in the recommended order, and include both positive coverage (all three operators, chained assignment, accessor-only lowering, class-decorator context.name) and negative guards (explicit name wins, member target, nested class, +=), plus a check that recording the name does not introduce a class binding that would shadow the outer variable.
  • The author reports the new tests fail on 1.4.0 and pass on the branch, and that the broader decorator/transpiler suites pass; the one red CI lane was an unrelated Windows bake test.

@robobun
robobun force-pushed the farm/80aecbe5/decorator-inferred-class-name branch from 68a8a87 to 1ed1f67 Compare August 15, 2026 13:20
…r &&= after the target

Standard decorator lowering rewrites a class expression to `_class = class {}`,
so the parser records the name the expression would have been given by its
position and the lowering restores it. The binary visitor only recorded that
name for `=`; the logical assignment operators perform the same NamedEvaluation
of their right side when the target is an identifier, so
`a ??= class { @dec m() {} }` ended up named "_class" (or "" with a class
decorator) instead of "a".
@robobun
robobun force-pushed the farm/4055ec3e/decorated-class-logical-assign-name branch from d33bb32 to b115ed8 Compare August 15, 2026 14:55

@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 — extends the existing decorator_class_name propagation from = to the three logical assignment operators, matching the spec's NamedEvaluation rule.

What was reviewed:

  • Confirmed ECMA-262 gives ??=/||=/&&= the same NamedEvaluation rule as = (identifier target only), and compound arithmetic operators do not — the operator list is exactly right.
  • The save/restore of prev_decorator_class_name and the inner guards (EClass, should_lower_standard_decorators, EIdentifier left) are unchanged; only the operator predicate widened.
  • Checked the later BinNullishCoalescingAssign | BinLogicalOrAssign arm (the {}.field ??= v optimization) — it requires EDot on the left, disjoint from the EIdentifier case here.
  • Tests cover the three operators (method + class decorators), negative guards (explicit name, member target, nested, +=), and the scoping interaction with the base PR.
Extended reasoning...

Overview

One hunk in src/js_parser/visit/visit_binary.rs: the condition that sets p.decorator_class_name before visiting the right operand of an assignment is widened from e_.op == BinAssign to a matches! over BinAssign | BinNullishCoalescingAssign | BinLogicalOrAssign | BinLogicalAndAssign. Everything else in the block — the anonymous check, the EClass/should_lower_standard_decorators/EIdentifier guards, and the save/restore of the previous name — is unchanged. Four new test.concurrent cases in test/bundler/transpiler/es-decorators.test.ts exercise the fix and pin the negative cases.

Security risks

None. This is transpiler output correctness (the inferred .name of a lowered decorated class expression); no untrusted-input parsing, no allocation logic, no FFI/syscall/crypto surface.

Level of scrutiny

Low-to-medium. The change is a mechanical widening of an operator predicate at a single visitor site, and the spec is unambiguous that exactly these four assignment operators perform NamedEvaluation of an anonymous right-hand side against an identifier target. The existing = path already validates the mechanism; three sibling operators are being added to the same path with no new control flow. The PR is stacked on #38757, and the fourth test pins the scoping property that base PR provides (the recorded name must not shadow the outer variable inside the class body).

Other factors

  • The comment-cop bot's feedback was addressed (comment trimmed to one line stating which operators perform NamedEvaluation).
  • The tests include both positive coverage and negative guards (+=, member target, nested class, explicit class name), and were cross-checked against Node per the description.
  • I checked the later match arm for BinNullishCoalescingAssign | BinLogicalOrAssign (the {}.field ??= value HMR optimization): it requires e_.left to be EDot, so it cannot overlap with the EIdentifier case that records the name.
  • The separate maybe_keep_expr_symbol_name call in the BinAssign arm (for --keep-names) is intentionally left as-is; the description notes #35307 handles keep-names for these operators separately, so this PR stays scoped to the decorator path.

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.

1 participant