From 45d2c6ceb44bd267fac30a3adb47ad0739659283 Mon Sep 17 00:00:00 2001 From: Jonas Rembser Date: Wed, 11 Mar 2026 14:14:23 +0100 Subject: [PATCH] [CPyCppyy] Remove pointer-based fallback in CPPInstance equality check The Python proxy for C++ objects previously implemented a fallback equality comparison based on proxy type and the held C++ pointer address when no C++ `operator==` / `operator!=` was available. This behavior was misleading, because it made expressions like `a == b` appear to perform a value comparison even though no corresponding C++ operator was defined. This patch removes the implicit pointer-based fallback and instead raises a TypeError when equality is requested between two CPPInstance proxies for which no C++ equality operator can be resolved. This avoids silently changing semantics and makes unsupported comparisons explicit. The only case where the pointer-based fallback is kept is for proxies of the exact same type when at least one wraps a `nullptr`: in this case, comparison continues to be performed on the pointer value, because comparing by value would not be possible. This preserves existing cppyy behavior in a case that is not semantically ambiguous. Although C++ would also allow comparisons between nullptr pointers of types related by inheritance, broadening the rule would silently change previous cppyy behavior, where such comparisons returned `False`. To summarize: this change prevents ambiguous equality semantics while avoiding silent behavior changes for existing code by raising a type error in cases that are ambiguous or where previous behavior was different from C++ semantics. Closes #21347. --- .../pyroot/cppyy/CPyCppyy/src/CPPInstance.cxx | 70 ++++++++++--------- .../cppyy/cppyy/test/test_advancedcpp.py | 17 +++-- .../pyroot/cppyy/cppyy/test/test_datatypes.py | 6 +- .../cppyy/cppyy/test/test_regression.py | 2 +- .../test/tobject_comparisonops.py | 4 +- roottest/python/basic/PyROOT_datatypetest.py | 1 - roottest/python/cpp/PyROOT_cpptests.py | 9 ++- .../regression/PyROOT_regressiontests.py | 4 +- 8 files changed, 61 insertions(+), 52 deletions(-) diff --git a/bindings/pyroot/cppyy/CPyCppyy/src/CPPInstance.cxx b/bindings/pyroot/cppyy/CPyCppyy/src/CPPInstance.cxx index 301e9d39d260e..1f5a696f3c4f1 100644 --- a/bindings/pyroot/cppyy/CPyCppyy/src/CPPInstance.cxx +++ b/bindings/pyroot/cppyy/CPyCppyy/src/CPPInstance.cxx @@ -532,22 +532,6 @@ static inline PyObject* eqneq_binop(CPPClass* klass, PyObject* self, PyObject* o Py_RETURN_TRUE; } -static inline void* cast_actual(void* obj) { - void* address = ((CPPInstance*)obj)->GetObject(); - if (((CPPInstance*)obj)->fFlags & CPPInstance::kIsActual) - return address; - - Cppyy::TCppType_t klass = ((CPPClass*)Py_TYPE((PyObject*)obj))->fCppType; - Cppyy::TCppType_t clActual = Cppyy::GetActualClass(klass, address); - if (clActual && clActual != klass) { - intptr_t offset = Cppyy::GetBaseOffset( - clActual, klass, address, -1 /* down-cast */, true /* report errors */); - if (offset != -1) address = (void*)((intptr_t)address + offset); - } - - return address; -} - #define CPYCPPYY_ORDERED_OPERATOR_STUB(op, ometh, label) \ if (!ometh) { \ @@ -592,25 +576,47 @@ static PyObject* op_richcompare(CPPInstance* self, PyObject* other, int op) result = eqneq_binop((CPPClass*)Py_TYPE(other), other, (PyObject*)self, op); if (result) return result; - // default behavior: type + held pointer value defines identity; if both are - // CPPInstance objects, perform an additional autocast if need be - bool bIsEq = false; - - if ((Py_TYPE(self) == Py_TYPE(other) && \ - self->GetObject() == ((CPPInstance*)other)->GetObject())) { - // direct match - bIsEq = true; - } else if (CPPInstance_Check(other)) { - // try auto-cast match - void* addr1 = cast_actual(self); - void* addr2 = cast_actual(other); - bIsEq = addr1 && addr2 && (addr1 == addr2); + // for non-CPPInstance objects, let Python handle dispatch/fallback + if (!CPPInstance_Check(other)) { + Py_INCREF(Py_NotImplemented); + return Py_NotImplemented; } - if ((op == Py_EQ && bIsEq) || (op == Py_NE && !bIsEq)) - Py_RETURN_TRUE; + // if both proxies have the same type and at least one wraps nullptr, + // comparing nullness is unambiguous + auto ptr1 = self->GetObject(); + auto ptr2 = ((CPPInstance*)other)->GetObject(); + if(ptr1 == nullptr || ptr2 == nullptr) { + // Take this branch only for exact proxy type matches. Broadening this + // to inheritance-related types would be allowed in C++, but it would + // silently change previous cppyy behavior, where equality comparison + // of different-typed nullptr always resulted in `False`, straying away + // from C++. + if (Py_TYPE(self) == Py_TYPE(other)) { + bool bIsEq = ptr1 == ptr2; + if ((op == Py_EQ && bIsEq) || (op == Py_NE && !bIsEq)) + Py_RETURN_TRUE; + Py_RETURN_FALSE; + } + } - Py_RETURN_FALSE; + // in the remaining cases, the semantics of the comparison are ambiguous, so we raise an exception + const char* opstr = op == Py_EQ ? "==" : "!="; + const char* lhs = Py_TYPE(self)->tp_name; + const char* rhs = Py_TYPE(other)->tp_name; + const char *msg = + "\nC++ equality operator '%s' is not defined for objects of type \"%s\" and \"%s\"." + "\n\nThe Python proxy no longer falls back to comparing by type and held C++ pointer " + "address, because that can be misleading: using `%s` may look like a value comparison " + "even though no corresponding C++ operator is available." + "\n\nSome comparisons that are well-formed in C++ are still rejected here to avoid " + "changing legacy cppyy behavior silently." + "\n\nDefine a suitable C++ equality operator for these operands, or compare object identity explicitly " + "through another mechanism if that is what you intend." + "\n"; + + PyErr_Format(PyExc_TypeError, msg, opstr, lhs, rhs, opstr); + return NULL; } // ordered comparison operators diff --git a/bindings/pyroot/cppyy/cppyy/test/test_advancedcpp.py b/bindings/pyroot/cppyy/cppyy/test/test_advancedcpp.py index 0b6ba190a79c0..f11c9003c4ec4 100644 --- a/bindings/pyroot/cppyy/cppyy/test/test_advancedcpp.py +++ b/bindings/pyroot/cppyy/cppyy/test/test_advancedcpp.py @@ -445,10 +445,10 @@ def test09_opaque_pointer_passing(self): #assert o == cppyy.bind_object(cobj, o.__class__) #assert o == cppyy.bind_object(cobj, "some_concrete_class") assert cppyy.addressof(o) == cppyy.addressof(cppyy.bind_object(addr, some_concrete_class)) - assert o == cppyy.bind_object(addr, some_concrete_class) - assert o == cppyy.bind_object(addr, type(o)) - assert o == cppyy.bind_object(addr, o.__class__) - assert o == cppyy.bind_object(addr, "some_concrete_class") + assert o is cppyy.bind_object(addr, some_concrete_class) + assert o is cppyy.bind_object(addr, type(o)) + assert o is cppyy.bind_object(addr, o.__class__) + assert o is cppyy.bind_object(addr, "some_concrete_class") raises(TypeError, cppyy.bind_object, addr, "does_not_exist") raises(TypeError, cppyy.bind_object, addr, 1) @@ -493,7 +493,6 @@ def test10_object_identity(self): r4 = r1.m_other assert r3 is r4 - assert r3 == r2 assert r3 is r2 r3.extra = 42 @@ -531,13 +530,13 @@ def test12_actual_type(self): b = base_class() d = derived_class() - assert b == b.cycle(b) + assert b is b.cycle(b) assert id(b) == id(b.cycle(b)) - assert b == d.cycle(b) + assert b is d.cycle(b) assert id(b) == id(d.cycle(b)) - assert d == b.cycle(d) + assert d is b.cycle(d) assert id(d) == id(b.cycle(d)) - assert d == d.cycle(d) + assert d is d.cycle(d) assert id(d) == id(d.cycle(d)) assert isinstance(b.cycle(b), base_class) diff --git a/bindings/pyroot/cppyy/cppyy/test/test_datatypes.py b/bindings/pyroot/cppyy/cppyy/test/test_datatypes.py index 1404a5d99148d..140b1c4a624f1 100644 --- a/bindings/pyroot/cppyy/cppyy/test/test_datatypes.py +++ b/bindings/pyroot/cppyy/cppyy/test/test_datatypes.py @@ -644,7 +644,7 @@ def test09_global_ptr(self): d = gbl.get_global_pod() assert gbl.is_global_pod(d) - assert c == d + assert c is d assert id(c) == id(d) e = gbl.CppyyTestPod() @@ -1008,8 +1008,8 @@ def test19_object_and_pointer_comparisons(self): l1 = cppyy.bind_object(0, gbl.FourVector) assert not l1 - assert c1 != l1 - assert l1 != c1 + # assert c1 != l1 + # assert l1 != c1 l2 = cppyy.bind_object(0, gbl.FourVector) assert l1 == l2 diff --git a/bindings/pyroot/cppyy/cppyy/test/test_regression.py b/bindings/pyroot/cppyy/cppyy/test/test_regression.py index 81c6a74fd0eda..d66ebeddaeb58 100644 --- a/bindings/pyroot/cppyy/cppyy/test/test_regression.py +++ b/bindings/pyroot/cppyy/cppyy/test/test_regression.py @@ -253,7 +253,7 @@ def test11_cobject_addressing(self): a = cppyy.gbl.CObjA() co = cppyy.ll.as_cobject(a) - assert a == cppyy.bind_object(co, 'CObjA') + assert a is cppyy.bind_object(co, 'CObjA') assert a.m_int == 42 assert cppyy.bind_object(co, 'CObjA').m_int == 42 diff --git a/bindings/pyroot/pythonizations/test/tobject_comparisonops.py b/bindings/pyroot/pythonizations/test/tobject_comparisonops.py index 4f39f8ea90d67..8645c57944f13 100644 --- a/bindings/pyroot/pythonizations/test/tobject_comparisonops.py +++ b/bindings/pyroot/pythonizations/test/tobject_comparisonops.py @@ -19,7 +19,7 @@ def test_eq(self): o = ROOT.TObject() # TObject::IsEqual compares internal address - self.assertTrue(o == o) + self.assertTrue(o.IsEqual(o)) # Test comparison with no TObject self.assertFalse(o == 1) @@ -31,7 +31,7 @@ def test_ne(self): o = ROOT.TObject() # TObject::IsEqual compares internal address - self.assertFalse(o != o) + self.assertFalse(not o.IsEqual(o)) # Test comparison with no TObject self.assertTrue(o != 1) diff --git a/roottest/python/basic/PyROOT_datatypetest.py b/roottest/python/basic/PyROOT_datatypetest.py index 2ec8192d756b7..a11b1c0429e87 100644 --- a/roottest/python/basic/PyROOT_datatypetest.py +++ b/roottest/python/basic/PyROOT_datatypetest.py @@ -598,7 +598,6 @@ def test10_global_ptr(self): d = gbl.get_global_pod() assert gbl.is_global_pod(d) - assert c == d assert c is d e = gbl.CppyyTestPod() diff --git a/roottest/python/cpp/PyROOT_cpptests.py b/roottest/python/cpp/PyROOT_cpptests.py index 4ebdc363b51e6..c34c4e85ee6f6 100644 --- a/roottest/python/cpp/PyROOT_cpptests.py +++ b/roottest/python/cpp/PyROOT_cpptests.py @@ -230,7 +230,7 @@ def test12VoidPointerPassing( self ): pZ = Z.getZ(0) self.assertEqual( Z.checkAddressOfZ( pZ ), True ) - self.assertEqual( pZ , Z.getZ(1) ) + # self.assertEqual( pZ , Z.getZ(1) ) import array # Not supported in p2.2 @@ -302,8 +302,11 @@ def test15ObjectAndPointerComparisons( self ): l1 = MakeNullPointer( TLorentzVector ) self.assertFalse( l1 ) - self.assertNotEqual( c1, l1 ) - self.assertNotEqual( l1, c1 ) + # Forbidden by the type system + with self.assertRaises(TypeError): + self.assertNotEqual( c1, l1 ) + with self.assertRaises(TypeError): + self.assertNotEqual( l1, c1 ) l2 = MakeNullPointer( TLorentzVector ) self.assertEqual( l1, l2 ) diff --git a/roottest/python/regression/PyROOT_regressiontests.py b/roottest/python/regression/PyROOT_regressiontests.py index cb85a297bd5d8..669974d915f46 100644 --- a/roottest/python/regression/PyROOT_regressiontests.py +++ b/roottest/python/regression/PyROOT_regressiontests.py @@ -360,7 +360,9 @@ def test1IterateWithBaseIterator( self ): b = a.begin() e = a.end() - self.assertNotEqual( a, e ) + # This comparison is forbidden by the type system + with self.assertRaises(TypeError): + a == e b.__preinc__() self.assertEqual( b, e )