-
Notifications
You must be signed in to change notification settings - Fork 1
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 stage4 - implement except* #7
base: main_old
Are you sure you want to change the base?
Conversation
e7337e8
to
5698710
Compare
d7a2b7f
to
2409b6f
Compare
e5129bf
to
1e1aea6
Compare
6e67e84
to
c6a79b7
Compare
…f a a singleton exception was wrapped into an ExceptionGroup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here are a few random comments. This PR is very big and I have not found the courage yet to try and understand every part of it, but the parts that I did check (in particular the parser and some of the tests) all check out.
with self.assertRaises(TypeError): | ||
try: | ||
raise OSError("blah") | ||
except *ExceptionGroup as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me -- is the recommended style except *E
, or is it except* E
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question, we started with except *E
, but I found myself gravitating towards except* E
because it feels like the *
is operating on the except and not on the E. I find except*
pretty, but then I wonder if this will be confusing:
>>> try:
... raise ValueError
... except (*TypeError, *ValueError):
... pass
...
...
Traceback (most recent call last):
File "<stdin>", line 2, in <module>
ValueError
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "<stdin>", line 3, in <module>
TypeError: Value after * must be an iterable, not type
>>>
Well - "except *ValueError" doesn't care that *
is operating on a non-iterable.
Maybe except*
is really a new token (should we think again about catch?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
my 2c: I like except *E
better because it resonates with argument unpacking syntax. I think that symmetry is what makes this proposal attractive in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, this is subtle and subjective, but except *
doesn't look like a whole new syntax to me -- it's merely about modifying the existing construct to support unpacking. Whereas except*
does feel like a new syntax, which perhaps will raise the bar of accepting this PEP a tad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have two concerns with where we are at the moment. One is what I said above about naming except*
- is the unpacking analogy valid? The syntax makes it look like we're unpacking the type or type tuple.
The second issue is about semantics: We add except*
because we can't change except
, but during the sprint we had the idea of adding catch
with a view to it possibly being a replacement for except. Let's imagine we do this and rename except*
to catch
, and then users who write new code can just use catch and forget about except. Then we need to not wrap plain exceptions by an exception group, so
try:
raise ValueError(12)
catch ValueError as e:
"""e is ValueError(12), not ExceptionGroup("", ValueError(12))"""
Con: the runtime type of e is less certain. Pro: You can use catch for all your exception handling needs (so you don't need to learn about except if you're writing new code).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not so keen on catch
, I think it will cause confusion, and as you say the type becomes uncertain. But I think it deserves a place in the rejected ideas section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO changing the syntax to catch
will make the PEP dead in the water. It instantly raises questions like "why do we have two different keywords" etc. I'm very -1 on catch
.
One is what I said above about naming except*- is the unpacking analogy valid? The syntax makes it look like we're unpacking the type or type tuple.
That's fair, I did stretch the analogy there a bit. Still, unpacking-like syntax interpretation resonates with me, personally, better.
Python/traceback.c
Outdated
/* TODO: where should this be? It is used here (in traceback.c) and in pythonrun.c | ||
* maybe we have a utility like this somewhere already? | ||
*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does either file already call a _Py
function from the other? If not I would put it here, since this is the more low-level file. And the name needs to be changed to something starting with _Py
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll make it _Py_WriteIndent.
Followup question:
Before this PR, pythonrun calls PyTraceBack_Print. After this PR it calls the new function PyTraceBack_PrintIndented. PyTraceBack_Print is declared in traceback.h as PyAPI_FUNC(int), but I added PyTraceBack_PrintIndented without that. I guess this means that PyTraceBack_PrintIndented should be _PyTraceBack_PrintIndented (as in private).
I'm working on raise in |
} | ||
else { | ||
/* case 2: exception type matcher */ | ||
return PyErr_GivenExceptionMatches(exc, matcher); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how does this interact with with virtual subclasses? bpo url: https://bugs.python.org/issue12029
currently except*
is described as:
The matching of
except*
clauses against anExceptionGroup
is performed recursively, using theExceptionGroup.split()
method:
and the split method with a type is defined as:
eg.split(TypeError)
, is equivalent toeg.split(lambda e: isinstance(e, TypeError))
.
so from the spec I'd expect that:
def test_exception_match_calls_subclasscheck(self):
class SubclassEvery(type):
def __subclasscheck__(cls, other):
return other is not ExceptionGroup
def __instancecheck__(cls, other):
return cls.__subclasscheck__(type(other))
class Special(Exception, metaclass=SubclassEvery):
pass
try:
raise ValueError
except *Special: # caught
pass
try:
raise ValueError
except Special: # not caught
pass
but the code uses PyErr_GivenExceptionMatches which has the bpo-12029 behaviour
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExceptionGroup
cannot be subclassed, and for other exceptions this is exactly the same issue as bpo-12029. Or do you think it is a different issue here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case eg.project(Special)
, is different from eg.project(lambda e: isinstance(e, Special))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider an isinstance_gem function that uses PyErr_GivenExceptionMatches:
>>> def isinstance_gem(instance, types):
... return bool(ctypes.pythonapi.PyErr_GivenExceptionMatches(ctypes.py_object(instance), ctypes.py_object(types)))
...
>>> isinstance_gem(ValueError(), Special)
False
>>> isinstance(ValueError(), Special)
True
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to the same (known) issue of bpo-12029, not a new issue introduced in this PEP. Is that correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's either an error in the PEP or an issue in this PR
the PEP reads:
eg.split(TypeError)
, is equivalent toeg.split(lambda e: isinstance(e, TypeError))
.
if it's an error in the pep then it should instead be:
def given_exception_matches(given, exc): try: raise given except exc: return True except: return False
eg.split(TypeError)
, is equivalent toeg.split(lambda e: given_exception_matches(e, TypeError))
.
otherwise should this code be changed to call PyObject_IsInstance
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I understand thanks.
This PR is stale because it has been open for 30 days with no activity. |
Example:
Gives this output:
TODO list:
(1) support raise inside the handler, and chaining of exception groups.
(2) nice syntax error when someone mixes except and except*.
(3) in ceval the JUMP_IF_NOT_EG_MATCH code peeks into the stack to see the val of the exception. Should this be DUPPED by the compiler like exc? (I'm not sure why the compiler is doing DUP_TOP just so that the JUMP_IF_NOT_EXC_MATCH code can pop it).
(4) rename exc --> nested_exceptions (or something like that). Make it immutable.
(5) rename ExceptionGroup.project() --> ExceptionGroup.split().