Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
10 changes: 9 additions & 1 deletion src/js_parser/visit/visit_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1057,7 +1057,14 @@
let target = e_.target.unwrap_inlined();
let index = e_.index.unwrap_inlined();

if p.options.features.minify_syntax {
// `a[n]` is a property reference; replacing it with a value changes
// the result of `delete` (reference → value makes it return `true`)
// and of assignment (writes to the wrong place). Neither `"foo"[2]`
// → `"o"` nor `[x][0]` → `x` is safe in those positions.
if p.options.features.minify_syntax
&& !is_delete_target
&& in_.assign_target == js_ast::AssignTarget::None
{

Check failure on line 1067 in src/js_parser/visit/visit_expr.rs

View check run for this annotation

Claude / Claude Code Review

[x][0] fold not guarded for call targets — changes this binding

The new guard adds `!is_delete_target` and `assign_target == None` but omits `!is_call_target` — the third position where the property-reference/value distinction is observable. With `minify_syntax` on, `[obj.m][0]()` (this = the temporary array) folds to `obj.m()` (this = obj), changing the receiver. `is_call_target` is already computed at line 887 and `can_be_inlined_from_property_access` accepts `EDot`/`EIdentifier`, so the fold fires; per REVIEW.md's "fix the whole class", add `&& !is_call_t
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
if let Some(number) = index.data.as_e_number() {
if number.value() >= 0.0
&& number.value() < (usize::MAX as f64)
Expand Down Expand Up @@ -1218,6 +1225,7 @@
}
}
Op::UnDelete => {
p.delete_target = e_.value.data;
Comment thread
robobun marked this conversation as resolved.
p.visit_expr_in_out(&mut e_.value, ExprIn::default());
Comment thread
robobun marked this conversation as resolved.
}
_ => {
Expand Down
5 changes: 4 additions & 1 deletion src/jsc/RuntimeTranspilerCache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,10 @@ bun_core::declare_scope!(cache, visible);
/// bindings from the compiled bytecode after the module-loader rewrite, so the
/// record no longer carries them; blobs written in the old numbering must not
/// be read back.
const EXPECTED_VERSION: u32 = 24;
/// Version 25: `p.delete_target` is now set before visiting the `delete`
/// operand, so `delete E.Member` / `delete {f:x}.f` / `delete [x][0]` /
/// `delete "s"[n]` no longer fold to a value under `minify_syntax`.
Comment thread
robobun marked this conversation as resolved.
Outdated
const EXPECTED_VERSION: u32 = 25;

/// Source files smaller than this are not written to / read from the on-disk
/// transpiler cache. Originally 50 KiB, which excluded almost every file in a
Expand Down
30 changes: 30 additions & 0 deletions test/bundler/bundler_cjs2esm.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,36 @@ describe("bundler", () => {
stdout: '[[{"xyz":456},456],[{"xyz":123},123],[{"xyz":456},456],[{"xyz":123},123]]',
},
});
itBundled("cjs2esm/DeleteExportsPropertyDeopt", {
files: {
"/entry.js": /* js */ `
import * as lib from './lib.js';
console.log(lib.a, lib.b);
`,
"/lib.js": /* js */ `
exports.a = 1;
exports.b = 2;
delete exports.a;
`,
},
cjs2esm: { unhandled: ["/lib.js"] },
run: { stdout: "undefined 2" },
});
itBundled("cjs2esm/DeleteModuleExportsPropertyDeopt", {
files: {
"/entry.js": /* js */ `
import * as lib from './lib.js';
console.log(lib.a, lib.b);
`,
"/lib.js": /* js */ `
module.exports.a = 1;
module.exports.b = 2;
delete module.exports.a;
`,
},
cjs2esm: { unhandled: ["/lib.js"] },
run: { stdout: "undefined 2" },
});
// https://github.com/oven-sh/bun/issues/4565
// `exports.x = ...` as the unbraced body of if/while/do/else must not be
// converted to `var $x = ...; export { $x as x };` because `export` is only
Expand Down
11 changes: 11 additions & 0 deletions test/bundler/transpiler/assign-to-import.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,17 @@ describe("assigning to an imported binding", () => {
`import * as ns from "./m.mjs"; const k = "x"; ns[k] = 5;\n`,
'Cannot assign to property on import "ns"',
],
["delete namespace property", `import * as ns from "./m.mjs"; delete ns.x;\n`, 'Cannot assign to import "x"'],
[
"delete string-index namespace property",
`import * as ns from "./m.mjs"; delete ns["x"];\n`,
'Cannot assign to import "x"',
],
[
"delete computed namespace property",
`import * as ns from "./m.mjs"; const k = "x"; delete ns[k];\n`,
'Cannot assign to property on import "ns"',
],
])("bun build still rejects it: %s", async (_name, entry, diagnostic) => {
using dir = tempDir("assign-to-import-build", {
"m.mjs": mod,
Expand Down
51 changes: 51 additions & 0 deletions test/bundler/transpiler/transpiler.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,57 @@ describe("Bun.Transpiler", () => {
it("works nested", () => {
ts.expectPrintedMin_('const a = ["hey"][0][0];', 'const a = "h"');
});
it("bails out when the index is a delete or assignment target", () => {
// `a[n]` is a property reference; folding it to a value changes the
// result of `delete` and the effect of assignment.
ts.expectPrintedMin_("x = delete [y][0]", "x = delete [y][0]");
ts.expectPrintedMin_("x = delete [y.z][0]", "x = delete [y.z][0]");
ts.expectPrintedMin_("x = delete { f: y }.f", "x = delete { f: y }.f");
ts.expectPrintedMin_("x = delete { f: y }['f']", "x = delete { f: y }.f");
ts.expectPrintedMin_('x = delete "foo"[2]', 'x = delete "foo"[2]');
ts.expectPrintedMin_("x = [y][0] = 5", "x = [y][0] = 5");
ts.expectPrintedMin_("x = [y][0] += 5", "x = [y][0] += 5");
ts.expectPrintedMin_("[y][0]++", "[y][0]++");
ts.expectPrintedMin_('x = "foo"[2] = 5', 'x = "foo"[2] = 5');
// Still inlined outside those positions.
ts.expectPrintedMin_("x = [y][0]", "x = y");
ts.expectPrintedMin_('x = "foo"[2]', 'x = "o"');
ts.expectPrintedMin_("x = delete [y][0].z", "x = delete y.z");
});
it("does not inline an enum member under delete", () => {
const pre = "enum E { A = 1 }\n";
const lastLine = out => out.trimEnd().split("\n").at(-1);
expect(lastLine(ts.parsed(pre + "x = delete E.A;", false))).toBe("x = delete E.A;");
expect(lastLine(ts.parsed(pre + 'x = delete E["A"];', false))).toBe('x = delete E["A"];');
expect(lastLine(ts.parsedMin(pre + "x = delete E.A;", false))).toBe("x = delete E.A;");
// Still inlined when read.
expect(lastLine(ts.parsed(pre + "x = E.A;", false))).toBe("x = 1 /* A */;");
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
it("does not substitute --define for a delete target", () => {
// `user_undefined` is defined as `undefined` in the transpiler config above.
ts.expectPrintedMin_("x = delete user_undefined", "x = delete user_undefined");
// A later read is still substituted (delete_target is per-node, not per-symbol).
ts.expectPrintedMin_(
"x = delete user_undefined; y = user_undefined;",
"x = delete user_undefined;\ny = void 0;\n",
);
});
it("preserves delete/assign semantics at runtime", async () => {
const src = `
var obj = { p: 1 };
var r1 = delete [obj.p][0];
var y = 1;
[y][0] = 5;
var k = { f: 7 };
var r2 = delete { f: k.f }.f;
console.log(JSON.stringify([obj.p, r1, y, k.f, r2]));
`;
await using proc = Bun.spawn({ cmd: [bunExe(), "-e", src], env: bunEnv, stderr: "pipe" });
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toBe("");
expect(stdout).toBe("[1,true,1,7,true]\n");
expect(exitCode).toBe(0);
});
it("bails out on optional-chain index into enum", () => {
const pre = "enum Foo { A }\nenum Bar { 'a-b' = 1 }\n";
const lastLine = out => out.trimEnd().split("\n").at(-1);
Expand Down
Loading