Skip to content

Fix dict view comparisons and avoid temporary sets #8849

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
youknowone merged 1 commit into from
Sep 27, 2026
Merged

Fix dict view comparisons and avoid temporary sets #8849

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 65 additions & 76 deletions crates/vm/src/builtins/dict.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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,
Expand Down Expand Up @@ -1490,87 +1488,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,
);
}
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);
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));
}
});
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)
}
}

Expand Down Expand Up @@ -1688,11 +1678,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
Expand Down
160 changes: 160 additions & 0 deletions extra_tests/snippets/builtin_dict.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,3 +408,163 @@ def test_func(**kwargs):
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
Loading