Skip to content

Commit 8ea5d22

Browse files
youknowoneyouknowone
committed
Reconcile explicit PyList APIs with current main
Keep list operations on Py<PyList> and route the C API through them. Reuse current checked slice assignment and finalizer ordering, retaining the Traverse method rename so list.clear() is not shadowed. Keep replaced items alive through the write guard scope without a redundant terminal drop. Assisted-by: Claude Code:claude-opus-5-5 Assisted-by: Codex:model-version-unavailable
1 parent f1f38c1 commit 8ea5d22

33 files changed

Lines changed: 308 additions & 171 deletions

‎crates/capi/src/listobject.rs‎

Lines changed: 54 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,7 @@ use crate::object::define_py_check;
33
use crate::pystate::with_vm;
44
use crate::util::FfiPtrExt;
55
use core::ffi::c_int;
6-
use rustpython_vm::AsObject;
7-
use rustpython_vm::PyObjectRef;
86
use rustpython_vm::builtins::PyList;
9-
use rustpython_vm::sliceable::{SaturatedSlice, SliceableSequenceMutOp, SliceableSequenceOp};
107

118
define_py_check!(fn PyList_Check, types.list_type);
129
define_py_check!(exact fn PyList_CheckExact, types.list_type);
@@ -25,19 +22,16 @@ pub extern "C" fn PyList_New(size: isize) -> *mut PyObject {
2522
pub unsafe extern "C" fn PyList_Size(obj: *mut PyObject) -> isize {
2623
with_vm(|vm| {
2724
let list = unsafe { obj.assume_borrowed_and_cast::<PyList>(vm) }?;
28-
Ok(list.__len__())
25+
Ok(list.len())
2926
})
3027
}
3128

3229
#[unsafe(no_mangle)]
3330
pub unsafe extern "C" fn PyList_GetItemRef(obj: *mut PyObject, index: isize) -> *mut PyObject {
3431
with_vm(|vm| {
3532
let list = unsafe { obj.assume_borrowed_and_cast::<PyList>(vm) }?;
36-
index
37-
.try_into()
38-
.ok()
39-
.and_then(|index: usize| list.borrow_vec().get(index).map(ToOwned::to_owned))
40-
.ok_or_else(|| vm.new_index_error(format!("list index out of range: {index}")))
33+
// A negative index wraps past the end and is out of range.
34+
list.get_item(index as usize, vm)
4135
})
4236
}
4337

@@ -50,25 +44,17 @@ pub unsafe extern "C" fn PyList_SetItem(
5044
with_vm(|vm| {
5145
let list = unsafe { list.assume_borrowed_and_cast::<PyList>(vm) }?;
5246
let item = unsafe { item.assume_owned() };
53-
let index_error =
54-
|| vm.new_index_error(format!("list assignment index out of range: {index}"));
55-
if index < 0 {
56-
return Err(index_error());
57-
}
58-
59-
let mut list_mut = list.borrow_vec_mut();
60-
match index - list_mut.len() as isize {
61-
..0 => {
62-
list_mut[index as usize] = item;
63-
Ok(())
64-
}
47+
// A negative index wraps past the end and is out of range.
48+
let index = index as usize;
49+
{
50+
let mut list_mut = list.borrow_vec_mut();
6551
// This is somewhat a hack, we assume that we are populating a list right after PyList_New
66-
0 if list_mut.capacity() > index as usize => {
52+
if index == list_mut.len() && list_mut.capacity() > index {
6753
list_mut.push(item);
68-
Ok(())
54+
return Ok(());
6955
}
70-
0.. => Err(index_error()),
7156
}
57+
list.set_item(index, item, vm)
7258
})
7359
}
7460

@@ -77,7 +63,7 @@ pub unsafe extern "C" fn PyList_Append(list: *mut PyObject, item: *mut PyObject)
7763
with_vm(|vm| {
7864
let list = unsafe { list.assume_borrowed_and_cast::<PyList>(vm) }?;
7965
let item = unsafe { item.assume_borrowed() }.to_owned();
80-
list.borrow_vec_mut().push(item);
66+
list.append(item);
8167
Ok(())
8268
})
8369
}
@@ -91,14 +77,7 @@ pub unsafe extern "C" fn PyList_Insert(
9177
with_vm(|vm| {
9278
let list = unsafe { list.assume_borrowed_and_cast::<PyList>(vm) }?;
9379
let item = unsafe { item.assume_borrowed() }.to_owned();
94-
let mut vec = list.borrow_vec_mut();
95-
let index = if index < 0 {
96-
index + vec.len() as isize
97-
} else {
98-
index
99-
}
100-
.clamp(0, vec.len() as isize) as usize;
101-
vec.insert(index, item);
80+
list.insert(index, item);
10281
Ok(())
10382
})
10483
}
@@ -107,7 +86,7 @@ pub unsafe extern "C" fn PyList_Insert(
10786
pub unsafe extern "C" fn PyList_Reverse(list: *mut PyObject) -> c_int {
10887
with_vm(|vm| {
10988
let list = unsafe { list.assume_borrowed_and_cast::<PyList>(vm) }?;
110-
list.borrow_vec_mut().reverse();
89+
list.reverse();
11190
Ok(())
11291
})
11392
}
@@ -116,7 +95,7 @@ pub unsafe extern "C" fn PyList_Reverse(list: *mut PyObject) -> c_int {
11695
pub unsafe extern "C" fn PyList_AsTuple(list: *mut PyObject) -> *mut PyObject {
11796
with_vm(|vm| {
11897
let list = unsafe { list.assume_borrowed_and_cast::<PyList>(vm) }?;
119-
Ok(vm.ctx.new_tuple(list.borrow_vec().to_vec()))
98+
Ok(list.to_tuple(vm))
12099
})
121100
}
122101

@@ -128,9 +107,7 @@ pub unsafe extern "C" fn PyList_GetSlice(
128107
) -> *mut PyObject {
129108
with_vm(|vm| {
130109
let list = unsafe { list.assume_borrowed_and_cast::<PyList>(vm) }?;
131-
let vec = list.borrow_vec();
132-
let sliced = vec.getitem_by_slice(vm, SaturatedSlice::from_parts(low, high, 1))?;
133-
Ok(vm.ctx.new_list(sliced))
110+
Ok(list.get_slice(low, high, vm))
134111
})
135112
}
136113

@@ -143,31 +120,26 @@ pub unsafe extern "C" fn PyList_SetSlice(
143120
) -> c_int {
144121
with_vm(|vm| {
145122
let list = unsafe { list.assume_borrowed_and_cast::<PyList>(vm) }?;
146-
let slice = SaturatedSlice::from_parts(low, high, 1);
147-
let mut vec = list.borrow_vec_mut();
148-
149123
let Some(itemlist) = (unsafe { itemlist.assume_borrowed_or_opt() }) else {
150-
vec.delitem_by_slice(vm, slice)?;
124+
list.del_slice(low, high, vm)?;
151125
return Ok(());
152126
};
153-
154-
let items: Vec<PyObjectRef> = itemlist.try_to_value(vm)?;
155-
vec.setitem_by_slice(vm, slice, &items)
127+
list.set_slice(low, high, itemlist, vm)
156128
})
157129
}
158130

159131
#[unsafe(no_mangle)]
160132
pub unsafe extern "C" fn PyList_Sort(list: *mut PyObject) -> c_int {
161133
with_vm(|vm| {
162134
let list = unsafe { list.assume_borrowed_and_cast::<PyList>(vm) }?;
163-
vm.call_method(list.as_object(), "sort", ())?;
164-
Ok(())
135+
list.sort_default(vm)
165136
})
166137
}
167138

168139
#[cfg(test)]
169140
mod tests {
170141
use pyo3::exceptions::PyIndexError;
142+
use pyo3::ffi;
171143
use pyo3::prelude::*;
172144
use pyo3::types::{PyList, PyListMethods};
173145

@@ -288,4 +260,39 @@ mod tests {
288260
assert_eq!(list.get_item(2).unwrap().extract::<u32>().unwrap(), 3);
289261
})
290262
}
263+
264+
#[test]
265+
fn list_del_slice() {
266+
Python::attach(|py| {
267+
let list = PyList::new(py, [1, 2, 3, 4]).unwrap();
268+
list.del_slice(1, 3).unwrap();
269+
assert_eq!(list.extract::<Vec<u32>>().unwrap(), [1, 4]);
270+
})
271+
}
272+
273+
#[test]
274+
fn list_negative_indices() {
275+
Python::attach(|py| unsafe {
276+
let list = PyList::new(py, [1, 2, 3, 4]).unwrap();
277+
278+
assert!(ffi::PyList_GetItemRef(list.as_ptr(), -1).is_null());
279+
assert!(PyErr::take(py).unwrap().is_instance_of::<PyIndexError>(py));
280+
281+
let slice = Bound::from_owned_ptr(py, ffi::PyList_GetSlice(list.as_ptr(), -1, 2));
282+
assert_eq!(slice.extract::<Vec<u32>>().unwrap(), [1, 2]);
283+
284+
let repl = PyList::new(py, [9]).unwrap();
285+
assert_eq!(ffi::PyList_SetSlice(list.as_ptr(), -1, 1, repl.as_ptr()), 0);
286+
assert_eq!(list.extract::<Vec<u32>>().unwrap(), [9, 2, 3, 4]);
287+
})
288+
}
289+
290+
#[test]
291+
fn list_set_slice_self() {
292+
Python::attach(|py| {
293+
let list = PyList::new(py, [1, 2]).unwrap();
294+
list.set_slice(2, 2, list.as_any()).unwrap();
295+
assert_eq!(list.extract::<Vec<u32>>().unwrap(), [1, 2, 1, 2]);
296+
})
297+
}
291298
}

‎crates/derive-impl/src/pyclass.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -845,7 +845,7 @@ pub(crate) fn impl_pyclass(attr: PunctuatedNestedMeta, item: Item) -> Result<Tok
845845
//
846846
// For clear (tp_clear):
847847
// 1. no `clear`: HAS_CLEAR = HAS_TRAVERSE (default: same as traverse)
848-
// 2. `clear` or `clear = true`: HAS_CLEAR = true, try_clear calls Traverse::clear
848+
// 2. `clear` or `clear = true`: HAS_CLEAR = true, try_clear_refs calls Traverse::clear_refs
849849
// 3. `clear = false`: HAS_CLEAR = false (rare: traverse without clear)
850850
let has_traverse = class_meta.inner().contains_key("traverse");
851851
let has_clear = if class_meta.inner().contains_key("clear") {
@@ -889,7 +889,7 @@ pub(crate) fn impl_pyclass(attr: PunctuatedNestedMeta, item: Item) -> Result<Tok
889889

890890
let try_clear_body = if has_clear {
891891
quote! {
892-
::rustpython_vm::object::Traverse::clear(self, out);
892+
::rustpython_vm::object::Traverse::clear_refs(self, out);
893893
}
894894
} else {
895895
quote! {
@@ -906,7 +906,7 @@ pub(crate) fn impl_pyclass(attr: PunctuatedNestedMeta, item: Item) -> Result<Tok
906906
#try_traverse_body
907907
}
908908

909-
fn try_clear(&mut self, out: &mut ::std::vec::Vec<::rustpython_vm::PyObjectRef>) {
909+
fn try_clear_refs(&mut self, out: &mut ::std::vec::Vec<::rustpython_vm::PyObjectRef>) {
910910
#try_clear_body
911911
}
912912
}

‎crates/derive-impl/src/pystructseq.rs‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -592,8 +592,8 @@ pub(crate) fn impl_pystruct_sequence(
592592
self.0.try_traverse(traverse_fn)
593593
}
594594

595-
fn try_clear(&mut self, out: &mut ::std::vec::Vec<::rustpython_vm::PyObjectRef>) {
596-
self.0.try_clear(out)
595+
fn try_clear_refs(&mut self, out: &mut ::std::vec::Vec<::rustpython_vm::PyObjectRef>) {
596+
self.0.try_clear_refs(out)
597597
}
598598
}
599599

‎crates/stdlib/src/elementtree.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -202,7 +202,7 @@ pub(crate) mod _elementtree {
202202
inner.children.traverse(traverse_fn);
203203
}
204204

205-
fn clear(&mut self, out: &mut Vec<PyObjectRef>) {
205+
fn clear_refs(&mut self, out: &mut Vec<PyObjectRef>) {
206206
// Only reached for an object the collector has already proven
207207
// unreachable, so grabbing the lock cannot race with user code.
208208
let Some(mut inner) = self.inner.try_write() else {
@@ -1405,7 +1405,7 @@ pub(crate) mod _elementtree {
14051405
st.pi_event.traverse(traverse_fn);
14061406
}
14071407

1408-
fn clear(&mut self, out: &mut Vec<PyObjectRef>) {
1408+
fn clear_refs(&mut self, out: &mut Vec<PyObjectRef>) {
14091409
let Some(mut st) = self.state.try_write() else {
14101410
return;
14111411
};
@@ -1960,7 +1960,7 @@ pub(crate) mod _elementtree {
19601960
st.handle_doctype.traverse(traverse_fn);
19611961
}
19621962

1963-
fn clear(&mut self, out: &mut Vec<PyObjectRef>) {
1963+
fn clear_refs(&mut self, out: &mut Vec<PyObjectRef>) {
19641964
let Some(mut st) = self.state.try_write() else {
19651965
return;
19661966
};

‎crates/stdlib/src/pickle.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -556,7 +556,7 @@ mod _pickle {
556556
}
557557
}
558558

559-
fn clear(&mut self, out: &mut Vec<PyObjectRef>) {
559+
fn clear_refs(&mut self, out: &mut Vec<PyObjectRef>) {
560560
let state = self.read_state.get_mut();
561561
out.extend(state.read.take());
562562
out.extend(state.readinto.take());

‎crates/vm/src/builtins/dict.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -333,7 +333,7 @@ unsafe impl Traverse for PyDict {
333333
self.entries.traverse(traverse_fn);
334334
}
335335

336-
fn clear(&mut self, out: &mut Vec<PyObjectRef>) {
336+
fn clear_refs(&mut self, out: &mut Vec<PyObjectRef>) {
337337
// Pop all entries and collect both keys and values
338338
for (key, value) in self.entries.drain_entries() {
339339
out.push(key);
@@ -1475,8 +1475,8 @@ unsafe impl Traverse for PyFrozenDict {
14751475
fn traverse(&self, tracer: &mut TraverseFn<'_>) {
14761476
self.dict.traverse(tracer);
14771477
}
1478-
fn clear(&mut self, out: &mut Vec<PyObjectRef>) {
1479-
Traverse::clear(&mut self.dict, out);
1478+
fn clear_refs(&mut self, out: &mut Vec<PyObjectRef>) {
1479+
Traverse::clear_refs(&mut self.dict, out);
14801480
}
14811481
}
14821482

‎crates/vm/src/builtins/function.rs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ unsafe impl Traverse for PyFunction {
118118
self.qualname.lock().traverse(tracer_fn);
119119
}
120120

121-
fn clear(&mut self, out: &mut Vec<crate::PyObjectRef>) {
121+
fn clear_refs(&mut self, out: &mut Vec<crate::PyObjectRef>) {
122122
// Pop closure if present (equivalent to Py_CLEAR(func_closure))
123123
if let Some(closure) = self.closure.take() {
124124
out.push(closure.into());

0 commit comments

Comments
 (0)