Repository navigation
Keep this undefined for call targets and template tags that a rewrite turned into a property access - #40829
Keep this undefined for call targets and template tags that a rewrite turned into a property access#40829robobun wants to merge 8 commits into
this undefined for call targets and template tags that a rewrite turned into a property access#40829Conversation
…te turned into a property access
A call target or template tag that is a property access binds `this` to
the object. When a fold, a single-use inlining, a dotted define, or the
bundler's namespace rewrite turned some other expression into a property
access in that position, the printer emitted it bare and `this` changed:
`import { fn } from "./cjs"; fn()` bundled to `import_cjs.fn()`,
`(0, o.f)\`x\`` printed as `o.f\`x\``, and `const t = a?.b; t\`x\`` printed
as `a?.b\`x\``, which is a SyntaxError.
Mirror esbuild: `E::Call` gets `target_was_originally_property_access`
and `E::Template` gets `tag_was_originally_property_access`, set by the
parser when the callee or tag is an `EDot` or `EIndex`. The printer wraps
the target in `(0, ...)` when it is a property access now but was not one
originally. An import item in that position is wrapped by the
`EImportIdentifier` arm through the same flag. The parser tracks
`template_tag` next to `call_target` so the `{f}.f` and `[o.f][0]`
inlining and dotted defines treat a tag like a call target. The
optional-chain check for tags now inspects the tag node.
Synthesized calls with a property-access target set the flag. The runtime
transpiler cache version is bumped because printed output changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (25)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughChangesThe parser and AST now record original property access for call targets and template tags. Visitors, lowerers, and generated expressions preserve this metadata. The printer emits Receiver semantics
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR preserves undefined 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is detailed and covers the problem, fix, verification, regression tests, cache changes, and known CI limitations. It provides the information required by the template, although it uses Problem and Fix headings instead of the template headings. Comment |
|
Updated 4:39 PM PT - Aug 28th, 2026
❌ @robobun, your commit c7fe42e has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40829That installs a local version of the PR into your bun-40829 --bun |
…raps The happy-dom and libraries.js corpus bundles call named imports of node builtins (net.isIP, fs.readFileSync, url.fileURLToPath). In CommonJS output those are namespace members, so they now print as (0, import_net.isIP)(...) and the JS and bytecode hashes move on every platform at once.
|
Reproduced on main (65362b5) with CI on f61cb42: 180 of 181 jobs pass. The one red lane is darwin x64, where b3549b6 fixes the 5d98550 fixes a regression found in a self-review of the printer rule: with CI on c7fe42e (build 108032): |
#36735 The clippy lint trivially_copy_pass_by_ref rejects a reference to the 8-byte E::ImportIdentifier in is_unbound_call_target. The new test cases cover the folds that PR #36735 guarded one by one: a ternary whose test is typeof or a dropped ||, a call or template tag nested in the left operand of a comma, the {}.x ??= v fold, and an index into an object literal. The printer wraps every one of them.
optimize_props_method_calls turns props.foo(1) into a CallExpression whose callee is the props.foo property load. The printer now wraps a property access call target in (0, ...) unless the call was originally a property access, so the compiled output became (0, props.foo)(1) and this changed from props to undefined. Babel prints the callee as written. Set the flag from the callee that codegen emits. Also lock in the output for a bare call of a namespace export: f() inside namespace NS prints (0, NS.f)(), as esbuild does.
|
Landing order note: once the printer keeps the parentheses of Verified on bun 1.4.1 with |
|
Landing order note for #42402. #42402 fixes a regression from #41186 that is in 1.4.1 and 1.4.2. An import item made of When this PR is rebased after #42402:
#42452 lists the shapes of this kind that are wrong on 1.4.0 too. This PR covers all of them. |
…#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 -->
|
Update to my landing order note above, the one that asks for #40848 to land first: that constraint no longer applies. Upstream WebKit fixed the engine side in WebKit/WebKit@7fbe85cfbfdc, and main has it since #44615 (in Bun v1.4.3). On a debug build of main at 34cbc70, The printer side is still open on main: a plain file with |
Problem
thisto the object. When a fold turns another expression into a property access in that position, the printer emitted it bare andthischanged:(0, o.f)\x`and(true && o.f)`x`both printed aso.f`x`.bun runhits the same folds, sobun file.js` disagreed with Node.import { fn } from "./cjs"; fn()bundled toimport_cjs.fn()(the wrap check insrc/js_printer/lib.rsmatchedEIdentifier, notEImportIdentifier).const t = a?.b; t\x`printeda?.b`x``, a SyntaxError.Fix
E::Callgetstarget_was_originally_property_accessandE::Templategetstag_was_originally_property_access, set inparse_suffix.rs. The printer wraps the target in(0, ...)when it is a property access now but was not one originally.EImportIdentifierarm wraps through the same flag.template_tagnext tocall_targetso the{f}.fand[o.f][0]inlining paths and dotted--definesubstitution treat a tag like a call target. The paren check for an optional chain inspects the tag.test/bundler/transpiler/transpiler.test.js,test/bundler/transpiler/runtime-transpiler.test.ts(output equals Node),test/bundler/bundler_cjs.test.ts. Alsotest/bundler/**,test/bake/dev,test/js/bun/resolve.Background
a.b()passesaasthis.(0, a.b)()andfn()wherefnholdsa.bpassundefined. Tagged templates follow the same rule.import_cjs.fn). Node calls such an export withthis === undefined.hmr.require,import.meta.require, Temporal.from, the React compilerMethodCallandCallExpression, classic JSX factory, decorator.call). The others target identifiers.Notes
CallKind::TargetWasOriginallyPropertyAccessandETemplate.TagWasOriginallyPropertyAccessinjs_ast.go, the(0, ...)paths injs_printer.go(printExprforECallandETemplate), andp.templateTaginjs_parser.go. esbuild 0.28.2 output for the repros matches this PR except for import items used as template tags (import_lib.who\``), where this PR also wraps.bun runoutput for the fixtures inruntime-transpiler.test.tswas compared with Node 26.3.0.test/bundler/bundler_npm.test.ts(ReactSSR): fivejsxDEVcalls are now(0,Ha.jsxDEV)(...), which moves two source map columns by 16 and grows the file by 20 bytes.react-compiler.test.ts.snap: threeimport_react.*calls are now wrapped.bundler_bytecode_portable.test.ts: the corpus bundles now print(0, import_net.isIP)(...)style calls, so the snapshot hashes moved.this#36735 fixed the template tag case with a guard in each fold. It is closed in favor of this PR. The folds it guarded one by one are covered by the printer here, and its test cases are folded into the tests: a ternary whose test istypeof xor a dropped||(theCouldHaveSideEffectspath ine_if), a call or tag nested in the left operand of a comma (the latecall_targetcapture ine_binary), the{}.x ??= vfold, and({ f: o.f })["f"].optimize_props_method_callsturnsprops.render(1)into aCallExpressionwhose callee is theprops.renderproperty load. An audit of every synthesizedE::Callfound that this one printed as(0, props.render)(1)with the new printer rule, while Babel printsprops.render(1). Codegen now sets the flag from the callee it emits (5d98550, testreact-compiler/PropsMethodCallKeepsReceiver).f()insidenamespace NS { export const f = ... }becomesNS.f()in the output. It now prints(0, NS.f)(), which keepsthisundefined as the source call did. esbuild 0.21 prints the same. tsc printsNS.f()and leaksNSasthis. Locked in by theTypeScript namespace exportscase.[f][0]()folds tof()(since js_parser: stop the [x][0] / {f:x}.f folds from changing optional-chain / this / assignment semantics #36730). Node passes the array asthisthere, bun passesundefined.[o.f][0]()gets the(0, o.f)wrap, sothisisundefinedin both.bun build --format=cjsdrops"use strict"inside__commonJSwrappers (the cjs and iife bundler tests here use a probe that reports "unbound" for bothundefinedandglobalThis);--format=iife --target=nodewith externals emits__require("node:module")before__requireexists; JavaScriptCore evaluates(y?.z)\`withthis === undefinedwhere V8 usesy(the runtime test avoids athis`-dependent tag there).compile/HelloWorldWithProcessVersionsBunfails on a debug build independent of this change (process.versions.bunkeeps the-debugsuffix).no test proof · iteration 4 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bundler_npm.test.ts, test/bundler/bundler_bytecode_portable.test.ts