Skip to content

Enable TestMethodsMutating #6215

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Enable TestMethodsMutating #6215

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3440811
Enable TestMethodsMutating
kralka Oct 25, 2025
35d8120
[squash] Maybe a forgotten skipped test?
kralka Oct 25, 2025
d7c8839
[squash] add cleanup by ShaharNaveh
kralka Oct 26, 2025
71066ce
[squash] Test of changed size in the lookup mehtod
kralka Oct 28, 2025
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
1 change: 0 additions & 1 deletion Lib/test/test_set.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1898,7 +1898,6 @@ class TestBinaryOpsMutating_Subclass_Set(TestBinaryOpsMutating, unittest.TestCas
constructor2 = set


@unittest.skip("TODO: RUSTPYTHON; segfault")
class TestMethodsMutating(TestOperationsMutating):

def test_issubset_with_mutation(self):
Expand Down
10 changes: 6 additions & 4 deletions vm/src/dict_inner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -582,10 +582,12 @@ impl<T: Clone> Dict<T> {
});
loop {
let index_index = idxs.next();
let index_entry = *unsafe {
// Safety: index_index is generated
inner.indices.get_unchecked(index_index)
};
let index_entry_ptr = inner.indices.get(index_index);
Copy link
Collaborator

@ShaharNaveh ShaharNaveh Oct 25, 2025

Choose a reason for hiding this comment

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

Would something like this work?

let index_entry = match inner.indices.get(index_index) {
  None => continue 'outer,
  Some(v) => v,
};

Copy link
Author

@kralka kralka Oct 28, 2025

Choose a reason for hiding this comment

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

Yes, done, thank you.

if index_entry_ptr.is_none() {
// Dictionary was modified under our hands, see TestMethodsMutating.
continue 'outer;
}
let index_entry = *index_entry_ptr.unwrap();
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Incomplete fix: idxs must be reset when concurrent modification is detected.

The safety check correctly detects when the dictionary is modified concurrently (e.g., when user's __eq__ method clears the dictionary, shrinking it from size 16 to 8), but idxs retains its stale mask. When continuing the outer loop, line 580-582 will reuse the existing GenIndexes via get_or_insert_with, causing idxs.next() to keep generating out-of-bounds indices and triggering an infinite retry loop.

Apply this diff to reset idxs when concurrent modification is detected:

                 let index_entry_ptr = inner.indices.get(index_index);
                 if index_entry_ptr.is_none() {
                     // Dictionary was modified under our hands, see TestMethodsMutating.
+                    idxs = None;
                     continue 'outer;
                 }
                 let index_entry = *index_entry_ptr.unwrap();
📝 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
let index_entry_ptr = inner.indices.get(index_index);
if index_entry_ptr.is_none() {
// Dictionary was modified under our hands, see TestMethodsMutating.
continue 'outer;
}
let index_entry = *index_entry_ptr.unwrap();
let index_entry_ptr = inner.indices.get(index_index);
if index_entry_ptr.is_none() {
// Dictionary was modified under our hands, see TestMethodsMutating.
idxs = None;
continue 'outer;
}
let index_entry = *index_entry_ptr.unwrap();
🤖 Prompt for AI Agents 
In vm/src/dict_inner.rs around lines 585 to 590, when the
concurrent-modification check finds index_entry_ptr.is_none() you must reset the
cached GenIndexes (idxs) before continuing the 'outer loop so the next iteration
will recreate it with the current mask; modify the branch to clear/take the idxs
(set to None or take ownership) immediately prior to continue 'outer to avoid
reusing a stale mask that causes out-of-bounds indices and an infinite retry
loop.

match index_entry {
IndexEntry::DUMMY => {
if free_slot.is_none() {
Expand Down
Loading