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

mypy seems to lose the type of an async AsyncIterator function that overrides a superclass #12662

Closed
zzzeek opened this issue Apr 23, 2022 · 7 comments
Labels
bug mypy got something wrong

Comments

@zzzeek
Copy link

zzzeek commented Apr 23, 2022

Getting this both with 0.942 as well as latest master:

from typing import AsyncIterator
from typing import Sequence


class AsyncResultWorking:
    "a class with two async methods."

    async def _some_impl(self) -> str:
        ...

    async def partitions(self) -> AsyncIterator[str]:
        ...


class AsyncResultBroken:
    """a class with the identical two async methods as AsyncResultWorking,
    but the 'partitions' method is now implemented."""

    async def _some_impl(self) -> str:
        ...

    async def partitions(self) -> AsyncIterator[str]:
        while True:
            partition = await self._some_impl()
            if partition:
                yield partition
            else:
                break


class AsyncResultSubclassWorking(AsyncResultWorking):
    """subclassing the working class is fine"""

    async def partitions(self) -> AsyncIterator[str]:
        ...


class AsyncResultSubclassBroken(AsyncResultBroken):
    """subclassing the broken class expresses confusion over the
    'partitions' method signature.

    """
    async def partitions(self) -> AsyncIterator[str]:
        ...

error:

test3.py:43: error: Return type "Coroutine[Any, Any, AsyncIterator[str]]" of "partitions" incompatible with return type "AsyncIterator[str]" in supertype "AsyncResultBroken"  [override]
Found 1 error in 1 file (checked 1 source file)

if you remove the implementation from AsyncResultBroken.partitions, then it passes. So, I'm wondering am I not using AsyncIterator correctly? but if I'm not, neither mypy nor pyright are saying anything. Also, even if the method body of partitions were wrong, the signatures match exactly.

@zzzeek zzzeek added the bug mypy got something wrong label Apr 23, 2022
@JelleZijlstra
Copy link
Member

Async generator signatures are a bit confusing. If there's no yield in the child function, the function is a normal coroutine and returns whatever its return type is, wrapped in an awaitable. But with a yield, it's an async generator, and it returns an AsyncIterator that is not wrapped in an awaitable.

@hauntsaninja
Copy link
Collaborator

Specifically, you likely want to do one of the following:

def partitions(self) -> AsyncIterator[str]: ...

or

async def partitions(self) -> AsyncIterator[str]:
    if False: yield

@hauntsaninja
Copy link
Collaborator

(duplicate of #5070 #5385)

@zzzeek
Copy link
Author

zzzeek commented Apr 23, 2022

Async generator signatures are a bit confusing. If there's no yield in the child function, the function is a normal coroutine and returns whatever its return type is, wrapped in an awaitable. But with a yield, it's an async generator, and it returns an AsyncIterator that is not wrapped in an awaitable.

OK, the subclass is a typing only stub that has no implementation, so users are in fact using the superclass in any case. I guess we would use the "False" trick then, or just a typing: ignore as I've been doing, or maybe just adjust the supertype to not be "async". this feels a little wrong though. an overridden method (edit: that explicitly indicates non-implemented for its body) should be able to have the identical interface as the superclass, this at the moment feels like one of the most non-intuitive things I've seen in a long time.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Apr 24, 2022

The "problem" is the interaction between two facts:

  • the presence of yield in an (async) function is a fundamental change to its return type
  • adding async changes a function's return type

Note that the first alone is enough to cause problems in synchronous code, e.g. see

if False:

Consider:

async def x(): return "asdf"
async def y(): yield "asdf"
async def z(): return y()

The correspond to the following types:

x: Callable[[], Awaitable[str]]
y: Callable[[], AsyncIterator[str]]
z: Callable[[], Awaitable[AsyncIterator[str]]]

In the current world, we annotate these like so:

async def x() -> str: return "asdf"
async def y() -> AsyncIterator[str]: yield "asdf"
async def z() -> AsyncIterator[str]: return y()

There is an alternative, fully explicit world in which we annotate them like this. This would avoid the confusion here, but make async types quite cumbersome to use:

async def x() -> Awaitable[str]: return "asdf"
async def y() -> AsyncIterator[str]: yield "asdf"
async def z() -> Awaitable[AsyncIterator[str]]: return y()

@zzzeek
Copy link
Author

zzzeek commented Apr 24, 2022

I understand all of that. the issue is that a method that has ... for the body is at least in my mind a nomenclature that indicates "this method is not actually implemented, it's only for illustrating the calling interface". in such a case, mypy should give the developer the benefit of the doubt and not assume that the actual method body does not contain a yield.

if ... doesn't actually have that meaning, then at least a body that raises NotImplementedError should be recognized as such, and in this case mypy still thinks the method is implemented.

the bigger issue here is that when libraries move from .pyi files to inline typing, there still needs to be facilities to create interfaces that are "typing only", classes, methods and functions that aren't actually used at runtime. My assumption was that usage of the ellipses ... is generally understood even in .py files to be more or less meaning "this is a stub", such as the way we use them in @overload etc.

@zzzeek
Copy link
Author

zzzeek commented Apr 24, 2022

in fact I know that mypy considers ... to mean "not implemented", because the ... object is not an AsyncIterator[str] either yet no error is raised. this is documented by mypy at https://mypy.readthedocs.io/en/stable/stubs.html?highlight=ellipses#using-stub-file-syntax-at-runtime .

So I think this is still a bug. mypy should not be assuming anything about the contents of this method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong
Projects
None yet
Development

No branches or pull requests

3 participants