From 1168c661946aca642ecbb96554061c17044b9cf8 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 error message states why the comparison is not supported and what to do instead: define a C++ `operator==` for the operands, implement `__eq__` and `__ne__` from Python, or compare the addresses of the wrapped C++ objects explicitly with `cppyy.addressof()`. 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. Since this is a change in behavior that user code can rely on, it is announced in the ROOT 6.42 release notes, and the new semantics are explained in the "Equality comparisons" section of the pythonizations documentation. Closes #21347. --- README/ReleaseNotes/v642/index.md | 47 ++++++++++++ .../pyroot/cppjit/src/cpyrt/CPPInstance.cxx | 72 ++++++++++--------- .../pyroot/cppjit/test/test_advancedcpp.py | 5 +- bindings/pyroot/cppjit/test/test_datatypes.py | 8 ++- .../pyroot/cppjit/test/test_regression.py | 6 +- .../pythonizations/doc/pythonizations.md | 50 +++++++++++++ .../pyroot/pythonizations/test/tobject.py | 20 ++++-- .../regression/PyROOT_regressiontests.py | 6 +- 8 files changed, 168 insertions(+), 46 deletions(-) diff --git a/README/ReleaseNotes/v642/index.md b/README/ReleaseNotes/v642/index.md index 1ca43ed2bf536..e003d3b2d209e 100644 --- a/README/ReleaseNotes/v642/index.md +++ b/README/ReleaseNotes/v642/index.md @@ -146,6 +146,53 @@ resolve its symbols from the `libROOTPythonizations` Python extension module. Replace `obj.Connect(signal, "TPyDispatcher", disp, "Dispatch()")` with `obj.Connect(signal, callable)`. +### Equality comparisons of C++ objects follow C++ semantics + +**This is a behavior change that can affect existing code.** Comparing two +Python proxies of C++ objects with `==` or `!=` now requires the C++ class to +provide the corresponding operator (if only one of `operator==` and +`operator!=` is defined, the other one is still derived from it by negation). +When there is no C++ equality operator for the operands, a `TypeError` is +raised: + +```python +ROOT.gInterpreter.Declare("class MyClass {};") + +a = ROOT.MyClass() +b = ROOT.MyClass() + +a == b # raises TypeError in ROOT 6.42, was False before +a == a # raises TypeError in ROOT 6.42, was True before +``` + +Previously, such comparisons silently fell back to comparing the type and the +address of the wrapped C++ object. That fallback was misleading, since `a == b` +looked like a comparison by value even though the C++ class did not define one. +It also produced inconsistent results for classes that define only one of the +two operators: for example, `v1 == v2` for two **RooRealVar** objects with the +same value returned `True` via `RooAbsReal::operator==`, while `v1 != v2` +returned `True` as well, because `!=` fell back to comparing addresses. + +If you relied on the old behavior to check whether two proxies refer to the +same C++ object, compare the addresses explicitly with +`ROOT.addressof(a) == ROOT.addressof(b)`. If a comparison by value is +meaningful for your class, define an `operator==` for it in C++, or implement +`__eq__` and `__ne__` from Python with a custom pythonization. + +Two exceptions to the new rule are kept, to avoid changing behavior where the +old semantics were unambiguous: + + * Proxies of the *same* type are still compared by address if at least one of + them wraps a `nullptr`, because a comparison by value is not possible then. + * Comparisons with objects that are not C++ proxies are left to Python, so + `a == 1` is still `False` and `a != 1` is still `True`. + +Related to this, the undocumented `__eq__`/`__ne__` pythonization of +**TObject**, which forwarded to `TObject::IsEqual()`, is gone. Classes +inheriting from `TObject` that override `IsEqual()` need to call it explicitly, +or define a C++ `operator==`. More details can be found in the +[pythonizations documentation](https://root.cern/doc/master/group__Pythonizations.html). + ## I/O * Reading a collection without its dictionary no longer crashes when the elements hold a `std::string` or a `TString`. The emulated collection proxy relocated its elements with a raw memory copy when its buffer had to grow, which corrupts an object that points into itself, such as a `std::string` using the small string optimization; the invalid pointer was then freed when the object was destroyed. Such elements are now destroyed and reconstructed at the new location instead. This affected for instance a `std::vector>` read back without a dictionary. diff --git a/bindings/pyroot/cppjit/src/cpyrt/CPPInstance.cxx b/bindings/pyroot/cppjit/src/cpyrt/CPPInstance.cxx index 05d0bc987d913..ab112bb1054d7 100644 --- a/bindings/pyroot/cppjit/src/cpyrt/CPPInstance.cxx +++ b/bindings/pyroot/cppjit/src/cpyrt/CPPInstance.cxx @@ -592,23 +592,6 @@ static inline PyObject* eqneq_binop(CPPClass* klass, PyObject* self, Py_RETURN_TRUE; } -static inline void* cast_actual(void* obj) { - void* address = ((CPPInstance*)obj)->GetObject(); - if (((CPPInstance*)obj)->fFlags & CPPInstance::kIsActual) - return address; - - interop::TCppScope_t klass = ((CPPClass*)Py_TYPE((PyObject*)obj))->fCppType; - interop::TCppScope_t clActual = interop::GetActualClass(klass, address); - if (clActual && clActual != klass) { - intptr_t offset = interop::GetBaseOffset( - clActual, klass, address, -1 /* down-cast */, true /* report errors */); - if (offset != -1) - address = (void*)((intptr_t)address + offset); - } - - return address; -} - #define CPYRT_ORDERED_OPERATOR_STUB(op, ometh, label) \ if (!ometh) { \ PyCallable* pyfunc = \ @@ -659,25 +642,48 @@ static PyObject* op_richcompare(CPPInstance* self, PyObject* other, int 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* msg = + "\n'%s' is not supported between instances of \"%s\" and \"%s\", " + "because these operands have no C++ " + "equality operator." + "\n\nIf comparing by value is meaningful for these types, define a C++ " + "`operator==` for them, or " + "implement `__eq__` and `__ne__` in Python." + "\n\nTo check whether the two proxies refer to the same C++ object, " + "compare their addresses explicitly " + "with `cppjit.addressof()`." + "\n"; + + PyErr_Format(PyExc_TypeError, msg, opstr, Py_TYPE(self)->tp_name, + Py_TYPE(other)->tp_name); + return NULL; } // ordered comparison operators diff --git a/bindings/pyroot/cppjit/test/test_advancedcpp.py b/bindings/pyroot/cppjit/test/test_advancedcpp.py index b59ddc59e1af4..402163e8eb6bb 100644 --- a/bindings/pyroot/cppjit/test/test_advancedcpp.py +++ b/bindings/pyroot/cppjit/test/test_advancedcpp.py @@ -524,7 +524,6 @@ def test10_object_identity(self): r4 = r1.m_other assert r3 is r4 - assert r3 == r2 assert r3 is r2 r3.extra = 42 @@ -738,7 +737,9 @@ def test21_access_to_global_variables(self): assert type(o) != type(cppjit.gbl.g_abstract_ptr) cppjit.gbl.g_abstract_ptr = o assert type(o) != type(cppjit.gbl.g_abstract_ptr) - assert cppjit.gbl.g_abstract_ptr == o + # the classes have no C++ equality operator, so the proxies have to be + # compared by address explicitly + assert cppjit.addressof(cppjit.gbl.g_abstract_ptr) == cppjit.addressof(o) cppjit.gbl.g_abstract_ptr = cppjit.nullptr cppjit.cppexec("std::vector* gtestv1 = nullptr;") diff --git a/bindings/pyroot/cppjit/test/test_datatypes.py b/bindings/pyroot/cppjit/test/test_datatypes.py index 5e508d32dcee6..5c914fcd7c6db 100644 --- a/bindings/pyroot/cppjit/test/test_datatypes.py +++ b/bindings/pyroot/cppjit/test/test_datatypes.py @@ -1124,8 +1124,12 @@ def test19_object_and_pointer_comparisons(self): l1 = cppjit.bind_object(0, gbl.FourVector) assert not l1 - assert c1 != l1 - assert l1 != c1 + # comparing null pointers of unrelated types is not supported, since + # there is no C++ equality operator for these operands + with raises(TypeError): + c1 != l1 + with raises(TypeError): + l1 != c1 l2 = cppjit.bind_object(0, gbl.FourVector) assert l1 == l2 diff --git a/bindings/pyroot/cppjit/test/test_regression.py b/bindings/pyroot/cppjit/test/test_regression.py index c879ec1b8d2af..9d3528692d208 100644 --- a/bindings/pyroot/cppjit/test/test_regression.py +++ b/bindings/pyroot/cppjit/test/test_regression.py @@ -889,9 +889,11 @@ def triggerChange(self): def changeCallback(self, b): assert type(b) == type(self.derived) - assert b == self.derived + # the classes involved have no C++ equality operator, so the + # proxies have to be compared by address explicitly + assert cppjit.addressof(b) == cppjit.addressof(self.derived) cast = cppjit.gbl.std.addressof[type(b)] - assert cast(b) == cast(self.derived) + assert cppjit.addressof(cast(b)) == cppjit.addressof(cast(self.derived)) self.success = True g = Glue() diff --git a/bindings/pyroot/pythonizations/doc/pythonizations.md b/bindings/pyroot/pythonizations/doc/pythonizations.md index 6983ea0a7eaa8..8c4816afbec26 100644 --- a/bindings/pyroot/pythonizations/doc/pythonizations.md +++ b/bindings/pyroot/pythonizations/doc/pythonizations.md @@ -126,6 +126,56 @@ unconditionally. It is only available for polymorphic base classes, which is why the `Base` class in the example has a virtual destructor. +### Equality comparisons + +The `==` and `!=` operators on C++ object proxies follow C++ semantics: they +call the `operator==` or `operator!=` that the C++ class provides. If only one +of the two operators is defined, the other one is derived from it by negation. + +If the class provides no equality operator at all, the comparison is **not** +silently answered by comparing the addresses of the wrapped C++ objects. +Instead, a `TypeError` is raised: + +~~~{.py} +ROOT.gInterpreter.Declare("class MyClass {};") + +a = ROOT.MyClass() +b = ROOT.MyClass() + +a == b # raises TypeError +~~~ + +An address comparison would be misleading here, because `a == b` looks like a +comparison by value even though the C++ class does not define one. Note that +raising is also what protects you from Python's own fallback: if the proxy +declared the comparison unsupported, Python would silently compare object +identity, just like the `is` operator does. + +There are three ways out if you hit this error: + +1. Define an `operator==` for your C++ class, if comparing by value is + meaningful for it. This is the recommended solution, since the class then + behaves consistently in C++ and in Python. +2. Implement `__eq__` and `__ne__` from Python, for example with a custom + pythonization as explained in the next section. +3. If you want to know whether two proxies refer to the same C++ object, + compare their addresses explicitly: + ~~~{.py} + ROOT.addressof(a) == ROOT.addressof(b) + ~~~ + +A few special cases are worth pointing out: + + * Comparing a proxy with an object that is not a C++ proxy is left to Python, + so for example `a == 1` is `False` and `a != 1` is `True`. + * Two proxies of the *same* type are still compared by address if at least + one of them wraps a `nullptr`, because comparing by value is impossible in + that case. If the types differ, a `TypeError` is raised, even though C++ + would allow the comparison for related types. + * Comparing a null proxy to `None` raises a `TypeError` as well. Use the + truth value of the proxy (`if not a: ...`) to check for a null pointer, and + `a is None` to check for Python's `None`. + ### Custom pythonizations for C++ user classes by example This example shows how to use the `@pythonization` decorator to add extra diff --git a/bindings/pyroot/pythonizations/test/tobject.py b/bindings/pyroot/pythonizations/test/tobject.py index c923e2b60c8b3..958593577346f 100644 --- a/bindings/pyroot/pythonizations/test/tobject.py +++ b/bindings/pyroot/pythonizations/test/tobject.py @@ -44,8 +44,10 @@ class TObjectComparisonOps(unittest.TestCase): """ Test for the comparison operators of TObject and subclasses: __eq__, __ne__, __lt__, __le__, __gt__, __ge__. - Such pythonizations rely on TObject::IsEqual and TObject::Compare, - which can be reimplemented in subclasses. + The ordering pythonizations rely on TObject::Compare, which can be + reimplemented in subclasses. There is no __eq__/__ne__ pythonization: + equality follows C++ semantics, so comparing objects of a class without + a C++ equality operator raises a TypeError. """ num_elems = 3 @@ -54,8 +56,11 @@ class TObjectComparisonOps(unittest.TestCase): def test_eq(self): o = ROOT.TObject() - # TObject::IsEqual compares internal address - self.assertTrue(o == o) + # TObject has no C++ operator==, so the comparison is not supported. + # To compare by address, TObject::IsEqual can be used explicitly. + with self.assertRaises(TypeError): + o == o + self.assertTrue(o.IsEqual(o)) # Test comparison with no TObject self.assertFalse(o == 1) @@ -66,8 +71,11 @@ def test_eq(self): def test_ne(self): o = ROOT.TObject() - # TObject::IsEqual compares internal address - self.assertFalse(o != o) + # TObject has no C++ operator!=, so the comparison is not supported. + # To compare by address, TObject::IsEqual can be used explicitly. + with self.assertRaises(TypeError): + o != o + self.assertFalse(not o.IsEqual(o)) # Test comparison with no TObject self.assertTrue(o != 1) diff --git a/roottest/python/regression/PyROOT_regressiontests.py b/roottest/python/regression/PyROOT_regressiontests.py index 92824cbbcde87..9edb2bfe30125 100644 --- a/roottest/python/regression/PyROOT_regressiontests.py +++ b/roottest/python/regression/PyROOT_regressiontests.py @@ -245,7 +245,11 @@ def test1IterateWithBaseIterator( self ): b = a.begin() e = a.end() - self.assertNotEqual( a, e ) + self.assertNotEqual( b, e ) + + # the AttributeList itself has no equality operator with an iterator + with self.assertRaises(TypeError): + a == e b.__preinc__() self.assertEqual( b, e )