Skip to content

Move PyList Python methods and operators to Py<PyList> - #8848

Open
youknowone wants to merge 4 commits into
RustPython:mainfrom
youknowone:explicit-payload
Open

youknowone wants to merge 4 commits into
RustPython:mainfrom
youknowone:explicit-payload

Conversation

@youknowone

@youknowone youknowone commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

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 read borrow_vec().len().

  • The Python interface of list moves from the PyList payload to a #[pyclass] impl Py<PyList> block, registered through with(Py, ...):

    • every #[pymethod] (append, extend, insert, clear, copy, __sizeof__, reverse, __reversed__, __getitem__, count, index, pop, remove, sort) and #[pyclassmethod] __class_getitem__
    • the operators __add__ (which replaces concat()), __iadd__, __mul__, __imul__, __setitem__, __delitem__, __contains__ and inplace_concat

    The #[pyclass] impl PyList block keeps only with(...) and flags(...).

  • PyList keeps the helpers that work on the payload: _getitem, _setitem, _delitem, repeat, irepeat and sort, which sorted() and dir() also call on a PyList value before it becomes an object. Py<PyList> reaches them through the payload field. The mapping and sequence slots call the Py<PyList> operators.

  • GenericAlias.__dir__ works on the PyList value returned by vm.dir(), so it appends through borrow_vec_mut() and checks membership with mut_contains().

  • Deref is unchanged and behavior is unchanged.

Rename Traverse::clear to clear_refs

Traverse::clear becomes clear_refs, and MaybeTraverse::try_clear becomes try_clear_refs. Before this, list.clear() on a PyRef<PyList> resolved to the Traverse trait method instead of Py<PyList>::clear. The derive macros and every existing impl are renamed. try_clear_obj, HAS_CLEAR and #[pyclass(clear)] are unchanged.

Expose the PyList_* operations on Py

Only the operations that have a PyList_* counterpart are pub on Py<PyList>. Names follow pyo3's PyListMethods.

Py<PyList> C API
len(), is_empty() PyList_Size
get_item(index, vm) PyList_GetItemRef
set_item(index, item, vm) PyList_SetItem
get_slice(low, high, vm) PyList_GetSlice
set_slice(low, high, items, vm) PyList_SetSlice
del_slice(low, high) PyList_SetSlice with NULL
append, extend, insert, clear, reverse (now pub) PyList_Append, PyList_Extend, PyList_Insert, PyList_Clear, PyList_Reverse
sort(vm) PyList_Sort
to_tuple(vm) PyList_AsTuple
  • The sort pymethod is now 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 the list lock is released.
  • The capi PyList_Size, PyList_Append, PyList_Insert, PyList_Reverse and PyList_AsTuple call these methods.
  • A unit test covers the new methods.

Call the Py methods from the remaining capi PyList_* functions

PyList_GetItemRef, PyList_SetItem, PyList_GetSlice, PyList_SetSlice and PyList_Sort now call the Py<PyList> methods. Their behavior changes to match the C API:

  • 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.

New capi tests cover negative bounds, deleting a slice, and assigning a list into itself.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Corrected C API list indexing so negative indices are treated as out of range.
    • Improved list slicing behavior, including deletion and replacing a list’s contents with itself.
  • Improvements
    • Updated list operations to provide consistent behavior across Python-facing and C API usage, with expanded coverage for list mutations and sorting.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: RustPython/RustPython/.coderabbit.yml

Review profile: CHILL

Plan: Advanced

Run ID: 0471d9e0-20a5-45ec-a3e2-cb03be74a30a

📥 Commits

Reviewing files that changed from the base of the PR and between a90cbe6 and d4112a2.

