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

Fix FixtureFunction TypeAlias and TypeVars #1045

Merged
merged 4 commits into from
Jan 21, 2025

Conversation

cdce8p
Copy link
Contributor

@cdce8p cdce8p commented Jan 18, 2025

If TypeVars are used in TypeAliases, they need to be passed along as well. Otherwise it's just interpreted as Any similar to all other generic type without subscripts.

This PR reworks the TypeAliases and TypeVars to properly type the fixture function.

@codecov-commenter
Copy link

codecov-commenter commented Jan 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.31%. Comparing base (5a0c567) to head (9a74294).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1045      +/-   ##
==========================================
+ Coverage   91.27%   91.31%   +0.04%     
==========================================
  Files           2        2              
  Lines         573      576       +3     
  Branches       75       76       +1     
==========================================
+ Hits          523      526       +3     
  Misses         30       30              
  Partials       20       20              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@seifertm seifertm left a comment

Choose a reason for hiding this comment

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

Thanks for the patch.
I must admit that I don't fully understand it, though.

You're saying that TypeVars need used as part of a function signature. Previously, they were used in type aliases which, in turn, were used in the function signature, but the type checker doesn't understand that indirection. Is that the problem that this patch solves?

@cdce8p
Copy link
Contributor Author

cdce8p commented Jan 21, 2025

Update: After looking at it some more, I was able to simplify the typing even further by using ParamSpec. This has the advantage that it now fully works with mypy too.


Original post

You're saying that TypeVars need used as part of a function signature. Previously, they were used in type aliases which, in turn, were used in the function signature, but the type checker doesn't understand that indirection. Is that the problem that this patch solves?

The issue here are the generic type aliases. Let me provide a simplified example:

T = TypeVar("T")
U = TypeVar("U")
Alias = Union[T, U]
x: Alias
reveal_type(x)  # Union[Any, Any]

Alias here is similar to FixtureFunction. To be used correctly, the type alias itself would need to change to Alias[T, U] which the type checker could infer as Union[T?, U?]. The mypy docs on that topic might also be helpful: https://mypy.readthedocs.io/en/stable/generics.html#generic-type-aliases

--
To come back to the current usage. If you run pyright (in strict mode), these are a few of the error it generates

  /.../pytest_asyncio/plugin.py:66:1 - error: Type of "FixtureFunctionMarker" is partially unknown
    Type of "FixtureFunctionMarker" is "type[(Unknown) -> Unknown]" (reportUnknownVariableType)
  /.../pytest_asyncio/plugin.py:66:35 - error: Expected type arguments for generic type alias "FixtureFunction" (reportMissingTypeArgument)
  /.../pytest_asyncio/plugin.py:66:53 - error: Expected type arguments for generic type alias "FixtureFunction" (reportMissingTypeArgument)
  /.../pytest_asyncio/plugin.py:119:5 - error: Return type is unknown (reportUnknownParameterType)
  /.../pytest_asyncio/plugin.py:120:5 - error: Type of parameter "fixture_function" is unknown (reportUnknownParameterType)
  /.../pytest_asyncio/plugin.py:120:23 - error: Expected type arguments for generic type alias "FixtureFunction" (reportMissingTypeArgument)
  /.../pytest_asyncio/plugin.py:132:6 - error: Expected type arguments for generic type alias "FixtureFunction" (reportMissingTypeArgument)
  /.../pytest_asyncio/plugin.py:136:5 - error: Return type, "(Unknown) -> Unknown", is partially unknown (reportUnknownParameterType)
  /.../pytest_asyncio/plugin.py:152:5 - error: Return type, "Unknown | ((Unknown) -> Unknown)", is partially unknown (reportUnknownParameterType)
  /.../pytest_asyncio/plugin.py:153:5 - error: Type of parameter "fixture_function" is partially unknown
    Parameter type is "Unknown | None" (reportUnknownParameterType)
  /.../pytest_asyncio/plugin.py:153:23 - error: Expected type arguments for generic type alias "FixtureFunction" (reportMissingTypeArgument)
  /.../pytest_asyncio/plugin.py:156:6 - error: Expected type arguments for generic type alias "FixtureFunction" (reportMissingTypeArgument)

As you might see, there are a lot of Unknown types which are a result of the issue mentioned above. In the end it does work out though (at least for pyright) as it simply disregards the decorator as "untyped". Mypy interprets the return type as Any.

from collections.abc import AsyncGenerator
import pytest_asyncio

@pytest_asyncio.fixture()  # Overload 1
async def fixture_1() -> AsyncGenerator[int]:
    yield 1

@pytest_asyncio.fixture  # Overload 2
async def fixture_2() -> AsyncGenerator[int]:
    yield 1
# pyright (the decorator is just ignored)
reveal_type(fixture_1)  # () -> AsyncGenerator[int, None]
reveal_type(fixture_2)  # () -> AsyncGenerator[int, None]

# mypy
reveal_type(fixture_1)  # Union[Any, Any]
reveal_type(fixture_2)  # Union[Any, Any]

After the changes in this PR, pyright no longer emits any warnings for the pytest_asyncio.fixture decorator. The reveal_type outputs also look better

# pyright (decorator inferred correctly)
reveal_type(fixture_1)  # () -> AsyncGenerator[int, None]
reveal_type(fixture_2)  # () -> AsyncGenerator[int, None]

# mypy
reveal_type(fixture_1)  # Never  # probably a bug in mypy
reveal_type(fixture_2)  # def () -> typing.AsyncGenerator[builtins.int, None]

# mypy (with latest changes)
reveal_type(fixture_1)  # def () -> typing.AsyncGenerator[builtins.int, None]
reveal_type(fixture_2)  # def () -> typing.AsyncGenerator[builtins.int, None]

Copy link
Contributor

@seifertm seifertm left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation and the overhaul! I like the second version with ParamSpec much better.

I can imagine it was a lot of effort to get to this point, especially with different behaviors between mypy and pyright.

Before we can merge, we need to inform the users about the change. Could you add an entry to docs/reference/changelog.rst where you briefly describe what's been fixed? Please also mention the added dependency on typing_extensions in a separate bullet point, since this is relevant for downstream packagers.

@seifertm
Copy link
Contributor

Thanks @cdce8p!

@seifertm seifertm enabled auto-merge January 21, 2025 11:51
@seifertm seifertm added this pull request to the merge queue Jan 21, 2025
Merged via the queue into pytest-dev:main with commit 3205555 Jan 21, 2025
15 checks passed
@cdce8p cdce8p deleted the fix-fixture-function branch January 21, 2025 11:56
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.

3 participants