GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-45292: [PEP-654] add except* by iritkatriel · Pull Request #29581 · python/cpython

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Nov 16, 2021 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

https://bugs.python.org/issue45292

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@iritkatriel
bpo-45292: [PEP-654] add except*
b0ca0b7
@iritkatriel
iritkatriel requested review from lysnikolaou, markshannon and pablogsal as code owners November 16, 2021 19:34
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Nov 16, 2021
@bedevere-bot bedevere-bot added the awaiting core review label Nov 16, 2021
@iritkatriel
iritkatriel requested review from 1st1 and gvanrossum November 16, 2021 19:34
@iritkatriel
iritkatriel marked this pull request as draft November 16, 2021 19:34
@iritkatriel
iritkatriel requested a review from erlend-aasland November 16, 2021 19:56
gvanrossum
gvanrossum reviewed Nov 17, 2021
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Grammar/python.gram Resolved

Uh oh!

There was an error while loading. Please reload this page.

erlend-aasland
erlend-aasland reviewed Nov 17, 2021
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel reacted with thumbs up emoji iritkatriel reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 1 reaction
Comment thread Objects/exceptions.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/exceptions.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/exceptions.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/exceptions.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/exceptions.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Nov 17, 2021

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Nov 17, 2021

Copy link
Copy Markdown
Member

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

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Nov 17, 2021

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel added 5 commits November 17, 2021 19:14
@iritkatriel
fold invalid_except_star_stmt into invalid_except_stmt. Add custom me…
cad9126
…ssage for mixed except and except*. Add tests for custom errors
@iritkatriel
Erlend's first review
798aaa7
@iritkatriel
fix grammar rule for mixed except/except*
f95272c
@iritkatriel
fix doctest failure
ca44878
@iritkatriel
remove duplication in except* handling
adc834d
@iritkatriel

iritkatriel commented Nov 19, 2021

Copy link
Copy Markdown
Member Author

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

erlend-aasland reacted with rocket emoji
All reactions
  • 🚀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel
fix bug - move POP_BLOCK to the end in case of except*
e3fa2f7
erlend-aasland
erlend-aasland reviewed Nov 19, 2021
View reviewed changes
Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel
Merge remote-tracking branch 'upstream/main' into bpo-45292-except_star
1908fd8
@iritkatriel iritkatriel mentioned this pull request Nov 21, 2021
bpo-45834: Move runtime 'except:' check to the parser #29625
Closed
iritkatriel and others added 2 commits November 21, 2021 15:42
@iritkatriel
Merge remote-tracking branch 'upstream/main' into bpo-45292-except_star
5846329
@iritkatriel Erlend Egeberg Aasland
Use Py_NewRef(_PyLong_GetZero());
f9a626c
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
isidentical
isidentical reviewed Nov 21, 2021
View reviewed changes
Comment thread Parser/Python.asdl Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel added 2 commits November 22, 2021 01:05
@iritkatriel
Split AST node Try -> Try/TryStar. Separate try-except and try-except…
fdc948b
…* cases in compiler
@iritkatriel
Revert doctest changes (not needed with TryStar)
488f9fa
This reverts commit ca44878.
@iritkatriel @gvanrossum
"three items" --> "3items"
22bf207
Co-authored-by: Guido van Rossum <[email protected]>
gvanrossum
gvanrossum reviewed Dec 8, 2021
View reviewed changes
Comment thread Doc/library/dis.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel
add spaces
66d830a
@iritkatriel

iritkatriel commented Dec 10, 2021

Copy link
Copy Markdown
Member Author

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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel
Merge remote-tracking branch 'upstream/main' into bpo-45292-except_star
f2b74a5
erlend-aasland
erlend-aasland reviewed Dec 10, 2021
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

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 🗑️

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread Objects/exceptions.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/exceptions.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/exceptions.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ast.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Python/ceval.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Objects/frameobject.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel and others added 7 commits December 11, 2021 15:17
@iritkatriel Erlend Egeberg Aasland
Apply first batch of suggestions from code review
035fe5c
Thank you Erlend!

Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@iritkatriel
the remaining comments from Erlend
5d52de3
@iritkatriel
sign what's new
a1591f0
@iritkatriel
Merge remote-tracking branch 'upstream/main' into bpo-45292-except_star
ce36211
@iritkatriel
'== -1' --> '< 0' for consistency. Added a missing recursion guard.
20d9ccd
@iritkatriel
Merge remote-tracking branch 'upstream/main' into bpo-45292-except_star
3a642e6
@iritkatriel
Merge branch 'main' into bpo-45292-except_star
13e0d0f
erlend-aasland
erlend-aasland reviewed Dec 13, 2021
View reviewed changes
Comment thread Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel and others added 3 commits December 14, 2021 11:36
@iritkatriel Erlend Egeberg Aasland
whitespace
078f050
Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@iritkatriel
Merge branch 'bpo-45292-except_star' of https://github.com/iritkatrie…
a91b219
…l/cpython into bpo-45292-except_star
@iritkatriel
Merge remote-tracking branch 'upstream/main' into bpo-45292-except_star
564b8a0
gvanrossum
gvanrossum approved these changes Dec 14, 2021
View reviewed changes

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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.

Merge whenever you feel ready!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

iritkatriel, erlend-aasland, and asvetlov reacted with hooray emoji
All reactions
  • 🎉 3 reactions
@iritkatriel
iritkatriel merged commit d60457a into python:main Dec 14, 2021
@bedevere-bot bedevere-bot removed the awaiting merge label Dec 14, 2021
@markshannon

markshannon commented Dec 14, 2021

Copy link
Copy Markdown
Member

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Dec 14, 2021

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@iritkatriel

iritkatriel commented Dec 14, 2021

Copy link
Copy Markdown
Member Author

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@gvanrossum

gvanrossum commented Dec 14, 2021 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@markshannon

markshannon commented Dec 14, 2021

Copy link
Copy Markdown
Member

Unfortunately, this change results in a 1% slowdown

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

gvanrossum
gvanrossum reviewed Dec 14, 2021
View reviewed changes
Comment thread Python/compile.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@cdce8p cdce8p mentioned this pull request Apr 22, 2022
TODO list for Python 3.11 support pylint-dev/astroid#1516
Closed
5 tasks done
@iritkatriel
iritkatriel deleted the bpo-45292-except_star branch May 20, 2022 11:31
@iritkatriel iritkatriel mentioned this pull request Apr 10, 2022
Implement PEP 654: Exception Groups #89455
Closed
@coderabbitai coderabbitai Bot mentioned this pull request Apr 26, 2026
ratchet: pre-commit guardrails — atomic-write + pytest.raises hygiene + T3-narrow (5 review rounds) curdriceaurora/fo-core#219
Merged
8 of 10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gvanrossum gvanrossum gvanrossum approved these changes
@erlend-aasland erlend-aasland erlend-aasland left review comments
@isidentical isidentical isidentical left review comments
@pablogsal pablogsal pablogsal approved these changes
@lysnikolaou lysnikolaou Awaiting requested review from lysnikolaou lysnikolaou is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner
@1st1 1st1 Awaiting requested review from 1st1

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

9 participants

@iritkatriel @gvanrossum @markshannon @bedevere-bot @ambv @pablogsal @erlend-aasland @isidentical @the-knights-who-say-ni
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.