From 7136539830d98f13f0daea775a59b7f98038377b Mon Sep 17 00:00:00 2001 From: Jorge Polanco <55784702+Jorge-Polanco-Roque@users.noreply.github.com> Date: Sat, 29 Aug 2026 19:04:58 -0600 Subject: [PATCH 1/8] Give dict a CPython-style message for unhashable keys Dict operations (subscription, membership, get/pop/setdefault, ...) now raise "cannot use 'X' as a dict key (unhashable type: 'X')", matching CPython 3.14 and mirroring the existing PySetInner wrapping. The key is only materialized on the error path, so hashable keys pay no extra cost. Enables Lib/test/test_dict.py::DictTest::test_unhashable_key. Assisted-by: Claude Opus 4.8 (Anthropic) --- Lib/test/test_dict.py | 1 - crates/vm/src/builtins/dict.rs | 56 +++++++++++++++++++++++++--------- 2 files changed, 42 insertions(+), 15 deletions(-) diff --git a/Lib/test/test_dict.py b/Lib/test/test_dict.py index e11dd17f1d9..b0a999c54ef 100644 --- a/Lib/test/test_dict.py +++ b/Lib/test/test_dict.py @@ -1614,7 +1614,6 @@ class Shenanigans: self.assertEqual(holds_reference.ref['data'], 42) self.assertEqual(holds_reference.attr, "whatever") - @unittest.expectedFailure # TODO: RUSTPYTHON def test_unhashable_key(self): d = {'a': 1} key = [1, 2, 3] diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index 9837c7c86de..c09bc9e2834 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -293,6 +293,30 @@ impl PyDict { self.entries.len() == 0 } + /// Re-wrap the `TypeError` raised for an unhashable key with the + /// dict-specific wording used by CPython (mirrors `PySetInner`). + /// The key is only materialized on the error path, so hashable keys pay + /// no extra cost. + fn wrap_unhashable_error( + result: PyResult, + key: &K, + vm: &VirtualMachine, + ) -> PyResult { + match result { + Err(cause) if cause.fast_isinstance(vm.ctx.exceptions.type_error) => { + let message = cause.as_object().str(vm)?; + let key = key.to_pyobject(vm); + let err = vm.new_type_error(format!( + "cannot use '{}' as a dict key ({message})", + key.class().name() + )); + err.set___cause__(Some(cause)); + Err(err) + } + result => result, + } + } + /// Set item variant which can be called with multiple /// key types, such as str to name a notable one. pub fn inner_setitem( @@ -301,7 +325,7 @@ impl PyDict { value: PyObjectRef, vm: &VirtualMachine, ) -> PyResult<()> { - self.entries.insert(vm, key, value) + Self::wrap_unhashable_error(self.entries.insert(vm, key, value), key, vm) } pub(crate) fn inner_delitem( @@ -309,7 +333,7 @@ impl PyDict { key: &K, vm: &VirtualMachine, ) -> PyResult<()> { - self.entries.delete(vm, key) + Self::wrap_unhashable_error(self.entries.delete(vm, key), key, vm) } pub fn get_or_insert( @@ -426,7 +450,7 @@ impl PyDict { } fn __contains__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult { - self.entries.contains(vm, &*key) + Self::wrap_unhashable_error(self.entries.contains(vm, &*key), &*key, vm) } fn __delitem__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { @@ -454,10 +478,8 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - Ok(self - .entries - .get(vm, &*key)? - .unwrap_or_else(|| default.unwrap_or_none(vm))) + let found = Self::wrap_unhashable_error(self.entries.get(vm, &*key), &*key, vm)?; + Ok(found.unwrap_or_else(|| default.unwrap_or_none(vm))) } #[pymethod] @@ -467,8 +489,12 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - self.entries - .setdefault(vm, &*key, || default.unwrap_or_none(vm)) + Self::wrap_unhashable_error( + self.entries + .setdefault(vm, &*key, || default.unwrap_or_none(vm)), + &*key, + vm, + ) } #[pymethod] @@ -512,7 +538,7 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - match self.entries.pop(vm, &*key)? { + match Self::wrap_unhashable_error(self.entries.pop(vm, &*key), &*key, vm)? { Some(value) => Ok(value), None => default.ok_or_else(|| vm.new_key_error(key)), } @@ -660,9 +686,10 @@ impl AsMapping for PyDict { impl AsSequence for PyDict { fn as_sequence() -> &'static PySequenceMethods { static AS_SEQUENCE: LazyLock = LazyLock::new(|| PySequenceMethods { - contains: atomic_func!(|seq, target, vm| PyDict::sequence_downcast(seq) - .entries - .contains(vm, target)), + contains: atomic_func!(|seq, target, vm| { + let result = PyDict::sequence_downcast(seq).entries.contains(vm, target); + PyDict::wrap_unhashable_error(result, target, vm) + }), ..PySequenceMethods::NOT_IMPLEMENTED }); &AS_SEQUENCE @@ -765,7 +792,8 @@ impl Py { key: &K, vm: &VirtualMachine, ) -> PyResult { - if let Some(value) = self.entries.get(vm, key)? { + let found = PyDict::wrap_unhashable_error(self.entries.get(vm, key), key, vm)?; + if let Some(value) = found { Ok(value) } else if let Some(value) = self.missing_opt(key, vm)? { Ok(value) From e3beef032c37be4970de9f12d2d437c0cc3c76b4 Mon Sep 17 00:00:00 2001 From: Jorge Polanco <55784702+Jorge-Polanco-Roque@users.noreply.github.com> Date: Sat, 29 Aug 2026 20:28:26 -0600 Subject: [PATCH 2/8] dict: only rewrite hashing failures, not key-comparison errors wrap_unhashable_error previously rewrote any TypeError from a dict operation as an unhashable-key error, but those operations also compare keys on a hash collision, so a TypeError from a colliding key's __eq__ was mislabeled. Disambiguate on the error path by re-hashing the key, so the successful path still hashes exactly once (keeps the do-not-rehash / atomic invariants) while comparison errors propagate unchanged. Adds a colliding-key regression test. Assisted-by: Claude Opus 4.8 (Anthropic) --- crates/vm/src/builtins/dict.rs | 14 +++++++++++--- extra_tests/snippets/builtin_dict.py | 27 +++++++++++++++++++++++++++ 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index c09bc9e2834..737db1cef24 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -295,15 +295,23 @@ impl PyDict { /// Re-wrap the `TypeError` raised for an unhashable key with the /// dict-specific wording used by CPython (mirrors `PySetInner`). - /// The key is only materialized on the error path, so hashable keys pay - /// no extra cost. + /// + /// Dict operations both hash the key and, on a hash collision, compare it + /// against existing keys, so a `TypeError` can come from either step. Only + /// genuine hashing failures should be rewritten; a `TypeError` from key + /// comparison (e.g. a colliding key's `__eq__`) must propagate unchanged. + /// We disambiguate on the error path by re-hashing the key: the successful + /// (common) path hashes exactly once, so hashable keys pay no extra cost. fn wrap_unhashable_error( result: PyResult, key: &K, vm: &VirtualMachine, ) -> PyResult { match result { - Err(cause) if cause.fast_isinstance(vm.ctx.exceptions.type_error) => { + Err(cause) + if cause.fast_isinstance(vm.ctx.exceptions.type_error) + && key.key_hash(vm).is_err() => + { let message = cause.as_object().str(vm)?; let key = key.to_pyobject(vm); let err = vm.new_type_error(format!( diff --git a/extra_tests/snippets/builtin_dict.py b/extra_tests/snippets/builtin_dict.py index 83a8c5f9945..9ff69e47f3b 100644 --- a/extra_tests/snippets/builtin_dict.py +++ b/extra_tests/snippets/builtin_dict.py @@ -408,3 +408,30 @@ def test_func(**kwargs): assert list(result.keys()) == expected_keys, ( f"Expected {expected_keys}, got {list(result.keys())}" ) + + +# A TypeError raised while *comparing* keys (e.g. from a colliding key's +# __eq__) must propagate unchanged, not be rewritten as an unhashable-key +# error. Only genuine hashing failures get the dict-specific "unhashable" +# wording. +class BadEq: + def __hash__(self): + return 42 # fixed hash forces a collision + + def __eq__(self, other): + raise TypeError("nope") + + +bad = {} +bad[BadEq()] = 1 +with assert_raises(TypeError) as cm: + bad[BadEq()] # hashes fine (42), then compares against the colliding key +assert "nope" in str(cm.exception), str(cm.exception) +assert "dict key" not in str(cm.exception), ( + f"comparison error mislabeled as unhashable: {cm.exception}" +) + +# A genuinely unhashable key still reports the dict-specific message. +with assert_raises(TypeError) as cm: + {}[[]] +assert "as a dict key" in str(cm.exception), str(cm.exception) From c9070986c725a5972b66aa4c874aa5e7a022782a Mon Sep 17 00:00:00 2001 From: Jorge Polanco <55784702+Jorge-Polanco-Roque@users.noreply.github.com> Date: Sun, 30 Aug 2026 16:20:49 -0600 Subject: [PATCH 3/8] dict: hash unhashable keys once up front and cover every insertion path Addresses the review feedback (thanks @luantaraschi): - The message now reaches update(), the constructor and |= as well, not just __setitem__: merge_object_with_override and merge_from_seq2 now hash the key up front and thread it into contains_known_hash/insert_known_hash. - Hash the key once instead of re-hashing on the error path. This threads the hash through the *_known_hash operations (add dict_inner get_known_hash), so a __hash__ that fails only on its first call is still reported instead of escaping unwrapped. - Use the fully-qualified type name (matching CPython's %T). - Only an exact TypeError is rewritten; a __hash__ raising a TypeError subclass now propagates unchanged. pop() and setdefault() have no *_known_hash entry point on the inner map, so they hash once up front for the message and then do their own single lookup. Assisted-by: Claude Opus 4.8 --- crates/vm/src/builtins/dict.rs | 89 ++++++++++++++++------------ crates/vm/src/dict_inner.rs | 11 ++++ extra_tests/snippets/builtin_dict.py | 55 +++++++++++++++++ 3 files changed, 117 insertions(+), 38 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index 737db1cef24..d7a2f075876 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -153,11 +153,12 @@ impl PyDict { Ok(keys_method) => { let keys = keys_method.call((), vm)?.get_iter(vm)?; while let PyIterReturn::Return(key) = keys.next(vm)? { - if !override_existing && dict.contains(vm, &*key)? { + let hash = Self::hash_or_unhashable(&*key, vm)?; + if !override_existing && dict.contains_known_hash(vm, &*key, hash)? { continue; } let val = other.get_item(&*key, vm)?; - dict.insert(vm, &*key, val)?; + dict.insert_known_hash(vm, &*key, hash, val)?; } true } @@ -261,10 +262,11 @@ impl PyDict { for (index, element) in iter.iter::(vm)?.enumerate() { let (key, value) = Self::update_sequence_pair(element?, index, vm)?; - if !override_existing && dict.contains(vm, &*key)? { + let hash = Self::hash_or_unhashable(&*key, vm)?; + if !override_existing && dict.contains_known_hash(vm, &*key, hash)? { continue; } - dict.insert(vm, &*key, value)?; + dict.insert_known_hash(vm, &*key, hash, value)?; } Ok(()) } @@ -293,35 +295,34 @@ impl PyDict { self.entries.len() == 0 } - /// Re-wrap the `TypeError` raised for an unhashable key with the - /// dict-specific wording used by CPython (mirrors `PySetInner`). + /// Hash `key`, turning a hashing failure into the dict-specific + /// "cannot use 'X' as a dict key (...)" wording used by CPython. /// - /// Dict operations both hash the key and, on a hash collision, compare it - /// against existing keys, so a `TypeError` can come from either step. Only - /// genuine hashing failures should be rewritten; a `TypeError` from key - /// comparison (e.g. a colliding key's `__eq__`) must propagate unchanged. - /// We disambiguate on the error path by re-hashing the key: the successful - /// (common) path hashes exactly once, so hashable keys pay no extra cost. - fn wrap_unhashable_error( - result: PyResult, - key: &K, - vm: &VirtualMachine, - ) -> PyResult { - match result { - Err(cause) - if cause.fast_isinstance(vm.ctx.exceptions.type_error) - && key.key_hash(vm).is_err() => - { + /// The returned hash is threaded into the `*_known_hash` operations, so the + /// key is hashed exactly once. This is why the message is produced here + /// rather than around the operation: a `TypeError` from key *comparison* + /// (e.g. a colliding key's `__eq__`) is raised by the operation itself and + /// must propagate unchanged, and hashing up front also means a `__hash__` + /// that fails only intermittently is still reported (the old approach + /// re-hashed on the error path, so a hash that succeeded the second time + /// escaped unwrapped). + fn hash_or_unhashable(key: &K, vm: &VirtualMachine) -> PyResult { + match key.key_hash(vm) { + Ok(hash) => Ok(hash), + // Exact `TypeError` only: a `__hash__` raising a *subclass* of + // `TypeError` (e.g. a user `MyTypeError`) must propagate unchanged, + // matching CPython's exact-type check. + Err(cause) if cause.class().is(vm.ctx.exceptions.type_error) => { let message = cause.as_object().str(vm)?; let key = key.to_pyobject(vm); let err = vm.new_type_error(format!( "cannot use '{}' as a dict key ({message})", - key.class().name() + key.class().fully_qualified_name(vm) )); err.set___cause__(Some(cause)); Err(err) } - result => result, + Err(other) => Err(other), } } @@ -333,7 +334,8 @@ impl PyDict { value: PyObjectRef, vm: &VirtualMachine, ) -> PyResult<()> { - Self::wrap_unhashable_error(self.entries.insert(vm, key, value), key, vm) + let hash = Self::hash_or_unhashable(key, vm)?; + self.entries.insert_known_hash(vm, key, hash, value) } pub(crate) fn inner_delitem( @@ -341,7 +343,12 @@ impl PyDict { key: &K, vm: &VirtualMachine, ) -> PyResult<()> { - Self::wrap_unhashable_error(self.entries.delete(vm, key), key, vm) + let hash = Self::hash_or_unhashable(key, vm)?; + if self.entries.delete_if_exists_known_hash(vm, key, hash)? { + Ok(()) + } else { + Err(vm.new_key_error(key.to_pyobject(vm))) + } } pub fn get_or_insert( @@ -350,6 +357,9 @@ impl PyDict { key: PyObjectRef, default: impl FnOnce() -> PyObjectRef, ) -> PyResult { + // No `setdefault_known_hash` on the inner map; hash once up front for the + // unhashable message, then let `setdefault` do its own (single) lookup. + Self::hash_or_unhashable(&*key, vm)?; self.entries.setdefault(vm, &*key, default) } @@ -458,7 +468,8 @@ impl PyDict { } fn __contains__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult { - Self::wrap_unhashable_error(self.entries.contains(vm, &*key), &*key, vm) + let hash = Self::hash_or_unhashable(&*key, vm)?; + self.entries.contains_known_hash(vm, &*key, hash) } fn __delitem__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { @@ -486,7 +497,8 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - let found = Self::wrap_unhashable_error(self.entries.get(vm, &*key), &*key, vm)?; + let hash = Self::hash_or_unhashable(&*key, vm)?; + let found = self.entries.get_known_hash(vm, &*key, hash)?; Ok(found.unwrap_or_else(|| default.unwrap_or_none(vm))) } @@ -497,12 +509,9 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - Self::wrap_unhashable_error( - self.entries - .setdefault(vm, &*key, || default.unwrap_or_none(vm)), - &*key, - vm, - ) + Self::hash_or_unhashable(&*key, vm)?; + self.entries + .setdefault(vm, &*key, || default.unwrap_or_none(vm)) } #[pymethod] @@ -546,7 +555,8 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - match Self::wrap_unhashable_error(self.entries.pop(vm, &*key), &*key, vm)? { + Self::hash_or_unhashable(&*key, vm)?; + match self.entries.pop(vm, &*key)? { Some(value) => Ok(value), None => default.ok_or_else(|| vm.new_key_error(key)), } @@ -695,8 +705,10 @@ impl AsSequence for PyDict { fn as_sequence() -> &'static PySequenceMethods { static AS_SEQUENCE: LazyLock = LazyLock::new(|| PySequenceMethods { contains: atomic_func!(|seq, target, vm| { - let result = PyDict::sequence_downcast(seq).entries.contains(vm, target); - PyDict::wrap_unhashable_error(result, target, vm) + let hash = PyDict::hash_or_unhashable(target, vm)?; + PyDict::sequence_downcast(seq) + .entries + .contains_known_hash(vm, target, hash) }), ..PySequenceMethods::NOT_IMPLEMENTED }); @@ -800,7 +812,8 @@ impl Py { key: &K, vm: &VirtualMachine, ) -> PyResult { - let found = PyDict::wrap_unhashable_error(self.entries.get(vm, key), key, vm)?; + let hash = PyDict::hash_or_unhashable(key, vm)?; + let found = self.entries.get_known_hash(vm, key, hash)?; if let Some(value) = found { Ok(value) } else if let Some(value) = self.missing_opt(key, vm)? { diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index 76d2c50f0cb..65a16644508 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -573,6 +573,17 @@ impl Dict { self._get_inner(vm, key, hash) } + /// [`Self::get`] with a known hash. Same contract as + /// [`Self::insert_known_hash`]. + pub(crate) fn get_known_hash( + &self, + vm: &VirtualMachine, + key: &K, + hash: HashValue, + ) -> PyResult> { + self._get_inner(vm, key, hash) + } + /// Return a stable entry hint for `key` if present. /// /// The hint is the internal entry index and can be used with diff --git a/extra_tests/snippets/builtin_dict.py b/extra_tests/snippets/builtin_dict.py index 9ff69e47f3b..3ccd542f035 100644 --- a/extra_tests/snippets/builtin_dict.py +++ b/extra_tests/snippets/builtin_dict.py @@ -435,3 +435,58 @@ def __eq__(self, other): with assert_raises(TypeError) as cm: {}[[]] assert "as a dict key" in str(cm.exception), str(cm.exception) + +# The message reaches every insertion path, not just __setitem__: the +# constructor, update() and |= all go through the same wrapping. +for make in ( + lambda: dict([([], 1)]), + lambda: {}.update([([], 1)]), + lambda: {}.__ior__([([], 1)]), +): + with assert_raises(TypeError) as cm: + make() + assert "as a dict key" in str(cm.exception), str(cm.exception) + +# The key is hashed once up front, so a __hash__ that fails only on its first +# call is still reported (a re-hash on the error path would let it escape). +class FlakyHash: + _calls = 0 + + def __hash__(self): + FlakyHash._calls += 1 + if FlakyHash._calls == 1: + raise TypeError("first call fails") + return 0 + + +with assert_raises(TypeError) as cm: + {}[FlakyHash()] +assert "as a dict key" in str(cm.exception), str(cm.exception) + +# The type name is the fully qualified one, like CPython's %T. +def _make_nested(): + class Nested: + __hash__ = None + + return Nested + + +with assert_raises(TypeError) as cm: + {}[_make_nested()()] +assert "_make_nested..Nested" in str(cm.exception), str(cm.exception) + + +# A __hash__ raising a *subclass* of TypeError is left unchanged (CPython +# checks the exact type), so `except MySubclass` still catches it. +class MyTypeError(TypeError): + pass + + +class SubclassHash: + def __hash__(self): + raise MyTypeError("custom") + + +with assert_raises(MyTypeError) as cm: + {}[SubclassHash()] +assert "as a dict key" not in str(cm.exception), str(cm.exception) From 53c4db2c622de9bb29a70a743f8b6461d2edccf1 Mon Sep 17 00:00:00 2001 From: Jorge Polanco <55784702+Jorge-Polanco-Roque@users.noreply.github.com> Date: Sun, 30 Aug 2026 16:32:52 -0600 Subject: [PATCH 4/8] dict: thread the known hash through setdefault and pop as well Follow-up on the review: setdefault() and pop() previously hashed the key a second time (inside the inner map) after hash_or_unhashable already hashed it, so a stateful __hash__ failing on the second call would skip the dict-specific message. Add setdefault_known_hash / pop_known_hash to the inner map and thread the precomputed hash, so every operation now hashes exactly once. Adds a regression test asserting setdefault/pop call __hash__ only once. Assisted-by: Claude Opus 4.8 --- crates/vm/src/builtins/dict.rs | 14 +++++----- crates/vm/src/dict_inner.rs | 38 +++++++++++++++++++++++++++- extra_tests/snippets/builtin_dict.py | 23 +++++++++++++++++ 3 files changed, 66 insertions(+), 9 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index d7a2f075876..3d1f3c7d976 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -357,10 +357,8 @@ impl PyDict { key: PyObjectRef, default: impl FnOnce() -> PyObjectRef, ) -> PyResult { - // No `setdefault_known_hash` on the inner map; hash once up front for the - // unhashable message, then let `setdefault` do its own (single) lookup. - Self::hash_or_unhashable(&*key, vm)?; - self.entries.setdefault(vm, &*key, default) + let hash = Self::hash_or_unhashable(&*key, vm)?; + self.entries.setdefault_known_hash(vm, &*key, hash, default) } pub fn from_attributes(attrs: PyAttributes, vm: &VirtualMachine) -> PyResult { @@ -509,9 +507,9 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - Self::hash_or_unhashable(&*key, vm)?; + let hash = Self::hash_or_unhashable(&*key, vm)?; self.entries - .setdefault(vm, &*key, || default.unwrap_or_none(vm)) + .setdefault_known_hash(vm, &*key, hash, || default.unwrap_or_none(vm)) } #[pymethod] @@ -555,8 +553,8 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - Self::hash_or_unhashable(&*key, vm)?; - match self.entries.pop(vm, &*key)? { + let hash = Self::hash_or_unhashable(&*key, vm)?; + match self.entries.pop_known_hash(vm, &*key, hash)? { Some(value) => Ok(value), None => default.ok_or_else(|| vm.new_key_error(key)), } diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index 65a16644508..753facb1d62 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -883,12 +883,32 @@ impl Dict { Ok(()) } + /// Callers within the crate thread a known hash (see + /// [`Self::setdefault_known_hash`]); this hashing wrapper is kept for API + /// symmetry with the other operations. + #[allow(dead_code)] pub(crate) fn setdefault(&self, vm: &VirtualMachine, key: &K, default: F) -> PyResult where K: DictKey + ?Sized, F: FnOnce() -> T, { let hash = key.key_hash(vm)?; + self.setdefault_known_hash(vm, key, hash, default) + } + + /// [`Self::setdefault`] with a known hash. Same contract as + /// [`Self::insert_known_hash`]. + pub(crate) fn setdefault_known_hash( + &self, + vm: &VirtualMachine, + key: &K, + hash: HashValue, + default: F, + ) -> PyResult + where + K: DictKey + ?Sized, + F: FnOnce() -> T, + { let mut default = Some(default); loop { let (index_entry, index_index) = self.lookup(vm, key, hash, None)?; @@ -1239,13 +1259,29 @@ impl Dict { Ok(ControlFlow::Break(removed)) } - /// Retrieve and delete a key + /// Retrieve and delete a key. + /// + /// Callers within the crate thread a known hash (see + /// [`Self::pop_known_hash`]); this hashing wrapper is kept for API symmetry + /// with the other operations. + #[allow(dead_code)] pub(crate) fn pop( &self, vm: &VirtualMachine, key: &K, ) -> PyResult> { let hash_value = key.key_hash(vm)?; + self.pop_known_hash(vm, key, hash_value) + } + + /// [`Self::pop`] with a known hash. Same contract as + /// [`Self::insert_known_hash`]. + pub(crate) fn pop_known_hash( + &self, + vm: &VirtualMachine, + key: &K, + hash_value: HashValue, + ) -> PyResult> { let removed = loop { let lookup = self.lookup(vm, key, hash_value, None)?; match self.pop_inner(lookup) { diff --git a/extra_tests/snippets/builtin_dict.py b/extra_tests/snippets/builtin_dict.py index 3ccd542f035..ccd21321774 100644 --- a/extra_tests/snippets/builtin_dict.py +++ b/extra_tests/snippets/builtin_dict.py @@ -490,3 +490,26 @@ def __hash__(self): with assert_raises(MyTypeError) as cm: {}[SubclassHash()] assert "as a dict key" not in str(cm.exception), str(cm.exception) + + +# Every operation hashes the key exactly once (the hash is threaded into the +# inner map), so a __hash__ that would fail on a second call is never called +# twice. setdefault() and pop() went through their own lookup before. +class CountingHash: + calls = 0 + + def __hash__(self): + CountingHash.calls += 1 + if CountingHash.calls >= 2: + raise TypeError("must not hash twice") + return 7 + + +CountingHash.calls = 0 +{}.setdefault(CountingHash(), 1) +assert CountingHash.calls == 1, CountingHash.calls + +CountingHash.calls = 0 +with assert_raises(KeyError): + {}.pop(CountingHash()) +assert CountingHash.calls == 1, CountingHash.calls From 63ec8c7b985bacb3011dc42d4a42aee851ebbe93 Mon Sep 17 00:00:00 2001 From: Jorge Polanco <55784702+Jorge-Polanco-Roque@users.noreply.github.com> Date: Mon, 31 Aug 2026 20:39:44 -0600 Subject: [PATCH 5/8] test: hash the key on a non-empty dict so CPython 3.14 exercises the pop path `{}.pop(CountingHash())` on an empty dict does not hash the key under CPython 3.14 (the lookup short-circuits), so `CountingHash.calls == 1` was 0 and the snippet failed its CPython parity run. Use a non-empty dict so the lookup must hash the key exactly once, on both CPython and RustPython. Also applied `ruff format` (two blank lines before top-level defs). Assisted-by: Claude Opus 4.8 (Anthropic) --- extra_tests/snippets/builtin_dict.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/extra_tests/snippets/builtin_dict.py b/extra_tests/snippets/builtin_dict.py index ccd21321774..c0099401ba4 100644 --- a/extra_tests/snippets/builtin_dict.py +++ b/extra_tests/snippets/builtin_dict.py @@ -447,6 +447,7 @@ def __eq__(self, other): make() assert "as a dict key" in str(cm.exception), str(cm.exception) + # The key is hashed once up front, so a __hash__ that fails only on its first # call is still reported (a re-hash on the error path would let it escape). class FlakyHash: @@ -463,6 +464,7 @@ def __hash__(self): {}[FlakyHash()] assert "as a dict key" in str(cm.exception), str(cm.exception) + # The type name is the fully qualified one, like CPython's %T. def _make_nested(): class Nested: @@ -511,5 +513,7 @@ def __hash__(self): CountingHash.calls = 0 with assert_raises(KeyError): - {}.pop(CountingHash()) + # A non-empty dict so the lookup must hash the key (CPython skips hashing + # entirely when popping from an empty dict). + {1: 1}.pop(CountingHash()) assert CountingHash.calls == 1, CountingHash.calls From 7c1695f4c8b65ccf11e9e3afa7277b6bbd000d03 Mon Sep 17 00:00:00 2001 From: Jorge Polanco <55784702+Jorge-Polanco-Roque@users.noreply.github.com> Date: Sat, 5 Sep 2026 09:14:50 -0600 Subject: [PATCH 6/8] dict: drop unused internal pop wrapper Per review, the vm-taking Dict::pop wrapper was dead code (#[allow(dead_code)], kept only for API symmetry) with no callers. Remove it and fold its doc into pop_known_hash. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/vm/src/dict_inner.rs | 17 +---------------- 1 file changed, 1 insertion(+), 16 deletions(-) diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index 753facb1d62..b79dbab28a3 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -1259,22 +1259,7 @@ impl Dict { Ok(ControlFlow::Break(removed)) } - /// Retrieve and delete a key. - /// - /// Callers within the crate thread a known hash (see - /// [`Self::pop_known_hash`]); this hashing wrapper is kept for API symmetry - /// with the other operations. - #[allow(dead_code)] - pub(crate) fn pop( - &self, - vm: &VirtualMachine, - key: &K, - ) -> PyResult> { - let hash_value = key.key_hash(vm)?; - self.pop_known_hash(vm, key, hash_value) - } - - /// [`Self::pop`] with a known hash. Same contract as + /// Retrieve and delete a key, given a known hash. Same contract as /// [`Self::insert_known_hash`]. pub(crate) fn pop_known_hash( &self, From fe49c5931cbc8afd89ecd55e5bed7623376c8de1 Mon Sep 17 00:00:00 2001 From: Jorge Polanco <55784702+Jorge-Polanco-Roque@users.noreply.github.com> Date: Sun, 6 Sep 2026 09:56:42 -0600 Subject: [PATCH 7/8] dict: rename pop_known_hash to pop Per review, with the plain pop gone the _known_hash suffix no longer earns its keep, so drop it. Matches the remaining known-hash helpers that still pair with a base method. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/vm/src/builtins/dict.rs | 2 +- crates/vm/src/dict_inner.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index 3d1f3c7d976..78b7ac5df2f 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -554,7 +554,7 @@ impl PyDict { vm: &VirtualMachine, ) -> PyResult { let hash = Self::hash_or_unhashable(&*key, vm)?; - match self.entries.pop_known_hash(vm, &*key, hash)? { + match self.entries.pop(vm, &*key, hash)? { Some(value) => Ok(value), None => default.ok_or_else(|| vm.new_key_error(key)), } diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index b79dbab28a3..af186821b7d 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -1261,7 +1261,7 @@ impl Dict { /// Retrieve and delete a key, given a known hash. Same contract as /// [`Self::insert_known_hash`]. - pub(crate) fn pop_known_hash( + pub(crate) fn pop( &self, vm: &VirtualMachine, key: &K, From 6e832792317688e64f6634c9cbc855365f3e40ee Mon Sep 17 00:00:00 2001 From: Jorge Polanco <55784702+Jorge-Polanco-Roque@users.noreply.github.com> Date: Sat, 12 Sep 2026 08:57:18 -0600 Subject: [PATCH 8/8] dict: fold setdefault wrapper into setdefault, drop unused setdefault_entry Per review, apply the same pattern as pop: the dead setdefault wrapper only hashed and delegated, so remove it and rename setdefault_known_hash to setdefault (callers already thread the hash). Also drop setdefault_entry, which this PR added but nothing uses. Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/vm/src/builtins/dict.rs | 4 +-- crates/vm/src/dict_inner.rs | 56 ++-------------------------------- 2 files changed, 5 insertions(+), 55 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index 78b7ac5df2f..46abe0d5fa7 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -358,7 +358,7 @@ impl PyDict { default: impl FnOnce() -> PyObjectRef, ) -> PyResult { let hash = Self::hash_or_unhashable(&*key, vm)?; - self.entries.setdefault_known_hash(vm, &*key, hash, default) + self.entries.setdefault(vm, &*key, hash, default) } pub fn from_attributes(attrs: PyAttributes, vm: &VirtualMachine) -> PyResult { @@ -509,7 +509,7 @@ impl PyDict { ) -> PyResult { let hash = Self::hash_or_unhashable(&*key, vm)?; self.entries - .setdefault_known_hash(vm, &*key, hash, || default.unwrap_or_none(vm)) + .setdefault(vm, &*key, hash, || default.unwrap_or_none(vm)) } #[pymethod] diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index af186821b7d..ff42fe3b9e2 100644 --- a/crates/vm/src/dict_inner.rs +++ b/crates/vm/src/dict_inner.rs @@ -883,22 +883,9 @@ impl Dict { Ok(()) } - /// Callers within the crate thread a known hash (see - /// [`Self::setdefault_known_hash`]); this hashing wrapper is kept for API - /// symmetry with the other operations. - #[allow(dead_code)] - pub(crate) fn setdefault(&self, vm: &VirtualMachine, key: &K, default: F) -> PyResult - where - K: DictKey + ?Sized, - F: FnOnce() -> T, - { - let hash = key.key_hash(vm)?; - self.setdefault_known_hash(vm, key, hash, default) - } - - /// [`Self::setdefault`] with a known hash. Same contract as - /// [`Self::insert_known_hash`]. - pub(crate) fn setdefault_known_hash( + /// Get the value for `key`, inserting `default()` if it is absent, given a + /// known hash. Same contract as [`Self::insert_known_hash`]. + pub(crate) fn setdefault( &self, vm: &VirtualMachine, key: &K, @@ -938,43 +925,6 @@ impl Dict { } } - #[allow(dead_code)] - pub(crate) fn setdefault_entry( - &self, - vm: &VirtualMachine, - key: &K, - default: F, - ) -> PyResult<(PyObjectRef, T)> - where - K: DictKey + ?Sized, - F: FnOnce() -> T, - { - let hash = key.key_hash(vm)?; - let mut default = Some(default); - loop { - let (index_entry, index_index) = self.lookup(vm, key, hash, None)?; - if let Some(index) = index_entry.index() { - let inner = self.read(); - if let Some(entry) = inner.get_entry_checked(index, index_index) { - return Ok((entry.key.clone(), entry.value.clone())); - } - continue; - } - let mut inner = self.write(); - if inner.indices.get(index_index) != Some(&index_entry) { - continue; - } - let value = default - .take() - .expect("default must only be computed on insertion")(); - let key_obj = key.to_pyobject(vm); - let ret = (key_obj.clone(), value.clone()); - self.invalidate_keys_version(); - inner.unchecked_push(index_index, hash, key_obj, value, index_entry); - return Ok(ret); - } - } - pub(crate) fn len(&self) -> usize { self.read().used }