Skip to content

richcompare suppresses all errors raised from descriptors of comparison special methods #131151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
kcdodd opened this issue Mar 12, 2025 · 4 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@kcdodd
Copy link
Contributor

kcdodd commented Mar 12, 2025

Bug report

Bug description:

This is reporting possibly incorrect suppression of all errors generated by descriptors for the comparison special methods __eq__, __lt__, etc. (and possibly some other special methods). This only affects errors during the __get__ stage, while errors from the actual call propagate as expected.

I say possibly incorrect because I can see this being one of those sensitive underbellies that could affect existing behavior for some libraries. However, this seems inconsistent relative to some other special methods like __contains__, __bool__, etc., and since it suppresses all errors like SystemExit, MemoryError, KeyboardInterrupt, etc. it can hide underlying problems or behaviors that are hard to diagnose.

Minimal example

Minimal demonstration of the issue below is a descriptor which always raises an exception during __get__. The result of comparison through == returns False instead of raising the error. The type of error raised from __get__ is not important, it could have been from exit() or anything else down the stack. However, other special methods like __bool__ do raise the exception, as does directly accessing __eq__ as a method.

class BadDescriptor:
  def __get__(self, obj, cls=None):
    assert False

class BadBase:
  __eq__ = BadDescriptor()
  __bool__ = BadDescriptor()

b = BadBase()

print(f"{b == 1=}")
print(f"{b.__eq__(1)=}")
b == 1=False
Traceback (most recent call last):
  File "bad_descriptor.py", line 24, in <module>
    print(f"{b.__eq__(1)=}")
             ^^^^^^^^
  File "bad_descriptor.py", line 14, in __get__
    assert False
           ^^^^^
AssertionError

Suspected cause and proposed resolution

The source of this behavior appears to be in Objects/typeobject.c function slot_tp_richcompare. Currently if an error occurs while getting the comparison method from a descriptor the error is cleared and NotImplemented is returned. Looking at some of the other special methods it seems like there is some inconsistency, some check for PyErr_Occurred and propagate the exception (EG slot_nb_bool), while others clear the error and default to some other action (E.G. slot_tp_repr).

    if (func == NULL) {
        PyErr_Clear();
        Py_RETURN_NOTIMPLEMENTED;
    }

A couple experimental resolutions were attempted to look at the consequence. The first was to treat errors similar to slot_nb_bool, propagating them instead of suppressing.

    if (func == NULL) {
        if(PyErr_Occurred()){
            return NULL;
        }

        Py_RETURN_NOTIMPLEMENTED;
    }

However, this caused a failed test which seems to be checking that raising AttributeError should lead to suppression and default comparison. It is not clear to me if that actually should be the expected behavior, and the comment in the test seems to indicate that it may actually be testing for some internal regression:

======================================================================
ERROR: testForExceptionsRaisedInInstanceGetattr2 (test.test_class.ClassTests.testForExceptionsRaisedInInstanceGetattr2)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/cdodd/proj/contrib/cpython/Lib/test/test_class.py", line 592, in testForExceptionsRaisedInInstanceGetattr2
    E() == E() # In debug mode, caused a C-level assert() to fail
    ^^^^^^^^^^
  File "/home/cdodd/proj/contrib/cpython/Lib/test/test_class.py", line 580, in booh
    raise AttributeError("booh")
AttributeError: booh

But, as a compromise the following alternative checking specifically for AttributeError passes all tests enabled for my current platform. Depending on feedback here I am willing to contribute a pull request once I have a better idea as to what is expected/desired in these cases.

    if (func == NULL) {
        PyObject* exc = PyErr_Occurred();

        if (exc != NULL){
          if (PyErr_GivenExceptionMatches(exc, PyExc_AttributeError)){
            // NOTE: suppresses only "attribute" errors
            PyErr_Clear();
          }else{
            return NULL;
          }
        }

        Py_RETURN_NOTIMPLEMENTED;
    }

CPython versions tested on:

3.14, CPython main branch

Operating systems tested on:

Linux

Linked PRs

@kcdodd kcdodd added the type-bug An unexpected behavior, bug, or error label Mar 12, 2025
@picnixz picnixz added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Mar 12, 2025
@picnixz
Copy link
Member

picnixz commented Mar 12, 2025

I'm asking fellow core devs that may have an idea why it was designed like this @JelleZijlstra @iritkatriel @encukou @vstinner

@JelleZijlstra
Copy link
Member

Yes, it should probably suppress AttributeError only; I suspect it was simply written this way because it's easier. I think we can probably afford to change this, but in 3.14 only.

Also, we should make sure to do this consistently for all special methods with fallbacks.

@kcdodd
Copy link
Contributor Author

kcdodd commented Mar 14, 2025

So far I think that there are sort of several categories of special methods, at least in terms of internal details, and I think it would be a larger effort to try to get similar "fallback" behavior from raising AttributeError from a descriptor in all cases. These are my current understanding:

  • "optional" special methods - These have predefined (internal) fallbacks like __eq__ , __hash__, __repr__, __iter__; as far as I can tell these all have very similar internal path, and ones that were clearing the errors, and have prototyped a resolution.
  • Non-optional comparisons like __lt__ etc that can fallback via reflecting binary operations; via "richcompare" these would get the same implementation as __eq__, unless there was different special case based on the op.
  • "semi-optional" special methods - Like __str__, I am not sure exactly where it happens, but it falls back to __repr__. However, it doesn't currently go through the same code path as __repr__, so an AttributeError doesn't cause fallback.
  • Other binary operators like __add__ etc. that can fallback via reflecting, but the lookups to these don't go through the same path, so special casing AttributeError would not behave the same way.
  • Inherited special methods like __format__ - fallback basically means traversing the MRO.

Honestly, I would actually advocate against fallbacks from raising AttributeError from a descriptor (in principle). It's not really possible to know where an AttributeError came from, or whether it was intended to trigger a fallback. Just my opinion, but I think returning NotImplemented from __get__ would be a more reasonable way to "un-get" the descriptor if that were desired behavior.

To me it would be a compromise to not have other errors cause fallback. For some context, the embarrassing way I discovered this was a virtually infinite recursion error in a descriptor I made for __eq__ . It would return False (the default) at the stack limit, but could never make its way back up the stack because it would just bounce around at the stack limit between recursion and returning False.

@kcdodd
Copy link
Contributor Author

kcdodd commented Apr 28, 2025

This may have been addressed by #131588, but have not dug too deep into the changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

No branches or pull requests

3 participants