fix(vm): provide detailed error for circular from imports

ever0de · ever0de · commit 9c09aef303a9 · 2025-07-15T00:03:42.000+09:00
diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py
@@ -1380,8 +1380,6 @@ def test_crossreference2(self):
         self.assertIn('partially initialized module', errmsg)
         self.assertIn('circular import', errmsg)
 
-    # TODO: RUSTPYTHON
-    @unittest.expectedFailure
     def test_circular_from_import(self):
         with self.assertRaises(ImportError) as cm:
             import test.test_import.data.circular_imports.from_cycle1
diff --git a/vm/src/builtins/module.rs b/vm/src/builtins/module.rs
@@ -1,3 +1,5 @@
+use std::sync::atomic::AtomicBool;
+
 use super::{PyDict, PyDictRef, PyStr, PyStrRef, PyType, PyTypeRef};
 use crate::{
     AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,
@@ -51,6 +53,8 @@ pub struct PyModule {
     // weaklist
     // for logging purposes after md_dict is cleared
     pub name: Option<&'static PyStrInterned>,
+
+    pub(crate) initializing: AtomicBool,
 }
 
 impl PyPayload for PyModule {
@@ -73,13 +77,15 @@ impl PyModule {
         Self {
             def: None,
             name: None,
+            initializing: AtomicBool::new(false),
         }
     }
 
     pub const fn from_def(def: &'static PyModuleDef) -> Self {
         Self {
             def: Some(def),
             name: Some(def.name),
+            initializing: AtomicBool::new(false),
         }
     }
 
diff --git a/vm/src/frame.rs b/vm/src/frame.rs
@@ -1362,20 +1362,50 @@ impl ExecutingFrame<'_> {
     #[cfg_attr(feature = "flame-it", flame("Frame"))]
     fn import_from(&mut self, vm: &VirtualMachine, idx: bytecode::NameIdx) -> PyResult {
         let module = self.top_value();
-        let name = self.code.names[idx as usize];
-        let err = || vm.new_import_error(format!("cannot import name '{name}'"), name.to_owned());
+        let name_to_import = self.code.names[idx as usize];
+
         // Load attribute, and transform any error into import error.
-        if let Some(obj) = vm.get_attribute_opt(module.to_owned(), name)? {
+        if let Some(obj) = vm.get_attribute_opt(module.to_owned(), name_to_import)? {
             return Ok(obj);
         }
-        // fallback to importing '{module.__name__}.{name}' from sys.modules
-        let mod_name = module
-            .get_attr(identifier!(vm, __name__), vm)
-            .map_err(|_| err())?;
-        let mod_name = mod_name.downcast::<PyStr>().map_err(|_| err())?;
-        let full_mod_name = format!("{mod_name}.{name}");
-        let sys_modules = vm.sys_module.get_attr("modules", vm).map_err(|_| err())?;
-        sys_modules.get_item(&full_mod_name, vm).map_err(|_| err())
+
+        let fallback_result: Option<PyResult> = module
+            .get_attr(&vm.ctx.new_str("__name__"), vm)
+            .ok()
+            .and_then(|mod_name| mod_name.downcast_ref::<PyStr>().map(|s| s.to_owned()))
+            .and_then(|mod_name_str| {
+                let full_mod_name =
+                    format!("{}.{}", mod_name_str.as_str(), name_to_import.as_str());
+                vm.sys_module
+                    .get_attr("modules", vm)
+                    .ok()
+                    .and_then(|sys_modules| sys_modules.get_item(&full_mod_name, vm).ok())
+            })
+            .map(Ok);
+
+        if let Some(Ok(sub_module)) = fallback_result {
+            return Ok(sub_module);
+        }
+
+        if is_module_initializing(module, vm) {
+            let module_name = module
+                .get_attr(&vm.ctx.new_str("__name__"), vm)
+                .ok()
+                .and_then(|n| n.downcast_ref::<PyStr>().map(|s| s.as_str().to_owned()))
+                .unwrap_or_else(|| "<unknown>".to_owned());
+
+            let msg = format!(
+                "cannot import name '{}' from partially initialized module '{}' (most likely due to a circular import)",
+                name_to_import.as_str(),
+                module_name
+            );
+            Err(vm.new_import_error(msg, name_to_import.to_owned()))
+        } else {
+            Err(vm.new_import_error(
+                format!("cannot import name '{}'", name_to_import.as_str()),
+                name_to_import.to_owned(),
+            ))
+        }
     }
 
     #[cfg_attr(feature = "flame-it", flame("Frame"))]
@@ -2372,3 +2402,15 @@ impl fmt::Debug for Frame {
         )
     }
 }
+
+fn is_module_initializing(module: &PyObject, vm: &VirtualMachine) -> bool {
+    let spec = match module.get_attr(&vm.ctx.new_str("__spec__"), vm) {
+        Ok(spec) if !vm.is_none(&spec) => spec,
+        _ => return false,
+    };
+    let initializing_attr = match spec.get_attr(&vm.ctx.new_str("_initializing"), vm) {
+        Ok(attr) => attr,
+        Err(_) => return false,
+    };
+    initializing_attr.try_to_bool(vm).unwrap_or(false)
+}
diff --git a/vm/src/import.rs b/vm/src/import.rs
@@ -1,8 +1,10 @@
 //! Import mechanics
 
+use std::sync::atomic::Ordering;
+
 use crate::{
     AsObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject,
-    builtins::{PyBaseExceptionRef, PyCode, list, traceback::PyTraceback},
+    builtins::{PyBaseExceptionRef, PyCode, PyModule, list, traceback::PyTraceback},
     scope::Scope,
     version::get_git_revision,
     vm::{VirtualMachine, thread},
@@ -156,9 +158,25 @@ pub fn import_code_obj(
     let sys_modules = vm.sys_module.get_attr("modules", vm)?;
     sys_modules.set_item(module_name, module.clone().into(), vm)?;
 
-    // Execute main code in module:
-    let scope = Scope::with_builtins(None, attrs, vm);
-    vm.run_code_obj(code_obj, scope)?;
+    {
+        struct InitializingGuard<'a> {
+            module: &'a PyModule,
+        }
+
+        impl<'a> Drop for InitializingGuard<'a> {
+            fn drop(&mut self) {
+                self.module.initializing.store(false, Ordering::Relaxed);
+            }
+        }
+
+        module.initializing.store(true, Ordering::Relaxed);
+        let _guard = InitializingGuard { module: &module };
+
+        // Execute main code in module:
+        let scope = Scope::with_builtins(None, attrs, vm);
+        vm.run_code_obj(code_obj, scope)?;
+    }
+
     Ok(module.into())
 }
 

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,5 @@`
	`1`	`+use std::sync::atomic::AtomicBool;`
	`2`	`+`
`1`	`3`	`use super::{PyDict, PyDictRef, PyStr, PyStrRef, PyType, PyTypeRef};`
`2`	`4`	`use crate::{`
`3`	`5`	`AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,`
`@@ -51,6 +53,8 @@ pub struct PyModule {`
`51`	`53`	`// weaklist`
`52`	`54`	`// for logging purposes after md_dict is cleared`
`53`	`55`	`pub name: Option<&'static PyStrInterned>,`
	`56`	`+`
	`57`	`+ pub(crate) initializing: AtomicBool,`
`54`	`58`	`}`
`55`	`59`
`56`	`60`	`impl PyPayload for PyModule {`
`@@ -73,13 +77,15 @@ impl PyModule {`
`73`	`77`	`Self {`
`74`	`78`	`def: None,`
`75`	`79`	`name: None,`
	`80`	`+ initializing: AtomicBool::new(false),`
`76`	`81`	`}`
`77`	`82`	`}`
`78`	`83`
`79`	`84`	`pub const fn from_def(def: &'static PyModuleDef) -> Self {`
`80`	`85`	`Self {`
`81`	`86`	`def: Some(def),`
`82`	`87`	`name: Some(def.name),`
	`88`	`+ initializing: AtomicBool::new(false),`
`83`	`89`	`}`
`84`	`90`	`}`
`85`	`91`