Repository navigation
Conversation
With experimentalDecorators, lower_class reused a decorated member's key expression both in the class body (or the relocated field initializer) and as the __legacyDecorateClassTS argument, so a computed key with side effects ran twice and the decorator was applied to a different property than the one defined. Evaluate the key into a temporary instead: in place for methods and accessors, and in a var before the class for fields, whose initializers are moved into the constructor.
|
Updated 10:05 AM PT - Aug 13th, 2026
❌ @robobun, your commit 0fa1876 has some failures in 🧪 To try this PR locally: bunx bun-pr 38142That installs a local version of the PR into your bun-38142 --bun |
|
Warning Review limit reached
Next review available in: 11 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 |
|
Status
|
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. Because it changes lower_class codegen in the parser — including a deliberate deviation from tsc's evaluation order for decorated field keys (evaluated before the class rather than in source position) and new renamer/scope registration for the generated temporaries — a human look would still be worthwhile.
Checked: the S::Local default is var so the pre-class declaration hoists as intended; record_usage counts line up with the two emitted identifier uses (over-count by one for initializer-less fields is harmless); primitive-literal and inlined-enum keys skip the new path via unwrap_inlined().is_primitive_literal(); the field-relocation block below correctly picks up the rewritten prop.key for this[_computedKey].
Extended reasoning...
Overview
The PR fixes double-evaluation of computed keys on legacy-decorated class members. In src/js_parser/p.rs's lower_class, a decorated member with a non-literal computed key now allocates a temporary via a new declare_var_temp_ref helper: methods rewrite the key in place to [_computedKey = expr], fields hoist var _computedKey = expr; before the class and the relocated initializer becomes this[_computedKey]. One var statement holding all temporaries is emitted before the class. ~50 lines of Rust plus a 9-test describe in decorators.test.ts (methods/accessors/statics, param decorators, fields with/without init, multiple instances, multiple classes, evaluation order, transpiled snapshot) and one itBundled case exercising cross-module and sibling-block temporaries.
Security risks
None. This is transpiler codegen for TypeScript's experimentalDecorators; no auth, crypto, filesystem, or untrusted-input parsing is touched.
Level of scrutiny
Moderate-to-high. lower_class is a load-bearing codegen path and the change interacts with two subtle subsystems: (1) the renamer — declare_var_temp_ref walks up to the hoisting scope and registers the ref both in scope.generated and declared_symbols, which the PR description says is required to keep temporaries distinct across bundled modules and sibling blocks; and (2) evaluation order — decorated field keys are now evaluated before the class (all field keys first, then in-body members), which differs from tsc's source-order evaluation. The PR justifies this by noting Bun doesn't relocate static initializers the way tsc does, so a static initializer that constructs the class needs the key already evaluated. That's a reasonable tradeoff but is a design call a maintainer should confirm.
Other factors
The tests are thorough and the PR description states 8/9 of the new transpiler tests fail on the released build. I verified the mechanics: S::Local { ..Default::default() } emits var (so hoisting is correct), the field-relocation block at lines ~6693-6717 reads back the rewritten prop.key and thus emits this[_computedKey], and is_primitive_literal after unwrap_inlined correctly excludes string/number/inlined-enum keys from the new path. I didn't find a case where the generated var could shadow or collide — the hoisting-scope walk plus DeclaredSymbol registration matches how the using lowering handles its temps. The description also notes #38125 adds an equivalent helper for accessor; whichever lands second should dedupe.
|
On the evaluation-order point from the review, for whoever looks at this: decorated field keys were not evaluated in source position before this change either. The field is taken out of the class body, so its key ran after the class (in the decorate call, and again in the relocated initializer). This PR moves that single evaluation to just before the class, so an instance created by a static initializer of the same class ( |
|
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. Closing in favor of that PR. I ran the tests of this PR against a build of #40830. Seven of the nine tests in "decorated members with computed keys" pass, and so does
The inline output snapshot differs for the same reason: the keys are captured as |
Problem
experimentalDecorators, a decorated class member whose key is computed has the key expression evaluated twice: once by the class body and again as the argument of the generated__legacyDecorateClassTS(...)call. If the expression is not pure, the member is defined under one key and the decorator is applied to another (repro below: the prototype getsm1, the decorator receivesm2).this[key()] = init(orA[key()] = initfor statics), so their key is also re-evaluated on every construction.lower_classinsrc/js_parser/p.rs(theprop.ts_decoratorsblock, previouslydescriptor_key = prop.key, and the field relocation below it) reuses the key expression in every place that needs the key, so the printer emits it several times.[_a = key()]() {}and__decorate([...], A.prototype, _a, null)), and Bun's standard-decorator lowering inlower_decorators.rsalready does the same (_computedKey). Only the legacy path was missing it.Repro (tsconfig with
"experimentalDecorators": true)Before:
After (same as tsc):
bun buildoutput after the fix forclass A { @dec [k()]() {} @dec [k()] = 1 }(helper definition omitted):The runtime transpiler emits the same shape with the temporaries spelled
__bun_temp_ref_1$,__bun_temp_ref_2$.Fix
lower_classnow creates a temporary, passes it to__legacyDecorateClassTS, and emits onevarstatement declaring the temporaries in front of the class statement.[_computedKey = key()]() {}. This is tsc's output shape, and the key keeps its source-order evaluation position.var _computedKey = key();, and the relocated initializer becomesthis[_computedKey] = init. It is evaluated before the class rather than after it because the constructor can already run while the class is being defined (static instance = new A()); tsc gets the same guarantee by also relocating static initializers, which Bun does not do. The "instance created while the class is being defined" test pins this.["x"],[1], inlined enum members) take the old path unchanged: evaluating them twice is unobservable. The inline snapshot in the new tests shows no temporary is introduced for them.declare_var_temp_ref:generate_temp_ref_with_scopeon the scope thevarhoists to, plus aDeclaredSymbolfor the current part. This is the registration theusinglowering does for its temporaries, and whatdeclare_generated_symbol's comment asks for. In the runtime transpiler the temporaries print as file-unique__bun_temp_ref_N$; in the bundler the renamer produces_computedKey,_computedKey2, ... across all files of the chunk. Both parts are needed: without theDeclaredSymbol, every class in a bundle shares one_computedKey; registering in the current scope instead of the hoisting scope makes two classes in sibling blocks share one hoistedvar. The bundler test below fails in both of those variants (checked by building them). Accept and lower theaccessorkeyword in TypeScript files using experimentalDecorators #38125 adds the same helper privately foraccessor; whichever lands second can drop its copy.test/bundler/transpiler/decorators.test.ts, newdescribe("decorated members with computed keys"): methods/accessors/static methods, parameter decorators, instance fields across two instances, static fields, fields without an initializer (includingdeclare), construction during the class definition, two classes in one scope, evaluation order, and an inline snapshot of the transpiled shape. 8 of the 9 fail on the released build (the no-initializer one is a guard that the new path does not add an evaluation), all pass with the fix.test/bundler/bundler_edgecase.test.ts,edgecase/TypeScriptDecoratorComputedKeyTemporaries: two modules plus two sibling blocks in the entry, every instance constructed after all classes exist. Fails on the released build (12 evaluations instead of 6).bun bd testondecorators,decorator-metadata,bundler_decorator_metadata,ts-use-define-for-class-fields,es-decorators,es-decorators-esbuild,esbuild/ts,bundler_edgecase,integration/typegraphqland regression tests 27526 / 27575: pass.Background
experimentalDecorators, Bun keeps the class statement and appends one__legacyDecorateClassTS(decorators, target, key, descriptor)statement per decorated member (the equivalent of tsc's__decorate). Decorated fields are additionally taken out of the class body and their initializer is re-emitted as an assignment in the constructor (or after the class for statics). js_parser: keep [[Define]] semantics for decorated class fields #35537 proposes to stop relocating them; if that lands, the field branch here becomes the method branch.[expr]. Natively the expression runs once, when the class is defined; any lowering that prints it in two places changes that, which is only observable when the expression has side effects or returns a fresh value.generate_temp_ref). The bundler renames instead, taking top-level names from each part'sdeclared_symbolsand nested names from the scope tree, so a generatedvarhas to be registered in the scope it really binds in and, at module level, as a declared symbol.