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

Using Generic Exceptions (ExceptionGroups in particular) with raises in a type-safe manner is not ideal #13115

Open
tapetersen opened this issue Jan 7, 2025 · 6 comments · May be fixed by #13134
Labels
topic: typing type-annotation issue type: bug problem that needs to be addressed

Comments

@tapetersen
Copy link

tapetersen commented Jan 7, 2025

As I understand the arguments for not extending pytest.raises to automatically unwrap ExceptionGroupsas discussed in #11538 I've tried to use the ExceptionInfo.group_contains() method which works well enough for the runtime parts.

However since the return value of raises() ( ExceptionInfo) is generic in the exception type, the stricter type-checkers will complain about the unknown type-argument of the ExceptionGroup.

# pyright: strict

import pytest
import sys
from typing import cast

if sys.version_info < (3, 11):
    from exceptiongroup import ExceptionGroup

class FooError(Exception):
    pass

# Pyright will report an error for the code below as:
# Type of "exc_group_info" is partially unknown
#   Type of "exc_group_info" is "ExceptionInfo[ExceptionGroup]"PylancereportUnknownVariableType
with pytest.raises(ExceptionGroup) as exc_group_info:
    raise ExceptionGroup("Some error occured", [FooError("Error")])

assert exc_group_info.group_contains(FooError)

If trying to rectify this by supplying the type-argument it fails various checks in raises that checks that the argument is an instance of type which GenericAlias (that you get when indexing a generic class) is not.

# If the type is added pyright is ok but the runtime is not:
# Traceback (most recent call last):
#  File ".../test.py", line 21, in <module>
#    with pytest.raises(ExceptionGroup[ExceptionGroup[FooError]]) as exc_group_info:
#         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
#  File ".../.venv/lib/python3.12/site-packages/_pytest/python_api.py", line 959, in raises
#    raise TypeError(msg.format(not_a))
# TypeError: expected exception must be a BaseException type, not GenericAlias
with pytest.raises(ExceptionGroup[ExceptionGroup[FooError]]) as exc_group_info:
    raise ExceptionGroup("Some error occured", [FooError("Error")])

assert exc_group_info.group_contains(FooError)

The correct casting that is required to get this correct would have to be something like below.

# With the cast we can get it to work but that is quite verbose and doesn't
# catch errors of BaseExceptionGroup  vs ExceptionGroup in the cast for example.
with pytest.raises(cast(type[ExceptionGroup[FooError]], ExceptionGroup)) as exc_group_info:
    raise ExceptionGroup("Some error occured", [FooError("Error")])

assert exc_group_info.group_contains(FooError)

This could probably be fixed by handling generic Exceptions (or at least ExceptionGroups) explicitly in raises using https://docs.python.org/3/library/typing.html#typing.get_origin.

@tapetersen tapetersen changed the title Using Generic Exceptions (ExceptionGroups in particular) with raises in a type-safe manner is not ideal. Using Generic Exceptions (ExceptionGroups in particular) with raises in a type-safe manner is not ideal Jan 7, 2025
@Zac-HD Zac-HD added type: bug problem that needs to be addressed topic: typing type-annotation issue labels Jan 9, 2025
@Zac-HD
Copy link
Member

Zac-HD commented Jan 9, 2025

Yep, I think calling get_origin() on generics to 'unwrap' the bare type as part of this code would be a great fix. Would you be interested in opening a PR?

@tapetersen
Copy link
Author

tapetersen commented Jan 10, 2025

I'll take a shot at it.

The only hesitation is that it may look somewhat misleading in that we would still actually catch all ExceptionGroups and don't validate that only the given exception class is actually contained.

i.e. the following satisfying the logic of raises without a single ValueError being thrown.

with pytest.raises(ExceptionGroup[ValueError]) as exc_info:
    raise ExceptionGroup("Some errors occurred", [RuntimeException("Normal error occurred")])

# Of course if you add an assert group_contains which is how I would use it
assert exc_info.group_contains(ValueError)

There are some similarities to the standard isinstance(list_of_ints, list[str]) where the choice was to not support it.

I personally still think practicality beats purity here and that the narrower focus and implied experience-level of pytest users (especially users of pytest.raises with ExceptionGroup and strong typing) but wanted to check.

Of course you can dig deeper and recognize a parametrized ExceptionGroup, extract the parametrized type and if it's an exception do some further magic with ExceptionGroup split but that feels a bit too far with an endless can of worms waiting.

@Zac-HD
Copy link
Member

Zac-HD commented Jan 10, 2025

The only hesitation is that it may look somewhat misleading in that we would still actually catch all ExceptionGroups and don't validate that only the given exception class is actually contained.

Hmm, that's a good point, and actually does make me more nervous about just unwrapping the generic - "tests check less than the developer expected" is a classic way for bugs to sneak through to production.

We've actually been working on #12504 via trio.testing.RaisesGroup() (eg in python-trio/trio#3145), with the goal of merging it into Pytest once the API has been shaken out a bit. Maybe we could push people towards RaisesGroup if their annotation is more precise than (Base)ExceptionGroup[Any / (Base)Exception], so there's a clean syntax that strict type-checkers allow but without leaving the implication that we're checking anything more precise than we actually are? (which would imply waiting, ugh)

@tapetersen
Copy link
Author

tapetersen commented Jan 10, 2025

For all my uses just always using the full ExceptionGroup[Exception] or similar even with ExceptionGroup[Any] would be fine but all of those still results in a GenericAlias which fails at runtime.

The real annoyance and problem for me/us here is really only that ExceptionGroup is generic but without a default type-argument which causes pyright/pylance to complain about the resulting type of the return value being incomplete/unspecified on the stricter setting and there being no really good place to add it as you can't simply type-annotate the result of a context-manager.

I do realize that is a quite narrow and specific concern though with annoying/ugly but fully functional workarounds so this is not really something pressing.

Loose thought without thinking too deep would be if it would be worth it if runtime could accept just the explicit common types ExceptionGroup[Exception], BaseExceptionGroup[BaseException] and maybe Any as well for argument and raise for anything else.

@Zac-HD
Copy link
Member

Zac-HD commented Jan 11, 2025

Yep, I'd be happy to accept a PR for that minimal version anytime.

@tapetersen tapetersen linked a pull request Jan 14, 2025 that will close this issue
6 tasks
tapetersen added a commit to tapetersen/pytest that referenced this issue Jan 15, 2025
@tapetersen
Copy link
Author

I added a PR with discussed fixes (Only accepts specifically ExceptionGroup[Exception] and BaseExceptionGroup[Exception]. with a hopefully meaningful error message for others).

A bit larger patch than I would've liked due to the need to handle them in tuples as well but seems to work well both on versions with native support and without. Was a bit unsure about where to add the tests so some feedback on the PR would be welcome.

tapetersen added a commit to tapetersen/pytest that referenced this issue Jan 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: typing type-annotation issue type: bug problem that needs to be addressed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants