[doc] clarify that except does not match virtual subclasses of the specified exception type

[577e5a5d-caa3-40c5-8bd6-665e4097b609]

BPO	12029
Nosy	@gvanrossum, @warsaw, @ncoghlan, @pitrou, @benjaminp, @jwilk, @merwok, @alex, @bitdancer, @Trundle, @durban, @ericsnowcurrently, @berkerpeksag, @vadmium, @jimjjewett, @serhiy-storchaka, @charettes, @MojoVampire, @mbmccoy, @iritkatriel
PRs	python/cpython#6461 python/cpython#32027 python/cpython#32034 python/cpython#32035
Dependencies	bpo-22540: speed up isinstance and issubclass for the usual cases
Files	issue12029.patch exception_proper_subclass_matching_v2.patch exception_proper_subclass_matching_v3.patch pyobject_issubclass_isinstance_speedup.patch exception_proper_subclass_matching_v4.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields: ```python assignee = None closed_at = created_at = labels = ['interpreter-core', '3.9', '3.10', '3.11', 'type-feature', 'docs'] title = '[doc] clarify that except does not match virtual subclasses of the specified exception type' updated_at = user = 'https://bugs.python.org/acooke' ``` bugs.python.org fields: ```python activity = actor = 'iritkatriel' assignee = 'docs@python' closed = True closed_date = closer = 'iritkatriel' components = ['Documentation', 'Interpreter Core'] creation = creator = 'acooke' dependencies = ['22540'] files = ['25548', '36777', '36778', '36779', '36780'] hgrepos = [] issue_num = 12029 keywords = ['patch'] message_count = 51.0 messages = ['135521', '136712', '136717', '160399', '160418', '160426', '160427', '160428', '160430', '160432', '160435', '160436', '160461', '160468', '161473', '200418', '200420', '200421', '200829', '201329', '202019', '205830', '205880', '228158', '228159', '228171', '228195', '228196', '228198', '228209', '228213', '228214', '228215', '228216', '228219', '234434', '253270', '253272', '253288', '253958', '273436', '315257', '315267', '315268', '412778', '412798', '412799', '412801', '415700', '415705', '415707'] nosy_count = 28.0 nosy_names = ['gvanrossum', 'barry', 'jamesh', 'ncoghlan', 'pitrou', 'fijal', 'benjamin.peterson', 'jwilk', 'eric.araujo', 'alex', 'r.david.murray', 'Trundle', 'cvrebert', 'daniel.urban', 'yorik.sar', 'docs@python', 'Yury.Selivanov', 'eric.snow', 'berker.peksag', 'martin.panter', 'Jim.Jewett', 'serhiy.storchaka', 'gcbirzan', 'charettes', 'josh.r', 'V\xc3\xa1clav Dvo\xc5\x99\xc3\xa1k', 'Michael McCoy', 'iritkatriel'] pr_nums = ['6461', '32027', '32034', '32035'] priority = 'normal' resolution = 'fixed' stage = 'resolved' status = 'closed' superseder = None type = 'enhancement' url = 'https://bugs.python.org/issue12029' versions = ['Python 3.9', 'Python 3.10', 'Python 3.11'] ```

[577e5a5d-caa3-40c5-8bd6-665e4097b609]

Hi,

In general, registering a class with an ABC is equivalent to making it a subclass (isinstance and issubclass are patched through ABCMeta). However, this does not work for exceptions (see example below, where exception is not caught).

