Skip to content

bundler: keep a property read's this and getter for an unbound item of a require() / import() local - #42402

Open
robobun wants to merge 4 commits into
mainfrom
robobun/cab301aa/namespace-local-read-this-and-getters
Open

robobun wants to merge 4 commits into
mainfrom
robobun/cab301aa/namespace-local-read-this-and-getters

Conversation

@robobun

@robobun robobun commented Sep 12, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Since bundler: read import() / require() exports without a namespace object #41186 (1.4.1 to 1.4.3), a bundle can call a CommonJS export with the wrong this. (0, x_1.fn)(), which TypeScript emits, prints x_1.fn() with --minify-syntax. An unused ns.a; is removed, so a getter does not run.
  • Cause: maybe_rewrite_property_access (src/js_parser/fold.rs:175) makes ns.f an import item. The folds and expr_can_be_removed_if_unused then treat it as a bound ES import.

Fix

  • P::has_value_for_this_in_call replaces the Expr method. It also accepts an item whose namespace_alias names a namespace local (P::is_namespace_local_read).
  • expr_can_be_removed_if_unused returns false for such an item.
  • A kept wrapper or read is always valid, and the parser cannot know the binding.
  • Verified: test/bundler/bundler_dynamic_import_dce.test.ts (4 new cases, all fail on main), 23 other bundler suites. Self-reviewed (Notes).

Background

Downsides

  • For a bound item, a wrapped call keeps its wrapper (+4 bytes with --minify). An unused top-level read keeps its export: const unused = THREE.WebGLRenderer grows a three bundle from 343,632 to 919,596 bytes.
  • Real code shows none of that: 0 bytes on zod, effect, three and 196 unminified builds of 110 packages.
  • Binary .text: +768 bytes. Instructions: +0.04% on typeorm, inside the noise floor on three and zod.
Notes

Reach. No user issue reports this. It still changes real output today. bun build --minify of import "<pkg>" was run for each of the 110 packages in test/node_modules. 96 build, and in 17 of them main drops 2,440 (0, …) wrappers that this branch keeps: typeorm 661, mongodb 520, verdaccio 367, fastify 254, @grpc/grpc-js 188. Each one is a call that gets the required module as this.

Repro with a real package. cross-fetch@4.1.0 (dist/browser-ponyfill.js) ends with exports = ctx.fetch; exports.fetch = ctx.fetch; module.exports = exports, where ctx.fetch is the platform fetch when there is one. TypeScript 6.0.2 (module: commonjs) compiles

import { fetch } from "cross-fetch";
await fetch(url);

to const cross_fetch_1 = require("cross-fetch"); await (0, cross_fetch_1.fetch)(url);. With bun build --minify --target=browser the bundle calls cross_fetch_1.fetch(url). That calls the platform fetch with the function object as this. Browsers reject that receiver (Illegal invocation). Checked under node 26.3.0 with a globalThis.fetch that makes the same receiver check, with and without --format=cjs:

no flags --minify
1.4.0 ok 200 ok 200
1.4.1, 1.4.2, 1.4.3, main ok 200 TypeError: … Illegal invocation
this branch ok 200 ok 200

Released binaries (each bundle is built with that binary and run under node 26.3.0. "main" is a release build of 5866b5f. The entries are the ones in the new tests, reduced):

node on the source 1.4.0 1.4.1 to 1.4.3, main same, --minify-syntax this branch, both
(true ? ns.f : 0)(), (1 && ns.f)(), (0 || ns.f)(), (null ?? ns.f)() undefined x4 same exports x4 exports x4 undefined x4
(0, ns.f)(), (0, ns.f)?.(), const f = ns.f; f() undefined x3 same undefined x3 exports x3 undefined x3
getter count after ns.boot; void ns.boot; const unused = ns.boot; if (ns.boot) {} 4 4 1 0 4
const first = ns.first; return ns.second + first first,second same first,second second,first first,second

1.4.0 gives node's result in each cell, with and without --minify-syntax. For an import() local of a CommonJS module, node itself reads a snapshot and runs no getter, so there the reference is 1.4.0 (3 getter runs for 3 reads. 1.4.1 to main: 0).

Measurements. Two release builds of the merged tree (main at 5866b5f): one with the src/ of main, one with the src/ of this branch.

  • Binary (size): .text 106,331,459 to 106,332,227 (+768). data and bss do not change. The stripped file has 106,501,896 bytes in both builds.
  • Instructions, user space, all threads, median of 11 interleaved runs. The noise floor is a byte-identical copy of the main binary, run in the same rounds. perf and valgrind are not installed in the container and its package mirrors do not resolve, so a small C program read PERF_COUNT_HW_INSTRUCTIONS through perf_event_open.
input main this branch difference noise floor
three 0.186.1, static import, --minify 194,228,271 194,406,905 +0.09% 0.12%
zod 4.6.7 through import(), --minify 113,550,190 113,002,491 -0.48% 0.61%
typeorm through require(), --minify 1,024,276,578 1,024,670,365 +0.04% 0.003%
50 libraries in one bundle (libraries.js of the bytecode corpus) 20,385,348,179 20,386,319,481 +0.005% 0.005%

Only typeorm is above its noise floor. It is TypeScript CommonJS output, so it has the most items and gets 661 wrappers back.

  • Wall clock, 30 interleaved runs of the three and typeorm builds: the difference is below the noise floor (2.4% to 5.8%), so it says nothing.
  • Output bytes, main against this branch, each with no flags, --minify-syntax and --minify:
entry result
const { z } = await import("zod"), const zod = await import("zod") with zod.z reads 0 bytes in 6 builds
const { pipe } = await import("effect"), const effect = require("effect") (effect 4.0.5 is ESM only, so the items are bound) 0 bytes in 6 builds
const THREE = await import("three"), import * as THREE from "three" 0 bytes in 6 builds
TypeScript CommonJS emit (0, effect_1.pipe)(…), a wrapped call of a bound item +0, +5 (--minify-syntax), +4 (--minify)
const unused = THREE.WebGLRenderer; added to the three entry, no flags 343,632 to 919,596. 1.4.0: 1,364,307
the same with --minify 249,170 to 532,539. 1.4.0: 750,699
const unused = THREE.Quaternion; (an export the bundle needs anyway) +46, +10 with --minify
the 16 bundles that bundler_bytecode_portable.test.ts snapshots (lodash, acorn, react-dom, svelte, undici, happy-dom, immutable, 50 libraries in one file) byte-identical, .js and .jsc, 23,703,231 bytes of JavaScript
110 packages of test/node_modules, import "<pkg>" and require("<pkg>"), no flags 196 builds, 0 differ
the same with --minify 192 builds, 46 differ, +26,541 bytes on 130,831,659 (+0.02%), 6,731 wrappers restored

The minified growth on the 110 packages is the fix itself, on CommonJS items: restored (0, x.f)() wrappers, and a local that no longer moves past an item read (let s={…};return Dt.f(s) in tsyringe, as 1.4.0 and esbuild print it). webpack is left out of the minified count: main gives two different outputs for it in four runs.

The cost for a bound item, and how to remove it. The parser decides before the linker knows the binding, so it has to assume the item is a property read. 1.4.1 to 1.4.3 assume the opposite, which is the bug. A bound item therefore keeps a wrapper and an unused top-level read, as on 1.4.0. To give both back, the decision has to move to where the binding is known. For this, that is the printer rule of #40829. For an unused read, the parser can record the items that a part reads at top level, and the linker can keep the part only if one of them is unbound. Neither is in this PR.

How the item is found. fold.rs gives the item a namespace_alias whose namespace_ref is the local. is_namespace_local_read looks that ref up in dynamic_import_namespace_locals. The other writers of namespace_alias (the import scanner, the hot-reload paths in p.rs) use the namespace of an import statement, never a namespace local.

Why the Expr method is gone. Expr::has_value_for_this_in_call cannot see the symbol table, so it cannot see these items. With it removed, a new fold site has to call the parser method. #41159 (open) adds a fold site and rewrites both ternary arms with the Expr method. After a rebase it does not compile until it calls p.has_value_for_this_in_call.

esbuild (0.18.6 from test/node_modules, --bundle --minify-syntax): for const ns = require("./x.cjs") it keeps ns.boot; and (0, ns.who)(). For import * as ns of the same file it drops the read and prints ns.who().

Scope. Only const / let locals and .then parameters are tracked, so var x = require(...) (Babel, ES5 TypeScript output) was never affected. const x = __importDefault(require(...)) / __importStar(...) is not a direct require() initializer and is not tracked either. With --format=esm, require() of a package in DEFAULT_UNWRAP_COMMONJS_PACKAGES (react, react-dom, react-is, scheduler, react-client, react-server, react-refresh) outside try becomes an import statement. Its items are import * as ns items, so they behave as on 1.4.0 and this PR does not change them.

Related.

Not changed. ns.f() written as a direct call on a bound item still calls f() with this === undefined (the behaviour change that #41186 lists). These shapes are wrong on 1.4.0 too, so they are not part of this regression. They print the same on 1.4.0, on main and on this branch (#42452, closed, has the repros):

  • The tagged-template form (0, o.f)`x` folds for each plain EDot with --minify-syntax.
  • (0, ns.f)() and (1 && ns.f)() for an item of import * as ns, or of a named import, of a CommonJS module that stays wrapped lose the wrapper.
  • Inside such a CommonJS module, (0, exports.f)() and (1 && exports.f)() lose the wrapper (ECommonjsExportIdentifier).
  • ([] ? o.m : 0)() folds to o.m() for each property access (the ternary path for a test that is known but not free of side effects).

#40829 covers the this part of all four in the printer.

Self-review. A review of the diff raised these points. Addressed: the test for the item no longer depends on the order of the import scanner (map lookup), a new fold site cannot call a check that misses these items (the Expr method is gone), each changed fold site has a test shape ((false ? 0 : ns.f)(), [ns.f][0](), ns["f"]), the getter test covers await import(), .then and a plain local that must not move past a read, and the notes above name #40829, #41159, the unwrap list and the siblings. Not taken: a fix for the sibling shapes, because they are not part of the regression.

Suites run on the debug build of the merged tree, all pass: bundler_dynamic_import_dce (337), bundler_minify, bundler_cjs2esm, bundler_cjs, bundler_promiseall_deadcode, bundler_barrel, bundler_splitting, bundler_edgecase, bundler_regressions, bundler_bun, bundler_jsx, bundler_feature_flag, bundler_drop, bundler_npm, bundler_browser, bundler_plugin, esbuild/dce, esbuild/importstar, esbuild/importstar_ts, esbuild/splitting, esbuild/extra, esbuild/default, esbuild/ts, transpiler/transpiler.test.js. The 4 new cases also fail 4 of 4 on the release build of main and pass 4 of 4 on the release build of this branch. bundler_bytecode_portable times out on a debug build (5 s per test), so its 16 bundles were built with both release builds and compared (see Measurements).


[human-review] gate passed · iteration 0 · 5 files touched

fails on main (without fix)
ASAN without fix: 4 FAILED
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_dynamic_import_dce.test.ts
bun test v1.4.4 (367d939d9)

test/bundler/bundler_dynamic_import_dce.test.ts:
(pass) bundler > dynamic_import_dce/AwaitDestructure [923.86ms]
(pass) bundler > dynamic_import_dce/AwaitDestructureAlias [456.24ms]
(pass) bundler > dynamic_import_dce/AwaitDot [402.70ms]
(pass) bundler > dynamic_import_dce/AwaitIndex [382.19ms]
(pass) bundler > dynamic_import_dce/LetBinding [395.67ms]
(pass) bundler > dynamic_import_dce/TwoSitesUnion [435.38ms]
(pass) bundler > dynamic_import_dce/BailoutRest [399.12ms]
(pass) bundler > dynamic_import_dce/BailoutDefault [493.51ms]
(pass) bundler > dynamic_import_dce/BailoutComputed [411.86ms]
(pass) bundler > dynamic_import_dce/SplittingNarrowedExports [441.14ms]
(pass) bundler > dynamic_import_dce/SplittingTwoImportersUnion [751.57ms]
(pass) bundler > dynamic_import_dce/SplittingEscapeKeepsAll [447.59ms]
(pass) bundler > dynamic_import_dce/SplittingAwaitDot [409.26ms]
(pass) bundler > dynamic_import_dce/SplittingThenDestructure [462.00ms]
(pass) bundler > dynam
... (truncated)

release without fix: all passed
bun test v1.4.4-canary.1 (7d3da10c3)

test/bundler/bundler_dynamic_import_dce.test.ts:
(pass) bundler > dynamic_import_dce/AwaitDestructure [222.54ms]
(pass) bundler > dynamic_import_dce/AwaitDestructureAlias [24.21ms]
(pass) bundler > dynamic_import_dce/AwaitDot [27.28ms]
(pass) bundler > dynamic_import_dce/AwaitIndex [19.87ms]
(pass) bundler > dynamic_import_dce/LetBinding [30.19ms]
(pass) bundler > dynamic_import_dce/TwoSitesUnion [20.91ms]
(pass) bundler > dynamic_import_dce/BailoutRest [24.81ms]
(pass) bundler > dynamic_import_dce/BailoutDefault [24.53ms]
(pass) bundler > dynamic_import_dce/BailoutComputed [30.37ms]
(pass) bundler > dynamic_import_dce/SplittingNarrowedExports [27.66ms]
(pass) bundler > dynamic_import_dce/SplittingTwoImportersUnion [43.14ms]
(pass) bundler > dynamic_import_dce/SplittingEscapeKeepsAll [29.16ms]
(pass) bundler > dynamic_import_dce/SplittingAwaitDot [21.08ms]
(pass) bundler > dynamic_import_dce/SplittingThenDestructure [23.60ms]
(pass) bundler > dynamic_import_dce/SplittingPromiseAllDestructure [31.97ms]
(pass) bundler > dynamic_import_dce/SplittingPromiseAllThenDestructure [26.85ms]
(pass) bundler > dynamic_import_dce/SplittingPro
... (truncated)
passes on PR (with fix)
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/bundler/bundler_dynamic_import_dce.test.ts
bun test v1.4.4 (367d939d9)

test/bundler/bundler_dynamic_import_dce.test.ts:
(pass) bundler > dynamic_import_dce/AwaitDestructure [921.40ms]
(pass) bundler > dynamic_import_dce/AwaitDestructureAlias [444.44ms]
(pass) bundler > dynamic_import_dce/AwaitDot [409.02ms]
(pass) bundler > dynamic_import_dce/AwaitIndex [358.61ms]
(pass) bundler > dynamic_import_dce/LetBinding [434.96ms]
(pass) bundler > dynamic_import_dce/TwoSitesUnion [463.81ms]
(pass) bundler > dynamic_import_dce/BailoutRest [482.50ms]
(pass) bundler > dynamic_import_dce/BailoutDefault [419.22ms]
(pass) bundler > dynamic_import_dce/BailoutComputed [419.15ms]
(pass) bundler > dynamic_import_dce/SplittingNarrowedExports [470.40ms]
(pass) bundler > dynamic_import_dce/SplittingTwoImportersUnion [844.95ms]
(pass) bundler > dynamic_import_dce/SplittingEscapeKeepsAll [498.27ms]
(pass) bundler > dynamic_import_dce/SplittingAwaitDot [575.43ms]
(pass) bundler > dynamic_import_dce/SplittingThenDestructure [593.84ms]
(pass) bundler > dynam
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 2062ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/54] rustc bun_ast 
[2/54] rustc bun_resolve_builtins 
[3/54] rustc bun_install_types 
[4/54] rustc bun_options_types 
[5/54] rustc bun_http 
[6/54] rustc bun_parsers 
[7/54] rustc bun_sourcemap 
[8/54] rustc bun_js_printer 
[9/54] rustc bun_sema_parser 
[10/54] rustc bun_react_compiler 
[11/54] rustc bun_sema_driver 
[12/54] rustc bun_css 
[13/54] rustc bun_sema_baselines 
[14/54] rustc bun_ini 
[15/54] rustc bun_js_parser 
[16/54] rustc bun_resolver 
[17/54] rustc bun_router 
[18/54] rustc bun_lint 
[19/54] rustc bun_bundler 
[20/54] rustc bun_transpiler 
[21/54] rustc bun_lint_oxlint 
[22/54] rustc bun_standalone_graph 
[23/54] rustc bun_lint_graph 
[24/54] rustc bun_lint_react_compiler 
[25/54] rustc bun_bunfig 
[26/54] rustc bun_lint_jest 
[27/54] rustc bun_lint_unicorn 
[28/54] rustc bun_lint_plugins 
[29/54] rustc bun_lint_react 
[30/54] rustc bun_lint_eslint 
[31/54] rustc bun_install 
[32/54] rustc bun_lint_typescript 
[33/54] rustc bun_jsc 
[34/54] rustc bun_ast_jsc 
[35/54] rustc bun_sys_jsc 
... (truncated)
diff hotspot
src/ast/expr.rs                                 |   5 --
 src/js_parser/p.rs                              |  38 ++++++--
 src/js_parser/visit/visit_binary.rs             |   8 +-
 src/js_parser/visit/visit_expr.rs               |   6 +-
 test/bundler/bundler_dynamic_import_dce.test.ts | 113 ++++++++++++++++++++++++
 5 files changed, 150 insertions(+), 20 deletions(-)

