Skip to content

The type of __default= in dict.pop and dict.get doesn't look right #10293

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

Closed
superbobry opened this issue Jun 9, 2023 · 5 comments · Fixed by #10501
Closed

The type of __default= in dict.pop and dict.get doesn't look right #10293

superbobry opened this issue Jun 9, 2023 · 5 comments · Fixed by #10501

Comments

@superbobry
Copy link
Contributor

superbobry commented Jun 9, 2023

dict.pop is currently defined as

typeshed/stdlib/builtins.pyi

Lines 1086 to 1089 in 1851423

@overload
def pop(self, __key: _KT) -> _VT: ...
@overload
def pop(self, __key: _KT, __default: _VT | _T) -> _VT | _T: ...

Note the union with two type parameters as a type of __default=. This type is ambiguous if dict.pop is called with a value of type _VT. For example, given

d: dict[str, str]
d.pop("foo")

a type checker can choose any value for _T, since str is a subtype of str | _T regardless of what _T is.

It seems the union was introduced in 2fdcd2e, which unfortunately does not seem to give any reasoning for that. I think we should drop it and use _T instead in both dict.pop and dict.get (and similarly for MutableMapping), i.e.

class dict(Generic[_KT, _VT]):
   # ...
   @overload
   def pop(self, __key: _KT, __default: _T) -> _T: ...
@JelleZijlstra
Copy link
Member

Yes, I think you're right. Feel free to send a PR!

@superbobry
Copy link
Contributor Author

Will do!

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jun 9, 2023

I'm a little confused. In your example, why would d.pop("foo") match the def pop(self, __key: _KT, __default: _VT | _T) -> _VT | _T: ... overload (since __default does not have a default value in that overload)?

But I'm fine with anything as long as the following three cases type check:

from typing import assert_type

def f(d: dict[str, int]):
    assert_type(d.pop("key"), int)
    assert_type(d.pop("key", 1), int)
    assert_type(d.pop("key", None), int | None)

@superbobry
Copy link
Contributor Author

superbobry commented Jun 9, 2023

Ooops, sorry. I was meaning to write d.pop("foo", "bar"). I've updated the type to be dict[str, str] to minimize the edits to the original post.

I think all three cases should type check with the proposed definition.

@superbobry
Copy link
Contributor Author

Okay, another correction. The right signature for seems to be

class dict(Generic[_KT, _VT]):
   # ...
   @overload
   def pop(self, __key: _KT, __default: _T) -> _VT | _T: ...

because a dict can return a value from an existing entry.

superbobry added a commit to superbobry/typeshed that referenced this issue Jun 9, 2023
superbobry added a commit to superbobry/typeshed that referenced this issue Jun 9, 2023
superbobry added a commit to superbobry/typeshed that referenced this issue Jun 9, 2023
superbobry added a commit to superbobry/typeshed that referenced this issue Jun 9, 2023
eltoder added a commit to eltoder/typeshed that referenced this issue Jul 22, 2023
eltoder added a commit to eltoder/typeshed that referenced this issue Jul 22, 2023
AlexWaygood added a commit that referenced this issue Jul 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants