[Python] Deprecate TObject __eq__ pythonization

[guitargeek]

The TObject __eq__ pythonization calls TObject::Equals(), which by default does a pointer comparison. That can be very confusing for Python users.

Here is how the current warning will look like:

/home/rembserj/code/root/root_build/../chck.py:11: FutureWarning:
TObject.__eq__ is deprecated and will be removed in ROOT 6.40.

It forwards to TObject::Equals(), which uses pointer comparison if not overridden in derived classes.
This may be confusing, because people expect value comparisons.
Use Pythons `is` for pointer comparison, or request/implement `operator==` on the C++ side if you need value-based equality checks.
  print(n1 == n1)
True

[vepadulano]

Just a fly-by thought, this pythonization has been around for so many years that, while I still strongly believe its current implementation is buggy, I am afraid this will introduce a backwards-incompatible behaviour far, far worse than #19061. One possible way to mitigate this is to have a check that if there is no operator == implementation (or equivalent) for a class, then we can raise an informative exception. Actually, we should probably just raise a warning and declare a deprecation phase for this. Also, note that while the discussion at #19061 is regarding TH1 classes and derived only, this PR is going to have effects for every TObject derived class.

[github-actions]

Test Results

    20 files      20 suites   3d 19h 51m 59s ⏱️
 3 043 tests  3 043 ✅ 0 💤 0 ❌
59 159 runs  59 159 ✅ 0 💤 0 ❌

Results for commit 37f4365.

[guitargeek]

That's exactly my intention! I wanted to see what the CI says, and if there are no problems deprecate this Pythonization.

I am afraid this will introduce a backwards-incompatible behaviour far, far worse than #19061.

In my opinion, the silent behavior changes that are far worse. Removing a method like suggested by this PR at least gives you an error (or warning after this PR is updated).

You also have to see the picture with ROOT 6.34 before the UHI introduction in 6.36. What you guys did with the new __eq__ was to introduce a change of behavior without documenting it, and as you can see from our tests and #19035 this even confused our own code. So it definitely has the potential to create bugs in user code. Doesn't matter how wrong or "buggy" you think the old behavior of comparing by pointer was: it has been there for many years as you said and people might rely on it.

Also, note that while the discussion at #19061 is regarding TH1 classes and derived only, this PR is going to have effects for every TObject derived class.

Yes, and this is a bonus 🙂

[vepadulano]

Yes, and this is a bonus 🙂

Only if we make sure this won't disrupt users' code. As things stand, this PR cannot be merged. My suggestion is to start a deprecation phase with a warning, then make it an exception or not implemented in a later ROOT release like 6.40 or 6.42

[guitargeek]

My suggestion is to start a deprecation phase with a warning, then make it an exception or not implemented in a later ROOT release like 6.40 or 6.42

Good idea! I have implemented what you suggested, and also added an entry in the release notes.

[vepadulano]

Great thank you! Let's see if it doesn't break any existing test in the CI, then this can be merged 👍