Add collections module#535
Conversation
Introduce an importable `collections` module and implement `collections.deque`, the first of the commonly-used container datatypes. - New `Deque` heap type (`VecDeque`-backed) with maxlen support, threaded through the heap/GC/repr/eq/iteration/host-boundary dispatch. - `collections` module registration (StandardLib, StaticStrings, module attrs). - deque exposed as `collections.deque`; supports append/appendleft/pop/popleft/ extend/extendleft/insert/remove/clear/copy/count/index/reverse/rotate, integer indexing, iteration (with mutation detection), membership, ordering, and CPython-matching error messages. - Consolidated test_cases coverage and limitations/collections.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015FUUYYxP68sbHXsdr2qGE8
Implement the two dict-like collections members via a single new heap variant `DictSubclass` that wraps a backing `dict` (referenced by id) plus per-kind state, keeping central-dispatch churn low. - `defaultdict`: default_factory attribute, missing-key insertion via builtin factories (int/list/set/dict/...), dict-compatible construction and methods, copy preserving the subclass. - `Counter`: tallying construction from iterables/mappings/kwargs, `c[missing]` returning 0, most_common, elements, update/subtract, and the +/-/&/| arithmetic operators (wired through binary.rs), count-descending repr. - Backing-dict-by-id design lets views (keys/values/items) reference the real dict and lets __getitem__ mutate on a defaultdict miss. - dict()/dict.update() now treat a dict subclass as a mapping; membership, iteration, ordering, and host-boundary conversion handle the new variant. - Consolidated test_cases for both types and limitations/collections.md. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015FUUYYxP68sbHXsdr2qGE8
Implement `collections.namedtuple(...)` as a module function returning a callable class (`NamedTupleClass` heap type) that constructs `NamedTuple` instances, and narrow the type checker's `collections` surface to the implemented members. - namedtuple factory: field-name parsing (string or iterable), CPython-matching identifier/keyword/underscore/duplicate validation, rename=True, defaults, and an ignored module kwarg. - NamedTupleClass: callable (via call_heap_callable), _fields/_field_defaults/ _make/__name__ class surface, `<class '__main__.Name'>` repr. - NamedTuple instances gain _fields/_asdict/_replace/_make plus inherited count/index; sequence unpacking now handles namedtuple and deque; namedtuple is a subclass of tuple for isinstance. type() identity is a documented divergence. - Narrowed collections stub (custom/collections/__init__.pyi + vendored) exposes only deque/defaultdict/Counter/namedtuple so type-checking matches runtime; update.py now copies custom package overrides. - Consolidated namedtuple + import test_cases; limitations updated. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015FUUYYxP68sbHXsdr2qGE8
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
15 issues found across 31 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/monty/src/types/dict.rs">
<violation number="1" location="crates/monty/src/types/dict.rs:1093">
P3: The `dict()` initialization documentation is now stale: this branch makes `DictSubclass` sources use mapping-copy semantics, but the nearby comment still says only real `dict` values do. Consider updating that comment so future changes don't reintroduce iterable-of-pairs assumptions for `defaultdict`/`Counter`.</violation>
</file>
<file name="limitations/namedtuple.md">
<violation number="1" location="limitations/namedtuple.md:15">
P2: `limitations/` files must only document divergences from CPython, never matching behavior. The new entries under "## Supported operations" describe features that match CPython (isinstance is True, named-tuple methods, sequence unpacking, etc.). Move these to a `collections.md` file (if one is being created for the new module) or remove them entirely — this file should only document what differs.</violation>
</file>
<file name="crates/monty-typeshed/update.py">
<violation number="1" location="crates/monty-typeshed/update.py:344">
P2: Package overrides can fail at runtime — `shutil.copy2(file, dest_pkg / file.name)` raises `FileNotFoundError` if `dest_pkg` doesn't already exist, because `copy2` does not create parent directories. Add `dest_pkg.mkdir(parents=True, exist_ok=True)` before the inner copy loop so the override works for any package name, not just those pre-created by COPY_FILES.</violation>
</file>
<file name="crates/monty/src/types/deque.rs">
<violation number="1" location="crates/monty/src/types/deque.rs:114">
P2: Large `maxlen` can crash the runtime on platforms where `usize` is smaller than `i64`, because conversion uses `expect(...)` in user-facing argument parsing. Returning a Python exception for conversion overflow would keep behavior safe and consistent with other numeric conversion paths.</violation>
</file>
<file name="crates/monty/src/types/iter.rs">
<violation number="1" location="crates/monty/src/types/iter.rs:430">
P2: Deque iteration mutation checks currently miss non-size mutations, so iterators can continue after the deque was mutated in-place. This comes from gating the error on length inequality only, which does not cover rotate/reordering mutations.</violation>
<violation number="2" location="crates/monty/src/types/iter.rs:676">
P3: The fallback to `0` masks an invalid DictSubclass backing-dict invariant and can silently turn iteration into immediate exhaustion. Using an unreachable/assert path here would fail fast like the rest of DictSubclass accessors.</violation>
</file>
<file name="limitations/collections.md">
<violation number="1" location="limitations/collections.md:31">
P2: This sentence says the repr `str(type(x))` matches CPython behavior. Per project rules, this file must only document divergences from CPython, never matching behavior.</violation>
</file>
<file name="crates/monty/src/value.rs">
<violation number="1" location="crates/monty/src/value.rs:268">
P2: Deque ordering can produce stale results or panic when an element comparison mutates either operand. The new `py_cmp` dispatch should be paired with mutation detection in `Deque::py_cmp`, similar to deque iteration’s `RuntimeError: deque mutated during iteration` handling.</violation>
<violation number="2" location="crates/monty/src/value.rs:1722">
P1: Deque membership can panic or continue with stale indexes when `__eq__` mutates the deque during `x in d`. Checking the current deque length before each indexed clone would preserve CPython’s `RuntimeError: deque mutated during iteration` behavior and avoid an out-of-bounds access.</violation>
</file>
<file name="crates/monty/src/types/dict_subclass.rs">
<violation number="1" location="crates/monty/src/types/dict_subclass.rs:693">
P1: Large Counter updates can produce incorrect counts (or panic under overflow checks) when an existing count and delta exceed i64 range. The direct `current + delta` in `counter_add_one` should use checked arithmetic and raise an overflow error instead of overflowing silently.</violation>
</file>
<file name="crates/monty/src/types/namedtuple_class.rs">
<violation number="1" location="crates/monty/src/types/namedtuple_class.rs:272">
P3: Identical private helper `py_repr_str` is defined in both `namedtuple_class.rs` and `namedtuple.rs`. Both files contain the exact same `fn py_repr_str(s: &str) -> String { format!("'{s}'") }` module-private function. Consider extracting it to a shared location (e.g., a utility module or re-exporting from one file to the other) to avoid the duplication — any future change to the repr format would need to be applied in both places.</violation>
</file>
<file name="crates/monty/src/types/type.rs">
<violation number="1" location="crates/monty/src/types/type.rs:295">
P1: `Type::is_instance_of` is missing subclass relationships for `collections.defaultdict` and `collections.Counter` as subclasses of `dict`. In CPython, both `defaultdict` and `Counter` are subclasses of `dict`, meaning `isinstance(defaultdict(), dict)` returns `True`. Without adding these entries, `isinstance` and issubclass checks will return `False` for these types vs `dict`, breaking code that type-checks for dict-like behavior. Add `} else if (self == Self::DefaultDict || self == Self::Counter) && other == Self::Dict { true }` to the method, consistent with the existing `NamedTuple` → `Tuple` relationship pattern on line 293-294.</violation>
</file>
<file name="crates/monty-typeshed/custom/collections/__init__.pyi">
<violation number="1" location="crates/monty-typeshed/custom/collections/__init__.pyi:86">
P2: The `deque` type stub includes `__reduce__`, but the runtime does not implement it (the limitations doc explicitly says "No `__copy__` / pickling / `__reduce__`"). Type checkers will consider it valid, leading to false positives. Consider removing it from the stub.</violation>
<violation number="2" location="crates/monty-typeshed/custom/collections/__init__.pyi:87">
P2: The `deque` type stub advertises `__add__`, `__mul__`, `__iadd__`, and `__imul__` methods, but the runtime raises `TypeError` for all four operations. The stub's own comment states it only exposes what the runtime implements, so these should be removed from the stub to keep type-checking in sync with runtime behavior (or added to the runtime if that's the intent).</violation>
<violation number="3" location="crates/monty-typeshed/custom/collections/__init__.pyi:143">
P2: The `Counter` type stub includes `__iadd__`, `__isub__`, `__iand__`, and `__ior__` methods, but the runtime does not support in-place operators between Counters (as documented in limitations). Type checkers will consider these valid, but using them will produce a runtime error. Consider removing them from the stub.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| HeapReadOutput::Deque(deque) => { | ||
| let len = deque.get(vm.heap).as_deque().len(); | ||
| for i in 0..len { | ||
| let el = deque.clone_item(i, vm); |
There was a problem hiding this comment.
P1: Deque membership can panic or continue with stale indexes when __eq__ mutates the deque during x in d. Checking the current deque length before each indexed clone would preserve CPython’s RuntimeError: deque mutated during iteration behavior and avoid an out-of-bounds access.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/value.rs, line 1722:
<comment>Deque membership can panic or continue with stale indexes when `__eq__` mutates the deque during `x in d`. Checking the current deque length before each indexed clone would preserve CPython’s `RuntimeError: deque mutated during iteration` behavior and avoid an out-of-bounds access.</comment>
<file context>
@@ -1715,7 +1716,28 @@ impl Value {
+ HeapReadOutput::Deque(deque) => {
+ let len = deque.get(vm.heap).as_deque().len();
+ for i in 0..len {
+ let el = deque.clone_item(i, vm);
+ let eq = item.py_eq(&el, vm);
+ el.drop_with_heap(vm);
</file context>
| } | ||
| None => 0, | ||
| }; | ||
| if let Some(old) = dict.set(key, Value::Int(current + delta), vm)? { |
There was a problem hiding this comment.
P1: Large Counter updates can produce incorrect counts (or panic under overflow checks) when an existing count and delta exceed i64 range. The direct current + delta in counter_add_one should use checked arithmetic and raise an overflow error instead of overflowing silently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/types/dict_subclass.rs, line 693:
<comment>Large Counter updates can produce incorrect counts (or panic under overflow checks) when an existing count and delta exceed i64 range. The direct `current + delta` in `counter_add_one` should use checked arithmetic and raise an overflow error instead of overflowing silently.</comment>
<file context>
@@ -0,0 +1,859 @@
+ }
+ None => 0,
+ };
+ if let Some(old) = dict.set(key, Value::Int(current + delta), vm)? {
+ old.drop_with_heap(vm);
+ }
</file context>
| true | ||
| } else if self == Self::NamedTuple && other == Self::Tuple { | ||
| // a namedtuple is a subclass of tuple in Python | ||
| true |
There was a problem hiding this comment.
P1: Type::is_instance_of is missing subclass relationships for collections.defaultdict and collections.Counter as subclasses of dict. In CPython, both defaultdict and Counter are subclasses of dict, meaning isinstance(defaultdict(), dict) returns True. Without adding these entries, isinstance and issubclass checks will return False for these types vs dict, breaking code that type-checks for dict-like behavior. Add } else if (self == Self::DefaultDict || self == Self::Counter) && other == Self::Dict { true } to the method, consistent with the existing NamedTuple → Tuple relationship pattern on line 293-294.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/types/type.rs, line 295:
<comment>`Type::is_instance_of` is missing subclass relationships for `collections.defaultdict` and `collections.Counter` as subclasses of `dict`. In CPython, both `defaultdict` and `Counter` are subclasses of `dict`, meaning `isinstance(defaultdict(), dict)` returns `True`. Without adding these entries, `isinstance` and issubclass checks will return `False` for these types vs `dict`, breaking code that type-checks for dict-like behavior. Add `} else if (self == Self::DefaultDict || self == Self::Counter) && other == Self::Dict { true }` to the method, consistent with the existing `NamedTuple` → `Tuple` relationship pattern on line 293-294.</comment>
<file context>
@@ -281,6 +290,9 @@ impl Type {
true
+ } else if self == Self::NamedTuple && other == Self::Tuple {
+ // a namedtuple is a subclass of tuple in Python
+ true
} else {
false
</file context>
|
|
||
| ## Supported operations | ||
|
|
||
| - Construction via a `collections.namedtuple` class (positional, keyword, and |
There was a problem hiding this comment.
P2: limitations/ files must only document divergences from CPython, never matching behavior. The new entries under "## Supported operations" describe features that match CPython (isinstance is True, named-tuple methods, sequence unpacking, etc.). Move these to a collections.md file (if one is being created for the new module) or remove them entirely — this file should only document what differs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At limitations/namedtuple.md, line 15:
<comment>`limitations/` files must only document divergences from CPython, never matching behavior. The new entries under "## Supported operations" describe features that match CPython (isinstance is True, named-tuple methods, sequence unpacking, etc.). Move these to a `collections.md` file (if one is being created for the new module) or remove them entirely — this file should only document what differs.</comment>
<file context>
@@ -1,38 +1,42 @@
## Supported operations
+- Construction via a `collections.namedtuple` class (positional, keyword, and
+ trailing defaults).
- Indexing by integer: `nt[0]`. `IndexError` on out-of-range.
</file context>
| for pkg_dir in CUSTOM_DIR.iterdir(): | ||
| if not pkg_dir.is_dir(): | ||
| continue | ||
| dest_pkg = STDLIB_DIR / pkg_dir.name |
There was a problem hiding this comment.
P2: Package overrides can fail at runtime — shutil.copy2(file, dest_pkg / file.name) raises FileNotFoundError if dest_pkg doesn't already exist, because copy2 does not create parent directories. Add dest_pkg.mkdir(parents=True, exist_ok=True) before the inner copy loop so the override works for any package name, not just those pre-created by COPY_FILES.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty-typeshed/update.py, line 344:
<comment>Package overrides can fail at runtime — `shutil.copy2(file, dest_pkg / file.name)` raises `FileNotFoundError` if `dest_pkg` doesn't already exist, because `copy2` does not create parent directories. Add `dest_pkg.mkdir(parents=True, exist_ok=True)` before the inner copy loop so the override works for any package name, not just those pre-created by COPY_FILES.</comment>
<file context>
@@ -334,6 +334,17 @@ def main() -> int:
+ for pkg_dir in CUSTOM_DIR.iterdir():
+ if not pkg_dir.is_dir():
+ continue
+ dest_pkg = STDLIB_DIR / pkg_dir.name
+ for file in pkg_dir.glob('*.pyi'):
+ shutil.copy2(file, dest_pkg / file.name)
</file context>
| def __setitem__(self, key: SupportsIndex, value: _T, /) -> None: ... # type: ignore[override] | ||
| def __delitem__(self, key: SupportsIndex, /) -> None: ... # type: ignore[override] | ||
| def __contains__(self, key: object, /) -> bool: ... | ||
| def __reduce__(self) -> tuple[type[Self], tuple[()], None, Iterator[_T]]: ... |
There was a problem hiding this comment.
P2: The deque type stub includes __reduce__, but the runtime does not implement it (the limitations doc explicitly says "No __copy__ / pickling / __reduce__"). Type checkers will consider it valid, leading to false positives. Consider removing it from the stub.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty-typeshed/custom/collections/__init__.pyi, line 86:
<comment>The `deque` type stub includes `__reduce__`, but the runtime does not implement it (the limitations doc explicitly says "No `__copy__` / pickling / `__reduce__`"). Type checkers will consider it valid, leading to false positives. Consider removing it from the stub.</comment>
<file context>
@@ -0,0 +1,201 @@
+ def __setitem__(self, key: SupportsIndex, value: _T, /) -> None: ... # type: ignore[override]
+ def __delitem__(self, key: SupportsIndex, /) -> None: ... # type: ignore[override]
+ def __contains__(self, key: object, /) -> bool: ...
+ def __reduce__(self) -> tuple[type[Self], tuple[()], None, Iterator[_T]]: ...
+ def __iadd__(self, value: Iterable[_T], /) -> Self: ...
+ def __add__(self, value: Self, /) -> Self: ...
</file context>
| def __delitem__(self, key: SupportsIndex, /) -> None: ... # type: ignore[override] | ||
| def __contains__(self, key: object, /) -> bool: ... | ||
| def __reduce__(self) -> tuple[type[Self], tuple[()], None, Iterator[_T]]: ... | ||
| def __iadd__(self, value: Iterable[_T], /) -> Self: ... |
There was a problem hiding this comment.
P2: The deque type stub advertises __add__, __mul__, __iadd__, and __imul__ methods, but the runtime raises TypeError for all four operations. The stub's own comment states it only exposes what the runtime implements, so these should be removed from the stub to keep type-checking in sync with runtime behavior (or added to the runtime if that's the intent).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty-typeshed/custom/collections/__init__.pyi, line 87:
<comment>The `deque` type stub advertises `__add__`, `__mul__`, `__iadd__`, and `__imul__` methods, but the runtime raises `TypeError` for all four operations. The stub's own comment states it only exposes what the runtime implements, so these should be removed from the stub to keep type-checking in sync with runtime behavior (or added to the runtime if that's the intent).</comment>
<file context>
@@ -0,0 +1,201 @@
+ def __delitem__(self, key: SupportsIndex, /) -> None: ... # type: ignore[override]
+ def __contains__(self, key: object, /) -> bool: ...
+ def __reduce__(self) -> tuple[type[Self], tuple[()], None, Iterator[_T]]: ...
+ def __iadd__(self, value: Iterable[_T], /) -> Self: ...
+ def __add__(self, value: Self, /) -> Self: ...
+ def __mul__(self, value: int, /) -> Self: ...
</file context>
| let mapping_id = match other_value { | ||
| Value::Ref(id) => match vm.heap.get(*id) { | ||
| HeapData::Dict(_) => Some(*id), | ||
| HeapData::DictSubclass(sub) => Some(sub.dict_id()), |
There was a problem hiding this comment.
P3: The dict() initialization documentation is now stale: this branch makes DictSubclass sources use mapping-copy semantics, but the nearby comment still says only real dict values do. Consider updating that comment so future changes don't reintroduce iterable-of-pairs assumptions for defaultdict/Counter.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/types/dict.rs, line 1093:
<comment>The `dict()` initialization documentation is now stale: this branch makes `DictSubclass` sources use mapping-copy semantics, but the nearby comment still says only real `dict` values do. Consider updating that comment so future changes don't reintroduce iterable-of-pairs assumptions for `defaultdict`/`Counter`.</comment>
<file context>
@@ -1077,13 +1077,28 @@ struct DictUpdateArgs {
+ let mapping_id = match other_value {
+ Value::Ref(id) => match vm.heap.get(*id) {
+ HeapData::Dict(_) => Some(*id),
+ HeapData::DictSubclass(sub) => Some(sub.dict_id()),
+ _ => None,
+ },
</file context>
| let dict_id = sub.dict_id(); | ||
| let len = match heap.get(dict_id) { | ||
| HeapData::Dict(dict) => dict.len(), | ||
| _ => 0, |
There was a problem hiding this comment.
P3: The fallback to 0 masks an invalid DictSubclass backing-dict invariant and can silently turn iteration into immediate exhaustion. Using an unreachable/assert path here would fail fast like the rest of DictSubclass accessors.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/types/iter.rs, line 676:
<comment>The fallback to `0` masks an invalid DictSubclass backing-dict invariant and can silently turn iteration into immediate exhaustion. Using an unreachable/assert path here would fail fast like the rest of DictSubclass accessors.</comment>
<file context>
@@ -649,6 +661,26 @@ impl IterValue {
+ let dict_id = sub.dict_id();
+ let len = match heap.get(dict_id) {
+ HeapData::Dict(dict) => dict.len(),
+ _ => 0,
+ };
+ Some(Self::HeapRef {
</file context>
| _ => 0, | |
| _ => unreachable!("DictSubclass.dict_id must reference a Dict"), |
| } | ||
|
|
||
| /// Renders a Python `repr()` of a string (single-quoted) for error messages. | ||
| fn py_repr_str(s: &str) -> String { |
There was a problem hiding this comment.
P3: Identical private helper py_repr_str is defined in both namedtuple_class.rs and namedtuple.rs. Both files contain the exact same fn py_repr_str(s: &str) -> String { format!("'{s}'") } module-private function. Consider extracting it to a shared location (e.g., a utility module or re-exporting from one file to the other) to avoid the duplication — any future change to the repr format would need to be applied in both places.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At crates/monty/src/types/namedtuple_class.rs, line 272:
<comment>Identical private helper `py_repr_str` is defined in both `namedtuple_class.rs` and `namedtuple.rs`. Both files contain the exact same `fn py_repr_str(s: &str) -> String { format!("'{s}'") }` module-private function. Consider extracting it to a shared location (e.g., a utility module or re-exporting from one file to the other) to avoid the duplication — any future change to the repr format would need to be applied in both places.</comment>
<file context>
@@ -0,0 +1,563 @@
+}
+
+/// Renders a Python `repr()` of a string (single-quoted) for error messages.
+fn py_repr_str(s: &str) -> String {
+ format!("'{s}'")
+}
</file context>
|
@dbreunig this is claude's first pass, not very good apparently. I'll fix it up, but anything else you particularly need from |
|
No, the proposed API here looks good. It is a common stumbling blog when an RLM engages in map-reduce patterns. |
Introduce an importable
collectionsmodule and implementcollections.deque,the first of the commonly-used container datatypes.
Dequeheap type (VecDeque-backed) with maxlen support, threadedthrough the heap/GC/repr/eq/iteration/host-boundary dispatch.
collectionsmodule registration (StandardLib, StaticStrings, module attrs).collections.deque; supports append/appendleft/pop/popleft/extend/extendleft/insert/remove/clear/copy/count/index/reverse/rotate,
integer indexing, iteration (with mutation detection), membership, ordering,
and CPython-matching error messages.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_015FUUYYxP68sbHXsdr2qGE8
Summary by cubic
Adds an importable
collectionsmodule withdeque,defaultdict,Counter, andnamedtuple. Includes new heap/VM support (construction, iteration, binary ops) and a narrowedcollectionsstub so type checking matches runtime.New Features
deque: fast appends/pops on both ends, optionalmaxlen, indexing, and iteration with mutation detection.defaultdict:default_factorywith missing-key insertion; behaves likedict;copy()preserves the subclass.Counter: counting from iterables/mappings/kwargs;most_common,elements,update,subtract; supports+,-,&,|; missing keys read as 0.namedtuple: factory returns a callable class (_fields,_field_defaults,_make); instances support_asdict,_replace; works with sequence unpacking.dequeandnamedtuple;dict()/dict.update()accept dict subclasses; deque mutation during iteration raisesRuntimeError; binary ops routed forCounter.Migration
deque,defaultdict,Counter, andnamedtupleare available fromcollections; other names (e.g.OrderedDict,ChainMap,UserDict) will fail at type-check and runtime.collections.abcis not importable at runtime (use only in annotations).namedtuplefromcollectionsif you referenced a barenamedtuple.Written for commit 6d7d66b. Summary will update on new commits.