bpo-45292: [PEP-654] add except*

[iritkatriel]

https://bugs.python.org/issue45292

[gvanrossum]

Throwaway comment. You may want to ask @pablogsal and/or @lysnikolaou for a review of the parser parts.

[erlend-aasland]

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 ;)

[iritkatriel]

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.

[gvanrossum]

@markshannon Can you look at Irit’s last comment?

Is it perhaps because the exception table is generated by magic opposes?

[iritkatriel]

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.

[iritkatriel]

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

[gvanrossum]

I'm assuming Mark and Erlend will handle the remaining review tasks here. I may occasionally take a peek and find something superficial. :-)

[iritkatriel]

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?

[erlend-aasland]

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:

1. PEP 7 nitpicks. (I started applying PEP 7 comments to Python/compile.c as well, but I guess it's ok to disregard PEP 7 for that file.)
2. Minor C API nitpicks (mostly use Py_Is, Py_IsNone, etc.). Do with these comments as you want 🗑️

[gvanrossum]

Merge whenever you feel ready!

[markshannon]

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.

[gvanrossum]

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

[iritkatriel]

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.

[gvanrossum]

Let's move that discussion back to bpo-45292.

[markshannon]

Unfortunately, this change results in a 1% slowdown