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

Inconsistent Treatment of Class Constructor by Protocol #10482

Closed
rmorshea opened this issue May 16, 2021 · 7 comments · Fixed by #14121
Closed

Inconsistent Treatment of Class Constructor by Protocol #10482

rmorshea opened this issue May 16, 2021 · 7 comments · Fixed by #14121
Labels

Comments

@rmorshea
Copy link

In the code below we define one protocol which, when called, returns another. In effect we're describing a "factory". However, while MyPy is able to recognize incorrect protocol implementations when given a function, it fails to do the same for class constructors.

Playground link: https://mypy-play.net/?mypy=latest&python=3.9&gist=3b9250b17032b659fc36746e659bcdb5

from typing import Protocol

class Factory(Protocol):
    def __call__(self) -> Result:
        ...

class Result(Protocol):
    a: int
    
class BadImplementation:
    ...

def bad_implementation() -> BadImplementation:
    ...

something: Factory = BadImplementation  # no error
something_else: Factory = bad_implementation  # yes error

We would expect MyPy to give an error in both cases given they result in the same type, but it only complains when given a standard function.

@ilevkivskyi
Copy link
Member

Hm, this is because in typeshed type.__call__ is annotated as returning (*Any, **Any) -> Any, making all class objects implementing arbitrary callback protocols. Mypy uses metaclass as a fallback for protocol implementation if implementation is a class object, so we will need to either tighten definition in typeshed, or ignore metaclass __call__ if constructor didn't match.

TBH I am not sure which one is better.

@JelleZijlstra
Copy link
Member

Typeshed is technically right there: type.__call__ takes arbitrary arguments (https://github.com/python/cpython/blob/5cfb7d19f5242c9b8ffd2fe30a24569e85a99e1d/Objects/typeobject.c#L1235). So it's probably best to handle this case specially in mypy.

@ilevkivskyi
Copy link
Member

ilevkivskyi commented Nov 16, 2022

Problem is not much in what it takes, but in what it returns, it should be object, not Any. I guess this will fix 95% of relevant use cases. But maybe we can try ignoring __call__, it is just that it is a bit of pain, is_(proper)_subtype() already have like a dozen random flags, this will need one more. Unless, we just want to always skip __call__ on a metaclass (or only on builtins.type?)

@JelleZijlstra
Copy link
Member

I see, opened python/typeshed#9215 to change it. Let's see if there's any negative effects.

@ilevkivskyi
Copy link
Member

Btw funny thing is that we have exactly a test like the original repro, but it passes, because we use test fixtures for most protocol tests, not full typeshed stubs.

@ilevkivskyi
Copy link
Member

@JelleZijlstra Hm, it looks like it didn't work (see mypy_primer). People often want to instantiate items in list of unrelated class objects. I guess we need to come up with some skipping/ignoring rules.

@JukkaL
Copy link
Collaborator

JukkaL commented Nov 16, 2022

The return type being a protocol isn't important. I think that we should compare the signature of __init__ from the actual type object against the callable protocol, instead of type.__call__ from typeshed. I don't think that we can fix this in typeshed.

Another example:

from typing import Protocol

class P(Protocol):
    def __call__(self, x: int) -> C: ...

class C:
    def __init__(self, x: str) -> None: ...

def f(x: str) -> C:
    return C(x)

x: P = C  # no error
y: P = f  # yes error

JukkaL pushed a commit that referenced this issue Nov 17, 2022
…4121)

Fixes #10482 

This is not very principled, but should work except people will want to
explicitly check some metaclass `__call__`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants