Repository navigation
Conversation
|
Warning Review limit reached
On-demand reviews are free for the next 14 days. After that, they cost $0.25 per reviewed file. Or wait 2 minutes for your next included review. View limit detailsLimit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Status: ready for review. Reproduced on 1.4.3-canary (d316760) with the |
There was a problem hiding this comment.
LGTM — narrow, conservative deopt with matching coverage.
Checked that both parallel lift sites in maybe_rewrite_property_access (the exports.x identifier arm and the E::Special::ModuleExports arm) get the same guard, and that name is the byte-slice already compared against literal names nearby, so == b"__proto__" is the local idiom. The change only widens the existing is_delete_target() deopt, so previously-correct modules are unaffected — the fallback is the baseline __commonJS wrapper. The new itBundled case exercises dot, bracket, module.exports., and the read-only form, asserts all four stay wrapped via cjs2esm.unhandled, and pins runtime semantics (own keys, prototype inheritance, namespace/default interop). Runtime-computed keys and Object.defineProperty already bypass the lift path, so no additional sibling sites needed the guard.
Extended reasoning...
Overview
This PR touches two files: a two-condition addition in src/js_parser/fold.rs (lines 466 and 677) that makes the CommonJS-to-ESM unwrapping optimization deoptimize when it encounters exports.__proto__ / module.exports.__proto__, and a new itBundled test case in test/bundler/bundler_cjs2esm.test.ts. The bug is that exports.__proto__ = v was being lifted to a hoisted binding named $__proto__ plus a namespace getter, but at runtime that assignment goes through the Object.prototype.__proto__ accessor and sets the prototype of module.exports — no own property is created, so a lifted binding cannot model it.
Security risks
None. This is a bundler output-correctness fix in the JS parser's constant-folding / property-access rewriting pass. No untrusted input parsing, no auth/crypto/permissions surface, no memory management. The change is purely additive to an existing deoptimization gate and can only cause modules that previously (incorrectly) unwrapped to now keep their __commonJS wrapper — the wrapper is the conservative baseline that already matches Node semantics.
Level of scrutiny
Low-to-moderate. The logic change is two || name == b"__proto__" clauses appended to conditions that already handle the analogous delete exports.x case, in both parallel arms of maybe_rewrite_property_access. The byte-slice equality matches the surrounding style (name == b"filename", name == b"path" a few lines above). REVIEW.md's "fix the whole class" is satisfied: both the exports_ref identifier path and the ModuleExports special path are updated symmetrically, and the test covers dot, bracket, module.exports., and read-only forms. Dynamic index access and Object.defineProperty already fall outside the static-member lift pattern and don't need the guard. The one-line source comment states the non-obvious semantic ("prototype of module.exports, not an export"), which is exactly the load-bearing "why" the review guidance asks for.
Other factors
The test is placed in the existing bundler_cjs2esm.test.ts alongside sibling cjs2esm/* cases and uses the itBundled harness with cjs2esm.unhandled to assert the wrapper is kept — this is the assertion that would fail on the pre-fix build (per the PR description, 3 of 4 files were lifted on canary). Runtime stdout is pinned with JSON.stringify of exact expected values including own-key sets, hasOwn checks, inherited-property reads, and namespace/default interop, so the assertion is strong rather than substring-based. No CODEOWNERS entries cover the changed paths. The bug hunt exited on dry_streak with no findings and no ruled-out candidates. The change cannot regress correct output — deoptimizing to the wrapper is always semantics-preserving, only forgoing tree-shaking for the rare module that touches __proto__.
|
Updated 6:59 PM PT - Sep 6th, 2026
❌ @robobun, your commit 388cf7d has 2 failures in
🧪 To try this PR locally: bunx bun-pr 41768That installs a local version of the PR into your bun-41768 --bun |
Problem
bun buildunwraps a CommonJS module to ESM bindings when an ES module imports it and the module only writesexports.<name>.exports.__proto__ = vwas treated as an export named__proto__: it becamevar $__proto__ = vplus a getter in__exportCjs(exports_lib, { __proto__: () => $__proto__ }, ...). In node that statement sets the prototype ofmodule.exports. Soimport lib from "./lib.cjs"; lib.inheritedisundefinedin the bundle and1in node and inbun run.maybe_rewrite_property_access(src/js_parser/fold.rs:462forexports.x,:673formodule.exports.x). It lifts every member name into aCommonJSExportIdentifier,__proto__included.Fix
exports.__proto__ormodule.exports.__proto__(dot or bracket form) deoptimizes the named-export unwrapping, the same waydelete exports.xalready does. The module keeps its__commonJSwrapper and the statement runs as written.module.exportsis a real object, so theObject.prototype.__proto__accessor behaves exactly as in node. esbuild never unwraps and matches node here too.test/bundler/bundler_cjs2esm.test.ts(cjs2esm/ExportsProtoMemberKeepsWrapper, fails on 1.4.3-canary with 3 of 4 files lifted). Also ran all ofbundler_cjs2esm,bundler_edgecase,bundler_minify,esbuild/default,esbuild/importstar.Background
ExportsKind::EsmWithDynamicFallbackFromCjs) turnsexports.a = 1intovar $a = 1; export { $a as a }so a CommonJS module imported from ESM can be tree-shaken and bound statically. Any use ofexportsthat is not a plain member pattern setscommonjs_named_exports_deoptimizedand the module falls back to the__commonJSwrapper.__proto__on an ordinary object is theObject.prototypeaccessor: assignment calls[[SetPrototypeOf]], a read returns the prototype. Neither creates or reads an own property, so no lifted binding can model it.__proto__export-name family. Keep __proto__ object keys as written when printing and synthesizing object literals #40812 covers the ESM faces (the__exportkey and the shorthand property); this change does not depend on it.Notes
Repro on 1.4.3-canary (d316760):
A
require()consumer was already correct because a required module is never unwrapped. A module that only readsexports.__proto__was already deoptimized by the "export that is never assigned" rule; the test includes that form so all four stay covered.