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

Should __class_getitem__ be considered part of the interface of Protocol classes? #11886

Closed
AlexWaygood opened this issue Jan 2, 2022 · 11 comments · Fixed by #15490
Closed
Labels
bug mypy got something wrong topic-protocols

Comments

@AlexWaygood
Copy link
Member

In python/typeshed#5869, __class_getitem__ was added to the stub for os.PathLike. The method exists on the class at runtime; however, the addition of the method caused regressions in mypy 0.920. PathLike is defined in typeshed as a protocol, and so the addition of the method to the stub led mypy to erroneously believe that a custom class could only be considered a subtype of PathLike if it implemented __class_getitem__. The change to the stub had to be reverted in python/typeshed#6591, in time for mypy 0.930.

The question is: is there a situation where it is ever useful for __class_getitem__ to be considered part of a protocol interface? Or, would it be better for __class_getitem__ to be special-cased, such that mypy does not consider it to be part of the interface, even if it is defined on a Protocol class?

Related: #11884, where a similar issue is being discussed w.r.t. __slots__. Cc. @sobolevn, who is working on the __slots__ issue, and @hauntsaninja, who filed the PR to fix the PathLike regression.

@AlexWaygood AlexWaygood added the bug mypy got something wrong label Jan 2, 2022
@AlexWaygood AlexWaygood changed the title Should __class_getitem__ be considered Should __class_getitem__ be considered as part of the interface of Protocol classes? Jan 2, 2022
@AlexWaygood AlexWaygood changed the title Should __class_getitem__ be considered as part of the interface of Protocol classes? Should __class_getitem__ be considered part of the interface of Protocol classes? Jan 2, 2022
@sobolevn
Copy link
Member

sobolevn commented Jan 2, 2022

How it works right now:

from typing import Protocol, runtime_checkable

@runtime_checkable
class ClassGetItem(Protocol):
    def __class_getitem__(cls, val) -> object:  # or `types.GenericAlias`
        pass

class Example:
    pass

def func(klass: ClassGetItem):
    pass

print(issubclass(Example, ClassGetItem))
func(Example())  # E: Argument 1 to "func" has incompatible type "Example"; expected "ClassGetItem"

What runtime does? It prints True!
What pyright says?

Argument of type "Example" cannot be assigned to parameter "klass" of type "ClassGetItem" in function "func"
  "Example" is incompatible with protocol "ClassGetItem"
    "__class_getitem__" is not present

So, it is complicated!

@sobolevn
Copy link
Member

sobolevn commented Jan 2, 2022

I am going to CC @erictraut as well

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jan 2, 2022

It looks as though __class_getitem__ and __slots__ are both explicitly listed in the source code as attributes the Python runtime will not consider when doing issubclass checks for protocols decorated with @runtime_checkable. So the result of the issubclass check is not an accident -- a deliberate decision was made by the core devs that it wouldn't make sense for __class_getitem__ to be considered part of the interface at runtime.

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jan 2, 2022

Git blame indicates that __class_getitem__ has been excluded from runtime issubclass checks for protocols since the addition of Protocol to the typing module in May 2019: python/cpython#13585.

@erictraut
Copy link

Currently pyright ignores any variables without type annotations when performing protocol comparisons, but if you add a type annotation to __slots__, it will fail the protocol comparison check if the class doesn't also declare a __slots__ variable.

It sounds like we should probably exclude both __slots__ and __class_getitem__ from normal class protocol checks.

I'm thinking we should leave the check for __class_getitem__ in place when comparing a class object (a type subclass) against a protocol. I mention this only for completeness since mypy doesn't currently support this part of PEP 544. ("A class object is considered an implementation of a protocol if accessing all members on it results in types compatible with the protocol members.")

Does that sound good? Here's a small unit test to clarify the proposal.

from typing import Any, Final, Iterable, Protocol

class B:
    ...

class C:
    def __class_getitem__(cls, __item: Any) -> Any:
        ...

