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

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
Prev Previous commit
Fix cond
  • Loading branch information
@ShaharNaveh
ShaharNaveh committed Mar 7, 2026
commit 9ddfc67b13c94ee29e0c5d79e53e23a7946e804a
2 changes: 1 addition & 1 deletion crates/vm/src/builtins/list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -562,7 +562,7 @@

// Search for the first index where items are different.
let mut i = 0;
while i < zlen || i < olen {
while i < zlen && i < olen {
if zelements.len() != zlen {
// Comparison mutated the list; refetch it.
zelements = zelf.borrow_vec().to_vec();
Expand All @@ -573,10 +573,10 @@
oelements = other.borrow_vec().to_vec();
}
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];

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
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;
}

Expand Down Expand Up @@ -613,9 +613,9 @@
// 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