Update PyO3 to 0.29 & enable c-api tests#8000
Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (40)
📝 WalkthroughWalkthroughThe PR updates the RustPython C-API crate to use PyO3's ChangesRustPython C-API Surface Expansion and Test Activation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 Tip You can get early access to new features in CodeRabbit.Enable the |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/ctypes dependencies:
dependent tests: (34 tests)
Legend:
|
43529c0 to
a46eb71
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
Cargo.toml (1)
122-122: ⚡ Quick winPin PyO3 git
revinCargo.toml
Cargo.lockalready resolves all PyO3 crates (pyo3,pyo3-ffi,pyo3-build-config,pyo3-macros,pyo3-macros-backend) to the same commit (de5816ada27bfa38cb3176d91bd6c93e206dcb3f), butCargo.tomlstill uses bare git URLs for bothpyo3andpyo3-ffi. Addrev = "de5816ada27bfa38cb3176d91bd6c93e206dcb3f"to both entries to make the manifest explicitly reproducible.🤖 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 `@Cargo.toml` at line 122, Update the Cargo.toml git dependencies for pyo3 and pyo3-ffi to pin them to the resolved commit: add rev = "de5816ada27bfa38cb3176d91bd6c93e206dcb3f" to the pyo3 = { git = "https://github.com/PyO3/pyo3" } and pyo3-ffi = { git = "https://github.com/PyO3/pyo3" } entries so the manifest explicitly matches the commit resolved in Cargo.lock.
🤖 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 24-31: The current logic collapses all sys_modules.get_item
failures into creating a new module; instead, explicitly handle each case: call
sys_modules.get_item(name, vm) and if it returns Err, propagate or return that
error rather than creating a module; if it returns Ok(Some(value)) but
value.downcast_ref::<PyModule>() is None, return a type/mismatch error instead
of overwriting sys.modules; only when get_item returns Ok(None) should you call
vm.new_module(name, vm.ctx.new_dict(), None) and sys_modules.set_item(name,
module.clone().into(), vm)?; update the block around sys_modules.get_item,
downcast_ref::<PyModule>(), vm.new_module, and sys_modules.set_item accordingly.
- Around line 16-18: The extern "C" function PyImport_AddModuleRef currently
calls CStr::from_ptr(name).to_str().expect(...), which can panic on invalid
UTF-8; change it to return an FfiResult error instead of panicking by replacing
the expect with a to_str().map_err(|_| vm.new_system_error("Name is not valid
UTF-8"))? expression (using the existing with_vm/FfiResult flow) so invalid
UTF-8 is converted into a Python SystemError via vm.new_system_error rather than
causing a panic.
In `@crates/capi/src/object.rs`:
- Around line 205-227: PyObject_GenericGetDict and PyObject_GenericSetDict
incorrectly call obj.get_attr / obj.set_attr which go through attribute
machinery and allow __getattribute__/descriptors to intercept; instead wire
these C-API functions to the VM helpers that bypass attribute hooks. Replace the
calls in PyObject_GenericGetDict and PyObject_GenericSetDict to use the direct
helpers (object_get_dict and object_generic_set_dict from
crates/vm/src/builtins/object.rs) that read/write the underlying instance dict
(obj.dict()/obj.instance_dict()) without routing through get_attr/set_attr so
the real instance dict is returned/updated.
In `@crates/capi/src/pyerrors.rs`:
- Around line 303-308: PyException_SetCause currently clones the borrowed raw
pointer (using to_owned()) which violates CPython’s reference-stealing semantics
and leaks/increfs; change it to take ownership of the incoming raw pointer for
`cause` (use the equivalent of a from_raw/from_owned conversion on the raw `*mut
PyObject`) so the caller’s reference is consumed, map NULL to Python None as
before for clearing, and then pass that owned PyBaseException into
`exc.set___cause__` instead of cloning; ensure no extra incref remains and the
raw pointer is not used after ownership is transferred.
In `@crates/capi/src/pylifecycle.rs`:
- Around line 61-63: The function Py_GetVersion currently returns String::as_ptr
which may lack a terminating NUL; change the static LazyLock to hold a CString
(e.g. static VERSION: LazyLock<CString> = LazyLock::new(||
CString::new(format!("{MAJOR}.{MINOR}.{MICRO}")).unwrap());) and return
VERSION.as_ptr() as *const c_char so callers get a stable, NUL-terminated C
string from Py_GetVersion. Ensure you import std::ffi::CString and keep the
LazyLock so the CString lives for the program duration.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 122: Update the Cargo.toml git dependencies for pyo3 and pyo3-ffi to pin
them to the resolved commit: add rev =
"de5816ada27bfa38cb3176d91bd6c93e206dcb3f" to the pyo3 = { git =
"https://github.com/PyO3/pyo3" } and pyo3-ffi = { git =
"https://github.com/PyO3/pyo3" } entries so the manifest explicitly matches the
commit resolved in Cargo.lock.
🪄 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: 4e562741-82e6-4a0e-ad6f-0b276776160f
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockLib/test/test_ctypes/test_python_api.pyis excluded by!Lib/**
📒 Files selected for processing (22)
Cargo.tomlcrates/capi/pyo3-rustpython.configcrates/capi/src/abstract_.rscrates/capi/src/bytesobject.rscrates/capi/src/ceval.rscrates/capi/src/complexobject.rscrates/capi/src/dictobject.rscrates/capi/src/floatobject.rscrates/capi/src/import.rscrates/capi/src/lib.rscrates/capi/src/listobject.rscrates/capi/src/longobject.rscrates/capi/src/methodobject.rscrates/capi/src/moduleobject.rscrates/capi/src/object.rscrates/capi/src/pycapsule.rscrates/capi/src/pyerrors.rscrates/capi/src/pylifecycle.rscrates/capi/src/refcount.rscrates/capi/src/tupleobject.rscrates/capi/src/unicodeobject.rscrates/vm/src/stdlib/builtins.rs
07d29f9 to
2149dcb
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/vm/src/stdlib/_ctypes/function.rs`:
- Around line 1435-1455: The code incorrectly maps a NULL py_object to
Some(None) and also leaks a refcount by using ManuallyDrop + clone; update the
convert_raw_result branch that handles restype_type "_type_" == "O" so that when
ptr == 0 you raise a ValueError("PyObject is NULL") on the vm and return None
(propagating an error), and when ptr != 0 construct the PyObjectRef by moving
ownership with PyObjectRef::from_raw(NonNull::new_unchecked(ptr as *mut
PyObject)) and return Some(obj) directly (do NOT wrap in ManuallyDrop or clone),
referencing the restype_type check, RawResult variants, and
PyObjectRef::from_raw to locate and fix the code.
🪄 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: bd14ff21-a37e-48fe-bff9-21046b4badbd
📒 Files selected for processing (2)
.github/workflows/ci.yamlcrates/vm/src/stdlib/_ctypes/function.rs
|
CI failures seem unrelated? |
ShaharNaveh
left a comment
There was a problem hiding this comment.
TYSM for all the work on this!
just a minor request:)
26b96e3 to
e77a566
Compare
25a2860 to
b8b775a
Compare
b8b775a to
04b8b1a
Compare
0.29 & enable c-api tests
| implementation=CPython | ||
| version=3.14 | ||
| implementation=RustPython | ||
| version=3.15 |
There was a problem hiding this comment.
abi3t is only supported from 3.15 onwards. This does not require actual 3.15 runtime support.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
04b8b1a to
901b388
Compare
|
@youknowone This is ready for review |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/vm/src/builtins/int.rs (1)
318-334: 💤 Low valueConsider fixing
as_u32_maskto match the correct pattern inas_u64_mask.The loop
out = out.wrapping_shl(32) | digithas a subtle issue: foru32,wrapping_shl(32)shifts by32 % 32 = 0bits (per Rust semantics), so it effectively ORs all digits together rather than extracting only the lower 32 bits.The new
as_u64_maskcorrectly takes only the first N digits. Consider applying the same approach here:Suggested fix
pub fn as_u32_mask(&self) -> u32 { let v = self.as_bigint(); v.to_u32() .or_else(|| v.to_i32().map(|i| i as u32)) .unwrap_or_else(|| { - let mut out = 0u32; - for digit in v.iter_u32_digits() { - out = out.wrapping_shl(32) | digit; - } + let out = v.iter_u32_digits().next().unwrap_or(0); match v.sign() { - Sign::Minus => out * -1i32 as u32, + Sign::Minus => out.wrapping_neg(), _ => out, } }) }🤖 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/vm/src/builtins/int.rs` around lines 318 - 334, The loop in the `as_u32_mask` method has a bug where `wrapping_shl(32)` on a u32 shifts by 0 bits (since 32 % 32 = 0 in Rust), so it ORs all digits together rather than extracting only the lower 32 bits. Fix this by applying the same pattern used in `as_u64_mask`, which correctly takes only the first N digits needed. Replace the loop logic to extract just the lower 32 bits from the bigint digits instead of accumulating all digits with an ineffective shift operation.
🤖 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.
Nitpick comments:
In `@crates/vm/src/builtins/int.rs`:
- Around line 318-334: The loop in the `as_u32_mask` method has a bug where
`wrapping_shl(32)` on a u32 shifts by 0 bits (since 32 % 32 = 0 in Rust), so it
ORs all digits together rather than extracting only the lower 32 bits. Fix this
by applying the same pattern used in `as_u64_mask`, which correctly takes only
the first N digits needed. Replace the loop logic to extract just the lower 32
bits from the bigint digits instead of accumulating all digits with an
ineffective shift operation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 7decf53d-4d5a-46fc-b2b9-f48922df202c
📒 Files selected for processing (2)
crates/capi/src/longobject.rscrates/vm/src/builtins/int.rs
ShaharNaveh
left a comment
There was a problem hiding this comment.
The capi came out really clean. I like it a lot!
Thanks for all the work you've put into it<3
2ff30a0 to
1c0fc55
Compare
|
@bschoenmaeckers is this ready to go? |
Yes this is ready to merge! |
|
@youknowone can you take a look at this? |
Summary by CodeRabbit
os.fspath, critical-section stubs, interpreter state access, and integer masking utilities.