class SupportsClassGetItem(Protocol):
    __slots__: str | Iterable[str] = ()
    def __class_getitem__(cls, __item: Any) -> Any:
        ...

b1: SupportsClassGetItem = B()  # OK (missing __class_getitem__ is ignored)
c1: SupportsClassGetItem = C()  # OK

b2: SupportsClassGetItem = B  # Error (missing __class_getitem__)
c2: SupportsClassGetItem = C  # OK

@AlexWaygood
Copy link
Member Author

To clarify, @erictraut, would the following code be supported under your proposal?

from types import GenericAlias
from typing import Any, Protocol, TypeVar

AnyStr_co = TypeVar("AnyStr_co", str, bytes, covariant=True)

class PathLike(Protocol[AnyStr_co ]):
    def __fspath__(self) -> AnyStr_co : ...
    def __class_getitem__(self, __item: Any) -> GenericAlias: ...

class StrPath:
    def __fspath__(self) -> str: ...

x: type[PathLike[str]] = StrPath
y = x()

This snippet currently passes mypy (with or without --strict) if I remove the __class_getitem__ method from PathLike, but fails with the __class_getitem__ method.

@erictraut
Copy link

Yes, the above example would pass with my proposal. It sounds like this meets the requirements for your use case, so I've implemented my proposal in pyright, and it will be included in the next release.

For completeness, this sample should also pass (if and when mypy adds support for class object protocol matching):

class PathLikeClass1(Protocol[AnyStr_co ]):
    def __fspath__(__self, self) -> AnyStr_co : ...

z1: PathLikeClass1[str] = StrPath

But this sample should not pass:

class PathLikeClass2(Protocol[AnyStr_co ]):
    def __fspath__(__self, self) -> AnyStr_co : ...
    def __class_getitem__(cls, __item: Any) -> GenericAlias: ...

z2: PathLikeClass2[str] = StrPath

@AlexWaygood
Copy link
Member Author

Yes, the above example would pass with my proposal. It sounds like this meets the requirements for your use case, so I've implemented my proposal in pyright, and it will be included in the next release.

For completeness, this sample should also pass (if and when mypy adds support for class object protocol matching):

class PathLikeClass1(Protocol[AnyStr_co ]):
    def __fspath__(__self, self) -> AnyStr_co : ...

z1: PathLikeClass1[str] = StrPath

But this sample should not pass:

class PathLikeClass2(Protocol[AnyStr_co ]):
    def __fspath__(__self, self) -> AnyStr_co : ...
    def __class_getitem__(cls, __item: Any) -> GenericAlias: ...

z2: PathLikeClass2[str] = StrPath

Great, then it sounds like we're in agreement. Thanks! 😀

@hauntsaninja
Copy link
Collaborator

A class object is considered an implementation of a protocol if accessing all members on it results in types compatible with the protocol members.

Should class objects with a __class_getitem__ be considered an implementation of a protocol requiring __getitem__?

from typing import Any, Protocol

class B:
    def __getitem__(self, __item: Any) -> Any: ...

class C:
    def __class_getitem__(cls, __item: Any) -> Any: ...

class Indexable(Protocol):
    def __getitem__(self, __item: Any) -> Any: ...

b1: Indexable = B()  # OK
c1: Indexable = C()  # Error

b2: Indexable = B  # Error (missing __getitem__)
c2: Indexable = C  # Error??

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jan 2, 2022

A class object is considered an implementation of a protocol if accessing all members on it results in types compatible with the protocol members.

Should class objects with a __class_getitem__ be considered an implementation of a protocol requiring __getitem__?

I'd argue using __class_getitem__ to satisfy an interface demanding an implementation of __getitem__ would be a bad anti-pattern! If you want a class object to satisfy that interface, you should be defining __getitem__ on the class's metaclass, rather than using __class_getitem__, which should be seen as an implementation detail of the typing system.

At least, that's what the docs on __class_getitem__ (recently rewritten by... me 😉) imply.

@hauntsaninja
Copy link
Collaborator

Great, less special casing it is.

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-protocols
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants