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 )