js_parser: keep the inferred name of lowered anonymous decorated class expressions - #38757
js_parser: keep the inferred name of lowered anonymous decorated class expressions#38757robobun wants to merge 9 commits into
Conversation
|
Warning Review limit reached
Next review available in: 2 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 (3)
Comment |
|
Reproduced on bun 1.4.0 and a debug build of main: Fix in this PR: the lowering no longer names the class binding after the context; it opens the class body with Stack: this PR is based on #38922 (the formatter has to display a redefined name, otherwise these classes would print as CI: the parser change alone was green on main (build 97393) and on the stack (97570); the current head 676ad01 (build 98188) and the current base 5727b0f (#38922, build 98123) each have only retried flakes, plus, on 98188, the Windows |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
On the overlap with #38758: both remove the synthesized binding and set the name with |
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js_parser/lower/lower_decorators.rs:2423-2435—defines_static_namescansnew_propertiesafter Phase 3 has already replaced every decorated computed key with anEIdentifiertemp, so@dec static ["name"]() {}no longer matches theEString("name")check — the__nameblock is injected, runs after the method is installed, and clobbers it back to a data property (the decorator then receives the string"Foo"instead of the function). Pre-PR handled this correctly via the binding name. Compute the has-static-nameflag from the originalclass.propertiesbefore Phase 3 rewrites the keys (the dynamicstatic [k]()case stays inherently undetectable, matching esbuild's--keep-nameslimitation).Extended reasoning...
What the bug is
The
defines_static_nameguard added in commit 337ccf8 only matches a static property whose key isExprData::EString(s)withs == "name"(src/js_parser/lower/lower_decorators.rs:128-137). It is called at line 2423 againstnew_properties, which is built in Phase 4 fromclass.properties— but Phase 3 (lines 1259-1281) has already iteratedclass.properties.slice_mut()and, for every property withIsComputedset and at least one decorator, replacedprop.keyin place withp.use_ref(key_ref, ...), anEIdentifierpointing at a_computedKeytemporary. So a decorated static method written as@dec static ["name"]() {}reachesdefines_static_namewithkey.data == EIdentifier(_computedKey), the guard returnsfalse, and thestatic { __name(this, "Foo") }block is injected.Step-by-step proof
Given:
function dec(fn, ctx) { console.log(typeof fn); return fn; } const Foo = class { @dec static ["name"]() { return 1; } }; console.log(typeof Foo.name);
- Phase 3 (line 1259): the property has
IsComputedandts_decorators.len_u32() > 0, soprop.keyis rewritten fromEString("name")toEIdentifier(_computedKey)(line 1280), and_computedKey = "name"is emitted as a pre-eval statement. - Phase 4: the decorated public static method is copied into
new_propertiesviaprop_copy(prop), which shallow-copieskey: p.key— now theEIdentifier. - Line 2423:
defines_static_name(&new_properties)sees only anEIdentifierkey, returnsfalse, andstatic { __name(this, "Foo") }is inserted at index 0. - Emitted body (in order):
static { __name(this, "Foo") }, thenstatic [_computedKey]() { return 1; }. - At runtime, per ClassDefinitionEvaluation:
SetFunctionName(F, "_class")runs at step 12; static methods/accessors are installed on the constructor at step 25 (soF.namebecomes the user's method); then static blocks execute at step 31 — the injected__name(this, "Foo")runsObject.defineProperty(F, "name", {value: "Foo", configurable: true}), reconfiguring the accessor/method back to a plain data property. The user's method is discarded. - In the suffix,
__decorateElement(_init, 9, _computedKey, [dec], _class)readsObject.getOwnPropertyDescriptor(_class, "name"), which is now{value: "Foo", …}, and passes"Foo"(a string) to the decorator instead of the original method.
Native and pre-PR output:
function/function. Post-PR output:string/string.Why this is a regression
Before this PR, the removed block at lines 1145-1152 gave the class a syntactic binding name:
_class = class Foo { static [_computedKey]() {} }. In that shapeSetFunctionName(F, "Foo")runs at step 12, before step 25 installs the static method — so the method'sDefinePropertyOrThrowruns last and survives. There is no injected static block to re-clobber it. So the string-literal computed form worked pre-PR and is broken post-PR.Why the existing guard doesn't catch it
defines_static_namematches onlyEString. There is no parser/visitor pass that clearsIsComputedfrom constant string-literal computed keys in class members, so["name"]keeps itsIsComputedflag, Phase 3 hoists it, and by the time the guard runs theEStringis gone. The undecorated computed form (static ["name"]() {}without@dec) is unaffected — Phase 3 only hoists decorated computed keys, so itsEStringreachesnew_propertiesintact and the guard fires. The PR's ownDecoratedMethodtest (@dec static name() {}, non-computed) also passes for the same reason.Impact
Any anonymous decorated class expression with
@dec static ["name"]() {}(orget/setvariants) has that member silently replaced with a data property, and the decorator is handed the string class name instead of the function. This is not just a naming glitch — it destroys user code and hands the decorator garbage. The dynamic formconst k = "name"; @dec static [k]() {}has the same failure and is fundamentally undetectable at compile time; that is the known trade-off of the__name-block approach vs. the removed binding-name approach and matches esbuild's--keep-nameslimitation. But the string-literal form is statically knowable and pre-PR handled it correctly.How to fix
Compute the has-static-
nameflag from the originalclass.propertiesbefore Phase 3 mutates the keys (e.g. right afterexpr_class_is_anonymousis set), still restricting to static elements with anEString("name")key. Then use that captured flag at line 2423 instead of scanningnew_properties. Alternatively, skip the Phase-3 key hoist when the computed key is already a side-effect-free constant literal — that also lets the existingEStringmatch fire and avoids emitting a useless_computedKey = "name"temp — but the first option is the smaller change. - Phase 3 (line 1259): the property has
|
On the computed-key finding from the review ( |
|
#38758 had converged on the same expression-path change as this PR (static block plus the source-property guard), so it has been reduced to the statement form that this PR leaves alone ( |
820428e to
4997c18
Compare
There was a problem hiding this comment.
I re-reviewed the current revision and found no new issues — the earlier findings (static name getter/method clobbering, plain static name fields over-matching the guard, decorated static accessor name suppressing the block) are all addressed, and the remaining dynamic-computed-key edge is documented as a known limitation matching esbuild. Given the ClassDefinitionEvaluation-ordering subtlety this went through and the coordination with #38758's stacked statement-form fix, a maintainer sign-off on the final shape would still be worthwhile.
What was reviewed:
defines_static_name_methodguard on source properties: static method/getter/setter, undecoratedaccessor, decorated["name"]— each maps to whether the emitted body keeps anameelement; matches the tests.restore_inferred_namecomputed pre-Phase-2 so decorated computed keys still have their literal key; block inserted at index 0 via existingmake_static_block/call_rt.can_be_class_binding_nameand its imports removed with no remaining callers;is_eval_or_argumentsstill used elsewhere inparser.
Extended reasoning...
Overview
The PR changes src/js_parser/lower/lower_decorators.rs (~40 lines net) to fix the inferred .name of anonymous decorated class expressions after standard-decorator lowering. Instead of synthesizing a class binding named after the context (which the bundler's renamer then mangles to Bar2, and which cannot hold non-identifier names, and which shadows the outer variable inside the class body), it now injects static { __name(this, "<name>") } at the top of the class body. A new defines_static_name_method guard skips the block when the source class already installs a body-resident static name method/getter/setter/undecorated-accessor. Ten new tests span bundler_edgecase.test.ts (bundled + minified + CJS-wrapped) and es-decorators.test.ts (naming contexts, no-context, outer-binding visibility, static-initializer ordering, static name matrix, decorated static accessor).
Security risks
None. This is transpiler output-shape only; no untrusted input parsing, no I/O, no allocation of user-controlled sizes.
Level of scrutiny
Medium-high. It changes emitted code for every anonymous decorated class expression, and correctness depends on the exact two-pass evaluation order in ClassDefinitionEvaluation (methods installed before static blocks/fields run in source order). Getting the guard right took three rounds of iteration on this PR — each round found a real observable regression in an edge of the static-name matrix. The final guard now correctly distinguishes body-resident members (methods, undecorated accessors) from suffix-installed ones (fields, decorated accessors), and reads source properties before Phase 2's computed-key hoisting. The test coverage is thorough and cross-checked against Node.
Other factors
- All prior inline findings from this reviewer are resolved; the last one (dynamic
[k]keys evaluating to"name") is a compile-time-undecidable case explicitly accepted as a known limitation in the PR description, matching esbuild's--keep-names. - CI is green on all decorator/bundler suites; the sole red test (
test-http-chunk-problem.js) fails identically on main. - #38758 has been rescoped to the
export default class { … }statement form and is stacked on this branch; a maintainer should confirm that landing order. - Not approving because parser lowering changes with this much semantic subtlety, plus a related stacked PR to coordinate, warrant a human look even though the automated review is now clean.
4997c18 to
e32d6e9
Compare
e32d6e9 to
68a8a87
Compare
|
Review on #38787 (stacked on this branch) turned up a small gap in this PR's |
…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".
68a8a87 to
1ed1f67
Compare
A computed key that same-file enum inlining turned into
EInlinedEnum(EString("name")) is a static `name` member too.
There was a problem hiding this comment.
The EInlinedEnum case from the last review is now addressed (commit 676ad01: key.unwrap_inlined().data plus a dedicated TS-enum test), and this pass found no further issues. Given the number of spec-interaction edge cases this went through (static name method/field/accessor ordering, decorated vs undecorated accessors, computed keys, enum inlining) and the accepted limitation on runtime-computed name keys, a maintainer look at the final shape would still be worthwhile before merging.
What was reviewed:
defines_static_name_method:IsMethod/ undecoratedAutoAccessorgating,unwrap_inlined()on the key, run against source properties before Phase 2 rewrites decorated computed keys.- Static block placement at index 0 so it precedes body-resident static field initializers and
__privateAddblocks; suffix-installed members (decorated accessors, class decorator) still overwrite afterward. - Removal of the synthesized class binding no longer shadows the outer variable inside the class body — covered by the "class body still refers to the outer binding" test.
- Bundler/minifier tests confirm the string-literal name survives
NumberRenamerand identifier minification.
Extended reasoning...
Overview
This PR changes how the standard-decorator lowering (src/js_parser/lower/lower_decorators.rs) preserves the inferred .name of an anonymous decorated class expression. Previously it synthesized a class binding symbol named after the context, which the bundler's renamer would then rename (Bar → Bar2) and which could not hold non-identifier names. The new approach drops the binding entirely and instead injects static { __name(this, "<name>") } as the first class body element, guarded by defines_static_name_method so a user-declared static name method/getter/setter/undecorated-accessor is not clobbered. can_be_class_binding_name and its imports are removed. Three bundler tests and eight transpiler tests are added covering naming contexts, non-identifier names, static-initializer ordering, the static-name guard variants, decorated static accessors, inlined TS enum keys, and console.log/Bun.inspect display (the latter depends on base PR #38922).
Security risks
None. This is compile-time AST rewriting with no untrusted-input parsing beyond what the JS parser already does; the injected helper (__name) is an existing runtime.js export that __decorateElement already calls.
Level of scrutiny
High — this is spec-sensitive transpiler output. ClassDefinitionEvaluation ordering (methods installed before static blocks/fields run) is exactly what four earlier review rounds turned on: the initial unconditional block clobbered a user static get name(); narrowing to any static name over-matched fields; the auto-accessor arm over-matched decorated accessors that are lifted to the suffix; and the key match missed EInlinedEnum-wrapped strings. Each was fixed with a corresponding test, and this pass found nothing new. The remaining known gap — a runtime-computed key evaluating to "name" — is compile-time undecidable, matches esbuild's --keep-names behavior, and is documented in the PR description.
Other factors
- Stacked on #38922 (formatter change so
console.logshows the__name-set name); one of the new tests pins that output, so merge order matters. - Design choice (leading static block vs. trailing
__name(_class, ...)as in #38758) is deliberate and justified: only the block runs before body-resident static field initializers, sostatic x = this.namesees the right value. #38758 has been re-scoped to the statement-formexport default classcase this PR does not touch. - Test coverage is thorough and each earlier finding has a pinning test; the existing esbuild decorator conformance suite is stated to pass.
Given the iteration history and the subtlety of the ordering semantics, deferring for a maintainer to confirm the final guard shape and the accepted computed-key limitation rather than auto-approving.
Stacked on #38922 (this PR's base branch): the formatter change there is what keeps
console.logof these classes printing[class Bar]once the binding is gone (see Fix). Merge #38922 first; GitHub retargets this one to main.Problem
.nameonce the module is bundled:bun repro.jsprintsBar;bun build repro.jsprintsBar2(the output contains_class = class Bar2 {), and--minifyprints a one-letter name. The same happens at the top level of a CommonJS-wrapped module, and foraccessormembers, which use the same lowering. It does not happen with a class decorator only because__decorateElement(_init, 0, "Bar", ...)re-applies the name at runtime.lower_implinsrc/js_parser/lower/lower_decorators.rsrewrites the expression to_class = class {}, which would infer the name"_class", and compensated by creating a symbol named after the context (Bar) and making it the class's own binding name. That symbol lands in the scope that also declares the user'sconst Bar, and the bundler's renamer (assign_names_in_scopeinsrc/js_printer/renamer.rs) renames the secondBarit meets in a scope; declaring it in the class's own scope would not help, since the renamer also renames bindings that shadow an enclosing name (bun buildturns an explicitconst Bar = class Bar {}intoclass Bar2today)."_class":{ "foo-bar": class { @dec m() {} } },{ default: ... },{ "": ... },export default (class { @dec m() {} })(node:"foo-bar","default","","default"), and a class with no naming context at all is named"_class"instead of"";let Bar = class { m() { return Bar } ... }; Bar = 1,m()returns the class instead of1.Fix
static { __name(this, "Bar") }("Bar"being the context name, or""when there is none); the class decorator path still passes the same name to__decorateElementas before.Barto the outer variable exactly as the source did. A static block runs during class definition, before the static field initializers the lowering leaves in the body (static x = this.name) and before the generated__privateAddblocks, so everything that could observe the name still sees it; putting the__namecall after the class expression would not. This is the shape esbuild emits for the same situation under--keep-names.accessorkeyedname(defines_static_name_method, checked on the source properties before decorated computed keys are replaced by temporaries, so@dec static ["name"]() {}counts, and looking through the wrapper same-file enum inlining leaves on a key, sostatic [Key.Name]() {}withName = "name"counts too): those are installed from the class body before static blocks run, so the block would overwrite them, which a class binding never did. Members the lowering installs from the suffix (astatic namefield, decorated or not, and a decoratedaccessor) do not suppress the block: they replace the name after it, and static initializers before them still have to read the inferred name. Known limitation: a computed key that only evaluates to"name"at runtime (static [k]() {}) is not detected and the block overwrites that member, as esbuild's--keep-namesalso does; suppressing the block for every computed static key would instead drop the name for classes withstatic [Symbol.iterator]()and the like.console.log,Bun.inspectand bun:test name a class from its executable, which for the lowered class is the_classtemporary, so with the binding gone they printed[class _class]/_class {}where 1.4.0 printed[class Bar]/Bar {}(.nameitself was right). inspect: display a class or function name set with Object.defineProperty #38922, the base of this PR, makes them use anameset withObject.defineProperty, which is also what tsc-compiled decorated classes need today; the newconsole.log and Bun.inspect show the inferred nametest pins[class Item] Item { ... },[class Child extends Item]and, for a class with no naming context,[class (anonymous)]for this PR's output.__nameis an existingruntime.jshelper (__decorateElementalready calls it for class decorators), so bundled and unbundled output both have it;can_be_class_binding_name(the identifier filter for the old binding) is removed with the binding.test/bundler/bundler_edgecase.test.ts:DecoratedAnonymousClassExprKeepsInferredName(function scope:const,accessor-only class, assignment, non-identifier key), the same input withminifyIdentifiers/minifySyntax/minifyWhitespace, and a CommonJS-wrapped module. On the unfixed build they print["Bar2","Baz2","Qux2","_class4"],["e","x","q","y"]andBar2.test/bundler/transpiler/es-decorators.test.ts, newinferred names of lowered anonymous class expressionsblock: non-identifier names, every naming context (const, assignment, destructuring defaults, object key, static and instance class field, class decorator), no context gives"", the class body still sees the outer binding after reassignment, and the name is visible to inline static fields, a static private field initializer and a static block, with and without a class decorator. Theexport default (class { @dec foo() {} })test now also checks.name === "default". A further test covers classes declaring their own staticnamegetter, setter, method, decorated method, decorated computed["name"]method, a method keyed by an inlined TypeScript enum member (.tsfile),accessor, and a field or decoratedaccessorpreceded by an initializer that readsthis.name. Expectations were cross-checked against node on the same code with the decorators removed. 4 of these fail on the unfixed build; the naming-context, static-initializer and static-nametests are regression guards for dropping the binding.es-decorators-esbuild.test.ts(esbuild's 147 conformance tests),es-decorators.test.ts,decorators.test.ts,decorator-metadata.test.ts,bundler_decorator_metadata.test.ts,ts-use-define-for-class-fields.test.ts,regression/issue/27575.test.ts,transpiler.test.js,esbuild/ts.test.ts,esbuild/default.test.ts,bundler_edgecase.test.ts,bundler_minify.test.ts,bundler_naming.test.ts;cargo clippy -p bun_js_parseris clean.Background
.namefrom where it appears (const Bar = class {},{ Bar: class {} },Bar = class {}, a default value in a destructuring pattern,export default). It only applies when the expression is directly in that position; once the lowering wraps it in_class = class {}, ...the inferred name becomes_class. The parser records the position's name indecorator_class_nameand passes it to the lowering asname_from_context._classtemporary, followed by a comma list of__decorateElement(...)calls. Decorated and relocated static elements run in that suffix, but undecorated static fields stay in the class body and are evaluated while the class is being defined.static { ... }) run in source order with the static field initializers during class definition, withthisbound to the class, so a block placed first runs before any other static initializer.NumberRenamer: it walks each scope, keeps the first symbol with a given name, and renames later ones and ones that shadow a name from an enclosing scope by appending a number. It never sees string literals.__name(target, name)insrc/runtime.jsdefines thenameproperty on a function, the same helper the class decorator path uses.__name(_class, ...)after the class expression, and additionally fixes theexport default class { @dec m() {} }statement form. Built at 50085c0,const Bar = class { static x = this.name; @dec m() {} }givesBar.x === "_class"there ("Bar"on 1.4.0, node and this PR), because the class body has already run by then; that is what the static-block placement here is for. The statement-form fix is independent of this PR and can land on top of it.Emitted code for the repro, before and after
Before (
bun build):After:
no test proof · iteration 4 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts