crates/wasm/src/js_module.rs (1)

623-623: LGTM - Re-export added for module_def pattern.

The addition of pub(crate) use _js::module_def; aligns with the crate-wide module initialization refactoring. Note that this re-export is placed at the end of the file, unlike most other modules where it appears at the top. While functionally equivalent, you may consider moving it to line 2 (after the PyJsValue, PyPromise re-export) for consistency with other stdlib modules.

crates/vm/src/builtins/module.rs (1)

53-71: Remove redundant import.

PyPayload is already imported at the file level (line 3), so the use crate::PyPayload; on line 54 is unnecessary.

Proposed fix

     pub fn create_module(&'static self, vm: &VirtualMachine) -> PyResult<PyRef<PyModule>> {
-        use crate::PyPayload;
-
         // Create module (use create slot if provided, else default creation)
         let module = if let Some(create) = self.slots.create {

crates/stdlib/src/_sqlite3.rs (1)

847-867: Propagate setup_module_exceptions errors instead of panicking.

Line 856 invokes setup_module_exceptions, which still uses unwrap() internally. Since module_exec now returns PyResult<()>, consider returning PyResult<()> from setup_module_exceptions and using ? to avoid panics during module init.

crates/vm/src/stdlib/sysconfigdata.rs (1)

14-25: Chain __module_exec before custom init.

Custom module_exec doesn’t call the macro-generated __module_exec, so any #[pyattr]/#[pyfunction] exports added now or later won’t be registered. Consider calling it first, consistent with other stdlib modules.

♻️ Suggested change

     fn module_exec(vm: &VirtualMachine, module: &Py<PyModule>) -> PyResult<()> {
+        __module_exec(vm, module);
         // Set build_time_vars attribute
         let build_time_vars = build_time_vars(vm);

crates/vm/src/stdlib/sys.rs (1)

168-173: Consider deduplicating builtin module names after augmentation.

If any native module definition already uses "sys" or "builtins", this could yield duplicates in sys.builtin_module_names. A quick dedup after sorting prevents that edge case.

♻️ Proposed tweak

-        module_names.sort();
+        module_names.sort();
+        module_names.dedup();

crates/vm/src/vm/interpreter.rs (1)

67-72: Avoid per-interpreter Box::leak for _sysconfigdata aliasing.

This repeats a previously flagged issue: leaking on each interpreter creation grows memory unboundedly. Prefer a process-wide OnceLock to leak once.

🩹 Suggested fix (same as prior review)

     if let Some(&sysconfigdata_def) = all_module_defs.get("_sysconfigdata") {
+        use std::sync::OnceLock;
         let sysconfigdata_name = crate::stdlib::sys::sysconfigdata_name();
         // Leak the string to get a 'static lifetime for the key
-        let leaked_name: &'static str = Box::leak(sysconfigdata_name.into_boxed_str());
-        all_module_defs.insert(leaked_name, sysconfigdata_def);
+        static SYSCONFIGDATA_NAME: OnceLock<&'static str> = OnceLock::new();
+        let leaked_name =
+            *SYSCONFIGDATA_NAME.get_or_init(|| Box::leak(sysconfigdata_name.into_boxed_str()));
+        all_module_defs.insert(leaked_name, sysconfigdata_def);
     }

crates/vm/src/stdlib/imp.rs (1)

143-149: Clean up sys.modules when exec slot fails.

This was flagged in a previous review: if def.slots.exec returns an error on line 148, the module inserted at line 144 remains in sys.modules, leaving a partially initialized module cached. This can cause issues with subsequent imports.

🔧 Suggested fix

             // Add to sys.modules BEFORE exec (critical for circular import handling)
             sys_modules.set_item(&*name, module.clone().into(), vm)?;

             // Phase 2: Call exec slot (can safely import other modules now)
             if let Some(exec) = def.slots.exec {
-                exec(vm, &module)?;
+                if let Err(err) = exec(vm, &module) {
+                    let _ = sys_modules.del_item(&*name, vm);
+                    return Err(err);
+                }
             }

crates/vm/src/stdlib/mod.rs (1)

96-102: Potential duplicate posix module registration on WASI targets.

The cfg conditions may still overlap for WASI:

• Line 96-97: #[cfg(any(unix, target_os = "wasi"))] matches WASI
• Lines 98-101: #[cfg(all(any(not(target_arch = "wasm32"), target_os = "wasi"), not(any(unix, windows))))] also matches WASI since target_os = "wasi" satisfies the first part and WASI is neither unix nor windows

This could cause posix::module_def(ctx) to be included twice in the vector for WASI builds.

#!/bin/bash
# Verify the cfg conditions for WASI target
# Check if both conditions can be true simultaneously for wasi

echo "Checking posix module cfg conditions in mod.rs..."
rg -n "posix::module_def" crates/vm/src/stdlib/mod.rs -B2 -A1

echo ""
echo "Checking how other files handle WASI posix module:"
rg -n "target_os.*wasi" crates/vm/src/stdlib/mod.rs

crates/vm/src/vm/interpreter.rs (1)

438-474: Consider tracking the FIXME about duplicate frozen core_modules.
If you’d like, I can help draft a follow-up to avoid double-generation while keeping without_stdlib() and init_stdlib() consistent.

crates/derive-impl/src/pymodule.rs (1)

56-90: Consider guarding def.slots.exec assignment or documenting that module_exec must not be cfg-gated.

The detection at lines 86-90 sets has_module_exec = true based on the function name alone, without checking func.attrs for #[cfg] guards. While no cfg-gated module_exec functions currently exist in the codebase (all use conditional code inside the function body instead), the implementation remains fragile: if a user adds a top-level #[cfg]-gated module_exec, the fallback suppression (line 149) combined with the unconditional reference at line 142 would cause silent build failures on excluded configurations.

Prevent this by either checking cfg attributes during detection, conditionally wrapping the def.slots.exec assignment, or documenting that module_exec cannot be cfg-gated at the function level.

crates/vm/src/stdlib/os.rs (1)

1900-1924: Consider error propagation in module_exec.

The function uses multiple unwrap() calls (lines 1906, 1908, 1911, 1914) that could panic during module initialization if get_attr or add fails. While the current callers in posix.rs and nt.rs don't handle errors from this function, propagating errors would be more consistent with the pattern in errno.rs which uses ? for error handling.

Given the callers currently ignore the result anyway (they just call os::module_exec(vm, module);), this is a lower-priority refactor that could be addressed in a follow-up.

♻️ Suggested change for error propagation

-pub fn module_exec(vm: &VirtualMachine, module: &Py<PyModule>) {
+pub fn module_exec(vm: &VirtualMachine, module: &Py<PyModule>) -> PyResult<()> {
     let support_funcs = _os::support_funcs();
     let supports_fd = PySet::default().into_ref(&vm.ctx);
     let supports_dir_fd = PySet::default().into_ref(&vm.ctx);
     let supports_follow_symlinks = PySet::default().into_ref(&vm.ctx);
     for support in support_funcs {
-        let func_obj = module.get_attr(support.name, vm).unwrap();
+        let func_obj = module.get_attr(support.name, vm)?;
         if support.fd.unwrap_or(false) {
-            supports_fd.clone().add(func_obj.clone(), vm).unwrap();
+            supports_fd.clone().add(func_obj.clone(), vm)?;
         }
         if support.dir_fd.unwrap_or(false) {
-            supports_dir_fd.clone().add(func_obj.clone(), vm).unwrap();
+            supports_dir_fd.clone().add(func_obj.clone(), vm)?;
         }
         if support.follow_symlinks.unwrap_or(false) {
-            supports_follow_symlinks.clone().add(func_obj, vm).unwrap();
+            supports_follow_symlinks.clone().add(func_obj, vm)?;
         }
     }
     // ... extend_module! ...
+    Ok(())
 }

This would require updating callers in posix.rs, nt.rs, and posix_compat.rs to propagate the error.

crates/vm/src/vm/mod.rs (1)

231-232: Minor grammar issue in error message.

Consider: "rustpython_pylib may not be set" instead of "maybe not set" for grammatical correctness.

Suggested fix

             let guide_message = if cfg!(feature = "freeze-stdlib") {
-                "`rustpython_pylib` maybe not set while using `freeze-stdlib` feature. Try using `rustpython::InterpreterBuilder::init_stdlib` or manually call `builder.add_frozen_modules(rustpython_pylib::FROZEN_STDLIB)` in `rustpython_vm::Interpreter::builder()`."
+                "`rustpython_pylib` may not be set while using `freeze-stdlib` feature. Try using `rustpython::InterpreterBuilder::init_stdlib` or manually call `builder.add_frozen_modules(rustpython_pylib::FROZEN_STDLIB)` in `rustpython_vm::Interpreter::builder()`."

crates/wasm/src/vm_class.rs (1)

24-35: Propagate __module_exec errors (already noted earlier).
Line 30 ignores the PyResult from __module_exec, so init errors won’t propagate.

crates/vm/src/stdlib/typing.rs (1)

191-218: Propagate __module_exec errors.

Same issue: __module_exec(vm, module); discards errors; use ? to avoid silent init failures.

crates/vm/src/stdlib/ctypes.rs (1)

1281-1311: Propagate __module_exec errors.

Same issue: ignoring the Result hides init failures.

crates/vm/src/stdlib/imp.rs (1)

122-149: Remove sys.modules entry if exec fails.

If the exec slot errors, a partially initialized module remains cached. Clean it up before propagating the error to match CPython’s behavior.

🔧 Proposed fix

-            if let Some(exec) = def.slots.exec {
-                exec(vm, &module)?;
-            }
+            if let Some(exec) = def.slots.exec {
+                if let Err(err) = exec(vm, &module) {
+                    let _ = sys_modules.del_item(&*name, vm);
+                    return Err(err);
+                }
+            }

crates/vm/src/stdlib/mod.rs (1)

96-102: Avoid duplicate posix module registration on WASI.

Both cfgs can match for target_os = "wasi", causing posix::module_def(ctx) to be included twice. Consider narrowing the first entry to unix-only (or remove one entry).

🛠 Suggested fix

-        #[cfg(any(unix, target_os = "wasi"))]
+        #[cfg(unix)]
         posix::module_def(ctx),

crates/stdlib/src/pyexpat.rs (1)

42-66: Register pyexpat submodules in sys.modules for qualified imports

The module still attaches model/errors as attributes without registering pyexpat.model/pyexpat.errors in sys.modules, which can break qualified imports and introspection.

crates/vm/src/stdlib/imp.rs (1)

118-149: Remove partially initialized modules on exec failure.
If exec errors, the module stays in sys.modules and may be reused in a bad state.

🛠️ Suggested fix

-            if let Some(exec) = def.slots.exec {
-                exec(vm, &module)?;
-            }
+            if let Some(exec) = def.slots.exec {
+                if let Err(err) = exec(vm, &module) {
+                    let _ = sys_modules.del_item(&*name, vm);
+                    return Err(err);
+                }
+            }

crates/vm/src/stdlib/typing.rs (1)

191-218: Propagate __module_exec failures.
The result is dropped; initialization errors can be silently ignored.

🛠️ Suggested fix

-        __module_exec(vm, module);
+        __module_exec(vm, module)?;

crates/vm/src/stdlib/ctypes.rs (1)

1281-1312: Propagate __module_exec failures.
The result is dropped; initialization errors can be silently ignored.

🛠️ Suggested fix

-        __module_exec(vm, module);
+        __module_exec(vm, module)?;

crates/vm/src/stdlib/sys.rs (1)

168-174: Duplicate: dedupe builtin_module_names after sorting.
module_defs can already include "sys"/"builtins", so the tuple may contain duplicates.

crates/derive-impl/src/pymodule.rs (1)

85-90: Duplicate: reject cfg‑gated module_exec to avoid missing symbol.

crates/vm/src/stdlib/mod.rs (1)

96-102: Duplicate posix module registration on WASI targets.

On WASI targets, both conditions match:

• #[cfg(any(unix, target_os = "wasi"))] (line 96) matches because target_os = "wasi"
• #[cfg(all(any(not(target_arch = "wasm32"), target_os = "wasi"), not(any(unix, windows))))] (lines 98-101) also matches because WASI is wasm32 but has target_os = "wasi", and WASI is neither unix nor windows

This results in posix::module_def(ctx) being included twice in the vector for WASI builds, which could cause issues during module registration.

🔧 Suggested fix

         #[cfg(any(unix, target_os = "wasi"))]
         posix::module_def(ctx),
         #[cfg(all(
             any(not(target_arch = "wasm32"), target_os = "wasi"),
-            not(any(unix, windows))
+            not(any(unix, windows, target_os = "wasi"))
         ))]
         posix::module_def(ctx),

Alternatively, remove the WASI-specific entry since it's already covered:

-        #[cfg(any(unix, target_os = "wasi"))]
-        posix::module_def(ctx),
+        #[cfg(unix)]
+        posix::module_def(ctx),
+        #[cfg(target_os = "wasi")]
+        posix::module_def(ctx),
         #[cfg(all(
             any(not(target_arch = "wasm32"), target_os = "wasi"),
             not(any(unix, windows))
         ))]
         posix::module_def(ctx),

