Fix dict view comparisons and avoid temporary sets (#8849)

Assisted-by: Codex:GPT-6

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::<PySet>().is_some()
+        || obj.downcast_ref::<PyFrozenSet>().is_some()
+        || obj.downcast_ref::<PyDictKeys>().is_some()
+        || obj.downcast_ref::<PyDictItems>().is_some()
+}
+
 // Set operations defined on set-like views of the dictionary.
 #[pyclass]
 trait ViewSetOps: DictView {
-    fn to_set(zelf: PyRef<Self>, vm: &VirtualMachine) -> PyResult<PySetInner> {
-        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<Self>, other: ArgIterable, vm: &VirtualMachine) -> PyResult<PySet> {
-        let zelf = Self::to_set(zelf, vm)?;
-        let inner = zelf.symmetric_difference(other, vm)?;
-        Ok(PySet { inner })
-    }
-
-    fn __and__(zelf: PyRef<Self>, other: ArgIterable, vm: &VirtualMachine) -> PyResult<PySet> {
-        let zelf = Self::to_set(zelf, vm)?;
-        let inner = zelf.intersection(other, vm)?;
-        Ok(PySet { inner })
-    }
-
-    fn __or__(zelf: PyRef<Self>, other: ArgIterable, vm: &VirtualMachine) -> PyResult<PySet> {
-        let zelf = Self::to_set(zelf, vm)?;
-        let inner = zelf.union(other, vm)?;
-        Ok(PySet { inner })
-    }
-
-    fn __sub__(zelf: PyRef<Self>, other: ArgIterable, vm: &VirtualMachine) -> PyResult<PySet> {
-        let zelf = Self::to_set(zelf, vm)?;
-        let inner = zelf.difference(other, vm)?;
-        Ok(PySet { inner })
-    }
-
-    fn __rsub__(zelf: PyRef<Self>, other: ArgIterable, vm: &VirtualMachine) -> PyResult<PySet> {
-        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<Self>,
         other: &PyObject,
         op: PyComparisonOp,
         vm: &VirtualMachine,
     ) -> PyResult<PyComparisonValue> {
-        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::<Self>() {
+            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::<PyObjectRef>::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<PyObjectRef> = zelf.as_object().to_owned().try_into_value(vm)?;
-        let rhs: Vec<PyObjectRef> = 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<Self>, object: ArgIterable, vm: &VirtualMachine) -> PyResult<bool> {
-        // 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::<PyObjectRef>::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

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