Skip to content
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

Exception group stage1 (by @iritkatriel) #20

Closed
wants to merge 52 commits into from

Conversation

gvanrossum
Copy link
Owner

This PR is just so I can more easily comment...

Copy link
Owner Author

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also for @1st1 to review.

Lib/types.py Outdated
@@ -300,4 +300,81 @@ def wrapped(*args, **kwargs):
NoneType = type(None)
NotImplementedType = type(NotImplemented)

class TracebackGroup:
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this new code (TbGroup and ExGroup) should probably live in a separate file, don't you think? types.py should not have actual class definitions (it must be importable blindingly fast).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also - do we want to keep the names ExceptionGroup and TracebackGroup?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let’s keep these for now.

PS. I’ll be AFK the rest of the day and much of the rest of the week.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK thanks. We'll try to make progress. Unit tests and the like.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try to make a context manager to handle exceptions, to exercise the join.

Lib/types.py Outdated
self.__traceback_group__ = TracebackGroup(self.excs, self.frame)

def split(self, E):
''' returns two new ExceptionGroups: match, rest
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might as well bring up now that PEP 8 recommends a different style for docstrings (one line summary, blank line, then more text as needed -- or just a one line summary; and always using """).

@@ -0,0 +1,282 @@
#
# This source file is part of the EdgeDB open source project.
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... @1st1, does all this need to stay in the file in case we copy it into the stdlib?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I'll contribute it in a clean way under PSFL.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, FWIW, the current code still doesn't work with EGs properly. If an EG contains a CancelledError, the TaskGroup would propagate the CancelledError, not the EG (thus discarding all other errors). To fix this we'll need to fix the asyncio itself to recognize CancelledErrors wrapped in a EG. I'll do that as soon as we have some basic high-level API.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Which makes me think that taskgroup.py doesn’t belong in this PR.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I copied it over from the previous branch but will remove.

Lib/types.py Outdated Show resolved Hide resolved
Lib/types.py Outdated Show resolved Hide resolved
Lib/types.py Outdated Show resolved Hide resolved
Lib/types.py Outdated
Comment on lines 374 to 378
def __str__(self):
return f"ExceptionGroup({self.excs})"

def __repr__(self):
return str(self)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you define just __repr__, the other will default to that.

Lib/types.py Outdated
self.__traceback__ = types.TracebackType(None, self.frame, 0, 0)
self.__traceback_group__ = TracebackGroup(self.excs, self.frame)

def split(self, E):
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that we have split(), what do you think join() will look like? A class method? I was also thinking perhaps it can be spelled as +.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need join? Or do we just make a new list of exceptions (some of which can be EGs) and construct a new EG (which takes care or creating the new TBG)?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure, but if you split and then join, the pieces the result should be the original EG no matter how the split turned out.

output.txt Outdated
@@ -0,0 +1,112 @@
Running Release|Win32 interpreter...
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Do we need this file? They typically go quickly out of sync with the code.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I'll remove it and the test script as soon as we have unit tests. Which should be.. soon

Copy link

@1st1 1st1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good start I think.

@iritkatriel
Copy link

The tests are a bit of a mess still, but their coverage is decent now.

@iritkatriel
Copy link

So the handler has some return value that means "naked raise", and it raises in all other cases?

Oh, that's a nice API -- the handler can choose whether to appear in the traceback or not.

Not sure we're talking about the same thing. In my version it will be in the traceback unless it returns "naked raise", which makes the catcher return False (as under the current if handler_excs is match: case).

@gvanrossum
Copy link
Owner Author

So the handler has some return value that means "naked raise", and it raises in all other cases?

Oh, that's a nice API -- the handler can choose whether to appear in the traceback or not.

Not sure we're talking about the same thing. In my version it will be in the traceback unless it returns "naked raise", which makes the catcher return False (as under the current if handler_excs is match: case).

Hm, if the handler creates a fresh exception and returns that, IIUC its frame won't be in the traceback; only raise e updates e.__traceback__. I guess when our __exit__ code decides to raise a different exception than what was passed in (because something was handled or because something was added) it must use raise <something> and then it will appear in the traceback.

Basically, don't pay attention to me, I am just talking to myself here. :-)

@iritkatriel
Copy link

Basically, don't pay attention to me, I am just talking to myself here. :-)

I'm talking to myself too, it's called 'conversation'. :)

@iritkatriel
Copy link

iritkatriel commented Nov 2, 2020

At the end of ExceptionGroupCatcher.__exit__ there's a trick I copied from trio where it patches up __context__ in the finally block. Can we mess with the traceback there as well, ie pop a frame from it?

@gvanrossum
Copy link
Owner Author

At the end of ExceptionGroupCatcher.__exit__ there's a trick I copied from trio where it patches up __context__ in the finally block. Can we mess with the traceback there as well, ie pop a frame from it?

I don't see why not. That's a clever trick, doing this in the finally after you've already raised it.

@iritkatriel
Copy link

The trick doesn't seem to work for traceback.

import sys
import traceback

def f():
    raise ValueError()

def g():
    try:
        f()
    finally:
        _, value, _ = sys.exc_info()
        value.__traceback__ = None

try:
    g()
except ValueError as e:
    traceback.print_tb(e.__traceback__)

Output (I expected f() to not be there):

Running Release|Win32 interpreter...
  File "C:\Users\User\src\cpython\x.py", line 16, in <module>
    g()
  File "C:\Users\User\src\cpython\x.py", line 9, in g
    f()
  File "C:\Users\User\src\cpython\x.py", line 5, in f
    raise ValueError()

@gvanrossum
Copy link
Owner Author

The trick doesn't seem to work for traceback.

Yeah, I discovered the same thing. Not sure why, possibly it has to do with the separation of exception type, value and traceback (maybe the tb is saved on the thread state while the finally block is running and restored from there, overwriting __traceback__).

match, _ = self.project(lambda e: e in keep)
return match

def extract_traceback(self, exc):

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we need this as part of the API. It's used in the tests and can move there.

@gvanrossum
Copy link
Owner Author

Hey all, I've been distracted away from this. I'd like to make sure we don't lose momentum -- I don't want this to just sit here and get stale until the next sprint. So... How are things going here? What should our next step be? How close is this to the proposal Yury wrote up immediately after the sprint?

@iritkatriel
Copy link

I think what we have here implements stage1 of the plan in python/exceptiongroups#9.
It follows the approach of trio, with the catcher context manager, so one thing we can perhaps do it to try and use this in trio to get an idea if it's making sense.

The next phase of the plan is to integrate the rendering from here into traceback.py. I can try to do that on another branch, unless we think that's premature at this point.

@github-actions
Copy link

github-actions bot commented Jan 7, 2021

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions bot added the stale label Jan 7, 2021
@iritkatriel
Copy link

I think we can close this PR - we have a more advanced version of the python-only implementation here: iritkatriel#3

@gvanrossum
Copy link
Owner Author

Agreed. This is really coming together -- thanks for plowing through!

@gvanrossum gvanrossum closed this Jan 8, 2021
@iritkatriel iritkatriel deleted the exceptionGroup-stage1 branch October 18, 2022 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants