js_printer: re-wrap delete of folded import/cjs-export identifier

[robobun]

Problem

// m.js: export let x = 1;
import * as ns from "./m.js";
console.log(delete (null ?? ns.x), ns.x);

$ node entry.mjs
true 1
$ bun build ./entry.mjs
// m.js
var x = 1;
// entry.mjs
console.log(delete x, x);
$ node <(bun build ./entry.mjs)
SyntaxError: Delete of an unqualified identifier in strict mode.

delete (null ?? ns.x) evaluates its operand to a value, so per spec it returns true with no effect. The bundler correctly emits delete (0, ns.x) under --no-bundle, but when bundling the ?? folds away and ns.x is rewritten to an EImportIdentifier pointing at the hoisted binding. The printer's (0, ...) re-wrap at src/js_printer/lib.rs:4004 is driven by is_identifier_or_numeric_constant_or_property_access, which only recognised EIdentifier | EDot | EIndex and so let the EImportIdentifier through as a bare delete x.

The same gap applies to every visit-pass rewrite that can land as a delete operand after a fold and prints as an identifier or property access:

node	example	previously printed
EImportIdentifier	delete (null ?? ns.x) (namespace import)	delete x
ECommonjsExportIdentifier	delete (null ?? exports.a) under cjs2esm	delete $a
ESpecial::ModuleExports	delete (null ?? module.exports) under cjs2esm	delete exports_entry
ERequireCallTarget	delete (null ?? require)	delete __require
ERequireMain / ERequireResolveCallTarget	delete (null ?? require.main)	delete __require.main
EInlinedEnum wrapping NaN/Infinity	const enum E { N = 0/0 }; delete (null ?? E.N)	delete NaN
EUndefined	delete (null ?? undefined) (define substitution)	delete undefined
EImportMetaMain	delete (null ?? import.meta.main)	delete import.meta.main / delete __require.main == __require.module
EImportMeta (bake dev / CJS ref)	delete (null ?? import.meta)	delete hmr.importMeta

Each bare-identifier form is a strict-mode SyntaxError in the emitted ESM bundle; the property-access forms delete a property the source never touched.

WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS is set at parse time from the syntactic operand, so it is correctly unset for delete (null ?? ns.x); the wrap just needs to recognise the post-visit node kinds.

Fix

Extend is_identifier_or_numeric_constant_or_property_access to match EImportIdentifier | ECommonjsExportIdentifier | ESpecial | ERequireCallTarget | ERequireResolveCallTarget | ERequireMain | EImportMeta | EImportMetaMain | EUndefined, and recurse through EInlinedEnum so the existing NaN/Infinity check reaches the wrapped value. The helper is only consulted when the parse-time flag is not set, so delete ns.x / delete require (flag set) continue to print without the wrap and this can only add wraps where the source never had Reference semantics.

esbuild's isIdentifierOrNumericConstantOrPropertyAccess has the same EImportIdentifier gap and esbuild --bundle miscompiles the first repro the same way; the other node kinds are either Bun-specific or print differently in esbuild.

This is orthogonal to #36734 (which makes p.delete_target live in the visitor): ns.x inside (null ?? ns.x) is not the delete target there either, so that PR does not cover this path.

Verification

$ bun bd test test/bundler/bundler_edgecase.test.ts -t DeleteFolded   # 8 new tests; fail on main
$ bun bd test test/bundler/bundler_cjs2esm.test.ts -t DeleteFolded    # 3 new tests; fail on main

Full bundler_edgecase.test.ts, bundler_cjs2esm.test.ts, bundler_minify.test.ts, transpiler.test.js, esbuild/default.test.ts, esbuild/ts.test.ts, esbuild/dce.test.ts unchanged.

---

no test proof · iteration 8 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/bundler/bundler_edgecase.test.ts

[coderabbitai]

Walkthrough

Changes

The expression classifier now recognizes additional identifier-like and property-access targets, including recursively evaluated inlined enum values. Bundler tests cover folded delete expressions across CommonJS exports, imports, namespace properties, require references, and enum values.

Folded delete expression handling

Layer / File(s)	Summary
Identifier-like expression classification src/js_printer/lib.rs	The classifier accepts additional import, CommonJS, special-expression, require-related, metadata, undefined, and inlined enum targets.
CommonJS export rewrite coverage test/bundler/bundler_cjs2esm.test.ts	Tests validate runtime behavior and generated wrapping for folded deletes involving exports and module.exports.
Broader folded delete regression coverage test/bundler/bundler_edgecase.test.ts	Tests cover namespace properties, imported bindings, require references, and inlined const enum values in standard and minified output.

