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

Subclasses of ExceptionGroup can wrap BaseExceptions #99553

Closed
Zac-HD opened this issue Nov 17, 2022 · 6 comments · Fixed by #99572
Closed

Subclasses of ExceptionGroup can wrap BaseExceptions #99553

Zac-HD opened this issue Nov 17, 2022 · 6 comments · Fixed by #99572
Labels
3.11 only security fixes 3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@Zac-HD
Copy link
Contributor

Zac-HD commented Nov 17, 2022

class MyEG(ExceptionGroup):
    """Holds BaseExceptions without itself being a BaseException."""

oops = MyEG("oops", [KeyboardInterrupt()])
assert isinstance(oops, Exception)
assert not isinstance(oops.exceptions[0], Exception)

I believe that this is a bug tracing to the period when PEP-654 did not intend (Base)ExceptionGroup to be usable as a parent class; and I think a sufficient fix would be to replace the type-equality check with an isinstance check in:

if (cls == PyExc_ExceptionGroup) {
if (nested_base_exceptions) {
PyErr_SetString(PyExc_TypeError,
"Cannot nest BaseExceptions in an ExceptionGroup");
goto error;

cc @iritkatriel; raised via agronholm/exceptiongroup#40 (comment)

Linked PRs

@Zac-HD Zac-HD added the type-bug An unexpected behavior, bug, or error label Nov 17, 2022
@iritkatriel
Copy link
Member

I believe we discussed this and decided that we leave it to the subclass author to deal with it. Maybe it needs to be documented (or reconsidered)?

CC @gvanrossum @1st1

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Nov 17, 2022

I think at least documented, though I'd prefer it was changed.

If people intend MyEG to contain BaseExceptions then the obvious way is to inherit from BaseExceptionGroup (and likely have a BaseMyEg; Trio has already shipped this pattern), so any cases where MyEG ends up containing e.g. a KeyboardInterrupt and then discarding that to an except Exception: would be unfortunate! I think the PEP argues against this well 🙂

The == comparison also makes static typing very confusing: if we have BaseExceptionGroup -> ExceptionGroup -> MyEG and the last can also contain BaseExceptions, then either ExceptionGroup.exceptions must be annotated as able to contain BaseExceptions when it cannot, the annotations for MyEG must say it cannot when in fact it can, or we violate the substitution principle. This last is currently the case at runtime, but typecheckers are not keen on such things.

@Zac-HD
Copy link
Contributor Author

Zac-HD commented Nov 17, 2022

Hah! It looks like I just find this persistently startling: #28569 (comment) (but I promise, no PEP this time 😉)

@gvanrossum
Copy link
Member

Yeah, I definitely recall that we discussed this before. But looking at the code it's possible that we were too conservative. In particular in the fallback clause we could have chosen to check if cls is a subclass of PyExc_ExceptionGroup if nested_base_exceptions is set and raise the same error as on line 742.

The thing we definitely can't do is adjusting the class if it is exactly PyExc_BaseExceptionGroup and nested_base_exceptions is false.

The question is, can we treat this as a bug and fix it (in 3.11.1 if it isn't out yet) or would such a fix be considered backwards compatible?

@iritkatriel
Copy link
Member

I think we can call it a bug. It’s better to change it in 3.11.1 than in 3.12.

@gvanrossum
Copy link
Member

Sounds good.

@iritkatriel iritkatriel added interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.11 only security fixes 3.12 bugs and security fixes labels Nov 18, 2022
iritkatriel added a commit to iritkatriel/cpython that referenced this issue Nov 18, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Nov 18, 2022
…aseException (pythonGH-99572)

(cherry picked from commit c8c6113)

Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
miss-islington added a commit that referenced this issue Nov 18, 2022
…eption (GH-99572)

(cherry picked from commit c8c6113)

Co-authored-by: Irit Katriel <1055913+iritkatriel@users.noreply.github.com>
Zac-HD added a commit to Zac-HD/cpython that referenced this issue Nov 20, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Apr 11, 2023
(cherry picked from commit 4cd1cc8)

Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>
miss-islington added a commit that referenced this issue Apr 11, 2023
(cherry picked from commit 4cd1cc8)

Co-authored-by: Zac Hatfield-Dodds <zac.hatfield.dodds@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.11 only security fixes 3.12 bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants