Repository navigation
Conversation
A decorated public class field was lowered to a plain assignment
('this.p = init' for instance, 'C.p = init' for static), which uses
[[Set]] semantics and invokes any inherited setter instead of creating
an own data property. An undecorated field in the same class kept the
native class-field [[Define]] semantics, so adding a decorator silently
flipped the semantics and could either read back the wrong value or
throw 'Attempted to assign to readonly property' on a getter-only
inherited accessor. tsc and esbuild both keep [[Define]] for decorated
fields.
Standard (TC39) decorators: emit '__publicField(this, key, value)'
instead of 'this[key] = value' for lowered instance and static fields.
Legacy experimentalDecorators: keep decorated fields in the class body
as native class fields instead of moving them into the constructor. The
'__legacyDecorateClassTS' call is unchanged. The constructor-injection
path this made dead has been removed. Decorated 'declare'/'abstract'
fields are still dropped from the body after their decorator call is
recorded.
|
Warning Review limit reached
Next review available in: 20 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 (2)
WalkthroughChangesDecorated field lowering
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No bugs found, but this is a user-visible transpiler semantics change that warrants a human look — it flips decorated public fields from [[Set]] to [[Define]], deletes ~150 lines of legacy constructor-injection lowering in p.rs, and rewrites the decorators random test (which previously certified the old [[Set]] behavior) to assert the opposite.
What was reviewed:
__publicField/__defNormalPropinruntime.js— usesdefinePropertywhen the key is inherited, so [[Define]] is preserved.constructor_functioninp.rsis still live (used foremitDecoratorMetadataat ~L6640);member_targethas no remaining callers.- Computed-key evaluation count is unchanged vs. the old
member_targetpath (key expr embedded once either way).
Extended reasoning...
Overview
The PR changes how decorated public class fields are lowered in both decorator flavors. For standard (TC39) decorators, lower_decorators.rs now emits __publicField(this|C, key, __runInitializers(...)) instead of a plain assignment. For legacy (experimentalDecorators) decorators, p.rs::lower_class no longer moves decorated fields into a synthesized constructor / post-class assignment — they stay as native class fields, and the ~150-line constructor-splice machinery (including synthesized super(...arguments)) is deleted. The now-unused member_target helper is removed. Tests are added for inherited-accessor shadowing (instance/static/getter-only) in both flavors, and two existing tests that encoded [[Set]] semantics are rewritten.
Security risks
None identified. This is compile-time AST lowering; no untrusted input reaches new allocation or syscall paths.
Level of scrutiny
High. This is a deliberate, user-visible behavior change to the transpiler: code that previously invoked an inherited setter on a decorated field will now shadow it with an own data property. The PR's argument (matches tsc 5.9 / esbuild / spec under useDefineForClassFields: true, which Bun assumes) is sound, but a maintainer should confirm that assumption still holds for the legacy-decorator path and that the rewritten decorators random test isn't losing coverage of something else (it dropped the StringAppender accessor-via-decorator pattern entirely and replaced it with a target/key recorder).
Other factors
- There are three outstanding
comment-copinline comments asking the added multi-line comment blocks be removed; those haven't been addressed yet. - I verified
constructor_functionremains live foremitDecoratorMetadata,member_targethas no remaining references, and__publicField→__defNormalPropusesObject.definePropertyon inherited keys (matching esbuild's helper). - The old code embedded
prop.keyonce viamember_target; the new code embeds it once viacall_rt, so computed-key side-effect evaluation count is unchanged. - The removed constructor-injection path also handled the no-constructor /
extendscase by synthesizingconstructor(){super(...arguments); ...}. Leaving fields as native class fields makes that unnecessary, but it interacts with parameter properties andemitDecoratorMetadata— worth a maintainer glance.
There was a problem hiding this comment.
I didn't find any bugs, but this is a user-visible semantic change to the transpiler (decorated fields switch from [[Set]] to [[Define]]) that removes ~150 lines of legacy-decorator lowering and rewrites existing tests to encode the new behavior — worth a human confirming the useDefineForClassFields: true assumption and the test rewrites.
What was reviewed:
__publicFieldinsrc/runtime.js— confirmed it usesdefinePropertywhenkey in obj, so the standard-decorator change gives [[Define]] semantics.constructor_functioninp.rs::lower_classis still live (used fordesign:paramtypesmetadata below the removed block).PropertyKind::Declare/Abstractonly reachlower_classwhen decorated (parse_property.rs:432,458), so the newcontinuedoesn't drop undecorated fields.- The removed
member_targethelper has no remaining callers.
Extended reasoning...
Overview
This PR changes how decorated public class fields are lowered so they preserve [[Define]] semantics instead of silently falling back to [[Set]]. It touches two lowering paths: standard TC39 decorators in src/js_parser/lower/lower_decorators.rs (swap this.k = ... / C.k = ... for __publicField(this, k, ...)), and legacy TS decorators in src/js_parser/p.rs::lower_class (stop moving decorated fields into a synthesized constructor / post-class assignments; leave them as native class fields and only drop declare/abstract bodies). The now-dead member_target helper and ~120 lines of constructor-injection machinery are removed. Tests in decorators.test.ts and es-decorators.test.ts add coverage for inherited accessors (instance, static, getter-only) and rewrite two existing tests that encoded the old [[Set]] behavior.
Security risks
None. This is a transpiler output-shape change; no untrusted input parsing, auth, crypto, or filesystem paths are affected.
Level of scrutiny
High. The change is in js_parser, which runs on every transpiled file, and it is a deliberate user-observable behavioral change (not a pure bugfix): any code that relied on a decorated field triggering an inherited setter will now behave differently. The decorators random test was substantially rewritten — the StringAppender prototype-accessor pattern it exercised no longer works under [[Define]] semantics, and the PR description argues (correctly, per my read) that tsc/esbuild with useDefineForClassFields: true behave the same way. A maintainer should sign off on that reasoning and on removing the constructor-splice machinery wholesale rather than gating it.
Other factors
- I confirmed
__publicField→__defNormalPropinsrc/runtime.js:209-215usesObject.definePropertywhen the key is on the prototype chain, so the standard-decorator fix does what it claims. constructor_functionis still read atp.rs:6634foremitDecoratorMetadatadesign:paramtypes, so removing the instance-member injection didn't leave it dead.- The new
Declare | Abstractdrop is scoped correctly:parse_property.rsonly returns those kinds whents_decorators.len() > 0, so undecorated fields never hit that branch. - One comment-cop thread on
p.rsis still open; the 3-line explanatory comment at lines 6606-6608 may be what it's flagging, though it reads as a legitimate why-comment to me. - No prior review from me on this PR.
There was a problem hiding this comment.
I didn't find any bugs. The fix looks correct — __publicField matches esbuild's helper, the declare/abstract drop is verified against parse_property, and the removed constructor-injection machinery is genuinely dead. But this is a user-visible semantics change to decorator lowering (some legacy prototype-accessor patterns will stop working, as the rewritten decorators random test shows), so a human should sign off on the behavior change and the two rewritten tests.
What was reviewed:
__publicField→__defNormalPropinruntime.jsusesdefinePropertywhenkey in obj, giving [[Define]] semantics as claimed.parse_property.rsconfirms undecorateddeclare/abstractreturnNone, so the newPropertyKind::Declare | Abstractskip only handles the decorated case.constructor_functionandarguments_strare still live foremit_decorator_metadata/ elsewhere;member_targethad no other callers.
Extended reasoning...
Overview
The PR fixes #20664 by switching decorated public class fields from [[Set]] to [[Define]] semantics in both lowering paths: standard (TC39) decorators in lower_decorators.rs now emit __publicField(this|C, key, __runInitializers(...)) instead of a plain assignment, and legacy (experimentalDecorators) in p.rs::lower_class now leaves decorated fields in the class body as native class fields instead of moving them into a synthesized constructor as this.x = init. About 170 lines of constructor-injection machinery (the instance_members/static_members collection, constructor synthesis, and the member_target helper) are removed. A new explicit PropertyKind::Declare | Abstract check drops those from the class body after their decorator call is recorded — verified against parse_property.rs, which only preserves declare/abstract properties when they carry decorators.
Security risks
None. This is transpiler output-shape work with no I/O, auth, or untrusted-input parsing changes.
Level of scrutiny
High. The transpiler is a critical hot path, and this is a user-visible semantics change: decorated fields that previously triggered inherited setters (the Emoji/StringAppender pattern in the old decorators random test) now shadow them with own data properties. The PR argues — correctly, and I verified __publicField's definition in runtime.js — that this matches tsc/esbuild under useDefineForClassFields: true and the spec. But it will break real code that relied on the old behavior, and two existing tests were rewritten to encode the new semantics rather than the old. Per the repo's review guidance ("never silently weaken, skip, or delete an existing test"), the rewrites are justified in the PR description, but a maintainer should confirm they agree the old test was certifying a bug rather than an intentional compat choice.
Other factors
- I confirmed
constructor_functionis still used (line ~6632,emit_decorator_metadata) andarguments_stris still used elsewhere (line 4532), so no dead code was left behind. - The instance-field
__publicFieldchange mirrors the existing__privateAddbranch immediately above it in both the static and instance paths — consistent with local convention. - Test coverage is good: instance/static/getter-only × both decorator flavors, plus the esbuild decorator suite (154 tests) still passes per the PR body.
- The perf test rewrite is reasonable since the O(M×N) splice path it guarded no longer exists; the replacement asserts the new output shape instead.
- All comment-cop threads are marked resolved.
There was a problem hiding this comment.
I didn't find bugs, but this changes user-observable transpilation semantics (decorated fields flip from [[Set]] to [[Define]]) and removes the legacy constructor-injection path, so it's worth a human confirming the direction.
Reviewed: the __publicField swap in lower_decorators.rs (matches the existing __privateAdd sibling arm and the runtime helper); the removed p.rs splice machinery — constructor_function is still consumed by emit_decorator_metadata, and member_target has no remaining callers; the decorators random rewrite encodes what tsc/esbuild produce under useDefineForClassFields: true; new tests cover instance/static/no-init/getter-only for both decorator flavors.
Extended reasoning...
Overview
The PR touches four files: two parser lowering paths (src/js_parser/lower/lower_decorators.rs for TC39 standard decorators, src/js_parser/p.rs::lower_class for TS legacy decorators) and their two test files. For standard decorators, this.p = __runInitializers(...) becomes __publicField(this, key, __runInitializers(...)), giving [[Define]] semantics via the existing runtime helper. For legacy decorators, ~170 lines that moved decorated fields into a synthesized constructor as this.p = init are deleted; decorated fields now stay in the class body as native class fields, with only declare/abstract kinds still stripped after their decorator call is recorded. The now-dead member_target helper is removed.
Security risks
None. This is pure transpiler output-shape work with no I/O, network, auth, or untrusted-input parsing changes.
Level of scrutiny
High — this is a deliberate semantics change to how every decorated public class field is emitted, in both decorator modes. The direction is well-argued (matches spec, tsc 5.9, esbuild 0.28, and Bun's own assumption per the // our version assumes useDefineForClassFields is true comment in visit/mod.rs), and the fix is at the right layer. But it will change behavior for user code that relied on the old [[Set]] lowering — the Emoji/StringAppender pattern in the rewritten test is exactly that pattern (property decorator installs a prototype accessor, initializer flows through it). A maintainer should sign off that the ecosystem-compat trade-off is intended, and that Bun does not want to gate this on the use_define_for_class_fields parser option that exists in parse_entry.rs.
Other factors
- Two existing tests were rewritten to encode the new behavior rather than the old. The PR description justifies each, and the rewrites still assert the strongest available invariants (own-property checks, exact decorator-call ordering across instance/static/symbol/computed keys), so coverage is not weakened — but per REVIEW.md, changing what a test asserts warrants a stated reason and human confirmation.
- The removed constructor-splice perf test guarded a code path that no longer exists; its replacement asserts the new output shape instead of a timing ratio, which is reasonable.
- I confirmed
constructor_functionis still read (fordesign:paramtypesmetadata emission),member_targethas no remaining references, and__publicFieldis a pre-existing runtime helper (src/runtime.js,src/ast/runtime.rs). - All comment-cop threads on the PR are marked resolved; CI build #80150 was still in progress at the time of the last timeline update.
|
The diff is green on every lane that built. Remaining CI failures in build #80227 are unrelated to this change:
The two decorator test files pass on every lane that ran them. Ready for review. |
|
A maintainer asked for one change that covers every experimental decorator lowering bug in this area. #40830 does that, and it includes the fix this PR makes (decorated fields stay in the class body, computed keys are captured once, parameter decorators use the enclosing scope, decorators that read a private name run in a static block, export default @dec class, accessor lowering). If #40830 lands, this PR can be closed. |
|
Superseded by #40830 (experimental decorators) and #40833 (standard decorators). Closing in favor of those two PRs. I ran the tests of this PR against a build of each:
|
2 participants
Fixes #20664
Reproduction
Before:
tsc 5.9 / esbuild 0.28 / Node (spec):
With a getter-only inherited accessor,
new C()threwTypeError: Attempted to assign to readonly property.. The same divergence reproduced underexperimentalDecorators: true.Cause
A decorated public field was lowered to
this.p = init(instance) orC.p = init(static), i.e. [[Set]] semantics, while the undecorated field in the same class stayed as a native class field ([[Define]]). Adding a decorator silently flipped the field from [[Define]] to [[Set]].lower_decorators.rsemittedStmt::assign(this.p, __runInitializers(...))into the synthesized constructor.p.rs::lower_classmoved each decorated field into the constructor asthis.p = init(or after the class asC.p = initfor static) and dropped it from the class body.Fix
__publicField(this, key, __runInitializers(...))/__publicField(C, key, ...)instead of plain assignment.__publicFieldusesObject.definePropertywhen the key is inherited, matching esbuild's lowering.useDefineForClassFields: true, which Bun assumes). The__legacyDecorateClassTScall is unchanged. The now-unreachable constructor-injection machinery is removed. Decorateddeclare/abstractfields are still dropped from the body after their decorator call is recorded.The
member_targethelper became dead and is removed.Tests
New tests cover instance, static, and getter-only inherited accessors for both decorator flavors. Two existing tests encoded the old [[Set]] behaviour and have been updated:
decorators random: theEmoji/StringAppenderpattern (property decorator installs a prototype accessor and expects the initializer to call it) does not work under tsc or esbuild withuseDefineForClassFields: trueeither; the test now asserts [[Define]] semantics and that the decorator calls still happen with the right targets/keys.[review] gate passed · iteration 1 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file