Repository navigation
Conversation
… initializer `accessor #x` is a private getter and setter over separate storage. The pair is on the object before the first field initializer runs, so `#x in this` is true from the start of construction. The storage is defined where the accessor is written. The undecorated form was printed as the field `#x`, and the decorated form used one WeakMap as brand and storage. Both made the name exist only once the initializer of the accessor had run. The undecorated form is `get #x` and `set #x` over a WeakMap again. The decorated form has a WeakSet brand that the object joins with the brands of private methods, next to the WeakMap of its value.
|
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 46 seconds 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 (3)
Comment |
|
Status How I reproduced it, on a debug build of main a22b2aa: const dec = (v, c) => {};
class N { #q = (this.seen = #p in this); get #p() { return 1 } set #p(v) {} }
class U { accessor #p = (this.seen = #p in this); m() {} }
class D { accessor #p = (this.seen = #p in this); @dec m() {} }
console.log(new N().seen, new U().seen, new D().seen); // main: true false false, this branch: true true true
class D2 { a = #p in this; @dec accessor #p = 1 }
class S { static a = #s in this; @dec static accessor #s = 1 }
console.log(new D2().a, S.a); // main: false false, this branch: true true
Fix: #42643 |
![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 the private-brand semantics of the decorator lowering (splitting accessor #x into a WeakSet brand + separate value WeakMap, and rerouting the undecorated case through a native get/set pair), a human look at the runtime-helper interaction would still be worthwhile.
What was reviewed:
- The
is_brandedsplit inlower_decorators.rs: WeakSet brand pushed toinstance_brands/static_brandsup front, separate_*_storageWeakMap fed to__decorateElementasextraand tostorage_init_effects— matches the(target, extra)shape the runtime helper already accepts. - The undecorated
accessor #pbranch:host_key_effectsskipped for private keys (nolast_key_hostupdate), initializer routed throughhosted_initializerso anonymous functions keep the#pname. - Cache version bump 32→33 present with an accurate history line.
- Test matrix covers instance/static × {hand-written pair, bare, beside decorated method, decorated}, plus decorator-replaced get/set and constructor-return-override; each lowered shape is asserted equal to the hand-written ground-truth row.
Extended reasoning...
Overview
This PR fixes a regression in ES-decorator lowering for private auto-accessors (accessor #p). On main, an undecorated accessor #p was collapsed to a plain private field, and a decorated one used a single WeakMap for both brand and storage — so #p in this was false in any initializer that ran before the accessor's declared position. The fix in src/js_parser/lower/lower_decorators.rs treats a private accessor like a private method for branding: a WeakSet brand added via __privateAdd before the first field initializer, plus a separate _<name>_storage WeakMap populated at the accessor's position. The non-lowered undecorated case now emits a native get #p/set #p pair over a WeakMap (so JSC installs the brand first), skipping host_key_effects for private keys and wrapping the initializer via hosted_initializer to preserve function naming. RuntimeTranspilerCache.rs bumps EXPECTED_VERSION 32→33. es-decorators.test.ts adds a 4-shape × instance/static matrix asserting every lowered form matches a hand-written get #p/set #p pair, plus a decorator-replaced-accessor case and a constructor-return-override case.
Security risks
None. This is transpiler output correctness for class-body syntax; no untrusted-input parsing boundaries, no auth/crypto/permissions, no I/O. The generated code uses the existing __privateAdd/__privateIn/__decorateElement runtime helpers unchanged.
Level of scrutiny
Moderate-to-high. Decorator lowering is spec-compliance code where subtle ordering (brand installation vs. field initializer evaluation vs. __decorateElement's (target, extra) contract) determines observable behavior. The change is well-scoped and the test matrix is strong — each lowered shape is compared against a hand-written ground-truth column, covering #p in this before/at the accessor position, read/write TypeErrors before storage exists, Object.create inheritance, and re-branding the same object. The cache version bump satisfies the "any change to cached/serialized output bumps the format version" rule. Still, this is not a mechanical change: it restructures how private accessor brands and storage are represented, and interacts with __decorateElement's handling of the extra argument and with PrivateLoweredInfo bookkeeping. Someone familiar with the decorator lowering (and #40833, which introduced the regression) should confirm the runtime-helper contract and the private_lowered_map interaction.
Other factors
No CODEOWNERS covers these paths. No prior reviews or outstanding objections in the timeline. The bug hunt exited on dry_streak with no findings and no ruled-out candidates. The new Rust comments are concise why-comments (explaining brand-vs-storage timing and why the initializer is wrapped), not narration. Tests were added to the existing es-decorators.test.ts file per convention, use the existing extraSections/extraExpected fixture pattern, and the PR description states 9 of 12 new fixture keys fail without the fix. The PR also notes the interaction with open #42588 (function naming for accessor x / @ dec f) and #38933/#38904 (shared var temporaries in repeated class expressions), which are explicitly left unchanged.
…e field The getter and setter `#x` now read and write `#x_accessor_storage`, a new private field at the place of the accessor, not a WeakMap next to the class. The pair and the field are native. The initializer runs as the field initializer it is: `new.target` is undefined, it does not move into the constructor, and each evaluation of the class has its own storage. The new name is not one the class or a class around it declares.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Updated 2:04 PM PT - Sep 13th, 2026
✅ @robobun, your commit dcf617336e40b6e33e9ca53052f5982402a9da53 passed in 🧪 To try this PR locally: bunx bun-pr 42643That installs a local version of the PR into your bun-42643 --bun |
This branch has not been deployed
2 participants
Problem
class U { accessor #p = (this.seen = #p in this) }; new U().seenisfalseon main (a22b2aa) andtrueon 1.4.2.accessor #pis a private getter and setter, which an object has before its first field initializer runs.accessor #xas the field#x(src/js_parser/lower/lower_decorators.rs:861).@dec accessor #xuses one WeakMap as brand and storage, filled where the accessor is written (:952).class D { a = #p in this; @dec accessor #p = 1 }givesfalsetoo.Fix
get #x/set #xover a new private field,#x_accessor_storage, at the place of the accessor. All three are native: the engine installs the pair first, and the field behaves as the#xfield of main does.#xbecomes a WeakSet brand that the object joins with the brands of private methods, before its first field. A second WeakMap holds the value.__decorateElementalready takes the two as(target, extra).test/bundler/transpiler/es-decorators.test.ts: 4 new fixture keys in 3 modes (9 of 12 fail without the fix) and 1 test for the new field. Alsoes-decorators-esbuild,decorators,decorator-metadata.Background
accessor x(decorators proposal) declares a getter, a setter and hidden storage. JavaScriptCore does not parse it, so bun always lowers it.#x in objasks whetherobjhas the private members of the class: the brand check.#privatename with a decorated member into a WeakMap or WeakSet that the helper__decorateElementcan reach.#x in obecomes__privateIn(_x, o).Notes
accessor xstill has. The review showed what that gives up against main, where the accessor is a native field. An accessor that is the last instance member runs its initializer in the constructor:new.targetis the class there, and a constructor with no top-levelsuper()statement (if (c) super(); else super()) throwsReferenceError. A class expression that is evaluated twice shares the storage. With the private field (second commit) none of these apply. The test "the value of an undecorated accessor #x is a private field of the class" pins them.{ f: () => {} }.fto the bare function #42585, js_parser: keep function, base class and staticnamenames through standard decorator lowering #42588 and js_parser: rewrite every assignment target of a lowered#privatemember #42651 each setEXPECTED_VERSIONto 33 too, so git reports a conflict on that line and the second to land takes the next number. js_parser: keep function, base class and staticnamenames through standard decorator lowering #42588 adds afield_nameargument tostorage_init_effects. At the call for a decorated#privatemember the first argument has to stayvalue_storage: withstoragethe value goes to the WeakSet brand and every@dec accessor #xthrows "Cannot add the same private member more than once".Output for
class U { accessor #p = (this.seen = #p in this); m() {} }:Output for
class D2 { a = #p in this; @dec accessor #p = 1 }:#x_accessor_storage, with a number appended while the class or a class around it declares that name (a reference from inside this class to such a name would reach the new field). The symbol is registered in the scope around the class, so--minifyrenames it with the other private names of a class inside a function. A class at the top level of a module keeps the long name.#v = init; get #p() {} set #p(v) {}), instance and static: the accessor alone, next to a decorated method, and decorated. Each class reads#p in this,this.#pandthis.#p = 2from an earlier initializer and from the accessor's own. All rows must equal the hand-written row.context.access.hasis true there (privateAccessorReplaced). Before, both threwTypeError: Cannot read from private field.privateAccessorTwice(a constructor that returns an existing object, run twice) passes with and without the fix. It pins that the new WeakSet brand throws on the second add, as the native pair does.accessor #pkeeps the name#p, as on main: the field would name it#p_accessor_storage, so the initializer goes throughhosted_initializer.accessor x = () => {}and@dec f = () => {}do not name the function after the member. js_parser: keep function, base class and staticnamenames through standard decorator lowering #42588 is open for that.bun build --minifyand--target=browser, and withuseDefineForClassFields: false.RuntimeTranspilerCacheversion 32 to 33, because the printed code for the same input changes.accessor xand@dec accessor #xkeep their value in a WeakMap, so the limits in the first note still apply to them (js_parser: declare decorator lowering temporaries per iteration inside loops (stacked on #38734) #38933 and js_parser: declare decorator lowering temporaries per evaluation in parameter defaults and field initializers #38904 are open for the shared temporaries).this.#x++,this.#x += vand the other forms on a decorated#xthat js_parser: lower standard decorators without moving class members #40833 lists as open (js_parser: rewrite every assignment target of a lowered#privatemember #42651 is open for them). They work on an undecoratedaccessor #x, because that pair is native.[human-review] gate passed · iteration 0 · 3 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