Skip to content
Merged
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
7 changes: 7 additions & 0 deletions src/js_printer/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1215,7 +1215,14 @@
fn is_identifier_or_numeric_constant_or_property_access(expr: &js_ast::Expr) -> bool {
use js_ast::ExprData;
match &expr.data {
ExprData::EIdentifier(_) | ExprData::EDot(_) | ExprData::EIndex(_) => true,

Check notice on line 1218 in src/js_printer/lib.rs

View check run for this annotation

Claude / Claude Code Review

Pre-existing: react_compiler codegen emits UnDelete with the flag unset → this helper's wrap neuters the delete

Pre-existing (not introduced or widened by this PR): `src/react_compiler/codegen.rs` emits `UnDelete` over an `EDot`/`EIndex` with `flags: E::UnaryFlags::empty()` at both `PropertyDelete` (:1994-2001) and `ComputedDelete` (:2047-2061), so the pre-existing `EDot | EIndex` arm here already re-wraps every `delete obj.prop` inside a React-compiled component into `delete (0, obj.prop)` — a no-op that returns `true`. The fix belongs in `react_compiler/codegen.rs` (set `WAS_ORIGINALLY_DELETE_OF_IDENTIF
Comment thread
robobun marked this conversation as resolved.
// These are produced by the visit pass (namespace-import rewrite, cjs2esm,
// `module.exports`/`import.meta.hot` inlining) and print as a bare identifier
// or a property access. Without the `(0, ...)` wrap the printed `delete` sees
// a Reference and its result/effect differ from the source.
ExprData::EImportIdentifier(_)
| ExprData::ECommonjsExportIdentifier(_)
| ExprData::ESpecial(_) => true,

Check warning on line 1225 in src/js_printer/lib.rs

View check run for this annotation

Claude / Claude Code Review

ESpecial arm has no test coverage

The `ESpecial(_)` arm has no test coverage — deleting `| ExprData::ESpecial(_)` breaks none of the 5 new tests, so per REVIEW.md ("confirm deleting each load-bearing clause of your fix breaks at least one test") it needs one, e.g. `delete (null ?? module.exports)` in a CJS entry. Minor: the comment says these "print as a bare identifier or a property access", but `E::Special::ResolvedSpecifierString` prints a string literal and `E::Special::HotDisabled` prints via `EUndefined` — over-wrapping th
Comment thread
robobun marked this conversation as resolved.
Outdated
ExprData::ENumber(e) => e.value().is_infinite() || e.value().is_nan(),

Check warning on line 1226 in src/js_printer/lib.rs

View check run for this annotation

Claude / Claude Code Review

Same-class siblings not swept: ERequireCallTarget / ERequireMain / ERequireResolveCallTarget / EImportMeta / EInlinedEnum

Same-class siblings still fall through to `_ => false`: `ERequireCallTarget` / `ERequireMain` / `ERequireResolveCallTarget` / `EImportMeta` all print as a bare identifier or property access, and `EInlinedEnum` can wrap an `ENumber(NaN|Infinity)` — so `delete (null ?? require)` bundles to `delete __require` and `const enum E { N = NaN }; delete (null ?? E.N)` bundles to `delete NaN`, both strict-mode SyntaxErrors via the identical fold path this PR fixes. Consider adding those variants alongside
Comment thread
robobun marked this conversation as resolved.
_ => false,
}
Expand Down
31 changes: 31 additions & 0 deletions test/bundler/bundler_cjs2esm.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -392,6 +392,37 @@ describe("bundler", () => {
stdout: '[[{"xyz":456},456],[{"xyz":123},123],[{"xyz":456},456],[{"xyz":123},123]]',
},
});
// `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, ...)`
// so `delete` still sees a value instead of the hoisted binding.
itBundled("cjs2esm/DeleteFoldedExportsPropertyRef", {
files: {
"/entry.js": /* js */ `
exports.a = 1;
console.log(delete (null ?? exports.a), exports.a);
console.log(delete (0, exports.a), exports.a);
`,
},
onAfterBundle: api => {
const code = api.readFile("out.js");
expect(code).not.toMatch(/^[^"]*delete\s+\$a\b/m);
},
run: { stdout: "true 1\ntrue 1" },
});
itBundled("cjs2esm/DeleteFoldedExportsPropertyRefConsumer", {
files: {
"/entry.js": /* js */ `
import { a } from "./lib.js";
console.log(a);
`,
"/lib.js": /* js */ `
exports.a = 1;
console.log(delete (null ?? exports.a), exports.a);
`,
},
run: { stdout: "true 1\n1" },
});
// 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
55 changes: 55 additions & 0 deletions test/bundler/bundler_edgecase.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2858,6 +2858,61 @@ describe("bundler", () => {
expect(out).not.toContain("require_foo\u2014bar");
},
});
// `delete (null ?? ns.x)` evaluates its operand to a value, so the result is
// `true` with no side effect. When bundling rewrites `ns.x` to the local
// binding (EImportIdentifier) after folding `??`, the printer must re-wrap
// the operand so `delete` still sees a value instead of the binding itself.
// Without the wrap the output is `delete x`, a strict-mode SyntaxError.
itBundled("edgecase/DeleteFoldedNamespacePropertyRef", {
files: {
"/entry.js": /* js */ `
import * as ns from "./m.js";
console.log(delete (null ?? ns.x), ns.x);
console.log(delete (0, ns.x), ns.x);
console.log(delete (true ? ns.x : 0), ns.x);
`,
"/m.js": /* js */ `
export let x = 1;
`,
},
onAfterBundle: api => {
const code = api.readFile("out.js");
expect(code).not.toMatch(/delete\s+x\b/);
expect(code).not.toMatch(/delete\s+ns\.x\b/);
},
run: { stdout: "true 1\ntrue 1\ntrue 1" },
});
itBundled("edgecase/DeleteFoldedNamespacePropertyRefMinify", {
files: {
"/entry.js": /* js */ `
import * as ns from "./m.js";
console.log(delete (null ?? ns.x), ns.x);
`,
"/m.js": /* js */ `
export let x = 1;
`,
},
minifySyntax: true,
minifyWhitespace: true,
run: { stdout: "true 1" },
});
// Same path via a direct named import: the identifier becomes an
// EImportIdentifier during the visit pass.
itBundled("edgecase/DeleteFoldedImportedBindingRef", {
files: {
"/entry.js": /* js */ `
import { x } from "./m.js";
console.log(delete (null ?? x), x);
`,
"/m.js": /* js */ `
export let x = 1;
`,
},
onAfterBundle: api => {
expect(api.readFile("out.js")).not.toMatch(/delete\s+x\b/);
},
run: { stdout: "true 1" },
});
});

for (const backend of ["api", "cli"] as const) {
Expand Down
Loading