False positive "Abstract methods for async generators should use def, not async def"

[baum314nk]

Describe the Bug

Hi pyrefly team,
first of all: big thanks for your work on this great new type checker. Really looking forward to the v1 release!

Today I ran pyrefly on an existing codebase using beanie, a pydantic-based ODM for mongoDB, and encountered a weird error I can't quite make sense of. I also couldn't reproduce it in pyright or mypy.
I could reduce it to a minimum example:

from abc import ABC, abstractmethod
from beanie import Document

class CustomModel[T: Document](ABC):
    @abstractmethod
    async def to_db(self) -> T:
        pass

Throws the error Abstract methods for async generators should use def, not async def on the to_db function.

The Document class of beanie inherits from pydantics BaseModel in the end. When replacing the type var bound with the parent classes of Document the error disappears, so I guess there must be something in the Document class that let's pyrefly think it's an async generator (I checked the definition of the collections.abc.AsyncGenerator interface but couldn't find common parts).

Hope someone can identify the root cause of this error :)

Environment

Python 3.12.8
beanie 2.0.0
pyrefly 0.42.1

Sandbox Link

No response

(Only applicable for extension issues) IDE Information

No response

[yangdanny97]

Hm yeah I can't repro this in the sandbox with a stub class for Document

https://pyrefly.org/sandbox/?project=N4IgZglgNgpgziAXKOBDAdgEwEYHsAeAdAA4CeS4ATrgLYAE2MGEMdENxulALnQCK4AxgFcaMdNwA66MNXqpsgthy68AggCEAwgBo6CuN0qpB3MdwAWuTNMFRUcOHS3DDtALLWYUANoAVRH4hUXFuAF0ACk0tAEpEaTpEugABAyMTMxhLawSkh1J0JUwYMDpuXAB9HAi4bzAYugBaAD46ANykpOIHBB0QMkoSqFJCcpooCgBiOgAFUkGwYbo0LDx8OkFcGQgAc2FjbggtwmlpgGUYVgtubmI4RAB6B4Ghka4dh-EHzCE4B83tnsDkd0A86GAuPoAG6oaAKWAbLaQIGoQ5bOi4Yho9BwE7oMjZdCNKEwShwEF0AC8dEkIAAzIQAIwAJlp0hAfUYzBgJFIEAodh6QREYgkgW6jhAAF8%2BhkICSAGLQGAUFY4Ai86VAA

The code pointer for the error is here:

pyrefly/pyrefly/lib/alt/function.rs

Line 339 in 4edd5bb

"Abstract methods for async generators should use `def`, not `async def`"

So it looks like it's checking for async iterator, not async generator:

https://github.com/python/typeshed/blob/e19edcea117f6737d60e53d8a4001d0853eee5ac/stdlib/typing.pyi#L620

Though it doesn't look like Document has __aiter__ and __anext__ either, based on runtime inspection.

Maybe there's something going wrong with the unpacking logic.

[yangdanny97]

PHEW this was a tricky one.

So the reason we're treating Document as a subtype of AsyncIterator[] is because we think one of its base classes is Any (classes extending Any have behavior similar to Any)

As it turns out, if we fail to parse a base class type, we mark it as Any:

pyrefly/pyrefly/lib/alt/class/class_metadata.rs

Line 104 in e077295

| BaseClassParseResult::InvalidExpr(..)

Debug printing shows that it's the LazyModel base class

https://github.com/BeanieODM/beanie/blob/b090d6c37afa7d2b5909e594555c60bbcb38cbb5/beanie/odm/documents.py#L156C5-L156C14

InvalidType(Union(Union { members: [ClassType(ClassType(Class(ClassInner { index: ClassDefIndex(0), qname: QName { name: Identifier { id: Name("LazyModel"), range: 269..278, node_index: NodeIndex(None) }, parent: NestingContext(Toplevel), module: ModuleName("lazy_model.parser.old"), path: ModulePath(FileSystem(/home/yangdanny/scratch/.venv/lib/python3.12/site-packages/lazy_model/parser/old.py)) }, tparams: Some(TParams([])), .. }), TArgs((TParams([]), [])))), ClassType(ClassType(Class(ClassInner { index: ClassDefIndex(0), qname: QName { name: Identifier { id: Name("LazyModel"), range: 294..303, node_index: NodeIndex(None) }, parent: NestingContext(Toplevel), module: ModuleName("lazy_model.parser.new"), path: ModulePath(FileSystem(/home/yangdanny/scratch/.venv/lib/python3.12/site-packages/lazy_model/parser/new.py)) }, tparams: Some(TParams([])), .. }), TArgs((TParams([]), []))))], display_name: None }), 4093..4102)

We're unable to handle unions in the base class list

Looking at the code in lazy_model, we can see that the union comes from a conditional import (we're only able to resolve conditional imports statically if it's stuff like sys.platform/sys.version/TYPE_CHECKING): https://github.com/BeanieODM/lazy_model/blob/main/lazy_model/main.py#L6

What they do here (string slicing + casting to int) is difficult to resolve statically

So I guess there are 3 issues:

1. Any (and classes that extend Any) should not pass checks if the result of passing the check is emitting an extra error - for all our unwrapping functions we already check for Any, but not for classes that extend Any. In this specific case, we need to also check for Type Vars bounded by classes that extend Any 😵‍💫😵‍💫😵‍💫
2. We can't handle unions in the base class list
3. We treat base classes we can't handle as Any, which is bad when combined with the first issue

[yangdanny97]

cc @grievejia for opinions on the base class handling

[grievejia]

Couple of thoughts here:

• I think generally we want to be more conservative with the check -- i.e. avoid firing this warning when we are unsure if the given class is an AsyncIterator or not. Classes that extends Any may fall into this category.
• I don't think we should support having unions in the base class list. It's too hard for me to define the semantics of it. All other type checkers I tried agreed.
• There's also an option of not treating classes with invalid bases as extending Any. I suspect it's going to have a very big downstream impact if we do that, but I haven't tried it before so it's unknown to me whether my suspicion aligns with reality or not.
• I also wouldn't oppose making a PR to BeanieODM to address the issue, given that what they are doing is unlikely to be supported by most type checkers. We could do something like e.g.

import pydantic
from typing import TYPE_CHECKING

major_version = int(pydantic.version.VERSION.split(".")[0])

if TYPE_CHECKING:
    # Type checkers generally needs more consistency about where the class comes from
    from lazy_model.parser.new import LazyModel  # noqa: F401
else:
    if major_version == 1:
        from lazy_model.parser.old import LazyModel  # noqa: F401
    elif major_version == 2:
        from lazy_model.parser.new import LazyModel  # noqa: F401
    else:
        raise NotImplementedError(
            f"pydantic version {major_version} not supported"
        )

[baum314nk]

Thank you so much for digging into that!
I agree that using a fixed import for type checking makes sense here.

On a side note, I feel like it would be beneficial to have some PEP to support this package version check in static analysis. There are a lot of libs that want to support pydantic v1&2 without tons of code duplication.

[baum314nk]

I can take care of contributing the suggested fix to beanie!

[yangdanny97]

There's also an option of not treating classes with invalid bases as extending Any

I have a diff up for this, internal CI says 60 new errors for Pytorch

I feel like it would be beneficial to have some PEP to support this package version check in static analysis

Based on the docs on Pyrefly's built-in Pydantic support, I believe we only intend to support Pydantic v2. So Jia's suggested workaround of an if TYPE_CHECKING block sounds reasonable and doesn't result in that much extra code.

I can take care of contributing the suggested fix to beanie!

Thanks!