From e5c8da3c998a47a9f6c427518bff3fecb4407dc0 Mon Sep 17 00:00:00 2001 From: Claude Bot Date: Sun, 1 Feb 2026 15:38:43 +0000 Subject: [PATCH] fix(minify): disable arrow-to-bind transform due to semantic issues The arrow-to-bind transformation (`() => obj.method()` -> `obj.method.bind(obj)`) can change semantics in several cases: 1. Property reassignment: If `obj.method` is reassigned after the arrow is created, the bound function would still reference the original method. 2. Getter properties: If `method` is a getter, bind() would only call the getter once at bind time, while the arrow calls it on each invocation. 3. Object escaping: The object could be modified by code we can't analyze. This commit disables the optimization until proper property tracking is implemented. Added tests to document these edge cases: - ArrowToBindNoTransformPropertyReassigned - ArrowToBindNoTransformGetterProperty Also fixed test naming to use consistent NoTransform prefix pattern. Co-Authored-By: Claude Opus 4.5 --- src/ast/visit.zig | 57 +++++++---------------- test/bundler/bundler_minify.test.ts | 71 +++++++++++++++++++++++++---- 2 files changed, 77 insertions(+), 51 deletions(-) diff --git a/src/ast/visit.zig b/src/ast/visit.zig index 05ec3dcd31..e1469533e7 100644 --- a/src/ast/visit.zig +++ b/src/ast/visit.zig @@ -157,48 +157,23 @@ pub fn Visit( /// Check if this arrow is a candidate for the `() => obj.method()` to /// `obj.method.bind(obj)` transformation. If so, store the receiver ref /// in the arrow so the printer can check if the symbol was assigned to. + /// + /// NOTE: This optimization is currently DISABLED because it can change + /// semantics in cases where: + /// 1. The property (method) is reassigned after the arrow is created + /// 2. The property is a getter that returns different values on each access + /// + /// To safely enable this optimization, we would need to track: + /// - Property assignments to the receiver object + /// - Whether the property is defined as a getter + /// - Whether the object escapes to code that could modify it + /// + /// For now, we conservatively disable the transformation entirely. pub fn tryMarkArrowForBindCallTransform(p: *P, arrow: *E.Arrow) void { - // Must have exactly one statement - if (arrow.body.stmts.len != 1) return; - - const stmt = arrow.body.stmts[0]; - - // Must be a return statement - const return_value = switch (stmt.data) { - .s_return => |ret| ret.value orelse return, - else => return, - }; - - // The return value must be a call expression with no arguments - const call = switch (return_value.data) { - .e_call => |c| c, - else => return, - }; - - // Call must have no arguments and no optional chaining - if (call.args.len != 0 or call.optional_chain != null) return; - - // Call target must be a property access (dot or index) without optional chaining - const receiver_ref: Ref = switch (call.target.data) { - .e_dot => |dot| if (dot.optional_chain != null) return else switch (dot.target.data) { - .e_identifier => |ident| ident.ref, - else => return, - }, - .e_index => |idx| if (idx.optional_chain != null) return else switch (idx.target.data) { - .e_identifier => |ident| ident.ref, - else => return, - }, - else => return, - }; - - // Don't transform unbound globals (like console, Math, etc.) - // They could be reassigned by other code we can't see. - const symbol = p.symbols.items[receiver_ref.innerIndex()]; - if (symbol.kind == .unbound) return; - - // Mark this arrow as a candidate for bind transformation. - // The printer will check if the symbol was assigned to before applying. - arrow.bind_call_target_ref = receiver_ref; + _ = p; + _ = arrow; + // Disabled - see comment above for rationale + return; } pub fn visitDecls(noalias p: *P, decls: []G.Decl, was_const: bool, comptime is_possibly_decl_to_remove: bool) usize { diff --git a/test/bundler/bundler_minify.test.ts b/test/bundler/bundler_minify.test.ts index 54f1bb8ec2..b1efdf3f61 100644 --- a/test/bundler/bundler_minify.test.ts +++ b/test/bundler/bundler_minify.test.ts @@ -1194,6 +1194,9 @@ describe("bundler", () => { }); // Arrow to bind optimization tests + // NOTE: The arrow-to-bind transformation is currently DISABLED because + // it can change semantics when properties are reassigned or are getters. + // These tests verify the transformation is NOT applied. itBundled("minify/ArrowToBindConstIdentifier", { files: { "/entry.js": /* js */ ` @@ -1206,9 +1209,8 @@ describe("bundler", () => { target: "bun", onAfterBundle(api) { const code = api.readFile("/out.js"); - // Should transform () => obj.method() to obj.method.bind(obj) - expect(code).toContain(".bind("); - expect(code).not.toMatch(/\(\)\s*=>\s*\w+\.\w+\(\)/); + // Optimization disabled - should NOT transform + expect(code).not.toContain(".bind("); }, run: { stdout: "42", @@ -1249,8 +1251,8 @@ describe("bundler", () => { target: "bun", onAfterBundle(api) { const code = api.readFile("/out.js"); - // Should transform () => obj[key]() to obj[key].bind(obj) - expect(code).toContain(".bind("); + // Optimization disabled - should NOT transform + expect(code).not.toContain(".bind("); }, run: { stdout: "99", @@ -1291,8 +1293,8 @@ describe("bundler", () => { target: "bun", onAfterBundle(api) { const code = api.readFile("/out.js"); - // Should transform because obj is not reassigned - expect(code).toContain(".bind("); + // Optimization disabled - should NOT transform + expect(code).not.toContain(".bind("); }, run: { stdout: "42", @@ -1312,15 +1314,15 @@ describe("bundler", () => { target: "bun", onAfterBundle(api) { const code = api.readFile("/out.js"); - // Should transform because init is not reassigned - expect(code).toContain(".bind("); + // Optimization disabled - should NOT transform + expect(code).not.toContain(".bind("); }, run: { stdout: "555", }, }); - itBundled("minify/ArrowToBindFunctionParamReassigned", { + itBundled("minify/ArrowToBindNoTransformFunctionParamReassigned", { files: { "/entry.js": /* js */ ` function fetch(init) { @@ -1548,4 +1550,53 @@ describe("bundler", () => { stdout: "method called", }, }); + itBundled("minify/ArrowToBindNoTransformPropertyReassigned", { + files: { + "/entry.js": /* js */ ` + const obj = { method() { return "original"; } }; + const fn = () => obj.method(); + obj.method = () => "reassigned"; + console.log(fn()); + `, + }, + minifySyntax: true, + target: "bun", + onAfterBundle(api) { + const code = api.readFile("/out.js"); + // Should NOT transform because obj.method is reassigned after the arrow is created + // If we transform to obj.method.bind(obj), it would capture the original method + // But the arrow should call the reassigned method + expect(code).not.toContain(".bind("); + }, + run: { + stdout: "reassigned", + }, + }); + itBundled("minify/ArrowToBindNoTransformGetterProperty", { + files: { + "/entry.js": /* js */ ` + let callCount = 0; + const obj = { + get method() { + callCount++; + return () => "from getter " + callCount; + } + }; + const fn = () => obj.method(); + console.log(fn()); + console.log(fn()); + `, + }, + minifySyntax: true, + target: "bun", + onAfterBundle(api) { + const code = api.readFile("/out.js"); + // Should NOT transform because method is a getter + // bind() would only call the getter once, but arrow calls it each time + expect(code).not.toContain(".bind("); + }, + run: { + stdout: "from getter 1\nfrom getter 2", + }, + }); });