Skip to content
Open
Show file tree
Hide file tree
Changes from 7 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
31 changes: 25 additions & 6 deletions src/js_parser/fold.rs
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,10 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
}
}
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 @@ -492,11 +495,14 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
}
}
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 @@ -510,9 +516,10 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
}

// 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 @@ -665,6 +672,18 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
}
E::Special::HotEnabled | E::Special::HotDisabled => {
let enabled = p.options.features.hot_module_reloading;
// The !enabled rewrites below produce `undefined` / `{}`;
// keep the property reference under delete/assign so
// `delete import.meta.hot.accept` stays a reference
// instead of `delete undefined`. The enabled rewrites
// all produce `hmr.<name>` references and must run so
// the printer doesn't fall back to `hmr.indirectHot`.
Comment thread
robobun marked this conversation as resolved.
Outdated
if !enabled
&& (identifier_opts.is_delete_target()
|| identifier_opts.assign_target() != js_ast::AssignTarget::None)
{
return None;
}
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
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 @@ 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();

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.
Outdated
if p.options.features.minify_syntax
&& !is_delete_target
&& !is_call_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 @@ -1218,6 +1224,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 @@ -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
83 changes: 83 additions & 0 deletions test/bundler/transpiler/transpiler.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,89 @@
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 = delete "foo".length', 'x = delete "foo".length');
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]()");
// 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)");

Check notice on line 166 in test/bundler/transpiler/transpiler.test.js

View check run for this annotation

Claude / Claude Code Review

Printer delete-wrap safety net misses EImportIdentifier/ECommonjsExportIdentifier

Pre-existing: the print-time `(0, …)` re-wrap these tests lock in (`is_identifier_or_numeric_constant_or_property_access` at src/js_printer/lib.rs:1215) matches only `EIdentifier|EDot|EIndex|ENumber(inf/nan)` — not `EImportIdentifier` or `ECommonjsExportIdentifier`, which are exactly what bun's own visitor produces from `ns.x` (import-namespace rewrite) and `exports.a` (cjs2esm). So under `bun build`, `delete (null ?? ns.x)` folds the `??` to an `EImportIdentifier`, the printer skips the wrap, a
Comment thread
robobun marked this conversation as resolved.
// Still inlined outside those positions.
ts.expectPrintedMin_("x = [y][0]", "x = y");
ts.expectPrintedMin_('x = "foo"[2]', 'x = "o"');
ts.expectPrintedMin_('x = "foo".length', "x = 3");
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.
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/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]();
var q = { p: 1 };
delete (0, q.p);
console.log(JSON.stringify([obj.p, r1, y, k.f, r2, r3, 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,1,7,true,false,1]\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