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..46abe0d5fa7 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,6 +295,37 @@ impl PyDict { self.entries.len() == 0 } + /// Hash `key`, turning a hashing failure into the dict-specific + /// "cannot use 'X' as a dict key (...)" wording used by CPython. + /// + /// 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().fully_qualified_name(vm) + )); + err.set___cause__(Some(cause)); + Err(err) + } + Err(other) => Err(other), + } + } + /// 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 +334,8 @@ impl PyDict { value: PyObjectRef, vm: &VirtualMachine, ) -> PyResult<()> { - self.entries.insert(vm, key, value) + let hash = Self::hash_or_unhashable(key, vm)?; + self.entries.insert_known_hash(vm, key, hash, value) } pub(crate) fn inner_delitem( @@ -309,7 +343,12 @@ impl PyDict { key: &K, vm: &VirtualMachine, ) -> PyResult<()> { - self.entries.delete(vm, key) + 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( @@ -318,7 +357,8 @@ impl PyDict { key: PyObjectRef, default: impl FnOnce() -> PyObjectRef, ) -> PyResult { - self.entries.setdefault(vm, &*key, default) + let hash = Self::hash_or_unhashable(&*key, vm)?; + self.entries.setdefault(vm, &*key, hash, default) } pub fn from_attributes(attrs: PyAttributes, vm: &VirtualMachine) -> PyResult { @@ -426,7 +466,8 @@ impl PyDict { } fn __contains__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult { - self.entries.contains(vm, &*key) + let hash = Self::hash_or_unhashable(&*key, vm)?; + self.entries.contains_known_hash(vm, &*key, hash) } fn __delitem__(&self, key: PyObjectRef, vm: &VirtualMachine) -> PyResult<()> { @@ -454,10 +495,9 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - Ok(self - .entries - .get(vm, &*key)? - .unwrap_or_else(|| default.unwrap_or_none(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))) } #[pymethod] @@ -467,8 +507,9 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { + let hash = Self::hash_or_unhashable(&*key, vm)?; self.entries - .setdefault(vm, &*key, || default.unwrap_or_none(vm)) + .setdefault(vm, &*key, hash, || default.unwrap_or_none(vm)) } #[pymethod] @@ -512,7 +553,8 @@ impl PyDict { default: OptionalArg, vm: &VirtualMachine, ) -> PyResult { - match self.entries.pop(vm, &*key)? { + let hash = Self::hash_or_unhashable(&*key, vm)?; + match self.entries.pop(vm, &*key, hash)? { Some(value) => Ok(value), None => default.ok_or_else(|| vm.new_key_error(key)), } @@ -660,9 +702,12 @@ 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 hash = PyDict::hash_or_unhashable(target, vm)?; + PyDict::sequence_downcast(seq) + .entries + .contains_known_hash(vm, target, hash) + }), ..PySequenceMethods::NOT_IMPLEMENTED }); &AS_SEQUENCE @@ -765,7 +810,9 @@ impl Py { key: &K, vm: &VirtualMachine, ) -> PyResult { - if let Some(value) = self.entries.get(vm, key)? { + 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)? { Ok(value) diff --git a/crates/vm/src/dict_inner.rs b/crates/vm/src/dict_inner.rs index 76d2c50f0cb..ff42fe3b9e2 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 @@ -872,12 +883,19 @@ impl Dict { Ok(()) } - pub(crate) fn setdefault(&self, vm: &VirtualMachine, key: &K, default: F) -> PyResult + /// 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, + hash: HashValue, + default: F, + ) -> PyResult 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)?; @@ -907,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 } @@ -1228,13 +1209,14 @@ impl Dict { Ok(ControlFlow::Break(removed)) } - /// Retrieve and delete a key + /// Retrieve and delete a key, given a known hash. Same contract as + /// [`Self::insert_known_hash`]. pub(crate) fn pop( &self, vm: &VirtualMachine, key: &K, + hash_value: HashValue, ) -> PyResult> { - let hash_value = key.key_hash(vm)?; 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 83a8c5f9945..c0099401ba4 100644 --- a/extra_tests/snippets/builtin_dict.py +++ b/extra_tests/snippets/builtin_dict.py @@ -408,3 +408,112 @@ 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) + +# 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) + + +# 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): + # 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