Skip to content

gh-103791: Make contextlib.suppress also act on exceptions within an ExceptionGroup - #103792

Merged
ambv merged 6 commits into
python:mainfrom
ambv:gh-103791
Apr 24, 2023
Merged

gh-103791: Make contextlib.suppress also act on exceptions within an ExceptionGroup#103792
ambv merged 6 commits into
python:mainfrom
ambv:gh-103791

Conversation

@ambv

@ambvambv commented Apr 24, 2023

Copy link
Copy Markdown
Contributor

Comment threadLib/contextlib.py
match, rest = excinst.split(self._exceptions)
if rest is None:
return True
raise rest

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't ideal as it makes the exception group's own traceback include the def __exit__(...) frame. For example:

 File "/Volumes/RAMDisk/cpython/Lib/test/test_contextlib.py", line 1220, in test_exception_groups
with suppress(ValueError):
File "/Volumes/RAMDisk/cpython/Lib/contextlib.py", line 454, in __exit__
raise rest from excinst
File "/Volumes/RAMDisk/cpython/Lib/test/test_contextlib.py", line 1221, in test_exception_groups
raise eg_all()

in case of the newly added test.

This isn't a big problem because:

  • the group's member exceptions contain pristine tracebacks, only the group itself contains extra frames; and
  • the raise of the original group is still included in the traceback because we used .split() to create the new object.

Ideally, we wouldn't need this. However, the API of __exit__ makes it impossible to replace the ExceptionGroup instance with another one, while ExceptionGroup itself makes its exceptions read-only.

@iritkatrieliritkatriel left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Minor doc tweaks.

Comment threadDoc/library/contextlib.rst Outdated
Comment threadMisc/NEWS.d/next/Library/2023-04-24-23-07-56.gh-issue-103791.bBPWdS.rst Outdated
Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@ambv
ambv enabled auto-merge (squash) April 24, 2023 22:02
hugovk
hugovk previously requested changes Apr 24, 2023

@hugovkhugovk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(just testing out "request changes")

@bedevere-bot

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@ambv
ambv dismissed hugovk’s stale reviewApril 24, 2023 22:09

OK, GitHub indeed doesn't disable auto-merge when changes are requested

@ambv
ambv merged commit 22bed58 into python:mainApr 24, 2023
carljm added a commit to carljm/cpython that referenced this pull request Apr 24, 2023
* main:
pythongh-87729: add LOAD_SUPER_ATTR instruction for faster super() (python#103497)
pythongh-103791: Make contextlib.suppress also act on exceptions within an ExceptionGroup (python#103792)
Comment threadLib/contextlib.py
return
if issubclass(exctype, self._exceptions):
return True
if issubclass(exctype, ExceptionGroup):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a specific reason to handle ExceptionGroup but not BaseExceptionGroup?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt it. Want to make a PR?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@ambv@bedevere-bot@iritkatriel@hugovk@Zac-HD