Repository navigation
Conversation
|
Warning Review limit reached
Next review available in: 10 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 |
|
Updated 7:05 AM PT - Aug 15th, 2026
❌ @robobun, your commit 9ca0448 has some failures in 🧪 To try this PR locally: bunx bun-pr 38758That installs a local version of the PR into your bun-38758 --bun |
|
Status: ready for review, stacked on #38757 (which is stacked on #38922); base is #38757's branch at 68a8a87, and this lands after it and gets rebased onto main once it is in. Reproduced on main with The delta here is |
|
This overlaps with #38757 (opened a minute earlier), which removes the same binding for the expression path. One difference in placement matters: emitting function dec() {}
const Bar = class {
static field = this.name;
static #priv = this.name;
static priv() { return this.#priv; }
@dec m() {}
};
console.log(JSON.stringify({ name: Bar.name, field: Bar.field, priv: Bar.priv() }));
const Baz = @dec class { static field = this.name; };
console.log(JSON.stringify({ name: Baz.name, field: Baz.field }));#38757 avoids this by opening the class body with |
|
Updated for the review, in 4bf3526:
Tests added for each of these (static initializers with and without a class decorator, the computed and accessor |
There was a problem hiding this comment.
The three findings from my earlier review are addressed in 4bf3526/7c08532 and this pass found nothing new; given the change touches spec-ordering-sensitive decorator lowering and overlaps with #38757, a human sign-off on the approach and coordination is still warranted.
Checked in this pass: has_static_name_method_or_accessor runs on class.properties in Phase 1 before Phase 2 hoists computed keys, and IsMethod covers getters/setters (parse_property.rs:225). The __name static block is inserted at index 0 after the static_private_add_blocks prepend (lower_decorators.rs:2412-2438), so it also runs before lowered static #priv initializers. source_class_name reading the symbol's original_name for named statement classes preserves the pre-PR class-decorator name string. The bun build spawn now drains stdout.
Extended reasoning...
Overview
The PR fixes .name on lowered anonymous classes with standard (TC39) decorators by replacing the previous "attach the inferred name as a syntactic class binding" approach with a leading static { __name(this, "<name>") } block. It threads name_from_context through lower_class → lower_standard_decorators_stmt → lower_impl so the anonymous export default class statement path knows to emit "default" instead of the synthesized mod_default symbol name, and it unifies the class-decorator name string on the same source_class_name. Eleven new tests cover both export default forms, non-identifier and empty inferred names, static-initializer ordering with and without a class decorator, the outer binding staying visible inside the body, declared static name members (field/getter/method/decorated/computed/accessor), and bundler renaming.
Prior review round
My first-round review (on the suffix-__name shape) flagged that undecorated static fields stay in the body and would observe "_class", that the static-name guard ran after Phase 2 hoisted decorated computed keys, and a stdout-drain nit. All three are fixed in the current revision: the name is set from a leading static block inserted at new_properties[0] after the static_private_add_blocks merge (so it precedes both undecorated static fields and lowered static-private __privateAdd blocks), the guard now inspects the source class.properties in Phase 1 and only skips for methods/accessors (a static name field runs after the block and wins on its own), and the build subprocess drains all three streams. All prior inline threads are resolved. The comment-cop flags were addressed by trimming to short pointers in 7c08532; the remaining comments document ordering invariants the first review round tripped over.
Security risks
None. Transpiler output-shape correctness for decorator lowering; no untrusted-input parsing surface added, no auth/crypto/permissions.
Level of scrutiny
Medium-high. This is spec-driven transpiler output in a path with subtle ordering (SetFunctionName vs. static element evaluation vs. relocated blocks vs. __decorateElement). The first revision had a real ordering regression, which argues for a maintainer confirming the static-block approach and its interaction with the rest of Phase 7. The author also noted overlap with #38757 (which uses the same leading-static-block shape for the expression path); a human should decide how the two land relative to each other.
Other factors
Test coverage is thorough (11 new tests, 9 verified to fail on the unfixed build; esbuild's 147-test decorator suite and the other decorator/transpiler files reported passing). The two CI failures (test-http-chunk-problem.js, bake/deinitialization.test.ts) are unrelated to the parser.
7c08532 to
48c88dc
Compare
|
Restructured to avoid overlapping with #38757, which was opened a minute before this PR and makes the same expression-path change (drop the class binding, restore the name from a leading This PR is now stacked on that branch (base set to The description is rewritten for the current diff; the earlier revisions are summarized in its details block. Once #38757 lands this gets rebased onto main. |
There was a problem hiding this comment.
I re-reviewed after the restructure onto #38757 and the switch to the leading static { __name(this, ...) } block — the three findings from the earlier revision (undecorated static fields observing _class, the defines_static_name_method guard missing @dec static ["name"], and the undrained build stdout) are all addressed, and this run found no new issues. Since it is stacked on an unlanded PR and changes spec-observable decorator lowering, a human look before merge is still worthwhile.
What was reviewed:
defines_static_name_methodnow runs on source properties before Phase 2 rewrites computed keys; verifiedIsMethodcovers getters/setters so the guard matches the test's Getter/Setter/Method/Accessor cases.- The
!is_expr && name_from_context.is_some()anonymity check — onlys_export_defaultsets it (when injecting the default-export symbol), and the otherlower_classcall site passesNone, so a named class stays untouched (covered by theNamedtest). new_properties.insert(0, block)places the__nameblock ahead of every retained static element, including undecorated static fields and the__privateAddblocks for static privates — thestatic field/static #priv/Field.seenBeforetests exercise that ordering.
Extended reasoning...
Overview
The PR threads a name_from_context: Option<&[u8]> parameter from visit_stmt.rs (s_export_default) through p.rs::lower_class into lower_decorators.rs::lower_standard_decorators_stmt and lower_impl, so an anonymous export default class { @dec ... } statement is treated as anonymous by the standard-decorator lowering. Combined with the base PR (#38757), the lowered class gets a leading static { __name(this, "default") } block and __decorateElement receives "default" as the class name, instead of the module's synthesized mod_default symbol leaking into .name and context.name. The diff shown is against main, so it also includes #38757's expression-path change (removal of can_be_class_binding_name, the defines_static_name_method helper, and the restore_inferred_name block insertion at Phase 7).
Security risks
None. This is a pure transpiler-output correctness change; no untrusted-input parsing paths, allocation sizing, or privilege boundaries are touched.
Level of scrutiny
Medium-high. Decorator lowering is spec-sensitive and the earlier revisions of this PR had real ordering bugs (undecorated static fields observing the temporary _class name; the name guard running after Phase 2 had rewritten computed keys). Those are now fixed by inserting the __name call as the first static block and computing the guard from class.properties before any rewriting. I traced IsMethod through parse_property.rs to confirm it is set for getters and setters, so the guard's IsMethod || AutoAccessor predicate matches all the method-shaped cases the test exercises; fields are intentionally excluded because they run after the leading block and win on their own. The one undecidable case (a runtime-computed key evaluating to "name") is acknowledged and unchanged from #38757's shape — it is not a regression relative to that base.
Other factors
- Stacked PR. The base is #38757, not
main. Approving this in isolation would be premature; the combined diff should land together or #38757 first. - Test coverage. Four new statement-form tests (member decorators, TypeScript variant, class decorator, named-class negative), plus the expression-path suite from #38757 (every naming context, non-identifier names, static-initializer ordering, the nine-variant static
namemember matrix, bundler rename/minify survival). TheField.seenBefore === "Field"assertion specifically pins the leading-block ordering that the first revision got wrong. - Prior review resolved. All three of my earlier inline findings and the comment-cop notes are addressed in the current commits (4bf3526 → 8469ac3); the author's replies match what the diff now does.
- Legacy path.
lower_classonly forwardsname_from_contexttolower_standard_decorators_stmt; the legacyexperimentalDecoratorsbranch in the same function ignores it, matching tsc'sdefault_1behavior as noted in the description.
|
Agreed on the order: #38757 first, then this gets rebased onto main and re-run; nothing here is meant to be merged on its own. |
8469ac3 to
fc5e78a
Compare
4997c18 to
e32d6e9
Compare
…ns with __name
Standard-decorator lowering rewrites `const Bar = class { @dec m() {} }`
into `_class = class ... {}`, which no longer infers the name "Bar". The
lowering restored it by giving the class a binding named after the
context. That binding collides with the `Bar` declared in the same scope,
so the bundler renamed it (`class Bar2`, or a minified name), it could
only carry identifier names (object keys such as "foo-bar" or "default"
fell back to "_class"), and it shadowed the outer `Bar` inside the body.
Leave the class anonymous and open its body with
`static { __name(this, "Bar") }` instead, so the name is a string literal
the renamer never touches and is in place before any static initializer
left in the body runs. Classes with no naming context get "" like an
undecorated anonymous class.
Static methods and accessors are installed on the constructor before any static block runs, so the injected __name block would overwrite a `static get name()` the class declares itself. Skip the block when the emitted body has a static member keyed `name`, the same rule esbuild applies for --keep-names.
Only static methods and accessors keyed `name` need to suppress the
__name block: they are installed before static blocks run. A `static
name` field runs after the block and replaces the name itself, and
skipping the block for it left earlier static initializers reading
"_class". The check now also runs before decorated computed keys are
replaced with temporaries, so `@dec static ["name"]() {}` is seen.
A decorated `static accessor name` is installed from the suffix, so the body has nothing the __name block could overwrite; skipping the block left earlier static initializers reading "_class".
e32d6e9 to
68a8a87
Compare
|
Another rebase heads-up: #38757 is now itself based on #38922 (the formatter has to display a name set with Object.defineProperty, otherwise classes lowered this way print as |
The visitor gives an anonymous `export default class` the module's default-export symbol as its class name so that decorator lowering has a binding to refer to, and the lowering then treated that symbol as the class's real name: `.name` came out as `mod_default`, and a class decorator received it as context.name. Pass "default" down as the class's name-from-context in that case so the lowering restores it with the same leading static block it already emits for anonymous class expressions and hands it to __decorateElement.
fc5e78a to
d7497b6
Compare
There was a problem hiding this comment.
Beyond the inline nit, I checked that both lower_class statement callers are updated (the non-default s_class path passes None, so named declarations are unaffected), that the widened class_is_anonymous at the class_name_str branch is behaviour-identical for expressions (original_class_name_for_decorator still takes precedence for named exprs), and that the legacy-decorator path in lower_class never sees the new argument.
Extended reasoning...
The current delta is a small mechanical extension of #38757's mechanism: thread name_from_context through lower_class → lower_standard_decorators_stmt → lower_impl, and widen two conditions from expr_class_is_anonymous to class_is_anonymous. I traced each condition's callers and confirmed the expression path is unchanged; the only new behaviour is the anonymous export default class statement getting the leading __name block and "default" as the __decorateElement name. The one open item is the 07:12 suggestion for a statement-form console.log(Cls) assertion, which the inline nit covers.
cdec8c8 to
9ca0448
Compare
There was a problem hiding this comment.
LGTM — the console.log/Bun.inspect assertion from the last round is in (9ca0448), and the statement-form delta on top of #38757 looks correct.
What was reviewed:
name_from_contextthreading: only twolower_classcallers exist, both updated; only the anonymous-default branch sets it, so named default exports and ordinary class statements passNoneand are unaffected.class_is_anonymouswidening inlower_impl: for statements it reduces toname_from_context.is_some(), so the__decorateElementname string andrestore_inferred_nameboth pick up"default"only for the injected-symbol case.- Tests cover the member-decorator and class-decorator statement forms, TS variant, the named-class negative case, the static
nameguard, and now formatter output.
Extended reasoning...
Overview
This is the statement-form delta stacked on #38757. It threads an Option<&[u8]> name-from-context through lower_class → lower_standard_decorators_stmt → lower_impl, set to ClauseItem::DEFAULT_ALIAS (b"default") exactly when s_export_default injects the module's default-export symbol as an anonymous class's binding. lower_impl then treats such a statement as anonymous (widening expr_class_is_anonymous to class_is_anonymous), so it gets #38757's leading static { __name(this, "default") } block and __decorateElement receives "default" instead of "mod_default". Five new tests in es-decorators.test.ts cover the variants; a runFiles helper is added alongside the existing runDecorator.
Changes since last review
My only outstanding note (08:24Z) was a nit asking for a console.log(Cls) / Bun.inspect assertion so the formatter path is pinned for the statement form. Commit 9ca0448 adds console.log(Cls.name, Cls, Bun.inspect(new Cls(), { compact: true })) to the first test (asserting [class default] default {}) and console.log(Cls.name, Cls) to the class-decorator test (asserting [class default]). That covers the ask; nothing else changed. All earlier findings (undecorated static field ordering, the defines_static_name_member computed-key gap, undrained stdout) were about the pre-restructure revisions and now live in #38757, whose mechanism this PR reuses without modification.
Security risks
None. This is compile-time transpiler output shaping for a spec-compliance fix (anonymous default-export class .name). No untrusted-input parsing, no allocation sizing driven by external data, no privilege or filesystem surface.
Level of scrutiny
Medium — js_parser is production-critical, but the diff is ~15 source lines that plumb one optional parameter and widen one boolean. I verified there are exactly two lower_class call sites (both updated), that DEFAULT_ALIAS is the existing b"default" constant used elsewhere in visit_stmt.rs for the same purpose, and that the legacy TS-decorator path in lower_class does not consume the new argument (matching the description's note that legacy lowering is unchanged).
Other factors
- The bug-hunting system found no issues on this revision.
- Test coverage is thorough for the delta: statement form with a static field / static block / static-method initializer all observing
"default", the TypeScript entry point,@dec export default class {}checkingctx.name/cls.name/the import, a named default export as the negative case, and the static-nameguard interacting with a getter vs. a decorated accessor. All follow the file's harness conventions (test.concurrent,tempDir, all three pipes drained, stderr/stdout asserted before exit code). - This is stacked; it merges into #38757's branch and gets rebased onto main after that lands. The delta reviewed here should be identical post-rebase.
|
9ca0448 adds the display assertions: the first statement-form test logs the class and an instance ( |
68a8a87 to
1ed1f67
Compare
|
The base of this PR, #38757, is closed. #40833 (a22b2aa) superseded it: the merged lowering has no The statement form that this PR fixes still fails on main. Measured on a debug build of 09bb546 under
The cause on main is the same as before. This PR needs new code on top of the
|
|
#44723 passes the name |
Status: the former base of this PR, #38757, is closed. #40833 superseded it. This PR now has main as its base and needs a rebase. Until then the diff also shows the old commits of #38757 and #38922. The own commits of this PR are 8b5c474, a547191, d7497b6 and 9ca0448. The text below describes the PR as it was on top of #38757. See #38758 (comment) for the cases that still fail on main.
Problem
export default classstatement gets the wrong name:export default class { @dec m() {} }inmod.jshas.name === "mod_default"; without the decorator it is"default".@dec export default class {}(decorator beforeexport) sets.nameto"mod_default"and passes it to the class decorator ascontext.name. Theexport default @dec class {}andexport default (class { ... })forms are expressions and are handled by js_parser: keep the inferred name of lowered anonymous decorated class expressions #38757.s_export_default(src/js_parser/visit/visit_stmt.rs:824) gives an anonymous class the module's default-export symbol as its class name, because decorator lowering needs a binding to refer to the class by.lower_implinsrc/js_parser/lower/lower_decorators.rscannot tell that binding apart from a user-written name, so the emitted declaration isclass mod_default { ... }and the__decorateElementname argument is"mod_default".Fix
s_export_defaultpasses"default"down as the class's name-from-context (throughlower_classandlower_standard_decorators_stmt) exactly when it injects the default-export symbol; every other statement passesNone, so a named class is unchanged.lower_impltreats a statement with a name-from-context as anonymous: it gets the same leadingstatic { __name(this, "default"); }block js_parser: keep the inferred name of lowered anonymous decorated class expressions #38757 emits for anonymous class expressions (same staticnamemember guard), and"default"is what__decorateElementreceives when there are class decorators, so.nameandcontext.nameare both"default"."default"at class definition, before its static elements run and before decorators are applied; the block runs at that point, and themod_defaultsymbol stays a purely internal binding (it is still what the module exports asdefault). LegacyexperimentalDecoratorslowering ignores the argument and is unchanged (tsc also emits a generated name there,default_1).test/bundler/transpiler/es-decorators.test.ts,export default classblock: the statement form (checking a static field initializer, a relocated static block and a static method initializer all observe"default", and thatconsole.logshows[class default]anddefault {}rather than themod_defaultbinding), the TypeScript variant,@dec export default class {}(context.name, the class passed to the decorator, the import, and itsconsole.logoutput), a default export declaring its own staticnamegetter (block suppressed) next to one with a decoratedstatic accessor name(block emitted, an earlier static field sees"default"), and a named@dec export default class Namedthat must keep reportingNamed. Built on js_parser: keep the inferred name of lowered anonymous decorated class expressions #38757 (68a8a87) without the source change, four of these fail withmod_defaultand the named one passes; with it all 72 tests in the file pass.es-decorators-esbuild.test.ts(esbuild's 147 decorator tests),decorators.test.ts,decorator-metadata.test.ts,bundler_decorator_metadata.test.ts.Background
export default class {}has no binding of its own in the source; the module'sdefaultexport refers to it through a synthesized symbol (printed asmod_default). Decorator lowering emits the class as a declaration and then statements that reference it (__decorateElement(...), relocated static blocks), so it needs that symbol to be the declaration's name.static { __name(this, "<name>") }block at the top of the body, which runs before any other static element;name_from_contextis the inferred name it uses. This PR feeds the statement path into that same mechanism. inspect: display a class or function name set with Object.defineProperty #38922 makesconsole.log/Bun.inspectdisplay a name set this way, which is why the display assertions here need the full stack.Emitted code and earlier revisions
export default class { @dec m() {} }now lowers to:and
@dec export default class {}passes"default":d_default = __decorateElement(_init, 0, "default", _dec, d_default).Earlier revisions of this PR (50085c0, 4bf3526) also replaced the expression path's class binding, first with a
__namecall after the class and then with the leading static block. #38757, opened a minute earlier, makes the same expression-path change with the same guard, so this PR was reduced to the statement-form delta on top of it. The review findings on the earlier revisions (undecorated static fields running before an appended__name; the guard missing@dec static ["name"]()) are addressed in #38757's shape, which this commit reuses.