Conversation
`process.env["NODE_ENV"]` (and any user `--define`d dot path spelled with brackets) was only substituted when `--minify` was passed, because the `a["b"]` => `a.b` rewrite that routes `EIndex` into the `e_dot` define lookup is gated on `minify_syntax`. The same gap made `--drop=console.log` miss `console["log"]()` and `--drop=a` miss `a["b"]["c"]()`. Add the define lookup (value substitution plus the `--drop` flag) to `e_index` for a string-literal index, and propagate the `--drop` flag through the index target visit, mirroring `e_dot`. esbuild checks defines on `EIndex` before the minify rewrite for the same reason.
|
Updated 8:51 AM PT - Jun 29th, 2026
❌ @robobun, your commit 0dda155 has 3 failures in
🧪 To try this PR locally: bunx bun-pr 33062That installs a local version of the PR into your bun-33062 --bun |
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughThe JS parser's Computed string-literal define/drop substitution
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
I didn't find any issues — the new e_index define lookup mirrors e_dot faithfully and the test coverage is solid — but this is core transpiler output semantics, so worth a quick human pass.
Extended reasoning...
Overview
This PR adds ~40 lines to src/js_parser/visit/visit_expr.rs so that computed string-literal member accesses (process.env["NODE_ENV"]) consult the define table the same way dot accesses (process.env.NODE_ENV) already do, independent of --minify. It also propagates the property_access_for_method_call_maybe_should_replace_with_undefined flag through the e_index target visit so --drop=a.b catches a.b["c"](). Three test files gain coverage for define substitution, --drop, and the NODE_ENV browser-target case.
I compared the new block against e_dot (visit_expr.rs:1398-1436) and is_dot_define_match (p.rs:6694-6727): the logic is a near-verbatim port — same assign-target guard, same valueless/drop branching, same break on first match — with the documented omission of the can_be_removed_if_unused / call_can_be_unwrapped_if_unused flag copies that E::Index lacks fields for. The is_utf8() guard matches the existing EIndex arm in is_dot_define_match, and the early return on substitution prevents double-handling when minify's a["b"] => a.b rewrite would have fired afterward. Optional chains remain correctly excluded via the existing optional_chain.is_some() check in is_dot_define_match.
Security risks
None. This is transpiler output shaping (define substitution / --drop); no auth, crypto, fs, or network surface.
Level of scrutiny
Moderate-to-high. The change is small and pattern-matched against proven code, but it lives in the core JS visitor and silently alters generated output for any code using bracket-notation property access that happens to collide with a define key. A subtle mistake here would manifest as wrong runtime values in user bundles rather than a build error, which argues for a human sanity check even though the diff reads cleanly.
Other factors
- No CODEOWNERS for
src/js_parser/. - New tests cover substitution, assign-target exclusion, non-literal/non-matching keys, and three nested
--dropshapes; PR description reports the fullbundler_*/transpiler/esbuild/*suites pass. - No prior reviews on the PR; only the robobun build comment.
|
CI status. The diff is green on every lane that exercises it; the remaining red is infrastructure and two pre-existing upstream issues, none of it caused by this change. The review finding was real and is fixed. claude's line comment on the target visit was correct: propagating the BuildKite. Build 66730 (the original fix) had 280 test jobs pass, covering every linux glibc, ASAN, Windows, and darwin 14 lane. Its only failures:
Build 66785 against 0dda155 is final: 282 jobs passed, 4 failed. None of the failing files are in this diff.
The GitHub Actions A parser change produces the same AST on every platform, so a failure confined to one musl GC-timing test, one darwin artifact-download step, and two docs files outside this diff cannot come from this change. I already spent the one retrigger; this is ready for review. |
In `e_index`, the target visit can set the parser-global
`method_call_must_be_replaced_with_undefined` for the enclosing `e_call`
to consume. Visiting the index expression next left that flag live, so a
call nested in the index consumed it instead: `console[lvl()]("x")` with
`--drop=console` emitted `console[undefined]("x")`, a runtime TypeError.
Stash and clear the flag across the index visit and restore it after, so
the enclosing call is the one that sees it. `e_dot` has no index
sub-expression, which is why it never had this problem.
There was a problem hiding this comment.
The fix in 0dda155 correctly addresses the flag-leak I flagged earlier and the regression test covers it — looks right to me, but since this changes code-gen semantics in the core parser visit pass (every a["b"] now hits the define table), I'd like a human to sign off.
Extended reasoning...
Overview
This PR extends e_index in src/js_parser/visit/visit_expr.rs to consult the --define/--drop table for computed string-literal member accesses (a["b"]), mirroring the existing e_dot path. It also propagates the property_access_for_method_call_maybe_should_replace_with_undefined flag through the target visit so --drop=a.b removes a.b["c"](), and stashes/restores the parser-global method_call_must_be_replaced_with_undefined across the index sub-expression visit. Three test files add coverage for define substitution, drop propagation, and the process.env["NODE_ENV"] browser-target case.
Follow-up on prior review
I previously flagged that propagating the drop flag into the target visit let it leak into the index visit, producing console[undefined]("x") for console[lvl()]("x") under --drop=console. Commit 0dda155 fixes this by stashing the flag with core::mem::replace immediately before p.visit_expr(&mut e_.index) and restoring it immediately after — the restore happens before any of the arm's early returns, and the private-identifier arm doesn't visit the index, so the scoping is sound. The new drop/ComputedDynamicIndex test exercises exactly this path. The bug-hunting pass on the fixed revision found nothing.
Security risks
None. This is a build-time AST transformation with no auth, network, filesystem, or crypto surface. The substituted values come from the user's own --define/--drop configuration.
Level of scrutiny
High. This is the core JS parser visit pass — every file Bun bundles or transpiles flows through e_index, and the change alters user-facing output semantics (code that previously emitted process.env["X"] verbatim now inlines it). The interaction between the per-visit ExprIn flag and the parser-global method_call_must_be_replaced_with_undefined is subtle enough that the first revision shipped a real regression; while the follow-up fix looks correct and is well-tested, I don't have high enough confidence to say a human shouldn't look at a code-gen behavior change in this layer.
Other factors
Test coverage is thorough (transpiler-level, bundler-level, and the targeted regression test), CI is green on every relevant lane, and there is no CODEOWNER for src/js_parser/. The change is small and closely mirrors the established e_dot pattern, so the remaining review burden should be light.
|
Another report of this came in today, framed around Same root cause as this PR (the |
|
Superseded by #40857. That PR looks up defines for a computed string index in The tests from this PR pass on that branch: |
1 participant
Problem
bun build --target=browserleavesprocess.env["NODE_ENV"]verbatim unless--minifyis also passed, while the dot formprocess.env.NODE_ENVis always substituted. The bracket form is what codemods, env-helper wrappers, and TS index signatures produce, and the leftoverprocessreference is aReferenceErrorin a browser. The same source therefore behaves differently between minified and unminified browser bundles.esbuild substitutes both forms regardless of minification.
Cause
The define table lookup only lives in the
EDotvisitor (e_dot). A computed access only reaches it through thea["b"]=>a.brewrite at the top ofe_index, which is gated onminify_syntax.is_dot_define_matchalready handlesEIndexintermediates (soprocess["env"].NODE_ENVworked); only the outermostEIndexwas never looked up.The same missing lookup affects user
--define(--define 'FOO.BAR="hi"'missesFOO["BAR"]) and--drop:--drop=console.logdoes not removeconsole["log"]("x")--drop=Bun.inspectdoes not removeBun.inspect["table"](), becausee_indexalso did not propagate the drop flag to its target the waye_dotdoesFix
In
e_index, when the index is a string literal, consult thedotsdefine table before theminify_syntaxrewrite: substitute on a non-valueless match, or set the--dropflag on a valueless one. Also propagateproperty_access_for_method_call_maybe_should_replace_with_undefinedthrough the target visit, matchinge_dot. This mirrors esbuild, which checks defines onEIndexahead of its minify rewrite.E::Indexhas nocan_be_removed_if_unused/call_can_be_unwrapped_if_unusedfields (unlikeE::Dot), so the DCE flag copies from thee_dotpath are intentionally not ported; doing so needs new fields onE::Indexand only affects a minification pessimization for the no-side-effect globals table, not correctness.The optional-chain forms (
a?.["b"], thedefault/DefineOptionalChaintodo test) remain a separate gap, tracked by #21084 / #28693.Verification
New tests fail on the released binary and pass with the fix:
Full
bundler_edgecase,bundler_env,bundler_browser,bundler_minify,transpiler,esbuild/default, andesbuild/dcesuites pass.