From 1889ee627fb6d17ac88a68e3e10b850419b60d89 Mon Sep 17 00:00:00 2001 From: youknowdot Date: Sat, 10 Oct 2026 05:44:03 -0700 Subject: [PATCH] Track live entries in reverse dictionary iterators (#9046) * Track live entries in reverse dictionary iterators Extract the Python 3.14-compatible remaining-count and same-size storage handling from #8954. Preserve the older deferred exhaustion and extra-yield contract, using explicit wrapping arithmetic rather than the Python 3.15 extra-yield error and cursor clamp. Cover deleted-slot hints and mutation after the last yield in the existing Python snippet. Assisted-by: OpenAI Codex:model identifier unavailable * Enable the repaired reverse-dict restoration regression The patched native interpreter unexpectedly passes the existing reverse-dict clear/restore test. Remove only its obsolete expected-failure decorator, preserving the canonical assertions and all other markers. Assisted-by: OpenAI Codex:model identifier unavailable * Preserve reverse iterator invalidation on atomic relocation Assisted-by: OpenAI Codex:model identifier unavailable --- Lib/test/test_dict.py | 1 - crates/vm/src/builtins/dict.rs | 25 ++++++++++++++---------- crates/vm/src/dict_inner.rs | 29 +++++++++++++++++++++------- extra_tests/snippets/builtin_dict.py | 19 ++++++++++++++++++ 4 files changed, 56 insertions(+), 18 deletions(-) diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index b0a999c54..195466e66 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1405,7 +1405,6 @@ class DictTest(unittest.TestCase): self.assertEqual(list(reversed(A(1, 0).__dict__)), ['x']) self.assertEqual(list(reversed(A(0, 1).__dict__)), ['y']) - @unittest.expectedFailure # TODO: RUSTPYTHON; RuntimeError: dictionary changed size during iteration def test_reversed_dict_after_clear_and_restore(self): d = {} for i in range(1000): diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index a0f58236a..a98ba4a43 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -1464,6 +1464,7 @@ macro_rules! dict_view { pub(crate) size: dict_inner::DictSize, /// As in `$iter_name`. changed: PyAtomic, + remaining: PyAtomic, internal: PyMutex>, } @@ -1477,8 +1478,10 @@ macro_rules! dict_view { impl $reverse_iter_name { fn new(dict: PyDictRef) -> Self { let size = dict.size(); - let position = size.entries_size.saturating_sub(1); + // usize::MAX represents the position before the first entry. + let position = size.entries_size.wrapping_sub(1); $reverse_iter_name { + remaining: Radium::new(size.used), size, changed: Radium::new(false), internal: PyMutex::new(PositionIterInternal::new(dict, position)), @@ -1520,8 +1523,10 @@ macro_rules! dict_view { } let internal = self.internal.lock(); match &internal.status { - IterStatus::Active(dict) if dict.size() == self.size => { - internal.rev_length_hint(|_| self.size.entries_size) + IterStatus::Active(dict) + if dict.size().has_same_used_and_relocations(&self.size) => + { + self.remaining.load(Ordering::Relaxed) } _ => 0, } @@ -1553,13 +1558,13 @@ macro_rules! dict_view { (Err(mutated()), None) } Ok(Some((found_index, item))) => { - let released = if found_index == 0 { - internal.exhaust() - } else { - internal.position = found_index - 1; - None - }; - (Ok(PyIterReturn::Return(($result_fn)(vm, item))), released) + // CPython 3.14 exposes the remaining count as size_t, + // even after same-size key replacement yields extra entries. + let remaining = zelf.remaining.load(Ordering::Relaxed); + zelf.remaining + .store(remaining.wrapping_sub(1), Ordering::Relaxed); + internal.position = found_index.wrapping_sub(1); + (Ok(PyIterReturn::Return(($result_fn)(vm, item))), None) } Ok(None) => (Ok(PyIterReturn::StopIteration(None)), internal.exhaust()), } diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index aaa3b9276..9bbc2550a 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -184,6 +184,8 @@ impl IndexEntry { struct DictInner { used: usize, filled: usize, + // Only private atomic relocations invalidate same-size reverse iteration. + relocation_epoch: u64, indices: Vec, entries: Vec>>, } @@ -210,7 +212,7 @@ impl Clone for Dict { } // Keep dense copies cheap, but do not carry mostly empty storage into // a new dictionary. pop_back can leave oversized indices without holes. - let inner = if inner.used >= inner.entries.len() - inner.entries.len() / 3 + let mut inner = if inner.used >= inner.entries.len() - inner.entries.len() / 3 && (inner.indices.len() <= 8 || inner.used >= inner.indices.len() / 4) { inner.clone() @@ -218,6 +220,7 @@ impl Clone for Dict { let mut copy = DictInner { used: inner.used, filled: inner.used, + relocation_epoch: 0, indices: Vec::new(), entries: Vec::with_capacity(inner.used), }; @@ -226,6 +229,7 @@ impl Clone for Dict { copy.resize(inner.used * 2); copy }; + inner.relocation_epoch = 0; Self { inner: PyRwLock::new(inner), keys_version: AtomicU32::new(0), @@ -239,6 +243,7 @@ impl Default for Dict { inner: PyRwLock::new(DictInner { used: 0, filled: 0, + relocation_epoch: 0, indices: vec![IndexEntry::FREE; 8], entries: Vec::new(), }), @@ -262,9 +267,16 @@ pub struct DictSize { pub entries_size: usize, pub used: usize, filled: usize, + relocation_epoch: u64, } -/// The dict was resized under an iterator holding an older [`DictSize`]. +impl DictSize { + pub(crate) fn has_same_used_and_relocations(&self, other: &Self) -> bool { + self.used == other.used && self.relocation_epoch == other.relocation_epoch + } +} + +/// The dict changed under an iterator holding an older [`DictSize`]. #[derive(Debug)] pub(crate) struct DictChanged; @@ -387,6 +399,7 @@ impl DictInner { entries_size: self.entries.len(), used: self.used, filled: self.filled, + relocation_epoch: self.relocation_epoch, } } @@ -856,10 +869,12 @@ impl Dict { /// Install an owned table, releasing the old entries after unlocking. pub(crate) fn replace_contents(&self, other: Self) { - let replacement = other.inner.into_inner(); + let mut replacement = other.inner.into_inner(); let _removed = { let mut inner = self.write(); self.invalidate_keys_version(); + // Relocation history belongs to this dict, not the incoming table. + replacement.relocation_epoch = inner.relocation_epoch; core::mem::replace(&mut *inner, replacement) }; } @@ -1082,7 +1097,8 @@ impl Dict { } } - /// [`Self::next_entry_checked`] in reverse. + /// Step backward, validating the live size and private relocation history + /// under the same read guard. Ordinary same-size mutations may change layout. pub(crate) fn prev_entry_checked( &self, mut position: EntryIndex, @@ -1090,7 +1106,7 @@ impl Dict { project: impl FnOnce(&PyObject, &T) -> R, ) -> Result, DictChanged> { let inner = self.read(); - if inner.size() != *old { + if !inner.size().has_same_used_and_relocations(old) { return Err(DictChanged); } loop { @@ -1471,6 +1487,7 @@ impl Dict { // this critical section. inner.entries.reserve(1); self.invalidate_keys_version(); + inner.relocation_epoch = inner.relocation_epoch.wrapping_add(1); let entry = inner.entries[entry_index].take().unwrap(); let new_index = inner.entries.len(); inner.indices[entry.index] = unsafe { @@ -1482,8 +1499,6 @@ impl Dict { if holes >= 2 && holes >= inner.used { // Moves do not increase `filled`, so ordinary insertion's // resize threshold cannot bound their accumulated holes. - // Two holes also ensure this operation changes DictSize, - // allowing existing iterators to detect the relocation. let indices_size = inner.indices.len(); inner.resize(indices_size); } diff --git a/extra_tests/snippets/builtin_dict.py b/extra_tests/snippets/builtin_dict.py index fb84bf48a..e593b92fd 100644 --- a/extra_tests/snippets/builtin_dict.py +++ b/extra_tests/snippets/builtin_dict.py @@ -378,6 +378,25 @@ assert it.__length_hint__() == 0 assert_raises(StopIteration, next, it) assert it.__length_hint__() == 0 +# Reverse hints count live entries, including when storage has deleted slots. +for reverse_iter in ( + reversed, + lambda d: reversed(d.keys()), + lambda d: reversed(d.values()), + lambda d: reversed(d.items()), +): + d = {0: 0, 1: 1, 2: 2, 3: 3} + del d[1] + it = reverse_iter(d) + for remaining in (3, 2, 1): + assert it.__length_hint__() == remaining + next(it) + assert it.__length_hint__() == 0 + # Exhaustion is observed by next(), rather than the final successful yield. + d[4] = 4 + assert_raises(RuntimeError, next, it) + assert it.__length_hint__() == 0 + # Test dictionary unpacking with non-mapping objects # This should raise TypeError for non-mapping objects with assert_raises(TypeError) as cm: