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 — gh-99553: fix bug where an ExceptionGroup subclass can wrap a BaseException by iritkatriel · Pull Request #99572 · python/cpython

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Nov 18, 2022 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

Fixes #99553.

  • Issue: Subclasses of ExceptionGroup can wrap BaseExceptions #99553

Sorry, something went wrong.

Uh oh!

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

All reactions
@iritkatriel
pythongh-99553: fix bug where an ExceptionGroup subclass can wrap a B…
da98a62
…aseException
@iritkatriel iritkatriel added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) needs backport to 3.11 only security fixes labels Nov 18, 2022
@bedevere-bot bedevere-bot mentioned this pull request Nov 18, 2022
Subclasses of ExceptionGroup can wrap BaseExceptions #99553
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Nov 18, 2022
@iritkatriel
iritkatriel requested a review from gvanrossum November 18, 2022 12:22
gvanrossum
gvanrossum approved these changes Nov 18, 2022
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.

Yeah, that’s what I thought.

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting core review labels Nov 18, 2022
@iritkatriel
iritkatriel merged commit c8c6113 into python:main Nov 18, 2022
@miss-islington

miss-islington commented Nov 18, 2022

Copy link
Copy Markdown
Contributor

Thanks @iritkatriel for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the awaiting merge label Nov 18, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Nov 18, 2022
@iritkatriel @miss-islington
pythongh-99553: fix bug where an ExceptionGroup subclass can wrap a B…
65c8f88
…aseException (pythonGH-99572)

(cherry picked from commit c8c6113)

Co-authored-by: Irit Katriel <[email protected]>
@bedevere-bot

bedevere-bot commented Nov 18, 2022

Copy link
Copy Markdown

GH-99580 is a backport of this pull request to the 3.11 branch.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Nov 18, 2022
miss-islington added a commit that referenced this pull request Nov 18, 2022
@miss-islington @iritkatriel
gh-99553: fix bug where an ExceptionGroup subclass can wrap a BaseExc…
82ab9e6
…eption (GH-99572)

(cherry picked from commit c8c6113)

Co-authored-by: Irit Katriel <[email protected]>
Zac-HD
Zac-HD reviewed Nov 18, 2022
View reviewed changes
Comment thread Doc/library/exceptions.rst
Comment on lines +968 to +970
Like :exc:`ExceptionGroup`, any subclass of :exc:`BaseExceptionGroup` which
is also a subclass of :exc:`Exception` can only wrap instances of
:exc:`Exception`.

@Zac-HD Zac-HD Nov 18, 2022 •
edited
Loading

Uh oh!

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

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.

As written, consistency suggests that class MyEG(BaseExceptionGroup, ValueError): should be unable to wrap anything but ValueErrors!

That may in fact be the intention, and it wouldn't break the trio.MultiError-as-BaseExceptionGroup deprecation pathway, but it's not the implementation and the tests don't distinguish these cases.

Otherwise, I think we'd want to document the behavior which is that "any subclass of ExceptionGroup can only wrap instances of Exception, like ExceptionGroup itself.", and therefore class MyEG(BaseExceptionGroup, ValueError): would be able to contain BaseExceptions 🤔

Sorry, something went wrong.

Uh oh!

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

All reactions

@iritkatriel iritkatriel Nov 18, 2022

Copy link
Copy Markdown
Member Author

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.

As written, this suggests that class MyEG(BaseExceptionGroup, ValueError): should be unable to wrap anything but ValueErrors!

Does it? In the doc the Exception becomes a hyperlink.

Sorry, something went wrong.

Uh oh!

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

All reactions

@iritkatriel iritkatriel Nov 18, 2022

Copy link
Copy Markdown
Member Author

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.

But feel free to make a PR with better wording.

Sorry, something went wrong.

Uh oh!

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

All reactions

@Zac-HD Zac-HD Nov 18, 2022

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 expect some users to read that as "if a subclass of BaseExceptionGroup is also a subclass of X, it can only wrap instances of X"; I'll open a PR to rephrase as "if it's also a subclass of Exception, it can't wrap BaseExceptions".

But then we're still treating Exception as particularly special for subclasses, which feels a bit strange.

Sorry, something went wrong.

Uh oh!

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

All reactions

@gvanrossum gvanrossum Nov 18, 2022

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.

Zac, I'm confused why you think the sentence as written could be interpreted that way. It says Exception twice. In this context that is very clearly a reference to the specific built-in class named Exception (it even links there), not a "free variable" that must refer to the same class in both places.

Regardless, even if we clear that up, it feels like it would be expensive to inspect the class hierarchy to find other subclasses of BaseException and try to constrain the arguments to inherit from those. And that's definitely not what the PEP intended. We want the common case (where you're using one of the two built-in EG classes) to be foolproof, and other cases possible.

Sorry, something went wrong.

Uh oh!

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

Zac-HD reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@Zac-HD Zac-HD Nov 19, 2022

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.

Apologies for the confusion, that's on me 😞

I do think the behavior in this PR is good, and I'll open a PR with some additional tests and maybe docs to distinguish it from the case that I was wondering about. Thanks again to both of you for everything.

Sorry, something went wrong.

Uh oh!

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

gvanrossum reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@Zac-HD Zac-HD mentioned this pull request Nov 20, 2022
gh-99553: add tests for ExceptionGroup wrapping #99615
Merged
@stefanor stefanor mentioned this pull request Dec 10, 2022
Needs update for 3.11.1 agronholm/exceptiongroup#50
Closed
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
+1 more reviewer
@Zac-HD Zac-HD Zac-HD left review comments
Reviewers whose approvals may not affect merge requirements

Uh oh!

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

Assignees

No one assigned

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Subclasses of ExceptionGroup can wrap BaseExceptions

Uh oh!

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

5 participants

@iritkatriel @miss-islington @bedevere-bot @gvanrossum @Zac-HD
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.