src/interpreter.rs (2)

30-37: Potential panic if state has multiple references.

The .unwrap() on PyRc::get_mut will panic if there are other references to vm.state. While this is likely safe during initialization (when init_hook runs), consider using expect() with a descriptive message to aid debugging if assumptions are violated.

-    let state = PyRc::get_mut(&mut vm.state).unwrap();
+    let state = PyRc::get_mut(&mut vm.state)
+        .expect("VM state should be uniquely owned during init_hook");

---

58-76: Potential panic on non-UTF-8 paths.

The .unwrap() on into_string() (line 64) will panic if BUILDTIME_RUSTPYTHONPATH contains non-UTF-8 characters. While unlikely in controlled build environments, consider using to_string_lossy() or handling the error gracefully:

-            .map(|path| path.into_os_string().into_string().unwrap())
+            .filter_map(|path| path.into_os_string().into_string().ok())

Alternatively, if non-UTF-8 paths should cause a build/runtime failure, document this expectation.

crates/vm/src/stdlib/ctypes.rs (1)

1281-1288: Propagate errors from __module_exec.

Line 1287 discards the PyResult return; errors will be silently ignored.

🔧 Suggested fix

-        __module_exec(vm, module);
+        __module_exec(vm, module)?;

crates/stdlib/src/pyexpat.rs (1)

