Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 107 additions & 2 deletions src/js_parser/lower/lower_decorators.rs
Original file line number Diff line number Diff line change
Expand Up @@ -756,7 +756,28 @@
if let Some(info) = map.get(&pi.ref_.inner_index()).copied() {
let mut obj_expr = tgt_idx.target;
self.rewrite_private_accesses_in_expr(&mut obj_expr, map);
let private_access = self.private_get_expr(obj_expr, &info, expr_loc);
// `x.#m(...)` becomes `__privateGet(x, _m).call(x, ...)`, which
// references the receiver twice. Only identifiers and `this` can
// be repeated safely; any other receiver is captured in a
// temporary so its side effects run once and nested private
// calls don't duplicate the whole subtree (the duplication is
// exponential in the length of a chain like `o.#m().#m().#m()`).
let (get_obj, this_arg) = match &obj_expr.data {
js_ast::ExprData::EIdentifier(id) => {
let obj_ref = id.ref_;
(obj_expr, self.use_ref(obj_ref, obj_expr.loc))
}
js_ast::ExprData::EThis(_) => {
(obj_expr, self.new_expr(E::This {}, obj_expr.loc))
}
_ => {
let tmp_ref = self.generate_temp_ref(Some(b"_obj"));
let write = self.assign_to(tmp_ref, obj_expr, expr_loc);
let read = self.use_ref(tmp_ref, expr_loc);
(write, read)
}
Comment thread
robobun marked this conversation as resolved.
};
let private_access = self.private_get_expr(get_obj, &info, expr_loc);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
let call_target = self.new_expr(
E::Dot {
target: private_access,
Expand All @@ -769,7 +790,7 @@
let bump = self.arena;
let orig_args = e.args.slice_mut();
let mut new_args = BumpVec::with_capacity_in(1 + orig_args.len(), bump);
new_args.push(obj_expr);
new_args.push(this_arg);
for arg in orig_args.iter_mut() {
self.rewrite_private_accesses_in_expr(arg, map);
new_args.push(*arg);
Expand Down Expand Up @@ -842,17 +863,67 @@
}
}
js_ast::ExprData::EFunction(e) => {
let temps_before = self.temp_refs_to_declare.len();
let stmts = e.func.body.stmts.slice_mut();
self.rewrite_private_accesses_in_stmts(stmts, map);
e.func.body.stmts = self.declare_capture_temps_in_fn_body(
e.func.body.stmts,
temps_before,
e.func.body.loc,
);
}
js_ast::ExprData::EArrow(e) => {
let temps_before = self.temp_refs_to_declare.len();
let stmts = e.body.stmts.slice_mut();
self.rewrite_private_accesses_in_stmts(stmts, map);
e.body.stmts =
self.declare_capture_temps_in_fn_body(e.body.stmts, temps_before, e.body.loc);
}
_ => {}
}
}

