Fix (ish) hanging tests in test_list.py#7382
Fix (ish) hanging tests in test_list.py#7382ShaharNaveh wants to merge 3 commits intoRustPython:mainfrom
test_list.py#7382Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplaced element-wise rich comparison for PyList with an index-based, mutation-aware comparison that re-reads list lengths and elements during iteration and handles Eq/Ne length shortcuts and differing-element comparisons with Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] test: cpython/Lib/test/test_list.py (TODO: 3) dependencies: dependent tests: (no tests depend on list) Legend:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/vm/src/builtins/list.rs`:
- Line 579: The code uses vm.bool_eq in the identity fast-path and later ignores
the requested comparator `op`; change the first-phase check to use
vm.identical_or_equal(zitem, oitem) instead of vm.bool_eq so
identical-but-unequal objects match the fast path, and in the second phase
replace the equality-only check with the proper comparator call (e.g.,
vm.bool_op(op, zitem, oitem) or the existing VM method that evaluates `op`) so
the ordered comparisons (using `op`) are honored; update the branches that
reference zitem, oitem and op accordingly.
- Around line 560-574: The comparison loop using zelements and oelements only
refreshes snapshots when lengths (zlen/olen) change, which allows in-place
mutations of elements to leave stale values; update the loop in the
equality/comparison routine (the block that uses zelf.borrow_vec(),
other.borrow_vec(), zelements, oelements, zlen, olen and the index i) so that
each iteration reads the current element(s) directly from the live lists (e.g.,
call zelf.borrow_vec() and other.borrow_vec() and index into them for the items
being compared) or otherwise refresh the snapshots for the specific indices
per-iteration instead of gating refreshes on length changes, ensuring
comparisons always use the current elements.
- Around line 565-577: The loop condition uses "while i < zlen || i < olen"
which allows indexing both snapshots when only one has an element; change it to
"while i < zlen && i < olen" so the prefix scan only runs while both zelements
and oelements have an index i; adjust no other logic — keep the existing refetch
of zelements/oelements and the subsequent size-based fallback unchanged
(identify zelements, oelements, zlen, olen, and i in the loop to locate the
change).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 6450e6c7-fcd0-4701-8cf8-bf43b2a67dc5
⛔ Files ignored due to path filters (1)
Lib/test/test_list.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/builtins/list.rs
| let mut zelements = zelf.borrow_vec().to_vec(); | ||
| let mut oelements = other.borrow_vec().to_vec(); | ||
|
|
||
| // Search for the first index where items are different. | ||
| let mut i = 0; | ||
| while i < zlen || i < olen { | ||
| if zelements.len() != zlen { | ||
| // Comparison mutated the list; refetch it. | ||
| zelements = zelf.borrow_vec().to_vec(); | ||
| } | ||
|
|
||
| if oelements.len() != olen { | ||
| // Comparison mutated the list; refetch it. | ||
| oelements = other.borrow_vec().to_vec(); | ||
| } |
There was a problem hiding this comment.
Length-preserving mutations still compare stale items.
The refresh logic only re-clones when len() changes. If an element comparison mutates list[i + 1] in place, the next iteration still reads the old zelements / oelements snapshots, so equality and ordering can be decided on objects that are no longer in either list. If this path is meant to be mutation-aware, reload the current items each iteration instead of gating refreshes on length changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/builtins/list.rs` around lines 560 - 574, The comparison loop
using zelements and oelements only refreshes snapshots when lengths (zlen/olen)
change, which allows in-place mutations of elements to leave stale values;
update the loop in the equality/comparison routine (the block that uses
zelf.borrow_vec(), other.borrow_vec(), zelements, oelements, zlen, olen and the
index i) so that each iteration reads the current element(s) directly from the
live lists (e.g., call zelf.borrow_vec() and other.borrow_vec() and index into
them for the items being compared) or otherwise refresh the snapshots for the
specific indices per-iteration instead of gating refreshes on length changes,
ensuring comparisons always use the current elements.
crates/vm/src/builtins/list.rs
Outdated
| while i < zlen || i < olen { | ||
| if zelements.len() != zlen { | ||
| // Comparison mutated the list; refetch it. | ||
| zelements = zelf.borrow_vec().to_vec(); | ||
| } | ||
|
|
||
| if oelements.len() != olen { | ||
| // Comparison mutated the list; refetch it. | ||
| oelements = other.borrow_vec().to_vec(); | ||
| } | ||
|
|
||
| let zitem = &zelements[i]; | ||
| let oitem = &oelements[i]; |
There was a problem hiding this comment.
Use && for the prefix scan bounds.
Line 565 currently enters the loop when only one list still has an element, but Lines 576-577 index both snapshots unconditionally. A prefix case like [1] < [1, 2] reaches i == 1 and panics before the size-based fallback at Line 592.
🛠️ Minimal fix
- while i < zlen || i < olen {
+ while i < zlen && i < olen {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while i < zlen || i < olen { | |
| if zelements.len() != zlen { | |
| // Comparison mutated the list; refetch it. | |
| zelements = zelf.borrow_vec().to_vec(); | |
| } | |
| if oelements.len() != olen { | |
| // Comparison mutated the list; refetch it. | |
| oelements = other.borrow_vec().to_vec(); | |
| } | |
| let zitem = &zelements[i]; | |
| let oitem = &oelements[i]; | |
| while i < zlen && i < olen { | |
| if zelements.len() != zlen { | |
| // Comparison mutated the list; refetch it. | |
| zelements = zelf.borrow_vec().to_vec(); | |
| } | |
| if oelements.len() != olen { | |
| // Comparison mutated the list; refetch it. | |
| oelements = other.borrow_vec().to_vec(); | |
| } | |
| let zitem = &zelements[i]; | |
| let oitem = &oelements[i]; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/builtins/list.rs` around lines 565 - 577, The loop condition
uses "while i < zlen || i < olen" which allows indexing both snapshots when only
one has an element; change it to "while i < zlen && i < olen" so the prefix scan
only runs while both zelements and oelements have an index i; adjust no other
logic — keep the existing refetch of zelements/oelements and the subsequent
size-based fallback unchanged (identify zelements, oelements, zlen, olen, and i
in the loop to locate the change).
| let zitem = &zelements[i]; | ||
| let oitem = &oelements[i]; | ||
|
|
||
| if !vm.bool_eq(zitem, oitem)? { |
There was a problem hiding this comment.
Use the correct comparator in both phases.
Line 579 should keep the identity fast path via vm.identical_or_equal, otherwise identical self-inequal objects stop matching. And Lines 613-619 ignore op entirely, so ordered comparisons like [1] < [2] devolve to equality and return the wrong result.
🛠️ Minimal fix
- if !vm.bool_eq(zitem, oitem)? {
+ if !vm.identical_or_equal(zitem, oitem)? {
break;
}
@@
- let res = vm.identical_or_equal(zitem, oitem)?;
+ let res = zitem.rich_compare_bool(oitem, op, vm)?;
Ok(PyComparisonValue::Implemented(res))Also applies to: 613-619
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/vm/src/builtins/list.rs` at line 579, The code uses vm.bool_eq in the
identity fast-path and later ignores the requested comparator `op`; change the
first-phase check to use vm.identical_or_equal(zitem, oitem) instead of
vm.bool_eq so identical-but-unequal objects match the fast path, and in the
second phase replace the equality-only check with the proper comparator call
(e.g., vm.bool_op(op, zitem, oitem) or the existing VM method that evaluates
`op`) so the ordered comparisons (using `op`) are honored; update the branches
that reference zitem, oitem and op accordingly.
1 participant
Summary by CodeRabbit