fix(cjs_wrap): flat-emit module.exports = <Class> that closes over a top-level binding (#4933)

crates/perry/src/commands/compile/cjs_wrap/hoist_classes.rs

@@ -565,6 +565,150 @@ fn collect_top_level_let_const_var_names(source: &str) -> Vec<String> {
     names
 }
 
+/// Issue #4933 — collect the names of every **top-level** `class <Name>`
+/// declaration anchored at column 0, regardless of whether it would hoist.
+/// `extract_top_level_class_decls` only returns the classes it actually
+/// hoists (it refuses any whose body references an IIFE-local binding,
+/// #2310), so a `module.exports = StackUtils` whose `StackUtils` reads a
+/// top-level `const natives = …` is invisible to the hoisted-name list.
+/// The flat-emit path (wrap.rs) needs to know the assignment target is a
+/// real top-level class before it drops the IIFE, hence this companion
+/// scan. Uses the same column-0 anchor + identifier rule as the hoist scan.
+pub fn top_level_class_names(source: &str) -> Vec<String> {
+    let bytes = source.as_bytes();
+    let mut names: Vec<String> = Vec::new();
+    let mut i = 0usize;
+    while i < bytes.len() {
+        let at_line_start = i == 0 || bytes[i - 1] == b'\n';
+        if !at_line_start {
+            i += 1;
+            continue;
+        }
+        let mut p = i;
+        while p < bytes.len() && (bytes[p] == b' ' || bytes[p] == b'\t') {
+            p += 1;
+        }
+        if p + 6 <= bytes.len() && &bytes[p..p + 6] == b"class " {
+            let name_start = p + 6;
+            let mut name_end = name_start;
+            while name_end < bytes.len() {
+                let c = bytes[name_end];
+                if !(c.is_ascii_alphanumeric() || c == b'_' || c == b'$') {
+                    break;
+                }
+                name_end += 1;
+            }
+            if name_end > name_start {
+                if let Ok(name) = std::str::from_utf8(&bytes[name_start..name_end]) {
+                    if !name.is_empty() && !names.contains(&name.to_string()) {
+                        names.push(name.to_string());
+                    }
+                }
+            }
+        }
+        while i < bytes.len() && bytes[i] != b'\n' {
+            i += 1;
+        }
+        i += 1;
+    }
+    names
+}
+
+/// Issue #4933 — true if the CJS body has a `return` statement at the very
+/// top level (brace depth 0). The IIFE wrap turns the module body into a
+/// function, so a top-level `return` (legal in a CommonJS module, where
+/// Node wraps the body in a function) is valid there. The flat-emit path
+/// drops the IIFE and runs the body at ESM module scope, where such a
+/// `return` would change meaning — so we keep the IIFE for those modules.
+/// Detection is brace-depth-aware with string/template/comment skipping,
+/// mirroring `collect_top_level_let_const_var_names`. A braced top-level
+/// return (`if (x) { return; }`) sits at depth ≥ 1 and is not caught here;
+/// Perry already treats a module-scope `return` as a no-op rather than an
+/// error, so the residual risk is a rare semantic nuance, not a miscompile.
+pub fn source_has_top_level_return(source: &str) -> bool {
+    let bytes = source.as_bytes();
+    let mut depth: i32 = 0;
+    let mut i = 0usize;
+    while i < bytes.len() {
+        match bytes[i] {
+            // Track only `{`/`}` — block / function / class bodies — like the
+            // sibling `collect_top_level_let_const_var_names`. Counting `(`/`[`
+            // too would let an un-skipped regex literal's brackets corrupt the
+            // depth and mis-flag a function-body `return` as top-level (the
+            // stack-utils `const methodRe = /…\[as…\]…/` false positive).
+            b'{' => {
+                depth += 1;
+                i += 1;
+                continue;
+            }
+            b'}' => {
+                depth -= 1;
+                i += 1;
+                continue;
+            }
+            b'"' | b'\'' => {
+                let q = bytes[i];
+                i += 1;
+                while i < bytes.len() && bytes[i] != q {
+                    if bytes[i] == b'\\' && i + 1 < bytes.len() {
+                        i += 2;
+                        continue;
+                    }
+                    i += 1;
+                }
+                i += 1;
+                continue;
+            }
+            b'`' => {
+                i += 1;
+                while i < bytes.len() && bytes[i] != b'`' {
+                    if bytes[i] == b'\\' && i + 1 < bytes.len() {
+                        i += 2;
+                        continue;
+                    }
+                    i += 1;
+                }
+                i += 1;
+                continue;
+            }
+            b'/' if i + 1 < bytes.len() && bytes[i + 1] == b'/' => {
+                while i < bytes.len() && bytes[i] != b'\n' {
+                    i += 1;
+                }
+                continue;
+            }
+            b'/' if i + 1 < bytes.len() && bytes[i + 1] == b'*' => {
+                i += 2;
+                while i + 1 < bytes.len() && !(bytes[i] == b'*' && bytes[i + 1] == b'/') {
+                    i += 1;
+                }
+                if i + 1 < bytes.len() {
+                    i += 2;
+                }
+                continue;
+            }
+            _ => {}
+        }
+        if depth == 0 && bytes[i] == b'r' && source[i..].starts_with("return") {
+            let before_ok = i == 0
+                || !(bytes[i - 1].is_ascii_alphanumeric()
+                    || bytes[i - 1] == b'_'
+                    || bytes[i - 1] == b'$'
+                    || bytes[i - 1] == b'.');
+            let after = i + "return".len();
+            let after_ok = after >= bytes.len()
+                || !(bytes[after].is_ascii_alphanumeric()
+                    || bytes[after] == b'_'
+                    || bytes[after] == b'$');
+            if before_ok && after_ok {
+                return true;
+            }
+        }
+        i += 1;
+    }
+    false
+}
+
 /// Issue #2310 — true if `class_body` contains any of the given names as a
 /// bare identifier (word-boundary match). Used to gate the hoist in
 /// `extract_top_level_class_decls`. We don't try to be precise about

crates/perry/src/commands/compile/cjs_wrap/mod.rs

@@ -53,6 +53,7 @@ pub(self) use extract_requires::{
 };
 pub(self) use hoist_classes::{
     extract_top_level_class_decls, rewrite_module_exports_class_expression,
+    source_has_top_level_return, top_level_class_names,
 };
 
 // Public API consumed by `compile.rs` / `collect_modules.rs`.
@@ -69,6 +70,7 @@ mod tests {
     use super::extract_requires::{
         extract_require_aliases_with_ranges, extract_require_specifiers,
     };
+    use super::hoist_classes::{source_has_top_level_return, top_level_class_names};
     use super::wrap::{wrap_commonjs, wrap_commonjs_for_target};
     use std::fs;
     use std::path::PathBuf;
@@ -648,6 +650,87 @@ module.exports = SafeBuffer;"#;
         assert!(wrapped.contains("export { Child };"));
     }
 
+    #[test]
+    fn wrap_flat_emits_class_module_exports_that_closes_over_top_level_const() {
+        // Issue #4933: `module.exports = StackUtils` where the class reads a
+        // top-level `const` (so the #2310 hoist guard refuses to lift it). The
+        // old path degraded to `export default _cjs`, losing class identity —
+        // statics, `.prototype`, and the closure all read `undefined` on the
+        // consumer side. The flat path drops the IIFE so the class stays a real
+        // top-level declaration with full identity.
+        let src = "const natives = ['a', 'b'];\n\
+                   class StackUtils {\n\
+                     static nodeInternals() { return natives.slice(); }\n\
+                     clean(s) { return 'x' + s; }\n\
+                   }\n\
+                   module.exports = StackUtils;";
+        let wrapped = wrap_commonjs(src, &PathBuf::from("/tmp/test.js"));
+        assert!(
+            wrapped.contains("export default StackUtils;"),
+            "expected direct default export of StackUtils, got:\n{}",
+            wrapped
+        );
+        assert!(
+            wrapped.contains("export { StackUtils };"),
+            "expected named export of StackUtils, got:\n{}",
+            wrapped
+        );
+        assert!(
+            !wrapped.contains("export default _cjs;"),
+            "flat emission must not fall back to the opaque _cjs default, got:\n{}",
+            wrapped
+        );
+        assert!(
+            !wrapped.contains("const _cjs = (function()"),
+            "flat emission must drop the IIFE wrapper, got:\n{}",
+            wrapped
+        );
+        // The CommonJS runtime shims still run at module scope.
+        assert!(wrapped.contains("const __cjs_module = { exports: {} };"));
+        assert!(wrapped.contains("const _cjs = __cjs_module.exports;"));
+    }
+
+    #[test]
+    fn top_level_class_names_lists_refused_and_hoisted_classes() {
+        let src = "const t = 1;\nclass A { m(){ return t; } }\nclass B {}\n";
+        let names = top_level_class_names(src);
+        assert_eq!(names, vec!["A".to_string(), "B".to_string()]);
+    }
+
+    #[test]
+    fn top_level_return_detection_ignores_returns_inside_bodies_and_regexes() {
+        // No top-level return: every `return` sits inside a function/class body,
+        // and the regex literal's brackets must not corrupt brace depth.
+        let no_return = "const re = /^(.*?) \\[as (.*?)\\]$/;\n\
+                         class C {\n\
+                           m() { if (true) { return 1; } return 2; }\n\
+                         }\n\
+                         module.exports = C;";
+        assert!(
+            !source_has_top_level_return(no_return),
+            "function-body returns must not count as top-level"
+        );
+        // A genuine module-top return keeps the IIFE.
+        let yes_return = "if (!supported) return;\nmodule.exports = {};";
+        assert!(source_has_top_level_return(yes_return));
+    }
+
+    #[test]
+    fn wrap_keeps_iife_for_class_module_exports_with_top_level_return() {
+        // A top-level `return` is legal in CommonJS but not at ESM module scope,
+        // so the IIFE wrap must be retained even for `module.exports = <Class>`.
+        let src = "const t = 1;\n\
+                   if (!t) return;\n\
+                   class C { m(){ return t; } }\n\
+                   module.exports = C;";
+        let wrapped = wrap_commonjs(src, &PathBuf::from("/tmp/test.js"));
+        assert!(
+            wrapped.contains("const _cjs = (function()"),
+            "module with a top-level return must keep the IIFE, got:\n{}",
+            wrapped
+        );
+    }
+
     #[test]
     fn wrap_keeps_cjs_default_when_module_exports_is_object_literal() {
         let src = "module.exports = { foo: 1, bar: 2 };";
@@ -1079,15 +1162,21 @@ module.exports = SafeBuffer;"#;
         );
     }
 