Possibly related PRs

• oven-sh/bun#36734: Both address folded delete handling and CommonJS-to-ESM export semantics.
• oven-sh/bun#36741: Both modify delete-expression handling for rewritten or folded references.
• oven-sh/bun#36744: Both cover folded deletes involving inlined enum values such as NaN and Infinity.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title clearly identifies the JavaScript printer change for folded delete expressions involving import and CommonJS export identifiers.
Description check	✅ Passed	The description explains the problem, fix, affected expression kinds, and verification steps with relevant test results.

---

_{Comment @coderabbitai help to get the list of available commands.}

[claude]

LGTM — the inline nits (untested ESpecial arm, additional sibling variants, and the pre-existing react_compiler flag gap) are worth considering but don't affect correctness of what's shipped: the helper's sole call site only ever adds the (0, ...) wrap when the parse-time flag is unset, so the new arms cannot change semantics for any input that was previously correct.

Extended reasoning...

Overview

Adds three match arms (EImportIdentifier | ECommonjsExportIdentifier | ESpecial) to is_identifier_or_numeric_constant_or_property_access in src/js_printer/lib.rs, plus 5 new itBundled tests across bundler_edgecase.test.ts and bundler_cjs2esm.test.ts. Fixes delete (null ?? ns.x) bundling to delete x (a strict-mode SyntaxError) after the visit pass folds ?? and rewrites ns.x to a synthetic identifier node.

Security risks

None. Pure output-correctness fix in the JS printer; no untrusted-input parsing, no allocation, no I/O.

Level of scrutiny

Low. The helper has exactly one call site (lib.rs:4011), guarded by !e.flags.contains(WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS), and returning true only ever emits an extra (0, ...) wrap around a delete operand that was not syntactically a Reference in source. Over-wrapping is semantically a no-op (delete (0, X) → evaluate X, return true), so the change is monotone-safe: it can fix miscompiles but cannot introduce new ones for parser-produced ASTs. The PR description's reasoning matches the code.

Other factors