gate history · 2 passed · 0 rejected · iteration 0

evidence per changed file
file                                             reads  edits  tests
src/ast/expr.rs                                      1      1     32
src/js_parser/p.rs                                   9      4     32
src/js_parser/visit/visit_binary.rs                  2      0     33
src/js_parser/visit/visit_expr.rs                    4      0     33
test/bundler/bundler_dynamic_import_dce.test.ts      2      1     32

… of a require() / import() local

`ns.f` off a local that holds a require() / import() result is an import
item since #41186. The linker binds it only to an export of a bundled ES
module. For a CommonJS or external importee, or an escaped namespace, it
prints `ns.f`, a property read on an object.

The parser treated the item as a bound ES import:
- It dropped the wrapper of `(0, ns.f)()`, `(true ? ns.f : 0)()`,
  `(1 && ns.f)()`, `(0 || ns.f)()`, `(null ?? ns.f)()`, so the call got
  `ns` as `this`. With --minify-syntax it also moved `const f = ns.f`
  into `f()`.
- It removed an unused `ns.a` read, `void ns.a`, and `if (ns.a) {}`,
  and moved one read past another, so a getter did not run or ran in a
  different order.

Now the parser keeps both behaviours of a property read for such an item.
@robobun

robobun commented Sep 12, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 8:46 AM PT - Oct 16th, 2026

