Move PyList Python methods and operators to Py<PyList> - #8848
youknowone wants to merge 4 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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: Repository: RustPython/RustPython/.coderabbit.yml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (20)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthrough
ChangesList data access
Traversal cleanup hooks
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Refactor Suggested reviewers: Merge Risk: ⚪ Minimal · up to No identified issue currently blocks merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The list and cleanup interfaces have a broad compatibility surface, but the inspected paths preserve list type checks, bounded access, and reference cleanup. No introduced security bypass was established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❓ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 114 functions across 22 files. (1 skipped: 1 too large.) ✨ 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 |
e82b749 to
e758e5b
Compare
bbcb1bd to
a90cbe6
Compare
The #[pymethod]s, #[pyclassmethod] __class_getitem__ and the operators (__add__, which replaces concat(), __iadd__, __mul__, __imul__, __getitem__, __setitem__, __delitem__, __contains__, inplace_concat) are defined in a #[pyclass] impl of Py<PyList>, registered through with(Py, ...). The #[pyclass] impl of PyList keeps the class attributes and is empty. PyList keeps the helpers that work on the payload (_getitem, _setitem, _delitem, repeat, irepeat and sort, which also sorts PyList values that have no object yet); Py<PyList> reaches them through the payload field. The mapping and sequence slots call the Py<PyList> operators. PyList::__len__() is removed; its callers read borrow_vec().len(). GenericAlias.__dir__ appends to the PyList returned by vm.dir() through borrow_vec_mut() and checks membership with mut_contains(). Assisted-by: Claude Code:claude-opus-5-5
Traverse::clear() and MaybeTraverse::try_clear() are renamed to clear_refs() and try_clear_refs(). The old name shadowed the clear() method of containers such as Py<PyList> wherever Traverse was in scope, because method lookup reached the trait impl for PyRef<T> before dereferencing to Py<T>. The #[pyclass(clear)] attribute and HAS_CLEAR are unchanged. Assisted-by: Claude Code:claude-opus-5-5
Py<PyList> gains pub len(), is_empty(), get_item(), set_item(), get_slice(), set_slice(), del_slice(), sort() and to_tuple(), matching PyList_Size, PyList_GetItemRef, PyList_SetItem, PyList_GetSlice, PyList_SetSlice, PyList_Sort and PyList_AsTuple. append(), extend(), insert(), clear() and reverse() (PyList_Append, PyList_Extend, PyList_Insert, PyList_Clear, PyList_Reverse) become pub. The sort pymethod is implemented by sort_with_options(). get_slice(), set_slice() and del_slice() clamp both bounds to the list. set_item(), set_slice() and del_slice() drop the replaced elements after releasing the list lock. The capi PyList_Size, PyList_Append, PyList_Insert, PyList_Reverse and PyList_AsTuple call these methods. Assisted-by: Claude Code:claude-opus-5-5
PyList_GetItemRef, PyList_SetItem, PyList_GetSlice, PyList_SetSlice and PyList_Sort call get_item(), set_item(), get_slice(), set_slice(), del_slice() and sort() on Py<PyList>. - PyList_GetSlice and PyList_SetSlice clamp negative bounds to 0 instead of counting them from the end of the list. - PyList_SetSlice collects the new items before taking the list lock, so a list can be assigned into itself. - PyList_GetItemRef and PyList_SetItem raise IndexError without the index in the message. - PyList_Sort sorts the list directly instead of calling its sort method. - PyList_SetItem still appends when the index equals the length and the list has spare capacity, as after PyList_New. Assisted-by: Claude Code:claude-opus-5-5
a90cbe6 to
d4112a2
Compare
Follow-up to #8819, same rules applied to
list. Four commits.Move PyList Python methods and operators to Py
Py<PyList>::borrow_vec()/borrow_vec_mut()are the views.PyList::__len__()is removed; its callers readborrow_vec().len().The Python interface of
listmoves from thePyListpayload to a#[pyclass] impl Py<PyList>block, registered throughwith(Py, ...):#[pymethod](append,extend,insert,clear,copy,__sizeof__,reverse,__reversed__,__getitem__,count,index,pop,remove,sort) and#[pyclassmethod] __class_getitem____add__(which replacesconcat()),__iadd__,__mul__,__imul__,__setitem__,__delitem__,__contains__andinplace_concatThe
#[pyclass] impl PyListblock keeps onlywith(...)andflags(...).PyListkeeps the helpers that work on the payload:_getitem,_setitem,_delitem,repeat,irepeatandsort, whichsorted()anddir()also call on aPyListvalue before it becomes an object.Py<PyList>reaches them through thepayloadfield. The mapping and sequence slots call thePy<PyList>operators.GenericAlias.__dir__works on thePyListvalue returned byvm.dir(), so it appends throughborrow_vec_mut()and checks membership withmut_contains().Derefis unchanged and behavior is unchanged.Rename Traverse::clear to clear_refs
Traverse::clearbecomesclear_refs, andMaybeTraverse::try_clearbecomestry_clear_refs. Before this,list.clear()on aPyRef<PyList>resolved to theTraversetrait method instead ofPy<PyList>::clear. The derive macros and every existing impl are renamed.try_clear_obj,HAS_CLEARand#[pyclass(clear)]are unchanged.Expose the PyList_* operations on Py
Only the operations that have a
PyList_*counterpart are pub onPy<PyList>. Names follow pyo3'sPyListMethods.Py<PyList>len(),is_empty()PyList_Sizeget_item(index, vm)PyList_GetItemRefset_item(index, item, vm)PyList_SetItemget_slice(low, high, vm)PyList_GetSliceset_slice(low, high, items, vm)PyList_SetSlicedel_slice(low, high)PyList_SetSlicewithNULLappend,extend,insert,clear,reverse(now pub)PyList_Append,PyList_Extend,PyList_Insert,PyList_Clear,PyList_Reversesort(vm)PyList_Sortto_tuple(vm)PyList_AsTuplesortpymethod is now implemented bysort_with_options().get_slice,set_sliceanddel_sliceclamp both bounds to the list.set_item,set_sliceanddel_slicedrop the replaced elements after the list lock is released.PyList_Size,PyList_Append,PyList_Insert,PyList_ReverseandPyList_AsTuplecall these methods.Call the Py methods from the remaining capi PyList_* functions
PyList_GetItemRef,PyList_SetItem,PyList_GetSlice,PyList_SetSliceandPyList_Sortnow call thePy<PyList>methods. Their behavior changes to match the C API:PyList_GetSliceandPyList_SetSliceclamp negative bounds to 0 instead of counting them from the end of the list.PyList_SetSlicecollects the new items before taking the list lock, so a list can be assigned into itself.PyList_GetItemRefandPyList_SetItemraiseIndexErrorwithout the index in the message.PyList_Sortsorts the list directly instead of calling itssortmethod.PyList_SetItemstill appends when the index equals the length and the list has spare capacity, as afterPyList_New.New capi tests cover negative bounds, deleting a slice, and assigning a list into itself.
🤖 Generated with Claude Code
Summary by CodeRabbit