Repository navigation
Subclasses of ExceptionGroup can wrap BaseExceptions #99553
Description
class MyEG(ExceptionGroup):
"""Holds BaseExceptions without itself being a BaseException."""
oops = MyEG("oops", [KeyboardInterrupt()])
assert isinstance(oops, Exception)
assert not isinstance(oops.exceptions[0], Exception)I believe that this is a bug tracing to the period when PEP-654 did not intend (Base)ExceptionGroup to be usable as a parent class; and I think a sufficient fix would be to replace the type-equality check with an isinstance check in:
Lines 740 to 744 in bc390dd
cc @iritkatriel; raised via agronholm/exceptiongroup#40 (comment)
Linked PRs
- gh-99553: fix bug where an ExceptionGroup subclass can wrap a BaseException #99572
- [3.11] gh-99553: fix bug where an ExceptionGroup subclass can wrap a BaseException (GH-99572) #99580
- gh-99553: add tests for ExceptionGroup wrapping #99615
- [3.11] gh-99553: add tests for ExceptionGroup wrapping (GH-99615) #103435
Activity
iritkatriel commentedon Nov 17, 2022
I believe we discussed this and decided that we leave it to the subclass author to deal with it. Maybe it needs to be documented (or reconsidered)?
CC @gvanrossum @1st1
Zac-HD commentedon Nov 17, 2022
I think at least documented, though I'd prefer it was changed.
If people intend
MyEGto containBaseExceptions then the obvious way is to inherit fromBaseExceptionGroup(and likely have aBaseMyEg; Trio has already shipped this pattern), so any cases whereMyEGends up containing e.g. aKeyboardInterruptand then discarding that to anexcept Exception:would be unfortunate! I think the PEP argues against this well 🙂The
==comparison also makes static typing very confusing: if we haveBaseExceptionGroup->ExceptionGroup->MyEGand the last can also containBaseExceptions, then eitherExceptionGroup.exceptionsmust be annotated as able to containBaseExceptions when it cannot, the annotations forMyEGmust say it cannot when in fact it can, or we violate the substitution principle. This last is currently the case at runtime, but typecheckers are not keen on such things.Zac-HD commentedon Nov 17, 2022
Hah! It looks like I just find this persistently startling: #28569 (comment) (but I promise, no PEP this time 😉)
gvanrossum commentedon Nov 17, 2022
Yeah, I definitely recall that we discussed this before. But looking at the code it's possible that we were too conservative. In particular in the fallback clause we could have chosen to check if
clsis a subclass ofPyExc_ExceptionGroupifnested_base_exceptionsis set and raise the same error as on line 742.The thing we definitely can't do is adjusting the class if it is exactly
PyExc_BaseExceptionGroupandnested_base_exceptionsis false.The question is, can we treat this as a bug and fix it (in 3.11.1 if it isn't out yet) or would such a fix be considered backwards compatible?
iritkatriel commentedon Nov 17, 2022
I think we can call it a bug. It’s better to change it in 3.11.1 than in 3.12.
gvanrossum commentedon Nov 17, 2022
Sounds good.
pythongh-99553: fix bug where an ExceptionGroup subclass can wrap a B…
gh-99553: fix bug where an ExceptionGroup subclass can wrap a BaseExc…
pythongh-99553: fix bug where an ExceptionGroup subclass can wrap a B…
gh-99553: fix bug where an ExceptionGroup subclass can wrap a BaseExc…
pythongh-99553: clarify doc and add tests
gh-99553: add tests for ExceptionGroup wrapping (#99615)
pythongh-99553: add tests for ExceptionGroup wrapping (pythonGH-99615)
gh-99553: add tests for ExceptionGroup wrapping (GH-99615)
pythongh-99553: add tests for ExceptionGroup wrapping (python#99615)
pythongh-99553: add tests for ExceptionGroup wrapping (python#99615)