mirror of
https://github.com/oven-sh/bun
synced 2026-02-02 15:08:46 +00:00
Fix comma operator optimization to preserve 'this' binding semantics (#21653)
## Summary - Fix transpiler bug where comma expressions like `(0, obj.method)()` were incorrectly optimized to `obj.method()` - This preserved the `this` binding instead of stripping it as per JavaScript semantics - Add comprehensive regression test to prevent future issues ## Root Cause The comma operator optimization in `src/js_parser.zig:7281` was directly returning the right operand when the left operand had no side effects, without checking if the expression was being used as a call target. ## Solution - Added the same `is_call_target` check that other operators (nullish coalescing, logical OR/AND) use - When a comma expression is used as a call target AND the right operand has a value for `this`, preserve the comma expression to strip the `this` binding - Follows existing patterns in the codebase for consistent behavior ## Test Plan - [x] Reproduce the original bug: `(0, obj.method)()` incorrectly preserved `this` - [x] Verify fix: comma expressions now correctly strip `this` binding in function calls - [x] All existing transpiler tests continue to pass - [x] Added regression test covering various comma expression scenarios - [x] Tested edge cases: nested comma expressions, side effects, different operand types 🤖 Generated with [Claude Code](https://claude.ai/code) --------- Co-authored-by: Claude Bot <claude-bot@bun.sh> Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
This commit is contained in:
@@ -88,8 +88,21 @@ pub fn CreateBinaryExpressionVisitor(
|
||||
.bin_comma => {
|
||||
// "(1, 2)" => "2"
|
||||
// "(sideEffects(), 2)" => "(sideEffects(), 2)"
|
||||
// "(0, this.fn)" => "this.fn"
|
||||
// "(0, this.fn)()" => "(0, this.fn)()"
|
||||
if (p.options.features.minify_syntax) {
|
||||
e_.left = SideEffects.simplifyUnusedExpr(p, e_.left) orelse return e_.right;
|
||||
if (SideEffects.simplifyUnusedExpr(p, e_.left)) |simplified_left| {
|
||||
e_.left = simplified_left;
|
||||
} else {
|
||||
// The left operand has no side effects, but we need to preserve
|
||||
// the comma operator semantics when used as a call target
|
||||
if (is_call_target and e_.right.hasValueForThisInCall()) {
|
||||
// Keep the comma expression to strip "this" binding
|
||||
e_.left = Expr{ .data = Prefill.Data.Zero, .loc = e_.left.loc };
|
||||
} else {
|
||||
return e_.right;
|
||||
}
|
||||
}
|
||||
}
|
||||
},
|
||||
.bin_loose_eq => {
|
||||
|
||||
@@ -2865,6 +2865,35 @@ console.log(foo, array);
|
||||
// check("let x = arg0?.[foo]; (0, x)()", "let x = arg0?.[foo];\nx();");
|
||||
});
|
||||
|
||||
it("comma operator transforms", () => {
|
||||
const expectPrinted = (code, out) => {
|
||||
expect(parsed(code, true, true, transpilerMinifySyntax)).toBe(out);
|
||||
};
|
||||
|
||||
// Comma operator should be optimized when not used as call target
|
||||
expectPrinted("(0, 1)", "1");
|
||||
expectPrinted("(0, foo)", "foo");
|
||||
expectPrinted("(sideEffect(), foo)", "(sideEffect(), foo)");
|
||||
|
||||
// Comma operator should preserve 'this' binding semantics when used as call target
|
||||
expectPrinted("(0, obj.method)()", "(0, obj.method)()");
|
||||
expectPrinted("(0, obj[key])()", "(0, obj[key])()");
|
||||
expectPrinted("(0, obj?.method)()", "(0, obj?.method)()");
|
||||
expectPrinted("(0, obj?.[key])()", "(0, obj?.[key])()");
|
||||
|
||||
// Side effects should still be preserved in call context
|
||||
expectPrinted("(sideEffect(), obj.method)()", "(sideEffect(), obj.method)()");
|
||||
|
||||
// Non-method calls should still be optimized even in call context
|
||||
expectPrinted("(0, func)()", "func()");
|
||||
expectPrinted("(0, getValue())()", "getValue()()");
|
||||
|
||||
// Non-call target with function call as second value should be optimized
|
||||
expectPrinted("(0, obj.method)", "obj.method");
|
||||
expectPrinted("(0, obj[key])", "obj[key]");
|
||||
expectPrinted("(0, func())", "func()");
|
||||
});
|
||||
|
||||
it("constant folding", () => {
|
||||
const expectPrinted = (code, out) => {
|
||||
expect(parsed(code, true, true, transpilerMinifySyntax)).toBe(out);
|
||||
|
||||
44
test/regression/issue/comma-operator-this-binding.test.ts
Normal file
44
test/regression/issue/comma-operator-this-binding.test.ts
Normal file
@@ -0,0 +1,44 @@
|
||||
import { expect, test } from "bun:test";
|
||||
import { bunEnv, bunExe, tempDirWithFiles } from "harness";
|
||||
|
||||
test("comma operator should strip 'this' binding in function calls", async () => {
|
||||
const dir = tempDirWithFiles("comma-operator-test", {
|
||||
"test.js": `
|
||||
const doThing = () => {};
|
||||
|
||||
const cool = {
|
||||
value: "beans",
|
||||
logValue() {
|
||||
console.log(this?.value || "undefined");
|
||||
}
|
||||
}
|
||||
|
||||
// Direct call - should preserve 'this'
|
||||
cool.logValue();
|
||||
|
||||
// Comma operator calls - should strip 'this'
|
||||
(0, cool.logValue)();
|
||||
(doThing(), cool.logValue)();
|
||||
`,
|
||||
});
|
||||
|
||||
await using proc = Bun.spawn({
|
||||
cmd: [bunExe(), "test.js"],
|
||||
env: bunEnv,
|
||||
cwd: dir,
|
||||
stderr: "pipe",
|
||||
stdout: "pipe",
|
||||
});
|
||||
|
||||
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
|
||||
|
||||
expect(exitCode).toBe(0);
|
||||
expect(stderr).toBe("");
|
||||
|
||||
// Should output: beans, undefined, undefined
|
||||
const lines = stdout.trim().split("\n");
|
||||
expect(lines).toHaveLength(3);
|
||||
expect(lines[0]).toBe("beans");
|
||||
expect(lines[1]).toBe("undefined");
|
||||
expect(lines[2]).toBe("undefined");
|
||||
});
|
||||
Reference in New Issue
Block a user