Implement cell comparison and repr - #8458
Open
name-of-okja wants to merge 1 commit into
Open
Conversation
Contributor
📦 Library DependenciesThe following Lib/ modules were modified. Here are their dependencies: [x] lib: cpython/Lib/reprlib.py dependencies:
dependent tests: (245 tests)
[x] test: cpython/Lib/test/test_funcattrs.py (TODO: 1) dependencies: dependent tests: (no tests depend on funcattrs) Legend:
|
Contributor
📝 WalkthroughWalkthrough
ChangesPyCell behavior
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/vm/src/builtins/function.rs`:
- Around line 1585-1588: Update the PyCell formatting branch in the function
containing the Some(value) match to truncate value.class().slot_name() to at
most 80 Unicode characters before passing it to format!, while preserving the
existing repr structure and address formatting; add or update a regression test
if the surrounding test suite covers cell representations.
- Line 1574: Update the Some/Some branch of Comparable’s rich-comparison
handling to return the PyObjectRef produced by rich_compare directly instead of
coercing it with is_true(vm). Preserve special handling that converts only
Boolean or NotImplemented results, while leaving empty-cell ordering unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8e491cd7-5a34-4657-a35c-6c2e76fcec4d
⛔ Files ignored due to path filters (2)
Lib/test/test_funcattrs.pyis excluded by!Lib/**Lib/test/test_reprlib.pyis excluded by!Lib/**
📒 Files selected for processing (1)
crates/vm/src/builtins/function.rs
There was a problem hiding this comment.
Pull request overview
Adds CPython-parity behavior for cell objects by implementing content-based comparison and a CPython-like repr, and updates stdlib tests that were previously marked as expected failures for RustPython.
Changes:
- Make
cellunhashable to preserve the equality/hash invariant after switching to content-based equality. - Add a
cellrepr that includes the cell address and (when non-empty) the contained object’s type and address. - Un-xfail CPython tests that now pass (
test_funcattrs.CellTest.test_comparison,test_reprlib.test_cell).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| Lib/test/test_reprlib.py | Removes expectedFailure marker for cell repr test now that cell.__repr__ is implemented. |
| Lib/test/test_funcattrs.py | Removes expectedFailure marker for cell comparison test now that cell rich-compare is implemented. |
| crates/vm/src/builtins/function.rs | Implements cell comparison + repr and marks cell as unhashable. |
Suppressed comments (1)
crates/vm/src/builtins/function.rs:1576
- In the full-cell case,
a.rich_compare(b, op, vm)?.is_true(vm)coerces the rich-compare result tobool, which is not what CPython does for cells (it returns the rawPyObject_RichCompareresult). This also means a non-bool__eq__result will be truth-tested (invoking__bool__/__len__) and can raise where CPython would not. Implementtp_richcomparedirectly and returnEither::A(...)for the full-vs-full branch, while keeping the empty-ordering behavior as a boolean.
impl Comparable for PyCell {
fn cmp(
zelf: &Py<Self>,
other: &PyObject,
op: PyComparisonOp,
vm: &VirtualMachine,
) -> PyResult<PyComparisonValue> {
let other = class_or_notimplemented!(Self, other);
// compare cells by contents; empty cells come before anything else
match (zelf.get(), other.get()) {
(Some(a), Some(b)) => a.rich_compare(b, op, vm)?.is_true(vm).map(Into::into),
(a, b) => Ok(op.eval_ord(b.is_none().cmp(&a.is_none())).into()),
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| #[pyclass(with(Constructor))] | ||
| #[pyclass(with(Constructor, Comparable, Representable))] |
Contributor
Author
There was a problem hiding this comment.
Correct on both counts, and done in 962ad63 — dropped Comparable and gave PyCell its own #[pyslot] slot_richcompare, returning Either::A for full-vs-full and Either::B only for the empty-cell ordering, where a bool is the right answer.
The __bool__ side effect you flagged is the sharpest part of this. With an __eq__ that returns an object whose __bool__ raises, CPython hands back the object, while the coerced version raised a RuntimeError that CPython never produces. Non-bool results were flattened too: 'I am not a bool' became True.
PyBaseObject was a useful precedent for filling the slot by hand rather than through the trait.
The cell type filled neither the richcompare nor the repr slot, so object's address-based defaults showed through: cell(1) == cell(1) was False, ordering raised TypeError, and repr rendered <cell object at 0x...> rather than <cell at 0x...: int object at 0x...>. Compare cells by contents, with empty cells ordering before everything else, and render CPython's repr for both the filled and empty cases. The comparison fills the richcompare slot directly rather than going through Comparable, whose cmp() can only answer with a bool. CPython returns whatever PyObject_RichCompare produced, so a contained __eq__ that yields a non-bool must pass through untouched; coercing it would also call __bool__ and surface exceptions CPython never raises. The empty-cell branch still answers with a bool, so both arms are needed and the slot returns Either. The repr truncates the contained type name the way "%.80s" does: at most 80 bytes, dropping a character the cut would leave incomplete. Mark the type unhashable. Content-based equality combined with the inherited identity hash would break the hash/eq contract, and hashing the contents is not possible either because cell_contents is writable. CPython gets this implicitly, because defining tp_richcompare suppresses tp_hash inheritance. Reference: CPython Objects/cellobject.c, cell_richcompare and cell_repr. Assisted-by: Claude Code:claude-opus-5
name-of-okja
force-pushed
the
fix/cell-repr-and-comparison
branch
from
August 7, 2026 10:49
bba2b7f to
962ad63
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (1)
crates/vm/src/builtins/function.rs:1600
- The PR description explicitly says the
%.80struncation of the contained value’s type name is not reproduced, but the implementation here does truncate to 80 bytes. This is a behavioral discrepancy (and adds extra complexity) relative to what the PR claims.
Either update the PR description to match the intended CPython-faithful behavior, or (if the goal is to avoid truncation) drop the truncation logic and render the full type_name.
// CPython renders the type name with "%.80s", which reads at
// most 80 bytes and drops a character left incomplete by the cut.
let mut end = type_name.len().min(80);
while !type_name.is_char_boundary(end) {
end -= 1;
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PyCellfilled neither the richcompare nor the repr slot, soobject'saddress-based defaults showed through. Cells were compared as boxes rather than
by what they hold:
The box is an implementation detail, so CPython compares the contents instead
and renders both addresses in the repr. This ports the two missing slots.
Comparison follows
cell_richcompare, which delegates tocell_compare_impl:https://github.com/python/cpython/blob/v3.14.0/Objects/cellobject.c#L85-L115
Contents are compared when both cells are full; otherwise the emptiness flags
are compared, which is how CPython gets "empty cells come before anything
else".
Two details of that delegation are load-bearing:
The slot is filled directly rather than through
Comparable, whosecmpcan only answer with a bool. CPython returns whatever
PyObject_RichCompareproduced, so a contained
__eq__yielding a non-bool has to pass throughuntouched — and coercing it would additionally call
__bool__, raisingexceptions CPython never raises. The empty-cell branch still answers with a
bool, so the slot returns
Either.PyObject_RichCompareis used rather thanPyObject_RichCompareBool,matching CPython. The latter short-circuits
Eqon identity, which isobservable:
Repr follows
cell_repr, including the empty case and the%.80sbound onthe contained type name — at most 80 bytes, dropping a character the cut would
leave incomplete:
https://github.com/python/cpython/blob/v3.14.0/Objects/cellobject.c#L117-L128
The type also becomes unhashable. This is not cosmetic: content-based
equality combined with the inherited identity hash would break the invariant
that objects comparing equal hash equally, so
{cell(1): v}[cell(1)]wouldraise
KeyError. Hashing the contents is not an option either, becausecell_contentsis writable and a key's hash has to stay put:https://github.com/python/cpython/blob/v3.14.0/Objects/cellobject.c#L158-L170
CPython arrives at the same place implicitly —
cellleavestp_hashemptywhile filling
tp_richcompare, and the two slots are inherited as a pair:https://github.com/python/cpython/blob/v3.14.0/Objects/typeobject.c#L8261-L8276
listreaches it explicitly withPyObject_HashNotImplemented, which is theshape
unhashable = truealready has in RustPython.Test Plan
@unittest.expectedFailurefrom the two tests that now pass:test_funcattrs.CellTest.test_comparisonandtest_reprlib.test_cell.behaviour — repr (filled, empty, constructed directly), all six comparison
operators, empty-vs-filled ordering in both directions, non-cell operands,
the
nanidentity case, hashing, and mutation throughcell_contents.Output is identical once addresses are normalised.
__eq__/__lt__returning a non-bool or an object whose
__bool__raises, and type names of79/80/81 ASCII bytes plus 100 each of 2-, 3- and 4-byte characters. The 3-byte
case is the one that pins the bound to bytes rather than characters — CPython
renders 26 characters (78 bytes) there, since a 27th would reach 81.
test_funcattrsandtest_reprlibpass with no unexpected successes.serialization (
test_pickle,test_copy,test_marshal), framereconstruction (
test_inspect,test_pdb,test_annotationlib), cyclecollection (
test_gc,test_weakref), plustest_richcmp,test_sys,test_code,test_scope,test_types,test_descr,test_class,test_super,test_builtin,test_functools,test_typing,test_dataclasses,test_generators,test_doctestand others.cargo test -p rustpython-vm -p rustpython-common,cargo fmt --checkandcargo clippy --all-targetsare clean.One gap worth naming: no test in the CPython suite covers
cellbeingunhashable, so that part rests on the reasoning above rather than on coverage.
Nothing in
crates/orLib/hashes a cell or depends on the old repr.