-    /// Issue #2310 — when a top-level class body references a let/const
-    /// declared at the IIFE's top level (the ws/lib/sender.js shape:
-    /// `let randomPoolPointer; class Sender { static frame(){ … r++ } }`),
-    /// hoisting the class out of the IIFE would sever the closure over
-    /// `randomPoolPointer` and the compile hard-errors with
-    /// `Undefined variable in update expression`. Verify the class stays
-    /// inside the IIFE body and is NOT in the hoisted-class block.
+    /// Issue #2310 / #4933 — a top-level class body that references a
+    /// let/const declared at the IIFE's top level (the ws/lib/sender.js shape:
+    /// `let pointer; class Sender { static next(){ … pointer++ } }`) cannot be
+    /// *hoisted* above the IIFE — that would sever the closure and the compile
+    /// hard-errors with `Undefined variable in update expression`.
+    ///
+    /// For a `module.exports = Sender` default-export class, the #4933 flat
+    /// emission supersedes the old IIFE-retention mitigation: dropping the IIFE
+    /// puts BOTH the class and `let pointer` at module scope, so the closure
+    /// (including the `pointer++` mutation) survives AND the class keeps full
+    /// identity — the consumer's default import sees its statics / `.prototype`
+    /// instead of an opaque `_cjs`. Verify the wrap flat-emits the class
+    /// (no IIFE, direct default export) and still parses.
     #[test]
