Skip to content

Fix (ish) hanging tests in test_list.py #7382

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
ShaharNaveh wants to merge 3 commits into
base: main
Choose a base branch
from
+76 −8
Draft

Fix (ish) hanging tests in test_list.py #7382

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
287e053
Fix (ish) hanging tests in `test_list.py`
ShaharNaveh Mar 7, 2026
1a79e32
cspell
ShaharNaveh Mar 7, 2026
9ddfc67
Fix cond
ShaharNaveh Mar 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Fix (ish) hanging tests in test_list.py
  • Loading branch information
@ShaharNaveh
ShaharNaveh committed Mar 7, 2026
commit 287e0533b11ede095547bd7d613bc962b48c89af
3 changes: 1 addition & 2 deletions Lib/test/test_list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -228,7 +228,6 @@ class L(list): pass
with self.assertRaises(TypeError):
(3,) + L([1,2])

@unittest.skip("TODO: RUSTPYTHON; hang")
def test_equal_operator_modifying_operand(self):
# test fix for seg fault reported in bpo-38588 part 2.
class X:
Expand All @@ -254,7 +253,7 @@ def __eq__(self, other):
list4 = [1]
self.assertFalse(list3 == list4)

@unittest.skip("TODO: RUSTPYTHON; hang")
@unittest.expectedFailure # TODO: RUSTPYTHON; AssertionError: TypeError not raised
def test_lt_operator_modifying_operand(self):
# See gh-120298
class evil:
Expand Down
77 changes: 71 additions & 6 deletions crates/vm/src/builtins/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
class::PyClassImpl,
convert::ToPyObject,
function::{ArgSize, FuncArgs, OptionalArg, PyComparisonValue},
iter::PyExactSizeIterator,
protocol::{PyIterReturn, PyMappingMethods, PySequenceMethods},
recursion::ReprGuard,
sequence::{MutObjectSequenceOp, OptionalRangeArgs, SequenceExt, SequenceMutExt},
Expand Down Expand Up @@ -547,11 +546,77 @@
return Ok(res.into());
}
let other = class_or_notimplemented!(Self, other);
let a = &*zelf.borrow_vec();
let b = &*other.borrow_vec();
a.iter()
.richcompare(b.iter(), op, vm)
.map(PyComparisonValue::Implemented)

let mut zlen = zelf.__len__();

Check warning on line 550 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zlen)
let mut olen = other.__len__();
if matches!(op, PyComparisonOp::Eq | PyComparisonOp::Ne) && (zlen != olen) {

Check warning on line 552 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zlen)
// Shortcut: if the lengths differ, the lists differ
return Ok(PyComparisonValue::Implemented(matches!(
op,
PyComparisonOp::Ne
)));
}

let mut zelements = zelf.borrow_vec().to_vec();

Check warning on line 560 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zelements)
let mut oelements = other.borrow_vec().to_vec();

Check warning on line 561 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (oelements)

// Search for the first index where items are different.
let mut i = 0;
while i < zlen || i < olen {

Check warning on line 565 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zlen)
if zelements.len() != zlen {

Check warning on line 566 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zlen)

Check warning on line 566 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zelements)
// Comparison mutated the list; refetch it.
zelements = zelf.borrow_vec().to_vec();

Check warning on line 568 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zelements)
}

if oelements.len() != olen {

Check warning on line 571 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (oelements)
// Comparison mutated the list; refetch it.
oelements = other.borrow_vec().to_vec();

Check warning on line 573 in crates/vm/src/builtins/list.rs

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (oelements)
}
Comment on lines +560 to +574
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


let zitem = &zelements[i];
let oitem = &oelements[i];
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

Suggested change
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).


if !vm.bool_eq(zitem, oitem)? {
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 7, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

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.

break;
}

// Refetch list sizes as calling the comparison may mutate the list.
zlen = zelf.__len__();
olen = other.__len__();

i += 1;
}

zlen = zelf.__len__();
olen = other.__len__();
if i >= zlen || i >= olen {
// No more items to compare -- compare sizes
let res = match op {
PyComparisonOp::Eq => zlen == olen,
PyComparisonOp::Ne => zlen != olen,
PyComparisonOp::Lt => zlen < olen,
PyComparisonOp::Le => zlen <= olen,
PyComparisonOp::Gt => zlen > olen,
PyComparisonOp::Ge => zlen >= olen,
};

return Ok(PyComparisonValue::Implemented(res));
};

// We have an item that differs -- shortcuts for EQ/NE.
match op {
PyComparisonOp::Eq => return Ok(PyComparisonValue::Implemented(false)),
PyComparisonOp::Ne => return Ok(PyComparisonValue::Implemented(true)),
_ => {}
}

// Compare the final item again using the proper operator.
zelements = zelf.borrow_vec().to_vec();
oelements = other.borrow_vec().to_vec();
let zitem = &zelements[i];
let oitem = &oelements[i];
let res = vm.identical_or_equal(zitem, oitem)?;
Ok(PyComparisonValue::Implemented(res))
}
}

Expand Down
Loading