Use PyO3 main & enable c-api tests#8000
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds many C-API exports and VM helpers, enables previously-disabled test modules, consolidates imports, tweaks builtin ChangesC-API Surface and Test Expansion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 |
📦 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:)
e6505c1 to
176a6ac
Compare
bf75de7 to
26b96e3
Compare
Summary by CodeRabbit
New Features
Tests
Chores
Bug Fixes