Add integer c-api support#7905
Merged
Merged
Conversation
Contributor
|
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 (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a new ChangesPython Long Integer FFI Bindings
Sequence Diagram(s)sequenceDiagram
participant C_Caller
participant FFI_Constructor as PyLong_From*
participant VM_Context as with_vm
participant VM_CTX as vm.ctx
participant PyInt as Python_int
C_Caller->>FFI_Constructor: call PyLong_From*(C integer)
FFI_Constructor->>VM_Context: enter VM context
VM_Context->>VM_CTX: vm.ctx.new_int(value)
VM_CTX->>PyInt: create Python int object
PyInt-->>FFI_Constructor: PyObject* returned
FFI_Constructor-->>C_Caller: PyObject*
sequenceDiagram
participant C_Caller
participant FFI_Extractor as PyLong_As*
participant PyObject_Ptr
participant BigInt_Conversion
participant VM_Context
C_Caller->>FFI_Extractor: call PyLong_As*(PyObject*)
FFI_Extractor->>PyObject_Ptr: dereference & coerce
PyObject_Ptr->>BigInt_Conversion: extract bigint value
BigInt_Conversion->>BigInt_Conversion: try downcast to C type
alt success
BigInt_Conversion-->>FFI_Extractor: numeric value
FFI_Extractor-->>C_Caller: return C integer
else overflow/failure
BigInt_Conversion->>VM_Context: set overflow error
VM_Context-->>FFI_Extractor: error indicator
FFI_Extractor-->>C_Caller: error sentinel (ERR_VALUE)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/capi/src/longobject.rs (1)
61-83: 🏗️ Heavy liftReplace the dead test block with enabled coverage of the exported FFI helpers.
#[cfg(false)]means none of this runs, and these tests only exercise PyO3'sPyInt, not the newPyLong_From*/PyLong_As*symbols. Please swap this for enabled tests around the FFI helpers so ABI regressions get caught.🤖 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/longobject.rs` around lines 61 - 83, Replace the dead #[cfg(false)] test module with an active #[cfg(test)] mod tests that directly exercises the crate's exported FFI helpers (the PyLong_From* / PyLong_As* symbols) instead of PyO3's PyInt: remove #[cfg(false)], add tests that call the exported functions (e.g., the crate's PyLong_From<u64>-style and PyLong_As<u64>-style helpers via their extern "C" signatures), perform round-trip conversions (create a long from a Rust integer and extract it back) and assert correctness so ABI/FFI regressions are caught; keep the test harness using Python::attach or equivalent to run under the Python GIL as in the original tests.
🤖 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/longobject.rs`:
- Around line 49-58: The function PyLong_AsUnsignedLongLong currently returns a
signed c_longlong and directly downcasts to PyInt, causing wrong behavior for
values > i64::MAX and ignoring __index__; change its return type and conversion
path to use c_ulonglong and the index protocol: in PyLong_AsUnsignedLongLong use
try_index instead of try_downcast::<PyInt>, obtain the BigInt via as_bigint()
from the indexed object, then convert using try_into::<c_ulonglong>() and map
conversion errors to vm.new_overflow_error("Python int too large to convert to C
unsigned long long"); ensure the C signature and any uses expect c_ulonglong
rather than c_longlong.
---
Nitpick comments:
In `@crates/capi/src/longobject.rs`:
- Around line 61-83: Replace the dead #[cfg(false)] test module with an active
#[cfg(test)] mod tests that directly exercises the crate's exported FFI helpers
(the PyLong_From* / PyLong_As* symbols) instead of PyO3's PyInt: remove
#[cfg(false)], add tests that call the exported functions (e.g., the crate's
PyLong_From<u64>-style and PyLong_As<u64>-style helpers via their extern "C"
signatures), perform round-trip conversions (create a long from a Rust integer
and extract it back) and assert correctness so ABI/FFI regressions are caught;
keep the test harness using Python::attach or equivalent to run under the Python
GIL as in the original tests.
🪄 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: 20f487c3-3f6e-470b-ab40-139b5fe87ea9
📒 Files selected for processing (2)
crates/capi/src/lib.rscrates/capi/src/longobject.rs
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/capi/src/longobject.rs (1)
49-58:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse the index protocol in the unsigned extractor.
This still rejects valid
__index__providers thatPyLong_AsLongaccepts, and the overflow text names the signed C type. That makesPyLong_AsUnsignedLongLongobservably inconsistent with the rest of this file.Suggested fix
- unsafe { &*obj } - .to_owned() - .try_downcast::<PyInt>(vm)? - .as_bigint() - .try_into() - .map_err(|_| vm.new_overflow_error("Python int too large to convert to C long long")) + unsafe { &*obj } + .to_owned() + .try_index(vm)? + .as_bigint() + .try_into() + .map_err(|_| { + vm.new_overflow_error("Python int too large to convert to C unsigned long long") + })#!/bin/bash set -euo pipefail echo "Compare the signed and unsigned long extractors:" sed -n '37,59p' crates/capi/src/longobject.rs echo echo "Show the coercion helpers used in this file:" rg -n -C2 'PyLong_As|try_index\(|try_downcast::<PyInt>' crates/capi/src/longobject.rs🤖 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/longobject.rs` around lines 49 - 58, The unsigned extractor PyLong_AsUnsignedLongLong currently bypasses the index protocol and uses PyInt downcast and a signed overflow message; update it to mirror the signed extractor by using the index protocol helper (e.g., call try_index or the same helper used by PyLong_AsLong) to coerce obj to an integer, then convert the resulting BigInt to c_ulonglong and return vm.new_overflow_error with text naming the unsigned C type ("Python int too large to convert to C unsigned long long") on overflow; locate and follow the pattern in the signed extractor (PyLong_AsLong) and reuse its coercion/try_index logic and error handling.
🤖 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.
Duplicate comments:
In `@crates/capi/src/longobject.rs`:
- Around line 49-58: The unsigned extractor PyLong_AsUnsignedLongLong currently
bypasses the index protocol and uses PyInt downcast and a signed overflow
message; update it to mirror the signed extractor by using the index protocol
helper (e.g., call try_index or the same helper used by PyLong_AsLong) to coerce
obj to an integer, then convert the resulting BigInt to c_ulonglong and return
vm.new_overflow_error with text naming the unsigned C type ("Python int too
large to convert to C unsigned long long") on overflow; locate and follow the
pattern in the signed extractor (PyLong_AsLong) and reuse its coercion/try_index
logic and error handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: f50d506c-f461-4305-8a5a-c291b6a58f56
📒 Files selected for processing (2)
crates/capi/src/longobject.rscrates/capi/src/util.rs
youknowone
approved these changes
May 19, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes