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