Add more list functions to the c-api#8045
Conversation
📝 WalkthroughWalkthroughThis PR extends RustPython's CPython compatibility layer by adding four new list C-API functions: ChangesList C-API Extensions
Sequence Diagram(s)No sequence diagram generated. This PR introduces FFI function implementations within a single module without multi-component sequential flows that would benefit from visualization. Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 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: 1
🤖 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 160-164: PyList_Sort currently downcasts to PyList and uses
vm.call_method(list.as_object(), "sort", ()) which performs normal Python-level
dispatch and can run overridden user code; change it to invoke the builtin list
sort implementation directly (not via instance dispatch). Specifically, replace
vm.call_method(...) in PyList_Sort with a direct call to the internal list
sorting routine (the in-place sorter for PyList) or call the "sort" function
bound to the exact built-in list type rather than the instance (e.g., resolve
the attribute on vm.ctx.types.list_type and invoke that bound function with the
list as self, or call PyList's internal sort method such as the library's
sort_inplace routine), ensuring no instance-level __dict__/subclass override can
run. Use the symbols PyList_Sort, PyList, and avoid vm.call_method to implement
this change.
🪄 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: 5c374d9b-913d-4252-8864-62b444550774
📒 Files selected for processing (1)
crates/capi/src/listobject.rs
| pub unsafe extern "C" fn PyList_Sort(list: *mut PyObject) -> c_int { | ||
| with_vm(|vm| { | ||
| let list = unsafe { &*list }.try_downcast_ref::<PyList>(vm)?; | ||
| vm.call_method(list.as_object(), "sort", ())?; | ||
| Ok(()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the implementation and inspect surrounding code
FILE="crates/capi/src/listobject.rs"
nl -ba "$FILE" | sed -n '130,230p'
# Inspect how list sorting is implemented on the RustPython side
rg -n "fn\s+sort|class.*sort|PyList_.*Sort|\"sort\"" crates/capi/src/listobject.rs crates/vm crates/payload crates | head -n 200
# Find the PyList definition and its sort method implementation
rg -n "struct\s+PyList|impl\s+PyList|PyPayload.*PyList|pymethod.*sort|fn\s+sort\(" crates | head -n 200
# Check what try_downcast_ref::<PyList> does / whether it accepts subclasses
rg -n "try_downcast_ref<.*PyList|try_downcast_ref" crates/vm crates/payload crates -S | head -n 200Repository: RustPython/RustPython
Length of output: 108
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/capi/src/listobject.rs"
# Show the relevant section with line numbers (cat -n)
if [ -f "$FILE" ]; then
echo "==== $FILE (lines 130-220) ===="
sed -n '130,220p' "$FILE" | cat -n
else
echo "Missing file: $FILE"
exit 1
fi
echo "==== Search: PyList_Sort and call_method(\"sort\") ===="
rg -n "PyList_Sort|call_method\(.*\"sort\"|\"sort\"\s*,\s*\(\s*\)" crates/capi/src/listobject.rs crates/capi/src -S
echo "==== Search: list sort implementation (PyList::sort / method sort) ===="
rg -n "impl\s+PyList|fn\s+sort\(|pymethod.*sort|method.*sort|\"sort\"" crates -S | head -n 200
echo "==== Search: try_downcast_ref::<PyList> usage ===="
rg -n "try_downcast_ref::<\s*PyList\s*>" crates -SRepository: RustPython/RustPython
Length of output: 6883
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect try_downcast_ref implementation/semantics
rg -n "try_downcast_ref::<\s*PyList\s*>|try_downcast_ref<|fn\s+try_downcast_ref" crates -S | head -n 200
# Try to find the concrete trait/method definition (more targeted)
rg -n "try_downcast_ref\s*\(" crates/vm crates/common crates/core crates -S | head -n 200Repository: RustPython/RustPython
Length of output: 1249
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/vm/src/object/core.rs"
echo "=== try_downcast_ref in $FILE (around line ~1549) ==="
sed -n '1520,1605p' "$FILE" | cat -n
echo "=== Search for PyPayload / downcast logic around try_downcast_ref ==="
rg -n "trait\s+PyPayload|downcast|payload_type_id|TypeId|is_a|downcast_ref" crates/vm/src/object/core.rs crates/vm/src/object -S | head -n 200Repository: RustPython/RustPython
Length of output: 12850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect PyPayload::validate_downcastable_from default + semantics
FILE="crates/vm/src/object/payload.rs"
echo "=== $FILE (relevant section) ==="
sed -n '1,120p' "$FILE" | cat -n
# Inspect PyList payload/type implementation to see what validates downcastable
FILE="crates/vm/src/builtins/list.rs"
echo "=== $FILE: search PyPayload impl for PyList ==="
rg -n "PyPayload\s+for\s+PyList|impl\s+PyPayload\s+for\s+PyList|PAYLOAD_TYPE_ID|validate_downcastable_from" "$FILE" crates/vm/src/builtins -S | head -n 200
echo "=== $FILE: show around PyList impl blocks near sort ==="
rg -n "struct\s+PyList|impl\s+PyList|fn\s+sort\(" "$FILE" -SRepository: RustPython/RustPython
Length of output: 5750
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/vm/src/builtins/list.rs"
# Show PyPayload impl for PyList (near line 84)
sed -n '60,120p' "$FILE" | cat -n
# Show more of the impl to include validation/downcast details (if any)
sed -n '120,220p' "$FILE" | cat -n
# Show the PyList sort implementation around sort() method
sed -n '350,470p' "$FILE" | cat -nRepository: RustPython/RustPython
Length of output: 10897
🌐 Web query:
CPython PyList_Sort implementation listobject.c calls overridden sort subclass
💡 Result:
In CPython, the list.sort() method is implemented in Objects/listobject.c using a highly optimized C function, list_sort_impl (which corresponds to the list.sort operation). This implementation does not call overridden sort methods defined in subclasses of list [1][2][3]. The internal C implementation treats list subclasses as standard list objects for the purposes of sorting [1][3]. When list.sort() is called on an instance of a list subclass, the C code directly manipulates the underlying data structure of the object—effectively ignoring any custom sort logic defined in the Python layer of the subclass [1][2][3]. This is a documented behavior pattern in CPython where many C-implemented methods for built-in types (like list, dict, and heapq operations) operate on the concrete C structure and bypass Python-level overrides [4][1][2][3]. The implementation utilizes an adaptive, stable, natural mergesort algorithm (often referred to as Timsort), designed for efficiency by minimizing comparisons, which are expensive in CPython [5][6][7]. Because the implementation relies on direct memory access and low-level C functions to handle the sorting logic, it does not perform the standard Python method resolution order (MRO) lookups that would be required to detect and execute a subclass-overridden sort method [1][3]. Consequently, if a user overrides sort in a list subclass, calling .sort() on an instance of that class will still execute the native C implementation, not the custom Python code [1][2].
Citations:
- 1: https://bugs.python.org/issue10977
- 2: https://bugs.python.org/issue3935
- 3: https://stackoverflow.com/questions/13924393/heapq-heapify-does-not-work-with-a-subclassed-list
- 4: https://bugs.python.org/issue27561
- 5: https://github.com/python/cpython/blob/v3.9.2/Objects/listobject.c
- 6: https://github.com/python/cpython/blob/15f44ab043b37c064d6891c7864205fed9fb0dd1/Objects/listsort.txt
- 7: https://github.com/python/cpython/blob/main/Objects/listsort.txt
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find call_method implementation
rg -n "fn\s+call_method\s*\(" crates/vm -S | head -n 50
# Also search for the string-based call path
rg -n "call_method\(.*\"sort\"" crates/vm crates/capi -S | head -n 50
# Inspect call_method internals (where it does attribute lookup / descriptor resolution)
# Try to locate the specific method that `call_method` delegates to.
rg -n "call_method\(|call_method_with|" crates/vm -S | head -n 200
# Check for helpers used by call_method for attribute resolution by name
rg -n "get_attr|lookup_attr|slot|descriptor|MRO|resolve" crates/vm/src -S | head -n 200Repository: RustPython/RustPython
Length of output: 47
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Locate the definition of call_method
rg -n "call_method" crates/vm/src -S | head -n 200
# 2) Look specifically for the signature text "pub .*call_method" / "fn .*call_method"
rg -n "pub\s+.*call_method|fn\s+.*call_method" crates/vm/src -S | head -n 200
# 3) Find where call_method resolves attributes by name ("sort" in our case will be passed as a string)
rg -n "call_method.*lookup|lookup.*call_method|resolve.*call_method|get_attr|lookup_attr|descriptor|MRO" crates/vm/src -S | head -n 200Repository: RustPython/RustPython
Length of output: 38370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="crates/vm/src/vm/vm_object.rs"
sed -n '90,210p' "$FILE" | cat -n
# Also inspect get_attribute/get_attr helpers used by call_method
rg -n "get_attr_inner|get_attribute_opt|call_if_get_descriptor|call_get_descriptor|method_name" crates/vm/src/vm/vm_object.rs crates/vm/src/vm/mod.rs crates/vm/src/vm/method.rs -S | head -n 200Repository: RustPython/RustPython
Length of output: 6400
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find PyMethod::get implementation
rg -n "struct\s+PyMethod|impl\s+PyMethod|fn\s+get\s*\(" crates/vm/src/vm/method.rs crates/vm/src -S | head -n 200
# Show the relevant portion around PyMethod::get / attribute resolution
sed -n '1,220p' crates/vm/src/vm/method.rs | cat -nRepository: RustPython/RustPython
Length of output: 8851
Avoid Python-level method dispatch in PyList_Sort.
PyList_Sort downcasts to PyList and then calls vm.call_method(list.as_object(), "sort", ()), which resolves "sort" via PyMethod::get against the object’s runtime class and instance __dict__. That allows a list subclass override (or an instance attribute named sort) to run user code, unlike CPython’s PyList_Sort behavior. Resolve/invoke the sort implementation from the exact built-in list type (or call the internal list sort routine) instead of generic call_method.
🤖 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 160 - 164, PyList_Sort currently
downcasts to PyList and uses vm.call_method(list.as_object(), "sort", ()) which
performs normal Python-level dispatch and can run overridden user code; change
it to invoke the builtin list sort implementation directly (not via instance
dispatch). Specifically, replace vm.call_method(...) in PyList_Sort with a
direct call to the internal list sorting routine (the in-place sorter for
PyList) or call the "sort" function bound to the exact built-in list type rather
than the instance (e.g., resolve the attribute on vm.ctx.types.list_type and
invoke that bound function with the list as self, or call PyList's internal sort
method such as the library's sort_inplace routine), ensuring no instance-level
__dict__/subclass override can run. Use the symbols PyList_Sort, PyList, and
avoid vm.call_method to implement this change.
Summary by CodeRabbit
New Features
Tests