Repository navigation
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughPrivate decorator lowering now uses synthetic private brand fields for affected instance members. Undecorated auto-accessors now use native private storage with generated accessors. Private-name tracking and access rewriting cover additional initializer contexts, with expanded transpiler tests. ChangesDecorator lowering
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Updated 7:05 AM PT - Aug 14th, 2026
❌ @robobun, your commit 7db08ef has some failures in 🧪 To try this PR locally: bunx bun-pr 35708That installs a local version of the PR into your bun-35708 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js_parser/lower/lower_decorators.rs`:
- Around line 1302-1309: Update the will_inject_undecorated condition in the
decorator lowering logic to include PropertyKind::AutoAccessor for undecorated
properties. Ensure computed auto-accessor keys enter the existing hoisting path
so their expressions are evaluated once and reused by the synthesized get and
set members.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7f1f67e7-19df-45b4-81bd-c698144c56bf
📒 Files selected for processing (2)
src/js_parser/lower/lower_decorators.rstest/bundler/transpiler/es-decorators.test.ts
6cda80f to
b140b13
Compare
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:1308-1311— Theundecorated_accessordisjunct added in 6cda80f is not gated onlower_all_private, so a bareclass C { [(order.push('a'),'m')](){}; accessor [(order.push('b'),'k')] = 1 }— no decorators, no#-privates — now hoists only the accessor's key ahead of the class and printsb,ainstead ofa,b(main printeda,b,b: doubled but in-order). This is the same partial-hoist reordering as the open comment onwill_inject_undecoratedabove, but with a strictly wider trigger surface — the 'hoist every computed key in alower_all_privateclass' fix proposed there would not cover this case, since herelower_all_private = false. Either hoist every computed key in any class that enters this lowering, or capture the accessor key inline at its source position ([_computedKey = expr]on the storage field followed byget [_computedKey]/set [_computedKey]) so it stays on the ClassDefinitionEvaluation timeline.Extended reasoning...
What breaks
Commit 6cda80f added a third disjunct to the Phase-2 computed-key hoist at lower_decorators.rs:1308-1311:
let undecorated_accessor = undecorated && prop.kind == PropertyKind::AutoAccessor; if prop.flags.contains(Flags::Property::IsComputed) && prop.key.is_some() && (!undecorated || will_inject_undecorated || undecorated_accessor)
Unlike
will_inject_undecorated(line 1303),undecorated_accessorhas nolower_all_privateguard. Any class containing anaccessorkeyword enters this lowering (should_lower_standard_decorators = features.standard_decorators && (has_any_decorators || has_auto_accessor)), so an undecoratedaccessor [expr]in a class with no decorators and no#-privates has its key hoisted intopre_eval_stmts— emitted at line 2615, before the class statement at line 2618. Sibling computed keys on undecorated methods, getters/setters, and static fields match none of the three disjuncts and stay in the class body, evaluating during ClassDefinitionEvaluation.Step-by-step repro
const order = []; class C { [(order.push('a'), 'm')]() {} accessor [(order.push('b'), 'k')] = 1; } console.log(order.join(','));
- The class has no decorator and no
#-private →lower_all_private = false. It has anaccessor, so it enterslower_standard_decorators_stmt. - Phase-2, method
[(order.push('a'),'m')](){}:undecorated = true,will_inject_undecorated = false(lower_all_privateis false),undecorated_accessor = false(kindNormal) → condition(!true || false || false) = false→ key not hoisted, stays in the class body. - Phase-2,
accessor [(order.push('b'),'k')]:undecorated_accessor = true→ key hoisted topre_eval_stmts(line 1329). - Emit:
var _computedKey = (order.push('b'),'k'); class C { [(order.push('a'),'m')](){}; #_accessor_storage0 = 1; get [_computedKey](){…}; set [_computedKey](v){…} }. - Runtime:
_computedKeyinitializer runs →b. Then ClassDefinitionEvaluation evaluates the method key →a. Output:b,a.
Per spec (ClassDefinitionEvaluation step 27), all class-element computed keys evaluate in source order at definition time — expected
a,b.Why this is a regression
On
mainthe Phase-2 gate wasprop.ts_decorators.len_u32() > 0only, so neither key was hoisted. The accessor lowered toget [(order.push('b'),'k')]() {…}; set [(order.push('b'),'k')](v) {…}at its source position (both getter and setter pushkey: prop.keyverbatim). ClassDefinitionEvaluation ranaat n=0 thenbtwice →a,b,b. Wrong count, butabeforebper spec. 6cda80f trades main's double-evaluation for out-of-order evaluation — a different spec violation, newly introduced by this PR.Why this is distinct from the open comment on
will_inject_undecoratedThe existing comment at line 1309 targets
will_inject_undecorated(32ebdf0), which requireslower_all_private = true(a decorated member AND a#-private in the class). Its proposed fix is 'hoist every computed key in alower_all_privateclass'. That fix would not cover this repro: herelower_all_private = false, no decorator or#-private is present, and the class enters the lowering solely because it contains anaccessor. That comment also predates 6cda80f and lists 'accessors' among elements whose keys 'stay in the class body' — stale relative to the final commit. The author needs to know the fix scope is wider thanlower_all_privateclasses.Why nothing catches it
The new test at es-decorators.test.ts:1329 (
accessor [(n++, 'k')]) has the accessor as the only class element, so there is no sibling computed key to reorder against.Fix direction
Two shapes work:
- Hoist everything. If any computed key in the class is going to be hoisted (decorated,
will_inject_undecorated, orundecorated_accessor), hoist all computed keys in the class intopre_eval_stmtsin source order — regardless oflower_all_private. This subsumes the fix for the earlier comment. - Capture inline. The single-evaluation goal of 6cda80f is achievable without
pre_eval_stmts: emit the storage field with a computed key that binds the temp at the accessor's source position —[_computedKey = expr]; get [_computedKey](){…}; set [_computedKey](v){…}— so the key evaluates on the ClassDefinitionEvaluation timeline alongside its siblings. (This would also let thewill_inject_undecorateddisjunct go away if the same shape is used for constructor-routed fields, e.g. a class-bodystatic { _computedKey = expr }placeholder or the synthetic-#__Napproach from the comment at line 1723.)
- The class has no decorator and no
|
Re the "Additional findings" review about |
There was a problem hiding this comment.
b140b13's rework does resolve the three concerns from the last round — public fields stay native (NamedEvaluation/new.target/computed-key order intact), and class_private_names is now seeded from every parsed private-kind symbol so the enclosing-class shadowing case is covered. The remaining inline finding is the one parallel brand-add site that wasn't moved with the others.
Extended reasoning...
The b140b13 restructure took the direction suggested in the previous three reviews: keep every native class-body field in place and move the lowered __privateAdd calls onto the class-body timeline via synthetic #__lowered_N fields. I re-checked each of the three prior concerns against the current diff — the __publicField block, init_in_ctor, and the will_inject_undecorated computed-key hoist are all gone; class_private_names now iterates p.symbols for every private-kind symbol, which covers privates declared in enclosing classes; and the new tests exercise .name/new.target and the Outer/Inner shadowing case. Those are resolved. The inline finding covers the one sibling site (emit_private_add at the decorated-private-method arm, line 2018) that still targets constructor_inject_stmts, which reintroduces the two-timeline split for that specific combination.
There was a problem hiding this comment.
No new issues found on 96a59e4; the four prior rounds of findings (enclosing-class private shadowing, computed-key hoist ordering, NamedEvaluation/new.target/super()-scan loss from __publicField, and the decorated-private-method brand-add timeline split) are all addressed and each has a covering test. Deferring to a human because this is a structural rework of class-element evaluation order in the standard-decorator lowering.
What was checked this round:
- The remaining
constructor_inject_stmtspopulators (Phase 7:__runInitializers(_init, 5, this)and decorated instance field/accessor inits) already ran after Phase-4-pushed items onmain, so moving the Phase-4 items to the class-body timeline preserves relative order — the@dec #a; #b = this.readA()and@dec #a; accessor b = this.readA()cases throw identically onmainand here. class_private_namesis seeded fromp.symbolsafter the full parse, so it covers privates declared in enclosing and following-sibling classes; over-conservative but sound.emit_private_addis fully removed; all three instance brand-add sites (undecorated private method/getter/setter, undecorated private field, decorated private method/getter/setter) go throughinline_brand_field.
Extended reasoning...
Overview
The PR changes how lower_standard_decorators handles undecorated auto-accessors and, when lower_all_private is set, undecorated instance #-privates. Instead of a module-scope WeakMap plus a constructor-body __privateAdd, undecorated accessors now emit a native #<key>_accessor_storage class-body field, and undecorated instance-private brand-adds become synthetic #__lowered_N = void __privateAdd(...) class-body fields at the member's source position. emit_private_add is deleted; fresh_private_name and inline_brand_field replace it. ~140 net lines in lower_decorators.rs plus 11 new tests.
Security risks
None. This is transpiler output-shape logic; no untrusted input reaches an allocation size, syscall, or privilege boundary. The generated private names are derived from parsed identifiers and collision-checked against the parser's own symbol table.
Level of scrutiny
High. This is spec-correctness code for class-element evaluation order — bugs here silently change runtime behaviour of user code, and the PR has already had four rounds of real regressions found and fixed during review (each requiring a follow-up commit). The final approach in b140b13 + 96a59e4 is structurally the right one — keep every undecorated instance initializer on the class-body timeline so the engine handles NamedEvaluation, new.target, computed-key order, and super() — but the churn history warrants a human confirming the final diff matches the described design.
Other factors
I re-traced the remaining constructor_inject_stmts writers at lines 2270/2312/2322/2330 (all Phase-7, decorated instance fields/accessors and their extra-initializers) against main's ordering: on main these were already appended after the Phase-4 undecorated-private pushes, so moving Phase-4 items earlier onto the class body cannot introduce a new before/after inversion relative to main. The decorated-instance-private-field arm (line 2312) still lands in the constructor, but every ordering I could construct against a class-body-timeline sibling matches main's behaviour (both throw, or both succeed). Test coverage is good — each prior finding has a dedicated regression test, plus the emitted-shape assertion.
There was a problem hiding this comment.
The 96a59e4 fix looks correct — the decorated instance private method/getter/setter arm now emits its __privateAdd via inline_brand_field, so all three instance brand-add sites live on the class-body timeline in source order, and the added test covers it. Given how many subtle ordering interactions this PR touched across five revisions, it's worth a human pass before merging.
What was reviewed
- Verified 96a59e4 pushes the brand-field into
new_propertiesat the method's source position andemit_private_addis fully removed. - Checked the reversed case (
#a = this.readM()before@dec #m()) — still fails, but that matchesmain(pre-existing spec gap: private methods should install before all fields regardless of position), so no regression. - Confirmed Phase 5 does not rewrite
nprop.initializer, so a lowered-private access inside a field/accessor initializer (e.g.accessor b = this.#aunderlower_all_private) is unrewritten — same asmain'sconstructor_inject_stmts, not a regression. constructor_inject_stmtsis still populated for decorated-field__runInitializers; only the three brand-add sites moved off it.
Extended reasoning...
Overview
The PR reworks how the standard-decorator lowering handles undecorated accessor members and undecorated instance #-privates in a lower_all_private class. Storage for undecorated accessors becomes a native class-body #<key>_accessor_storage field (matching tsc) instead of a module-scope WeakMap, and each lowered instance __privateAdd is emitted as the initializer of a synthetic #__lowered_N class-body field at the member's source position instead of being spliced into the constructor. This fixes two shipped bugs (initializer ordering vs. sibling fields, and the same-name-in-subclass WeakMap collision) and a pre-existing throw when a public field reads a preceding lowered private.
Two files touched: src/js_parser/lower/lower_decorators.rs (~150 net lines, one helper removed, two added) and test/bundler/transpiler/es-decorators.test.ts (+11 tests).
Security risks
None — pure transpiler output-shape change; no I/O, no untrusted input parsing beyond what the JS parser already does.
Level of scrutiny
High. Decorator/class-field lowering is spec-dense: initialization order, private-name lexical scoping, brand-check timing, NamedEvaluation, and new.target all interact. The PR went through five substantive revisions, each fixing a real regression a prior revision introduced (computed-key evaluation order, __publicField stripping NamedEvaluation, enclosing-class private shadowing, decorated-private-method brand timing). The final approach — keep every undecorated instance initializer on the class-body timeline and leave public fields native — is structurally the right one and each earlier finding is now covered by a test, but the churn history alone warrants a maintainer's read of the final diff.
Other factors
The last commit (96a59e4) addressed my previous finding correctly: the decorated instance private method/getter/setter arm at ~line 1994 now routes through inline_brand_field and emit_private_add is deleted, so a following #a = this.readM() sees the method's brand. I also checked the mirror ordering (field before method) and the un-rewritten-initializer path — both behave exactly as on main, so no net regression. The private-name collision seed iterates every private-kind symbol in p.symbols, which after the parse pass covers enclosing classes; the new nested-class test exercises it. constructor_inject_stmts remains for decorated-field __runInitializers only, so the super()-scan is no longer newly reachable from this change.
|
CI status: the last two runs (builds 81462 and 81598) had every |
Same fix as the undecorated arm: a string key like "a-b" otherwise becomes an invalid `var _a-b` binding and the output fails to parse.
InitializeInstanceElements adds every private method and accessor brand before running field initializers, so a field that textually precedes the #-method it calls must still work. Route all three brand-add sites back through one emit_private_add helper: static brands go to a static block as before, instance method/getter/setter brands are collected and placed ahead of the class body's fields, and instance field brands keep their source position because they carry the initializer.
An undecorated accessor is now rewritten up front into the three class elements it denotes (a #<key>_accessor_storage field and a getter/setter pair over it) and those flow through the ordinary field and method arms. Outside lower_all_private this emits exactly what the dedicated arm did; inside it, a private-keyed accessor is now WeakMap/WeakSet-lowered and registered like every other private, so a sibling #-method body hoisted out of the class can still reference it. The expansion runs before Phase 2 because later phases index class.properties by position, and after the lower_all_private scan so synthesized storage fields do not put a class into lowering mode by themselves.
|
CI status for 240a8da (build 95845): the only lane that stayed red is |
Static field brands (accessor storage and #-fields) were collected and prepended to the class body, so under lowering a static accessor or #-field initializer ran before the static fields declared above it. Route them like instance field brands: a static block at the member's source position. Only method brands, static or instance, are hoisted, so the two brand lists collapse into one.
There was a problem hiding this comment.
I re-reviewed the current head and found no bugs — the seven earlier findings from my July 25 passes (two-timeline ordering under lower_all_private, __publicField/computed-key regressions, enclosing-class #-name shadowing, decorated-private-method brand ordering) are all resolved by the reworked expand_auto_accessor + inline_brand_field/hoisted_brands shape, and each now has a covering test. Given ~500 lines of semantics-heavy transpiler lowering plus the stated overlap with #31926/#31930, a maintainer sign-off is still warranted.
What was reviewed:
- Verified every prior finding has a corresponding fix and test (source-order, subclass same-name, nested-class shadowing, NamedEvaluation/
new.target, decorated-#mbrand-before-field). - Checked
hoisted_brandsplaces method/getter/setter brands ahead of every field while#-field brands (with initializers) stay at source position, matching InitializeInstanceElements; static path usesmake_static_blockinnew_propertiesso static order is preserved. - Checked the collision scan seeds from this class's keys plus every enclosing
ClassBodyscope, and that generated names feed back into the set so two accessors in one class can't collide. - Checked Phase 5 now rewrites
nprop.initializer(covers directthis.#areads from field/accessor/static-block initializers) and that the computed-accessor key is captured once viaget [_k = expr].
Extended reasoning...
Overview
This PR reworks how the standard-decorator lowering in src/js_parser/lower/lower_decorators.rs handles undecorated accessor members and lowered #-privates inside a class that enters the decorator machinery. Undecorated accessors are now expanded up front into a native #<key>_accessor_storage field plus a get/set pair (expand_auto_accessor), and every lowered instance __privateAdd is emitted as a synthetic #__lowered_N = void __privateAdd(...) class-body field at its source position (inline_brand_field) instead of being spliced into the constructor. Method/getter/setter brand-adds go to a hoisted_brands list that is prepended to new_properties; #-field brand-adds (which carry an initializer) stay inline. The pre-scan for lower_all_private and a new class_private_names collision set are hoisted ahead of Phase 2, and Phase 5's private-access rewrite now visits initializer in addition to value and static-block bodies. The old inline WeakMap-based undecorated-accessor arm and the static_private_add_blocks merge block are deleted. ~500 net lines in the lowering plus 20 new tests in es-decorators.test.ts.
Security risks
None. This is a pure AST-to-AST transform inside the transpiler; no I/O, no untrusted-length arithmetic, no FFI. Generated names are derived from parser-owned symbol slices and only ever land in the emitted class body. is_identifier gates the string-key path in both the undecorated and decorated accessor arms so non-identifier keys fall back to a counter rather than producing an invalid binding.
Level of scrutiny
High. The decorator lowering determines the runtime semantics of every decorated class Bun transpiles, and the failure modes here are subtle wrong-order evaluation and #-name shadowing rather than crashes. This PR went through five revisions in response to seven concrete bug reports on earlier heads; the current shape is the structural fix those reviews converged on (keep everything on the class-body timeline, hoist method brands only). It also intersects two open PRs (#31926 handles the no-decorator-at-all path; #31930 uniquifies module-scope temporaries), and the description explicitly flags that some new tests will need rewriting once #31926 lands. That coordination and the sheer surface area put this outside what I'd auto-approve.
Other factors
Test coverage is thorough: instance/static ordering, subclass and sibling-class same-name accessors, same-class and enclosing-class private-name collision, #-keyed and non-identifier/computed-key accessors, direct and indirect reads of a lowered private from field/accessor/static initializers, method-brand-before-field, NamedEvaluation/new.target preservation, and an emitted-shape assertion. The mechgate evidence shows 12 of these fail on ASAN main and all pass on the branch. All 257 decorator-suite tests pass on every CI lane per build 95845; the one red lane is an unrelated Windows dev-server fixture. I traced each of my July-25 findings to a corresponding change in the current diff and a covering test, and looked for new two-timeline hazards introduced by hoisted_brands (static method brands correctly go through make_static_block into new_properties at source position, not the hoisted list — only instance brands are hoisted, which matches spec). Nothing further to flag; deferring for maintainer review given size and cross-PR coordination.
|
Cross-checked this branch against the field-initializer half of #28118 while working on the same defect from the other direction (#38742, now closed in favor of this PR). Applied this PR's diff onto current main (applies cleanly, 43afad2) and ran the following against that build; all produce the tsc / esbuild result:
The only case of mine this does not change is the pre-existing order of If useful, the extra cases above are written up as runtime tests in the |
…ng matrix Two new sections in the matrix fixture, run in all eleven modes: - issue31921: a class with accessors and no decorators whose static accessor initializer and static block reference a native private name, and the a/block/b order of static accessor initializers against a static block. - accessorStorage: an instance and a static accessor of one name, a user #a next to accessor a, an enclosing class's private name read from an inner class, and a computed accessor key that evaluates once and is shared by the getter and the setter, with and without a decorator in the class. These are the cases #31926 and #35708 added for the same bugs.
|
Superseded by #40833, which lowers every field of a decorated class in source order and lowers every private member with it, so an undecorated |
Refs #29837
What
When the standard-decorator lowering handles an undecorated
accessor x = v, emit a class-body private field for the backing storage instead of a module-scopeWeakMap:When a class has a decorated member and a
#-private so that the existing lowering turns every#-private intoWeakMap/WeakSetstorage, each undecorated instance#-private's__privateAddcall is emitted as the initializer of a synthetic class-body private field at that member's source position instead of being spliced into the constructor body:Why
The previous lowering injected
__privateAdd(this, _<key>, init)into the constructor body aftersuper(), while plain sibling fields stayed as class-body fields. Class-body fields evaluate before any constructor-body statement, so the accessor's initializer ran after every plain field regardless of source order; in the first examplec = this.bran before the storage was populated and threwCannot read from private field. Because the storage binding name was derived from the key alone and declared at module scope, a subclass with the same accessor name as its base reused the sameWeakMapand constructing it threw:Mechanically, an undecorated
accessoris expanded up front into the field and getter/setter pair it denotes, and those flow through the same arms as user-written members. That is what produces the native shape above, and it also means a#-keyed accessor in a class that lowers its privates is lowered and registered like any other private, so a sibling#-method body hoisted out of the class can still reference it (previously aSyntaxError).Lowered instance
#-method, getter and setter brands are placed ahead of the class body's fields (InitializeInstanceElements installs every method brand before any field initializer runs), so a field may call a#-method declared below it; field brands (lowered#-fields and accessor storage) carry their initializer and stay at their source position. This applies to static members the same way: static method brands are hoisted, static field brands become a static block at the member's position, whereas previously every static brand was prepended to the class body and a lowered static field could run before the static fields above it.Keeping every undecorated instance initializer on the class-body timeline fixes both without touching untouched fields: the engine still applies NamedEvaluation,
new.target-in-field-initializer and computed-key semantics to public fields, and the super()-scan never runs for classes that have only undecorated instance members. This also fixes the pre-existing failures for undecorated initializers that read a preceding lowered#-private: indirectly through a method (second example, previously a runtimeCannot read from private field), and directly asb = this.#a/accessor c = this.#a/#d = this.#a/static #b = C.#a(previously aSyntaxError: Cannot reference undeclared private names, because the private-access rewrite pass skipped field initializers and the static brand blocks; it now visits both).Generated
#-privates (accessor storage and the synthetic#__lowered_Nfields) are named against the class's own private keys plus the members of every enclosing class-body scope, so they cannot textually shadow a private declared in the same class or any enclosing class. Storage is named#<key>_accessor_storage, matching tsc; non-identifier and computed keys fall back to a counter (the old lowering emitted an invalidvar _a-bbinding foraccessor "a-b"; the decorated-accessor arm had the same hole and gets the same guard). A computed accessor key is captured once at the getter's key position (get [_k = expr]()/set [_k](v)), so it is evaluated a single time, in class-body order, and shared by both members; previously it was evaluated once per synthesized member.Decorated accessors and decorated fields are untouched; they still route through the runtime helpers because the decorator protocol requires out-of-class access to the storage.
Verification
bun runon a.jsfile before this change:After:
New tests in
test/bundler/transpiler/es-decorators.test.tscover instance and static initializer order, the subclass and sibling-class same-name cases, storage-name collisions with same-class and enclosing-class privates, the#a; accessor b; csource-order case underlower_all_private, method brands installed before a preceding field that calls them, a#-keyed accessor referenced from a hoisted#-method body, direct reads of a lowered private from field/accessor/static initializers, the.name/new.targetguard, private-name (instance and static), non-identifier string and computed accessor keys, and the emitted shape. The existinges-decoratorstests, 147 esbuild-suite decorator tests and 24 legacy decorator tests all pass.Related: #31930 gives the remaining module-scope lowering temporaries (
_init,_base,_dec, and the storage bindings still used for decorated members) file-unique names; #35537 switches decorated plain fields from[[Set]]to[[Define]]. Neither overlaps with this change.[review] gate passed · iteration 6 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 10 passed · 0 rejected · iteration 6
evidence per changed file