Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-99553: fix bug where an ExceptionGroup subclass can wrap a BaseException#99572

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
iritkatriel merged 1 commit intopython:mainfromiritkatriel:subclass
Nov 18, 2022

Conversation

iritkatriel
Copy link
Member

@iritkatrieliritkatriel commentedNov 18, 2022
edited by bedevere-bot
Loading

@iritkatrieliritkatriel added type-bugAn unexpected behavior, bug, or error interpreter-core(Objects, Python, Grammar, and Parser dirs) needs backport to 3.11only security fixes labelsNov 18, 2022
Copy link
Member

@gvanrossumgvanrossum left a 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.

@miss-islington
Copy link
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!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull requestNov 18, 2022
…aseException (pythonGH-99572)(cherry picked from commitc8c6113)Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
@bedevere-bot
Copy link

GH-99580 is a backport of this pull request to the3.11 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.11only security fixes labelNov 18, 2022
miss-islington added a commit that referenced this pull requestNov 18, 2022
…eption (GH-99572)(cherry picked from commitc8c6113)Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
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`.
Copy link
Contributor

@Zac-HDZac-HDNov 18, 2022
edited
Loading

Choose a reason for hiding this comment

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

As written, consistency suggests thatclass MyEG(BaseExceptionGroup, ValueError): should be unable to wrap anything butValueErrors!

That may in fact be the intention, and it wouldn't break thetrio.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 ofExceptionGroup can only wrap instances ofException, likeExceptionGroup itself.",and thereforeclass MyEG(BaseExceptionGroup, ValueError): would be able to containBaseExceptions 🤔

Copy link
MemberAuthor

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.

Copy link
MemberAuthor

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.

Copy link
Contributor

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 treatingException as particularly special for subclasses, which feels a bit strange.

Copy link
Member

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 saysException twice. In this context that is very clearly a reference to the specific built-in class namedException (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 ofBaseException 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 casespossible.

Zac-HD reacted with thumbs up emoji
Copy link
Contributor

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.

gvanrossum reacted with thumbs up emoji
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@gvanrossumgvanrossumgvanrossum approved these changes

@Zac-HDZac-HDZac-HD left review comments

Assignees
No one assigned
Labels
interpreter-core(Objects, Python, Grammar, and Parser dirs)type-bugAn unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Subclasses ofExceptionGroup can wrapBaseExceptions
5 participants
@iritkatriel@miss-islington@bedevere-bot@gvanrossum@Zac-HD

[8]ページ先頭

©2009-2025 Movatter.jp