Repository navigation
js_parser: keep function, base class and static name names through standard decorator lowering - #42588
js_parser: keep function, base class and static name names through standard decorator lowering#42588robobun wants to merge 4 commits into
name names through standard decorator lowering#42588Conversation
…dard decorator lowering
A decorated field or an accessor passes its initializer to a runtime helper as
a call argument, where an anonymous function or class is no longer named by
the field. It is now wrapped as `{ key: value }[key]`, like the initializer of
a field that carries the effects of its neighbors.
The class a decorated class extends was kept in a `_base` temporary for
`__decoratorStart`, so an anonymous class or function in the extends clause was
named after the temporary. `__decoratorStart` now reads the prototype of the
class itself and the extends clause stays as written.
`__decorateElement` set the name of a class with class decorators after its
static methods and accessors were defined, which replaced a static member
called `name`. It now only replaces the string every class starts with.
scripts/build/codegen.ts: runtime.out.js is bundled from runtime.bun.js, which
re-exports runtime.js. Only the former was an input of the build edge, so an
edit to runtime.js did not rebuild it.
|
Warning Review limit reached
On-demand reviews are free for the next 7 days. After that, they cost $0.25 per reviewed file. Or wait 6 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (5)
Comment |
|
Updated 1:50 PM PT - Sep 13th, 2026
✅ @robobun, your commit 3d2466135600742927e3ee2771bad5ce6ee8d18c passed in 🧪 To try this PR locally: bunx bun-pr 42588That installs a local version of the PR into your bun-42588 --bun |
|
Status Reproduced on main a22b2aa (debug build) and on 1.4.3-canary b993710 with: const dec = (v, c) => {};
class A { @dec f = () => {}; @dec static g = function () {}; @dec h = class {}; }
class B extends class {} { @dec m() {} }
@dec class C { static name() { return "static method" } }
console.log(new A().f.name, A.g.name, new A().h.name); // "" "" "" want f g h
console.log(Object.getPrototypeOf(B).name); // "_base$2" want ""
console.log(typeof C.name); // "string" want "function"With |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes emitted decorator-lowering output and reworks the __decoratorStart/__decorateElement runtime helper contract (diverging from esbuild's _base approach), a human look at the design would still be worthwhile.
What was reviewed:
named_by_fieldkey reuse —FieldName::Keyis a constant andFieldName::Computedis a temp ref, so emitting the key twice (as property and as index) is side-effect-free.__decoratorStart(base, target)backward compat — old one-arg output leavestargetundefined sobaseis used as before;extends nullgivesFunction.prototypewith noSymbol.metadata, covered by theNultest.- The
__getOwnPropDesc(target, "name")?.valueguard — static getters/accessors have no.value, static methods have a function.value, and static fields run after the leading block soG.name = 1still wins. base_refremoval leaves no other consumers; cache version bumped 32→33;runtime.jsadded as an explicit ninja input.
Extended reasoning...
Overview
This PR fixes three .name-related correctness bugs in the standard-decorator lowering pass: (1) decorated fields/accessors now name their anonymous initializer via a { key: value }[key] wrapper routed through a new named_by_field helper; (2) the _base = <extends> temporary is removed and __decoratorStart gains a second target argument so metadata inheritance is derived from Object.getPrototypeOf(this) in the leading static block; (3) __decorateElement skips __name(target, name) when the class already has a non-string own name descriptor. It touches src/js_parser/lower/lower_decorators.rs (~77 lines net), src/runtime.js (two helpers), bumps RuntimeTranspilerCache 32→33, adds src/runtime.js as an explicit ninja input in scripts/build/codegen.ts, and adds three test.concurrent cases plus a names fixture section in test/bundler/transpiler/es-decorators.test.ts.
Security risks
None. This is transpiler output shaping and runtime helper logic for decorator lowering; no auth, crypto, network, filesystem, or untrusted-input parsing is touched. The __getOwnPropDesc(target, "name") read avoids invoking a user getter (it inspects the descriptor, not target.name).
Level of scrutiny
Medium-high. The lowering pass is subtle — small changes here alter the code emitted for every user of standard decorators, and REVIEW.md calls out that the parser's reference is esbuild and control-flow divergence should be deliberate. Removing _base in favor of Object.getPrototypeOf(this) inside a static block is a deliberate divergence from esbuild's shape (well-justified in the PR body: (0, class {}) gets folded back by Bun's runtime minifier), and the two-arg __decoratorStart introduces a helper-contract change with a backward-compat branch. These are the kind of design choices a maintainer familiar with lower_decorators.rs should sign off on, even though I found nothing wrong with them.
Other factors
Test coverage is thorough and follows the repo's conventions: test.concurrent, stderr/stdout asserted before exitCode, and the variant matrix is wide (private/computed/symbol/numeric/__proto__ keys, static vs instance, decorated vs plain accessors, extends null, anonymous class expressions, static method/getter/accessor/field named name, and the value seen by decorator init functions). The cache-version bump satisfies the REVIEW.md rule for changed serialized output, the dead base_ref block is deleted rather than left behind, and no CODEOWNERS entry covers the changed paths. The bug hunt exited on dry_streak with no findings and no ruled-out candidates.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Problem
@dec class C { static name() {} }; C.name()throwsTypeError: C.name is not a function. A class decorator replaces a static method or getter callednamewith the class name. All releases with standard decorators do this..namevalues differ from the undecorated class.class A { @dec onClick = () => {} }gives"", so a stack frame readsat <anonymous>, notat onClick.class B extends class {} { @dec m() {} }names the base class"_base$2".src/js_parser/lower/lower_decorators.rs,src/runtime.js):__decorateElementruns__name(target, name)after the static methods exist. A decorated initializer becomes a call argument. The extends clause becomes_base = class {}.Fix
name:__decorateElementcalls__nameon a class only while its ownnameis still a string. No emitted code changes.decorated_initializerandstorage_init_effectsemit{ f: () => {} }["f"], the form thathosted_initializer(js_parser: lower standard decorators without moving class members #40833) uses._basetemporary. The leading static block calls__decoratorStart(undefined, this), and the helper usesObject.getPrototypeOf(this).test/bundler/transpiler/es-decorators.test.ts, 6 of 424 fail withsrc/from main.scripts/build/codegen.tsmakesruntime.jsan input ofruntime.out.js(a part of build: list the files the bake and runtime.out.js bundles read outside their globs #38096). Self-reviewed: 3 concerns raised, 2 addressed (Notes).Background
x = () => {}andclass { x = () => {} }name an anonymous function or class afterx. A call argument does not.namestring. A static method or accessor callednamereplaces it.__decoratorStartcreates theSymbol.metadataobject of a class. It inherits from the metadata of the extended class.Notes
Where this comes from
"","_a","string"), and tsc 5.9 gives the same empty initializer names. Babel keeps the initializer names..name(its tests pinh = (effects, { h: () => {} }.h)). This PR applies the rule to the sites where the value is a call argument.Emitted code, before and after
Which initializers are wrapped
FieldName::Keyfor a string or number key,FieldName::Computedwith the_computedKeytemporary for any other computed key (a symbol key gives[description]).#privatefield or accessor: the key is the string"#p", so the name is#pas it is natively.accessor, decorated or not. Its value goes through__privateAdd(this, _storage, value), so it lost the name without any decorator in the class. An undecoratedaccessor #xis a plain#xfield and was already right.__proto__keys are emitted as computed ({ ["__proto__"]: fn }), ashosted_initializeralready did.function keep() {},class { static name() {} }) keeps it, because this is real NamedEvaluation and not adefinePropertyonname.#private, computed, numeric or symbol-keyed field (#p = @dec class {}). It is lowered to a comma expression before the field sees it, and its name comes fromdecorator_class_name, which knows identifier keys only. It is""before and after this PR, with or without a decorator on the field. js_parser: name lowered anonymous classes after numeric, non-ASCII and private property keys #38787 is the open PR for that.__decoratorStart__decoratorStart(base)becomes__decoratorStart(base, target). Withtarget,baseisObject.getPrototypeOf(target). Forextends nullthat isFunction.prototype, which has noSymbol.metadata, so the metadata parent isnullas before. A class with no extends clause still gets__decoratorStart(undefined)._base = (0, class {}). It is correct inbun runand in a bundle that node runs. It is not correct in a bundle that Bun runs, because the syntax minifier of the runtime transpiler folds(0, class {})back toclass {}. Without a temporary there is nothing to fold, andbun buildandbun build --minifyoutput give""too.bun build --no-bundle, which imports the helpers frombun:wrap) still passes_baseas the only argument. I ran such a file from 1.4.3-canary on this build: the metadata chain is the same.__nameon a classaccessorcallednamereplaces it. A static field callednamewas already right, because static fields run after the leading static block.typeof Object.getOwnPropertyDescriptor(target, "name")?.value == "string". It does not readtarget.name, so a static getter is not called."name", an anonymous class expression (const B = @dec class { static name() {} }), and the decorator seeingtypeof value.name === "function".Related open PRs
{ f: () => {} }.fto the bare function #42585: when Bun runs output that it transpiled before (bun build --no-bundle,--target=node,Bun.Transpiler; not--target=bun), the runtime syntax minifier folds{ f: () => {} }["f"]to() => {}. In that lane the initializer names of this PR and the hosted names of js_parser: lower standard decorators without moving class members #40833 are lost again. js_parser: do not fold{ f: () => {} }.fto the bare function #42585 removes that fold. The staticnameand extends fixes do not depend on it.{ f: () => {} }.fto the bare function #42585 also setsRuntimeTranspilerCacheto 33. The PR that lands second takes 34.runtime.out.jsand bake bundles read. The one-linecodegen.tschange here is itsruntime.out.jspart. If build: list the files the bake and runtime.out.js bundles read outside their globs #38096 lands first, this hunk goes away.Self-review
{ f: () => {} }.fto the bare function #42585 bumps the same cache version and adds tests at the same place ines-decorators.test.ts, and this body did not link it. Addressed: linked above, with what depends on it and who takes version 34.TypeError, and "Where this comes from" and "Not covered" say the rest.nameguard into its own PR without a cache bump. Not done: the three fixes share tests and one rule, and the same review recommends one PR.Other
RuntimeTranspilerCacheversion 32 → 33, because the emitted code changes.es-decorators.test.ts(424),es-decorators-esbuild.test.ts(147),decorators.test.ts(24),decorator-metadata.test.ts(5),ts-use-define-for-class-fields.test.ts(11),regression/issue/27526,regression/issue/27575.git checkout origin/main -- src/, the 6 failures are the 3 new tests in "members stay where they are written" and the newnamesfixture section in.js,.tsand bundled mode. The other 418 pass.[human-review] gate passed · iteration 0 · 5 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file