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

Make ExitStack, AbstractContextManager and AsyncAbstractContextManager generic in return type of __exit__ #11048

Merged
merged 18 commits into from
Apr 22, 2024

Conversation

Daverball
Copy link
Contributor

@Daverball Daverball commented Nov 20, 2023

This comment has been minimized.

@Daverball
Copy link
Contributor Author

Quite a few mypy primer hits here. This change probably does not make sense until after PEP696 support.

This comment has been minimized.

@srittau
Copy link
Collaborator

srittau commented Feb 14, 2024

See #11422 for the type var generics feature tracker.

This comment has been minimized.

This comment has been minimized.

@Daverball
Copy link
Contributor Author

Daverball commented Mar 15, 2024

This is really encouraging, PEP-696 truly shines here. I'll check if there are any other places in typeshed that use AbstractContextManager/AbstractAsyncContextManager that should have their second parameter specified.


I fixed the obvious ones, the other ones are probably fine using the default as a fallback for now (or maybe we should just set them all to bool | None, so type checkers without full PEP-696 support still have a good UX).

This comment has been minimized.

This comment has been minimized.

@Daverball Daverball changed the title Make ExitStack generic in return type of __exit__ Make ExitStack, Abstract generic in return type of __exit__` Mar 16, 2024
@Daverball Daverball changed the title Make ExitStack, Abstract generic in return type of __exit__` Make ExitStack, AbstractContextManager and AsyncAbstractContextManager generic in return type of __exit__ Mar 16, 2024
@srittau srittau removed the status: deferred Issue or PR deferred until some precondition is fixed label Mar 20, 2024
@srittau
Copy link
Collaborator

srittau commented Mar 20, 2024

TypeVar defaults are now available in typeshed.

This comment has been minimized.

@Daverball Daverball marked this pull request as ready for review March 20, 2024 06:59
@Daverball
Copy link
Contributor Author

One of the mypy_primer hits is irrelevant, since it is the same unrelated error with different types and the other one looks like an improvement to me.

There are a couple of caveats to this change however:

  • Right now I'm explicitly specifying the second parameter in subclasses to AbstractContextManager/AbstractAsyncContextManager in the sdlib as long as they override __exit__ with a different value than the default for clarity. Technically this shouldn't be necessary and we could keep the surface area of the change smaller. Or we could go the opposite way and make sure we always specify the second parameter everywhere in the stdlib. Either of those options would probably be more manageable for maintainers of type checkers that don't yet support PEP-696, compared to the mish mash we have now.
  • typing.ContextManager and typing.AsyncContextManager are just aliases for the ABCs that changed, but this means the Python docs are no longer accurate. Technically it is a harmless mismatch, since you can omit the parameter, that is missing from the docs, but we should probably still consider updating the docs, if we accept this change.

@Daverball
Copy link
Contributor Author

Daverball commented Mar 21, 2024

https://discuss.python.org/t/passing-only-one-typevar-of-two-when-using-defaults/49134 clued me into an issue, since at runtime typing.ContextManager/typing.AsyncContextManager inherit from Generic/Protocol they will emit runtime errors if you try to use the second argument, this is in contrast to any __class_getitem__ methods added to builtins and ABC, which don't validate the number of arguments.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

stdlib/typing.pyi Outdated Show resolved Hide resolved

This comment has been minimized.

@Daverball
Copy link
Contributor Author

Daverball commented Mar 23, 2024

The new primer hit seems fine to me. I think the only reason that previously didn't error is because AbstractContextManager[Any] was in the MRO, which used to be an alias for ContextManager[Any], so mypy took a shortcut and didn't attempt to structurally match, because the simpler subclassing check already told it everything is fine (due to Any). This could actually be considered a mypy bug. I'm not sure how I feel about subclasses that override all occurrences of a TypeVar, that was previously bound to Any with a concrete type still behaving as if the type was Any in some scenarios... Although I guess that's just the price you pay if you omit a type parameter when subclassing (or set it to Any)

Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

werkzeug (https://github.com/pallets/werkzeug)
+ src/werkzeug/debug/__init__.py:368: error: Unused "type: ignore" comment  [unused-ignore]

mongo-python-driver (https://github.com/mongodb/mongo-python-driver)
+ pymongo/__init__.py:176: error: Incompatible return value type (got "_TimeoutContext", expected "ContextManager[None]")  [return-value]
+ pymongo/__init__.py:176: note: Following member(s) of "_TimeoutContext" have conflicts:
+ pymongo/__init__.py:176: note:     Expected:
+ pymongo/__init__.py:176: note:         def __enter__(self) -> None
+ pymongo/__init__.py:176: note:     Got:
+ pymongo/__init__.py:176: note:         def __enter__(self) -> _TimeoutContext

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

LGTM, the new primer hits are promising. I'll leave it open a bit, in case another maintainer wants to chime in.

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.

2 participants