Fix some pyo3 testcase against the c-api#8349
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe C API updates adjust submodule import behavior, static method wrapping, attribute existence checks, and capsule null-pointer handling. Tests cover each modified path, including submodule access, zero-argument static functions, attribute queries, and invalidated capsules. ChangesC API correctness
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
a7e985d to
f9b0705
Compare
f9b0705 to
1e0f75a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/capi/src/import.rs`:
- Around line 11-22: Update the import flow to retain the result of the initial
vm.import(name, 0) call and return that value when sys_modules lacks the
requested entry, instead of invoking vm.import a second time. Preserve the
existing cached-module lookup and return the cached module when present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0f3bf272-6628-4ec2-9a4e-511e0fbb2ceb
📒 Files selected for processing (5)
crates/capi/src/import.rscrates/capi/src/methodobject.rscrates/capi/src/object.rscrates/capi/src/pycapsule.rscrates/vm/src/builtins/capsule.rs
| vm.import(name, 0)?; | ||
|
|
||
| let sys_modules = vm | ||
| .sys_module | ||
| .get_attr(rustpython_vm::identifier!(vm, modules), vm)?; | ||
| let sys_modules = sys_modules.try_downcast_ref::<PyDict>(vm)?; | ||
|
|
||
| if let Some(module) = sys_modules.get_item_opt(name, vm)? { | ||
| Ok(module) | ||
| } else { | ||
| vm.import(name, 0) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== rust tooling availability =="
command -v rustc || true
command -v cargo || true
echo "== locate files =="
fd -a 'import\.rs$' . | sed 's#^\./##'
echo "== inspect target file =="
cat -n crates/capi/src/import.rs
echo "== search vm.import usages/signatures =="
rg -n "fn import|\.import\(" crates -SRepository: RustPython/RustPython
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git diff stat/name status =="
git diff --stat || true
git diff -- crates/capi/src/import.rs || true
echo "== nearby tests =="
cat -n crates/capi/src/import.rs | sed -n '70,105p'
echo "== repository rustfmt/clippy availability =="
if command -v cargo >/dev/null 2>&1; then
cargo fmt -- --check -q || true
cargo clippy -- -D warnings -- -W clippy::all 2>&1 | head -n 200 || true
fiRepository: RustPython/RustPython
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
timeout 30 bash -c 'cat -n crates/capi/src/import.rs || true' || trueRepository: RustPython/RustPython
Length of output: 200
Reuse the first import result instead of calling vm.import twice.
The fallback reaches vm.import(name, 0) only when the initial import succeeded but did not cache sys_modules[name]. Returning that successful result avoids re-running module creation/import-time side effects while still preserving the intended cache-presence behavior.
Proposed fix
- vm.import(name, 0)?;
+ let imported = vm.import(name, 0)?;
let sys_modules = vm
.sys_module
.get_attr(rustpython_vm::identifier!(vm, modules), vm)?;
@@
if let Some(module) = sys_modules.get_item_opt(name, vm)? {
Ok(module)
} else {
- vm.import(name, 0)
+ Ok(imported)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| vm.import(name, 0)?; | |
| let sys_modules = vm | |
| .sys_module | |
| .get_attr(rustpython_vm::identifier!(vm, modules), vm)?; | |
| let sys_modules = sys_modules.try_downcast_ref::<PyDict>(vm)?; | |
| if let Some(module) = sys_modules.get_item_opt(name, vm)? { | |
| Ok(module) | |
| } else { | |
| vm.import(name, 0) | |
| } | |
| let imported = vm.import(name, 0)?; | |
| let sys_modules = vm | |
| .sys_module | |
| .get_attr(rustpython_vm::identifier!(vm, modules), vm)?; | |
| let sys_modules = sys_modules.try_downcast_ref::<PyDict>(vm)?; | |
| if let Some(module) = sys_modules.get_item_opt(name, vm)? { | |
| Ok(module) | |
| } else { | |
| Ok(imported) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/capi/src/import.rs` around lines 11 - 22, Update the import flow to
retain the result of the initial vm.import(name, 0) call and return that value
when sys_modules lacks the requested entry, instead of invoking vm.import a
second time. Preserve the existing cached-module lookup and return the cached
module when present.
There was a problem hiding this comment.
I've now a different implementation using import_from. Can you verify if this is correct?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Import API definitions and implementations =="
rg -n -A35 -B8 --glob '*.rs' \
'pub\s*(?:\([^)]*\)\s*)?fn\s+import_from\b|pub\s*(?:\([^)]*\)\s*)?fn\s+import\b' \
crates
echo "== C API import entry points and direct usages =="
rg -n -A5 -B5 --glob '*.rs' \
'\bPyImport_Import\s*\(|\bimport_from\s*\(' \
crates/capi crates 2>/dev/null || true
echo "== Relevant test/dependency locations =="
fd -a -t f . | rg -i '(pyo3|import).*\.rs$' | head -n 100
echo "== Current target file with line numbers =="
cat -n crates/capi/src/import.rsLength of output: 11668
🤖 web_search executed:
CPython PyImport_Import source implementation dotted module name fromlist documentation Python 3.14
Length of output: 19861
@bschoenmaeckers, this is not quite correct as a general PyImport_Import implementation.
vm.import_from(name, ("*",), 0) does return the requested submodule—so py.import("collections.abc") now yields collections.abc and the added regression test is covered. However, "*" is semantically meaningful: import handling may process a package’s __all__ and import additional members. PyImport_Import should not introduce those unrelated side effects merely to obtain a dotted module result.
The documented C API behavior also distinguishes an empty versus non-empty from-list for dotted imports, so I need to confirm the exact CPython implementation before determining whether the desired compatibility behavior is to return the top-level package or the resolved submodule.
| with_vm(|vm| { | ||
| let name = unsafe { (&*name).try_downcast_ref::<PyStr>(vm)? }; | ||
| vm.import(name, 0) | ||
| let from_list = PyTuple::new_ref_typed(vec![vm.ctx.new_str("*")], &vm.ctx); |
There was a problem hiding this comment.
is this correct change? I didn't check code yet, but looking not matching to documented behavior
https://docs.python.org/3/c-api/import.html#c.PyImport_Import
There was a problem hiding this comment.
Before this change the testcase failed. It returned the top level module instead of abc. Current implementation passes all tests so it may be correct.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/capi/src/import.rs`:
- Around line 11-12: Update the from_list sentinel in the PyImport_Import
implementation to use a non-wildcard name such as "__doc__" instead of "*". Keep
the existing vm.import_from call and module import behavior unchanged while
preventing wildcard __all__ expansion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 42c9ce2a-60a1-49bd-b9d8-50b637fcaad3
📒 Files selected for processing (1)
crates/capi/src/import.rs
| let from_list = PyTuple::new_ref_typed(vec![vm.ctx.new_str("*")], &vm.ctx); | ||
| vm.import_from(name, &from_list, 0) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching import.rs:\n'
fd -a 'import\.rs$' . | sed 's#^\./##'
printf '\nFile outline:\n'
ast-grep outline crates/capi/src/import.rs --view compact || true
printf '\nRelevant source:\n'
cat -n crates/capi/src/import.rs
printf '\nSearch for import_from and PyImport_Import references:\n'
rg -n "import_from|PyImport_Import|PyImport_ImportModule|PyImport_Import\\(|new_ref_typed|__all__|__doc__" crates/capi src . --glob '*.rs' | head -200Repository: RustPython/RustPython
Length of output: 12300
🌐 Web query:
CPython Python/import.c PyImport_ImportModuleNoBlock __all__ dummy "__doc__"
💡 Result:
PyImport_ImportModuleNoBlock was a C API function in CPython that historically provided a way to import modules without blocking on the global import lock [1][2]. Background and Functionality: In older versions of CPython, a global import lock was used to protect the module import process, which could lead to deadlocks in multi-threaded applications [2]. PyImport_ImportModuleNoBlock was introduced as a "clumsy workaround" to allow C extensions to import modules without waiting for this lock, raising an ImportError instead if the lock was held by another thread [3][2]. Changes in Python 3.3+: With the introduction of per-module import locks in Python 3.3, the original purpose of this function became obsolete [1][2]. Consequently, PyImport_ImportModuleNoBlock was refactored to simply act as an alias for PyImport_ImportModule [4][3][5]. Deprecation and Removal: Due to its redundancy, the function was deprecated in Python 3.13 and is scheduled for removal in Python 3.15 [1][5]. Developers are advised to use PyImport_ImportModule instead [1]. Regarding the specific terms in your query ("all", "dummy", "doc"): These are standard Python identifiers and attributes [6]. While they appear throughout the CPython codebase (often used for defining module exports or docstrings), they are not specifically or uniquely associated with the internal logic of PyImport_ImportModuleNoBlock [3][7]. The function's implementation in Python/import.c is straightforward, simply wrapping the standard import call [4][3].
Citations:
- 1: https://docs.python.org/3/c-api/import.html
- 2: https://docs.python.org/3/whatsnew/3.3.html
- 3: https://hg.python.org/cpython/rev/edb9ce3a6c2e
- 4: https://android.googlesource.com/platform/external/python/cpython3/+/2e31e5f83d4ef377099c9e71427b7e31e1da145f/Python/import.c
- 5: C API: Deprecate PyImport_ImportModuleNoBlock() function python/cpython#105396
- 6: https://www.princemishra.com/2021/07/how-python-import-system-works.html
- 7: https://hg.python.org/lookup/edb9ce3a6c2e
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Import implementation around import_from/import_code:\n'
sed -n '320,430p' crates/vm/src/import.rs | cat -n -v
printf '\nFrame import_from and __all__ handling:\n'
sed -n '6920,6985p' crates/vm/src/frame.rs | cat -n -v
printf '\nC API implementation around PyImport_Import calls:\n'
rg -n "PyImport_Import|Module_Lookup|Import_Import|fromlist|__all__" crates/vm/src crates/capi/src crates/stdlib/src --glob '*.rs' | head -200
printf '\nPython runtime behavior for wildcard/non-wildcard imports into a pkg with __all__:\n'
cat > /tmp/rustpython_wildcard_probe.py <<'PY'
import sys
import pathlib
sys.path.insert(0, str(pathlib.Path(__file__).parent))
import importlib.util
spec = importlib.util.find_spec('collections.abc')
target = pathlib.Path(spec.origin).parent
(target / '_wildcardpkg.py').write_text("import sys\n")
(target / '_nonwild.py').write_text("import sys\n")
(target / '_wildcardpkg' / 'mod1.py').write_text("import sys\n")
(target / '_nonwild' / 'mod1.py').write_text("import sys\n")
imp = __import__('_wildcardpkg', fromlist=['*'])
imp = __import__('_nonwild', fromlist=['__doc__'])
PY
python3 /tmp/rustpython_wildcard_probe.py || trueRepository: RustPython/RustPython
Length of output: 10480
🌐 Web query:
RustPython crates/vm/src/import.rs import_from from_list "*"
💡 Result:
In RustPython, import_from is a method on the VirtualMachine struct used to perform Python's "from... import..." statement, while from_list refers to the list of names being imported from the specified module. Technical Details: - The import_from method on VirtualMachine is designed to be equivalent to executing Python code like from module_name import item1, item2 [1]. - In the RustPython codebase (specifically within crates/vm/src/vm/mod.rs and the VirtualMachine implementation), from_list is typically represented as a tuple of strings (&Py<PyTuple<PyStrRef>>), representing the symbols to be imported from the target module [1][2]. - Internally, this often involves invoking the Python importlib machinery. The import system in RustPython is largely designed to leverage the standard Python importlib (a pure Python implementation of the import machinery) by bootstrapping it, rather than re-implementing the entire complex import logic in Rust [3]. - The import_from function signature generally requires the module_name, the from_list (a tuple of names to import), and a level (for relative imports, where 0 is an absolute import) [1][2]. - When bytecode is executed, the IMPORT_FROM opcode is used to retrieve an individual name from the imported module [3][4]. If you are working with the source code in crates/vm/src/import.rs, you will find utilities for handling package resolution, such as resolve_name for calculating relative paths and calc_package for determining the package context from globals [5]. The actual coordination between the Rust VM and the Python-level importlib is a core part of how these functions maintain compatibility with standard Python import semantics [5][3].
Citations:
- 1: https://docs.rs/rustpython-vm/latest/rustpython_vm/vm/struct.VirtualMachine.html
- 2: ever0de@eef8890
- 3: Implement import by using importlib #958
- 4: https://github.com/RustPython/RustPython/blob/885cf5c2/scripts/dis_dump.py
- 5: https://github.com/RustPython/RustPython/blob/885cf5c2/crates/vm/src/import.rs
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Use a tmpdir outside site-packages so the probe can write files safely.
tmpdir="$(mktemp -d)"
cd "$tmpdir"
cat > probe.py <<'PY'
import sys
sys.path.insert(0, ".")
pathlib(__import__("pathlib").Path("pkg_star")).mkdir()
pathlib("pkg_star/mod1.py").write_text("")
pathlib("pkg_star").write_bytes(b"from . import mod1")
pkg = __import__("pkg_star", fromlist=["*"])
print("pkg_star.star_imported", "mod1" in sys.modules)
sys.modules.pop("pkg_star.mod1", None)
pathlib("pkg_nostar").mkdir()
pathlib("pkg_nostar/mod1.py").write_text("")
pathlib("pkg_nostar").write_bytes(b"from . import mod1")
pkg = __import__("pkg_nostar", fromlist=["__doc__"])
print("pkg_nostar.star_imported", "pkg_nostar.mod1" in sys.modules)
PY
python3 probe.pyRepository: RustPython/RustPython
Length of output: 354
🏁 Script executed:
#!/bin/bash
set -euo pipefail
tmpdir="$(mktemp -d)"
cd "$tmpdir"
cat > probe.py <<'PY'
import sys, pathlib
pathlib.Path("pkg_star").mkdir(parents=True)
pathlib.Path("pkg_star/mod1.py").write_text("")
pathlib.Path("pkg_star/__init__.py").write_bytes(b"from . import mod1")
pkg = __import__("pkg_star", fromlist=["*"])
print("pkg_star.star_imported", "pkg_star.mod1" in sys.modules)
del sys.modules["pkg_star.mod1"]
pathlib.Path("pkg_nostar").mkdir(parents=True)
pathlib.Path("pkg_nostar/mod1.py").write_text("")
pathlib.Path("pkg_nostar/__init__.py").write_bytes(b"from . import mod1")
pkg = __import__("pkg_nostar", fromlist=["__doc__"])
print("pkg_nostar.star_imported", "pkg_nostar.mod1" in sys.modules)
PY
python3 probe.pyRepository: RustPython/RustPython
Length of output: 389
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat > /tmp/python_import_check.py <<'PY'
import importlib.util
spec = importlib.util.find_spec("collections.abc")
print(spec.origin)
print(spec.submodule_search_locations)
PY
python3 /tmp/python_import_check.py
cat > /tmp/collections_abc_probe.py <<'PY'
import sys
pkg = __import__("collections.abc", fromlist=["__doc__"])
print("seq_loaded", "collections.abc.Sequence" in sys.modules)
print("abc_loaded", pkg is sys.modules["collections.abc"])
print("has Sequence", hasattr(pkg, "Sequence"))
PY
python3 /tmp/collections_abc_probe.pyRepository: RustPython/RustPython
Length of output: 255
🌐 Web query:
RustPython crates/vm/src/vm/mod.rs VirtualMachine import_from __doc__ fromlist
💡 Result:
In RustPython, the VirtualMachine::import_from method is a core function used to implement the Python import mechanism, specifically handling statements like from module_name import item1, item2 [1]. The method signature is defined as follows: pub fn import_from<'a>( &self, module_name: impl AsPyStr<'a>, from_list: &Py, level: usize,) -> PyResult Parameters: 1. module_name: The name of the module to import from [1]. 2. from_list: A tuple of strings (PyTuple) representing the names (or attributes) to be imported from the module [1]. This corresponds to the list of names specified after the import keyword in a Python from... import... statement [2]. 3. level: An integer indicating whether to use absolute or relative imports (0 for absolute, >0 for relative) [1][3]. Functionality: This method is roughly equivalent to the Python statement from module_name import item1, item2 [1]. Internally, it invokes the import system to locate the module and then extracts the requested symbols specified in the from_list [1][4]. The VirtualMachine struct is defined in crates/vm/src/vm/mod.rs [5], and the import logic frequently interacts with the importlib machinery initialized during the VM setup [4]. While VirtualMachine::import handles standard imports (import module_name), import_from provides the necessary mechanism for accessing specific objects from within a module [1].
Citations:
- 1: https://docs.rs/rustpython-vm/latest/rustpython_vm/vm/struct.VirtualMachine.html
- 2: https://docs.rs/rustpython-ast/latest/rustpython_ast/struct.StmtImportFrom.html
- 3: https://github.com/RustPython/RustPython/blob/1bc0f2b87fa5f6d2b26416befd74dc5ee52d75d6/vm/src/vm.rs
- 4: https://github.com/RustPython/RustPython/blob/885cf5c2/crates/vm/src/import.rs
- 5: https://github.com/RustPython/RustPython/blob/885cf5c2/crates/vm/src/vm/mod.rs
Use a non-wildcard sentinel for PyImport_Import.
PyImport_Import is a full-module import API, but passing ["*"] calls RustPython’s import_from path and treats it as a wildcard from ... import *, which expands package __all__ before returning the module. Prefer a dummy fromlist such as ["__doc__"] to avoid side effects while still avoiding the Python wildcard logic.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/capi/src/import.rs` around lines 11 - 12, Update the from_list
sentinel in the PyImport_Import implementation to use a non-wildcard name such
as "__doc__" instead of "*". Keep the existing vm.import_from call and module
import behavior unchanged while preventing wildcard __all__ expansion.
Found some bugs while running pyo3 test-suite against our implementation. Copied relevant testcases and fixed them.
Summary by CodeRabbit