[doc] clarify that except does not match virtual subclasses of the specified exception type #56238
Comments
|
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, 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 |
|
Scouting around the CPython codebase a bit, I speculate that the cause of this behavior is that PyErr_GivenExceptionMatches() in errors.c uses PyType_IsSubtype() [which simply walks a class's __mro__ checking for pointer equality] rather than PyObject_IsSubclass()/PyObject_IsInstance() [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? |
|
Surveying the docs, the current behavior *is* /technically/ correct (in a suspiciously precise way) according to the Language Reference: The Tutorial is by contrast much more vague: |
|
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. |
|
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. |
|
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. |
|
Perhaps ABCMeta could raise a UserWarning when creating an Exception subclass? |
|
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. |
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. |
|
Basically, someone needs to produce a patch and we can go from there. |
|
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. Sorry for the mistakes and weird phrasing, posting this off my phone. |
|
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 |
|
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. |
|
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. |
|
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... |
|
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 exceptionI 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 FalseWe 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). |
|
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:
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. |
I think that the performance question can only really be answered by |
|
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 _MyExcBut 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. |
|
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? |
|
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. |
|
Does this introduce a slowdown when the type doesn't match? That is, clearly George's example: try: won't be slowed because the fast path will get an immediate hit. But what about: try: (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). |
|
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. |
|
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. |
|
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. |
|
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() |
|
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. |
|
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. |
|
Sorry, my last message referred to Github PR6460 / pull_request6160. |
|
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!) |
|
Checking my comment history here, a past me was terribly bad at linking the On Mon, Feb 7, 2022 at 10:12 AM Guido van Rossum <report@bugs.python.org>
|
|
To summarise the discussion so far: The arguments in favour of changing exception matching to match on virtual base classes are:
The arguments against the change are
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: Line 3831 in 7ba1cc8 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. |
|
Thanks you. I think it's reasonable to reject the feature request and instead update the docs, so let's do that. |
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:
bugs.python.org fields:
The text was updated successfully, but these errors were encountered: