js_parser: set p.delete_target before visiting the delete operand - #36734
js_parser: set p.delete_target before visiting the delete operand#36734robobun wants to merge 11 commits into
Conversation
The UnDelete visit arm never wrote p.delete_target, so every
is_delete_target check derived from it (in e_identifier, e_dot,
e_index, e_import_meta, handle_identifier, and
maybe_rewrite_property_access) was permanently false. The port carried
the field and all the guards over from esbuild but dropped the one
assignment that makes them meaningful.
Observable on main with minify_syntax (default for `bun run`):
delete [obj.p][0] -> folded to `delete obj.p` (runtime wrong)
delete {f: x}.f -> folded to `delete x`
delete E.A (TS enum) -> folded to `delete 1`
delete ns.prop (bundle) -> no "Cannot assign to import" error
delete exports.foo -> CJS->ESM unwrap not deoptimized
delete DEFINED (--define) -> substituted to `delete <value>`
The bun-specific "foo"[n] and [x][n] folds in e_index also ran with no
delete/assign-target guard at all, so the first case above stayed
broken even with delete_target wired. Those now bail when the index
expression is a delete target or an assignment target (`[y][0] = 5`
previously folded to `y = 5`).
RuntimeTranspilerCache version bumped since the runtime transpiler
enables minify_syntax.
WalkthroughChangesProperty-access minification now preserves delete, assignment, increment, and call-target semantics. Import.meta handling and the transpiler cache version are updated. Regression tests cover transpilation, CommonJS conversion, and namespace import deletion. Delete semantics
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
[o.m][0]() has this = the temporary array; folding to o.m() makes this = o. Add !is_call_target to the same guard (delete/assign/call are the three positions where a property reference behaves differently from its value) and cover it in the existing tests.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@test/bundler/transpiler/transpiler.test.js`:
- Around line 163-171: Add the missing ts.parsedMin assertion in the “does not
inline an enum member under delete” test for delete E["A"], expecting the
minified output to remain delete E["A"]; and preserve the existing parsed and
read-access assertions.
🪄 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: f16d8a30-7db6-4d52-aed2-f1c096ee7631
📒 Files selected for processing (5)
src/js_parser/visit/visit_expr.rssrc/jsc/RuntimeTranspilerCache.rstest/bundler/bundler_cjs2esm.test.tstest/bundler/transpiler/assign-to-import.test.tstest/bundler/transpiler/transpiler.test.js
The EImportMeta arm in maybe_rewrite_property_access produced values (undefined, require.main == module, string literals) where a property reference is required. `delete import.meta.hot` became `delete undefined` (strict-mode SyntaxError) and `delete import.meta.main` under --format=cjs became `(delete require.main) == module`. Gate the inline branches on !is_delete_target && assign_target == None and fall through to the plain E::Dot return, matching the neighbouring EObject/enum arms. Also add the parsedMin assertion for `delete E["A"]` suggested in review.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js_parser/visit/visit_expr.rs:1230-1233— TheOp::UnDeletearm now setsp.delete_target(mirroring esbuild'sp.deleteTarget = e.Value.Data) but omits the rest of that same esbuild arm: thecanBeDeletedBefore/canBeDeletedAftercheck that wraps the operand in(0, x)when visiting turns a non-Reference into a Reference. Becausee_iffolds*e = e_.yesat line 1514 with nois_delete_targetguard and nominify_syntaxgate,delete (true ? a.b : 0)becomesdelete a.bunder plainbun run— Node prints1, Bun printsundefined. Per REVIEW.md ("for ported code, the reference implementation is the spec — diff control flow against it" / "fix the whole class"), either port the before/after wrap here or addis_delete_targethandling ine_if; the cache version was already bumped for this class, so deferring means another bump.Extended reasoning...
What the bug is
The PR ports one line of esbuild's
case js_ast.UnOpDelete:—p.deleteTarget = e.Value.Data— into theOp::UnDeletearm at visit_expr.rs:1231. But esbuild's arm does more: it capturescanBeDeletedBefore := canBeDeleted(e.Value)before visiting,canBeDeletedAfterafter, and if visiting turned a non-Reference operand into a Reference (canBeDeletedAfter && !canBeDeletedBefore), wraps it as(0, x)sodeletestill sees a value. Bun's arm has none of that — it is exactly two lines — and nocan_be_deletedhelper exists anywhere insrc/(grep confirms).The newly-activated
is_delete_targetguards ine_dot/e_index/e_identifier/etc. cover folds of the direct operand, but they cannot help when a fold replaces the whole operand node with a different kind.e_if(lines 1470–1544) does exactly that: it computes onlyis_call_target(line 1472), never readsp.delete_target, and for a compile-time-constant test does*e = e_.yesat line 1514 (or*e = e_.noat 1541) with only anis_call_target && has_value_for_this_in_call()guard at line 1507. There is nominify_syntaxgate on this fold.Step-by-step proof
var a = { b: 1 }; delete (true ? a.b : 0); console.log(a.b);
Op::UnDeletesetsp.delete_target = <EIf node>and visits the operand.e_ifcomputesis_call_target = false(the EIf is notp.call_target). It never checksp.delete_target.SideEffects::to_booleanon the literaltruereturnsok=true, value=true, NoSideEffects, so control reaches line 1507.is_call_targetis false, so line 1514 runs:*e = e_.yes— the operand becomes theEDotnodea.b.- Back in
Op::UnDelete, there is no post-visit check, so the emitted code isdelete a.b.
Spec: Per ECMA-262 §13.14.3, ConditionalExpression applies GetValue to the selected branch, so
(true ? a.b : 0)evaluates to the value1, not a Reference;delete <value>returnstrueand deletes nothing. Node prints1. After the fold,delete a.bactually deletes the property; Bun printsundefined.Why nothing else prevents it
e_ifnever readsp.delete_target, so wiring it up in this PR did nothing for this path.- The fold is not gated on
minify_syntax, so it fires under plainbun run, not justbun build --minify. - The
(0, x)wrap machinery already exists for the call-target case at line 1507 (has_value_for_this_in_call), demonstrating the codebase models this exact hazard for calls but omits it for delete. - esbuild's general fix — the before/after check in the
UnOpDeletearm itself — is what covers any fold that changes the operand's Reference-ness, without instrumenting every fold site. That is the half of the arm this PR did not port.
Why this belongs in this PR
This is pre-existing (the
e_iffold predates the PR), but:- The PR description says it is "mirroring esbuild's
p.deleteTarget = e.Value.Data", and the wrap lives in that same esbuild arm. REVIEW.md: "For ported code, the reference implementation (esbuild, Node) is the spec — diff control flow against it before 'fixing' apparent bugs." - It is the same bug class the PR is fixing ("replacing a property reference with a value under
delete", inverted: replacing a value-producing form with a reference). REVIEW.md: "Fix the whole class in the same PR (same-class sites are ONE concern, not scope creep)." - The PR already accepted this exact reasoning once: the earlier review comment about
!is_call_targetat the[x][0]fold was applied in commit ab3843d on the same "fix the whole class" basis. RuntimeTranspilerCacheversion was bumped to 25 for this class of change. If the wrap is added in a later PR, the version must be bumped again, and any cache entry written between the two PRs persists the miscompile.
Fix
Either port esbuild's
canBeDeletedBefore/canBeDeletedAftercheck into theOp::UnDeletearm (the general fix — a smallcan_be_deletedhelper matchingEDot/EIndex/EIdentifier, and ifafter && !beforewrap the visited operand as(0, operand)), or add anis_delete_targetcomputation ine_ifand gate the*e = e_.yes/*e = e_.nofolds the same wayis_call_targetis gated. The former is preferable because it also covers any other fold that changes operand kind (e.g. comma-operator simplification). Add a test alongside the new delete cases:ts.expectPrintedMin_("x = delete (true ? a.b : c)", "x = delete (true ? a.b : c)"); // or, if the wrap is ported: "x = delete (0, a.b)"
and a runtime case in the "preserves delete/assign/call-receiver semantics at runtime" test.
-
🟡
src/js_parser/visit/visit_expr.rs:1060-1070— The sibling bun-specific fold"str".length → <number>inmaybe_rewrite_property_access(fold.rs:445-453,EStringarm) has the same reference→value hazard but nois_delete_target/assign_targetguard —delete "abc".lengthstill folds todelete 3(strict-mode TypeError →true, sloppyfalse→true). The adjacentEObjectarm (fold.rs:470-473) already checks all three, and this PR now deliversis_delete_target=trueto theEStringarm via.with_is_delete_target(is_delete_target)at line 1442; it just never reads it. One-line guard +ts.expectPrintedMin_('x = delete "foo".length', 'x = delete "foo".length')alongside the newdelete "foo"[2]case.Extended reasoning...
What the bug is
The PR guards the bun-specific
"foo"[n] → "o"and[x][0] → xfolds against delete/assign/call targets (visit_expr.rs:1064-1070) on the grounds that "a[n]is a property reference; replacing it with a value changes the result ofdelete". But the sibling bun-specific fold"str".length → <number>inmaybe_rewrite_property_access(src/js_parser/fold.rs:445-453,EStringarm) is the same shape — a property reference on a string literal replaced by a value — and has no such guard:js_ast::ExprData::EString(str_) => { if p.options.features.minify_syntax { // minify "long-string".length to 11 if name == b"length" { if let Some(len) = e_string_javascript_length(&str_) { return Some(p.new_expr(E::Number::new(len as f64), loc)); } } } }
identifier_opts.is_delete_target()and.assign_target()are never consulted, even though the immediately adjacentEObjectarm (fold.rs:470-473) checks all three (!is_delete_target(),assign_target() == None,!is_call_target()).Concrete walk-through
delete "abc".lengthwithminify_syntaxon:Op::UnDelete(visit_expr.rs:1231, added by this PR) setsp.delete_target = EDot("abc".length).e_dotcomputesis_delete_target = true(line 1347) and callsmaybe_rewrite_property_access(..., IdentifierOpts::default()...with_is_delete_target(is_delete_target))at lines 1434-1442.- fold.rs:445-453 matches
EString, checks onlyp.options.features.minify_syntax && name == b"length", and returnsSome(E::Number(3.0))—identifier_optsis never read. - Output:
delete 3.
Observable difference
- Sloppy mode:
delete "abc".length→false(String exotic wrapper.lengthis a non-configurable own property;[[Delete]]returnsfalse). After fold:delete 3→true. - Strict mode (all ES modules,
bun rundefault):delete "abc".length→TypeError: Cannot delete property 'length' of [object String]. After fold:delete 3→true, silently swallowing the throw.
The assign case is worse in principle:
"abc".length = 5folds the LHS to3, emitting the syntactically-invalid assignment3 = 5.Why nothing else prevents it
e_dotreachesmaybe_rewrite_property_accessbefore any otheris_delete_target-aware path can intervene, and theEStringarm returnsSome(...)unconditionally onminify_syntax && name == b"length". The flag is delivered — thanks to this PR — but the arm never reads it. Before this PR the flag was alwaysfalseanyway (the assignment at line 1231 didn't exist), so the fold fired for the same reason; this PR wires the flag correctly to every consumer except this one arm.Why this is in scope for this PR
This PR's stated purpose is to wire
p.delete_targetand add the missing guard to the bun-specific string/array-index folds. The.lengthfold is another bun-specific string-literal fold with the identical reference→value hazard, and the PR's own new test at transpiler.test.js:150 assertsx = delete "foo"[2]stays as-is —x = delete "foo".lengthis the exact same shape. The PR'sRuntimeTranspilerCacheversion-25 comment even listsdelete "s"[n]as no longer folding to a value;.lengthis the sibling case. Per REVIEW.md ("Fix the whole class in the same PR … Grep for every sibling site sharing the pattern"), theEStringarm is the odd one out next to the already-guardedEObjectarm.Fix
Gate fold.rs:446-452 on
!identifier_opts.is_delete_target() && identifier_opts.assign_target() == js_ast::AssignTarget::None(matching theEObjectarm;is_call_targetis optional here since calling a number throws either way). Addts.expectPrintedMin_('x = delete "foo".length', 'x = delete "foo".length')alongside the newdelete "foo"[2]case in transpiler.test.js.Marked nit: real correctness bug and a legitimate same-class omission, but the trigger (
delete "literal".length) is highly contrived and the observable output is unchanged by this PR (the fold fired before too, sinceis_delete_targetwas permanentlyfalse). One-line guard + one test line. -
🟡
src/js_parser/visit/visit_expr.rs:1066-1070— The guard now covers delete/assign/call, but tagged-template tags are the fourth position with call-receiver semantics:[o.m][0]`x`bindsthisto the temporary array, but folding too.m`x`bindsthistoo.e_template(line 695-696) visits the tag via plainp.visit_exprwithout settingp.call_target, sois_call_targetis false there and the fold fires. A full fix needs thep.template_tagfield the port has TODO'd at lines 1044/1443 — probably a follow-up; for now the comment at 1060-1065 could note the gap so it doesn't read as exhaustive.Extended reasoning...
What the bug is
The guard at lines 1066-1070 now covers three of the four positions where a property Reference's base is observably distinct from the value it produces:
deletetarget, assignment target, and call target. Tagged-template tags are the fourth. Per ECMA-262, a tagged template evaluates its tag as a Reference and passes it throughEvaluateCall, which uses the Reference base asthisValue— exactly the same receiver semantics as a plain call. So[o.m][0]`tpl`invokeso.mwiththisbound to the temporary array[o.m], but folding it too.m`tpl`bindsthistoo.Why the current guard doesn't catch it
e_template(line 695-696) visits the tag viap.visit_expr(e_.tag.as_mut().unwrap())— a plain visit that does not writep.call_target(onlye_calldoes that) and there is nop.template_tagfield to write. So whene_indexruns on the tag expression,is_call_targetat line 887 isfalse,is_delete_targetisfalse, andin_.assign_targetisNone— the guard passes and the fold at ~1097-1104 fires.can_be_inlined_from_property_access(src/ast/expr.rs:53-61) rejects onlyESpread/EMissing, soEDotpasses and[o.m][0]becomeso.m.Step-by-step proof
With
minify_syntaxon:var o = { m() { return this === o } }; console.log([o.m][0]`x`);
e_templatevisits its tag[o.m][0]via plainp.visit_expr();p.call_targetis not set to this node.e_indexcomputesis_call_target = ptr::eq(p.call_target, e.data)at line 887 →false.is_delete_target→false.in_.assign_target→None.- The guard at 1066-1070 passes.
target.data.as_e_array()matches a single-element array; index is0.0; the elemento.misEDot, whichcan_be_inlined_from_property_accessaccepts. *e = inlined; return;— the tag is nowo.m, and the emitted code iso.m`x`.
Before the fold: the Reference base of
[o.m][0]is the array[o.m], sothisinsidemis that array;this === oisfalse. Node printsfalse.
After the fold:o.m`x`—this === oistrue. Bun printstrue.Why this is nit, not blocking
Unlike the previous round's
!is_call_targetaddition — a one-token change using a local already computed at line 887 — closing this gap requires porting thep.template_tagparser-state field that doesn't exist yet. It's TODO'd at lines 1044 and 1443 (// .is_template_tag = is_template_tag,/// .is_template_tag = p.template_tag != null,), and grep confirms notemplate_tagfield on the parser struct. Wiring it means: adding the field alongsidecall_target/delete_targetinp.rs, setting it ine_templatebefore visiting the tag, computingis_template_tagine_index/e_dot/e_identifier, and un-TODO'ing the two existing sites. That's a separate porting task with its own test surface, and the fold was already unguarded for template tags before this PR — nothing newly regresses.REVIEW.md's "fix the whole class in the same PR" was the basis for the previous round adding
!is_call_target, and this is unambiguously the same class. But that rule is calibrated for same-effort sibling sites; here the fourth sibling requires infrastructure the port lacks. The PR strictly improves the guard (0/4 → 3/4), so blocking on the missing port feature isn't proportionate.How to fix
Either wire
p.template_tagin this PR (mirroring esbuild'sp.templateTag = e.Tag.Databefore visiting the tag, then&& !is_template_tagon this guard and un-TODO'ing 1044/1443), or — if that's out of scope — amend the comment at 1060-1065 to note tagged templates as a known gap so it doesn't claim to enumerate all Reference-observable positions, and file a follow-up.
`delete "abc".length` folded to `delete 3` (true) instead of `false`. The EString arm in maybe_rewrite_property_access now matches its neighbouring EObject arm's guard. Also lock in the existing printer-side `(0, x)` wrap for `delete (true ? a.b : 0)` with a test; bun handles that case via WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS at print time rather than a visit-time before/after check.
|
On the three additional findings:
** |
…n printer wrap The HotEnabled/HotDisabled arm in maybe_rewrite_property_access returned EUndefined for `import.meta.hot.accept` etc. with HMR off, producing `delete undefined` / `undefined = fn`. Bail to the outer E::Dot so the output stays a property reference (`undefined.accept`, which throws at runtime exactly as the unfolded input would). Also add lock-in transpiler tests for `delete (0, a.b)` / `?? || && ?:`. bun handles those at print time via WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS (set in parse_prefix, checked in the printer) rather than esbuild's visit-time canBeDeleted before/after wrap; the output and runtime behaviour match esbuild.
…abled Under HMR-enabled (Bake dev server), every rewrite in this arm produces a valid hmr.<name> property reference; bailing left `ESpecial::HotEnabled` in the tree, which prints as `hmr.indirectHot` (a throwing Proxy), so `import.meta.hot.data = obj` regressed. The delete-undefined/assign-undefined hazard only exists on the !enabled branches.
There was a problem hiding this comment.
No new findings this pass; all prior threads are resolved. Deferring to a human because this changes the transpiler visit pass (affects every file bun run / bun build touches) and bumps the runtime transpiler cache version.
What was reviewed:
p.delete_targetassignment vs. esbuild'sp.deleteTarget = e.Value.Data; confirmed the print-timeWAS_ORIGINALLY_DELETE_...re-wrap covers the value→reference direction, so the omittedcanBeDeletedAfterport is intentional.- The
!enabledgate on theHotEnabled|HotDisabledbail — verified it no longer routesimport.meta.hot.data = xthroughhmr.indirectHotunder the dev server. - Remaining sibling gaps (tagged-template tag receiver, dot-define under delete,
EImportIdentifierin the printer re-wrap) are all pre-existing / byte-identical before and after, and documented in the PR body.
Extended reasoning...
Overview
The PR restores a one-line assignment dropped from the esbuild port — p.delete_target = e_.value.data in the Op::UnDelete visit arm — which makes every existing is_delete_target guard in e_identifier/e_dot/e_index/handle_identifier/maybe_rewrite_property_access reachable for the first time. It then adds the same !is_delete_target && assign_target == None (and !is_call_target where receiver identity matters) gate to four bun-specific folds that had none: the [x][n]/"s"[n] index fold, the "str".length fold, the import.meta.{main,hot,dir,...} inline, and the import.meta.hot.<method> inline. RuntimeTranspilerCache version is bumped 24→25. Tests cover transpiler output, bun build diagnostics, cjs2esm de-opt, and end-to-end runtime semantics.
Security risks
None. Pure transpiler-output correctness; no auth/crypto/permissions/network surface.
Level of scrutiny
High. visit_expr.rs and fold.rs run on every file the runtime transpiler and bundler process, and minify_syntax is on by default for bun run. The cache version bump invalidates every existing .pile on users' machines. This is exactly the kind of change where a subtle guard error miscompiles real code silently.
Other factors
This PR has already been through three review passes from me. I raised one real regression (the HotEnabled|HotDisabled guard breaking import.meta.hot.data = x under the Bake dev server — routed through the throwing hmr.indirectHot Proxy), which was fixed in d8337c4 by gating on !enabled. I raised two same-class sibling gaps ("str".length and import.meta.hot.<method>), both fixed. The three remaining items — tagged-template tags (needs p.template_tag from #36599), string-valued dot-defines under delete (matches esbuild), and EImportIdentifier/ECommonjsExportIdentifier missing from the printer's re-wrap helper — are all pre-existing, byte-identical before and after, and now documented in the PR body's "Not changed" section.
Test coverage is thorough (transpiler output assertions, runtime spawn test, bundler diagnostics, cjs2esm de-opt) and verified to fail on main. The change is correct as far as I can tell, but given the blast radius and that it took multiple rounds to converge, a maintainer should sign off.
Two more parallel-arm sites in the same bug class:
- `BinNullishCoalescingAssign | BinLogicalOrAssign` in `visit_right_and_finish`
(the `{}.x ??= v` HMR fold) returned `e_.right` bare with no
`has_value_for_this_in_call()` guard. Fires without `minify_syntax`.
`({}.x ??= obj.m)()` / `({}.x ??= obj.m)\`t\`` now emit `(0, obj.m)`.
- `substitute_single_use_symbol_in_expr`'s `ETemplate` arm lacked the
'don't substitute into a target that could change this' check the parallel
`ECall` arm has. `let x = obj.m; x\`t\`` no longer inlines to
`obj.m\`t\``.
Both were pre-existing for `is_call_target` too and neither is covered by
#36730/#36734.
|
CI status: conflicts resolved in c320ace (merged main; #36730 had landed with overlapping guards on the #88723: 171 passed so far. All bundler/transpiler/parser tests (the files this PR touches) are green. The one The gate (robobun/evidence) passed on the earlier sha on both ASAN and release. Ready for merge pending the build finishing. |
|
Updated 3:52 AM PT - Aug 4th, 2026
❌ @robobun, your commit c320ace has 1 failures in
🧪 To try this PR locally: bunx bun-pr 36734That installs a local version of the PR into your bun-36734 --bun |
`--drop=Bun` on `delete Bun.inspect()` replaces the call with EUndefined; without minify that prints as `delete undefined`, which parses back as delete-of-a-bare-identifier (strict-mode SyntaxError). The test passed on main only because the runtime transpiler then substituted `undefined` to `void 0` while is_delete_target was permanently false. is_identifier_or_numeric_constant_or_property_access already covers ENumber(inf/nan) for the same reason (they print as `Infinity`/`NaN`); add EUndefined so the bundled output is `delete (0, undefined)` and stays valid under strict mode. Fixes bundler_drop.test.ts drop/DeleteAssignTarget.
…6740) ## Problem ```js // m.js: export let x = 1; import * as ns from "./m.js"; console.log(delete (null ?? ns.x), ns.x); ``` ```console $ 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 ```console $ 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. <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end -->
|
@robobun fix conflicts |
…er-delete-target # Conflicts: # src/js_parser/visit/visit_expr.rs # src/js_printer/lib.rs # src/jsc/RuntimeTranspilerCache.rs # test/bundler/bundler_cjs2esm.test.ts # test/bundler/transpiler/transpiler.test.js
|
Conflicts resolved in c320ace. Main had merged #36730 (the |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js_parser/visit/visit_expr.rs (1)
1232-1233: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore
P::delete_targetafter the operand visit.
Op::UnDeletewrites the marker at Line 1232 and leaves it active.P::e_import_metachecks onlyData::EImportMeta, not the operand identity. Afterdelete import.meta, a later unrelatedimport.metacan receive the delete-target flag and skip its valid define rewrite.Save the previous marker, visit the operand, and restore the previous marker. This also preserves an outer marker for nested deletes.
Proposed fix
Op::UnDelete => { + let previous_delete_target = p.delete_target; p.delete_target = e_.value.data; p.visit_expr_in_out(&mut e_.value, ExprIn::default()); + p.delete_target = previous_delete_target; }As per coding guidelines, “Store state on the object whose lifetime matches it, reset reusable per-operation state...”
🤖 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_parser/visit/visit_expr.rs` around lines 1232 - 1233, Update the Op::UnDelete handling around p.delete_target and p.visit_expr_in_out to save the existing delete_target, set the operand marker, visit the operand, then restore the saved marker afterward. Preserve any outer marker during nested deletes and ensure later unrelated import.meta expressions do not inherit the temporary state.Source: Coding guidelines
🤖 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_parser/visit/visit_expr.rs`:
- Around line 1232-1233: Update the Op::UnDelete handling around p.delete_target
and p.visit_expr_in_out to save the existing delete_target, set the operand
marker, visit the operand, then restore the saved marker afterward. Preserve any
outer marker during nested deletes and ensure later unrelated import.meta
expressions do not inherit the temporary state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 53f58a98-bb5d-45be-99bf-3e316c6e13ba
📒 Files selected for processing (3)
src/js_parser/fold.rssrc/js_parser/visit/visit_expr.rssrc/jsc/RuntimeTranspilerCache.rs
|
On the save/restore suggestion for The self-review probed this exact concern ("stale-delete-target-import-meta-tag-match" and "delete-target-not-reset-after-visit") and dismissed both as not-applicable. |
…en-sh#36740) ## Problem ```js // m.js: export let x = 1; import * as ns from "./m.js"; console.log(delete (null ?? ns.x), ns.x); ``` ```console $ 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 oven-sh#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 ```console $ 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. <!-- robobun:evidence:begin --> --- **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 <!-- robobun:evidence:end -->
2 participants
Problem
The
UnDeletevisit arm never wrotep.delete_target, so everyis_delete_targetcheck derived from it (ine_identifier,e_dot,e_index,e_import_meta,handle_identifier, andmaybe_rewrite_property_access) was permanentlyfalse. The port carried the field and all the guards over from esbuild but dropped the one assignment that makes them reachable.With
minify_syntaxon (the default forbun run):Fix
Op::UnDeletenow setsp.delete_target = e_.value.databefore visiting the operand, mirroring esbuild'sp.deleteTarget = e.Value.Data. This activates every existingis_delete_targetguard.Three bun-specific rewrite sites that produce a value where a property reference is required had no guard of their own and are now gated on
!is_delete_target && assign_target == None(and!is_call_targetwhere receiver identity matters):"foo"[n] -> "o"/[x][n] -> xfolds ine_index.delete [obj.p][0]previously becamedelete obj.p;[y][0] = 5becamey = 5;[o.m][0]()becameo.m()(wrong receiver)."str".length -> nfold inmaybe_rewrite_property_access.delete "abc".lengthfolded todelete 3(trueinstead offalse/TypeError).import.meta.{main,hot,dir,file,path,url}andimport.meta.hot.<method>inlines inmaybe_rewrite_property_access.delete import.meta.hot/delete import.meta.hot.acceptbecamedelete undefined(strict-mode SyntaxError);delete import.meta.mainunder--format=cjsbecame(delete require.main) == module.RuntimeTranspilerCacheversion bumped since the runtime transpiler enablesminify_syntax.Not changed
delete (0, a.b)/delete (true ? a.b : x)/?? || &&: bun re-wraps these at print time viaUnaryFlags::WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS(set inparse_prefix.rs, consumed atsrc/js_printer/lib.rs:4004) rather than esbuild's visit-time before/after check; output and runtime behaviour already match esbuild. Tests added to lock this in.delete process.env.NODE_ENVwith a string--define: esbuild also substitutes todelete "development"; the port matches the reference here.[o.m][0]`x`): the fourth reference-observable position, but bun has nop.template_tagfield yet (TODO stubs atvisit_expr.rs:1044/1443). js_parser: fold property access on multi-property object literals #36599 introduces that state; the&& !is_template_tagterm can be appended once it lands.Verification
bun bd test test/bundler/transpiler/transpiler.test.js -t "property access inlining"(5 new tests; fail on main)bun bd test test/bundler/transpiler/assign-to-import.test.ts(3 new cases; fail on main)bun bd test test/bundler/bundler_cjs2esm.test.ts -t Delete(2 new tests; fail on main)transpiler.test.js(188 pass),bundler_minify.test.ts(42 pass),bundler_cjs2esm.test.ts(25 pass),esbuild/default.test.ts(151 pass),esbuild/dce.test.ts(78 pass),bundler_edgecase.test.ts(117 pass).[review] gate passed · iteration 5 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 5 passed · 0 rejected · iteration 5
evidence per changed file