From 6c91c5bb2a6258b4e45e6850c170532b9879e89f Mon Sep 17 00:00:00 2001 From: Changjoon Date: Thu, 30 Apr 2026 04:45:23 +0900 Subject: [PATCH] Short-circuit identity in rich_compare_bool for Eq/Ne (PyObject_RichCompareBool parity) (#7734) * Short-circuit identity in rich_compare_bool for Eq/Ne (PyObject_RichCompareBool parity) CPython distinguishes two comparison entry points (Objects/object.c): - PyObject_RichCompare returns the raw __eq__ / __ne__ result; no identity short-circuit - PyObject_RichCompareBool returns bool; identity implies equality (and inequality is false on identity), short-circuiting before dispatch Collection membership / equality (x in [x], [nan] == [nan], set/dict comparisons) go through the bool variant and rely on the short-circuit. RustPython's rich_compare_bool skipped the identity check, so a buggy or raising __eq__ propagated even when the operand was the same object. Add an identity short-circuit at the top of rich_compare_bool for Eq (returns true) and Ne (returns false). Ordering ops fall through to _cmp because Python does not guarantee reflexivity for />=. _cmp itself is untouched, so == / != operators continue to invoke __eq__ / __ne__ exactly as before. Unmasks test_dictviews.TestDictViews.test_compare_error. Verified byte-identical with CPython 3.14.4 across 53 scenarios in 10 categories (collection membership / equality / ordering ops / NaN / hash collision / dict views / list-set-dict ops). 14-module regression sweep ~2,402 tests passes with no regressions. * Route proxy comparisons through PyObject_RichCompare The identity short-circuit added to rich_compare_bool exposed a latent bug in three proxy types (weakref, weakproxy, mappingproxy): they were delegating their __eq__ to the bool variant on referents, while CPython uses PyObject_RichCompare so the referent's __eq__ runs even when the referents share identity. Fixes test_weak_keyed_cascading_deletes which depends on key __eq__ firing during dict deletion to trigger a side-effect that mutates the key list. --- Lib/test/test_dictviews.py | 1 - crates/vm/src/builtins/mappingproxy.rs | 11 +++++++---- crates/vm/src/builtins/weakproxy.rs | 11 +++++++---- crates/vm/src/builtins/weakref.rs | 21 ++++++++++++++------- crates/vm/src/protocol/object.rs | 14 ++++++++++++++ 5 files changed, 42 insertions(+), 16 deletions(-) diff --git a/Lib/test/test_dictviews.py b/Lib/test/test_dictviews.py index 2809417ff..4e5b91840 100644 --- a/Lib/test/test_dictviews.py +++ b/Lib/test/test_dictviews.py @@ -292,7 +292,6 @@ class DictSetTest(unittest.TestCase): self.assertRaises(TypeError, copy.copy, d.values()) self.assertRaises(TypeError, copy.copy, d.items()) - @unittest.expectedFailure # TODO: RUSTPYTHON def test_compare_error(self): class Exc(Exception): pass diff --git a/crates/vm/src/builtins/mappingproxy.rs b/crates/vm/src/builtins/mappingproxy.rs index fe7e0e178..3f9a2538d 100644 --- a/crates/vm/src/builtins/mappingproxy.rs +++ b/crates/vm/src/builtins/mappingproxy.rs @@ -5,7 +5,7 @@ use crate::{ class::PyClassImpl, common::{hash, lock::LazyLock}, convert::ToPyObject, - function::{ArgMapping, OptionalArg, PyComparisonValue}, + function::{ArgMapping, OptionalArg, PyArithmeticValue, PyComparisonValue}, object::{Traverse, TraverseFn}, protocol::{PyMappingMethods, PyNumberMethods, PySequenceMethods}, types::{ @@ -211,9 +211,12 @@ impl Comparable for PyMappingProxy { vm: &VirtualMachine, ) -> PyResult { let obj = zelf.to_object(vm)?; - Ok(PyComparisonValue::Implemented( - obj.rich_compare_bool(other, op, vm)?, - )) + // CPython parity (Objects/descrobject.c::mappingproxy_richcompare): + // delegate to PyObject_RichCompare on the underlying mapping. + let res = obj.rich_compare(other.to_owned(), op, vm)?; + PyArithmeticValue::from_object(vm, res) + .map(|o| o.try_to_bool(vm)) + .transpose() } } diff --git a/crates/vm/src/builtins/weakproxy.rs b/crates/vm/src/builtins/weakproxy.rs index 561e69d61..44579afcc 100644 --- a/crates/vm/src/builtins/weakproxy.rs +++ b/crates/vm/src/builtins/weakproxy.rs @@ -4,7 +4,7 @@ use crate::{ Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine, atomic_func, class::PyClassImpl, common::hash::PyHash, - function::{OptionalArg, PyComparisonValue, PySetterValue}, + function::{OptionalArg, PyArithmeticValue, PyComparisonValue, PySetterValue}, protocol::{PyIter, PyIterReturn, PyMappingMethods, PyNumberMethods, PySequenceMethods}, stdlib::builtins::reversed, types::{ @@ -301,9 +301,12 @@ impl Comparable for PyWeakProxy { vm: &VirtualMachine, ) -> PyResult { let obj = zelf.try_upgrade(vm)?; - Ok(PyComparisonValue::Implemented( - obj.rich_compare_bool(other, op, vm)?, - )) + // CPython parity (Objects/weakref.c::proxy_richcompare): delegate to + // PyObject_RichCompare on the referent, not the bool variant. + let res = obj.rich_compare(other.to_owned(), op, vm)?; + PyArithmeticValue::from_object(vm, res) + .map(|o| o.try_to_bool(vm)) + .transpose() } } diff --git a/crates/vm/src/builtins/weakref.rs b/crates/vm/src/builtins/weakref.rs index 0527142bb..43d4af382 100644 --- a/crates/vm/src/builtins/weakref.rs +++ b/crates/vm/src/builtins/weakref.rs @@ -6,7 +6,7 @@ use crate::common::{ use crate::{ AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine, class::PyClassImpl, - function::{FuncArgs, OptionalArg}, + function::{FuncArgs, OptionalArg, PyArithmeticValue, PyComparisonValue}, types::{ Callable, Comparable, Constructor, Hashable, Initializer, PyComparisonOp, Representable, }, @@ -127,15 +127,22 @@ impl Comparable for PyWeak { other: &PyObject, op: PyComparisonOp, vm: &VirtualMachine, - ) -> PyResult { + ) -> PyResult { op.eq_only(|| { let other = class_or_notimplemented!(Self, other); let both = zelf.upgrade().and_then(|s| other.upgrade().map(|o| (s, o))); - let eq = match both { - Some((a, b)) => vm.bool_eq(&a, &b)?, - None => zelf.is(other), - }; - Ok(eq.into()) + match both { + // CPython parity (Objects/weakref.c::weakref_richcompare): use + // PyObject_RichCompare on the referents, not the bool variant, + // so referent __eq__ runs even when referents share identity. + Some((a, b)) => { + let res = a.rich_compare(b, PyComparisonOp::Eq, vm)?; + PyArithmeticValue::from_object(vm, res) + .map(|obj| obj.try_to_bool(vm)) + .transpose() + } + None => Ok(zelf.is(other).into()), + } }) } } diff --git a/crates/vm/src/protocol/object.rs b/crates/vm/src/protocol/object.rs index 1ddbc1162..5613458c9 100644 --- a/crates/vm/src/protocol/object.rs +++ b/crates/vm/src/protocol/object.rs @@ -348,6 +348,20 @@ impl PyObject { op_id: PyComparisonOp, vm: &VirtualMachine, ) -> PyResult { + // CPython parity: PyObject_RichCompareBool guarantees identity implies + // equality (and inequality is false on identity), short-circuiting + // before dispatch. Collection membership / equality (e.g. `x in [x]`, + // `[nan] == [nan]`) depend on this even when `__eq__` would raise + // or return False. Only Eq/Ne are decidable from identity; ordering + // ops fall through to `_cmp` because Python does not guarantee + // reflexivity for `<`/`<=`/`>`/`>=`. + if self.is(other) { + match op_id { + PyComparisonOp::Eq => return Ok(true), + PyComparisonOp::Ne => return Ok(false), + _ => {} + } + } match self._cmp(other, op_id, vm)? { Either::A(obj) => obj.try_to_bool(vm), Either::B(other) => Ok(other),