mirror of
https://github.com/oven-sh/bun
synced 2026-02-02 15:08:46 +00:00
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 <noreply@anthropic.com>
This commit is contained in:
@@ -157,48 +157,23 @@ pub fn Visit(
|
|||||||
/// Check if this arrow is a candidate for the `() => obj.method()` to
|
/// Check if this arrow is a candidate for the `() => obj.method()` to
|
||||||
/// `obj.method.bind(obj)` transformation. If so, store the receiver ref
|
/// `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.
|
/// 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 {
|
pub fn tryMarkArrowForBindCallTransform(p: *P, arrow: *E.Arrow) void {
|
||||||
// Must have exactly one statement
|
_ = p;
|
||||||
if (arrow.body.stmts.len != 1) return;
|
_ = arrow;
|
||||||
|
// Disabled - see comment above for rationale
|
||||||
const stmt = arrow.body.stmts[0];
|
return;
|
||||||
|
|
||||||
// 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;
|
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn visitDecls(noalias p: *P, decls: []G.Decl, was_const: bool, comptime is_possibly_decl_to_remove: bool) usize {
|
pub fn visitDecls(noalias p: *P, decls: []G.Decl, was_const: bool, comptime is_possibly_decl_to_remove: bool) usize {
|
||||||
|
|||||||
@@ -1194,6 +1194,9 @@ describe("bundler", () => {
|
|||||||
});
|
});
|
||||||
|
|
||||||
// Arrow to bind optimization tests
|
// 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", {
|
itBundled("minify/ArrowToBindConstIdentifier", {
|
||||||
files: {
|
files: {
|
||||||
"/entry.js": /* js */ `
|
"/entry.js": /* js */ `
|
||||||
@@ -1206,9 +1209,8 @@ describe("bundler", () => {
|
|||||||
target: "bun",
|
target: "bun",
|
||||||
onAfterBundle(api) {
|
onAfterBundle(api) {
|
||||||
const code = api.readFile("/out.js");
|
const code = api.readFile("/out.js");
|
||||||
// Should transform () => obj.method() to obj.method.bind(obj)
|
// Optimization disabled - should NOT transform
|
||||||
expect(code).toContain(".bind(");
|
expect(code).not.toContain(".bind(");
|
||||||
expect(code).not.toMatch(/\(\)\s*=>\s*\w+\.\w+\(\)/);
|
|
||||||
},
|
},
|
||||||
run: {
|
run: {
|
||||||
stdout: "42",
|
stdout: "42",
|
||||||
@@ -1249,8 +1251,8 @@ describe("bundler", () => {
|
|||||||
target: "bun",
|
target: "bun",
|
||||||
onAfterBundle(api) {
|
onAfterBundle(api) {
|
||||||
const code = api.readFile("/out.js");
|
const code = api.readFile("/out.js");
|
||||||
// Should transform () => obj[key]() to obj[key].bind(obj)
|
// Optimization disabled - should NOT transform
|
||||||
expect(code).toContain(".bind(");
|
expect(code).not.toContain(".bind(");
|
||||||
},
|
},
|
||||||
run: {
|
run: {
|
||||||
stdout: "99",
|
stdout: "99",
|
||||||
@@ -1291,8 +1293,8 @@ describe("bundler", () => {
|
|||||||
target: "bun",
|
target: "bun",
|
||||||
onAfterBundle(api) {
|
onAfterBundle(api) {
|
||||||
const code = api.readFile("/out.js");
|
const code = api.readFile("/out.js");
|
||||||
// Should transform because obj is not reassigned
|
// Optimization disabled - should NOT transform
|
||||||
expect(code).toContain(".bind(");
|
expect(code).not.toContain(".bind(");
|
||||||
},
|
},
|
||||||
run: {
|
run: {
|
||||||
stdout: "42",
|
stdout: "42",
|
||||||
@@ -1312,15 +1314,15 @@ describe("bundler", () => {
|
|||||||
target: "bun",
|
target: "bun",
|
||||||
onAfterBundle(api) {
|
onAfterBundle(api) {
|
||||||
const code = api.readFile("/out.js");
|
const code = api.readFile("/out.js");
|
||||||
// Should transform because init is not reassigned
|
// Optimization disabled - should NOT transform
|
||||||
expect(code).toContain(".bind(");
|
expect(code).not.toContain(".bind(");
|
||||||
},
|
},
|
||||||
run: {
|
run: {
|
||||||
stdout: "555",
|
stdout: "555",
|
||||||
},
|
},
|
||||||
});
|
});
|
||||||
|
|
||||||
itBundled("minify/ArrowToBindFunctionParamReassigned", {
|
itBundled("minify/ArrowToBindNoTransformFunctionParamReassigned", {
|
||||||
files: {
|
files: {
|
||||||
"/entry.js": /* js */ `
|
"/entry.js": /* js */ `
|
||||||
function fetch(init) {
|
function fetch(init) {
|
||||||
@@ -1548,4 +1550,53 @@ describe("bundler", () => {
|
|||||||
stdout: "method called",
|
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",
|
||||||
|
},
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
Reference in New Issue
Block a user