Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
80 changes: 76 additions & 4 deletions crates/vm/src/stdlib/_collections/ordered_dict.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ pub(crate) mod ordered_dict {
AsObject, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, TryFromObject,
VirtualMachine, atomic_func,
builtins::{
PyDict, PyGenericAlias, PyMappingProxy, PyTuple, PyTypeRef,
PyDict, PyGenericAlias, PyMappingProxy, PyStrRef, PyTuple, PyTypeRef,
dict::{
PyDictItems, set_inner_number_or, set_inner_number_subtract, set_inner_number_xor,
set_item_view_number_xor, set_view_number_and,
Expand All @@ -27,6 +27,7 @@ pub(crate) mod ordered_dict {
Initializer, IterNext, Iterable, PyComparisonOp, Representable, SelfIter,
},
};
use rustpython_common::wtf8::Wtf8Buf;
use std::collections::HashMap;

const ODICT_ITER_REVERSED: u8 = 1;
Expand Down Expand Up @@ -977,14 +978,37 @@ pub(crate) mod ordered_dict {
set_item_view_number_xor(a, b, is_item_view(a) && is_item_view(b), vm)
}

fn ordered_view_repr(
view: &PyObject,
od: &PyRef<PyOrderedDict>,
kind: u8,
name: &str,
vm: &VirtualMachine,
) -> PyResult<PyStrRef> {
let Some(_guard) = ReprGuard::enter(vm, view) else {
return Ok(vm.ctx.intern_str("...").to_owned());
};
// Snapshot the view before calling user-defined repr methods, which
// can mutate the underlying ordered dictionary.
let iterator = PyODictIter::new(od.clone(), kind).to_pyobject(vm);
let items = PyIter::try_from_object(vm, iterator)?
.iter::<PyObjectRef>(vm)?
.collect::<PyResult<Vec<_>>>()?;
let list_repr = vm.ctx.new_list(items).as_object().repr(vm)?;
let mut result = Wtf8Buf::from(format!("{name}("));
result.push_wtf8(list_repr.as_wtf8());
result.push_str(")");
Ok(vm.ctx.new_str(result))
}

#[pyattr]
#[pyclass(name = "odict_keys", module = "builtins", unhashable = true, traverse)]
#[derive(Debug, PyPayload)]
struct PyODictKeys {
od: PyRef<PyOrderedDict>,
}

#[pyclass(with(Iterable, Comparable, AsMapping, AsSequence, AsNumber))]
#[pyclass(with(Iterable, Comparable, AsMapping, AsSequence, AsNumber, Representable))]
impl Py<PyODictKeys> {
#[pymethod]
fn __reversed__(&self, vm: &VirtualMachine) -> PyObjectRef {
Expand All @@ -1007,6 +1031,22 @@ pub(crate) mod ordered_dict {
}
}

impl Representable for PyODictKeys {
fn repr(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyStrRef> {
ordered_view_repr(
zelf.as_object(),
&zelf.od,
ODICT_ITER_KEYS,
"odict_keys",
vm,
)
}

fn repr_str(_zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<String> {
unreachable!("use repr instead")
}
}

impl AsMapping for PyODictKeys {
fn as_mapping() -> &'static PyMappingMethods {
static AS_MAPPING: PyMappingMethods = PyMappingMethods {
Expand Down Expand Up @@ -1077,7 +1117,7 @@ pub(crate) mod ordered_dict {
od: PyRef<PyOrderedDict>,
}

#[pyclass(with(Iterable, Comparable, AsMapping))]
#[pyclass(with(Iterable, Comparable, AsMapping, Representable))]
impl Py<PyODictValues> {
#[pymethod]
fn __reversed__(&self, vm: &VirtualMachine) -> PyObjectRef {
Expand All @@ -1091,6 +1131,22 @@ pub(crate) mod ordered_dict {
}
}

impl Representable for PyODictValues {
fn repr(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyStrRef> {
ordered_view_repr(
zelf.as_object(),
&zelf.od,
ODICT_ITER_VALUES,
"odict_values",
vm,
)
}

fn repr_str(_zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<String> {
unreachable!("use repr instead")
}
}

impl AsMapping for PyODictValues {
fn as_mapping() -> &'static PyMappingMethods {
static AS_MAPPING: PyMappingMethods = PyMappingMethods {
Expand Down Expand Up @@ -1128,7 +1184,7 @@ pub(crate) mod ordered_dict {
od: PyRef<PyOrderedDict>,
}

#[pyclass(with(Iterable, Comparable, AsMapping, AsSequence, AsNumber))]
#[pyclass(with(Iterable, Comparable, AsMapping, AsSequence, AsNumber, Representable))]
impl Py<PyOrderedDictItems> {
#[pymethod]
fn __reversed__(&self, vm: &VirtualMachine) -> PyObjectRef {
Expand Down Expand Up @@ -1163,6 +1219,22 @@ pub(crate) mod ordered_dict {
}
}

impl Representable for PyOrderedDictItems {
fn repr(zelf: &Py<Self>, vm: &VirtualMachine) -> PyResult<PyStrRef> {
ordered_view_repr(
zelf.as_object(),
&zelf.od,
ODICT_ITER_ITEMS,
"odict_items",
vm,
)
}

fn repr_str(_zelf: &Py<Self>, _vm: &VirtualMachine) -> PyResult<String> {
unreachable!("use repr instead")
}
}

impl AsMapping for PyOrderedDictItems {
fn as_mapping() -> &'static PyMappingMethods {
static AS_MAPPING: PyMappingMethods = PyMappingMethods {
Expand Down
54 changes: 54 additions & 0 deletions extra_tests/snippets/stdlib_collections.py
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,60 @@ def __repr__(self):
ordered = OrderedDict()
_count_elements(ordered, "bab")
assert list(ordered.items()) == [("b", 2), ("a", 1)]


# OrderedDict views show their contents in insertion order, including recursion.
def check_ordered_dict_view_repr():

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Remove the prohibited snippet test changes

This block adds a new test function and multiple assertions to extra_tests/snippets/stdlib_collections.py, but the repository's test-code policy explicitly disallows changes to test assertions or logic and says the only permitted test-file edits are expected-failure decorator changes. Revert this added test block before merging.

AGENTS.md reference: AGENTS.md:L275-L279

Useful? React with 👍 / 👎.

ordered = OrderedDict([("b", 2), ("a", 1)])
for method, contents in (
("keys", "['b', 'a']"),
("values", "[2, 1]"),
("items", "[('b', 2), ('a', 1)]"),
):
assert repr(getattr(OrderedDict(), method)()) == "odict_" + method + "([])"
assert (
repr(getattr(ordered, method)()) == "odict_" + method + "(" + contents + ")"
)

recursive = OrderedDict()
values = recursive.values()
items = recursive.items()
recursive["self"] = values
assert repr(values) == "odict_values([...])"
recursive["self"] = items
assert repr(items) == "odict_items([('self', ...)])"

# Repr must snapshot the contents before arbitrary repr code mutates them.
for method in ("keys", "values", "items"):

class MutatingRepr:
def __repr__(self):
ordered.clear()
return "mutated"

item = MutatingRepr()
ordered = OrderedDict([(item, item), ("tail", "last")])
expected = {
"keys": "odict_keys([mutated, 'tail'])",
"values": "odict_values([mutated, 'last'])",
"items": "odict_items([(mutated, mutated), ('tail', 'last')])",
}
assert repr(getattr(ordered, method)()) == expected[method]
assert not ordered

class BadRepr:
def __repr__(self):
raise ValueError("view repr")

ordered = OrderedDict([(BadRepr(), BadRepr())])
for method in ("keys", "values", "items"):
with assert_raises(ValueError) as caught:
repr(getattr(ordered, method)())
assert str(caught.exception) == "view repr"


check_ordered_dict_view_repr()

factory = defaultdict(lambda: 100)
_count_elements(factory, "a")
assert factory == {"a": 1}
Expand Down
Loading