✅ @robobun, your commit 7d3da10c3d2a730dee7927c753953639abe5e8e9 passed in Build #126599! 🎉


🧪   To try this PR locally:

bunx bun-pr 42402

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

bun-42402 --bun

@coderabbitai

coderabbitai Bot commented Sep 12, 2026 •

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Essentials

Run ID: 8e0778f9-82f6-407b-a430-756230a6b6fa

📥 Commits

Reviewing files that changed from the base of the PR and between 56e9d79 and 9216f8b.

📒 Files selected for processing (3)
  • src/ast/expr.rs
  • src/js_parser/p.rs
  • test/bundler/bundler_dynamic_import_dce.test.ts
💤 Files with no reviewable changes (1)
  • src/ast/expr.rs

Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.

Walkthrough

The parser detects namespace-local property reads from import() and require(). It preserves this during call-target substitution and retains potentially side-effectful reads. Expression optimizations and bundler tests use the updated behavior.

Changes

Namespace read preservation

Layer / File(s) Summary
Parser namespace-read semantics
src/js_parser/p.rs, src/ast/expr.rs
Added namespace-local read detection, centralized this-binding checks, and tree-shaking preservation for unlinked namespace reads. Removed the expression-level helper.
Expression optimization and regression coverage
src/js_parser/visit/visit_binary.rs, src/js_parser/visit/visit_expr.rs, test/bundler/bundler_dynamic_import_dce.test.ts
Updated binary and conditional simplifications to use the parser-level check. Added coverage for call receivers, getters, evaluation order, module forms, escaped namespaces, and minified syntax.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to 9216f

The namespace-read fix preserves receiver semantics and getter side effects, with regression coverage for normal and minified output.

Pre-merge checks: Passed 4
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 main fix: preserving this and getter behavior for property reads from unbound require() and import() locals. It is specific and related to the changeset, althoug…
Description check Passed The description explains the problem, cause, fix, scope, trade-offs, and verification results. It does not use the exact template headings, but it provides the required information in equivalent secti…
Full details: Title check

Explanation

The title clearly identifies the main fix: preserving this and getter behavior for property reads from unbound require() and import() locals. It is specific and related to the changeset, although somewhat long.

Full details: Description check

Explanation

The description explains the problem, cause, fix, scope, trade-offs, and verification results. It does not use the exact template headings, but it provides the required information in equivalent sections and is substantially complete.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reviewed this PR and didn't find any bugs. Because it changes parser-side minification/DCE semantics that every bundled output flows through, and the description notes an overlapping printer-side approach in #40829, a human familiar with the esbuild port should confirm this is the right layer for the fix.

What was reviewed:

  • Grepped has_value_for_this_in_call across src/ — all 7 js_parser call sites now go through the parser-aware wrapper; none remain on Expr:: directly.
  • The ECall substitution guard refactor is behavior-preserving: Expr::has_value_for_this_in_call is exactly EDot | EIndex, so the new predicate only widens the guard.
  • self.symbols[ref.inner_index() as usize] matches the indexing pattern used throughout src/js_parser/ (visit/mod.rs, scan_symbols.rs, p.rs).
  • Tests exercise comma / ?: / && / || / ?? / local-substitution / ?.() for require, await import, .then, escaped ESM, and external, with and without minifySyntax, plus getter retention and read ordering.
Extended reasoning...

Overview

The PR fixes a bundler regression where EImportIdentifier nodes synthesized from ns.a (where ns is a require()/import() result local) were treated as side-effect-free bound imports during minification. Two new helpers on P — is_namespace_local_read and a parser-aware has_value_for_this_in_call — are threaded through the seven wrapper-fold sites in visit_binary.rs / visit_expr.rs and the single-use-substitution call-target guard in p.rs. expr_can_be_removed_if_unused_without_dce_check now returns false for these reads. Four itBundled cases (two bugs × minifySyntax on/off) are added to the existing test/bundler/bundler_dynamic_import_dce.test.ts.

Security risks

None. This is bundler output-shape logic with no auth, crypto, filesystem, or network surface. The change is strictly more conservative — it only ever keeps a wrapper or a read that was previously dropped, never the reverse — so it cannot introduce new code into user bundles that wasn't there before.