53-63: Register pyexpat.model / pyexpat.errors in sys.modules for qualified imports.

crates/vm/src/stdlib/imp.rs (1)

143-149: Rollback sys.modules if exec fails.

Line 143 inserts the module before exec; if exec errors, the partially initialized module remains cached. Clean it up on failure to avoid stale imports.

🛠️ Proposed fix

-            if let Some(exec) = def.slots.exec {
-                exec(vm, &module)?;
-            }
+            if let Some(exec) = def.slots.exec {
+                if let Err(err) = exec(vm, &module) {
+                    let _ = sys_modules.del_item(&*name, vm);
+                    return Err(err);
+                }
+            }

crates/derive-impl/src/pymodule.rs (1)

85-90: Guard against cfg‑gated module_exec to avoid build breaks.
Line 85–90 marks any module_exec as present, but def.slots.exec = Some(module_exec) is always emitted. If a user #[cfg]‑gates module_exec, builds with the cfg disabled will fail due to an unresolved symbol. This was previously flagged; consider rejecting cfg‑gated module_exec or requiring a non‑gated fallback.

🛠️ Proposed fix

-        if let Item::Fn(func) = item
-            && func.sig.ident == "module_exec"
-        {
-            context.has_module_exec = true;
-        }
+        if let Item::Fn(func) = item
+            && func.sig.ident == "module_exec"
+        {
+            let has_cfg = func.attrs.iter().any(|a| a.path().is_ident("cfg"));
+            if has_cfg {
+                context.errors.push(syn::Error::new_spanned(
+                    &func.sig.ident,
+                    "`module_exec` must not be cfg-gated; move the cfg to the module or provide a non-gated fallback",
+                ));
+            } else {
+                context.has_module_exec = true;
+            }
+        }

#!/bin/bash
# Find cfg-gated module_exec definitions to validate impact.
rg -n --type=rust '#\[cfg[^\]]*\]\s*\n\s*fn\s+module_exec' -C2

crates/vm/src/stdlib/mod.rs (1)

96-102: Avoid duplicate posix::module_def registration on WASI.
Both cfg blocks match on WASI, so posix::module_def(ctx) is added twice. Keep a single entry to prevent double registration.

🛠 Suggested fix

-        #[cfg(any(unix, target_os = "wasi"))]
-        posix::module_def(ctx),
+        #[cfg(unix)]
+        posix::module_def(ctx),

crates/wasm/src/vm_class.rs (1)

29-34: Propagate __module_exec errors.
The return value is ignored, so init failures won’t surface.

🔧 Suggested fix

-        __module_exec(vm, module);
+        __module_exec(vm, module)?;

crates/vm/src/stdlib/sys.rs (1)

168-174: Deduplicate sys.builtin_module_names entries.

module_defs may already contain sys/builtins, so pushing them unconditionally can introduce duplicates in the exported tuple. Add dedup() after sorting to ensure uniqueness.

🩹 Proposed fix

         module_names.sort();
+        module_names.dedup();

crates/vm/src/stdlib/sysconfigdata.rs (1)

14-23: Optional: skip redundant sys.modules overwrite.

At Line 21–23, consider checking if sys.modules already contains the platform-specific key before setting it to avoid unnecessary mutation during reloads.

crates/vm/src/vm/interpreter.rs (1)

130-146: Consider returning global_state from the VM rather than cloning.

The comment on line 131 mentions "Don't clone here" but line 145 does clone vm.state. Since vm.state is already a PyRc<PyGlobalState>, the clone is cheap (just reference count bump). However, the function could simply return vm.state.clone() directly without the intermediate variable for clarity.

♻️ Optional simplification

     vm.initialize();

-    // Clone global_state for Interpreter after all initialization is done
-    let global_state = vm.state.clone();
-    (vm, global_state)
+    (vm, vm.state.clone())
 }