Skip to content

__init__ overloads in ABC class are not properly identified #12292

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
flisboac opened this issue Mar 4, 2022 · 3 comments
Closed

__init__ overloads in ABC class are not properly identified #12292

flisboac opened this issue Mar 4, 2022 · 3 comments
Labels
bug mypy got something wrong topic-overloads topic-paramspec PEP 612, ParamSpec, Concatenate

Comments

@flisboac
Copy link

flisboac commented Mar 4, 2022

Bug Report

Mypy doesn't seem to identify all overloads of the __init__ of an ABC class. Didn't test if the same happens for concrete classes, though.

To Reproduce

Please se this mypy playground: https://mypy-play.net/?mypy=latest&python=3.10&gist=0c727c040c7b0d58aa25931dc17cd09f

This can be reproduced with the following code snippet. Please read the comments in the code for some context in the example as well:

from abc import ABCMeta, abstractmethod
from typing_extensions import overload, ParamSpec
from typing import TypeVar, Sequence, Optional, Callable

PResourceParams = ParamSpec('PResourceParams')
TResource = TypeVar('TResource', bound='Resource', covariant=True)

# Multiple implementations of Resource will exist, but
# a small set of construction-time parameters are guaranteed
# to be supported by all implementations.
class Resource(metaclass=ABCMeta):

    @overload
    def __init__(self, pathname: str):
        ...

    @overload
    def __init__(self, *, path: Sequence[str], name: str):
        ...

    @abstractmethod
    def __init__(
        self,
        pathname: Optional[str] = None,
        *,
        path: Optional[Sequence[str]] = None,
        name: Optional[str] = None,
    ):
        ...

# It's also intended for other, more specialized base ABCs to exist,
# at a conceptual level.
class AnotherResourceRoot(Resource, metaclass=ABCMeta):
    ...

# A Provider will select a suitable Resource implementation
# based on conditions internal to its implementation.
# The intention is to validate the small subset of constructor
# parameters that are supported by all resource implementations,
# and complement them with defaults, or other parameters that
# must be injected by the provider implementation.
class ResourceProvider(metaclass=ABCMeta):
    
    # resource_type is explicitly a callable so that PResourceParams
    # can be captured, and some type safety regarding this default
    # set of parameters can be enforced. It also allows to return the
    # exact type (or base ABC) as requested.
    def resource(
        self,
        resource_type: Callable[PResourceParams, TResource],
        *args: PResourceParams.args,
        **kwargs: PResourceParams.kwargs,
    ) -> TResource:
        ...

def main(provider: ResourceProvider):
    # Unfortunately, mypy doesn't seem to understand the __init__
    # overloads. Errors in the next line:
    #
    #  > error: Unexpected keyword argument "path" for "resource" of "ResourceProvider"
    #  > error: Unexpected keyword argument "name" for "resource" of "ResourceProvider"
    #
    # mypy does understand/recognize, however:
    #
    # 1. The @abstractmethod __init__ in Resource, if I remove all overloads; and
    # 2. The first overload only; you can swap overload positions to make it work,
    #    but it's not the desired situation.
    #
    resource = provider.resource(
        AnotherResourceRoot,
        #'abc/def/ghi.def',
        path=['abc', 'def'],
        name='ghi.def',
    )
    reveal_type(resource)  #  => Resource

Expected Behavior

Should accept all overloads.

Actual Behavior

Only accepts the very first overload of __init__. Solution so far seems to be not to use overloads at all, and rely on just the single @abstractmethod signature.

Your Environment

  • Mypy version used: 0.931
  • Mypy command-line flags: (Please see mypy playground link)
  • Mypy configuration options from mypy.ini (and other config files): (Please see mypy playground link)
  • Python version used: 3.7
  • Operating system and version: (Please see mypy playground link)
@flisboac flisboac added the bug mypy got something wrong label Mar 4, 2022
@erictraut
Copy link

I don't know how (or whether) mypy's implementation of PEP 612 handles overloads. The PEP doesn't provide much guidance about how an overloaded function should bind to a ParamSpec. I think pyre handles this case in some manner. When I implemented PEP 612 in pyright, I chose not to support this case, so pyright emits an explicit error for your code above. Mypy doesn't emit an error, but it also doesn't appear to handle the binding of an overloaded signature to a ParamSpec.

@flisboac
Copy link
Author

flisboac commented Mar 5, 2022

@erictraut I know there's a PR going on regarding ParamSpec, but overall, support for it in mypy is somewhat decent. I tried it and in most places it works very well -- although things like Concatenate are entirely not implemented.

Now, it seems the ParamSpec is properly bound, but only for the first overload. You can swap overload positions, and they'll work as expected, which for me hints at the possibility of mypy not identifying all overloads in this specific scenario. I don't know if the problem is with __init__ or with the fact that the class is an ABC, or if it's something else (e.g. what if I used __new__ instead, would it be different?)

But I agree that ParamSpec could affect overload selection. Maybe the argument matching is not working, or is not implemented yet, which prevents mypy from selecting anything other the immediate/default overload (which in this case seems to be the first).

So far it's the first time I tried such an idiom (ie. __init__ to fix constructor arguments, and ParamSpec to avoid replicating them in factory functions), and it's very much possible that this is not the intended way those functionalities should be used (if that's the case, please do tell me!).

@ilevkivskyi
Copy link
Member

This seem to typecheck correctly now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong topic-overloads topic-paramspec PEP 612, ParamSpec, Concatenate
Projects
None yet
Development

No branches or pull requests

4 participants