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

Disallow Mapping as argument type? #3576

Closed
srittau opened this issue Jan 5, 2020 · 5 comments
Closed

Disallow Mapping as argument type? #3576

srittau opened this issue Jan 5, 2020 · 5 comments
Labels
project: policy Organization of the typeshed project

Comments

@srittau
Copy link
Collaborator

srittau commented Jan 5, 2020

This came up in #3569. See also the later comments in python/typing#11 for background. Mapping is not a protocol (as opposed to most other typing ABCs). This mean code like the following will not work:

from typing import Iterator, Mapping

class MyMap:
    def __getitem__(self, key: str) -> int:
        return 0
    def __iter__(self) -> Iterator[str]:
        return iter([])
    def __len__(self) -> int:
        return 0
    def __contains__(self, key: object) -> bool:
        return False

def foo(x: Mapping[str, int]) -> None:
    pass

foo(MyMap())  # error: Argument 1 to "foo" has incompatible type "MyMap"; expected "Mapping[str, int]"

Deriving MyMap from Mapping[str, int] works, though. I suggest we don't allow new submissions to use Mapping as argument type and work towards removing existing uses. Instead they should use a custom protocol, which can also be narrower than Mapping.

@srittau srittau added the project: policy Organization of the typeshed project label Jan 5, 2020
@djb4ai
Copy link
Contributor

djb4ai commented Mar 26, 2020

@srittau I am trying to create a Protocol in PR#3877 but when I use Protocol[_KT, _VT_co] it says Invariant type variable '_KT' used in protocol where contravariant one is expected, can't understand what could be causing this.

srittau added a commit to srittau/typeshed that referenced this issue Jul 10, 2020
typing.Mapping is not a protocol, which has caused problems in the past.
(E.g. python#3569, see also python#3576.) This
introduces a few narrow protocols to _typeshed.pyi that can be used in
place of Mapping.

Not all uses of Mapping can be replaced. For example, cgi.FieldStorage
explictly checks whether the supplied headers argument is a Mapping
instance.
JelleZijlstra pushed a commit that referenced this issue Jul 12, 2020
typing.Mapping is not a protocol, which has caused problems in the past.
(E.g. #3569, see also #3576.) This
introduces a few narrow protocols to _typeshed.pyi that can be used in
place of Mapping.

Not all uses of Mapping can be replaced. For example, cgi.FieldStorage
explictly checks whether the supplied headers argument is a Mapping
instance.
@jab
Copy link
Contributor

jab commented Aug 12, 2020

Rather than disallowing Mapping as an argument type because Mapping is not a protocol, is it better to instead make Mapping a protocol? That seems like it would result in better usability than the alternative of forbidding Mapping arguments. Worth writing down the pros and cons of both options before picking which one? Related protocols (SupportsGetItem, etc.) were recently added in #4325 since this issue was created. Does that change things?

Relatedly, if we had python/mypy#2922, I guess Mapping.register(MyMap) could be added to the example in the issue description to resolve the error, but mypy does not yet support ABCMeta.register.

@srittau
Copy link
Collaborator Author

srittau commented Aug 12, 2020

I think it's hard to make Mapping a protocol, since it's an abstract base class in collections.abc, and since it's so widely used. But maybe it would make sense to introduce a separate MappingProto (or similar) protocol and use that going forward. Although, using more specific protocols is still more useful, but harder to implement.

@gvanrossum
Copy link
Member

Indeed. We looked into this and found it would be very difficult. I strongly recommend just inheriting from Mapping over coming up with yet another duck typing protocol. FWIW mypy probably will never support .register().

@AlexWaygood
Copy link
Member

We have _typeshed.SupportsKeysAndGetItem now, and use it quite widely. It's a very useful protocol.

Possibly we could look into adding lints to detect bad uses of Mapping in argument annotations. The issue tracker at flake8-pyi would probably be a better place to discuss that, though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
project: policy Organization of the typeshed project
Projects
None yet
Development

No branches or pull requests

5 participants