Add list support to c-api#7944
Conversation
📝 WalkthroughWalkthroughAdds a public ChangesC-API List Object Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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
Summary by CodeRabbit