Skip to content
This repository was archived by the owner on Apr 10, 2022. It is now read-only.

Address feedback from Irit and Guido. #5

Merged
merged 8 commits into from
Oct 27, 2020
Merged

Address feedback from Irit and Guido. #5

merged 8 commits into from
Oct 27, 2020

Conversation

1st1
Copy link
Member

@1st1 1st1 commented Oct 26, 2020

No description provided.

@1st1 1st1 mentioned this pull request Oct 27, 2020
Copy link
Member

@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.

Thanks, agreed this is a much better use of GitHub!

I didn't read everything, but here are some comments.

Also, we should try to describe how tracebacks are handled by the various operations on ExceptionGroup (e.g. constructor, split, join).


```python
try:
raise *(ValueError('a'), ValueError('b'), TypeError('z'))
raise ExceptionGroup(ValueError('a'), ValueError('b'), TypeError('z'))
Copy link
Member

Choose a reason for hiding this comment

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

Did we discuss whether the constructor takes a list of exceptions or uses varargs (*args)?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, not yet, I believe. I guess we need to open a new issue to discuss the ExceptionGroup API in detail and later fold it into this proposal. @iritkatriel?

Copy link
Member

Choose a reason for hiding this comment

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

I think varargs is nice. In reality I think you would want to create it empty and add exceptions to it. But whichever way it's constructed - what happens when you try to raise an empty ExceptionGroup?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think varargs is nice. In reality I think you would want to create it empty and add exceptions to it. But whichever way it's constructed - what happens when you try to raise an empty ExceptionGroup?

I've created an issue to discuss that: #6

except_star.md Outdated
Comment on lines 388 to 399
# would crash with:
#
# ExceptionGroup(
# ValueError('a'),
# ExceptionGroup(
# TypeError('b'),
# TypeError('c'),
# ),
# ExceptionGroup(
# KeyError('d')
# )
# )
Copy link
Member

Choose a reason for hiding this comment

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

Yikes! But why would it keep that group? If you start with ExceptionGroup(A(), B()) and you write

except *B as e:
    raise e

I hope you wouldn't end up with ExceptionGroup(A(), ExceptionGroup(B()))?

Copy link
Member Author

Choose a reason for hiding this comment

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

I hoped you would comment on this! I feel like this is one of the trickiest behaviors for us to define.

If you start with ExceptionGroup(A(), B()) and you write

except *B as e:
    raise e

I hope you wouldn't end up with ExceptionGroup(A(), ExceptionGroup(B()))?

I think it should end up with ExceptionGroup(A(), ExceptionGroup(B())). What if we change your code to:

except *B as e:
    e.marker = 'spam'
    raise e

I would expect the marker='spam' attribute to be present somewhere in what propagates out of the try block. And if we merge e back and raise ExceptionGroup(A(), B()) in the end this attribute will be lost.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Hm... But should you be doing that? In a sense ‘e’ is ephemeral, only for use in this one block. Surely if you mutate ‘e’ and then use bare raise it should get lost? (Hm, how can we describe bare raise? Ideally it should push the exceptions extracted back exactly from where they were extracted. Maybe the EG API needs to be described more carefully.)

Copy link
Member Author

@1st1 1st1 Oct 27, 2020

Choose a reason for hiding this comment

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

Surely if you mutate ‘e’ and then use bare raise it should get lost?

That's a good point. So you propose that raise e would be equivalent to raise. If the user wants to "flatten" the hierarchy that would explicitly say raise ExceptionGroup(*e).

I like this idea because it makes it harder for users to accidentally flatten parts of their exceptions tree. The current wording was motivated by the e.marker = 'spam' example, but if we treat e as an ephemeral object we can perhaps ignore this use case.

This is also an area where a linter (or mypy, easily) can warn the user that they're working with an ephemeral object and that their changes or references to it will not persist the way they expect them to.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've pushed another update erasing any semantic difference between raise and raise e.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, you mean that bare raise doesn't update the traceback, whereas raise e would add that very "raise e" line to the traceback, right? Or is there something about e.__context__?

Copy link
Member

Choose a reason for hiding this comment

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

Both.

Copy link
Member Author

Choose a reason for hiding this comment

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

Guido, can you explain re __context__? I don't see any difference between raise and raise e here:

def foo():
    try:
        1 / 0
    except ZeroDivisionError:
        try:
            raise ValueError
        except Exception as e:
            raise e

try:
    foo()
except Exception as e:
    print(type(e), type(e.__context__))

# always prints "<class 'ValueError'> <class 'ZeroDivisionError'>"

or here:

def foo():
    try:
        1 / 0
    except ZeroDivisionError as e:
        raise e

try:
    foo()
except Exception as e:
    print(type(e), type(e.__context__))

# always prints "<class 'ZeroDivisionError'> <class 'NoneType'>"

Copy link
Member

Choose a reason for hiding this comment

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

It's not __context__ but the traceback that's different. Compare:

(v39) ~$ cat -n a.py; python a.py 
     1	def foo():
     2	    try:
     3	        1/0
     4	    except ZeroDivisionError as e:
     5	        raise e
     6	
     7	foo()
Traceback (most recent call last):
  File "/Users/guido/a.py", line 7, in <module>
    foo()
  File "/Users/guido/a.py", line 5, in foo
    raise e
  File "/Users/guido/a.py", line 3, in foo
    1/0
ZeroDivisionError: division by zero

to

(v39) ~$ cat -n b.py; python b.py 
     1	def foo():
     2	    try:
     3	        1/0
     4	    except ZeroDivisionError as e:
     5	        raise
     6	
     7	foo()
Traceback (most recent call last):
  File "/Users/guido/b.py", line 7, in <module>
    foo()
  File "/Users/guido/b.py", line 3, in foo
    1/0
ZeroDivisionError: division by zero

Copy link
Member Author

Choose a reason for hiding this comment

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

Got it, i'll push a commit clarifying this soon.

except_star.md Outdated
Comment on lines 444 to 445
We propose to replicate this behavior in the `except*` syntax as it is useful
as an escape hatch when it's clear that all exceptions can be silenced.
Copy link
Member

Choose a reason for hiding this comment

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

Unless you only allow return without expression that's not enough -- what would happen here?

try:
    raise ExceptionGroup(A(), B())
except *A:
    return 1
except *B:
    return 2

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, great catch. I think we should only allow bare return. I'll add a clarification.

(FWIW I never use this feature myself)

Copy link
Member Author

Choose a reason for hiding this comment

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

what would happen here?

I think it would return 1, because A() would be matched first. But when running async code this is too fragile and would only allow users to write unpredictable code. IMO we should disable non-None returns.

Copy link
Member

Choose a reason for hiding this comment

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

There’s room to disagree here, but I would prefer not allowing any return statements over only allowing bare return. I worry that allowing bare return with the proposed semantics is closing the door for assigning any semantics to return with expression and for break/continue.

Copy link
Member Author

Choose a reason for hiding this comment

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

but I would prefer not allowing any return statements over only allowing bare return

TBH I'd prefer that too. As I said earlier I myself don't use this feature. Relying on a single "return" buried in finally or some except smells like bad code to me. But I'm also not sure how popular this idiom is: I can only recall seeing it once or twice in the wild.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just pushed an update prohibiting returns in except* altogether.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in 610b4db

@1st1
Copy link
Member Author

1st1 commented Oct 27, 2020

@gvanrossum

Also, we should try to describe how tracebacks are handled by the various operations on ExceptionGroup (e.g. constructor, split, join).

We absolutely should. @iritkatriel would you like to contribute a section? Trio's MultiError API would be something to at least take a look at.

Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

I committed some minor edits. In addition:

  1. The two code snippets under "Raising an ExceptionGroup introduces nesting:" should come after the "Unmatched exceptions" section because they don't quite make sense yet where they are.

  2. around line 205: "(with the group of unprocessed exceptions referenced via the context attribute.)" We are now in the middle of processing an exception group, and this made me pause and ask "unprocessed at which point in time?" This needs to be made more precise, probably with pseudo code that covers all cases.

  3. I'll try to write up the section on tracebacks.

@1st1 1st1 merged commit e3a383e into master Oct 27, 2020
@1st1 1st1 deleted the fixes branch October 27, 2020 16:32
@1st1
Copy link
Member Author

1st1 commented Oct 27, 2020

@iritkatriel

I committed some minor edits. In addition:

Awesome, thanks!

The two code snippets under "Raising an ExceptionGroup introduces nesting:" should come after the "Unmatched exceptions" section because they don't quite make sense yet where they are.

I've reordered the sections.

around line 205: "(with the group of unprocessed exceptions referenced via the context attribute.)" We are now in the middle of processing an exception group, and this made me pause and ask "unprocessed at which point in time?" This needs to be made more precise, probably with pseudo code that covers all cases.

I've updated the language there.

@1st1
Copy link
Member Author

1st1 commented Oct 27, 2020

@iritkatriel @gvanrossum thanks for the thorough reviews! I've merged this pr. Here's the rendered document: https://github.com/python/exceptiongroups/blob/master/except_star.md

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants