From 0fa187695c509bc5548b8a6727d22de5d4ee18f2 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 09:35:12 +0000 Subject: [PATCH] js_parser: evaluate the computed key of a legacy-decorated member once 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. --- src/js_parser/p.rs | 88 +++++++- test/bundler/bundler_edgecase.test.ts | 63 ++++++ test/bundler/transpiler/decorators.test.ts | 236 +++++++++++++++++++++ 3 files changed, 378 insertions(+), 9 deletions(-) diff --git a/src/js_parser/p.rs b/src/js_parser/p.rs index e14684e6a2f2..c8d5671da4ea 100644 --- a/src/js_parser/p.rs +++ b/src/js_parser/p.rs @@ -6501,6 +6501,8 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O let mut instance_members = BumpVec::::new_in(self.arena); let mut static_members = BumpVec::::new_in(self.arena); let mut class_properties = BumpVec::::new_in(self.arena); + // Emitted as one `var` statement before the class. + let mut computed_key_decls = BumpVec::::new_in(self.arena); for prop in s_class.class.properties.slice_mut().iter_mut() { // merge parameter decorators with method decorators @@ -6550,8 +6552,45 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O // TODO: prop.kind == .declare and prop.value == null if prop.ts_decorators.len_u32() > 0 { - let descriptor_key = prop.key.expect("infallible: prop has key"); - let loc = descriptor_key.loc; + let key = prop.key.expect("infallible: prop has key"); + let loc = key.loc; + + // The key is used both to define the member and as the + // `__legacyDecorateClassTS` argument. Printing a computed key in both + // places evaluates it twice, so an impure key would decorate a different + // property than the one defined. Evaluate it once into a temporary + // instead. Methods do that in place, like tsc: `[_computedKey = expr]() {}`. + // Fields are moved out of the body below, and the constructor that + // receives them can already run while the class is being defined (a + // static initializer constructing the class), so their key is evaluated + // before the class: `var _computedKey = expr;`. + let key_is_evaluated = prop.flags.contains(Flags::Property::IsComputed) + && !key.unwrap_inlined().is_primitive_literal(); + let descriptor_key: Expr = if key_is_evaluated { + let key_ref = self.declare_var_temp_ref(b"_computedKey"); + let binding = self.b(B::Identifier { r#ref: key_ref }, loc); + self.record_usage(key_ref); + let key_temp = self.new_expr(E::Identifier::init(key_ref), loc); + if prop.flags.contains(Flags::Property::IsMethod) { + computed_key_decls.push(Decl { + binding, + value: None, + }); + prop.key = Some(Expr::assign(key_temp, key)); + } else { + computed_key_decls.push(Decl { + binding, + value: Some(key), + }); + // Becomes the `this[_computedKey]` of the relocated initializer. + prop.key = Some(key_temp); + } + + self.record_usage(key_ref); + self.new_expr(E::Identifier::init(key_ref), loc) + } else { + key + }; // TODO: when we have the `accessor` modifier, add `and !prop.flags.contains(.has_accessor_modifier)` to // the if statement. @@ -6609,11 +6648,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O ]); let args = ExprNodeList::from_arena_slice(args_slice); - let decorator = self.call_runtime( - prop.key.expect("infallible: prop has key").loc, - b"__legacyDecorateClassTS", - args, - ); + let decorator = self.call_runtime(loc, b"__legacyDecorateClassTS", args); let decorator_stmt = self.s( S::SExpr { value: decorator, @@ -6806,12 +6841,24 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O // https://github.com/evanw/esbuild/blob/e9413cc4f7ab87263ea244a999c6fa1f1e34dc65/internal/js_parser/js_parser_lower.go#L2742 } - let mut stmts_count: usize = - 1 + static_members.len() + instance_decorators.len() + static_decorators.len(); + let mut stmts_count: usize = 1 + + usize::from(!computed_key_decls.is_empty()) + + static_members.len() + + instance_decorators.len() + + static_decorators.len(); if s_class.class.ts_decorators.len_u32() > 0 { stmts_count += 1; } let mut stmts = BumpVec::::with_capacity_in(stmts_count, self.arena); + if !computed_key_decls.is_empty() { + stmts.push(self.s( + S::Local { + decls: G::DeclList::from_bump_vec(computed_key_decls), + ..Default::default() + }, + stmt.loc, + )); + } stmts.push(stmt); stmts.extend_from_slice(&static_members); stmts.extend_from_slice(&instance_decorators); @@ -7460,6 +7507,29 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O r#ref } + /// A temporary for a `var` statement emitted next to the statement being visited. The + /// symbol is registered in the scope the `var` hoists to, so the renamer keeps it distinct + /// from the temporaries of sibling blocks sharing that binding, and as a declared symbol + /// of the current part, which is what the bundler's renamer reads for top-level names + /// (see `declare_generated_symbol`). Without the renamer, `generate_temp_ref_with_scope` + /// gives it a file-unique name instead. + fn declare_var_temp_ref(&mut self, default_name: &'a [u8]) -> Ref { + let mut scope = self.current_scope_ref(); + while !scope.kind_stops_hoisting() { + scope = scope + .parent + .expect("infallible: the module scope stops hoisting"); + } + let r#ref = self.generate_temp_ref_with_scope(Some(default_name), scope); + self.declared_symbols + .append(DeclaredSymbol { + ref_: r#ref, + is_top_level: scope == self.module_scope, + }) + .expect("oom"); + r#ref + } + pub(crate) fn should_lower_using_declarations(&self, stmts: &[Stmt]) -> bool { // TODO: We do not support lowering await, but when we do this needs to point to that var let lower_await = false; diff --git a/test/bundler/bundler_edgecase.test.ts b/test/bundler/bundler_edgecase.test.ts index 7fe388d3a623..0cba5a84cc08 100644 --- a/test/bundler/bundler_edgecase.test.ts +++ b/test/bundler/bundler_edgecase.test.ts @@ -1717,6 +1717,69 @@ describe("bundler", () => { stdout: '[{"Y":{"0":"A","1":"B","A":0,"B":1,"Z":1}},0,1]', }, }); + // experimentalDecorators lowering evaluates each decorated computed key into a temporary. + // A decorated field reads its temporary from the constructor, so the temporaries of every + // class in the chunk (other modules, sibling blocks in the same module) have to stay distinct. + itBundled("edgecase/TypeScriptDecoratorComputedKeyTemporaries", { + files: { + "/entry.ts": /* ts */ ` + import { A } from "./a"; + import { B } from "./b"; + import { dec, key, decorated } from "./shared"; + let C, D; + { + class Block { @dec [key("cField")] = 1 } + C = Block; + } + { + class Block { @dec [key("dField")] = 1 } + D = Block; + } + console.log(JSON.stringify({ + decorated, + a: Object.keys(new A()), + aPrototype: Object.getOwnPropertyNames(A.prototype), + b: Object.keys(new B()), + bPrototype: Object.getOwnPropertyNames(B.prototype), + c: Object.keys(new C()), + d: Object.keys(new D()), + })); + `, + "/a.ts": /* ts */ ` + import { dec, key } from "./shared"; + export class A { + @dec [key("aField")] = 1; + @dec [key("aMethod")]() {} + } + `, + "/b.ts": /* ts */ ` + import { dec, key } from "./shared"; + export class B { + @dec [key("bField")] = 1; + @dec [key("bMethod")]() {} + } + `, + "/shared.ts": /* ts */ ` + let evaluations = 0; + export const decorated: string[] = []; + // Returns a different key every time it is evaluated. + export const key = (name: string) => name + ++evaluations; + export const dec = (_target: unknown, propertyKey: string) => { decorated.push(propertyKey); }; + `, + "/tsconfig.json": /* json */ `{ "compilerOptions": { "experimentalDecorators": true } }`, + }, + run: { + stdout: JSON.stringify({ + decorated: ["aField1", "aMethod2", "bField3", "bMethod4", "cField5", "dField6"], + a: ["aField1"], + aPrototype: ["constructor", "aMethod2"], + b: ["bField3"], + bPrototype: ["constructor", "bMethod4"], + c: ["cField5"], + d: ["dField6"], + }), + }, + }); itBundled("edgecase/TypeScriptNamespaceSiblingVariable", { files: { "/entry.ts": ` diff --git a/test/bundler/transpiler/decorators.test.ts b/test/bundler/transpiler/decorators.test.ts index 80800f17c415..99d1e8c8fdb6 100644 --- a/test/bundler/transpiler/decorators.test.ts +++ b/test/bundler/transpiler/decorators.test.ts @@ -746,6 +746,242 @@ test("decorators with different property key types", () => { let A = foo("a", "b", "c"); }); +// A computed key must be evaluated exactly once: the decorator has to receive the key the +// member was actually defined under. Every `key()` below returns a different string each +// time it is evaluated, so a second evaluation decorates (or defines) the wrong property. +describe("decorated members with computed keys", () => { + function keyCounter() { + const state = { evaluations: 0, decorated: [] as string[] }; + return { + state, + key(name: string) { + state.evaluations++; + return `${name}${state.evaluations}`; + }, + dec(_target: any, propertyKey: string) { + state.decorated.push(propertyKey); + }, + }; + } + + function ownKeys(target: object) { + return Object.getOwnPropertyNames(target).filter(name => !["length", "name", "prototype"].includes(name)); + } + + test("methods and accessors are decorated under the key they were defined with", () => { + const { state, key, dec } = keyCounter(); + + class A { + @dec [key("method")]() {} + @dec get [key("getter")]() { + return 1; + } + @dec set [key("setter")](_value: number) {} + @dec static [key("staticMethod")]() {} + } + + expect({ + ...state, + prototypeKeys: ownKeys(A.prototype), + staticKeys: ownKeys(A), + }).toEqual({ + evaluations: 4, + decorated: ["method1", "getter2", "setter3", "staticMethod4"], + prototypeKeys: ["constructor", "method1", "getter2", "setter3"], + staticKeys: ["staticMethod4"], + }); + }); + + test("parameter decorators on a method with a computed key", () => { + const { state, key } = keyCounter(); + function paramDec(_target: any, propertyKey: string, index: number) { + state.decorated.push(`${propertyKey}[${index}]`); + } + + class A { + [key("method")](@paramDec _a: string, @paramDec _b: string) {} + } + + expect({ ...state, prototypeKeys: ownKeys(A.prototype) }).toEqual({ + evaluations: 1, + decorated: ["method1[1]", "method1[0]"], + prototypeKeys: ["constructor", "method1"], + }); + }); + + test("instance fields are initialized under the decorated key, without re-evaluating it per instance", () => { + const { state, key, dec } = keyCounter(); + + class A { + @dec [key("field")] = "value"; + } + const first = new A(); + const second = new A(); + + expect({ + ...state, + first: Object.entries(first), + second: Object.entries(second), + }).toEqual({ + evaluations: 1, + decorated: ["field1"], + first: [["field1", "value"]], + second: [["field1", "value"]], + }); + }); + + test("static fields are initialized under the decorated key", () => { + const { state, key, dec } = keyCounter(); + + class A { + @dec static [key("staticField")] = 42; + } + + expect({ ...state, statics: ownKeys(A).map(name => [name, A[name]]) }).toEqual({ + evaluations: 1, + decorated: ["staticField1"], + statics: [["staticField1", 42]], + }); + }); + + test("fields without an initializer", () => { + const { state, key, dec } = keyCounter(); + + class A { + @dec [key("field")]: string; + @dec declare [key("declared")]: string; + @dec static [key("staticField")]: string; + } + new A(); + + expect(state).toEqual({ + evaluations: 3, + decorated: ["field1", "declared2", "staticField3"], + }); + }); + + test("an instance created while the class is being defined uses the decorated key", () => { + const { state, key, dec } = keyCounter(); + + class A { + static instance = new A(); + @dec [key("field")] = "value"; + } + + expect({ + ...state, + duringDefinition: Object.keys(A.instance), + afterDefinition: Object.keys(new A()), + }).toEqual({ + evaluations: 1, + decorated: ["field1"], + duringDefinition: ["field1"], + afterDefinition: ["field1"], + }); + }); + + test("each class keeps its own keys", () => { + const { state, key, dec } = keyCounter(); + + class A { + @dec [key("a")] = "a"; + @dec [key("am")]() {} + } + class B { + @dec [key("b")] = "b"; + @dec [key("bm")]() {} + } + + expect({ + ...state, + a: Object.keys(new A()), + b: Object.keys(new B()), + aPrototype: ownKeys(A.prototype), + bPrototype: ownKeys(B.prototype), + }).toEqual({ + evaluations: 4, + decorated: ["a1", "am2", "b3", "bm4"], + a: ["a1"], + b: ["b3"], + aPrototype: ["constructor", "am2"], + bPrototype: ["constructor", "bm4"], + }); + }); + + test("evaluation order", () => { + const evaluated: string[] = []; + const key = (name: string) => (evaluated.push(name), name); + function dec() {} + + class A { + @dec [key("method")]() {} + @dec [key("field")] = 1; + [key("undecoratedMethod")]() {} + @dec static [key("staticField")] = 2; + @dec get [key("getter")]() { + return 1; + } + } + + // Decorated fields leave the class body (their initializers move into the constructor or + // after the class), so their keys are evaluated right before the class. Everything that + // stays in the body is evaluated in source order, like an undecorated class. + expect(evaluated).toEqual(["field", "staticField", "method", "undecoratedMethod", "getter"]); + }); + + test("transpiled output evaluates each computed key once", () => { + const transpiler = new Bun.Transpiler({ + loader: "ts", + tsconfig: { compilerOptions: { experimentalDecorators: true } }, + }); + const output = transpiler.transformSync(` + declare const dec: any; + declare function key(): string; + class A { + @dec [key()]() {} + @dec [key()] = 1; + @dec static [key()] = 2; + @dec [key()]: string; + @dec ["literal"]() {} + @dec [1] = 3; + } + `); + expect(output).toMatchInlineSnapshot(` + "import { __legacyDecorateClassTS as __legacyDecorateClassTS_3r173x8m } from "bun:wrap"; + var __bun_temp_ref_1$, __bun_temp_ref_2$ = key(), __bun_temp_ref_3$ = key(), __bun_temp_ref_4$ = key(); + + class A { + constructor() { + this[__bun_temp_ref_2$] = 1; + this[1] = 3; + } + [__bun_temp_ref_1$ = key()]() {} + ["literal"]() {} + } + A[__bun_temp_ref_3$] = 2; + __legacyDecorateClassTS_3r173x8m([ + dec + ], A.prototype, __bun_temp_ref_1$, null); + __legacyDecorateClassTS_3r173x8m([ + dec + ], A.prototype, __bun_temp_ref_2$, undefined); + __legacyDecorateClassTS_3r173x8m([ + dec + ], A.prototype, __bun_temp_ref_4$, undefined); + __legacyDecorateClassTS_3r173x8m([ + dec + ], A.prototype, "literal", null); + __legacyDecorateClassTS_3r173x8m([ + dec + ], A.prototype, 1, undefined); + __legacyDecorateClassTS_3r173x8m([ + dec + ], A, __bun_temp_ref_3$, undefined); + " + `); + }); +}); + test("only property decorators", () => { let a = 0; class A {