From 90afe00d6c3c4ce4db71062867abf6246a76e20a Mon Sep 17 00:00:00 2001 From: 1ndahous3 <9205230+1ndahous3@users.noreply.github.com> Date: Sun, 27 Sep 2026 07:40:51 +0300 Subject: [PATCH] Fix dict view comparisons and avoid temporary sets (#8849) Assisted-by: Codex:GPT-6 --- crates/vm/src/builtins/dict.rs | 141 +++++++++++------------ extra_tests/snippets/builtin_dict.py | 160 +++++++++++++++++++++++++++ 2 files changed, 225 insertions(+), 76 deletions(-) diff --git a/crates/vm/src/builtins/dict.rs b/crates/vm/src/builtins/dict.rs index 2bed68c59..1afb16c03 100644 --- a/crates/vm/src/builtins/dict.rs +++ b/crates/vm/src/builtins/dict.rs @@ -1,6 +1,7 @@ use super::{ - IterStatus, PositionIterInternal, PyBaseExceptionRef, PyGenericAlias, PyMappingProxy, PySet, - PyStr, PyStrRef, PyTupleRef, PyType, PyTypeRef, locked_step, set, set::PySetInner, + IterStatus, PositionIterInternal, PyBaseExceptionRef, PyFrozenSet, PyGenericAlias, + PyMappingProxy, PySet, PyStr, PyStrRef, PyTupleRef, PyType, PyTypeRef, locked_step, set, + set::PySetInner, }; use crate::common::lock::LazyLock; use crate::object::{Traverse, TraverseFn}; @@ -13,10 +14,7 @@ use crate::{ common::{ascii, hash::PyHash}, dict_inner::{self, DictKey}, function::{ArgIterable, FuncArgs, KwArgs, OptionalArg, PyArithmeticValue, PyComparisonValue}, - iter::PyExactSizeIterator, - protocol::{ - PyIter, PyIterIter, PyIterReturn, PyMappingMethods, PyNumberMethods, PySequenceMethods, - }, + protocol::{PyIter, PyIterReturn, PyMappingMethods, PyNumberMethods, PySequenceMethods}, recursion::ReprGuard, types::{ AsMapping, AsNumber, AsSequence, Callable, Comparable, Constructor, DefaultConstructor, @@ -1489,87 +1487,79 @@ dict_view! { vm.new_tuple((key, value)).into() } +fn is_set_or_dict_view(obj: &PyObject) -> bool { + obj.downcast_ref::().is_some() + || obj.downcast_ref::().is_some() + || obj.downcast_ref::().is_some() + || obj.downcast_ref::().is_some() +} + // Set operations defined on set-like views of the dictionary. #[pyclass] trait ViewSetOps: DictView { - fn to_set(zelf: PyRef, vm: &VirtualMachine) -> PyResult { - let len = zelf.dict().__len__(); - let zelf: PyObjectRef = Self::iter(zelf, vm)?; - let iter = PyIterIter::new(vm, zelf, Some(len)); - PySetInner::from_iter(iter, vm) - } - - fn __xor__(zelf: PyRef, other: ArgIterable, vm: &VirtualMachine) -> PyResult { - let zelf = Self::to_set(zelf, vm)?; - let inner = zelf.symmetric_difference(other, vm)?; - Ok(PySet { inner }) - } - - fn __and__(zelf: PyRef, other: ArgIterable, vm: &VirtualMachine) -> PyResult { - let zelf = Self::to_set(zelf, vm)?; - let inner = zelf.intersection(other, vm)?; - Ok(PySet { inner }) - } - - fn __or__(zelf: PyRef, other: ArgIterable, vm: &VirtualMachine) -> PyResult { - let zelf = Self::to_set(zelf, vm)?; - let inner = zelf.union(other, vm)?; - Ok(PySet { inner }) - } - - fn __sub__(zelf: PyRef, other: ArgIterable, vm: &VirtualMachine) -> PyResult { - let zelf = Self::to_set(zelf, vm)?; - let inner = zelf.difference(other, vm)?; - Ok(PySet { inner }) - } - - fn __rsub__(zelf: PyRef, other: ArgIterable, vm: &VirtualMachine) -> PyResult { - let left = PySetInner::from_iter(other.iter(vm)?, vm)?; - let right = ArgIterable::try_from_object(vm, Self::iter(zelf, vm)?)?; - let inner = left.difference(right, vm)?; - Ok(PySet { inner }) - } - fn cmp( zelf: &Py, other: &PyObject, op: PyComparisonOp, vm: &VirtualMachine, ) -> PyResult { - match_class!(match other { - ref dictview @ Self => { - return zelf.dict().inner_cmp( - dictview.dict(), - op, - !zelf.class().is(vm.ctx.types.dict_keys_type), - vm, - ); + if let Some(dictview) = other.downcast_ref::() { + return zelf.dict().inner_cmp( + dictview.dict(), + op, + !zelf.class().is(vm.ctx.types.dict_keys_type), + vm, + ); + } + if !is_set_or_dict_view(other) { + return Ok(PyComparisonValue::NotImplemented); + } + if op == PyComparisonOp::Ne { + return ViewSetOps::cmp(zelf, other, PyComparisonOp::Eq, vm) + .map(|result| result.map(|equal| !equal)); + } + if !op.eval_ord(zelf.__len__().cmp(&other.length(vm)?)) { + return Ok(PyComparisonValue::Implemented(false)); + } + let (subset, superset) = if matches!(op, PyComparisonOp::Gt | PyComparisonOp::Ge) { + (other, zelf.as_object()) + } else { + (zelf.as_object(), other) + }; + let subset = ArgIterable::::try_from_object(vm, subset.to_owned())?; + for item in subset.iter(vm)? { + let item = item?; + if !superset.sequence_unchecked().contains(&item, vm)? { + return Ok(PyComparisonValue::Implemented(false)); } - ref _set @ PySet => { - let inner = Self::to_set(zelf.to_owned(), vm)?; - let zelf_set = PySet { inner }.into_pyobject(vm); - return PySet::cmp(zelf_set.downcast_ref().unwrap(), other, op, vm); - } - ref _dictitems @ PyDictItems => {} - ref _dictkeys @ PyDictKeys => {} - _ => { - return Ok(PyArithmeticValue::NotImplemented); - } - }); - let lhs: Vec = zelf.as_object().to_owned().try_into_value(vm)?; - let rhs: Vec = other.to_owned().try_into_value(vm)?; - lhs.iter() - .map(|o| &**o) - .richcompare(rhs.iter().map(|o| &**o), op, vm) - .map(PyComparisonValue::Implemented) + } + Ok(PyComparisonValue::Implemented(true)) } #[pymethod] fn isdisjoint(zelf: PyRef, object: ArgIterable, vm: &VirtualMachine) -> PyResult { - // TODO: to_set is an expensive operation. After merging #3316 rewrite implementation using PySequence_Contains. - let zelf = Self::to_set(zelf, vm)?; - let result = zelf.isdisjoint(object, vm)?; - Ok(result) + if zelf.is(object.as_object()) { + return Ok(zelf.__len__() == 0); + } + let other_is_larger = if is_set_or_dict_view(object.as_object()) { + let len = zelf.__len__(); + object.as_object().length(vm)? > len + } else { + false + }; + let (container, iterable) = if other_is_larger { + (object.as_object(), zelf.as_object()) + } else { + (zelf.as_object(), object.as_object()) + }; + let iterable = ArgIterable::::try_from_object(vm, iterable.to_owned())?; + for item in iterable.iter(vm)? { + let item = item?; + if container.sequence_unchecked().contains(&item, vm)? { + return Ok(false); + } + } + Ok(true) } } @@ -1687,11 +1677,10 @@ impl AsSequence for PyDictItems { let zelf = PyDictItems::sequence_downcast(seq); let key = &needle.as_slice()[0]; - if !zelf.dict.__contains__(key, vm)? { + let Some(found) = zelf.dict().inner_getitem_opt(&**key, vm)? else { return Ok(false); - } + }; let value = &needle.as_slice()[1]; - let found = zelf.dict().__getitem__(key.to_owned(), vm)?; vm.identical_or_equal(&found, value) }), ..PySequenceMethods::NOT_IMPLEMENTED diff --git a/extra_tests/snippets/builtin_dict.py b/extra_tests/snippets/builtin_dict.py index 83a8c5f99..bfda8a08b 100644 --- a/extra_tests/snippets/builtin_dict.py +++ b/extra_tests/snippets/builtin_dict.py @@ -408,3 +408,163 @@ expected_keys = ["x", "y", "w", "z"] assert list(result.keys()) == expected_keys, ( f"Expected {expected_keys}, got {list(result.keys())}" ) + + +def check_view_comparisons(view, other, expected): + assert ( + view == other, + view != other, + view < other, + view <= other, + view > other, + view >= other, + ) == expected + assert ( + other == view, + other != view, + other > view, + other >= view, + other < view, + other <= view, + ) == expected + + +# Dictionary views support the same comparisons with mutable and frozen sets. +config = {"host": "localhost", "port": 8080} +for view in (config.keys(), config.items()): + for set_type in (set, frozenset): + schema = set_type(view) + check_view_comparisons(view, schema, (True, False, False, True, False, True)) + smaller_schema = schema - {next(iter(schema))} + check_view_comparisons( + view, smaller_schema, (False, True, False, False, True, True) + ) + larger_schema = schema | {("extra",)} + check_view_comparisons( + view, larger_schema, (False, True, True, True, False, False) + ) + check_view_comparisons( + view, set_type({("other",)}), (False, True, False, False, False, False) + ) + +# Mixed keys/items views compare their members regardless of insertion order. +indexed_settings = {("port", 8080): None, ("host", "localhost"): None} +check_view_comparisons( + config.items(), indexed_settings.keys(), (True, False, False, True, False, True) +) +del indexed_settings[("port", 8080)] +check_view_comparisons( + config.items(), indexed_settings.keys(), (False, True, False, False, True, True) +) + +# Testing for common settings does not require hashable values. +left_settings = {"ports": [80, 443], "hosts": ["localhost"]} +right_settings = {"ports": [80, 443], "timeout": 30} +assert not left_settings.items().isdisjoint(right_settings.items()) +assert not right_settings.items().isdisjoint(left_settings.items()) +assert not left_settings.items().isdisjoint([("ports", [80, 443])]) +assert left_settings.items().isdisjoint({"ports": [8080]}.items()) +assert left_settings.items().isdisjoint(()) +assert left_settings.items().isdisjoint({"timeout": 30}.items()) +assert left_settings.items() != frozenset() +assert left_settings.items() >= frozenset() +assert not left_settings.items() <= frozenset() +settings_view = left_settings.items() +assert not settings_view.isdisjoint(settings_view) +assert {}.items().isdisjoint({}.items()) + + +def settings_then_error(setting): + yield setting + raise ValueError("remaining settings unavailable") + + +# A match stops consumption, while an error before a match still propagates. +assert not config.keys().isdisjoint(settings_then_error("host")) +assert not left_settings.items().isdisjoint(settings_then_error(("ports", [80, 443]))) +with assert_raises(ValueError): + config.keys().isdisjoint(settings_then_error("missing")) +with assert_raises(TypeError): + config.keys().isdisjoint(42) + + +class ViewMembershipKey: + hash_calls = 0 + + def __hash__(self): + self.hash_calls += 1 + return 42 + + +# Successful item membership needs one dictionary lookup. +membership_key = ViewMembershipKey() +membership_dict = {membership_key: [1, 2]} +membership_key.hash_calls = 0 +assert (membership_key, [1, 2]) in membership_dict.items() +assert membership_key.hash_calls == 1 + +for set_type in (set, frozenset): + + class ViewSetSubclass(set_type): + events = [] + + def __iter__(self): + self.events.append("iter") + return super().__iter__() + + def __contains__(self, item): + self.events.append("contains") + return super().__contains__(item) + + # View operations preserve a set subclass's iteration and membership hooks. + schema = ViewSetSubclass({"host", "port"}) + schema.events.clear() + assert config.keys() == schema + assert schema.events == ["contains", "contains"] + schema.events.clear() + assert config.keys() >= schema + assert schema.events == ["iter"] + schema.events.clear() + assert not config.keys().isdisjoint(schema) + assert schema.events == ["iter"] + larger_schema = ViewSetSubclass({"host", "port", "timeout"}) + schema.events.clear() + assert not config.keys().isdisjoint(larger_schema) + assert schema.events == ["contains"] + +# An empty view must still consume arbitrary iterables and validate their keys. +with assert_raises(TypeError): + {}.keys().isdisjoint([[]]) +with assert_raises(ValueError): + {}.keys().isdisjoint(settings_then_error("missing")) + +for set_type in (set, frozenset): + + class ClearingLengthSet(set_type): + def __len__(self): + changing_config.clear() + return 1 + + def __iter__(self): + raise ValueError("settings unavailable") + + # The view's size is captured before calling the other operand's __len__. + changing_config = {"host": "localhost", "port": 8080} + with assert_raises(ValueError): + changing_config.keys().isdisjoint(ClearingLengthSet({"host"})) + assert changing_config == {} + + +class ItemLookupDict(dict): + def __getitem__(self, key): + raise AssertionError("item membership called __getitem__") + + def __missing__(self, key): + raise AssertionError("item membership called __missing__") + + +# Item views inspect dictionary storage without invoking subclass lookup hooks. +lookup_items = ItemLookupDict(ports=[80, 443]).items() +assert ("ports", [80, 443]) in lookup_items +assert ("ports", [8080]) not in lookup_items +assert ("missing", []) not in lookup_items