This doesn't seem terribly surprising to me - I imagine that checking would slow down exception handling - but I couldn't find any documentation (and posting on c.l.p didn't turn up anything either).

So I thought I would raise it here - perhaps there is a possible fix (my obscure use case is that I have a backtracking search; backtracking occurs when a certain exception is encountered; making that exception an ABC and allowing existing exceptions to be registered with it allows the search to work with existing code without a wrapper that catches and translates exceptions that should trigger a backtrack). Or perhaps the docs could be extended. Or perhaps I've misunderstood something...

Cheers, Andrew

Python 3.2 (r32:88445, Feb 27 2011, 13:00:05) 
[GCC 4.5.0 20100604 [gcc-4_5-branch revision 160292]] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> from abc import ABCMeta
>>> class RootException(Exception,metaclass=ABCMeta): pass
... 
>>> class MyException(Exception): pass
... 
>>> RootException.register(MyException)
>>> try:
...     raise MyException
... except RootException:
...     print('caught')
... 
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
__main__.MyException

[77411a08-770c-471e-ba30-9528530a8d45]

Scouting around the CPython codebase a bit, I speculate that the cause of this behavior is that PyErr_GivenExceptionMatches() in errors.c uses PyTypeIsSubtype() [which simply walks a class's \_mro checking for pointer equality] rather than PyObject_IsSubclass()/PyObjectIsInstance() [which are smart enough to consult \_subclasscheck()/instancecheck() if they exist].

Of course, the more important issue here is whether this behavior is intended or not. I surmise python-dev needs to have a discussion about it?

[77411a08-770c-471e-ba30-9528530a8d45]

Surveying the docs, the current behavior *is* /technically/ correct (in a suspiciously precise way) according to the Language Reference: http://docs.python.org/dev/reference/compound_stmts.html#grammar-token-try_stmt : "For an except clause with an expression [...] the clause matches the exception if the resulting object is 'compatible' with the exception. An object is compatible with an exception if it is the class or a base class of the exception object" (which exactly describes what PyType_IsSubtype() checks for)

The Tutorial is by contrast much more vague: http://docs.python.org/dev/tutorial/errors.html#handling-exceptions : "if [the raised exception's] type matches the exception named after the except keyword, the except clause is executed, and then execution continues after the try statement." No definition of what it means for the types to "match" seems to be given.

[e3884a6a-06d4-4bc7-a44b-15a19b7aa84a]

The documentation for ABCMeta.register() says that it makes the other class a "virtual subclass". That would make the ABC a "virtual base class".

So whether the current behaviour is correct depends on whether you consider a "virtual base" to count as a base. From the reasoning behind the introduction of ABCs, it certainly sounds like it should count.

Also, this is a feature that works correctly in Pyton 2.7, so could trip people up who are trying to move to Python 3.

[gvanrossum]

I agree it's a bug and should be fixed. It's too confusing that there would be two slightly different interpretations of isinstance/issubclass where the isinstance() and issubclass() would be using the extended interpretation but the except clause would use the narrow interpretation.

The exception matching done by the except clause ought to be explainable in terms of issubclass/isinstance.

[benjaminp]

I think being able to catch exception with ABCs is esssentially useless. The originally stated "usecase" can be simply solved by putting classes into a tuple and putting that in the except clause.

In general, the whole abc machinary causes lots of code which expects instance and subclass checks to be side-effect free to be able to execute arbitrary code, which creates messes.

[pitrou]

Perhaps ABCMeta could raise a UserWarning when creating an Exception subclass?

[577e5a5d-caa3-40c5-8bd6-665e4097b609]

perhaps it could just work in a simple, consistent way?

in my original report i wondered whether there was a significant performance hit. but so far the objections against fixing this seem to be (1) a lawyer could be convinced the current behaviour is consistent with the docs (2) python 3 should remain compatible with python 2 (3) abcmeta is the sucksorz.

those don't seem like great arguments against making it just work right, to me.

[pitrou]

perhaps it could just work in a simple, consistent way?

That would be best obviously. But as Benjamin explained it's quite delicate to make it work while avoiding pitfalls where code involved in exception checking may itself fail with arbitrary errors - say, enter an infinite recursion. It's also why I think it would be a bad idea to fix it in 3.2 (the bugfix branch). In 3.3 we can take riskier decisions.

[benjaminp]

Basically, someone needs to produce a patch and we can go from there.

[eaf08195-02b1-4629-aae0-97d158c7f815]

I posted on python dev that this would slow exception checking considerably so that is a concern. As for possible bugs, this has been working in the 2 branch for a while now, so I don't think that is the biggest issue. As for possible use cases, writing a wrapper around backend, each with its own exceptions and still being able to catch a 'base' exception in your code while still having the ability to catch specific exceptions, without doing awkward stuff like looking at __cause__ (let alone that you have to reraise that in 2 for code that has to run on both branches). Yes, you could patch the exceptions' bases but that is what Abc was created to avoid.

Sorry for the mistakes and weird phrasing, posting this off my phone.

[eaf08195-02b1-4629-aae0-97d158c7f815]

I have a patch, with tests, but no Internet on my computer so going out, will post it when I get back/my Internet comes back

[e3884a6a-06d4-4bc7-a44b-15a19b7aa84a]

Benjamin: if you are after a use case for this feature, see https://code.djangoproject.com/ticket/15901

In Django, there are multiple database backends, each of which currently catch the adapter's DatabaseError and reraise it as Django's DatabaseError so that Django code can handle database errors in a standard way without having to care about which backend they came from. Unfortunately, this loses some information from the exception.

My idea for solving that bug was to make Django's DatabaseError an ABC. By registering the various adapter's DatabaseErrors with the ABC, it would not be necessary to catch and reraise them in the backends while still preserving the ability to catch the generic errors in the core. This works fine in Python 2.x, but it was pointed out that it would cause compatibility problems when porting to Python 3.2.

[eaf08195-02b1-4629-aae0-97d158c7f815]

As promissed the patch. It doesn't break any tests, and it passes the ones I added. I have a pybench one as well, which even though trivial, does point to the fact that there is a degradation in performance, but not sure it's worth posting here.

[74c4563b-ab1c-43d8-9219-30c4eca796bc]

When does the performance hit occur?

If it is only when an exception has been raised, and its own class is not listed by the except clause, then I personally wouldn't worry about it; tracing the MRO *could* get arbitrarily long already; it just doesn't in practice. The same should be true of virtual subclassing.

On the other hand, if it adds another module or three to the required startup set, that might be a concern...

[ncoghlan]

The performance hit is that such a change would potentially make it more expensive to figure out that a raised exception *doesn't* match a given "except" clause, along with the complexity of introducing execution of arbitrary code while still unwinding the stack looking for an exception handler for the original exception.

As Benjamin noted above we already support dynamic exception handling through dynamically bound tuple lookups, so I don't think this feature is needed for the Django used case:

>>> caught_exceptions = ()
>>> def f(to_raise):
...     try:
...         raise to_raise
...     except caught_exceptions:
...         print("Caught the exception")
... 
>>> f(Exception)
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "<stdin>", line 3, in f
Exception
>>> caught_exceptions = (Exception,)
>>> f(Exception)
Caught the exception

I know Guido indicated above that he considers the current behaviour a bug, and I even agree that enshrining the two slightly different definitions of "issubclass" is ugly, but the complete lack of use cases without other solutions and the complex implications of unifying them mean that I think it may be worth accepting the additional complexity in the language definition instead.

That means the question in my mind is whether we can make it less surprising/user-hostile by issuing a warning at class definition time.

Since all exceptions are required to inherit from BaseException in order to be permitted in raise statements *or* except clauses, it seems to me that having a check in PyType ready that emits a warning when it detects the use of the virtual subclass machinery should suffice. That is, all of these should emit a warning:

  class BadExceptionABC_1(BaseException, metaclass=abc.ABCMeta): pass
  class BadExceptionABC_2(abc.ABC, BaseException): pass
  class BadExceptionABC_3(BaseException):
      def __instancecheck__(*args): return False
  class BadExceptionABC_4(BaseException):
      def __subclasscheck__(*args): return False

We could even go further and make it a DeprecationWarning intially and upgrade to a full TypeError in a later release (although we obviously can't do that if Guido would prefer to unify the behaviour instead).

[ncoghlan]

Actually, I take back the performance comment - Jim's right that in the normal case, the slowdown should be minimal (it's just some additional checks that certain slots aren't populated on each listed exception class), and types can already influence that by messing with the MRO.

That just leaves the complexity argument associated with running arbitrary code at an unexpected place in the eval loop, and for that the tests in the patch would need to be strengthened with some pathological examples like:

• __subclasscheck__ raising an exception
• __subclasscheck__ raising and then suppressing an exception
• __subclasscheck__ invoking the garbage collector
• __subclasscheck__ hitting the recursion limit
• __subclasscheck__ provoking MemoryError

The games the current patch already has to play with the recursion limit also bother me.

However, if an alternate approach could be found that avoids the adjustment of the recursion limit in the eval loop, that also sensibly survived the kinds of arbitrary code execution torture tests I mention above, then I'd be far more sanguine about the idea of actually resolving the discrepancy rather than formalising it as part of the general fact that the exception hierarchy isn't as flexible as most of the rest of the language.

[pitrou]

Actually, I take back the performance comment - Jim's right that in the normal case, the slowdown should be minimal (it's just some additional checks that certain slots aren't populated on each listed exception class), and types can already influence that by messing with the MRO.

I think that the performance question can only really be answered by running (micro-)benchmarks here.

[9dde2db0-f7a5-41f9-b1f9-2c5e7a7b5954]

Can someone please point out why do we have to do that dance with recursion limit?

I've came upon this problem as well. I had some (bad) API I had to work with. It always raised the same exception with the only difference in the message. So I thought I could do something like this:

def message_contains(msg):
    class _MyExc(object):
        def __instancecheck__(self, exc):
            return msg in exc.args[0]
    return _MyExc

But after I tried it in number of different ways I found out that it's not possible.

So here's another reason to change this behavior.

[ncoghlan]

Because context managers are closer to try/finally blocks than they are to exception handling, the class-based implementation for the contextlib.suppress API uses issubclass rather than emulating the CPython exception handling semantics: http://hg.python.org/cpython/file/09153a9a3bb9/Lib/contextlib.py#l202

The exception checking in the unittest module is similarly based on issubclass: http://hg.python.org/cpython/file/09153a9a3bb9/Lib/unittest/case.py#l129

I'm planning to add the catch() and ExitLabel() context managers to contextlib2 this evening, and those too will be based on issubclass().

Perhaps as a near term thing, we should put an "implementation detail" notice somewhere in the language reference, pointing that it's the code using issubclass that is considered correct here, and CPython's exception handling that is considered out of line?

[ncoghlan]

A point on the safety/correctness front: I remembered we already run arbitrary code at roughly this point in the eval loop, as we have to invoke __iter__ to get the exceptions to check when an iterable is used in except clause.

That means allowing the subclass check hooks to run here isn't as radical a change as I first thought.

[gvanrossum]

"I remembered we already run arbitrary code at roughly this point in the eval loop, as we have to invoke __iter__ to get the exceptions to check when an iterable is used in except clause."

Are you sure? IIRC the except clause only accept exceptions and tuples of exceptions, not iterators.

[ncoghlan]

Ah, you're right - I found the example I was thinking of (Richard Jones's "Don't do this!" talk), and it was just demonstrating that the except clause accepts any expressions producing a tuple or BaseException instance, not that we call __iter__ at that point.

And since we do identity checks for the exception type matching (rather than equality checks), it looks like all the avenues for arbitrary code execution while checking if an exception handler matches a thrown an exception are closed off.

[379dc349-3a10-424f-b9d2-a0104f092359]

"it looks like all the avenues for arbitrary code execution while checking if an exception handler matches a thrown an exception are closed off."

This seems to be directly contradicted by your previous sentence: "the except clause accepts any expressions producing a tuple or BaseException instance".

e.g.

\===

>>> def f(): raise AttributeError
... 
>>> try: raise IndexError
... except f(): raise KeyError
... 
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 1, in f
AttributeError

===

(note that f() is evaluated only if the body of "try" actually raises)

[gvanrossum]

ISTM Nick meant that the exception that was raised can't cause arbitrary code execution.

On Wednesday, October 1, 2014, Antony Lee \report@bugs.python.org\ wrote:

Antony Lee added the comment:

"it looks like all the avenues for arbitrary code execution while checking if an exception handler matches a thrown an exception are closed off."

This seems to be directly contradicted by your previous sentence: "the except clause accepts any expressions producing a tuple or BaseException instance".

e.g.

\===

>>> def f(): raise AttributeError
...
>>> try: raise IndexError
... except f(): raise KeyError
...
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
IndexError

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
  File "<stdin>", line 1, in f
AttributeError

===

(note that f() is evaluated only if the body of "try" actually raises)

---------- nosy: +Antony.Lee

---

Python tracker \<report@bugs.python.org \<javascript:;>> \http://bugs.python.org/issue12029\

---

[ncoghlan]

Right, I had a specific concern related to the way the C level code works. On closer inspection, it turned out all the Python level code execution is complete by the time we reach the point I was worried about.

[birkenfeld]

I'm attaching a patch that works without changing the recursion limit, and adds some tests for pathological cases.

Instead, PyErr_GivenExceptionMatches is changed so that if an exception was previously set, it is replaced by an exception that PyObject_IsSubclass raises. In that way recursion errors should be propagated properly.

In exception matching, this means that exceptions (including recursion errors) from PyObject_IsSubclass are ignored. There is already an explicit test for this behavior in test_exceptions.

This behavior *could* be changed if intended by introducing a variant of PyErr_GivenExceptionMatches that can set an exception even if none was set before, and calling that in cmp_outcome in ceval.

[birkenfeld]

New version including (I think) correct refcount handling.

[birkenfeld]

Clarifying some comments in this one.

[pitrou]

I'm worried about the runtime cost of this. Code like this is an extremely common idiom and should remain fast:

try: x = d[key] except KeyError: # do something else

[birkenfeld]

Agreed.

Since type has __subclasscheck__ (why I don't know) this might result in a slowdown since all checks have to go through calling it in PyObject_IsSubclass.

Just noticed that given_exception_matches_inner can be simplified a bit since PyObject_IsSubclass already checks for tuples by itself.

[birkenfeld]

Quick microbenchmark:

try: {}["a"]
except KeyError: pass

original tip: 1.35 usec with patch v3: 1.55 usec

so it's about 15% slowdown for catching a simple exception on my machine.

[pitrou]

Since type has __subclasscheck__ (why I don't know)

Ouch, really? That means the PyType_IsSubtype path in PyObject_IsSubclass() is never taken?

I think we should add a tp_subclasscheck slot to minimize the general cost of this. Also, try to see if there aren't redundant type / tuple checks along the way.

[birkenfeld]

OK, with these two patches (speedup and v4) I can't see a significant slowdown anymore.

[birkenfeld]

IsSubclass speedup patch now tracked in bpo-22540.

[9dde2db0-f7a5-41f9-b1f9-2c5e7a7b5954]

Can we move forward and land this patch? It seems to be working and for some reason it even makes that microbenchmark work faster.

[99ffcaa5-b43b-4e8e-a35e-9c890007b9cd]

Does this introduce a slowdown when the type doesn't match? That is, clearly George's example:

try: {}["a"]
except KeyError: pass

won't be slowed because the fast path will get an immediate hit. But what about:

try: {}["a"] except TypeError: pass except ValueError: pass except KeyError: pass

(or with the KeyError handler higher up the call stack). The fast path speeds up the handled case, but it doesn't seem like it would help the unhandled case (where it would need to check the slow path for each unhandled exception type one at a time).

[99ffcaa5-b43b-4e8e-a35e-9c890007b9cd]

On rereading bpo-22540, maybe that won't be an issue (in the common case where no custom metaclasses are used for the exceptions to be caught, it looks like maybe there is no slow path to traverse?). Still worth double checking.

[bitdancer]

Note from discussion on duplicate bpo-25448: when this is fixed, the try/except documentation should be updated to indicate that the except clause test is equivalent to 'issubclass', so that the handling of virtual subclasses are implied by the doc. (The current proposed patch contains no doc changes.)

Apparently this also affects pypy.

[vadmium]

In the current (v4) patch, it looks like there is an unneeded “allow_new_exception” parameter that is always set to zero. So maybe the patch needs more careful thought.

Also I agree it needs documentation, what’s new, “changed in version 3.6”, etc.

[ncoghlan]

This question came up again recently over in bpo-27814, in the context of a proposal to add an "unless" parameter to contextlib.suppress(). I declined the RFE mainly on the basis of API complexity, but I also noted you can get something comparable in the current API by using virtual subclassing to say "If a subclass of these, but not of these": http://bugs.python.org/issue27814#msg273434

So the status quo is currently giving us a slightly odd discrepancy between normal except clauses and code that emulates them via issubclass()

[serhiy-storchaka]

Silencing exceptions like MemoryError, RecursionError or KeyboardInterrupt and returning a lying result doesn't look like a good idea to me. These exceptions can be raised in virtually any code for causes not related to executed code. MemoryError -- if other parts of the program allocated too much memory, RecursionError -- if the exception check is performed too deep in the execution stack, KeyboardInterrupt -- for obvious reasons.

[722f1956-bbdb-416e-8eb8-283787897138]

Amalgamating the patch history here, I've updated the tests on Github (PR6160) to include tests for both the recursive case and ensure the correct error is propagated up if an exception occurs during the subclass check.

I've also added a check to ensure that unittest's assertRaises behaves as expected. (The test currently passes on master, which is a bug if this doesn't get merged.)

Finally, the PR updates the documentation for try/except.

[722f1956-bbdb-416e-8eb8-283787897138]

Sorry, my last message referred to Github PR6460 / pull_request6160.

[gvanrossum]

Fixing the version field. Since it's a feature, this could not go into any version before 3.11.

Maybe Irit can look through the discussion and patch and see if there's value to doing this? (Feel free to decline!)

[722f1956-bbdb-416e-8eb8-283787897138]

Checking my comment history here, a past me was terribly bad at linking the correct PR on github.This is the correct link: https://github.com/python/cpython/pull/6461

On Mon, Feb 7, 2022 at 10:12 AM Guido van Rossum \report@bugs.python.org\ wrote:

Guido van Rossum \guido@python.org\ added the comment:

Fixing the version field. Since it's a feature, this could not go into any version before 3.11.

Maybe Irit can look through the discussion and patch and see if there's value to doing this? (Feel free to decline!)

---------- nosy: +iritkatriel versions: +Python 3.11 -Python 3.4, Python 3.5, Python 3.6, Python 3.7, Python 3.8, Python 3.9

---

Python tracker \report@bugs.python.org\ \https://bugs.python.org/issue12029\

---

[iritkatriel]

To summarise the discussion so far:

The arguments in favour of changing exception matching to match on virtual base classes are:

1. It is confusing that it doesn't follow issubclass semantics.
2. Two use cases were presented as practical motivation.
   • one in msg135521, which can be solved with the pattern in msg200418
   • one in msg200829, which is typically done with except: if condition: handle raise

The arguments against the change are

1. safety - calling python code from the exception propagation code
2. possible performance impact

I am not too worried about the performance of exception handling. I am also not impressed by the use cases.

For me it's mostly between the safety issue and the aesthetic language consistency issue.

The code path from when a RAISE opcode executes and until control passes to an except clause, is a very sensitive one and I have a lot of sympathy to the position that we should just change the documentation to say that matching is on non-virtual base classes. It is much easier to implement this feature than to predict how it would behave in all cases.

If we do decide to implement this, then I don't think the patch is the way we should do it. If the IsSubclass call fails, this should result in a "goto error", like when the match type is invalid: https://github.com/python/cpython/blob/7ba1cc8049fbcb94ac039ab02522f78177130588/Python/ceval.c#L3831

This means that the failure to determine whether the exception is a match is the dominant error, rather than something we print to the ether via the unraisablehook and interpret as non-match.

[gvanrossum]

Thanks you. I think it's reasonable to reject the feature request and instead update the docs, so let's do that.

[iritkatriel]

New changeset 45833b50f0ccf2abb01304c900afee05b6d01b9e by Irit Katriel in branch 'main': bpo-12029: [doc] clarify that except does not match virtual subclasses of the specified exception type (GH-32027) https://github.com/python/cpython/commit/45833b50f0ccf2abb01304c900afee05b6d01b9e

[iritkatriel]

New changeset 7fc12540e3e873d8ff49711e70fd691185f977b9 by Irit Katriel in branch '3.10': bpo-12029: [doc] clarify that except does not match virtual subclasses of the specified exception type (GH-32027) (GH-32034) https://github.com/python/cpython/commit/7fc12540e3e873d8ff49711e70fd691185f977b9