WIP: Mapping.{get,pop} can return default type

[tharvik]

Do not merge yet, it makes some tests from mypy fail, first merge it in mypy.

When using a Mapping, there is usually the idea to get or pop a key with a default of an existing guard object (instead of trying and expecting an exception). But in current typeshed, it is stated as

class Mapping(Generic[_KT, _VT]):
    def get(self, k: _KT, default: _VT = ...) -> _VT: ...

Wouldn't it be more correct to have something like this?

_DT = TypeVar('_DT')
class Mapping(Generic[_KT, _VT]):
    def get(self, k: _KT, default: _DT = ...) -> Union[_VT, _DT]: ...

thus also respecting the definition of dict.get which is stated in the documentation as

Return the value for key if key is in the dictionary, else default. If default is not given, it defaults to None, so that this method never raises a KeyError.

[matthiaskramm]

Yes, that looks better. That's what pytype has in its builtins, too: https://github.com/google/pytype/blob/master/pytype/pytd/builtins/__builtin__.pytd#L485

[gvanrossum]

I think pop() will need to be overloaded -- with one arg there's no possibility of it returning the default value (unlike get()).

[tharvik]

I took the path of overloading, it seems nicer (and clearer). We still need to merge the mypy PR though.

[rwbarton]

This overload variant (and all the others like it) should no longer have a default = ....

[tharvik]

Ho, correct, thanks for reviewing.

[rwbarton]

The overload will be needed under strict optional checking anyways, as otherwise there's no way to have the default default be None without always requiring the return type to contain None, even when a default is provided.

With this change we can also make Mapping covariant in its value type, as it should be.

[tharvik]

With this change we can also make Mapping covariant in its value type, as it should be.

So I did, but, you may want to check it because I just replaced _VT to _VT_to.

[gvanrossum]

Sadly this broke more than mypy. Our two internal repos each showed several problems due to this. I think I'll go roll this back so that some other mypy developer doesn't accidentally release mypy with this incorporated.

[gvanrossum]

Here's a simple repo: it seems to occur whenever the mapping keys are a tuple:

    from typing import Mapping, Dict, Tuple
    a = {('0', '0'): 42}  # type: Dict[Tuple[str, str], int]
    b = a.get(('1', '1'), 0)

This gives an error on the last line:

error: No overload variant of "get" of "dict" matches argument types [Tuple[builtins.str, builtins.str], builtins.int]

A similar error shows when the type uses Mapping instead of dict.

[gvanrossum]

So python/mypy#1595 is fixed, but there are still some remaining issues with this PR in our internal repos. I will see if I can find the time to understand those.

[gvanrossum]

Note to self: this was reverted, but I want to try again, because I still
think this is the right thing to do.

On Sat, May 28, 2016 at 2:09 AM, Valérian Rousset [email protected]
wrote:

With this change we can also make Mapping covariant in its value type, as
it should be.

So I did, but, you may want to check it because I just replaced _VT to
_VT_to.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#223 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/ACwrMonRrERcvt7XU2MvjxkjOcLv5-UHks5qGAZbgaJpZM4Imcv3
.

--Guido van Rossum (python.org/~guido)

[gnprice]

Because this PR was merged, it'd be easy for us to lose track of there still being something to do here, so I filed a new issue #278 for round 2.