js_parser: make the ESM/CJS classification and the module/exports bindings agree

src/ast/expr.rs

@@ -2665,8 +2665,9 @@ pub enum Equality {
     Equal,
     NotEqual,
     /// `require.main === module` (or `!==`/`==`/`!=`, in either order); the
-    /// caller rewrites this to an import_meta_main node.
-    RequireMainAndModule,
+    /// caller rewrites this to an import_meta_main node. Carries the `module`
+    /// identifier's ref so the caller can drop its recorded usage.
+    RequireMainAndModule(Ref),
 }
 
 impl From<bool> for Equality {
@@ -2703,7 +2704,8 @@ impl EqlKindT for StrictEql {
 /// blanket-impl'd for every `P<...>` instantiation below.
 pub trait EqlParser {
     fn arena(&self) -> &Bump;
-    fn module_ref(&self) -> Ref;
+    /// Does this identifier ref name the CommonJS `module` object?
+    fn is_commonjs_module_ref(&self, ref_: Ref) -> bool;
 }
 // `impl EqlParser for P<...>` lives in `bun_js_parser` (next to `P`).
 
@@ -2834,8 +2836,8 @@ impl Data {
                 // always re-ordered to the right side.
                 if matches!(right, Data::ERequireMain) {
                     if let Some(id) = left.as_e_identifier() {
-                        if id.ref_.eql(p.module_ref()) {
-                            return Equality::RequireMainAndModule;
+                        if p.is_commonjs_module_ref(id.ref_) {
+                            return Equality::RequireMainAndModule(id.ref_);
                         }
                     }
                 }

src/ast/symbol.rs

@@ -116,6 +116,10 @@ bitflags::bitflags! {
         /// Renaming can also break any identifier used inside a "with" statement.
         const MUST_NOT_BE_RENAMED = 1 << 2;
 
+        /// The parser already warned that this unbound `module`/`exports` is
+        /// assigned to inside an ECMAScript module. One warning per variable.
+        const DID_WARN_ABOUT_COMMONJS_IN_ESM = 1 << 3;
+
         const REMOVE_OVERWRITTEN_FUNCTION_DECLARATION = 1 << 4;
 
         /// Used in HMR to decide when live binding code is needed.
@@ -143,6 +147,7 @@ macro_rules! symbol_flag_accessors {
 symbol_flag_accessors! {
     must_start_with_capital_letter_for_jsx, set_must_start_with_capital_letter_for_jsx => MUST_START_WITH_CAPITAL_LETTER_FOR_JSX;
     must_not_be_renamed, set_must_not_be_renamed => MUST_NOT_BE_RENAMED;
+    did_warn_about_commonjs_in_esm, set_did_warn_about_commonjs_in_esm => DID_WARN_ABOUT_COMMONJS_IN_ESM;
     remove_overwritten_function_declaration, set_remove_overwritten_function_declaration => REMOVE_OVERWRITTEN_FUNCTION_DECLARATION;
     has_been_assigned_to, set_has_been_assigned_to => HAS_BEEN_ASSIGNED_TO;
 }

src/js_parser/lib.rs

@@ -247,9 +247,17 @@ impl<'a, const IS_TS: bool, const SCAN: bool> bun_ast::expr::EqlParser
     fn arena(&self) -> &bun_alloc::Arena {
         self.arena
     }
-    #[inline]
-    fn module_ref(&self) -> Ref {
-        self.module_ref
+    fn is_commonjs_module_ref(&self, ref_: Ref) -> bool {
+        if ref_.eql(self.module_ref) {
+            return true;
+        }
+        if !ref_.is_symbol() {
+            return false;
+        }
+        // In a file with ESM exports `module` is an unbound global. Bun still
+        // rewrites `require.main === module` to `import.meta.main` there.
+        let symbol = &self.symbols[ref_.inner_index() as usize];
+        symbol.kind == bun_ast::symbol::Kind::Unbound && symbol.original_name.slice() == b"module"
     }
 }
 

src/js_parser/p.rs

@@ -269,6 +269,16 @@ pub struct P<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> {
     // Used for forcing CommonJS
     pub(crate) has_with_scope: bool,
 
+    /// The source has an `export` statement or a top-level `await`. Set before
+    /// the visit pass, so it never sees the `export` keyword that unwrapping
+    /// `exports.foo = ...` to ESM synthesizes during the visit. This decides
+    /// whether `module` and `exports` are the CommonJS bindings or unbound
+    /// globals. The module type (`.mjs`, `"type": "module"`) does not count: a
+    /// file with only CommonJS syntax is CommonJS whatever its extension says.
+    pub(crate) has_esm_exports_syntax: bool,
+    /// `has_esm_exports_syntax`, or the module type is ESM. Use this when the
+    /// question is "is this file certainly ESM?", for example to decide
+    /// whether a direct `eval` could reach `module` or `exports`.
     pub(crate) is_file_considered_to_have_esm_exports: bool,
 
     pub(crate) has_called_runtime: bool,
@@ -1438,6 +1448,73 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
         self.commonjs_named_exports_deoptimized && self.commonjs_named_exports.count() > 0
     }
 
+    /// `left` is the target of an assignment in a file with ESM exports. Warn
+    /// once per variable when it is `module.exports`, `module.exports.foo`, or
+    /// `exports.foo` and `module`/`exports` is an unbound global.
+    pub(crate) fn warn_about_commonjs_variable_in_esm(&mut self, left: &Expr) {
+        let Some(dot) = left.data.e_dot() else {
+            return;
+        };
+        let (id, loc, is_module_exports_property) = match dot.target.data {
+            // "module.exports = ..." and "exports.foo = ..."
+            js_ast::ExprData::EIdentifier(id) => (id, dot.target.loc, false),
+            // "module.exports.foo = ..."
+            js_ast::ExprData::EDot(inner) if inner.name == b"exports" => match inner.target.data {
+                js_ast::ExprData::EIdentifier(id) => (id, inner.target.loc, true),
+                _ => return,
+            },
+            _ => return,
+        };
+        if !id.ref_.is_symbol() {
+            return;
+        }
+
+        let symbol = &mut self.symbols[id.ref_.inner_index() as usize];
+        if symbol.kind != js_ast::symbol::Kind::Unbound || symbol.did_warn_about_commonjs_in_esm() {
+            return;
+        }
+        let name = symbol.original_name.slice();
+        let is_commonjs_variable = if is_module_exports_property {
+            name == b"module"
+        } else {
+            (name == b"module" && dot.name == b"exports") || name == b"exports"
+        };
+        if !is_commonjs_variable {
+            return;
+        }
+        symbol.set_did_warn_about_commonjs_in_esm(true);
+
+        let (why_range, why_text) = self.why_esm_note();
+        let source = self.source;
+        self.log().add_range_warning_fmt_with_note(
+            Some(source),
+            js_lexer::range_of_identifier(source, loc),
+            format_args!(
+                "The CommonJS \"{}\" variable is treated as a global variable in an ECMAScript module and may not work as expected",
+                bstr::BStr::new(name)
+            ),
+            format_args!("{}", why_text),
+            why_range,
+        );
+    }
+
+    /// The syntax that makes this file an ECMAScript module, for a diagnostic
+    /// note. Only valid when `has_esm_exports_syntax` is set.
+    pub(crate) fn why_esm_note(&self) -> (bun_ast::Range, &'static str) {
+        debug_assert!(self.has_esm_exports_syntax);
+        if self.esm_export_keyword.len > 0 {
+            (
+                self.esm_export_keyword,
+                "This file is considered to be an ECMAScript module because of the \"export\" keyword here:",
+            )
+        } else {
+            (
+                self.top_level_await_keyword,
+                "This file is considered to be an ECMAScript module because of the top-level \"await\" keyword here:",
+            )
+        }
+    }
+
     pub(crate) fn record_usage(&mut self, ref_: Ref) {
         if self.is_revisit_for_substitution {
             return;
@@ -2642,9 +2719,10 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             self.scope_order_to_visit = buf.into_bump_slice();
         }
 
-        self.is_file_considered_to_have_esm_exports = !self.top_level_await_keyword.is_empty()
-            || !self.esm_export_keyword.is_empty()
-            || self.options.module_type == options::ModuleType::Esm;
+        self.has_esm_exports_syntax =
+            !self.top_level_await_keyword.is_empty() || !self.esm_export_keyword.is_empty();
+        self.is_file_considered_to_have_esm_exports =
+            self.has_esm_exports_syntax || self.options.module_type == options::ModuleType::Esm;
 
         self.push_scope_for_visit_pass(js_ast::scope::Kind::Entry, loc_module_scope)?;
         self.fn_or_arrow_data_visit.is_outside_fn_or_arrow = true;
@@ -2744,10 +2822,21 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             .generated
             .ensure_unused_capacity(generated_symbols_count as usize * 3);
 
-        self.exports_ref =
-            self.declare_common_js_symbol(js_ast::symbol::Kind::Hoisted, b"exports")?;
-        self.module_ref =
-            self.declare_common_js_symbol(js_ast::symbol::Kind::Hoisted, b"module")?;
+        // CommonJS-style exports are only enabled if this isn't using ECMAScript-
+        // style exports. You can still use "require" in ESM, just not "module" or
+        // "exports". You can also still use "import" in CommonJS.
+        if !self.has_esm_exports_syntax {
+            self.exports_ref =
+                self.declare_common_js_symbol(js_ast::symbol::Kind::Hoisted, b"exports")?;
+            self.module_ref =
+                self.declare_common_js_symbol(js_ast::symbol::Kind::Hoisted, b"module")?;
+        } else {
+            // Not bound in the module scope: `module` and `exports` in the source
+            // resolve to unbound globals, and `exports_ref` stays free for the
+            // ESM namespace object.
+            self.exports_ref = self.new_symbol(js_ast::symbol::Kind::Hoisted, b"exports");
+            self.module_ref = self.new_symbol(js_ast::symbol::Kind::Hoisted, b"module");
+        }
 
         self.require_ref =
             self.declare_common_js_symbol(js_ast::symbol::Kind::Unbound, b"require")?;
@@ -8687,6 +8776,7 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
             macro_call_count: 0,
             hoisted_ref_for_sloppy_mode_block_fn: Default::default(),
             has_with_scope: false,
+            has_esm_exports_syntax: false,
             is_file_considered_to_have_esm_exports: false,
             has_called_runtime: false,
             symbol_uses,

src/js_parser/parse/parse_entry.rs

@@ -1574,9 +1574,16 @@ impl<'a> Parser<'a> {
         // Checked after `to_ast`, which marks TypeScript type-only imports unused.
         let mut reject_import_statements = false;
 
-        if p.is_deoptimized_commonjs() {
+        if p.has_esm_exports_syntax {
+            // `export` or top-level `await` in the source wins over everything
+            // else. `module` and `exports` were never bound in this file, so no
+            // CommonJS export could have been recorded.
+            debug_assert!(!p.is_deoptimized_commonjs());
+            exports_kind = js_ast::ExportsKind::Esm;
+        } else if p.is_deoptimized_commonjs() {
             exports_kind = js_ast::ExportsKind::Cjs;
-        } else if p.esm_export_keyword.len > 0 || p.top_level_await_keyword.len > 0 {
+        } else if p.esm_export_keyword.len > 0 {
+            // Synthesized by unwrapping `exports.foo = ...` to an ESM export.
             exports_kind = js_ast::ExportsKind::Esm;
         } else if uses_exports_ref || uses_module_ref || p.has_top_level_return || p.has_with_scope
         {

src/js_parser/repl_transforms.rs

@@ -26,11 +26,6 @@ impl<'a, const TS: bool, const SCAN: bool> P<'a, TS, SCAN> {
         parts: &mut BumpVec<'bump, js_ast::Part>,
         bump: &'bump Bump,
     ) -> Result<(), bun_alloc::AllocError> {
-        // Skip transform if there's a top-level return (indicates module pattern)
-        if self.has_top_level_return {
-            return Ok(());
-        }
-
         // Collect all statements
         let mut total_stmts_count: usize = 0;
         for part in parts.iter() {

src/js_parser/visit/visit_binary.rs

@@ -236,9 +236,9 @@ impl BinaryExpressionVisitor {
             }
             Op::Code::BinLooseEq => {
                 match data_eql::<false, TYPESCRIPT, SCAN_ONLY>(&e_.left.data, &e_.right.data, p) {
-                    Equality::RequireMainAndModule => {
+                    Equality::RequireMainAndModule(module_ref) => {
                         p.ignore_usage_of_runtime_require();
-                        p.ignore_usage(p.module_ref);
+                        p.ignore_usage(module_ref);
                         return p.value_for_import_meta_main(false, v.loc);
                     }
                     Equality::Equal => return p.new_expr(E::Boolean { value: true }, v.loc),
@@ -266,8 +266,8 @@ impl BinaryExpressionVisitor {
             }
             Op::Code::BinStrictEq => {
                 match data_eql::<true, TYPESCRIPT, SCAN_ONLY>(&e_.left.data, &e_.right.data, p) {
-                    Equality::RequireMainAndModule => {
-                        p.ignore_usage(p.module_ref);
+                    Equality::RequireMainAndModule(module_ref) => {
+                        p.ignore_usage(module_ref);
                         p.ignore_usage_of_runtime_require();
                         return p.value_for_import_meta_main(false, v.loc);
                     }
@@ -289,8 +289,8 @@ impl BinaryExpressionVisitor {
             }
             Op::Code::BinLooseNe => {
                 match data_eql::<false, TYPESCRIPT, SCAN_ONLY>(&e_.left.data, &e_.right.data, p) {
-                    Equality::RequireMainAndModule => {
-                        p.ignore_usage(p.module_ref);
+                    Equality::RequireMainAndModule(module_ref) => {
+                        p.ignore_usage(module_ref);
                         p.ignore_usage_of_runtime_require();
                         return p.value_for_import_meta_main(true, v.loc);
                     }
@@ -316,8 +316,8 @@ impl BinaryExpressionVisitor {
             }
             Op::Code::BinStrictNe => {
                 match data_eql::<true, TYPESCRIPT, SCAN_ONLY>(&e_.left.data, &e_.right.data, p) {
-                    Equality::RequireMainAndModule => {
-                        p.ignore_usage(p.module_ref);
+                    Equality::RequireMainAndModule(module_ref) => {
+                        p.ignore_usage(module_ref);
                         p.ignore_usage_of_runtime_require();
                         return p.value_for_import_meta_main(true, v.loc);
                     }
@@ -674,6 +674,15 @@ impl BinaryExpressionVisitor {
                         was_anonymous_named_expr,
                     );
                 }
+
+                // Warn about "module.exports = ..." or "exports.foo = ..." in a
+                // file that also has ESM exports: there is no CommonJS binding
+                // for `module` or `exports` in such a file, so the assignment
+                // goes to a global. Only assignments warn, since `typeof module`
+                // is a legitimate way to detect CommonJS.
+                if p.options.bundle && p.has_esm_exports_syntax && !p.is_control_flow_dead {
+                    p.warn_about_commonjs_variable_in_esm(&e_.left);
+                }
             }
             Op::Code::BinNullishCoalescingAssign | Op::Code::BinLogicalOrAssign => {
                 // Special case `{}.field ??= value` to minify to `value`

src/js_parser/visit/visit_stmt.rs

@@ -1531,20 +1531,21 @@ impl<'a, const TYPESCRIPT: bool, const SCAN_ONLY: bool> P<'a, TYPESCRIPT, SCAN_O
     ) -> Result<(), Error> {
         // Forbid top-level return inside modules with ECMAScript-style exports
         if p.fn_or_arrow_data_visit.is_outside_fn_or_arrow {
-            let where_ = if p.esm_export_keyword.len > 0 {
-                p.esm_export_keyword
-            } else if p.top_level_await_keyword.len > 0 {
-                p.top_level_await_keyword
-            } else {
-                bun_ast::Range::NONE
-            };
-
-            if where_.len > 0 {
-                p.log().add_range_error(
+            if p.has_esm_exports_syntax {
+                let (why_range, why_text) = p.why_esm_note();
+                p.log().add_range_error_fmt_with_note(
                     Some(p.source),
-                    where_,
-                    b"Top-level return cannot be used inside an ECMAScript module",
+                    js_lexer::range_of_identifier(p.source, stmt.loc),
+                    format_args!("Top-level return cannot be used inside an ECMAScript module"),
+                    format_args!("{}", why_text),
+                    why_range,
                 );
+            } else {
+                p.has_top_level_return = true;
+                // Only the CommonJS wrapper can contain this `return`, so
+                // `exports.foo = ...` must stay as-is instead of being
+                // unwrapped to an ESM export.
+                p.deoptimize_common_js_named_exports();
             }
         }
 

src/jsc/RuntimeTranspilerCache.rs

@@ -55,7 +55,9 @@ bun_core::declare_scope!(cache, visible);
 /// offsets picked by a header byte) plus a body of tagged records with
 /// u8/u16/u32 ids and implied slots dropped, instead of fixed u32 arrays.
 /// Version 27: ModuleInfo string table holds Latin-1 / UTF-16 bodies, not WTF-8.
-const EXPECTED_VERSION: u32 = 27;
+/// Version 28: A top-level `return` makes a file CommonJS. Older entries
+/// classified such a file as ESM and hold its unwrapped output.
+const EXPECTED_VERSION: u32 = 28;
 
 /// Source files smaller than this are not written to / read from the on-disk
 /// transpiler cache. Originally 50 KiB, which excluded almost every file in a