fix(cjs): defer conditional CommonJS require() init instead of hoisting (#10437)

changelog.d/10674-cjs-conditional-require-deferred.md

@@ -0,0 +1,36 @@
+### Fixed
+
+- **CommonJS `require()` outside a function is no longer hoisted past the
+  control flow that guards it.** Perry's CJS→ESM wrap turned every
+  literal `require('S')` in a wrapped file into a static `import` at the
+  top of the module and eager-initialized the target — even when the call
+  sat inside `if (false)`, a false env check, `&&`/`??`, a ternary arm, a
+  `switch` case, or a loop that never iterates. A module reached only
+  through such a branch loaded (and could throw) at program start,
+  regardless of whether the branch ever ran; a module reached through a
+  taken branch loaded before the statements preceding it. This was the
+  sole remaining blocker compiling `pg` from source: `lib/index.js` guards
+  its optional native binding behind `if (forceNative) { require('./native') }`,
+  and `./native` requires the often-uninstalled `pg-native` — every
+  program using `pg` crashed at init with `Cannot find module 'pg-native'`
+  even though `forceNative` was false.
+  `cjs_wrap::extract_requires::function_local_specs` now classifies a
+  `require()` call site as deferred (Node's actual "loads only when
+  control flow reaches it" semantics) whenever it sits inside a
+  control-flow block (`if`/`for`/`while`/`switch`/`catch`/`try`/`else`/
+  `do`/`finally`) or a braceless/operator equivalent (`cond &&
+  require(...)`, `cond ? require(...) : x`, `for (...) require(...)` with
+  no block) — not only inside a function body as before. An ordinary
+  object literal or class body still does not count, so the common
+  `module.exports = { fs: require('fs'), path: require('path') }` barrel
+  shape stays eager. A `process.platform === '<literal>'` guard (the
+  node-pty Windows/Unix terminal split) is exempted from the broader
+  reclassification and keeps its existing eager treatment — the platform
+  is a compile-time-known build target, not a runtime unknown, and
+  `wrap_commonjs_for_target`'s dead-branch pruning already resolves it.
+
+Verified end-to-end: `pg` now compiles, links, and runs from real source
+under `perry.compilePackages`, reaching a real TCP connect attempt with no
+`pg-native` crash.
+
+Fixes #10437.

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

@@ -293,19 +293,40 @@ pub fn identifier_is_declared_binding(source: &str, name: &str) -> bool {
     false
 }
 
-/// Next.js lazy-require classification (single forward pass). Returns the set
-/// of specifiers whose EVERY `require('<spec>')` call site is lexically inside
-/// a FUNCTION body — never at module top level, and never inside a top-level
-/// control-flow block that runs at module load. Node loads such a module
-/// lazily (only when the enclosing function runs), so Perry must not eager-init
-/// it.
+/// Deferred-require classification (single forward pass). Returns the set of
+/// specifiers whose EVERY `require('<spec>')` call site is NOT guaranteed to
+/// run the moment the module loads — a function body (never called, or called
+/// later: #Next.js lazy-require), a control-flow block that may not run every
+/// time its enclosing scope runs (`if`/`for`/`while`/`switch`/`catch`/`with`/
+/// `else`/`try`/`do`/`finally`), or a braceless/operator-guarded equivalent of
+/// the same thing (`cond && require(...)`, `cond ? require(...) : x`, `for
+/// (...) require(...)` with no block). Node only ever loads such a module when
+/// control flow actually reaches the call, so Perry must not eager-init it
+/// either (issue #10437: `pg` guards its optional `pg-native` binding exactly
+/// this way, behind `if (forceNative) { require('./native') }`).
 ///
-/// Conservative by construction: a spec with any top-level call site (including
-/// top-level `if`/`for`/`try` blocks, which execute during module evaluation)
-/// is excluded and keeps the default eager behavior. A misclassification is
-/// self-correcting at runtime — the require shim triggers the target's init
-/// when `require()` is actually called — so this only governs eager-init-loop
-/// membership.
+/// An ordinary object literal (`{ key: require(...) }`), a class body, or a
+/// bare grouping block do NOT count — their contents run unconditionally
+/// whenever the enclosing statement/expression is reached, same as top level,
+/// so nesting inside one of those must not flip a spec to lazy (that would be
+/// the common `module.exports = { fs: require('fs'), path: require('path') }`
+/// barrel-export shape, which really is eager).
+///
+/// The ternary ALTERNATE arm (`cond ? x : require(...)`) is deliberately NOT
+/// matched — a bare `:` immediately before `require(` is indistinguishable
+/// from an object-literal property value or a `switch` case label without a
+/// real parse, and guessing wrong there risks the same barrel-export
+/// misclassification the object-literal exclusion above avoids. That shape
+/// keeps the conservative eager default (a known, narrow gap — not in scope
+/// for #10437's reproduction).
+///
+/// A false POSITIVE here (treating a genuinely-unconditional require as
+/// conditional) is harmless: the require shim still triggers the target's
+/// init at the exact point the call is lexically reached, which for an
+/// unconditional call is essentially the same moment eager pre-init would
+/// have run it. A false NEGATIVE (missing a genuinely-conditional call) is
+/// the actual bug class — the target loads (and can throw) before its
+/// guarding condition was ever evaluated.
 ///
 /// Brace/paren scanning runs on a comment/string/regex-masked copy (same
 /// length, code structure preserved) so literal braces never corrupt the scope
@@ -337,30 +358,65 @@ pub fn function_local_specs(source: &str) -> std::collections::HashSet<String> {
         return HashSet::new();
     }
 
+    // #10437 followup: a spec whose ONLY conditionality is a
+    // `process.platform === /!== '<literal>'` if/else guard (either branch —
+    // e.g. node-pty's `./windowsTerminal` / `./unixTerminal` split) must NOT
+    // be downgraded to lazy by the broader control-flow classification below.
+    // The platform is a build TARGET resolved at compile time, not a runtime
+    // unknown — `wrap_commonjs_for_target`'s `inactive_platform_guarded_requires`
+    // already prunes the dead branch's spec outright for a known target, and
+    // the live branch's spec keeps the eager `_req_N` classification it had
+    // before this fix. Treating a compile-time-resolved platform check as
+    // conditional the way a genuinely runtime-unknown check (env var,
+    // arbitrary function result) is would only add needless deferral, not
+    // fix a bug — #10437 is about conditions Perry cannot resolve at compile
+    // time.
+    let platform_guarded_specs = process_platform_guarded_specs(source);
+
     let mbytes = masked.as_bytes();
     let is_ident = |c: u8| c == b'_' || c == b'$' || c.is_ascii_alphanumeric();
     let control_keywords = ["if", "for", "while", "switch", "catch", "with", "else"];
+    // Bare-keyword control blocks with no parens (`try {`, `else {`, `do {`,
+    // `} finally {`) — as opposed to an object literal / class body / plain
+    // grouping block, whose opening `{` is also not preceded by `)`/`=>` but
+    // whose contents are NOT conditional (see doc comment above).
+    let bare_control_keywords = ["try", "else", "do", "finally"];
 
     #[derive(PartialEq)]
     enum Scope {
+        /// Function/method/arrow/IIFE body: reachability depends on whether,
+        /// and when, the function is ever called.
         Function,
+        /// A control-flow block that may not run every time its enclosing
+        /// scope runs.
         Block,
+        /// Anything else brace-delimited whose contents run unconditionally
+        /// when reached (object literal, class body, bare grouping block).
+        /// Nesting here does not itself make an enclosed `require()`
+        /// conditional.
+        Other,
     }
     let mut scopes: Vec<Scope> = Vec::new();
-    // spec → (seen any site, all sites so far in-function).
+    // spec → (seen any site, all sites so far conditionally-reached).
     let mut state: HashMap<&str, (bool, bool)> = HashMap::new();
     let mut next_site = 0usize;
-    let in_function = |scopes: &[Scope]| scopes.contains(&Scope::Function);
+    let gates_reachability = |scopes: &[Scope]| {
+        scopes
+            .iter()
+            .any(|s| matches!(s, Scope::Function | Scope::Block))
+    };
 
     let mut i = 0usize;
     while i < mbytes.len() {
         // Record any require site at this offset before processing the char.
         while next_site < sites.len() && sites[next_site].0 == i {
             let (_, spec) = sites[next_site];
-            let here = in_function(&scopes);
+            let conditional = !platform_guarded_specs.contains(spec)
+                && (gates_reachability(&scopes)
+                    || site_is_conditionally_guarded(&masked, mbytes, i, &is_ident));
             let e = state.entry(spec).or_insert((false, true));
             e.0 = true;
-            e.1 = e.1 && here;
+            e.1 = e.1 && conditional;
             next_site += 1;
         }
         match mbytes[i] {
@@ -381,7 +437,18 @@ pub fn function_local_specs(source: &str) -> std::collections::HashSet<String> {
                         Scope::Function
                     }
                 } else {
-                    Scope::Block
+                    // Not preceded by `)` or `=>`: a bare control keyword
+                    // (`try`/`else`/`do`/`finally`) is conditional; an object
+                    // literal, class body, or plain grouping block is not.
+                    let mut w = p;
+                    while w > 0 && is_ident(mbytes[w - 1]) {
+                        w -= 1;
+                    }
+                    if bare_control_keywords.iter().any(|k| *k == &masked[w..p]) {
+                        Scope::Block
+                    } else {
+                        Scope::Other
+                    }
                 };
                 scopes.push(kind);
             }
@@ -395,16 +462,19 @@ pub fn function_local_specs(source: &str) -> std::collections::HashSet<String> {
     // Any sites at EOF offset (defensive).
     while next_site < sites.len() {
         let (_, spec) = sites[next_site];
+        let conditional = !platform_guarded_specs.contains(spec)
+            && (gates_reachability(&scopes)
+                || site_is_conditionally_guarded(&masked, mbytes, mbytes.len(), &is_ident));
         let e = state.entry(spec).or_insert((false, true));
         e.0 = true;
-        e.1 = e.1 && in_function(&scopes);
+        e.1 = e.1 && conditional;
         next_site += 1;
     }
 
     state
         .into_iter()
-        .filter_map(|(spec, (seen, all_in_fn))| {
-            if seen && all_in_fn {
+        .filter_map(|(spec, (seen, all_conditional))| {
+            if seen && all_conditional {
                 Some(spec.to_string())
             } else {
                 None
@@ -413,6 +483,77 @@ pub fn function_local_specs(source: &str) -> std::collections::HashSet<String> {
         .collect()
 }
 
+/// Is the `require(` call whose match starts at masked-source offset
+/// `call_start` reached only conditionally by a nearby operator or a
+/// braceless control-flow header, even though it has no enclosing `{ }`
+/// scope of its own? Brace-scope tracking (above) can't see these shapes:
+/// `cond && require(...)` / `cond || require(...)` / `cond ?? require(...)`,
+/// the ternary CONSEQUENT arm `cond ? require(...) : x`, a braceless arrow
+/// `() => require(...)`, and a braceless control-flow body — `if (...)
+/// require(...)`, `for (...) require(...)`, `while (...) require(...)`,
+/// `else require(...)`, `do require(...)`.
+fn site_is_conditionally_guarded(
+    masked: &str,
+    mbytes: &[u8],
+    call_start: usize,
+    is_ident: &impl Fn(u8) -> bool,
+) -> bool {
+    let mut p = call_start;
+    while p > 0 && (mbytes[p - 1] as char).is_whitespace() {
+        p -= 1;
+    }
+    if p == 0 {
+        return false;
+    }
+    if p >= 2 {
+        let two = &masked[p - 2..p];
+        if two == "&&" || two == "||" || two == "??" || two == "=>" {
+            return true;
+        }
+    }
+    // Ternary consequent (`cond ? require(...) : x`) — a lone `?`, not the
+    // second char of `??` (already handled above).
+    if mbytes[p - 1] == b'?' && !(p >= 2 && mbytes[p - 2] == b'?') {
+        return true;
+    }
+    // Braceless control-flow header: `if (...)`, `for (...)`, `while (...)`
+    // immediately followed by the require call (no block).
+    if mbytes[p - 1] == b')' {
+        let head = matched_open_head(masked, mbytes, p - 1, is_ident);
+        return matches!(head.as_str(), "if" | "for" | "while");
+    }
+    // Bare `else`/`do` immediately before, with no parens and no block.
+    let mut w = p;
+    while w > 0 && is_ident(mbytes[w - 1]) {
+        w -= 1;
+    }
+    matches!(&masked[w..p], "else" | "do")
+}
+
+/// Every `require('<spec>')` specifier textually inside EITHER branch of a
+/// `if (process.platform === /!== '<literal>') { … } else { … }` guard.
+/// Mirrors the pattern `wrap.rs`'s `inactive_platform_guarded_requires`
+/// matches to prune the DEAD branch's spec for a known build target — this
+/// helper is target-independent and returns BOTH branches' specs, so the
+/// LIVE branch's spec (which `inactive_platform_guarded_requires` keeps) can
+/// be exempted from the general conditional-require classification above.
+fn process_platform_guarded_specs(source: &str) -> std::collections::HashSet<String> {
+    let re = perry_perex::tooling::Regex::new(
+        r#"(?s)if\s*\(\s*process\.platform\s*(?:===|!==)\s*['"][^'"]+['"]\s*\)\s*\{(?P<then>.*?)\}\s*else\s*\{(?P<else>.*?)\}"#,
+    )
+    .unwrap();
+    let mut specs = std::collections::HashSet::new();
+    for cap in re.captures_iter(source) {
+        if let Some(then) = cap.name("then") {
+            specs.extend(extract_require_specifiers(then.as_str()));
+        }
+        if let Some(els) = cap.name("else") {
+            specs.extend(extract_require_specifiers(els.as_str()));
+        }
+    }
+    specs
+}
+
 /// Given the index of a `)` in the masked source, walk back to its matching
 /// `(` and return the identifier/keyword immediately before that `(`.
 fn matched_open_head(

crates/perry/src/commands/compile/collect_modules.rs

@@ -1777,15 +1777,19 @@ fn collect_module_one(
         }
     }
 
-    // Next.js lazy-require: the CJS→ESM wrap names a binding `_lazyreq_N` when
-    // every `require('S')` call site is inside a function body (lazy in Node).
-    // Tag the import so `classify_eager_modules` leaves the target Deferred —
-    // matching Node, which only loads such a module when the enclosing function
-    // runs (e.g. jsonwebtoken, required only inside Next.js's request handlers).
-    // The require shim triggers the target's `__init` on first `require()`, so
-    // an over-eager classification is self-correcting at runtime. Limited to
-    // Perry-compiled (`NativeCompiled`) targets — native stdlib / V8 modules
-    // have their own init paths.
+    // Deferred require (#10437, originally the Next.js lazy-require case): the
+    // CJS→ESM wrap names a binding `_lazyreq_N` when every `require('S')` call
+    // site is NOT guaranteed to run the moment the module loads — inside a
+    // function body (lazy in Node: jsonwebtoken, required only inside Next.js's
+    // request handlers), or inside a top-level control-flow block / braceless
+    // equivalent that may never run (`if (forceNative) { require('./native') }`,
+    // pg's optional native binding). Tag the import so `classify_eager_modules`
+    // leaves the target Deferred — matching Node, which only loads such a
+    // module when control flow actually reaches the call. The require shim
+    // triggers the target's `__init` at that same call site, so an over-eager
+    // classification is self-correcting at runtime (it just runs a bit early).
+    // Limited to Perry-compiled (`NativeCompiled`) targets — native stdlib /
+    // V8 modules have their own init paths.
     {
         for import in &mut hir_module.imports {
             if import.type_only

test-files/_helpers/gap10437_cjs_lazy_require.cjs

@@ -0,0 +1,88 @@
+'use strict'
+// #10437: CommonJS `require()` outside a function is hoisted and run
+// unconditionally at module init, including inside `if (false)` and other
+// branches that never run. Every require below except H is inside a branch
+// that never executes; only H's side-effect module should ever load, and it
+// should load exactly at the point control flow reaches it (between the
+// "before taken branch" and "after taken branch" log lines) — not before
+// the first statement, and not before H's guarding condition was evaluated.
+//
+// This is the shape pg 8.22.0 hits verbatim: `lib/index.js` guards an
+// optional native binding behind `if (forceNative) { require('./native') }`,
+// and `./native` transitively requires the optional, often-uninstalled
+// `pg-native`. The crash-form section below reproduces that two-hop shape
+// with a target that genuinely does not resolve on disk.
+
+console.log('start')
+
+// A: literal false
+if (false) {
+  require('./gap10437_side_a.cjs')
+}
+// B: short-circuit
+false && require('./gap10437_side_b.cjs')
+// C: runtime-false env check (pg's `if (forceNative)` shape)
+if (process.env.PERRY_GAP10437_UNSET_C) {
+  require('./gap10437_side_c.cjs')
+}
+// D: ternary arm not taken
+const d = process.env.PERRY_GAP10437_UNSET_D ? require('./gap10437_side_d.cjs') : 'd-skipped'
+// E: switch case not taken
+switch (1) {
+  case 2:
+    require('./gap10437_side_e.cjs')
+}
+// F: loop body never runs
+for (let i = 0; i < 0; i++) require('./gap10437_side_f.cjs')
+// G: function never called (already correctly deferred pre-#10437)
+function never() {
+  return require('./gap10437_side_g.cjs')
+}
+
+console.log('before taken branch')
+
+// H: the taken branch — must load exactly here, not earlier.
+if (true) {
+  require('./gap10437_side_h.cjs')
+}
+
+console.log('after taken branch, d=' + d)
+
+// Caching: two conditional requires of the SAME module must run the side
+// effect once and return the SAME exports object both times.
+let capA = null
+let capB = null
+if (true) {
+  capA = require('./gap10437_counter.cjs')
+}
+if (true) {
+  capB = require('./gap10437_counter.cjs')
+}
+console.log('cache same=' + (capA === capB) + ' n=' + capA.n)
+
+// Crash-form (pg-native shape): an optional native binding behind an unset
+// env check, whose target itself unconditionally (but inside a
+// non-swallowing try/catch) requires a module that does not exist on disk.
+// Pre-fix this crashed the whole program with "Cannot find module" even
+// though the guarding env var was never set.
+let impl = 'js'
+if (process.env.PERRY_GAP10437_USE_NATIVE) {
+  impl = require('./gap10437_native_rethrow.cjs')
+}
+console.log('impl=' + impl)
+
+// A genuinely missing module behind a try/catch that SWALLOWS the error,
+// itself nested inside a condition that never runs.
+let fallback = 'default'
+if (process.env.PERRY_GAP10437_UNSET_FALLBACK) {
+  try {
+    fallback = require('./gap10437_does_not_exist.cjs')
+  } catch (e) {
+    fallback = 'caught'
+  }
+}
+console.log('fallback=' + fallback)
+
+console.log('end')
+
+module.exports = { never: never }