Uh oh!
There was an error while loading. Please reload this page.
bpo-45292: [PEP 654] add the ExceptionGroup and BaseExceptionGroup classes - #28569
Conversation
bedevere-bot
commented
Sep 26, 2021
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit c26ff6b38333ab6c990791628f827c5ab97998c2 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
bedevere-bot
commented
Sep 26, 2021
🤖 New build scheduled with the buildbot fleet by @pablogsal for commit c26ff6b38333ab6c990791628f827c5ab97998c2 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
4907240 to
6c6164aCompareiritkatriel
commented
Sep 26, 2021
This is part 1 - it rebased very easily so not much has changed in the area since April. |
gvanrossum
left a comment
There was a problem hiding this comment.
Make sure you run the buildbot for leak checks.
I'd recommend asking for a review from a core dev who is better than me at catching subtle mistakes in C code (Serhiy?).
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
iritkatriel
commented
Sep 27, 2021
Done.
Would be great to have a review from @serhiy-storchaka if he's got the time and interest, and also @vstinner and @ncoghlan . |
4b9c502 to
28d458fCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| self.assertIs( | ||
| type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG) |
There was a problem hiding this comment.
This also seems weird to me: I'd expect that if I want MyEG to hold a BaseException, I'd better create a MyBaseEG to do so, ala:
classMyBEG(BaseExceptionGroup):
@staticmethoddefcreate(msg, excs):
# per creation suggestion aboveifall(isinstance(e, Exception) foreinexcs):
returnMyEG(msg, excs)
returnMyBEG(msg, excs)
classMyEG(MyBEG, ExceptionGroup):
passThere was a problem hiding this comment.
For user-defined subclasses, we have to punt on the special behavior. And yes, user classes should probably do what you show, but we don't want to enforce it. (That would be making assumptions.)
There was a problem hiding this comment.
Fair enough!
IMO defining custom ExceptionGroup subclasses should be very rare anyway (maybe just Hypothesis and Trio?), and we can certainly deal with whatever complexity that requires.
There was a problem hiding this comment.
Eh, why would Hypothesis need to subclass EG? While we were called on arbitrarily forbidding subclassing (as we did in an earlier PEP draft) I don't see why it would be useful. What is your use case? Maybe there's something we can add to the design to avoid the need? (For Trio I could imagine they'd want to provide some kind of backwards compatibility with Trio's current solution.)
There was a problem hiding this comment.
Maybe a subclass to annotate the nested exceptions? See https://discuss.python.org/t/accepting-pep-654-exception-groups-and-except/10813/9
There was a problem hiding this comment.
I'd prefer not to change the exception type if we can avoid it, but the more important objection is that it complicates the reporting solely for implementation reasons:
classExplanation(Exception):
__module__="builtins"def__str__(self) ->str:
returnf"\n{self.args[0]}"try:
why="Failed!"raiseAssertionError(why)
exceptExceptionase:
msg=" You can reproduce this error by ...\n ..."raiseExplanation(msg) frome# Ideally something more like:e.__note__=msgraise$ python example.py
Traceback (most recent call last):
File "example.py", line 8, in <module>
raise AssertionError(why)
AssertionError: Failed!
# These lines are
The above exception was the direct cause of the following exception: # confusing for new # users, and they
Traceback (most recent call last): # only exist due File "example.py", line 10, in <module> # to implementation
raise Explanation(msg) from e # constraints :-(
Explanation: # You can reproduce this error by ...
...
There was a problem hiding this comment.
Hm, that does not look very principled. Why make it possible to print the
__note__when an exception is printed as the child of an EG but not when it's printed at the toplevel? Maybe Hypotheses could wrap exceptions in some kind of proxy exception?
I'm assuming that the note describes the leaf's membership in the EG rather than the leaf itself. If that's not the case I agree it doesn't make sense.
There was a problem hiding this comment.
The note is a description of the leaf, which could equally well be used for a single bare exception (though print() suffices).
There was a problem hiding this comment.
Sounds like the requirement is to add a feature to BaseException to enrich exceptions in a way that shows up in their __str__.
There was a problem hiding this comment.
I've created bpo45607 to track this request from Zac.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| self.assertIs( | ||
| type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG) |
There was a problem hiding this comment.
Eh, why would Hypothesis need to subclass EG? While we were called on arbitrarily forbidding subclassing (as we did in an earlier PEP draft) I don't see why it would be useful. What is your use case? Maybe there's something we can add to the design to avoid the need? (For Trio I could imagine they'd want to provide some kind of backwards compatibility with Trio's current solution.)
gvanrossum
left a comment
There was a problem hiding this comment.
@iritkatriel, have you managed to convince another core dev to review exceptions.c yet?
Uh oh!
There was an error while loading. Please reload this page.
| self.assertIs( | ||
| type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG) |
There was a problem hiding this comment.
Okay, I'll bite. What would you like to see supported upstream? A separate metadata item for each sub-exception? I'd say if you don't want to store that on the sub-exception in some dunder-ish attribute, you could indeed subclass EG to add that. There's an API so you can replicate this whenever an EG is being filtered. But I don't think we're going to support metadata that automatically gets replicated based on just this example -- in most cases I suspect you can just store an extra attribute on the individual exceptions.
iritkatriel
commented
Oct 5, 2021
Not that I know of. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
iritkatriel
commented
Oct 8, 2021
A PR for the traceback display code is here: iritkatriel#31 Once this PR is merged I will make a PR from that branch into main. But in the meantime it can be reviewed there, diffed against this PR. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| self.assertIs( | ||
| type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG) |
There was a problem hiding this comment.
Anyway, if you want to override the way the traceback gets printed, subclassing EG isn't enough -- traceback-printing is (a) built into the C code, and (b) reimplemented in pure Python in traceback.py. To get something extra printed (e.g. err.__note__ if it exists) you will have to write your own traceback-printing code -- though there are various reusable bits and pieces in traceback.py.
| self.assertIs( | ||
| type(MyEG("eg", [ValueError(12), KeyboardInterrupt(42)])), MyEG) |
There was a problem hiding this comment.
Anyway, if you want to override the way the traceback gets printed, subclassing EG isn't enough -- traceback-printing is (a) built into the C code, and (b) reimplemented in pure Python in traceback.py. To get something extra printed (e.g. err.__note__ if it exists) you will have to write your own traceback-printing code and invoke it at the right point -- though there are various reusable bits and pieces in traceback.py.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…an error. Tidy up error handling.
fee53c9 to
dbd72d1Compareiritkatriel
commented
Oct 22, 2021
(rebased) |
bedevere-bot
commented
Oct 22, 2021
🤖 New build scheduled with the buildbot fleet by @iritkatriel for commit dbd72d1 🤖 If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again. |
gvanrossum
commented
Oct 23, 2021
“And so it begins…” |
1st1
commented
Oct 23, 2021
@iritkatriel Congrats Irit. This is very impressive work. |
This adds the ExceptionGroup and BaseExceptionGroup classes, but does not yet update the traceback display code to work with them correctly.
https://bugs.python.org/issue45292