Conversation
…nts or new.target With ES module output, the parser lifts a CommonJS file out of its __commonJS wrapper. A file that reads `arguments` or `new.target` at the module scope then fails at load, because module code has neither. Both lifts in parse_entry.rs now keep the wrapper for such a file. The visitor records a `new.target` outside every function and class element with the nesting rule that `this` already uses.
|
Status: CI is green (Buildkite build 115118 passed). The diff is ready for a maintainer. Reproduced on main (f04caca) and on 1.4.3 canary with the four files in the Notes of the PR body:
The one red check, |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. WalkthroughThe parser now tracks top-level ChangesCommonJS wrapper semantics
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change preserves CommonJS wrapper bindings when required, with no concrete unresolved merge risk identified. 🚥 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.
LGTM — narrow, well-tested fix that follows the existing sibling-flag pattern at both lift gates.
What was reviewed:
- Verified
is_this_nestedhas the same scoping asnew.target(set invisit_func, class static blocks, and class property values/initializers; not set for arrows or before computed keys are visited), so the flag is accurate. - Confirmed the
argumentscheck mirrors theuses_exports_ref/uses_module_refpattern (module-scope unbound symbol + use count), and the third lift atparse_entry.rs:1486genuinely can't contain either read given its single-statementmodule.exports = require("literal")shape. - Tests cover the variant matrix (direct/typeof/arrow/computed-key, both gates, and a negative still-lifted case) and are appended to the right file.
Extended reasoning...
Overview
This PR fixes a bundler bug where the CJS→ESM lifting optimization would unwrap CommonJS files that read top-level arguments or new.target, producing bundles that fail at load time (ReferenceError / SyntaxError) despite a successful build. It adds one boolean field to the parser state (has_top_level_new_target), sets it in the e_new_target visitor gated on !fn_only_data_visit.is_this_nested, adds a small helper reads_wrapper_arguments_or_new_target() that also checks for an unbound module-scope arguments with nonzero use count, and consults that helper at both applicable lift gates in parse_entry.rs. Four itBundled tests are appended to test/bundler/bundler_cjs2esm.test.ts.
Security risks
None. This is a bundler correctness change: it only makes two existing deoptimization gates slightly more conservative (keeps the __commonJS wrapper in more cases). No untrusted input parsing changes, no auth/crypto/permissions, no allocation or FFI.
Level of scrutiny
Low-to-medium. The change is ~20 lines of logic plus ~110 lines of tests, and it slots directly alongside three existing sibling flags (has_top_level_return, has_with_scope, has_top_level_function_merged_with_var) checked at the same gates — the pattern is well established. The one non-obvious design choice is reusing is_this_nested to detect "top-level new.target"; I traced every writer of that flag in visit/mod.rs and confirmed it is set exactly where new.target gets its own binding (regular functions, class static blocks, class property values/initializers) and left alone for arrows and computed class keys, which is precisely new.target's scoping. The arguments side reuses the same module-scope unbound-symbol + use-count mechanism as uses_exports_ref/uses_module_ref, so dead references and function-local arguments are handled the same way.
Other factors
REVIEW.md's "fix the whole bug class" and "cover the variant matrix" are both satisfied: both lift gates are patched, the third redirect gate is provably immune by shape (acknowledged in the PR notes), and tests cover direct/typeof/arrow/computed-class-key, the React-specific unwrap path, and a negative case proving nested-function reads and dead top-level arguments still lift. No CODEOWNERS cover the changed paths, no outstanding third-party reviews, and the bug hunt exited on dry_streak with no findings. Comments added are short and load-bearing.
|
Updated 5:47 AM PT - Sep 13th, 2026
✅ @robobun, your commit 144ad5f5edfade3ac8080bf413cd5eb671c122b0 passed in 🧪 To try this PR locally: bunx bun-pr 42595That installs a local version of the PR into your bun-42595 --bun |
This branch has not been deployed
2 participants
Problem
bun buildlifts a CommonJS file out of its__commonJSwrapper. If the file reads top-levelargumentsornew.target, the bundle fails at load:ReferenceError: arguments is not definedorSyntaxError: new.target is only valid inside functions or static blocks.The build exits 0.src/js_parser/parse/parse_entry.rs(:1462,:1608) do not check for these reads.Fix
P::reads_wrapper_arguments_or_new_target(src/js_parser/p.rs:2020) is true when module-scope code uses anargumentsthat the file does not declare, or has anew.targetoutside every function and class element. The visitor finds the latter with the rulevalue_for_thisuses forthis.exports.foolift takes its existing deoptimization, and theexport *lift does not run. The file keeps its wrapper, a regular function (generateCodeForFileInChunkJS.rs:590) that has both bindings.test/bundler/bundler_cjs2esm.test.ts(four new tests, three fail on 1.4.3 canary). The Notes list the other suites..jsfiles).Background
exports.foo = valueintovar $foo = valueplus an ES export, so the file becomes module code. It runs only for ES module output.argumentsandnew.targetare that function's. Arrow functions and computed class keys read them from the enclosing scope.return. This PR does not change it.Notes
No GitHub issue reports this. A differential test of
bun runagainstbun buildoutput found it.The build prints no warning.
typeof argumentsalone changes from"object"to"undefined"with no error at all. 1.4.0 kept the wrapper for a default import, because the linker put the wrapper back for that import form. #41162 made a default import of a lifted file bind to its namespace, so the linker no longer does that.The parser puts an identifier that the file does not declare in the module scope as an unbound symbol. The check looks up
argumentsthere and reads its use count, the same wayuses_exports_refanduses_module_refread theirs.Repro (1.4.1, 1.4.2, 1.4.3 canary, and main):
bun build e1.mjs --target=bun --outfile=o.mjs && bun o.mjsbun run, nodearguments: 5 objectnew.target: undefinedarguments: 2 objectnew.target: undefinedReferenceError: arguments is not definedSyntaxError: new.target is only valid inside functions or static blocks.arguments: 2 objectnew.target: undefinedThe count is 2 in a bundle because
__commonJScalls the wrapper with(exports, module).Also checked by hand on this branch:
--target=nodeand--target=browser,--minify,--splitting,--format=iife, a named import,require()of the file,import()of the file with and without--splitting, and the file as the entry point. All print the 1.4.0 result.--format=cjsnever lifted.Reach: two scans of published npm packages during self-review (919 packages with 62,704 files, and about 1,127 packages with 30,715 files) found no file that takes the new path. So the bundle of every measured package is unchanged, and the only bundles that change are ones that fail to load today.
What the check does not do:
.jsfile with no CommonJS marker and nopackage.json"type"is still module code, sobun plain.jswith a top-levelnew.targetstill fails. That is the plain-script decision that bundler: keep the CommonJS wrapper when a var has the name of a top-level function #41269 already lists as a follow-up.argumentsreference (if (false) arguments) does not keep the wrapper, because the use count ignores dead code and the bundler removes the branch. This is howuses_exports_refanduses_module_refwork.new.targethas no symbol, so a flag records it, and the flag also counts dead code. Anew.targetthat stays in the output is a parse error, so the flag is conservative.arguments(var arguments = 1) is not affected. That binding is a strict-mode error in an ES module bundle with or without the wrapper. Reserve "arguments", "eval" and "await" as binding names in bundle output #34361 (open) renames it.module.exports = require("x")redirect (parse_entry.rs:1486), needs that statement to be the only one in the file, so it cannot contain either read.evalthat reads them already keeps the wrapper.Tests:
USE_SYSTEM_BUN=1(1.4.3 canary)bun bdwith this PRTopLevelArgumentsKeepsWrapperTopLevelNewTargetKeepsWrapperArgumentsAndNewTargetOfAFunctionAreStillLiftedReactSpecificUnwrappingTopLevelArgumentsOrNewTargetKeepsWrapperOther suites that pass on this branch (debug, ASAN): all of
bundler_cjs2esm,bundler_cjs,bundler_edgecase,esbuild/default,bundler_regressions,bundler_npm.[human-review] gate passed · iteration 0 · 4 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file