The 5 new tests all use run: { stdout: ... } (not just output regex), so they verify runtime semantics, and 3 of them additionally assert the miscompiled shape is absent. Full suites (bundler_edgecase, bundler_cjs2esm, bundler_minify, transpiler, esbuild/default, esbuild/dce) reported passing. The three inline findings are all non-blocking: two are nits (uncovered ESpecial arm, additional sibling node kinds for even-more-pathological inputs like delete (null ?? require)), and one is a pre-existing react_compiler/codegen.rs bug that this PR neither introduces nor widens.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/js_printer/lib.rs`:
- Around line 1219-1227: Add a printer or transpiler regression test covering
`delete (null ?? import.meta)`, ensuring the emitted output contains the `(0,
import.meta)` wrapper. Keep the test focused on preserving the `EImportMeta`
printing path, which bundled tests do not exercise.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e7e8b065-fea5-4678-8419-6313dfa15409

📥 Commits

Reviewing files that changed from the base of the PR and between f91d5c9 and 103ee5a.

📒 Files selected for processing (3)

• src/js_printer/lib.rs
• test/bundler/bundler_cjs2esm.test.ts
• test/bundler/bundler_edgecase.test.ts

[claude]

Additional findings (outside current diff — PR may have been updated during review):

• 🟡 src/js_printer/lib.rs:1225-1232 — Two more same-class siblings still fall through to _ => false after 14d0f4b: EImportMetaMain and EUndefined. delete (null ?? import.meta.main) bundles to delete import.meta.main (deletes the property; under target: node it emits delete __require.main == __require.module, which deletes __require.main), and delete (null ?? undefined) bundles to delete undefined (strict-mode SyntaxError) because the visit pass substitutes unbound undefined → EUndefined and print_undefined emits the bare identifier without minify. Adding | ExprData::EImportMetaMain(_) | ExprData::EUndefined(_) to the arm covers both.

  Extended reasoning...

  What's missing

  The follow-up commit 14d0f4b extended the arm at src/js_printer/lib.rs:1225-1231 to cover ERequireCallTarget | ERequireResolveCallTarget | ERequireMain | EImportMeta and added EInlinedEnum recursion, but two more visit-pass-produced discriminants still hit _ => false at line 1234:

  • EImportMetaMain — a distinct discriminant from EImportMeta (src/ast/expr.rs:1491). import.meta.main is rewritten by maybe_rewrite_property_access (fold.rs:494-496 → p.value_for_import_meta_main()) on every EDot{target: EImportMeta, name: "main"}, not gated by is_delete_target. The existing test edgecase/ImportMetaMain proves this node reaches the printer for entry files (it captures the literal string import.meta.main).
  • EUndefined — the visit pass substitutes an unbound undefined identifier via defines.for_identifier(b"undefined") → PureGlobalIdentifierValue::StrictUndefined → ExprData::EUndefined (defines_table.rs:232-243, visit_expr.rs:261-282). The pre-existing ENumber arm at line 1233 already handles the sibling define-substitutions for NaN/Infinity from the same table; EUndefined is the one member of that trio the helper doesn't catch.

  Step-by-step: EUndefined

  1. Source: console.log(delete (null ?? undefined)) in an ESM entry.
  2. Parse: operand is EBinary(??), so WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS is not set (parse_prefix.rs only sets it for EIdentifier|EDot|EIndex).
  3. Visit RHS: undefined is unbound and is_delete_target is false (the delete target is the EBinary, not this identifier — visit_expr.rs:177 checks matches!(p.delete_target.tag(), Tag::EIdentifier)), so define substitution runs and *e = EUndefined.
  4. null ?? EUndefined folds to EUndefined at visit_binary.rs:363-383 (unconditional, not gated on minify_syntax — the PR's own non-minified DeleteFoldedNamespacePropertyRef test relies on this fold).
  5. Printer at lib.rs:4018: flag unset, is_identifier_or_numeric_constant_or_property_access(EUndefined) → _ => false → no (0, ...) wrap.
  6. print_undefined (lib.rs:1818-1832) at level Prefix.sub(1) without minify_syntax prints bare undefined.

  Output: delete undefined in module code (always strict) — an early SyntaxError (undefined is an IdentifierReference, not a keyword), the identical failure mode this PR fixes for EImportIdentifier. Source semantics is true with no effect. With minify_syntax, print_undefined emits void 0 and delete void 0 is fine, so this only miscompiles in the non-minified path — consistent with the PR's own non-minified test coverage.

  Step-by-step: EImportMetaMain

  Same trace with delete (null ?? import.meta.main): parse-time operand is EBinary, visit rewrites import.meta.main → EImportMetaMain, ?? folds, helper returns false, no wrap.

  Printer output (lib.rs:2869-2919):

  • ESM / non-node (default): prints import.meta.main — a property access. delete import.meta.main attempts to delete the property from import.meta (behavior change; source evaluates to true with no effect).
  • target: node: prints __require.main == __require.module with no level-based paren wrapping in that arm. delete binds tighter than ==, so delete __require.main == __require.module parses as (delete __require.main) == __require.module — deleting __require.main and comparing the boolean. The existing test edgecase/ImportMetaMainTargetNode captures exactly this __require.main == __require.module string, proving the node target reaches this printer arm.

  Why the existing arms don't catch it

  EImportMeta(_) (line 1231) does not match EImportMetaMain(_) — they are separate ExprData variants. The prior review comment enumerated EImportMeta but not EImportMetaMain, so both reviewer and author missed it. The ESpecial(_) arm covers E::Special::HotDisabled (which prints via print_undefined), but a direct EUndefined node reaches the wildcard.

  Fix

  Add | ExprData::EImportMetaMain(_) | ExprData::EUndefined(_) to the arm at line 1231. Over-wrapping delete (0, void 0) under minify_syntax is harmless per the comment at lines 1223-1224, so no gate is needed. Sibling tests:

  itBundled("edgecase/DeleteFoldedUndefinedRef", {
    files: { "/entry.js": `console.log(delete (null ?? undefined));` },
    onAfterBundle: api => expect(api.readFile("out.js")).not.toMatch(/delete\s+undefined\b/),
    run: { stdout: "true" },
  });
  itBundled("edgecase/DeleteFoldedImportMetaMainRef", {
    files: { "/entry.js": `console.log(delete (null ?? import.meta.main));` },
    onAfterBundle: api => expect(api.readFile("out.js")).not.toMatch(/delete\s+import\.meta\.main\b/),
    run: { stdout: "true" },
  });

  Per REVIEW.md ("Fix the whole class in the same PR — same-class sites are one concern"), these belong here since 14d0f4b's stated purpose was to cover exactly this class of siblings. Both are pre-existing on main (as was everything 14d0f4b addressed) but live in the exact match arm this PR extends.

• 🟡 src/js_printer/lib.rs:1231 — The EImportMeta(_) arm added in 14d0f4b has no test — deleting | ExprData::EImportMeta(_) breaks none of the 8 new tests (DeleteFoldedRequireRefs covers ERequire*, DeleteFoldedInlinedConstEnumNaN covers EInlinedEnum, DeleteFoldedModuleExportsRef covers ESpecial). Per REVIEW.md ("confirm deleting each load-bearing clause of your fix breaks at least one test") it needs one — an output-shape check under --format=cjs (where import.meta prints via import_meta_ref as a bare symbol) or Format::InternalBakeDev (where it prints as <hmr>.importMeta), mirroring the DeleteFoldedModuleExportsRef approach.

  Extended reasoning...

  What's missing

  Commit 14d0f4b extended the match at src/js_printer/lib.rs:1225-1231 with four new variants alongside the original three, and added tests for three of them:

  Arm	Test
  ERequireCallTarget / ERequireResolveCallTarget / ERequireMain	edgecase/DeleteFoldedRequireRefs
  EInlinedEnum	edgecase/DeleteFoldedInlinedConstEnumNaN
  ESpecial	cjs2esm/DeleteFoldedModuleExportsRef (added after the prior review round)
  EImportMeta	— none —

  Grepping the 8 new DeleteFolded* tests for import.meta finds nothing; the only import.meta hits in bundler_edgecase.test.ts are the pre-existing ImportMetaMain* tests, which don't involve delete. Removing | ExprData::EImportMeta(_) from line 1231 leaves every test in this PR green.

  Why the arm is load-bearing

  At src/js_printer/lib.rs:2843-2867, EImportMeta prints in one of three ways:

  1. Format::InternalBakeDev → <hmr_ref>.importMeta — a property access. Without the (0, ...) wrap, delete (null ?? import.meta) would emit delete hmr.importMeta, actually deleting the runtime's importMeta property instead of returning true with no effect.
  2. CJS import_meta_ref path (line 2866) → print_symbol(import_meta_ref) — a bare identifier. delete <ident> in module/strict code is an early SyntaxError, exactly the failure mode this PR fixes for EImportIdentifier.
  3. default → literal import.meta — also a MemberExpression; delete import.meta is a runtime no-op returning true, so this path happens to be benign, but it doesn't cover the other two.

  So the arm is not defensive padding; in at least two output modes the printed form is a Reference that delete would act on incorrectly.

  Why this is being raised

  REVIEW.md's test rubric is explicit: "Confirm deleting each load-bearing clause of your fix breaks at least one test — a test that passes both ways is worse than no test." This exact rule was applied in the prior review round to the ESpecial(_) arm, and the author complied by adding cjs2esm/DeleteFoldedModuleExportsRef as an output-shape-only check (because a runtime check wasn't feasible there). EImportMeta was added in the same follow-up commit and is subject to the same standard.

  Step-by-step proof

  1. Source: console.log(delete (null ?? import.meta)) in a CJS entry with --format=cjs (or under the bake dev-server format).
  2. Parser: syntactic operand is EBinary(??), so WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS is not set.
  3. Visit pass folds null ?? import.meta → EImportMeta (unconditional ?? fold, same as every other test in this PR).
  4. Printer at lib.rs:4011: parse-time flag unset → consults is_identifier_or_numeric_constant_or_property_access(&e.value).
  5. With the EImportMeta(_) arm: returns true → emits delete (0, <import_meta_ref>) / delete (0, hmr.importMeta) → runtime evaluates to true, no side effect (correct).
  6. Without the arm: falls through to _ => false → emits delete <import_meta_ref> (strict-mode SyntaxError) or delete hmr.importMeta (deletes the property).

  No test in this PR reaches step 3 with an EImportMeta, so step 6 goes unobserved.

  Suggested test

  An output-shape check mirroring DeleteFoldedModuleExportsRef is the lightest option — e.g. a --format=cjs bundle asserting delete (0, <import_meta_ref>) in the output and not.toMatch(/delete\s+import_meta\b/), or a test/bake/ dev-server case asserting the wrap around hmr.importMeta. Either would fail if | ExprData::EImportMeta(_) is removed.

[robobun]

Addressed in 75fb43f: added EImportMetaMain and EUndefined to the match with three new tests (edgecase/DeleteFoldedUndefinedRef, edgecase/DeleteFoldedImportMetaMainRef, edgecase/DeleteFoldedImportMetaMainRefNode), all fail on main.

That completes the ExprData sweep: the remaining discriminants either print as a value literal / expression (never an IdentifierReference or member access), are already covered by EIdentifier | EDot | EIndex | ENumber, or cannot appear as a standalone delete operand.

On EImportMeta coverage: same reasoning as the withdrawn comment above. In every mode reachable from itBundled/Bun.Transpiler it prints the literal import.meta MetaProperty, where delete import.meta and delete (0, import.meta) are both true with no effect; the Reference-printing modes (InternalBakeDev -> hmr.importMeta, runtime CJS import_meta_ref) are not practically reachable from the test harness. Keeping it as a defensive match for those two modes.

[claude]

LGTM — the two inline notes are pre-existing adjacent issues (print-time cross-module enum inlining under the flag-set path, and the EImportMetaMain node-target level handling), neither introduced nor widened here; both belong in separate PRs as noted.

Extended reasoning...

Overview

Extends is_identifier_or_numeric_constant_or_property_access (src/js_printer/lib.rs:1215) to recognise nine additional post-visit ExprData variants and to recurse through EInlinedEnum, so the printer's delete (0, …) re-wrap at :4015 fires when a fold like null ?? ns.x collapses onto a synthetic identifier/property-access node. Eleven new itBundled tests across bundler_edgecase.test.ts and bundler_cjs2esm.test.ts, each a fail-before/pass-after runtime or output-shape assertion.

Security risks

None. Pure printer-output correctness in a total helper with no I/O, allocation, or user-controlled parsing.

Level of scrutiny

Medium — bundler codegen correctness. The helper has exactly one call site, gated by !WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS, so every new arm can only add a (0, …) wrap where the source operand was not syntactically an identifier/dot/index. Over-wrapping is semantics-preserving (delete (0, <value>) → true); under-wrapping was the bug. The one known producer that violates the flag invariant (react_compiler/codegen.rs emitting UnDelete with empty flags over EDot/EIndex) is pre-existing on the untouched EDot | EIndex arm and was already handed off.

Other factors

This has been through two prior review rounds; all blocking feedback (missing ERequire*/EImportMeta/EInlinedEnum siblings, missing ESpecial test coverage, comment length, missing EImportMetaMain/EUndefined) was addressed across 14d0f4b → 75fb43f. The author's ExprData sweep and the coderabbit EImportMeta reasoning check out against the print arms. The two remaining inline findings this run are pre-existing bugs in code paths this diff does not touch (the flag-set EDot print-time enum-inlining branch, and the EImportMetaMain else-branch precedence handling) — flagged for tracking, not blocking.

[claude]

LGTM — monotone extension of the delete re-wrap classifier; can only add (0, ...) wraps on the flag-unset path, never remove them.

What was reviewed:

• Sole call site at lib.rs:4015 confirms the helper is gated on !WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS, so over-matching (e.g. ESpecial::ResolvedSpecifierString, EImportMeta in ESM) is a harmless extra wrap, never a semantic change.
• EInlinedEnum recursion is bounded (wraps a leaf number/string) and correctly reaches the existing ENumber NaN/Infinity check.
• 11 new itBundled tests cover each added arm with runtime + output-shape assertions; all prior review threads resolved, remaining findings are pre-existing and handed off.

Extended reasoning...

Overview

Extends is_identifier_or_numeric_constant_or_property_access in src/js_printer/lib.rs with nine additional ExprData variants (EImportIdentifier, ECommonjsExportIdentifier, ESpecial, ERequireCallTarget, ERequireResolveCallTarget, ERequireMain, EImportMeta, EImportMetaMain, EUndefined) plus recursion through EInlinedEnum. Adds 8 tests to bundler_edgecase.test.ts and 3 to bundler_cjs2esm.test.ts. Net +11 lines Rust, ~180 lines of tests.

Security risks

None. Pure printer output-shape change in the bundler; no user input parsing, no allocation, no FFI, no auth/crypto surface.

Level of scrutiny

Low-medium. The change is a match-arm extension with a strong safety invariant: the helper is only consulted at lib.rs:4015 when the parse-time WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS flag is unset — i.e., the source's delete operand was syntactically not a Reference. Returning true for additional node kinds can therefore only add a (0, ...) wrap, converting the printed operand from a possible Reference back to a value, which is exactly what the source semantics require. There is no path by which this change removes a wrap or turns a value-delete into a Reference-delete. I checked that variants which don't strictly print as an identifier/property-access (ESpecial::ResolvedSpecifierString → string literal, ESpecial::HotDisabled/EUndefined → void 0 when minified, EImportMeta → MetaProperty in ESM, EImportMetaMain → equality expr under node target) are all safe to over-wrap: delete (0, <expr>) is semantically identical to delete <non-Reference-expr>.

Other factors

• All seven review threads are resolved. Earlier rounds drove out missing test coverage (ESpecial, EImportMetaMain, EUndefined) and the comment-length bot's complaints; three findings were explicitly classified as pre-existing (react_compiler codegen missing the flag, print-time cross-module enum inlining on the flag-set path, EImportMetaMain else-branch missing a level-based paren wrap) and handed off — none are introduced or widened here.
• Test coverage is thorough: each added arm has a dedicated test that fails on main (runtime SyntaxError or wrong property delete) and passes with the fix, plus negative output-shape regexes. The one output-shape-only test (cjs2esm/DeleteFoldedModuleExportsRef) documents why it can't run the bundle (a separate pre-existing cjs2esm issue).
• The EInlinedEnum recursion cannot loop: inlined-enum values are leaf ENumber/EString nodes.
• esbuild has the same EImportIdentifier gap per the PR description, so this is a genuine correctness improvement over the reference implementation, not a divergence.

[robobun]

The diff is ready. Buildkite 87515 finished: 177 lanes passed including every bundler lane with the 11 new DeleteFolded tests; 0 failed test jobs. The red status is 17 lanes canceled + 2 expired on EC2 agent-provisioning errors (Failed to create agent ... ec2.us-west-1.amazonaws.com 400 Unsupported, marked [pre-existing] i.e. also on main) plus 8 [flaky] annotations on unrelated tests (node-module-module, fetch-leak, cron --hot, tty-reopen, request-clone-leak, test-error-code-done-callback, child_process, inspect-error-leak), all of which passed on retry/alone and none of which touch the bundler or printer.

Local verification: all 11 new tests fail on main and pass with the fix; full bundler_edgecase/bundler_cjs2esm/bundler_minify/transpiler/esbuild/default/esbuild/ts/esbuild/dce suites pass. Gate passed on every iteration. Ready to merge.

[robobun]

^{Updated 5:00 PM PT - Aug 3rd, 2026}

✅ @robobun, your commit fa5ee8d8322a037d58597e03fe4f000e4c6d0919 passed in Build #88339! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 36740

That installs a local version of the PR into your bun-36740 executable, so you can run:

bun-36740 --bun

[Jarred-Sumner]

@robobun conflicts

[robobun]

Merged main and resolved the bundler_edgecase.test.ts conflict (both sides added tests before the closing });; kept both). All 11 DeleteFolded tests still fail on the released binary and pass on the rebuilt debug build.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/js_printer/lib.rs (1)

297-307: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Deduplicate entries after rename_key.

replace_string_id can replace old_id with a new_id that already exists. This can leave duplicate (specifier, type, phase) entries in the parallel vectors. Rebuilding index only overwrites the map entry and still serializes both records.

Compact the vectors during the rebuild and retain the first entry to preserve insertion order. Add a regression test where two distinct IDs converge on one ID.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/js_printer/lib.rs` around lines 297 - 307, Update the rebuild logic in
the touched branch of the relevant printer method to deduplicate the parallel
keys, values, and phases vectors by `(specifier, type, phase)` while rebuilding
`self.index`, retaining the first occurrence to preserve insertion order. Ensure
`replace_string_id`/`rename_key` convergence removes later duplicate records
before serialization, and add a regression test covering two distinct IDs
replaced by the same ID.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@src/js_printer/lib.rs`:
- Around line 297-307: Update the rebuild logic in the touched branch of the
relevant printer method to deduplicate the parallel keys, values, and phases
vectors by `(specifier, type, phase)` while rebuilding `self.index`, retaining
the first occurrence to preserve insertion order. Ensure
`replace_string_id`/`rename_key` convergence removes later duplicate records
before serialization, and add a regression test covering two distinct IDs
replaced by the same ID.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 521d7565-ddbe-455a-bf9f-935acd8f719c

📥 Commits

Reviewing files that changed from the base of the PR and between 103ee5a and fa5ee8d.

📒 Files selected for processing (1)

• src/js_printer/lib.rs

[robobun]

@coderabbitai the rename_key deduplication finding at src/js_printer/lib.rs:297-307 is outside this PR's diff (pre-existing code on main, unchanged here; this PR only touches is_identifier_or_numeric_constant_or_property_access at lines 1285-1302). Out of scope.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@robobun, confirmed. The rename_key code is pre-existing and outside this PR’s diff. I am withdrawing that finding.

---

✏️ Learnings added

Learnt from: robobun
URL: https://github.com/oven-sh/bun/pull/36740

Timestamp: 2026-08-03T22:23:04.449Z
Learning: In `src/js_printer/lib.rs`, the `rename_key` duplication around lines 297-307 is pre-existing on `main` and is unrelated to PR `#36740`. PR `#36740` changes `is_identifier_or_numeric_constant_or_property_access` around lines 1285-1302.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

