Fix ctypes.py_object handling#7836
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)
📝 WalkthroughWalkthroughAdds ctypes Changesctypes py_object Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
ShaharNaveh
left a comment
There was a problem hiding this comment.
Thanks!
If this doesn't fix anything in CPython's test_ctypes, could you please add a test in extra_tests/ so we won't have a regression for it?
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/base.rs`:
- Around line 2345-2358: The "O" match arm in base.rs reconstructs a PyObject
via read_ptr_from_buffer and uses crate::PyObjectRef::from_raw +
core::mem::ManuallyDrop::new + clone which always increments the refcount but
never consumes ownership; fix this by distinguishing borrowed vs owned pointers
and handling owned C-API returns by taking ownership (call from_raw without
wrapping in ManuallyDrop so the destructor runs / triggers a Py_DECREF when
dropped) or explicitly calling the equivalent decref for owned pointers, while
keeping the current clone path for borrowed/stored pointers; update the code
around read_ptr_from_buffer, the "O" match arm, and uses of
crate::PyObjectRef::from_raw / ManuallyDrop accordingly, or alternatively add a
clear comment in that same location documenting the chosen memory model if you
opt not to change ownership handling.
🪄 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: 039f45e8-ef72-4edd-be30-4ddd8d5a3094
📒 Files selected for processing (2)
crates/vm/src/stdlib/_ctypes/base.rscrates/vm/src/stdlib/_ctypes/simple.rs
We do not have enough capi implemented to test this. After #7787 is merged, I will be able to add a testcase. And probably implement the bytes c-api and enable the |
ShaharNaveh
left a comment
There was a problem hiding this comment.
concerns addressed at #7836 (comment)
|
Unfortunately, test_ctypes is very poor state and writing own test for ctypes is reasonably painful. |
While testing the python c-api using
ctypesin #7562, I stumbled on this bug. Previously allctypes.py_objectvalues were returned asNone. After this fix thePythonAPITestCase.test_PyBytes_FromStringAndSizetest case succeeds whenPyBytes_FromStringAndSizeis present in the c-api crate.Summary by CodeRabbit
Release Notes
py_objecttype support with improved Python object marshalling and NULL pointer validation during type conversions.