X Tutup
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 all commits
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
4 changes: 4 additions & 0 deletions .cspell.dict/rust-more.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ muldiv
nanos
nonoverlapping
objclass
oelements
olen
peekable
pemfile
powc
Expand Down Expand Up @@ -91,3 +93,5 @@ widestring
winapi
winresource
winsock
zelements
zlen
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__();
let mut olen = other.__len__();
if matches!(op, PyComparisonOp::Eq | PyComparisonOp::Ne) && (zlen != olen) {
// Shortcut: if the lengths differ, the lists differ
return Ok(PyComparisonValue::Implemented(matches!(
op,
PyComparisonOp::Ne
)));
}

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();
}
Comment on lines +560 to +574
Copy link
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];

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

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zitem)
let oitem = &oelements[i];

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

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (oitem)

if !vm.bool_eq(zitem, oitem)? {

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

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (oitem)

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

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zitem)
Copy link
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];

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

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zitem)
let oitem = &oelements[i];

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

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (oitem)
let res = vm.identical_or_equal(zitem, oitem)?;

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

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (oitem)

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

View workflow job for this annotation

GitHub Actions / Lint Rust & Python code 

Unknown word (zitem)
Ok(PyComparisonValue::Implemented(res))
}
}

Expand Down
Loading
X Tutup