_{You are interacting with an AI system.}

[claude]

LGTM — conflicts are resolved (fa5ee8d merges main into bundler_edgecase.test.ts only) and the substantive change is unchanged since the last green CI run.

Checked that is_identifier_or_numeric_constant_or_property_access still has exactly one call site, gated on !WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS, so the new arms can only add (0, ...) wraps where the source's delete operand was never a Reference — over-wrap is a no-op true, never a semantics change. Swept the added ExprData variants against their printer arms; each prints as an identifier/member-access in at least one mode and none regress the flag-set path. The 11 new itBundled tests each pair a runtime assertion with a negative output-shape regex; ESpecial::ModuleExports is output-shape-only for a documented pre-existing reason. The three pre-existing siblings flagged earlier (react-compiler UnaryFlags::empty(), print-time cross-module enum delete NaN, EImportMetaMain node-target level-wrap) live on paths this helper cannot see and were handed off separately.

Extended reasoning...

Overview

An 11-line addition to is_identifier_or_numeric_constant_or_property_access in src/js_printer/lib.rs, plus 11 new itBundled tests across test/bundler/bundler_edgecase.test.ts (8) and test/bundler/bundler_cjs2esm.test.ts (3). The helper now recognises the visit-pass rewrite node kinds (EImportIdentifier, ECommonjsExportIdentifier, ESpecial, ERequire*, EImportMeta/EImportMetaMain, EUndefined) and recurses through EInlinedEnum, so a folded delete (null ?? ns.x) re-wraps as delete (0, x) instead of emitting delete x (a strict-mode SyntaxError in the ESM bundle).

Security risks

None. This is a pure code-printer correctness fix on the bundler output path; no untrusted-input parsing, no auth/crypto/permissions, no memory management.

Level of scrutiny

Medium — the printer is production-critical, but the change is narrowly scoped: the helper's single call site (lib.rs:4085) is guarded by e.op == UnDelete && !e.flags.contains(WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS), so the flag-set path (delete ns.x, delete require) is untouched and the change can only add (0, ...) wraps where the source operand was already a value expression. An over-wrap on any of the new arms is semantically a no-op (delete <value> → true), so the failure mode is bounded to a few extra output bytes, never a behaviour change. The EInlinedEnum recursion is bounded by AST depth (no self-nesting).

Other factors

• All prior review threads are resolved: my earlier EImportMetaMain/EUndefined gap was fixed in 75fb43f; the comment-cop nits were trimmed; CodeRabbit withdrew its EImportMeta test request; the three 🟣 pre-existing siblings I flagged (react-compiler codegen, print-time cross-module enum inlining, EImportMetaMain node-target level-wrap) live on paths this helper never sees and were correctly deferred to separate PRs.
• Test coverage is thorough: each new variant has a dedicated test with both a negative output regex and a runtime stdout check (except DeleteFoldedModuleExportsRef, which is output-shape-only for a documented pre-existing reason). The PR description confirms fail-before/pass-after for all 11.
• CI: build 87515 passed all bundler lanes on the pre-merge head; the only activity since is fa5ee8d, a mechanical merge of main resolving a bundler_edgecase.test.ts add/add conflict — no source changes. Jarred's only comment ("conflicts") has been addressed by that merge.
• esbuild parity: the PR description notes esbuild has the same EImportIdentifier gap, so this is a strict improvement over the reference implementation for the shared case.