Level of scrutiny

Moderate-to-high. The code change itself is small, mechanical, and verifiably complete (grep confirms no Expr::has_value_for_this_in_call() callers remain in src/js_parser; the EDot | EIndex → predicate refactor is exact because that's the full body of the Expr method; the symbol-table indexing follows the file's ubiquitous pattern). The tests assert runtime-observable this values and getter execution order rather than output snapshots, so they prove the behavior end-to-end. However, this is parser-side minification semantics that interacts with a later linker binding decision, REVIEW.md calls out esbuild as the spec for ported code, and the PR itself flags #40829 as a competing printer-side approach for the this half — a maintainer who owns the port should weigh which layer the fix belongs at.

Other factors

No CODEOWNERS entry covers src/js_parser/ or test/bundler/. The bug hunt exited on dry_streak with no findings and no ruled-out candidates. The PR description's claim that the extensive bundler suites (minify, dce, importstar, cjs, cjs2esm, bytecode snapshots) still pass is consistent with the change being purely widening of an existing conservative check, but I did not independently run them.

@robobun

robobun commented Sep 12, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: the branch is merged with main again (7d3da10), so the conflict is gone. The one conflict was in the test file. The tests of this PR are now at the end of that file. The PR body has the measured costs.

How to reproduce (1.4.1, 1.4.2, 1.4.3 and main. 1.4.0 is correct):

// util.cjs
"use strict";
exports.tag = "EXPORTS";
exports.who = function () { return this === undefined ? "unbound" : this.tag; };

// entry.js
const util_1 = require("./util.cjs");
console.log(util_1.who(), (0, util_1.who)());
console.log((true ? util_1.who : 0)(), (1 && util_1.who)());
bun build ./entry.js --target=bun --minify-syntax --outdir out && bun out/entry.js
  • Expected (bun entry.js, node, the 1.4.0 bundle, this branch): EXPORTS unbound then unbound unbound.
  • 1.4.1 to 1.4.3 and main: EXPORTS EXPORTS then EXPORTS EXPORTS. Without --minify-syntax the first line is correct and the second line is not.

The getter case: a module that defines boot with Object.defineProperty(exports, "boot", { get() { return ++count; } }), read as ns.boot; void ns.boot; const unused = ns.boot; after const ns = require("./lazy.cjs"). The count is 3 on 1.4.0 and on this branch. It is 0 on 1.4.1 to 1.4.3 and on main, because the bundle has none of the three reads.

Both cases are in test/bundler/bundler_dynamic_import_dce.test.ts (UnboundItemCallWithoutThis, UnboundItemReadRunsGetter, each with and without minifySyntax).

ImportScanner::scan also sets was_originally_property_access, for the
items of `import * as ns`. It runs in to_ast, after every caller of
this check. Comment only.
Comment thread src/js_parser/p.rs Resolved
Comment thread src/js_parser/p.rs Resolved
Comment thread src/js_parser/p.rs Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/js_parser/visit/visit_expr.rs Resolved
…pr::has_value_for_this_in_call

- is_namespace_local_read looks the namespace_ref of the item's alias up
  in dynamic_import_namespace_locals. It no longer reads
  was_originally_property_access, which ImportScanner also sets later
  for the items of `import * as ns`.
- Expr::has_value_for_this_in_call is gone. It cannot see the symbol
  table, so a fold site that calls it misses these items. Each fold site
  now has to call P::has_value_for_this_in_call.
- expr_can_be_removed_if_unused gets a guard arm for such an item. The
  esbuild comment on the import-item arm is as before.
- Tests: `(false ? 0 : ns.f)()`, `[ns.f][0]()` and `ns["f"]` for the
  call, `await import()`, `.then` and a plain local that must not move
  past a read for the getter.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

Jarred-Sumner pushed a commit that referenced this pull request Sep 26, 2026
…#42549)

