[Python] Change of behavior in TObject equality comparisons

[guitargeek]

In commit 42f3188 (fortunately not included in any release yet), I removedthe TObject __eq__ Pythonization that calls TPython IsEqual() to do comparison by pointer. I assumed that this means there will be a TypeError or NotImplemented error when someone is still trying to do == or != for a C++ class that doesn't implement these operators explicitly, but I was wrong. Python actually falls back to a by-address comparison itself (like the is keyword).

This means that there was now a change in behavior in some corner cases, like the case where a class inherits from TObject, and overrides IsEqual with something that is not a pointer comparison.

Here is an example:

ROOT.gInterpreter.Declare("""
class MyDerived : public TObject {
public:
    MyDerived(int v) : value(v) {}

    // Just to be safe and trigger to Pythonization or Python `is` fallback
    bool operator==(const MyDerived&) const = delete;
    bool operator!=(const MyDerived&) const = delete;

    bool IsEqual(MyDerived &other) const { return value == other.value; }

private:
    int value;
};
""")

x1 = ROOT.MyDerived(1)
x2 = ROOT.MyDerived(1)
print(x1 == x2)
print(x1 != x2)

Output before 42f3188, where the TObject Pythonization forwards to IsEqual():

True
False

And in master, where Python falls back to address comparison:

False
True

That's pretty bad. But it gets worse! There is a silly RooFit example:

v1 = ROOT.RooRealVar("x1", "", 10.)
v2 = ROOT.RooRealVar("x2", "", 10.)

print(v1 == v2)
print(v1 != v2)

Before 42f3188:

True
True

In master :

True
False

There is also a change in behavior, but the previous behavior is inconsistent. What's going on here? The RooAbsReal base class implements operator== to compare the mathematical value, which is the same in this example. The class misses however to implement operator !=, so in that case the old behavior fell back to the pointer comparison via TObject::IsEqual(). That's why the behavior is inconsistent. In master, the != operator is not implemented, but before Python falls back to address comparison with is, it falls back to inverting the == comparison, which is why things become inconsistent!

This problem was found when testing Combine with ROOT master. In Combine, != is used all over the place for address comparisons of RooFit objects, so we can't afford a silent change in behavior, even though the old behavior didn't make sense.

We'll have to find a way out of this dilemma. Before 42f3188 the behavior was wrong, but users relied on it in the case of RooFit.

I think it would be best if cppyy sticks to C++ semantics: raise a NotImplementedError if one does a comparison that would not also be possible in C++. No Fallback to a magic IsEqual method in an undocumented Pythonization, and no Python fallback to address comparison that misleads the user in thinking equality by value is implemented. Our users would get some of these exceptions when updating ROOT, but I think that's for the better. The only extension compared to C++ behavior would be comparison with Python type instances if there is a meaningful Pythonization, like std::string with str.

[vepadulano]

My two cents:

• I agree that we should not fallback to undocumented Pythonizations
• I think raising NotImplementedError is the right thing to do. Unfortunately I don't think we can do this automatically (at least I can't think of a way right now), but we'll have to do it systematically whenever we see chances for potential confusion (the same topic applies to histograms for example).
• operator== for std::string compared against Python str makes sense, I don't know if we have an explicit/implicit Pythonization yet. If not, I believe we should have it.