Skip to content

stubtest: relax async checking for abstract methods #12343

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

Closed
wants to merge 15 commits into from

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Mar 13, 2022

Description

As has been pointed out in python/typeshed#7475, the current typeshed definitions for various async methods in the stub for typing are problematic.

Until recently, these were all "synchronous" methods that returned Awaitable[X]. However, they were recently changed to be async def methods. This more closely match their definitions at runtime, but is problematic: the classes in typing.pyi represent abstract interfaces, and it's not true that an object has to return a coroutine from its __anext__ method in order to be considered an AyncIterator (it just has to return something that's awaitable).

Unfortunately, following #12212, stubtest will now emit an error if a method is async at runtime but not in the stub.

I think it makes sense to relax this check for abstract methods, so that's what this PR proposes doing.

Test Plan

Three new test cases added to teststubtest.py.

cc. @hauntsaninja, @graingert

mypy/stubtest.py Outdated
Comment on lines 743 to 747
if isinstance(stub.type, mypy.types.CallableType):
ret_type = mypy.types.get_proper_type(stub.type.ret_type)
should_error = (
isinstance(ret_type, mypy.types.Instance)
and "__await__" not in ret_type.type.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.

Is there a more idiomatic way of figuring out whether the return type of a method in the stub is a subtype of Awaitable[Any]?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the unused awaitable stuff does something similar.

Copy link
Member Author

@AlexWaygood AlexWaygood Mar 22, 2022

Choose a reason for hiding this comment

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

Thanks! I've modified my PR to add a helper property in mypy.types to avoid code duplication

@github-actions

This comment has been minimized.

3 similar comments
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Collaborator

@hauntsaninja hauntsaninja left a comment

Choose a reason for hiding this comment

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

I'm not sure I fully understand this change, but I'm not yet convinced it's better than an allowlist entry. This seems to encourage LSP violations (nitpicky ones, but still — we seem to be trying to tighten up "returns coroutine" vs "returns awaitable" and this muddies the water). Note also that abstractmethods in Python can have implementations, so the use of an async def in an abstractmethod might have runtime meaning.

@AlexWaygood
Copy link
Member Author

I'm not sure I fully understand this change, but I'm not yet convinced it's better than an allowlist entry.

I see it as similar to the various synchronous abstract methods where we have a looser signature in the stub than the default implementation at runtime.

For example, collections.abc.Iterator.__iter__ and contextlib.AbstractContextManager.__enter__ both return self at runtime. But we wouldn't want them to be annotated with -> Self in the stubs, because these classes are abstract interfaces that represent formal definitions of what it means to be an "iterator" or a "context manager", and a class doesn't have to return self from its __iter__ method order for it to be considered an "iterator".

In the same way, AsyncIterator.__anext__ is a coroutine function at runtime -- that's the default implementation. But a class's __anext__ method doesn't have to be a coroutine function in order for it to be considered an "asynchronous iterator" (though it does need to return an awaitable). So it makes sense to have a looser signature for AsyncIterator.__anext__ than the default implementation, just like it does for collections.abc.Iterator.__iter__ and contextlib.AbstractContextManager.__enter__.

It seems to me that classes with abstract methods are generally going to represent abstract interfaces of some kind. As such, it might often be the case that the default implementation of an abstract method is a coroutine function, even if subclasses are not necessarily expected to implement the method as a coroutine function in order to be considered compatible with the interface. Hence, this PR :)

@AlexWaygood
Copy link
Member Author

If you're still not convinced, feel free to close this, no hard feelings :)

@github-actions

This comment has been minimized.

@AlexWaygood AlexWaygood marked this pull request as draft August 15, 2022 06:05
@AlexWaygood AlexWaygood marked this pull request as ready for review August 15, 2022 06:58
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 1, 2023

According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅

@JelleZijlstra JelleZijlstra self-requested a review June 1, 2023 21:23
class _CustomAwaitable2(Awaitable[Any]): ...
class Foo:
@abstractmethod
def abstract(self) -> _CustomAwaitable: ...
Copy link
Member

Choose a reason for hiding this comment

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

Why do we want to allow this? Doesn't seem particularly useful.

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 can't remember, and I'm not sure I care anymore. I guess I'll just close this PR :)

@AlexWaygood AlexWaygood closed this Jun 7, 2023
@AlexWaygood AlexWaygood deleted the async branch June 7, 2023 16:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants