Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Apr 24, 2023
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
11 changes: 10 additions & 1 deletion Lib/contextlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,16 @@ def __exit__(self, exctype, excinst, exctb):
# exactly reproduce the limitations of the CPython interpreter.
#
# See http://bugs.python.org/issue12029 for more details
return exctype is not None and issubclass(exctype, self._exceptions)
if exctype is None:
return
if issubclass(exctype, self._exceptions):
return True
if issubclass(exctype, ExceptionGroup):
Copy link
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
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?

match, rest = excinst.split(self._exceptions)
if rest is None:
return True
raise rest
Copy link
Contributor Author

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.

return False


class _BaseExitStack:
Expand Down
25 changes: 25 additions & 0 deletions Lib/test/support/testcase.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
class ExceptionIsLikeMixin:
def assertExceptionIsLike(self, exc, template):
"""
Passes when the provided `exc` matches the structure of `template`.
Individual exceptions don't have to be the same objects or even pass
an equality test: they only need to be the same type and contain equal
`exc_obj.args`.
"""
if exc is None and template is None:
return

if template is None:
self.fail(f"unexpected exception: {exc}")

if exc is None:
self.fail(f"expected an exception like {template!r}, got None")

if not isinstance(exc, ExceptionGroup):
self.assertEqual(exc.__class__, template.__class__)
self.assertEqual(exc.args[0], template.args[0])
else:
self.assertEqual(exc.message, template.message)
self.assertEqual(len(exc.exceptions), len(template.exceptions))
for e, t in zip(exc.exceptions, template.exceptions):
self.assertExceptionIsLike(e, t)
27 changes: 26 additions & 1 deletion Lib/test/test_contextlib.py
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
from contextlib import * # Tests __all__
from test import support
from test.support import os_helper
from test.support.testcase import ExceptionIsLikeMixin
import weakref


Expand Down Expand Up @@ -1148,7 +1149,7 @@ class TestRedirectStderr(TestRedirectStream, unittest.TestCase):
orig_stream = "stderr"


class TestSuppress(unittest.TestCase):
class TestSuppress(ExceptionIsLikeMixin, unittest.TestCase):

@support.requires_docstrings
def test_instance_docs(self):
Expand Down Expand Up @@ -1202,6 +1203,30 @@ def test_cm_is_reentrant(self):
1/0
self.assertTrue(outer_continued)

def test_exception_groups(self):
eg_ve = lambda: ExceptionGroup(
"EG with ValueErrors only",
[ValueError("ve1"), ValueError("ve2"), ValueError("ve3")],
)
eg_all = lambda: ExceptionGroup(
"EG with many types of exceptions",
[ValueError("ve1"), KeyError("ke1"), ValueError("ve2"), KeyError("ke2")],
)
with suppress(ValueError):
raise eg_ve()
with suppress(ValueError, KeyError):
raise eg_all()
with self.assertRaises(ExceptionGroup) as eg1:
with suppress(ValueError):
raise eg_all()
self.assertExceptionIsLike(
eg1.exception,
ExceptionGroup(
"EG with many types of exceptions",
[KeyError("ke1"), KeyError("ke2")],
),
)


class TestChdir(unittest.TestCase):
def make_relative_path(self, *parts):
Expand Down
22 changes: 2 additions & 20 deletions Lib/test/test_except_star.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import sys
import unittest
import textwrap
from test.support.testcase import ExceptionIsLikeMixin

class TestInvalidExceptStar(unittest.TestCase):
def test_mixed_except_and_except_star_is_syntax_error(self):
Expand Down Expand Up @@ -169,26 +170,7 @@ def f(x):
self.assertIsInstance(exc, ExceptionGroup)


class ExceptStarTest(unittest.TestCase):
def assertExceptionIsLike(self, exc, template):
if exc is None and template is None:
return

if template is None:
self.fail(f"unexpected exception: {exc}")

if exc is None:
self.fail(f"expected an exception like {template!r}, got None")

if not isinstance(exc, ExceptionGroup):
self.assertEqual(exc.__class__, template.__class__)
self.assertEqual(exc.args[0], template.args[0])
else:
self.assertEqual(exc.message, template.message)
self.assertEqual(len(exc.exceptions), len(template.exceptions))
for e, t in zip(exc.exceptions, template.exceptions):
self.assertExceptionIsLike(e, t)

class ExceptStarTest(ExceptionIsLikeMixin, unittest.TestCase):
def assertMetadataEqual(self, e1, e2):
if e1 is None or e2 is None:
self.assertTrue(e1 is None and e2 is None)
Expand Down