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

wrong positional-or-keyword information in Mapping and MutableMapping #13432

Open
beauxq opened this issue Jan 24, 2025 · 2 comments
Open

wrong positional-or-keyword information in Mapping and MutableMapping #13432

beauxq opened this issue Jan 24, 2025 · 2 comments

Comments

@beauxq
Copy link

beauxq commented Jan 24, 2025

Mapping and MutableMapping information for get and pop says that the default parameter is positional or keyword.

dict says positional only, and that is correct at runtime.

So this type checks with no errors, but crashes with TypeError:

from collections.abc import Mapping, MutableMapping


def foo(x: MutableMapping[str, int]) -> int:
    return x.pop("3", default=3)


foo({})  # crash: TypeError: dict.pop() takes no keyword arguments


def goo(x: Mapping[str, int]) -> int:
    return x.get("3", default=3)


goo({})  # crash: TypeError: dict.get() takes no keyword arguments


# same with MutableMapping.get
@Daverball
Copy link
Contributor

Daverball commented Feb 1, 2025

Yes, that's because dict violates LSP, which is documented in the stubs:

typeshed/stdlib/builtins.pyi

Lines 1125 to 1131 in c193cd2

# Positional-only in dict, but not in MutableMapping
@overload # type: ignore[override]
def get(self, key: _KT, /) -> _VT | None: ...
@overload
def get(self, key: _KT, default: _VT, /) -> _VT: ...
@overload
def get(self, key: _KT, default: _T, /) -> _VT | _T: ...

There's two ways to fix this, but both of them would need to happen in CPython, not in typeshed:

  1. We change the implementation of dict, so it accepts the keyword default parameter in addition to a positional one.
  2. We change the implementation of Mapping so it no longer accepts default as a keyword parameter.

While 1. would be less disruptive, it will potentially add some small overhead to every dict.get call. But changing the API contract of an ABC is a breaking change (especially, because Mapping.get has a default implementation), even if that very same contract is violated in the standard library. So it would be more difficult to push through.

Prototyping the second change in typeshed would allow us to see if any of the projects we check with mypy_primer rely on the current definition of Mapping.get. A code search will probably not provide anything conclusive, since get is such a generic method name. Although in a quick and dirty search I found at least one instance where people relied on os.environ.get allowing a keyword default, which would be broken, if the default implementation of Mapping.get changed, unless os._Environ provided a new get implementation, which does accept keyword arguments.

If we can very quickly find one such instance in the standard library, there's a high likelihood there's thousands more in both the standard library and other people's code.

@JelleZijlstra
Copy link
Member

It probably makes sense for us to claim in typeshed that Mapping/MutableMapping use positional-only arguments, since that's what dict does and in practice it's by far the most common mapping type. I think we do something similar in a few other places already.

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

No branches or pull requests

3 participants