Skip to content

typing: remove metaclass from Sized #9058

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

Merged
merged 1 commit into from
Nov 2, 2022

Conversation

hauntsaninja
Copy link
Collaborator

@github-actions
Copy link
Contributor

github-actions bot commented Nov 1, 2022

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we please add a regression test? This seems quite important to catch.

@hauntsaninja
Copy link
Collaborator Author

hauntsaninja commented Nov 1, 2022

The existing mypy test will catch it if you run it with latest mypy. The change in behaviour on mypy side bisects back to python/mypy#13579

@srittau
Copy link
Collaborator

srittau commented Nov 1, 2022

Wouldn't it make more sense to fix this in mypy?

@sobolevn
Copy link
Member

sobolevn commented Nov 1, 2022

I agree, this looks like a mypy bug, not a typeshed bug.

@JukkaL
Copy link
Contributor

JukkaL commented Nov 1, 2022

I agree that this looks like a mypy bug, but the metaclass declaration doesn't seem correct so this still PR seems reasonable:

>>> type(Protocol)
<class 'typing._ProtocolMeta'>

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 1, 2022

but the metaclass declaration doesn't seem correct

It does feel more correct than not having the metaclass declaration, though, since _ProtocolMeta is a subclass of ABCMeta

@srittau
Copy link
Collaborator

srittau commented Nov 1, 2022

Also, the behavior is very ABCMeta-like:

Python 3.9.2 (default, Feb 28 2021, 17:03:44) 
[GCC 10.2.1 20210110] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from typing import Sized
>>> Sized()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.9/typing.py", line 670, in __call__
    result = self.__origin__(*args, **kwargs)
TypeError: Can't instantiate abstract class Sized with abstract method __len__

That said, if this is not an easy fix in mypy, I'm fine with reverting for now.

Edit: Reverting with a comment linking to the appropriate mypy issue.

@AlexWaygood
Copy link
Member

I've just realised that at runtime, all classes directly inheriting from Protocol automatically have _ProtocolMeta as their metaclass. Maybe ideally type checkers should understand that all protocols have [a subclass of] ABCMeta as the metaclass? Which would mean that we wouldn't need any of these metaclass=ABCMeta declarations in typing.pyi.

But yeah, I agree with @srittau — whatever the case, it's not really a big deal, so I'm fine with this being merged for now.

@JukkaL
Copy link
Contributor

JukkaL commented Nov 1, 2022

A subclass of Protocol is implicitly an ABC, so explicitly giving the metaclass seems redundant, and we don't do it consistently in typeshed right now -- many protocols don't have an explicit metaclass.

Relevant example:

import abc
from typing import Protocol

class P(Protocol):
    @abc.abstractmethod
    def f(self) -> None: pass

class C(P):
    pass

C()  # TypeError: Can't instantiate abstract class C with abstract method f

I think that type checkers should infer the correct metaclass automatically. Mypy doesn't seem to do it right now. I've filed an issue about this: python/mypy#13979

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 1, 2022

Probably the reason why we specify the metaclass so consistently in typing.pyi is because at runtime these classes aren't actually protocols at all (they're ABCs, defined in _collections_abc.py rather than typing.py). They used to be ABCs in the stub, as well; it looks like laboriously specifying the metaclass in typing.pyi is a holdover from before these classes were made protocols in the stub in ec2cb8e

@JelleZijlstra
Copy link
Member

Why does the metaclass cause problems for Sized but not for Hashable and SupportsIndex, which look the same in the stub?

@sobolevn
Copy link
Member

sobolevn commented Nov 1, 2022

Related #8998

@erictraut
Copy link
Contributor

Many classes in typeshed claim to derive from Protocol but do not actually derive from Protocol at runtime. For example, according to the typeshed stubs, str derives from Sequence which derives from Collection which derives from Protocol. If a type checker were to assume that Protocol has a metaclass of _ProtocolMeta, an attempt to subclass from str and Enum will fail at type checking time but succeed at runtime. Unless typeshed can distinguish between a "real" Protocol and a "fake" Protocol, I don't think type checkers can properly detect when the introduction of a custom metaclass will generate a runtime exception.

@hauntsaninja
Copy link
Collaborator Author

Filed python/mypy#13986 for fixing the issue in mypy, merging this in the meantime, since srittau and alexwaygood are okay with it.

@hauntsaninja hauntsaninja merged commit a3ce512 into python:main Nov 2, 2022
@hauntsaninja hauntsaninja deleted the typing-siz branch November 2, 2022 01:38
@JukkaL
Copy link
Contributor

JukkaL commented Nov 2, 2022

To summarize the discussion above, this works around a bug in mypy and the original ABCMeta metaclass was correct, since Sized isn't a Protocol subclass at runtime. After python/mypy#13986 is fixed, this change can be reverted.

jab added a commit to jab/typeshed that referenced this pull request Dec 10, 2022
JelleZijlstra pushed a commit that referenced this pull request Dec 10, 2022
* Revert "`Collection` is `Sized` (#8977)"

This reverts commit 5bbba5d.

* Revert "typing: remove metaclass from Sized (#9058)"

This reverts commit a3ce512.

* Add regression test for issue 9296.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants