Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
28 changes: 22 additions & 6 deletions src/js_parser/fold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,10 @@
}
}
js_ast::ExprData::EString(str_) => {
if p.options.features.minify_syntax {
if p.options.features.minify_syntax
&& !identifier_opts.is_delete_target()
&& identifier_opts.assign_target() == js_ast::AssignTarget::None
{
// minify "long-string".length to 11
if name == b"length" {
if let Some(len) = e_string_javascript_length(&str_) {
Expand Down Expand Up @@ -493,11 +496,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;
Comment thread
robobun marked this conversation as 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 @@ -511,9 +517,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 Expand Up @@ -666,6 +673,15 @@
}
E::Special::HotEnabled | E::Special::HotDisabled => {
let enabled = p.options.features.hot_module_reloading;
// !enabled rewrites produce values (undefined/{}), so keep the
// reference under delete/assign. enabled rewrites produce hmr.<name>
// refs and must run (HotEnabled prints as throwing `hmr.indirectHot`).
Comment thread
robobun marked this conversation as resolved.
if !enabled
&& (identifier_opts.is_delete_target()
|| identifier_opts.assign_target() != js_ast::AssignTarget::None)
{
return None;
}

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

View check run for this annotation

Claude / Claude Code Review

HotDisabled unknown-property diagnostic lost under delete/assign

The new `!enabled && (is_delete_target || assign_target != None)` guard returns `None` before reaching the fallthrough `else` that emits `add_error_fmt("import.meta.hot.{} does not exist")`, so under HMR-disabled builds `import.meta.hot.typo = fn` and `delete import.meta.hot.typo` no longer produce the build-time diagnostic (they did before this PR; reads and `enabled=true` still error). The emitted output is now *more* runtime-correct (`undefined.typo = fn` vs the old `undefined = fn`), so this

Check notice on line 684 in src/js_parser/fold.rs

View check run for this annotation

Claude / Claude Code Review

method_call_must_be_replaced_with_undefined leaks from non-call import.meta.hot.<method>

Pre-existing (byte-identical before/after this PR): the `!enabled` branches at fold.rs:697 and :731 set `p.method_call_must_be_replaced_with_undefined = true` without gating on `identifier_opts.is_call_target()`. When `import.meta.hot.accept` is *read* in a non-call position (e.g. `globalThis.x = import.meta.hot.accept; foo();`), the flag leaks across statements and `e_call` on the next unrelated call replaces it with `undefined` — `foo()` is silently dropped. The `--drop` setters at visit_expr.
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
if name == b"data" {
return Some(if enabled {
Expr {
Expand Down
10 changes: 8 additions & 2 deletions src/js_parser/visit/visit_expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1057,8 +1057,13 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
let target = e_.target.unwrap_inlined();
let index = e_.index.unwrap_inlined();

// `[x][0] = v` writes into the temporary, not `x`.
if p.options.features.minify_syntax && in_.assign_target == js_ast::AssignTarget::None {
// Folding a property reference to a value is unsafe where the
// reference itself is observed (delete result, assign target; the call
// receiver case is handled inside via `(0, x)`).
Comment thread
robobun marked this conversation as resolved.
if p.options.features.minify_syntax
&& !is_delete_target
&& in_.assign_target == js_ast::AssignTarget::None
{
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 @@ -1224,6 +1229,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
}
}
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
3 changes: 2 additions & 1 deletion src/jsc/RuntimeTranspilerCache.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,8 @@ bun_core::declare_scope!(cache, visible);
/// Version 25: Every ModuleInfo record carries a trailing FetchParameters slot
/// so ImportEntry/ExportEntry/StarExportEntry moduleRequestType matches JSC's
/// after WebKit 90b2ecf79ae3 keyed m_loadedModules on (specifier, type).
const EXPECTED_VERSION: u32 = 25;
/// Version 26: `delete`/assign/call targets no longer fold under minify_syntax.
const EXPECTED_VERSION: u32 = 26;

/// 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" },
});
// `delete (null ?? exports.a)` evaluates to a value, so the result is `true`
// with no effect on the property. Under cjs2esm, `exports.a` is rewritten to
// an `ECommonjsExportIdentifier`; the printer has to re-wrap it as `(0, ...)`
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
69 changes: 69 additions & 0 deletions test/bundler/transpiler/transpiler.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,75 @@ 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 target", () => {
// `a[n]` is a property reference; folding it to a value changes the
// result of `delete`.
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 = delete "foo".length', 'x = delete "foo".length');
// Comma / `?:` / `??` / `||` / `&&` produce a value, not a reference;
// when a fold hoists the live arm up to the delete, the printer
// re-wraps it using WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS.
ts.expectPrinted_("x = delete (true ? a.b : 0)", "x = delete (0, a.b)");
ts.expectPrinted_("x = delete (true ? a : 0)", "x = delete (0, a)");
ts.expectPrintedMin_("x = delete (0, a.b)", "x = delete (0, a.b)");
ts.expectPrintedMin_("x = delete (null ?? a.b)", "x = delete (0, a.b)");
ts.expectPrintedMin_("x = delete (0 || a.b)", "x = delete (0, a.b)");
ts.expectPrintedMin_("x = delete (1 && a.b)", "x = delete (0, a.b)");
Comment thread
robobun marked this conversation as resolved.
// Still inlined outside those positions.
ts.expectPrintedMin_('x = "foo".length', "x = 3");
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;");
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("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");
ts.expectPrinted_("x = delete import.meta.hot.accept", "x = delete undefined.accept");
ts.expectPrinted_("import.meta.hot.accept = fn", "undefined.accept = fn");
// Reads are still inlined.
ts.expectPrinted_("x = import.meta.hot", "x = undefined");
});
it("preserves delete semantics at runtime", async () => {
const src = `
var obj = { p: 1 };
var r1 = delete [obj.p][0];
var k = { f: 7 };
var r2 = delete { f: k.f }.f;
var q = { p: 1 };
delete (0, q.p);
console.log(JSON.stringify([obj.p, r1, k.f, r2, q.p]));
`;
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,7,true,1]\n");
expect(exitCode).toBe(0);
});
it("bails out when the array item is an optional chain", () => {
// Folding `[a?.b][0]` to `a?.b` is unsafe when the result lands as the
// target of a surrounding optional-chain continuation: the two chains
Expand Down