Skip to content
Closed
Show file tree
Hide file tree
Changes from 4 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
14 changes: 9 additions & 5 deletions src/js_parser/fold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -492,11 +492,14 @@
}
}
js_ast::ExprData::EImportMeta(_) => {
if name == b"main" {
let can_inline = !identifier_opts.is_delete_target()
&& identifier_opts.assign_target() == js_ast::AssignTarget::None;

Check warning on line 496 in src/js_parser/fold.rs

View check run for this annotation

Claude / Claude Code Review

Sibling arms in maybe_rewrite_property_access still ignore is_delete_target/assign_target

Two sibling arms in this same `match` still fold a property reference to a value without checking `identifier_opts`: the `EString` arm (`"foo".length` → number) and the `ESpecial::HotEnabled|HotDisabled` arm (`import.meta.hot.<method>` → `undefined`). Under minify, `"foo".length = 5` emits `3 = 5` (invalid JS) and `delete import.meta.hot.accept` still emits `delete undefined` — the exact case this `can_inline` guard was added to prevent one arm above. Both are pre-existing and the triggers are c
Comment thread
robobun marked this conversation as resolved.
Resolved

if can_inline && name == b"main" {
return Some(p.value_for_import_meta_main(false, target.loc));
}

if name == b"hot" {
if can_inline && name == b"hot" {
return Some(Expr {
data: js_ast::ExprData::ESpecial(
if p.options.features.hot_module_reloading {
Expand All @@ -510,9 +513,10 @@
}

// Inline import.meta properties for Bake
if p.options.framework.is_some()
|| (p.options.bundle
&& p.options.output_format == js_parser::options::Format::Cjs)
if can_inline
&& (p.options.framework.is_some()
|| (p.options.bundle
&& p.options.output_format == js_parser::options::Format::Cjs))
{
if name == b"dir" || name == b"dirname" {
// Inline import.meta.dir
Expand Down
9 changes: 8 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,13 @@
let target = e_.target.unwrap_inlined();
let index = e_.index.unwrap_inlined();

if p.options.features.minify_syntax {
// Folding a property reference to a value is unsafe where the
// reference itself is observed (delete result, assign target, call receiver).
Comment thread
robobun marked this conversation as resolved.
Resolved
if p.options.features.minify_syntax
&& !is_delete_target
&& !is_call_target
&& in_.assign_target == js_ast::AssignTarget::None
{

Check warning on line 1066 in src/js_parser/visit/visit_expr.rs

View check run for this annotation

Claude / Claude Code Review

[x][0] fold still fires when the index is a tagged-template tag

The guard now covers delete/call/assign but not tagged-template tags — the fourth position where a property Reference's base is observable via `this` (ECMA-262 §13.3.11 uses `EvaluateCall` with the tag Reference, identical to regular calls). `e_template` visits its tag via bare `p.visit_expr(...)` without setting `p.call_target`, and `p.template_tag` was dropped from the port (only the commented-out stubs at :1044/:1439 remain), so ``[o.m][0]`x` `` folds to ``o.m`x` `` and changes `this` from th
Comment thread
robobun marked this conversation as resolved.
Resolved
Comment thread
robobun marked this conversation as resolved.
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 @@ -1217,7 +1223,8 @@
return;
}
}
Op::UnDelete => {
p.delete_target = e_.value.data;

Check warning on line 1227 in src/js_parser/visit/visit_expr.rs

View check run for this annotation

Claude / Claude Code Review

e_dot dot-define substitution not gated on is_delete_target

The dot-define substitution at `e_dot` (visit_expr.rs:1353) checks only `in_.assign_target == None` and not `!is_delete_target`, so `delete process.env.NODE_ENV` with a string-valued `--define` still emits `delete "development"` — the direct sibling of the `delete FOO` → `delete 1` case the PR description cites and the new `delete user_undefined` test covers. Pre-existing output (unchanged by this PR), but `is_delete_target` is now live at :1343 and the `e_identifier` arm at :263 already carries
Comment thread
robobun marked this conversation as resolved.
Resolved
p.visit_expr_in_out(&mut e_.value, ExprIn::default());

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

View check run for this annotation

Claude / Claude Code Review

UnDelete arm ports p.deleteTarget assignment but not esbuild's post-visit reference-shape check

The `Op::UnDelete` arm now sets `p.delete_target` (matching esbuild's `p.deleteTarget = e.Value.Data`) but omits the paired post-visit check from the same esbuild block — the `canBeDeletedBefore/After` wrap that re-inserts `(0, …)` when a fold turns a *value* operand into a *reference*. So the mirror direction of the class this PR guards at :1060 is still miscompiled: `delete (0, o.p)` / `delete (null ?? o.p)` / `delete (0 || o.p)` / `delete (1 && o.p)` / `delete (true ? o.p : z)` all fold to `d
Comment thread
robobun marked this conversation as resolved.
Resolved
}
_ => {
Expand Down
3 changes: 2 additions & 1 deletion src/jsc/RuntimeTranspilerCache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ 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: `delete`/assign/call targets no longer fold under minify_syntax.
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
68 changes: 68 additions & 0 deletions test/bundler/transpiler/transpiler.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,74 @@ 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/assign/call target", () => {
// `a[n]` is a property reference; folding it to a value changes the
// result of `delete`, the effect of assignment, and the `this`
// binding of a call.
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');
ts.expectPrintedMin_("x = [y.z][0]()", "x = [y.z][0]()");
ts.expectPrintedMin_("x = [y][0]()", "x = [y][0]()");
// 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");
ts.expectPrintedMin_("x = f([y][0])", "x = f(y)");
});
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;");
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.
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("does not inline import.meta.<prop> under delete or assignment", () => {
// Inlining would produce `delete undefined` (strict-mode SyntaxError) or
// `undefined = 5` for the HotDisabled value.
ts.expectPrinted_("x = delete import.meta.hot", "x = delete import.meta.hot");
ts.expectPrinted_("x = delete import.meta.main", "x = delete import.meta.main");
ts.expectPrinted_("import.meta.hot = 5", "import.meta.hot = 5");
ts.expectPrinted_("import.meta.main = 5", "import.meta.main = 5");
// Reads are still inlined.
ts.expectPrinted_("x = import.meta.hot", "x = undefined");
});
it("preserves delete/assign/call-receiver 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;
var o = { m() { return this === o } };
var r3 = [o.m][0]();
console.log(JSON.stringify([obj.p, r1, y, k.f, r2, r3]));
`;
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,false]\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