bpo-45292: [PEP-654] add except* by iritkatriel · Pull Request #29581 · python/cpython
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Throwaway comment. You may want to ask @pablogsal and/or @lysnikolaou for a review of the parser parts.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments about list/tuple API, refcounts, and test coverage in Objects/exceptions.c and Python/ceval.c. I'll save the PEP 7 nitpicks for later ;)
I'm still working on the compiler part (there is a bug there with restoring exc_info/stack state or something).
The bug is actually related to the exception table lookup.
This test fails at the 1/0:
try:
raise ValueError(42)
except:
try:
raise TypeError(12)
except* Exception:
pass
1/0
with:
lasti=1 STACK_LEVEL()=4 level = 6. # This is a print I added before the assertion.
Assertion failed: (STACK_LEVEL() >= level), function _PyEval_EvalFrameDefault, file ceval.c, line 5164.
If I change except* to except it prints:
lasti=1 STACK_LEVEL()=4 level = 3
The except*/except change should not impact the level of the 1/0 instruction.
@markshannon Can you look at Irit’s last comment?
Is it perhaps because the exception table is generated by magic opposes?
Here is the relevant dis:
def star():
try:
raise ValueError(42)
except:
try:
raise TypeError(12)
except* Exception:
pass
1/0
def old():
try:
raise ValueError(42)
except:
try:
raise TypeError(12)
except Exception:
pass
1/0
Old except:
>>> dis.dis(old)
2 0 NOP
3 2 LOAD_GLOBAL 0 (ValueError)
4 LOAD_CONST 1 (42)
6 CALL_FUNCTION 1
8 RAISE_VARARGS 1
>> 10 PUSH_EXC_INFO
4 12 POP_TOP
14 POP_TOP
16 POP_TOP
5 18 NOP
6 20 LOAD_GLOBAL 1 (TypeError)
22 LOAD_CONST 2 (12)
24 CALL_FUNCTION 1
26 RAISE_VARARGS 1
>> 28 PUSH_EXC_INFO
7 30 LOAD_GLOBAL 2 (Exception)
32 JUMP_IF_NOT_EXC_MATCH 22 (to 44)
34 POP_TOP
36 POP_TOP
38 POP_TOP
8 40 POP_EXCEPT
42 JUMP_FORWARD 2 (to 48)
7 >> 44 RERAISE 0
>> 46 POP_EXCEPT_AND_RERAISE
9 >> 48 LOAD_CONST 3 (1)
50 LOAD_CONST 4 (0)
52 BINARY_OP 11 (/)
54 POP_TOP
56 POP_EXCEPT
58 LOAD_CONST 0 (None)
60 RETURN_VALUE
>> 62 POP_EXCEPT_AND_RERAISE
ExceptionTable:
2 to 8 -> 10 [0]
10 to 16 -> 62 [3] lasti
20 to 26 -> 28 [3]
28 to 38 -> 46 [6] lasti
40 to 42 -> 62 [3] lasti
44 to 44 -> 46 [6] lasti
46 to 54 -> 62 [3] lasti
Except*:
>>> dis.dis(star)
2 0 NOP
3 2 LOAD_GLOBAL 0 (ValueError)
4 LOAD_CONST 1 (42)
6 CALL_FUNCTION 1
8 RAISE_VARARGS 1
>> 10 PUSH_EXC_INFO
4 12 POP_TOP
14 POP_TOP
16 POP_TOP
5 18 NOP
6 20 LOAD_GLOBAL 1 (TypeError)
22 LOAD_CONST 2 (12)
24 CALL_FUNCTION 1
26 RAISE_VARARGS 1
>> 28 PUSH_EXC_INFO
7 30 DUP_TOP_TWO
32 POP_TOP
34 ROT_FOUR
36 BUILD_LIST 0
38 ROT_FOUR
40 LOAD_GLOBAL 2 (Exception)
42 JUMP_IF_NOT_EG_MATCH 30 (to 60)
44 POP_TOP
46 POP_TOP
48 POP_TOP
8 50 JUMP_FORWARD 4 (to 60)
52 POP_TOP
54 LIST_APPEND 6
56 POP_TOP
58 POP_TOP
>> 60 POP_TOP
62 LIST_APPEND 2
64 POP_TOP
66 PREP_RERAISE_STAR
68 DUP_TOP
70 POP_JUMP_IF_TRUE 41 (to 82)
72 POP_TOP
74 POP_TOP
76 POP_TOP
78 POP_EXCEPT
80 JUMP_FORWARD 2 (to 86)
>> 82 RERAISE 0
>> 84 POP_EXCEPT_AND_RERAISE
9 >> 86 LOAD_CONST 3 (1)
88 LOAD_CONST 4 (0)
90 BINARY_OP 11 (/)
92 POP_TOP
94 POP_EXCEPT
96 LOAD_CONST 0 (None)
98 RETURN_VALUE
>> 100 POP_EXCEPT_AND_RERAISE
ExceptionTable:
2 to 8 -> 10 [0]
10 to 16 -> 100 [3] lasti
20 to 26 -> 28 [3]
28 to 48 -> 84 [6] lasti
50 to 50 -> 100 [3] lasti
52 to 82 -> 84 [6] lasti
84 to 84 -> 100 [3] lasti
86 to 92 -> 84 [6] lasti
94 to 98 -> 100 [3] lasti
I think it's wrong that an exception from line 90 jumps to line 84. I'm missing something that tells it to go directly to line 100 from there.
…ssage for mixed except and except*. Add tests for custom errors
@markshannon I think I found the bug - there was a POP_BLOCK that needed to move out of the loop. I'll add a few unit tests and push it.
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-authored-by: Guido van Rossum <gvanrossum@gmail.com>
I'm assuming Mark and Erlend will handle the remaining review tasks here.
I hope so, but let us know if you are not planning to (@markshannon , @erlend-aasland).
@ambv - you also had a close look/experimentation with this PR, any comments from you?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic job! Here's an "#*%!( lot of nitpicks; feel free to ignore them 🙂 But, more important: I found some branches that weren't covered by the test suite. If possible, we should increase coverage. I also left some comments regarding usage of some C API's.
Nitpicks come in two flavours:
- PEP 7 nitpicks. (I started applying PEP 7 comments to
Python/compile.cas well, but I guess it's ok to disregard PEP 7 for that file.) - Minor C API nitpicks (mostly use
Py_Is,Py_IsNone, etc.). Do with these comments as you want 🗑️
Thank you Erlend! Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Merge whenever you feel ready!
I don't think that this was ready to merge.
As I said before, this does too much in the interpreter that should be done in the compiler.
It is very hard to reason about the correctness of such large instructions.
Historically they have been bug magnets.
Sorry if we jumped the gun. But this works well enough so that we can start writing code that uses it (which is a requirement the SC gave us when they approved PEP 654). I suppose we could do that in a branch, but that's very awkward -- it places a high barrier on those who just want to experiment with what this feels like.
There will be plenty of opportunity to refactor the implementation -- we have many alpha releases left until beta 1 (late May), and even after that we can fix bugs until the release candidates start (in August).
I suppose we could do that in a branch
@ambv has been doing it in a branch and did not report any bugs so far. We need more people to start using it, we need feedback on both the semantics and the implementation, and we need to give @1st1 time to develop the asyncio parts.
Indeed, I am keen to hear @markshannon's ideas about refactoring/simplifying/optimizing the implementation. The first suggestion (2-3 weeks ago) didn't work because it overlooked that fact that we need to maintain state between the except* clauses. Hopefully we can find something that does work.
Let's move that discussion back to bpo-45292.