-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Provide detailed error for circular from imports
#5972
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -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::<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.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.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) | ||||||||||||||
| } | ||||||||||||||
|
Comment on lines
+2395
to
+2406
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainHelper function implementation is sound but relies on unset attribute. The
Consider checking the internal field directly: fn is_module_initializing(module: &PyObject, vm: &VirtualMachine) -> bool {
+ // First try to check the internal field if this is a PyModule
+ if let Ok(py_module) = module.downcast_ref::<PyModule>() {
+ use std::sync::atomic::Ordering;
+ return py_module.initializing.load(Ordering::Relaxed);
+ }
+
+ // Fallback to checking __spec__._initializing
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)
}Run the following script to check how CPython handles module initialization tracking: 🌐 Web query: 💡 Result: CPython tracks module initialization state for circular import detection by leveraging the The Instead, here is how it works:
Summary Table
In conclusion: Citations:
Streamline
fn is_module_initializing(module: &PyObject, vm: &VirtualMachine) -> bool {
- // Fallback to checking __spec__._initializing
- 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)
+ // Use internal initializing flag on PyModule
+ if let Ok(py_module) = module.downcast_ref::<PyModule>() {
+ use std::sync::atomic::Ordering;
+ return py_module.initializing.load(Ordering::Relaxed);
+ }
+ false
}🤖 Prompt for AI Agents |
||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
Critical: Disconnect between internal
initializingfield and Python-level__spec__._initializingattribute.The refactored import logic looks correct, but there's a fundamental issue: this code checks
__spec__._initializing(line 1389 viais_module_initializing), but theimport_code_objfunction inimport.rsonly sets the internalPyModule.initializingfield. There's no code that exposes this internal state to Python as__spec__._initializing.Without this connection, the circular import detection won't work as intended.
Consider either:
__spec__._initializinginimport_code_objwhen setting the internal fieldis_module_initializingto check the internalPyModule.initializingfield insteadRun the following script to verify if
__spec__._initializingis set anywhere in the codebase:🏁 Script executed:
Length of output: 62
🏁 Script executed:
Length of output: 62
🏁 Script executed:
Length of output: 1656
Critical: Circular‐import detection is broken—
spec._initializingis never setThe new
is_module_initializingpath in vm/src/frame.rs (lines 1389, 2405–2415) checks the Python‐level__spec__._initializingflag, but no code ever assigns_initializingon the module’s__spec__. We only toggle the internalPyModule.initializingfield in vm/src/import.rs (lines 168, 172), so the “partially initialized module” branch will never fire.To resolve this, choose one of the following fixes:
• In vm/src/import.rs (around lines 168–172), after calling
also set the Python‐level flag:
(and similarly for
false)• Or in vm/src/frame.rs’s
is_module_initializing(lines 2405–2415), downcastmoduletoPyModuleand read its internalinitializingflag directly instead of querying__spec__.Add appropriate tests to verify circular‐import error messages now appear.
🤖 Prompt for AI Agents