Conversation
…dings agree A file with an `export` statement or a top-level `await` is an ECMAScript module. The parser still bound `module` and `exports` in such a file to the CommonJS wrapper symbols. The bundler never declares those symbols for an ESM file, so the output referenced an undeclared `module_<file>` and `exports` aliased the ESM namespace object. When the file also assigned to `exports.foo`, the CommonJS named-export path classified it as CommonJS and the `export` statements were dropped without a diagnostic, while the runtime loaded the same file as ESM. Bind `module` and `exports` as CommonJS symbols only when the file has no `export` or top-level `await`. Otherwise they are unbound globals, as in esbuild, and the bundler warns once per variable when one of them is assigned to. `require.main === module` is still rewritten to `import.meta.main` in such files. The classification keeps Bun's content-based rule: a `.mjs` or `"type": "module"` file with only CommonJS syntax stays CommonJS. `has_top_level_return` was read but never set. Set it in `s_return` when the return is outside a function, so a file whose only CommonJS trait is a top-level `return` gets the CommonJS wrapper instead of a bare `return` in the output. The unwrapping of `exports.foo = ...` to an ESM export is turned off for such a file. A top-level `return` next to `export` or top-level `await` reports the error at the `return` with a note that points at the ESM syntax.
Contributor
|
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 selected for processing (8)
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review. WalkthroughThe parser now prioritizes explicit ESM syntax and top-level ChangesESM and CommonJS classification
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The parser changes align ESM/CJS classification with module and exports bindings, with the supplied targeted checks passing; no actionable merge-blocking risk remains. 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
Full details: Out of Scope Changes checkExplanation The PR includes substantial ESM module/exports binding changes, warnings, and related tests that are not required by the directly linked issue Full details: Description checkExplanation The description clearly explains the problem, implementation, scope, verification, and regression coverage. It does not use the exact template headings, but it provides the required information in equivalent sections.
Comment |
Collaborator
Author
|
Updated 12:36 AM PT - Aug 29th, 2026
❌ @robobun, your commit 56bb0dd has 1 failures in
🧪 To try this PR locally: bunx bun-pr 40840That installs a local version of the PR into your bun-40840 --bun |
This was referenced Aug 28, 2026
Collaborator
Author
|
Status: ready for review. Reproduced on 1.4.1 with the four cases from the report (mixed CI on 56bb0dd (build 108188): 181 of 182 jobs passed. The one red lane is |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…efine in ESM files Carries over the distinct cases from #37371. In a file with ESM exports, `module.id`, `module.require()` and `exports.foo` print as written, minified identifiers do not take the names `module` and `exports`, and `--format=cjs` output runs under node. `Bun.Transpiler` applies `define` for `module` and `exports` only in such files, and keeps `module.require()` as written there.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Collaborator
Author
|
Also fixes #8908: #35638 and #37371 are closed as superseded. Their tests pass on this branch, except the three #35638 tests that expected a top-level |
Collaborator
Author
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 3, 2026
…evel function (#41269) ### Problem - With ES module output, `bun build` lifts a CommonJS file out of its `__commonJS` wrapper. With a top-level `function top() {}` and `var top`, the bundle throws: `SyntaxError: Cannot declare a var variable that shadows a let/const/class variable: 'top'.` - In a function, the two declarations share one binding. In module code, a top-level function is lexical, so the `var` is an early error. The lifts (`src/js_parser/parse/parse_entry.rs:1441`, `:1605`) do not check this. - A named import fails in 1.4.0. Since #41162, a default import fails too. ### Fix - `declare_symbol` and `hoist_symbols` now set `has_top_level_function_merged_with_var` when a `var` and a function declaration in the module scope share one symbol. A repeated `var` or function, or a merge inside a nested function, does not set it. - With the flag, the `exports.foo` lift takes its existing `needs_decl_count > 0` deoptimization, and the `export *` lift does not run. That leaves `REDECLARED_BY_VAR` (#41251) unused, so this PR removes it. - Verified: `test/bundler/bundler_cjs2esm.test.ts` (three new tests, one changed), and 13 suites listed in the Notes. - Self-reviewed: 11 concerns raised, 8 addressed. The other 3 are older bugs, in the Notes. ### Background - The lift turns `exports.foo = value` into `var $foo = value` plus an ES export, only for ESM output (`src/bundler/ParseTask.rs:2503`). - A symbol merge binds two declarations to one symbol. - For a sloppy-mode `{ function f() {} }`, the parser emits `let f2 = function () {}; var f = f2;`. That `var` conflicts too. <details><summary>Notes</summary> I found this by reading the output of a lifted file. No issue reports it. Repro (a 1.4.1 canary, and main): ```js // lib.cjs function top() { return "declaration"; } var top = function () { return "var"; }; exports.top = top; ``` ```js // entry.mjs import { top } from "./lib.cjs"; console.log(top()); ``` `bun build ./entry.mjs --target=bun --outfile=o.js && bun o.js` throws. Node prints `var`. The same error occurs for the reverse order, for a `var` in an `if` block, for a sloppy block-level function next to a top-level one, and for `function*`. The first new test covers each case, and a default import. `--format=cjs` and `--format=iife` do not lift, so they are not affected. Test results: | Test | `USE_SYSTEM_BUN=1` (1.4.1 canary, before #41162) | debug build of main | `bun bd` with this PR | | --- | --- | --- | --- | | `VarWithTheNameOfATopLevelFunctionKeepsWrapper` | fail (1 of 6 wrappers) | fail (0 of 6) | pass | | `OtherRedeclarationsAreStillLifted` | pass | pass | pass | | `ReactSpecificUnwrappingVarWithTheNameOfAFunctionKeepsWrapper` | pass (predates #41188) | fail | pass | On the debug build of main, the default-import case and the `export *` case both throw the `SyntaxError` at load. `OtherRedeclarationsAreStillLifted` guards the checks. I removed the module-scope check in `declare_symbol` on a local build, and this test failed. This PR also changes `MethodCallKeepsThisWhenAVarRedeclaresTheFunction` from #41251. That test read the printed calls, because the bundle did not load. The file now keeps its wrapper, so the test runs the bundle. The output `lib lib lib lib` shows that each call gets `module.exports` as `this`. On main this test fails too. `REDECLARED_BY_VAR` marked a function declaration that a `var` merged into. Its only reader, `mark_commonjs_exports_that_ignore_this`, runs only for a file that is not deoptimized. A file with that merge is now deoptimized, so the flag can no longer change the output. The #41251 tests still pass. Other suites that pass on this branch: `bundler_cjs`, `bundler_barrel`, `bundler_splitting`, `bundler_minify`, `bundler_dynamic_import_dce`, `bundler_regressions`, `bundler_edgecase`, `esbuild/default`, `esbuild/dce`, `esbuild/importstar`, `esbuild/importstar_ts`, `esbuild/ts`, and `transpiler/transpiler.test.js`. Follow-ups that this PR does not change: bun runs a `.js` file with no module syntax and no CommonJS features as module code. So `bun plain.js` fails on the same two declarations. An HTML entry has the same problem, because the bundler emits a classic `<script src>` as `type="module"`. That is a separate decision about plain scripts. A top-level `return`, `new.target`, or `arguments` in a lifted file also fails at load. #40840 covers the `return` case. Alternative that I did not take: keep the lift, and print the merged `var top = x` as the assignment `top = x`. Module code allows an assignment to a function binding. That keeps tree shaking for these files, but each `var` form (declaration lists, `for` heads, destructuring) needs a rewrite. I found no real package with this pattern, so the wrapper costs nothing in practice. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 4 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 3 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_cjs2esm.test.ts" bun test v1.4.1 (a6c4cc2) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [720.52ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [336.99ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [401.76ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [382.10ms] (pass) bundler > cjs2esm/ExportsFunction [319.42ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [389.00ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [320.29ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [358.71ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [446.21ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [444.19ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [401.64ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [511.62ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [593 ... (truncated) release without fix: 45 FAILED bun test v1.4.1-canary.1 (a6c4cc2) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [19.88ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [9.13ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [8.11ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [7.37ms] (pass) bundler > cjs2esm/ExportsFunction [7.41ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [7.45ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [7.14ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [8.64ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [9.85ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [9.44ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [9.54ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [10.98ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [10.61ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRuntimeCondition [9.39ms] (pass) bundler > cjs2esm/UnwrappedModuleRequireAssigned [8.74ms] (pass) bundler > cjs2esm/UnwrappedModuleReq ... (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.1 (a6c4cc2) test/bundler/bundler_cjs2esm.test.ts: (pass) bundler > cjs2esm/ModuleExportsFunction [719.29ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJSModuleRef [405.07ms] (pass) bundler > cjs2esm/ImportNamedFromExportStarCJS [405.58ms] (pass) bundler > cjs2esm/BadNamedImportNamedReExportedFromCommonJS [315.27ms] (pass) bundler > cjs2esm/ExportsFunction [320.61ms] (pass) bundler > cjs2esm/ModuleExportsFunctionTreeShaking [324.47ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequire [321.18ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPoint [366.12ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPoint [399.89ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireEntryPointImportedByEntryPointSplitting [380.48ms] (pass) bundler > cjs2esm/ModuleExportsEqualsRequireTwoEntryPoints [336.30ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvProduction [464.06ms] (pass) bundler > cjs2esm/ModuleExportsBasedOnNodeEnvDevelopment [502 ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision cf871f6 features baseline 23 deps, 131 codegen, 1172 objects in 983ms ninja: Entering directory `/workspace/bun/build/release' [1/1244] install /workspace/bun bun install v1.4.1-canary.1 (a6c4cc2) Checked 25 installs across 62 packages (no changes) [5.00ms] [2/1244] gen bindgenv2 [3/1244] gen .bind.ts → GeneratedBindings.cpp [4/1244] fetch zlib [zlib] up to date [5/1244] fetch libjpeg-turbo [libjpeg-turbo] up to date [6/1217] fetch tinycc [tinycc] up to date [7/1216] gen bake.{client,server,error}.js -> bake.client.js, bake.server.js, bake.error.js [8/1216] install /workspace/bun/packages/bun-error bun install v1.4.1-canary.1 (a6c4cc2) Checked 1 install across 2 packages (no changes) [1.00ms] [9/1216] gen ErrorCode+*.h [10/1216] gen JSBuffer.lut.h Generating /workspace/bun/build/release/codegen/JSBuffer.lut.h from /workspace/bun/src/jsc/bindings/JSBuffer.cpp [11/1216] install /workspace/bun/src/node-fallbacks bun install v1.4.1-canary.1 (a6c4cc2) Checked 111 instal ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/ast/symbol.rs | 4 - src/js_parser/p.rs | 31 ++++++-- src/js_parser/parse/parse_entry.rs | 3 +- test/bundler/bundler_cjs2esm.test.ts | 137 +++++++++++++++++++++++++++++++++-- 4 files changed, 154 insertions(+), 21 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/ast/symbol.rs 1 2 35 src/js_parser/p.rs 8 12 35 src/js_parser/parse/parse_entry.rs 3 2 34 test/bundler/bundler_cjs2esm.test.ts 6 7 34 ``` </details> <!-- robobun:evidence:end -->
Jarred-Sumner
pushed a commit
that referenced
this pull request
Sep 23, 2026
#43745) ### Problem - 16 struct fields in 11 crates are never read. Two options are never set: `BufferWriter.append_null_byte` is never `true`, `TransposeState.import_record_tag` is never `Some`. - No lint reports them. rustc's `dead_code` pass counts `x.f = v` as a use of `f`. hawk counts the initializer as a reference. ### Fix - Delete each field with its initializers and stores, and the two branches that the options guard. 18 files, 105 lines removed. - Candidates come from a compiler probe: `#[deprecated]` on 12,134 struct fields, `cargo check --force-warn deprecated` for Linux, Windows, and macOS, then a syn pass marks each use as read or write. 134 fields have no read. Most stay (Notes). - Verified: `bun bd`, `bun run rust:check-all` (12 of 12 targets), `cargo check --workspace --all-targets`, and the tests in the Notes. - Self-reviewed: 4 concerns raised, 2 addressed. Rejected as outside a deletion: the now vestigial `written_without_trailing_zero()` and an older stale comment. ### Background - `MultiArrayList<T>` keeps each field of `T` in its own column. Code reads a column by name as a string (`items::<"name", _>()`), so the compiler sees no read. Those fields stay. - `BufferWriter` is the JS printer's output buffer. `done()` could append a NUL byte. No caller asks for it. ### Downsides - `bun_core::output::Source` becomes `Send + Sync`, because its raw pointer fields are gone. It lives in a `thread_local!` only. - The `bun_resolver::Result` in `_resolve` now drops when `_resolve` returns. It has no `Drop` effect: paths are borrowed, fds are `Copy`. <details><summary>Notes</summary> #### Removed, one line each - `bun_core::output::Source`: `buffered_stream`, `buffered_error_stream`, `stream`, `error_stream` (`*mut io::Writer`). `init()` cached them, nothing read them. The accessors of the same names return `*_backing.new_interface()`, which is a pointer cast with no side effect. - `bun_bundler`: `InputFileInfo.import_count` (`MetafileBuilder.rs`). - `bun_install`: `SecurityScanSubprocess.stderr_data` (an empty `Vec` that is never filled, the scanner's stderr is inherited), `PackageManager.root_progress_node`. The `progress.start(b"", 0)` call stays, because it starts the progress root. - `bun_js_parser`: `PropertyOpts.async_range`, `ScanPassResult.approximate_newline_count`, `TransposeState.import_record_tag`. #16624 replaced the only setter of the tag with `import_loader`, which stays. - `bun_jsc`: `VirtualMachine.has_terminated` (its readers were debug panics in `enqueue_task_concurrent`, which #37075 removed), `ResolveFunctionResult.result` (seven stores, no read). Nothing borrows from the stored value: `path` and `query_string` point into the resolver's arena and the specifier. - `bun_js_printer`: `BufferWriter.append_null_byte`, with the seven `= false` stores in `bun_jsc` and `bun_runtime`. - `bun_libarchive`: `BufferReadStream.reading`. - `bun_runtime`: `WindowsState.is_server` (`ipc.rs`, Windows only). Its last reader went away in #18688, before the Rust port. - `bun_csrf`: `GenerateOptions.encoding`. `csrf_jsc.rs` encodes the token itself, and `VerifyOptions.encoding` stays. - `bun_resolver`: `DataURL.url` (always `String::EMPTY`). - `bun_s3_signing`: `S3CredentialsWithOptions.virtual_hosted_style`. No code names it. Every reader uses `credentials.virtual_hosted_style` on the inner `S3Credentials`. #### Passed the probe, kept on purpose - `MultiArrayList` columns: fields of `JSMeta`, `File`, `InputFile`, `BundledAst`, `Entry`, `Node`, `WatchItem`, `LineOffsetTable`, `ServerComponentBoundary`, and others. - Owners of memory that other fields borrow: `ParseResult.source_contents_backing`, `LinkerContext.unique_key_buf`, `OutputFile.owned_src_path_text`, `HTTPResponseMetadata.owned_buf`, `PackageJSON.source_contents` and `json_tape`, `MatchedRoute.pathname_backing`, `SignResult.content_md5`, `KEventWaker.machport_buf`. - Values whose drop has an effect: `Repl.last_error` (a GC protect), `SecurityScanSubprocess.process`, `Watcher.thread`. - Options that are unfinished features or open bugs, not dead code: `md::Options.hard_soft_breaks` and `underline` (#39495, #39493), `jsc::virtual_machine::Options.dns_result_order` (#40703), `P.has_top_level_return` (#40840), `ReactRefresh.last_hook_seen` and `force_reset`, `DebugOptions.dump_environment_variables` (the `--dump-environment-variables` flag is parsed and ignored). - Kept by a comment in the source: `AllocatorConfiguration.long_running`, `DumpStackTraceOptions.skip_*`, `NameOfSymbol.has_property_key_comment`. - Removal needs more than a deletion: `PostgresSQLQuery` `Flags.is_done` (set by the JS `done()` call), `WorkerPipe.done` (leaves two empty reader callbacks), `WTFTimer.repeat` (leaves an unused FFI parameter), `MaxHeapAllocator.len` (leaves an empty `reset()`), `ReadToEndResult.err` (callers drop read errors, which looks like a bug), `Subcommand::Pack` with `PACK_PARAMS` (`bun pm pack` runs as `Subcommand::Pm`, so the pack help text is unreachable). #### Self-review Three independent read-only passes tried to prove each deletion wrong (readers through raw pointers, `offset_of!`, C++ layout mirrors, macros, other `cfg`s, unit tests, the Zig originals, drop effects). None found a reader. Concerns raised: the `Send + Sync` change of `Source` (now in Downsides), the style of the kept `progress.start` call (now `let _ =`, as in `install_with_manager.rs`), `BufferWriter::written_without_trailing_zero()` (nine callers, it only strips NUL bytes that the printer never appends now, a follow-up), and a comment above `ResolveFunctionResult.path` that names a function which no longer exists (it predates this change). #### Other scans of this run, all clean or already in an open pull request - A relink of the debug binary with `--gc-sections --print-gc-sections`, and a zero-mention identifier index over `src/`, `packages/`, `scripts/` and `build/debug/codegen/`. - `macro_rules!` without an invocation, Cargo features that nothing enables, `#if 0` and never-true `#if` blocks in the C++ bindings, patches that no dependency script applies, unused exports under `scripts/`, `tsc --noUnusedLocals` over `src/js` and `scripts/`. - A syn scan for enum variants that no expression constructs. The hits are FFI code tables, derive-constructed variants, or already claimed. #### Overlap with the open dead-code pull requests - Every removed line was checked per file against the diffs of the 36 open dead-code pull requests. None of them deletes these lines. Several touch the same files in other places (`output.rs`, `VirtualMachine.rs`, `parser.rs`, `p.rs`, `js_printer/lib.rs`, `jsc_hooks.rs`, `PackageManager.rs`, `data_url.rs`). #### Tests run with the debug build `test/js/bun/util/csrf.test.ts` (31 pass), `test/bundler/metafile.test.ts` (65 pass), `test/js/bun/archive.test.ts` (108 pass), `test/bundler/transpiler/transpiler.test.js` (222 pass), `test/js/bun/resolve/import-empty.test.js`, `test/js/bun/resolve/esModule.test.ts`, `test/cli/install/bun-install-security-provider.test.ts` (43 pass), `test/internal/source-lints/` (195 pass). Also a `data:` URL import, and `bun install` under a pty so that the progress root starts. No test is added. The change deletes fields that nothing reads, so there is no behavior to assert, and `test/internal/source-lints/CLAUDE.md` asks for no tests that pin dead symbols. </details>
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
export(or top-levelawait) that also assignsmodule.exportsorexports.foois bundled as CommonJS and itsexportstatements vanish with no diagnostic, whilebun runloads it as ESM (SyntaxError: Export named 'b' not found). An ESM file that only mentions them gets an undeclaredmodule_<file>in the bundle (ReferenceError: module_tla is not defined) andtypeof exportsis"object".prepare_for_visit_pass(src/js_parser/p.rs) bindsmodule/exportsto the CommonJS wrapper symbols in every file.P::has_top_level_returnis read by theexports_kindselection (parse_entry.rs) but never set, so a top-levelreturnis a SyntaxError underbun runand a barereturninbun buildoutput. Fixes top-levelreturnshould hint to the transpiler that a module is commonjs / bundler can emit top-level return #8908.Fix
module/exportsas CommonJS symbols only when the file has noexportand no top-levelawait. Otherwise they are unbound globals, as in esbuild, and the bundler warns once per variable on assignment. The file stays ESM, so a missing name is aNo matching exporterror.s_returnsetshas_top_level_returnoutside a function and turns off theexports.foo = ...to ESM unwrapping, so the file gets the CommonJS wrapper. Next toexport/top-levelawaitit errors at thereturnwith a note pointing at the ESM syntax.require.main === modulestill becomesimport.meta.mainin ESM files. A.mjsor"type": "module"file with only CommonJS syntax stays CommonJS (Bun's content-based rule).test/cli/run/esm-cjs-detection.test.ts(new, 31 tests; 13 fail on 1.4.1),test/bundler/bundler_cjs.test.ts(22 new; 17 fail before),test/bundler/esbuild/default.test.ts,test/bundler/transpiler/transpiler.test.js. The notes list the other suites.Background
exports_kindis the parser's verdict per file:Esm,Cjs, orNone. The linker wrapsCjsfiles in__commonJS(function(exports, module) {...})and stripsexportstatements inside the wrapper. It assumes anEsmfile has no user-visiblemodule/exports.exports_refis both theexportsparameter of a CommonJS wrapper and the ESM namespace object (var exports_<file> = {}).module_refis only declared for wrapped files. User code in an ESM file must not resolve to either symbol.exports.foo = 1tovar $foo = 1; export { $foo as foo }when bundling to ESM. That path synthesizes anexportkeyword during the visit, so the binding decision is taken before the visit pass and a top-levelreturnhas to deoptimize it.returnshould hint to the transpiler that a module is commonjs / bundler can emit top-level return #8908 covers only the top-levelreturn. The binding and thereturnclassification live in the same parser code, so both are fixed here. Supersedes parser: treat top-level return as a CommonJS hint #35638 (return only) and Leave module/exports unbound in files with ESM exports #37371 (bindings only).Notes
Repros, before and after (release 1.4.1 vs. this branch):
A fifth case found while testing:
exports.foo = 1; if (x) return; exports.bar = 2;failed to bundle withTop-level return cannot be used inside an ECMAScript module, because the unwrapping of the first statement had synthesized theexportkeyword before thereturnwas visited. It now bundles as a CommonJS wrapper with both assignments.Runtime output is unchanged for ESM files (the printer already printed
module/exportsby their original names), except files with a top-levelreturn.RuntimeTranspilerCacheEXPECTED_VERSIONis bumped so older entries for those files are not replayed.--format=iifewith a CommonJS entry point never invokes the wrapper; that is a separate pre-existing bug (#37843 is open for it), so the iife top-level-return test checks the output shape instead of running it.The new runtime test matrix:
bun runvsbun buildfor esm/cjs/iife, importers using default, named and namespace imports,.js/.mjs/.cjswith"type"none/module/commonjs.Tests carried over from #37371 in the second commit:
module.idandmodule.require()reads, minified identifiers,--format=cjsunder node, andBun.Transpilerdefineformodule/exports(applies only in files with ESM exports).test/bundler/esbuild/default.test.ts:WarnCommonJSExportsInESMBundlenow asserts the two warnings, andTopLevelReturnForbiddenImport/TopLevelReturnForbiddenImportAndModuleExportsare no longertodo.Other suites run locally (debug, ASAN):
test/bundler/esbuild/*,bundler_cjs,bundler_edgecase,bundler_cjs2esm,bundler_bun,bundler_browser,bundler_regressions,bundler_splitting,bundler_npm,bundler_minify,bun-build-api,cli.test.ts,test/bundler/transpiler/,test/cli/run/,test/js/bun/resolve/,test/js/bun/repl/,test/js/node/module/,test/js/bun/test/,test/js/third_party/es-module-lexer. The only failures were timeouts of slow leak/perf tests under the loaded container and PTY-less REPL terminal tests; each passes with a longer timeout or on the release binary.[review] gate passed · iteration 2 · 14 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 2 passed · 0 rejected · iteration 2
evidence per changed file