-    fn issue_2310_class_referencing_iife_let_is_not_hoisted() {
+    fn issue_2310_class_referencing_iife_let_flat_emits() {
         let src = "'use strict';\n\
                    const POOL_SIZE = 8;\n\
                    let pointer = 0;\n\
@@ -1096,17 +1185,25 @@ module.exports = SafeBuffer;"#;
                    }\n\
                    module.exports = Sender;\n";
         let wrapped = wrap_commonjs(src, &PathBuf::from("/tmp/sender.js"));
-        // The IIFE body must still contain the class — i.e. the wrap must
-        // not lift it above the `const _cjs = (function() { ... })()` line.
-        let iife_open = wrapped
-            .find("const _cjs = (function()")
-            .expect("wrap must produce the IIFE wrapper");
-        let class_pos = wrapped
-            .find("class Sender")
-            .expect("wrap must keep `class Sender` somewhere");
         assert!(
-            class_pos > iife_open,
-            "expected `class Sender` to stay inside the IIFE for #2310; got:\n{}",
+            wrapped.contains("export default Sender;"),
+            "expected flat default export of Sender, got:\n{}",
+            wrapped
+        );
+        assert!(
+            !wrapped.contains("const _cjs = (function()"),
+            "expected the IIFE to be dropped for the flat default-export class, got:\n{}",
+            wrapped
+        );
+        // `class Sender` and `let pointer` both land at module scope, so the
+        // mutable closure is preserved (behavioral parity verified separately).
+        assert!(wrapped.contains("class Sender"));
+        assert!(wrapped.contains("let pointer = 0;"));
+        let parsed = perry_parser::parse_typescript(&wrapped, "sender.js");
+        assert!(
+            parsed.is_ok(),
+            "flat-emitted sender wrap failed to parse: {:?}\nwrapped:\n{}",
+            parsed.err(),
             wrapped
         );
     }