/// Declare receiver-capture temporaries created past `temps_before` at the
/// top of the function body they were created in, so each invocation gets
/// a fresh binding. A binding hoisted outside the function would be shared
/// across invocations, and `__privateGet(_obj = recv, _s, getter)` runs the
/// user getter between the write and the `.call(_obj)` read; re-entering
/// the same call site through that getter would clobber the shared temp.
fn declare_capture_temps_in_fn_body(
&mut self,
stmts: js_ast::StmtNodeList,
temps_before: usize,
body_loc: bun_ast::Loc,
) -> js_ast::StmtNodeList {
let total = self.temp_refs_to_declare.len();
if total == temps_before {
return stmts;
}
let bump = self.arena;
let mut capture_decls = BumpVec::<G::Decl>::with_capacity_in(total - temps_before, bump);
for i in temps_before..total {
let capture_ref = self.temp_refs_to_declare[i].r#ref;
let binding = self.b(B::Identifier { r#ref: capture_ref }, body_loc);
capture_decls.push(G::Decl {
binding,
value: None,
});
}
self.temp_refs_to_declare.truncate(temps_before);
let decl_stmt = self.s(
S::Local {
decls: DeclList::from_bump_vec(capture_decls),
..Default::default()
},
body_loc,
);
let old_stmts = stmts.slice();
let mut new_stmts = BumpVec::<Stmt>::with_capacity_in(old_stmts.len() + 1, bump);
new_stmts.push(decl_stmt);
new_stmts.extend_from_slice(old_stmts);
js_ast::StmtNodeList::from_bump(new_stmts)
}

fn rewrite_private_accesses_in_stmts(&mut self, stmts: &mut [Stmt], map: &PrivateLoweredMap) {
for stmt_item in stmts.iter_mut() {
match &mut stmt_item.data {
Expand Down Expand Up @@ -1026,6 +1097,12 @@
let p = self;
let bump = p.arena;

// Receiver-capture temporaries created by `rewrite_private_accesses_in_expr`
// land in `temp_refs_to_declare`; everything pushed past this point is
// declared in a `var` statement alongside the other lowering variables
// right before output assembly.
let temp_refs_before = p.temp_refs_to_declare.len();

// ── Phase 1: Setup ───────────────────────────────
let mut class_name_ref: Ref;
let mut class_name_loc: bun_ast::Loc;
Expand Down Expand Up @@ -2353,6 +2430,34 @@
class.has_decorators = false;
class.should_lower_standard_decorators = false;

// Declare the receiver-capture temporaries that were created outside any
// function body (field initializers, static blocks, pre-eval/decorate
// expressions). Temps created inside method/function/arrow bodies were
// already declared there by `declare_capture_temps_in_fn_body`; these
// remaining sites run at most once per class evaluation, so a binding
// hoisted next to the other lowering variables is safe (and matches
// where esbuild hoists them).
Comment thread
robobun marked this conversation as resolved.
Outdated
if p.temp_refs_to_declare.len() > temp_refs_before {
let capture_count = p.temp_refs_to_declare.len() - temp_refs_before;
let mut capture_decls = BumpVec::<G::Decl>::with_capacity_in(capture_count, bump);
for i in temp_refs_before..p.temp_refs_to_declare.len() {
let capture_ref = p.temp_refs_to_declare[i].r#ref;
let binding = p.b(B::Identifier { r#ref: capture_ref }, loc);
capture_decls.push(G::Decl {
binding,
value: None,
});
}
p.temp_refs_to_declare.truncate(temp_refs_before);
prefix_stmts.push(p.s(
S::Local {
decls: DeclList::from_bump_vec(capture_decls),
..Default::default()
},
loc,
));
}

Check warning on line 2459 in src/js_parser/lower/lower_decorators.rs

View check run for this annotation

Claude / Claude Code Review

Duplicated capture-temp var-decl construction between declare_capture_temps_in_fn_body and the Phase-8 prefix_stmts block

nit: this block is near-identical to the body of `declare_capture_temps_in_fn_body` (lines 898–919) — both drain `temp_refs_to_declare[baseline..]` into `B::Identifier` decls in a `with_capacity_in` vec, truncate, and wrap in `S::Local{..Default::default()}`. Per CLAUDE.md ("the second time a multi-line block appears in your diff, extract a named helper"), consider a helper like `fn drain_capture_temp_decls(&mut self, baseline: usize, loc: Loc) -> Option<Stmt>` that both call sites consume — one
Comment thread
robobun marked this conversation as resolved.
Outdated

// ── Phase 8: Assemble output ─────────────────────
if is_expr {
let mut comma_parts = BumpVec::<Expr>::new_in(bump);
Expand Down
165 changes: 165 additions & 0 deletions test/bundler/transpiler/es-decorators.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -753,6 +753,171 @@
});
});

describe("private member calls in lowered classes", () => {
// When a class is lowered for standard decorators, `recv.#m(...)` becomes
// `__privateGet(recv, _m).call(recv, ...)`. The receiver must be evaluated
// exactly once: duplicating it re-runs side effects and makes the printed
// output grow exponentially for chains like `o.#m().#m().#m()`.
test("chained optional private calls do not explode the transpiled output size", () => {
const chain = "?.Foo.#m()".repeat(20);
const source = `class Foo {
static #x = -0;
static #m = function() {};
@decorator() est() {
return [o${chain}];
}
}`;

const transpiler = new Bun.Transpiler({ loader: "js", target: "bun" });
const output = transpiler.transformSync(source);

// Exponential duplication produced ~47 MB for a 20-call chain; the
// single-evaluation lowering stays in the kilobytes.
expect(output.length).toBeLessThan(50_000);
// The lowered output must still be valid syntax.
expect(() => new Bun.Transpiler({ loader: "js" }).transformSync(output)).not.toThrow();
});

test("double-call private chains in decorated static field initializers stay linear", () => {
// Fuzzer-minimized variant: each `.#method()()` link re-lowers the whole
// receiver, so duplicating it doubles the printed output per link
// (~30 links allocated multiple GB before aborting).
const chain = ".#method()()".repeat(20);
const source = `class C {
@decorator() static s = new C()${chain.slice(0, -2)};
#method() { return 1e999; }
}`;

const transpiler = new Bun.Transpiler({ loader: "ts", target: "bun", deadCodeElimination: true });
const output = transpiler.transformSync(source);

// Exponential duplication produced ~64 MB for 20 links; the
// single-evaluation lowering stays in the kilobytes.
expect(output.length).toBeLessThan(50_000);
// The lowered output must still be valid syntax.
expect(() => new Bun.Transpiler({ loader: "js" }).transformSync(output)).not.toThrow();
});

test("calling the result of a private method call evaluates each link once", async () => {
const { stdout, stderr, exitCode } = await runDecorator(`
function dec(value, ctx) { return value; }
let evals = 0;
class C {
@dec static s = new C().#method()().#method()().#method()();
#method() { evals++; const self = this; return () => self; }
}
console.log(C.s instanceof C, evals);
`);
expect(stderr).toBe("");
expect(stdout).toBe("true 3\n");
expect(exitCode).toBe(0);
});

test("receiver temps are scoped per invocation, not shared across reentrant calls", async () => {
// A private getter runs user code inside __privateGet, between the
// `_obj = recv` write and the `.call(_obj)` read. If the getter reenters
// the same call site, a temp hoisted outside the method would be
// clobbered and the outer call would see the inner receiver. Declaring
// the temp inside the method body gives each invocation its own binding.
const { stdout, stderr, exitCode } = await runDecorator(`
function dec(value, ctx) { return value; }
let nextId = 0;
let depth = 0;
const order = [];
class C {
get #g() {
if (depth++ === 0) make().run();
const self = this;
return function () { order.push(self.id + ":" + this.id); };
}
@dec run() { make().#g(); }
}
function make() { const c = new C(); c.id = ++nextId; return c; }
make().run();
console.log(JSON.stringify(order));
`);
expect(stderr).toBe("");
// Each entry pairs the receiver seen at getter time with the receiver
// the returned function was invoked on; they must always match.
expect(stdout).toBe('["4:4","2:2"]\n');
expect(exitCode).toBe(0);
});

test("private method call receiver is evaluated exactly once", async () => {
const { stdout, stderr, exitCode } = await runDecorator(`
function dec(value, ctx) { return value; }
let receiverEvals = 0;
class Counter {
static #m = function (x) { return [this === Counter, x]; };
@dec test() {
return getCounter().#m(42);
}
}
function getCounter() { receiverEvals++; return Counter; }
console.log(JSON.stringify(new Counter().test()), receiverEvals);
`);
expect(stderr).toBe("");
expect(stdout).toBe("[true,42] 1\n");
expect(exitCode).toBe(0);
});

test("chained optional private method calls return the right value", async () => {
const { stdout, stderr, exitCode } = await runDecorator(`
function dec(value, ctx) { return value; }
class Chain {
#tag;
constructor(tag) { this.#tag = tag; }
#next() { return { Chain: new Chain(this.#tag + 1) }; }
@dec run(o) {
return o?.Chain.#next()?.Chain.#next()?.Chain.#next()?.Chain.tag();
}
tag() { return this.#tag; }
}
console.log(new Chain(0).run({ Chain: new Chain(10) }));
`);
expect(stderr).toBe("");
expect(stdout).toBe("13\n");
expect(exitCode).toBe(0);
});

test("private method calls through `this` and identifier receivers still work", async () => {
const { stdout, stderr, exitCode } = await runDecorator(`
function dec(value, ctx) { return value; }
class Fast {
#p(n) { return "p" + n; }
@dec viaThis() { return this.#p(1); }
@dec viaIdent(other) { return other.#p(2); }
}
const f = new Fast();
console.log(f.viaThis(), f.viaIdent(new Fast()));
`);
expect(stderr).toBe("");
expect(stdout).toBe("p1 p2\n");
expect(exitCode).toBe(0);
});

// Decorated class expressions emit the capture temporaries through a
// different path (a `var` statement hoisted to the nearest statement
// list instead of prefix statements before the class statement).
test("decorated class expression evaluates chained private call receivers once", async () => {

Check warning on line 902 in test/bundler/transpiler/es-decorators.test.ts

View check run for this annotation

Claude / Claude Code Review

Stale comment: class-expression test no longer exercises the is_expr capture-temp hoisting path

🟡 nit: this comment was accurate when the test was added in 38025495, but ee1c2d3 changed the lowering so capture temps created inside a method body are declared at the top of that body via `declare_capture_temps_in_fn_body`. All complex receivers in this test live inside `@dec test(o) { ... }`, so their temps now go into the method body and the `is_expr` → `expr_var_decls` hoisting path (lower_decorators.rs:2440-2459) is no longer reached here. Either reword the comment, or move a complex-recei
Comment thread
robobun marked this conversation as resolved.
Outdated
const { stdout, stderr, exitCode } = await runDecorator(`
function dec(value, ctx) { return value; }
let evals = 0;
const C = class Foo {
static #m = function (tag) { return { Foo, tag }; };
@dec test(o) {
return o.effectful()?.Foo.#m("a")?.Foo.#m("b");
}
};
const o = { Foo: C, effectful() { evals++; return { Foo: C }; } };
console.log(new C().test(o).tag, evals);
`);
expect(stderr).toBe("");
expect(stdout).toBe("b 1\n");
expect(exitCode).toBe(0);
});
});

describe("accessor with TypeScript annotations", () => {
test("accessor with definite assignment assertion (!)", async () => {
using dir = tempDir("es-dec-accessor-bang", {
Expand Down
Loading