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

ExceptionGroup: make it generic #7626

Merged
merged 3 commits into from
Apr 15, 2022
Merged

Conversation

JelleZijlstra
Copy link
Member

See motivation in python/cpython#91549.

I used the following tests:

beg = BaseExceptionGroup("x", [SystemExit()])
assert_type(beg, BaseExceptionGroup[SystemExit])
assert_type(beg.exceptions, tuple[SystemExit | BaseExceptionGroup[SystemExit], ...])
assert_type(
    beg.subgroup(KeyboardInterrupt), BaseExceptionGroup[KeyboardInterrupt] | None
)
assert_type(
    beg.subgroup((KeyboardInterrupt, SystemExit)),
    BaseExceptionGroup[KeyboardInterrupt | SystemExit] | None,
)
assert_type(
    beg.split(KeyboardInterrupt),
    tuple[
        BaseExceptionGroup[KeyboardInterrupt] | None,
        BaseExceptionGroup[SystemExit] | None,
    ],
)
assert_type(
    beg.split((KeyboardInterrupt, SystemExit)),
    tuple[
        BaseExceptionGroup[KeyboardInterrupt | SystemExit] | None,
        BaseExceptionGroup[SystemExit] | None,
    ],
)

eg = ExceptionGroup("x", [ValueError(), KeyError()])
assert_type(eg, ExceptionGroup[ValueError | KeyError])
assert_type(
    eg.exceptions,
    tuple[ValueError | KeyError | ExceptionGroup[ValueError | KeyError], ...],
)

These all pass in pyright, but mypy for some reason fails on a lot of the split and subgroup calls, with errors like samples/exceptiongroup.py:107: error: Expression is of type "Optional[BaseExceptionGroup[Any]]", not "Optional[BaseExceptionGroup[KeyboardInterrupt]]". Seems like a bug.

The overloads I added trade precision in the generic argument against precision in the generic type itself. If you have a subclass ChildExceptionGroup(ExceptionGroup) and call .split(SomeException) on it, you get an ExceptionGroup[SomeException] from the type checker, though at runtime you'd get a ChildExceptionGroup[SomeException]. I think that's the right tradeoff because subclassing ExceptionGroup is likely to be rare.

@github-actions

This comment has been minimized.

2 similar comments
@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

I haven't studied the finer details of exception groups, but this seems reasonable, and mypy/pyright seem happy.

Comment on lines +1760 to +1765
@overload
def subgroup(
self, __condition: type[_BaseExceptionT] | tuple[type[_BaseExceptionT], ...]
) -> BaseExceptionGroup[_BaseExceptionT] | None: ...
@overload
def subgroup(self: Self, __condition: Callable[[_BaseExceptionT_co], bool]) -> Self | None: ...
Copy link
Collaborator

@srittau srittau Apr 15, 2022

Choose a reason for hiding this comment

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

One minor gripe I have is that _BaseExceptionT and _BaseExceptionT_co have a very similar name, which means that it's easy to miss the subtleties in cases like above. But I have no immediate idea for better names.

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially had them as _BaseExceptionT and _BaseExceptionU but that seemed worse.

) -> tuple[BaseExceptionGroup[_BaseExceptionT] | None, Self | None]: ...
@overload
def split(self: Self, __condition: Callable[[_BaseExceptionT_co], bool]) -> tuple[Self | None, Self | None]: ...
def derive(self: Self, __excs: Sequence[_BaseExceptionT_co]) -> Self: ...
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a bit of a tricky one. If I read PEP 654 correctly, the __excs provided don't need to have anything in common with the exceptions of self. So a "correct" annotation would be:

def derive(self: Self, __excs: Sequence[_BaseExceptionT]) -> Self[_BaseExceptionT]: ...

But I don't think that's possible. So I'm fine with this, until someone comes up with a better alternative.

Copy link
Member

Choose a reason for hiding this comment

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

Yet another use case for python/typing#548...

@youtux
Copy link
Contributor

youtux commented Jun 7, 2024

why should this be the case?

assert_type(beg.exceptions, tuple[SystemExit | BaseExceptionGroup[SystemExit], ...])

shouldn't instead be like this?

assert_type(beg.exceptions, tuple[SystemExit, ...])

Asking because it causes unexpected typing when using except*: https://mypy-play.net/?mypy=latest&python=3.12&gist=059952b22801648257f86bee1fbfd404

@youtux
Copy link
Contributor

youtux commented Jun 7, 2024

nevermind, just tried https://mypy-play.net/?mypy=latest&python=3.12&gist=059952b22801648257f86bee1fbfd404 and indeed it can have a nested ExceptionGroup in it. Quite unexpected for me, but this is not the forum to raise the issue.

Sign up for free to 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.

4 participants