📒 Files selected for processing (20)
  • crates/capi/src/listobject.rs
  • crates/derive-impl/src/pyclass.rs
  • crates/derive-impl/src/pystructseq.rs
  • crates/stdlib/src/elementtree.rs
  • crates/vm/src/builtins/dict.rs
  • crates/vm/src/builtins/function.rs
  • crates/vm/src/builtins/list.rs
  • crates/vm/src/builtins/slice.rs
  • crates/vm/src/builtins/str.rs
  • crates/vm/src/builtins/tuple.rs
  • crates/vm/src/builtins/type.rs
  • crates/vm/src/exceptions.rs
  • crates/vm/src/frame.rs
  • crates/vm/src/object/core.rs
  • crates/vm/src/object/traverse.rs
  • crates/vm/src/stdlib/_collections.rs
  • crates/vm/src/stdlib/_collections/ordered_dict.rs
  • crates/vm/src/stdlib/_functools.rs
  • crates/vm/src/stdlib/_testinternalcapi.rs
  • crates/vm/src/stdlib/_thread.rs

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

PyList operations now use payload-level methods, and C API list operations delegate to them. Related runtime paths use borrowed list lengths or mutable list operations. Traversal cleanup hooks use renamed methods across runtime types and generated implementations.

Changes

List data access

Layer / File(s) Summary
PyList operations and C API integration
crates/vm/src/builtins/list.rs, crates/capi/src/listobject.rs
List item access, mutation, slicing, sorting, and conversion use payload-level methods. Python protocol methods and C API operations delegate to these methods. Tests cover list operations and C API cases.
Borrowed lengths and mutable list consumers
crates/stdlib/src/_heapq.rs, crates/vm/src/builtins/object.rs, crates/vm/src/builtins/genericalias.rs
Heap algorithms and object slot processing read list lengths from borrowed vectors. Generic-alias directory construction checks and adds entries through mutable list operations.

Traversal cleanup hooks

Layer / File(s) Summary
Traversal hooks and generated implementations
crates/vm/src/object/traverse.rs, crates/derive-impl/src/pyclass.rs, crates/derive-impl/src/pystructseq.rs
Traversal traits and generated implementations use clear_refs and try_clear_refs in place of the prior hook names.
Runtime traversal implementations
crates/vm/src/builtins/*, crates/vm/src/stdlib/*, crates/stdlib/src/elementtree.rs, crates/vm/src/exceptions.rs, crates/vm/src/frame.rs, crates/vm/src/object/core.rs
Runtime traversal implementations use the renamed cleanup hooks. Existing reference-draining and cleanup behavior remains unchanged.

Priority: ⬇️ Low

Estimated code review effort: 3 (Moderate) | ~25 minutes

Change: Refactor

Suggested reviewers: shaharnaveh

Merge Risk: ⚪ Minimal · up to d4112

No identified issue currently blocks merging after normal checks.

Security Architecture Review

Security architecture risk: 🔵 Low · up to d4112

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
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The inspected attacker-influenced inputs are list objects, indices, slice bounds, and replacement iterables supplied through Python or native list APIs; their demonstrated effects are on the addressed interpreter list, not a newly identified privileged service or store.

Trust Boundaries and Controls

  • observed — At the C-to-VM boundary, the inspected wrappers cast the target to PyList before delegation. Negative item indices reach checked access and produce an index error; slice replacement gathers iterable contents before taking mutable list access.

Resilience and Maintainability Implications

  • observed — List garbage-collection cleanup still extracts child references, and generated clear dispatch calls the renamed hook rather than the Python-facing list clear method.
🚥 Pre-merge checks | ✅ 4 | ❓ 1

❌ Failed checks (1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ❓ Inconclusive 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… Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the primary change: moving PyList Python methods and operators to Py.
Full details: Docstring Coverage

Explanation

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)
  • Create a new PR

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@youknowone youknowone changed the title Move PyList object operations to Py<PyList> Move PyList Python methods and operators to Py<PyList> Sep 27, 2026
@youknowone
youknowone force-pushed the explicit-payload branch 2 times, most recently from bbcb1bd to a90cbe6 Compare September 27, 2026 05:05
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant