Skip to content

Commit 6c91c5b

Browse files
changjoon-parkchangjoon-park
authored
Short-circuit identity in rich_compare_bool for Eq/Ne (PyObject_RichCompareBool parity) (RustPython#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.
1 parent 3c297d4 commit 6c91c5b

5 files changed

Lines changed: 42 additions & 16 deletions

File tree

‎Lib/test/test_dictviews.py‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -292,7 +292,6 @@ def test_copy(self):
292292
self.assertRaises(TypeError, copy.copy, d.values())
293293
self.assertRaises(TypeError, copy.copy, d.items())
294294

295-
@unittest.expectedFailure # TODO: RUSTPYTHON
296295
def test_compare_error(self):
297296
class Exc(Exception):
298297
pass

‎crates/vm/src/builtins/mappingproxy.rs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ use crate::{
55
class::PyClassImpl,
66
common::{hash, lock::LazyLock},
77
convert::ToPyObject,
8-
function::{ArgMapping, OptionalArg, PyComparisonValue},
8+
function::{ArgMapping, OptionalArg, PyArithmeticValue, PyComparisonValue},
99
object::{Traverse, TraverseFn},
1010
protocol::{PyMappingMethods, PyNumberMethods, PySequenceMethods},
1111
types::{
@@ -211,9 +211,12 @@ impl Comparable for PyMappingProxy {
211211
vm: &VirtualMachine,
212212
) -> PyResult<PyComparisonValue> {
213213
let obj = zelf.to_object(vm)?;
214-
Ok(PyComparisonValue::Implemented(
215-
obj.rich_compare_bool(other, op, vm)?,
216-
))
214+
// CPython parity (Objects/descrobject.c::mappingproxy_richcompare):
215+
// delegate to PyObject_RichCompare on the underlying mapping.
216+
let res = obj.rich_compare(other.to_owned(), op, vm)?;
217+
PyArithmeticValue::from_object(vm, res)
218+
.map(|o| o.try_to_bool(vm))
219+
.transpose()
217220
}
218221
}
219222

‎crates/vm/src/builtins/weakproxy.rs‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ use crate::{
44
Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine, atomic_func,
55
class::PyClassImpl,
66
common::hash::PyHash,
7-
function::{OptionalArg, PyComparisonValue, PySetterValue},
7+
function::{OptionalArg, PyArithmeticValue, PyComparisonValue, PySetterValue},
88
protocol::{PyIter, PyIterReturn, PyMappingMethods, PyNumberMethods, PySequenceMethods},
99
stdlib::builtins::reversed,
1010
types::{
@@ -301,9 +301,12 @@ impl Comparable for PyWeakProxy {
301301
vm: &VirtualMachine,
302302
) -> PyResult<PyComparisonValue> {
303303
let obj = zelf.try_upgrade(vm)?;
304-
Ok(PyComparisonValue::Implemented(
305-
obj.rich_compare_bool(other, op, vm)?,
306-
))
304+
// CPython parity (Objects/weakref.c::proxy_richcompare): delegate to
305+
// PyObject_RichCompare on the referent, not the bool variant.
306+
let res = obj.rich_compare(other.to_owned(), op, vm)?;
307+
PyArithmeticValue::from_object(vm, res)
308+
.map(|o| o.try_to_bool(vm))
309+
.transpose()
307310
}
308311
}
309312

‎crates/vm/src/builtins/weakref.rs‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use crate::common::{
66
use crate::{
77
AsObject, Context, Py, PyObject, PyObjectRef, PyPayload, PyRef, PyResult, VirtualMachine,
88
class::PyClassImpl,
9-
function::{FuncArgs, OptionalArg},
9+
function::{FuncArgs, OptionalArg, PyArithmeticValue, PyComparisonValue},
1010
types::{
1111
Callable, Comparable, Constructor, Hashable, Initializer, PyComparisonOp, Representable,
1212
},
@@ -127,15 +127,22 @@ impl Comparable for PyWeak {
127127
other: &PyObject,
128128
op: PyComparisonOp,
129129
vm: &VirtualMachine,
130-
) -> PyResult<crate::function::PyComparisonValue> {
130+
) -> PyResult<PyComparisonValue> {
131131
op.eq_only(|| {
132132
let other = class_or_notimplemented!(Self, other);
133133
let both = zelf.upgrade().and_then(|s| other.upgrade().map(|o| (s, o)));
134-
let eq = match both {
135-
Some((a, b)) => vm.bool_eq(&a, &b)?,
136-
None => zelf.is(other),
137-
};
138-
Ok(eq.into())
134+
match both {
135+
// CPython parity (Objects/weakref.c::weakref_richcompare): use
136+
// PyObject_RichCompare on the referents, not the bool variant,
137+
// so referent __eq__ runs even when referents share identity.
138+
Some((a, b)) => {
139+
let res = a.rich_compare(b, PyComparisonOp::Eq, vm)?;
140+
PyArithmeticValue::from_object(vm, res)
141+
.map(|obj| obj.try_to_bool(vm))
142+
.transpose()
143+
}
144+
None => Ok(zelf.is(other).into()),
145+
}
139146
})
140147
}
141148
}

‎crates/vm/src/protocol/object.rs‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,20 @@ impl PyObject {
348348
op_id: PyComparisonOp,
349349
vm: &VirtualMachine,
350350
) -> PyResult<bool> {
351+
// CPython parity: PyObject_RichCompareBool guarantees identity implies
352+
// equality (and inequality is false on identity), short-circuiting
353+
// before dispatch. Collection membership / equality (e.g. `x in [x]`,
354+
// `[nan] == [nan]`) depend on this even when `__eq__` would raise
355+
// or return False. Only Eq/Ne are decidable from identity; ordering
356+
// ops fall through to `_cmp` because Python does not guarantee
357+
// reflexivity for `<`/`<=`/`>`/`>=`.
358+
if self.is(other) {
359+
match op_id {
360+
PyComparisonOp::Eq => return Ok(true),
361+
PyComparisonOp::Ne => return Ok(false),
362+
_ => {}
363+
}
364+
}
351365
match self._cmp(other, op_id, vm)? {
352366
Either::A(obj) => obj.try_to_bool(vm),
353367
Either::B(other) => Ok(other),

0 commit comments

Comments
 (0)