Fix test_import: import machinery, circular imports, and script shadowing#7034
Fix test_import: import machinery, circular imports, and script shadowing#7034youknowone merged 2 commits into
Conversation
📝 WalkthroughWalkthroughRefactors import machinery and dotted-import codegen: adds importlib wiring and helpers for origin/spec, stdlib-shadow detection, richer ImportError/AttributeError messages, enhanced frozen-module aliasing and frozen-code handling, plus codegen changes to expand dotted imports with aliases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Builtins as "__import__ (builtins)"
participant Import as "import_module_level"
participant Importlib as "vm.importlib"
participant Sys as "sys.modules / _find_and_load"
participant Frozen as "frozen lookup"
Caller->>Builtins: __import__(name, globals, locals, fromlist, level)
Builtins->>Import: import_module_level(name, globals, fromlist, level)
Import->>Importlib: consult importlib / __spec__ (origin)
Import->>Sys: check sys.modules for existing module
alt found in sys.modules
Sys-->>Import: module (may be initializing)
Import->>Caller: return module or raise partial-init ImportError (with origin/path)
else not found
Import->>Frozen: check frozen modules & aliases
Frozen-->>Import: frozen module or not found
alt module found
Import->>Importlib: load/execute module, set __spec__/__loader__
Import->>Caller: return module
else not found
Import->>Caller: raise ImportError (with origin/path metadata)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Sorry, something went wrong.
|
Code has been automatically formatted The code in this PR has been formatted using:
git pull origin test_import |
Sorry, something went wrong.
61d81a1 to
68aa709
Compare
February 8, 2026 14:45
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/vm/src/frame.rs (1)
386-393: ⚠️ Potential issue | 🟡 Minor
__dict__fallback callsrequire_strwhich raisesTypeErroron non-string keys — CPython only issues aDeprecationWarningand skips.In CPython's
import_all_from, when iterating__dict__(no__all__), non-string keys are skipped with aDeprecationWarning, not a hardTypeError. This code would raise where CPython would continue. If strict CPython compatibility is desired fortest_import, consider filtering/skipping non-string keys instead.
🤖 Fix all issues with AI agents
In `@crates/vm/src/import.rs`:
- Around line 347-471: The has_from truthiness check in import_module_level
currently swallows errors from fl.clone().try_to_bool(vm) via .ok(), so any
exception from __bool__ is ignored; change the logic around the fromlist
handling (the has_from computation) to call try_to_bool(vm) directly and
propagate any Err returned (instead of mapping to Option via .ok()), so that
errors from __bool__ bubble up as PyErrs; locate the has_from block around the
fromlist variable and replace the .and_then(|fl|
fl.clone().try_to_bool(vm).ok()) pattern with code that handles try_to_bool(vm)
-> Result<bool, PyErr> and returns Err when it fails.
In `@crates/vm/src/stdlib/imp.rs`:
- Around line 286-309: The unsafe cast in update_code_filenames that writes to
code.code.source_path must be replaced by safe interior mutability: change the
CodeObject field source_path to an interior-mutable type (e.g., Cell<&'static
PyStrInterned> or an AtomicPtr-backed wrapper) and update its initialization
sites accordingly, then modify update_code_filenames to call the safe setter
(e.g., code.code.source_path.set(new_name) or swap the pointer atomically)
instead of performing the raw pointer write; ensure recursive traversal (the
loop calling update_code_filenames on nested PyCode) and any synchronization via
existing VM locks (IMP_LOCK) remain correct.
🧹 Nitpick comments (4)
crates/vm/src/vm/interpreter.rs (1)
466-521: Consider extracting a small helper to reduce repetition in alias injection.The find-then-push pattern is repeated three times with minor variations. A small closure or helper could reduce the boilerplate while keeping intent clear.
♻️ Suggested refactor
// Collect and add frozen module aliases for test modules let mut entries: Vec<_> = iter.collect(); + + let find_code = |entries: &[(&'static str, FrozenModule)], name: &str| { + entries.iter().find(|(n, _)| *n == name).map(|(_, m)| m.code) + }; + - if let Some(hello_code) = entries - .iter() - .find(|(n, _)| *n == "__hello__") - .map(|(_, m)| m.code) - { + if let Some(hello_code) = find_code(&entries, "__hello__") { entries.push(("__hello_alias__", FrozenModule { code: hello_code, package: false })); entries.push(("__phello_alias__", FrozenModule { code: hello_code, package: true })); entries.push(("__phello_alias__.spam", FrozenModule { code: hello_code, package: false })); } - if let Some(code) = entries - .iter() - .find(|(n, _)| *n == "__phello__") - .map(|(_, m)| m.code) - { + if let Some(code) = find_code(&entries, "__phello__") { entries.push(("__phello__.__init__", FrozenModule { code, package: false })); } - if let Some(code) = entries - .iter() - .find(|(n, _)| *n == "__phello__.ham") - .map(|(_, m)| m.code) - { + if let Some(code) = find_code(&entries, "__phello__.ham") { entries.push(("__phello__.ham.__init__", FrozenModule { code, package: false })); }crates/vm/src/import.rs (1)
242-265: PreferOption<&PyObjectRef>over&Option<PyObjectRef>.Clippy flags
&Option<T>in function signatures (clippy::ref_option). UsingOption<&PyObjectRef>is more idiomatic—it avoids an unnecessary indirection and callers can simply passspec.as_ref().♻️ Proposed fix
-pub(crate) fn get_spec_file_origin( - spec: &Option<PyObjectRef>, - vm: &VirtualMachine, -) -> Option<String> { - let spec = spec.as_ref()?; +pub(crate) fn get_spec_file_origin( + spec: Option<&PyObjectRef>, + vm: &VirtualMachine, +) -> Option<String> { + let spec = spec?;As per coding guidelines, "Always run clippy to lint Rust code (
cargo clippy) before completing tasks and fix any warnings or lints that are introduced by your changes."crates/vm/src/frame.rs (1)
3298-3309: Unnecessary per-call string allocation — prefer&stror interned identifiers.
vm.ctx.new_str("__spec__")andvm.ctx.new_str("_initializing")allocate newPyStrobjects every timeis_module_initializingis called. Elsewhere in this file (e.g., line 278), raw&stris passed directly toget_attr. The same pattern works here.♻️ Proposed fix
fn is_module_initializing(module: &PyObject, vm: &VirtualMachine) -> bool { - let Ok(spec) = module.get_attr(&vm.ctx.new_str("__spec__"), vm) else { + let Ok(spec) = module.get_attr("__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 { + let Ok(initializing_attr) = spec.get_attr("_initializing", vm) else { return false; }; initializing_attr.try_to_bool(vm).unwrap_or(false) }crates/vm/src/stdlib/imp.rs (1)
254-254: Return type readability: consider a type alias for this complex tuple.
Option<(Option<PyRef<PyMemoryView>>, bool, Option<PyStrRef>)>is hard to parse at a glance. A named struct (even file-local) would improve readability, though this is non-blocking.
Sorry, something went wrong.
- Emit IMPORT_FROM instead of LOAD_ATTR for `import a.b.c as m` - Add "partially initialized module" error for circular imports - Add "cannot access submodule" error for initializing submodules - Implement script shadowing detection with "consider renaming" hint - Detect user scripts shadowing stdlib/third-party modules - Compute original sys.path[0] from sys.argv[0] - Check sys.stdlib_module_names for stdlib detection - Respect safe_path setting - Implement _imp._fix_co_filename for code source_path rewriting - Add data parameter to _imp.get_frozen_object with marshal deser - Fix import_from: check __spec__.has_location before using origin - Set ImportError.path attribute on import failures - Fix import_star error messages for non-str __all__/__dict__ items - Always call builtins.__import__ in import_inner
68aa709 to
0fc6522
Compare
February 8, 2026 15:00
Previously, try_to_bool errors were silently swallowed via .ok(), causing fromlist with broken __bool__ to default to false.
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: (module 'hello_only' not found) [x] test: cpython/Lib/test/test_frozen.py dependencies: dependent tests: (no tests depend on frozen) [ ] test: cpython/Lib/test/test_import (TODO: 3) dependencies: dependent tests: (no tests depend on import) [x] lib: cpython/Lib/importlib dependencies:
dependent tests: (67 tests)
Legend:
|
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@crates/vm/src/import.rs`:
- Around line 288-325: Compute and store the original sys.path[0] once at VM
initialization (in init_importlib_base or VM::new) instead of recomputing it
inside import.rs; add a new VM field (e.g., cached_sys_path_0: Option<String> or
sys_path_0) and initialize it by reading sys_module.argv[0] once using the
existing logic, then update the code in import.rs to read vm.cached_sys_path_0
(falling back to previous behavior or returning false if None) rather than
re-deriving from vm.sys_module.get_attr("argv") on every call; ensure the field
name you add matches uses in import-related functions (e.g., replace local
sys_path_0 computation with vm.cached_sys_path_0) and handle empty-string and
current_dir cases the same as before.
🧹 Nitpick comments (1)
crates/vm/src/import.rs (1)
504-542: Silently swallowingrich_compare_boolerror on line 519 is acceptable for a warning-only path, but worth a brief comment.The
unwrap_or(false)on therich_compare_boolresult means a broken__ne__on a package string silently suppresses the deprecation warning rather than propagating the error. This is intentional (we're inside a best-effort warning block), but a one-line// Ignore comparison errors; this is only advisory.would clarify intent for future readers.
Sorry, something went wrong.
470bd59
into
RustPython:main
Feb 8, 2026
import a.b.c as m(compile.rs)Summary by CodeRabbit
New Features
Bug Fixes