### Problem
- With `bun build --minify-syntax`, ``ns["tag"]`z` `` prints as
``$tag`z` `` when `tag` is a lifted CommonJS export. The tag then runs
with `this === undefined`: `TypeError: undefined is not an object
(evaluating 'this._helper')`. ``st["tag"]`z` `` on a default import and
``exports["tag"]`z` `` inside the CommonJS file break too.
- The cause is the `a["b"]` to `a.b` rewrite in `e_index`
(`src/js_parser/visit/visit_expr.rs:858`). It moves `p.call_target` and
`p.delete_target` to the new `E::Dot`, then visits it. It does not move
`p.template_tag` (added in #41251). So `e_dot` sees no tag, and the
linker binds the export directly.

### Fix
- The rewrite also points `p.template_tag` at the new `E::Dot`. esbuild
does the same (`p.templateTag = dot`).
- The second rewrite in `e_index` (`a["x" + "y"]`) is unchanged. It
returns the new `E::Dot` without a visit, so nothing reads the marker.
That form already works.
- Verified: `test/bundler/bundler_cjs2esm.test.ts` (3 new tests, all
fail on main). Also 11 other bundler suites (see the notes).
- Self-reviewed: 2 concerns raised, 2 addressed.

### Background
- Lifting turns `exports.foo = x` in a CommonJS file into `var $foo = x`
plus an ES export. The namespace object (`exports_lib`) stands in for
`module.exports`.
- ``X.name`z` `` passes `X` as `this`, like a method call. ``$name`z` ``
passes `undefined`. #41251 makes the linker keep `X.name` as written
when the file calls it or tags a template with it.
- `e_template` stores the tag node in `p.template_tag`. `e_dot` and
`e_index` compare their own node with it by address. A rewrite that
replaces the node must move the marker.

<details><summary>Notes</summary>

Repro (fails on 1.4.3-canary.1+6a92015fc and on main b993710). `git
tag --contains f31440d` lists bun-v1.4.1 and bun-v1.4.2, so both
releases have the same code:

```sh
cat > lib.cjs <<'EOF'
exports._helper = function (s) { return "helped:" + s[0]; };
exports.tag = function (s) { return this._helper(s); };
EOF
cat > entry.mjs <<'EOF'
import * as ns from "./lib.cjs";
console.log(ns["tag"]`z`);
EOF
bun build entry.mjs --minify-syntax --outfile out/min.js --target bun && bun out/min.js
# TypeError: undefined is not an object (evaluating 'this._helper')  at $tag
```

`out/min.js` on main holds ``console.log($tag`z`)``. With this change it
holds ``console.log(exports_lib.tag`z`)`` and prints `helped:z`.

The marker reaches three consumers, all through
`IdentifierOpts::is_template_tag` set in `e_dot`:

- `maybe_rewrite_property_access` (`src/js_parser/fold.rs:271`) for
`import * as ns`.
- `record_import_property_use` (`src/js_parser/p.rs:6712`) for a default
import.
- `note_commonjs_export_use` (`src/js_parser/p.rs:5955`) for
`exports.name` in the lifted file itself.

Each new test covers one consumer. Each test also has the computed
method call (`X["parse"]("y")`), which already worked, so both markers
are covered under `minifySyntax`. The name `tag` appears only in
computed form in each entry, because one marked use of a name keeps
every `X.name` of that file as written and would hide the fault.

The second rewrite (`ns["ta" + "g"]`, or an inlined TypeScript enum key)
does not go through `maybe_rewrite_property_access` at all. It prints
``exports_lib.tag`z` `` on main and with this change. In esbuild that
rewrite happens after `maybeRewritePropertyAccess` and carries no
marker.

Checked by hand with `--minify-syntax` and `--minify`, all print
`helped:z`: `const ns = await import("./lib.cjs")`,
`require("./lib.cjs")["tag"]`, `const ns = require("./lib.cjs")`,
`import * as ns` through an `export *` barrel, and `import { default as
st }`.

Sites this change leaves alone:

- Other folds check only for a call target, not for a template tag: the
comma fold in `e_binary`, the ternary fold in `e_if`, and the `[x][0]`
fold in `e_index`. They turn ``(0, o.f)`x` `` into ``o.f`x` ``. That is
the opposite fault (the tag gains a receiver). It also reaches plain
`bun run`: ``(0, o.f)`x` `` sees `this === o` there, and `undefined` in
node. #42452 (section 4) tracks them.
- #40829 (open, conflicts with main) is the candidate fix for those
folds. Its diff has the same three lines as this change, as one hunk of
26 files. It predates #41251. If this change lands first, #40829 drops
that hunk on its rebase.
- #42402 (open) changes `has_value_for_this_in_call` at the fold sites.
It does not touch this rewrite or `p.template_tag`.

The star import test matches the receiver with `\w+` and does not pin
`exports_lib`, because #41820 (open) gives `import *` of a lifted module
a namespace object of its own, with another name. The default import
test and the self test pin the exact output.

Suites run on the debug build: `bundler_cjs2esm`, `bundler_minify`,
`bundler_cjs`, `bundler_edgecase`, `bundler_dynamic_import_dce`,
`esbuild/default`, `esbuild/dce`, `esbuild/importstar`,
`esbuild/importstar_ts`, `esbuild/ts`, `transpiler/transpiler`,
`transpiler/macro-test`.

</details>

<!-- robobun:evidence:begin -->

---

**[human-review]** gate passed · iteration 0 · 2 files touched

<details><summary>fails on main (without fix)</summary>

```console
ASAN without fix: BUILD FAILED (no junit output)
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/bundler/bundler_cjs2esm.test.ts"
ninja: Entering directory `/workspace/bun/build/debug'
[1/66] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o
[2/66] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
  - ResolveMessage (15 fields)
  - BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
  - Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
  - ResourceUsage (8 fields)
  - Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
  - CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
  - FileSystemRouter (5 fields)
  - FrameworkFileSystemRouter (2 fields)
  - MatchedRoute (8 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts
  - Glob (5 fields)
Found 1 classes from /workspace/bun/src/runtime/api/h2.classes.ts
  - H2FrameParser (32 fields)
Found 9 classes from /workspace/bun/src
... (truncated)

release without fix: 3 FAILED
bun test v1.4.3-canary.1 (6a92015)

test/bundler/bundler_cjs2esm.test.ts:
(pass) bundler > cjs2esm/ModuleExportsFunction [29.67ms]
(pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [13.37ms]
(pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [14.05ms]
(pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [12.90ms]
(pass) bundler > cjs2esm/ExportsFunction [12.86ms]
(pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [14.55ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequire [14.10ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [14.87ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [12.71ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [11.37ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [12.24ms]
(pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [14.95ms]
(pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [15.54ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRuntimeCondition [11.08ms]
(pass) bundler > cjs2esm/UnwrappedModuleRequireAssigned [14.33ms]
(pass) bundler > cjs2esm/Unwrap
... (truncated)
```

</details>

<details><summary>passes on PR (with fix)</summary>

```console
ASAN with fix: all passed
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" "test/bundler/bundler_cjs2esm.test.ts"
bun test v1.4.3 (6a92015)

test/bundler/bundler_cjs2esm.test.ts:
(pass) bundler > cjs2esm/ModuleExportsFunction [956.60ms]
(pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [435.19ms]
(pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [376.00ms]
(pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [410.55ms]
(pass) bundler > cjs2esm/ExportsFunction [350.96ms]
(pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [570.76ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequire [366.45ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [483.54ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [421.50ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [435.10ms]
(pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [423.59ms]
(pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [613.50ms]
(pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [603
... (truncated)

release with fix: all passed
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped) in 739ms (unchanged)
ninja: Entering directory `/workspace/bun/build/release'
[1/61] cc obj/src/jsc/bindings/sqlite/sqlite3.c.o
[2/61] gen generated_host_exports.rs
generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited
[3/61] gen ZigGeneratedClasses.{cpp,h,rs}
Found 2 classes from /workspace/bun/src/jsc/resolve_message.classes.ts
  - ResolveMessage (15 fields)
  - BuildMessage (10 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Archive.classes.ts
  - Archive (4 fields, 1 class fields)
Found 2 classes from /workspace/bun/src/runtime/api/BunObject.classes.ts
  - ResourceUsage (8 fields)
  - Subprocess (20 fields)
Found 1 classes from /workspace/bun/src/runtime/api/cron.classes.ts
  - CronJob (5 fields)
Found 3 classes from /workspace/bun/src/runtime/api/filesystem_router.classes.ts
  - FileSystemRouter (5 fields)
  - FrameworkFileSystemRouter (2 fields)
  - MatchedRoute (8 fields)
Found 1 classes from /workspace/bun/src/runtime/api/Glob.classes.ts
  - Glob (5 fields)
Found 1 classes from /workspace/bun/src/runtime/api/h2
... (truncated)
```

</details>

<details><summary>diff hotspot</summary>

```
src/js_parser/visit/visit_expr.rs    |  3 ++
 test/bundler/bundler_cjs2esm.test.ts | 62 ++++++++++++++++++++++++++++++++++++
 2 files changed, 65 insertions(+)
```

</details>

**gate history** · 1 passed · 0 rejected · iteration 0

<details><summary>evidence per changed file</summary>

```
file                                  reads  edits  tests
src/js_parser/visit/visit_expr.rs         3      1     12
test/bundler/bundler_cjs2esm.test.ts      3      4     12
```

</details>

<!-- robobun:evidence:end -->
…nd-getters

Conflict: test/bundler/bundler_dynamic_import_dce.test.ts. Main added
ExternalReExportDestructureIsSnapshot (#42471) at the place where this
branch added its tests. Both are kept.

The tests of this branch now follow the KeepNamespace cases at the end
of the file. Those cases (CommonJS importee, external importee, escaped
namespace) are the conditions under which an item stays unbound.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant