From d86644e4565213755b973a57bcbc6e98dbf7bed9 Mon Sep 17 00:00:00 2001 From: Jiseok CHOI Date: Mon, 14 Jul 2025 22:08:15 +0900 Subject: [PATCH 1/5] fix(vm): provide detailed error for circular `from` imports --- Lib/test/test_import/__init__.py | 2 -- vm/src/builtins/module.rs | 6 ++++ vm/src/frame.rs | 60 +++++++++++++++++++++++++++----- vm/src/import.rs | 26 +++++++++++--- 4 files changed, 79 insertions(+), 15 deletions(-) diff --git a/Lib/test/test_import/__init__.py b/Lib/test/test_import/__init__.py index 89e5ec1534a..44e7da1033d 100644 --- 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 index f8e42b28e0b..efa8a9b6087 100644 --- 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,6 +77,7 @@ impl PyModule { Self { def: None, name: None, + initializing: AtomicBool::new(false), } } @@ -80,6 +85,7 @@ impl PyModule { Self { def: Some(def), name: Some(def.name), + initializing: AtomicBool::new(false), } } diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 460ba4392ef..2880e28068f 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -1363,19 +1363,48 @@ impl ExecutingFrame<'_> { 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()); + // Load attribute, and transform any error into import error. if let Some(obj) = vm.get_attribute_opt(module.to_owned(), name)? { 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::().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 = module + .get_attr(&vm.ctx.new_str("__name__"), vm) + .ok() + .and_then(|mod_name| mod_name.downcast_ref::().map(|s| s.to_owned())) + .and_then(|mod_name_str| { + let full_mod_name = format!("{}.{}", mod_name_str.as_str(), name.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::().map(|s| s.as_str().to_owned())) + .unwrap_or_else(|| "".to_owned()); + + let msg = format!( + "cannot import name '{}' from partially initialized module '{}' (most likely due to a circular import)", + name.as_str(), + module_name + ); + Err(vm.new_import_error(msg, name.to_owned())) + } else { + Err(vm.new_import_error( + format!("cannot import name '{}'", name.as_str()), + name.to_owned(), + )) + } } #[cfg_attr(feature = "flame-it", flame("Frame"))] @@ -2372,3 +2401,16 @@ impl fmt::Debug for Frame { ) } } + +fn is_module_initializing(module: &PyObject, vm: &VirtualMachine) -> bool { + let Ok(spec) = module.get_attr(&vm.ctx.new_str("__spec__"), vm) else { + return false; + }; + if vm.is_none(&spec) { + return false; + } + let Ok(initializing_attr) = spec.get_attr(&vm.ctx.new_str("_initializing"), vm) else { + return false; + }; + initializing_attr.try_to_bool(vm).unwrap_or(false) +} diff --git a/vm/src/import.rs b/vm/src/import.rs index c119405fe1d..6d718fe531f 100644 --- 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()) } From e4dc90f5a8c54d9eda99c257327366e2118ae352 Mon Sep 17 00:00:00 2001 From: Jiseok CHOI Date: Tue, 15 Jul 2025 00:47:21 +0900 Subject: [PATCH 2/5] refactor? --- vm/src/frame.rs | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 2880e28068f..bbaed0dd256 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -1369,20 +1369,15 @@ impl ExecutingFrame<'_> { return Ok(obj); } - let fallback_result: Option = module - .get_attr(&vm.ctx.new_str("__name__"), vm) - .ok() - .and_then(|mod_name| mod_name.downcast_ref::().map(|s| s.to_owned())) - .and_then(|mod_name_str| { - let full_mod_name = format!("{}.{}", mod_name_str.as_str(), name.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 { + let fallback_module = (|| { + let mod_name = module.get_attr(&vm.ctx.new_str("__name__"), vm).ok()?; + let mod_name_str = mod_name.downcast_ref::()?; + let full_mod_name = format!("{}.{}", mod_name_str.as_str(), name.as_str()); + let sys_modules = vm.sys_module.get_attr("modules", vm).ok()?; + sys_modules.get_item(&full_mod_name, vm).ok() + })(); + + if let Some(sub_module) = fallback_module { return Ok(sub_module); } From 9d6912c7d6309bd6638c63d180499afdea96dc45 Mon Sep 17 00:00:00 2001 From: Jiseok CHOI Date: Tue, 15 Jul 2025 00:57:28 +0900 Subject: [PATCH 3/5] remove native flag --- vm/src/builtins/module.rs | 6 ------ vm/src/import.rs | 26 ++++---------------------- 2 files changed, 4 insertions(+), 28 deletions(-) diff --git a/vm/src/builtins/module.rs b/vm/src/builtins/module.rs index efa8a9b6087..f8e42b28e0b 100644 --- a/vm/src/builtins/module.rs +++ b/vm/src/builtins/module.rs @@ -1,5 +1,3 @@ -use std::sync::atomic::AtomicBool; - use super::{PyDict, PyDictRef, PyStr, PyStrRef, PyType, PyTypeRef}; use crate::{ AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine, @@ -53,8 +51,6 @@ 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 { @@ -77,7 +73,6 @@ impl PyModule { Self { def: None, name: None, - initializing: AtomicBool::new(false), } } @@ -85,7 +80,6 @@ impl PyModule { Self { def: Some(def), name: Some(def.name), - initializing: AtomicBool::new(false), } } diff --git a/vm/src/import.rs b/vm/src/import.rs index 6d718fe531f..c119405fe1d 100644 --- a/vm/src/import.rs +++ b/vm/src/import.rs @@ -1,10 +1,8 @@ //! Import mechanics -use std::sync::atomic::Ordering; - use crate::{ AsObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject, - builtins::{PyBaseExceptionRef, PyCode, PyModule, list, traceback::PyTraceback}, + builtins::{PyBaseExceptionRef, PyCode, list, traceback::PyTraceback}, scope::Scope, version::get_git_revision, vm::{VirtualMachine, thread}, @@ -158,25 +156,9 @@ 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)?; - { - 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)?; - } - + // Execute main code in module: + let scope = Scope::with_builtins(None, attrs, vm); + vm.run_code_obj(code_obj, scope)?; Ok(module.into()) } From e53917c405569a08e47d42dbda819800d4e55464 Mon Sep 17 00:00:00 2001 From: Jiseok CHOI Date: Tue, 15 Jul 2025 01:05:47 +0900 Subject: [PATCH 4/5] use identifier & rollback comment --- vm/src/frame.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/vm/src/frame.rs b/vm/src/frame.rs index bbaed0dd256..5acdd94b15c 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -1368,9 +1368,9 @@ impl ExecutingFrame<'_> { if let Some(obj) = vm.get_attribute_opt(module.to_owned(), name)? { return Ok(obj); } - + // fallback to importing '{module.__name__}.{name}' from sys.modules let fallback_module = (|| { - let mod_name = module.get_attr(&vm.ctx.new_str("__name__"), vm).ok()?; + let mod_name = module.get_attr(identifier!(vm, __name__), vm).ok()?; let mod_name_str = mod_name.downcast_ref::()?; let full_mod_name = format!("{}.{}", mod_name_str.as_str(), name.as_str()); let sys_modules = vm.sys_module.get_attr("modules", vm).ok()?; @@ -1383,7 +1383,7 @@ impl ExecutingFrame<'_> { if is_module_initializing(module, vm) { let module_name = module - .get_attr(&vm.ctx.new_str("__name__"), vm) + .get_attr(identifier!(vm, __name__), vm) .ok() .and_then(|n| n.downcast_ref::().map(|s| s.as_str().to_owned())) .unwrap_or_else(|| "".to_owned()); From 69c0294729fa0f75c86f27c6154573f360cd2aa8 Mon Sep 17 00:00:00 2001 From: Jiseok CHOI Date: Tue, 15 Jul 2025 01:07:52 +0900 Subject: [PATCH 5/5] remove unneeded as_str --- vm/src/frame.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/vm/src/frame.rs b/vm/src/frame.rs index 5acdd94b15c..28a6ece4da0 100644 --- a/vm/src/frame.rs +++ b/vm/src/frame.rs @@ -1371,8 +1371,8 @@ impl ExecutingFrame<'_> { // fallback to importing '{module.__name__}.{name}' from sys.modules let fallback_module = (|| { let mod_name = module.get_attr(identifier!(vm, __name__), vm).ok()?; - let mod_name_str = mod_name.downcast_ref::()?; - let full_mod_name = format!("{}.{}", mod_name_str.as_str(), name.as_str()); + let mod_name = mod_name.downcast_ref::()?; + let full_mod_name = format!("{mod_name}.{name}"); let sys_modules = vm.sys_module.get_attr("modules", vm).ok()?; sys_modules.get_item(&full_mod_name, vm).ok() })(); @@ -1389,16 +1389,11 @@ impl ExecutingFrame<'_> { .unwrap_or_else(|| "".to_owned()); let msg = format!( - "cannot import name '{}' from partially initialized module '{}' (most likely due to a circular import)", - name.as_str(), - module_name + "cannot import name '{name}' from partially initialized module '{module_name}' (most likely due to a circular import)", ); Err(vm.new_import_error(msg, name.to_owned())) } else { - Err(vm.new_import_error( - format!("cannot import name '{}'", name.as_str()), - name.to_owned(), - )) + Err(vm.new_import_error(format!("cannot import name '{name}'"), name.to_owned())) } }