Add list support to c-api#7944
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:
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)
📝 WalkthroughWalkthroughAdds a public ChangesC-API List Object Support
Sequence DiagramsequenceDiagram
participant CCaller as C Caller
participant PyList_CAPI as PyList C-API
participant VM as VM
participant PyList as PyList
CCaller->>PyList_CAPI: call PyList_New(size) / PyList_* APIs
PyList_CAPI->>VM: with_vm (enter VM context)
VM->>PyList: allocate or downcast to PyList and operate on Vec
PyList->>VM: return results/errors
VM->>PyList_CAPI: propagate return value / error
PyList_CAPI->>CCaller: return pointer or status
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/listobject.rs`:
- Around line 13-15: PyList_New incorrectly uses Vec::with_capacity which leaves
length 0 and permits PyList_SetItem to append; change PyList_New to create a
vector with actual length size (e.g., resize with vm.ctx.none() or equivalent
placeholder) so the new list's logical length equals size, and validate size is
non-negative at the FFI boundary (return null/set error on negative). Update
PyList_SetItem to only replace elements when index < list.len() (do not grow
using capacity checks or append), and ensure it signals an IndexError on
out-of-range indices; reference the functions PyList_New and PyList_SetItem and
the list_mut.len()/resize behavior when making these changes.
- Around line 48-50: In PyList_SetItem, avoid casting a negative index to usize;
before the existing match on index - list_mut.len() as isize (and before using
list_mut[index as usize] = item), explicitly check if index < 0, set a Python
IndexError and return -1; keep the existing match logic for non-negative indices
and use list_mut and item as before so negative indexes no longer panic the
process.
🪄 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: 88eba30c-c001-4026-8c59-75a1d14d083e
📒 Files selected for processing (2)
crates/capi/src/lib.rscrates/capi/src/listobject.rs
1c76eba to
0f3361b
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/capi/src/listobject.rs (1)
13-15:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winGuard negative sizes before casting to
usize.A negative
sizebecomes a huge capacity request here. On anextern "C"boundary, that can turn a bad C input into a panic/abort path instead of a normal Python error/NULLreturn.🤖 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/listobject.rs` around lines 13 - 15, The function PyList_New currently casts a signed size to usize causing huge capacity requests for negative inputs; modify PyList_New to detect size < 0 and, inside the with_vm closure, set a Python error on the VM (e.g. a ValueError/OverflowError) and return a NULL pointer instead of calling vm.ctx.new_list; keep using with_vm and vm.ctx.new_list for the normal non-negative path.
🤖 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/listobject.rs`:
- Around line 13-15: The function PyList_New currently casts a signed size to
usize causing huge capacity requests for negative inputs; modify PyList_New to
detect size < 0 and, inside the with_vm closure, set a Python error on the VM
(e.g. a ValueError/OverflowError) and return a NULL pointer instead of calling
vm.ctx.new_list; keep using with_vm and vm.ctx.new_list for the normal
non-negative path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bf0b5a3c-19af-48d2-9f28-e7960f14f138
📒 Files selected for processing (1)
crates/capi/src/listobject.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/capi/src/listobject.rs (1)
83-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
PyList_Insertto normalizeindexlike CPython (never raiseIndexErrorjust becausewhereis out of range).
crates/capi/src/listobject.rscurrently checksif index as usize > vec.len()and then doesvec.insert(index as _, item), which causes:
- Any negative
indexto be cast to a hugeusizeand returnIndexError(CPython instead adjustswhereby addingnand clamps to0)- Any
index > lento returnIndexError(CPython instead clampswheretolenand inserts/appends)Update the implementation to match CPython’s
ins1normalization (where += nwhenwhere < 0, clamp to0; clampwhere > nton). Also update the disabledtest_list_insert: it currently assertslist.insert(2, 3).is_err()on a length-1 list, but CPython would append successfully.♻️ Suggested fix
pub unsafe extern "C" fn PyList_Insert( list: *mut PyObject, index: isize, item: *mut PyObject, ) -> c_int { with_vm(|vm| { let list = unsafe { &*list }.try_downcast_ref::<PyList>(vm)?; let item = unsafe { &*item }.to_owned(); let mut vec = list.borrow_vec_mut(); - if index as usize > vec.len() { - Err(vm.new_index_error(format!("list index out of range: {index}"))) - } else { - vec.insert(index as _, item); - Ok(()) - } + let len = vec.len() as isize; + let where_ = if index < 0 { + (index + len).max(0) as usize + } else { + (index as usize).min(vec.len()) + }; + vec.insert(where_, item); + Ok(()) }) }🤖 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/listobject.rs` around lines 83 - 100, PyList_Insert currently casts the signed index to usize and rejects out-of-range values; change it to follow CPython's ins1 normalization: read n = vec.len() as isize, if index < 0 then index += n, if index < 0 set index = 0, if index > n set index = n, then convert the final index to usize and call vec.insert(index as _, item) (do not perform the initial index as usize check). Update the associated test_list_insert expectation to accept inserting at index > len as appending (i.e., not an Err).
🤖 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.
Outside diff comments:
In `@crates/capi/src/listobject.rs`:
- Around line 83-100: PyList_Insert currently casts the signed index to usize
and rejects out-of-range values; change it to follow CPython's ins1
normalization: read n = vec.len() as isize, if index < 0 then index += n, if
index < 0 set index = 0, if index > n set index = n, then convert the final
index to usize and call vec.insert(index as _, item) (do not perform the initial
index as usize check). Update the associated test_list_insert expectation to
accept inserting at index > len as appending (i.e., not an Err).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bee7970f-ea6d-42f6-9fdd-7d5b6ec75a40
📒 Files selected for processing (1)
crates/capi/src/listobject.rs
fe2571d to
5032743
Compare
Summary by CodeRabbit