Repository navigation
Conversation
When --define is given an object or array literal, emit it once as
`var define_KEY_default = {...}` at module scope and substitute every
reference with that identifier, instead of inlining a fresh literal at
each use site.
This matches esbuild's output and makes two references to the same
define compare equal (a === b), lets mutations be visible across
references, and stops a define referenced in a hot loop from allocating
per iteration. It also fixes the case where assigning to an
object-valued define previously printed an invalid `{...} = x`
expression.
WalkthroughChangesHoisted define identity
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: tests pass locally (fail-before/pass-after). Diff is green. Linked #21319 and #21312 (output now matches esbuild). Not linking #21210: that issue is about primitive-valued defines on the LHS of an assignment ( Addressed review in bea1e6e + bc1585c: the CI build 82612: none of the failures touch this diff.
Ready for review. |
|
Updated 2:19 PM PT - Jul 26th, 2026
❌ @robobun, your commit 9c4f9f9 has 2 failures in
Add 🧪 To try this PR locally: bunx bun-pr 35958That installs a local version of the PR into your bun-35958 --bun |
||||||||||||||||||||||||||||||||||||||||||||||||||||||
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
Found 3 issues this PR may fix:
🤖 Generated with Claude Code |
![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 `@src/js_parser/lib.rs`:
- Around line 514-517: Add a dedicated SAFETY comment immediately before the
unsafe impl Sync for InjectedDefine, documenting why synchronized access is
sound for InjectedDefine::value; keep the existing Send comment attached only to
the unsafe impl Send.
🪄 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: af89bf53-ff7a-4aa3-80ae-13263def4110
📒 Files selected for processing (6)
src/bundler/defines.rssrc/bundler/options.rssrc/js_parser/lib.rssrc/js_parser/p.rssrc/js_parser/parse/parse_entry.rstest/bundler/esbuild/extra.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🔴
src/js_parser/p.rs:6350-6359— The new injected-define path returnsEIdentifier(ref_)without accounting fordeletetargets. With--define 'process.env.CFG={"x":1}', sourcedelete process.env.CFGnow emitsdelete define_process_env_CFG_default, which is an early SyntaxError in strict mode (all ESM output) — before this PR the same input emitted the validdelete { x: 1 }. Either skip the injected path for delete targets and inline the literal, or add a post-visit(0, x)wrap in theOp::UnDeletearm mirroringUnTypeof.Extended reasoning...
What the bug is
When a
--definekey is given an object/array value and the source containsdelete <that key>, the bundler now emitsdelete <bare-identifier>. In strict mode — which every ES module is, andFormat::Esmis the default output format —delete <IdentifierReference>is an early SyntaxError per ES §13.5.1.1. The bundled module fails to parse at load time.Code path
Walk
delete process.env.CFGwith--define 'process.env.CFG={"x":1}':- Parse —
parse_prefix.rs:408-414: thedeleteoperand isEDot, soUnaryFlags::WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESSis set on the unary node. - Visit the unary —
visit_expr.rs:1221-1223:Op::UnDeletevisits its operand withExprIn::default(), soin_.assign_target == None. Unlike theUnTypeofarm just above (lines 1179-1196), there is no post-visit fixup that wraps a newly-appeared identifier in(0, x). - Visit the e_dot operand —
visit_expr.rs:1347: the dot-define substitution gates only onin_.assign_target == None(which it is), so it callsvalue_for_define. - This PR's new code —
p.rs:6350-6359:injected_define_indexisSome, so it returnsEIdentifier(ref_)unconditionally. Theis_delete_targetparameter is ignored on this path. - Print —
js_printer/lib.rs:4102: the printer's protective(0, x)wrap fires only when!e.flags.contains(WAS_ORIGINALLY_DELETE_OF_IDENTIFIER_OR_PROPERTY_ACCESS). That flag is set (step 1), so the guard is false and the identifier prints bare.
Output:
delete define_process_env_CFG_default→ strict-mode SyntaxError.Why existing code doesn't prevent it
The printer guard at
js_printer/lib.rs:4102was designed for the reverse case: preventingdelete (0, x)from being simplified todelete x. It trusts theWAS_ORIGINALLY_...flag to mean "the source already had a valid delete target here, leave it alone". That assumption breaks when the visitor swaps an EDot for an EIdentifier while the parse-time flag remains set. TheUnTypeofarm has an explicit post-visitid_before/id_aftercheck for exactly this class of substitution;UnDeletedoes not.Regression
Before this PR,
value_for_definefell through to the tailExpr { data: value, loc }and returned theEObjectinline, producingdelete { x: 1 }.deleteon a non-Reference is valid strict-mode JS (evaluates totrue). So this is a valid → syntactically-invalid output regression: any bundle that containsdelete <object-valued-dot-define>now fails to load. (The identifier-key case —--define 'CFG={..}'+delete CFG— was already broken pre-PR because the source operand was an identifier and the flag was set anyway; but object/array dot defines were previously safe and are now broken.)Fix
Two options:
- In
value_for_define(p.rs:6350), skip the injected path when the expression is a delete target and fall through to inline the literal — restoring the pre-PRdelete { x: 1 }behaviour. Caveat: theis_delete_targetargument is computed fromp.delete_target, which is not currently set in theOp::UnDeletevisitor arm (grep shows it's only assigned at visit_expr.rs ~911/1026 during e_index→e_dot conversion), so this alone won't fire on the directdelete a.b.cpath — you'd also need to setp.delete_targetbefore visiting the operand in theUnDeletearm. - Or, more robustly, mirror the
UnTypeofpost-visit fixup (visit_expr.rs:1179-1196) in theOp::UnDeletearm: if the operand was not an identifier before visiting but is one after, wrap it in(0, x). That producesdelete (0, define_process_env_CFG_default), which is valid strict-mode JS and preserves the identity/hoisting benefit of this PR.
Repro
echo 'export {}; delete process.env.CFG;' > a.mjs bun build --define 'process.env.CFG={"x":1}' a.mjs # output contains: delete define_process_env_CFG_default # → SyntaxError: Delete of an unqualified identifier in strict mode
- Parse —
Also propagate AllocError from ensure_valid_identifier instead of catching it.
Compare the operand's symbol name before vs after the visit instead of only checking whether the tag became EIdentifier. This covers `delete CFG` (sloppy source) where the pre-visit operand is already an identifier that gets replaced by the hoisted define's identifier, while leaving source-written `delete x` / `delete unbound` untouched.
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/esbuild/extra.test.ts`:
- Around line 742-761: Update the DefineObjectDeleteBareIdentifier test
configuration to execute the bundled output and assert the expected stdout: true
for delete CFG, true for delete ARR, and false for delete keep. Preserve the
existing onAfterBundle regex checks, using the sibling DefineObjectDeleteTarget
test’s run configuration as the model.
🪄 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: 80107e7c-8e5e-4931-a252-a629b21181fc
📒 Files selected for processing (5)
src/js_parser/lib.rssrc/js_parser/p.rssrc/js_parser/parse/parse_entry.rssrc/js_parser/visit/visit_expr.rstest/bundler/esbuild/extra.test.ts
There was a problem hiding this comment.
Both earlier findings (OOM catch-default in prepare_for_visit_pass, and the bare-identifier delete CFG sibling case) are addressed and covered by tests. No further issues found, but this touches the core define-substitution path across the parser/visitor/part-emission and adds new unsafe impl Send/Sync, so it's worth a human look — particularly the per-module (not cross-module) hoisting tradeoff called out in the description.
What was reviewed:
Define::insert/InjectedDefine— index assignment happens for both identifier and dot defines;for_workerclonesinjected.value_for_define— falls through to the old inline path wheninjected_define_refsis empty (e.g. no visit pass), so no regression there.Op::UnDeletename-compare — verified source-writtendelete xstays unwrapped (name matches afterfind_symbol); the wrap now also fires for pre-existing identifier→identifier defines, which is the intended strict-mode fix.parse_entry.rs— parts are gated onuse_count_estimate > 0and markedcan_be_removed_if_unused, so unused defines don't bloat output.
Extended reasoning...
Overview
This PR changes how --define handles object/array values in the bundler: instead of inlining a fresh literal at every substitution site, it declares one var define_KEY_default = {...} per module and substitutes an identifier reference. Changes span src/js_parser/lib.rs (new InjectedDefine type, Define.injected field, DefineData.injected_define_index), src/js_parser/p.rs (symbol pre-declaration in prepare_for_visit_pass, new branch in value_for_define), src/js_parser/parse/parse_entry.rs (emit the var part per referenced define), src/js_parser/visit/visit_expr.rs (wrap delete operands that become identifiers via substitution), and src/bundler/{defines,options}.rs (field plumbing). Five new itBundled tests exercise identity, dot-defines, no-bundle mode, and both delete variants with runtime execution.
Security risks
None identified. Define values are parsed once at startup by the existing parse_env_json path; this PR only changes where the resulting AST node is emitted. The new unsafe impl Send/Sync for InjectedDefine mirrors the existing DefineData impls with the same rationale (immutable process-lifetime AST-store pointers, read-only after init) and now carries per-impl SAFETY comments.
Level of scrutiny
Medium-high. This is the bundler's define-substitution path — it runs on every parsed file when --define is set, and the Op::UnDelete change affects every delete <expr> regardless of defines. The change is well-scoped and well-tested, but it's not mechanical: it introduces new module-scope symbol declaration, a new Part-emission site (which interacts with tree-shaking via can_be_removed_if_unused / declared_symbols), and a name-comparison heuristic in the delete visitor. The PR also explicitly ships a partial esbuild-parity design (per-module hoisting rather than a cross-module <define:X> virtual module) — a maintainer should confirm that intermediate step is acceptable.
Other factors
Two prior review rounds from this bot were addressed in bea1e6e/bc1585c/9c4f9f9 (OOM propagation, the bare-identifier delete sibling, and running the delete test under sloppy-mode iife/node). The clippy SAFETY-comment failure was fixed. Test coverage is solid: all five new tests run: the bundle and assert stdout, and the identity test additionally counts literal occurrences in the output. The Op::UnDelete wrap now also applies to identifier→identifier defines (--define X=someGlobal + delete X), which is a behavior change beyond the linked issues but is correctly framed as a strict-mode-SyntaxError fix. CI's only hard failure is the binary-size gate against a stale baseline (per the author's note), unrelated to this diff.
|
Closing as part of a cleanup of stale pull requests. This PR has had no new commits since 2026-07-26, it conflicts with main, and its last CI run failed. This is not a judgment on the fix itself. The linked issue (#21312) stays open. If the problem still reproduces on a current build, reopen this PR after a rebase or open a new one against main. |
What
When
--defineis given an object or array literal, emit it once asvar define_KEY_default = {...}at module scope and substitute every reference with that identifier, instead of inlining a fresh{...}at each use site.Repro
Before:
After (matches esbuild):
Cause
value_for_defineinp.rsreturnedExpr { data: define_data.value, loc }for anything that wasn't an identifier or string. ForEObject/EArraythat meant the same AST node was printed inline at every substitution site, so the output contained N independent literals. esbuild instead routes compound define values through its--injectpath as a synthetic<define:KEY>module and substitutes a reference to its default export.Fix
Define::insertassigns aninjected_define_indexto anyDefineDatawhose value isEObject/EArrayand records it onDefine.injected.prepare_for_visit_passpre-declares one module-scope symbol per injected define (define_<key>_default, hash-suffixed when the renamer isn't in use).value_for_definereturnsEIdentifier(ref)for injected defines and records the usage.parse_entry.rsemitsvar define_X_default = {...}as its ownbeforepart (markedcan_be_removed_if_unused) for each injected define that was actually referenced.This also fixes a latent bug where assigning to an object-valued define printed
{"k":1} = x;, which is a runtime SyntaxError.Cross-module identity (a single object shared across every bundled file, esbuild's
<define:X>virtual module) is not included; it needs the--injectinfrastructure which bun has not ported. Each module now materializes the literal once instead of once per reference.Verification
Also ran the full
extra.test.ts(223 pass),transpiler.test.js(182 pass),bundler_env.test.ts,bundler_jsx.test.ts, and thebun-build-api.test.tsdefine cases.Fixes #21319
Fixes #21312
[review] gate passed · iteration 1 · 7 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 1
evidence per changed file