Overload signature of get to return an Optional value and to allow default to take any type to match runtime behavior.

[rowillia]

This chage more closely matches the behavior of get at runtime. Users can pass whatever they want in to the default
parameter and it will be returned if the key is absent. Additionally, get should return an Optional if called with
only one parameter.

test_get_default.py

z = {'a': 22}
reveal_type(z.get('b'))
reveal_type(z.get('b', 22))
reveal_type(z.get('b', 'hello'))

Before:

test_get_default.py:2: error: Revealed type is 'builtins.int*'
test_get_default.py:3: error: Revealed type is 'builtins.int*'
test_get_default.py:4: error: Revealed type is 'builtins.int*'
test_get_default.py:4: error: Argument 2 to "get" of "dict" has incompatible type "str"; expected "int"

After:

test_get_default.py:2: error: Revealed type is 'Union[builtins.int*, builtins.None]'
test_get_default.py:3: error: Revealed type is 'builtins.int'
test_get_default.py:4: error: Revealed type is 'Union[builtins.int, builtins.str*]'

[ambv]

LGTM. We also tested this against mypy's own tests with master, everything passes.

Rationale for accepting:

• fixes the API that should always have been using Optional as the return type from get()
• accepting any type for default= is the runtime behavior so even if it's not elegant, it describes reality
• if the value returned by default is incompatible for later use in the program, mypy will report this problem anyway, so no information is lost

[gvanrossum]

A heads up that this actually caused a bunch of errors in one of our internal codebase. I don't have time to go over those right now to see whether they are newly discovered bugs in our code or point to cases where the new signature of get() is still problematic, but I'll get back to this later.

[ambv]

Will you have time to go over this before 0.470 or would you prefer this be temporarily reverted?

[gvanrossum]

I propose to just not sync typeshed past this commit. For 0.470 we may not sync typeshed at all, or only past the commit that restores the symlink.

[rowillia]

@gvanrossum yeah, this could be a fairly disruptive chance to pull in last minute into mypy.

FWIW we found a bunch of legit bugs in our code base when I added this stub in. Things of the form:

for x in foo.get('hello'): #  oops
   pass

[ambv]

This issue comes back and back again, see python/mypy#278 for links to several PRs and issues about it.

What this particular PR changes is there is now one new Union[] as a return type. I recall @JukkaL mentioning a few times that currently Mypy has issues dealing with it (for example, see relevant comment here).

If this is what @gvanrossum is seeing failing now in the Dropbox codebase, a workaround in this case would be to rework the union into two separate overloads. Before:

@overload
def get(self, k: _KT) -> Optional[_VT]: ...
@overload
def get(self, k: _KT, default: _T) -> Union[_VT, _T]: ...

After:

@overload
def get(self, k: _KT) -> Optional[_VT]: ...
@overload
def get(self, k: _KT, default: _T) -> _VT: ...
@overload
def get(self, k: _KT, default: _T) -> _T: ...

[ambv]

Yeah, I think this is indeed what is happening. And long story short (explained in detail by Jukka in python/mypy#1693), Mypy considers operations to be valid for unions when they are valid for each element of the union. For return types, this surprisingly limits and not broadens what the type checker considers valid. If you want to broaden, you should use overloads with different argument signatures instead.

We should definitely get #1693 out there in docs, and possibly amend PEP 484 with a section about this. It's a subtlety that is bound to bite us in the future.

[gvanrossum]

Please do send a PR for PEP 484!

[ambv]

I will.

In this particular case, I think the Union return type is a reasonable thing to do, though. It warns the user that in fact the operation he/she considers safe, might not be. Consider this example:

d = {1: 2, 3: 4, 5: 6}  # type: Dict[int, int]
if len(d) < d.get(1) + 2:
    print('len(d)')

if len(d) < d.get(2, None) + 2:
    print('len(d)')

if len(d) < d.get(3, 'str') + 2:
    print('len(d)')

Before #822 landed, Mypy (with --python-version 3.6 --strict-optional --fast-parser) showed:

.local/unsafeget.py:11: error: Argument 2 to "get" of "dict" has incompatible type "str"; expected "Optional[int]"

This is incorrect, get() actually accepts anything. After #822 landed, Mypy correctly points out all problems in all three cases:

.local/unsafeget.py:5: error: Unsupported operand types for + ("Optional[int]" and "int")
.local/unsafeget.py:8: error: Unsupported operand types for + ("Optional[int]" and "int")
.local/unsafeget.py:11: error: Unsupported operand types for + ("Union[int, str]" and "int")

So, my conclusion is that in this case we really should keep the Union as a return type. Same with dict.pop() (see reverted #278) and next() (see open #827).

[JukkaL]

The three-overload version is problematic as well, since mypy doesn't like when multiple overload variants match (that is another mypy issue...). The union return type seems fine to me -- I'd like see examples where it causes problems with Dropbox code. Maybe it's just something we need to fix in mypy, or a problem in the code.

I'd argue that mypy treats union types correctly. For example, Optional[str] wouldn't really be very useful if mypy would be happy that an operation works with some union item -- it's essential that mypy requires that an operation works with both str and None, so that things like x + 'a' are flagged as errors if x has type Optional[str].

The typical problems with union return types should not apply to get, since it can return two possible values, and the type is not predictable in general. Consider this code:

x = ... # type: Dict[str, int]
y = x.get('a', [1])

Now Union[int, List[int]] is a reasonable type for y, and code dealing with y must be able to deal with both int and List[int] values.

[ambv]

I'd argue that mypy treats union types correctly.

Agreed, this only needs documentation, as per python/mypy#1693!

The one possible improvement to mypy itself that I recall is making mypy not report errors when the user is performing an operation that happens to be valid for all elements in the union. I can't find the specific PR/issue where this was raised though, so correct me if I'm wrong here.

[gvanrossum]

The one possible improvement to mypy itself that I recall is making mypy not report errors when the user is performing an operation that happens to be valid for all elements in the union.

Usually this works, e.g. this is fine:

class A:
    def f(self): pass
class B:
    def f(self): pass
def f(x: Union[A, B]):
    x.f()

but there are many places in mypy where Union has to be special-cased, and I believe some of these have not yet been discovered. We've definitely fixed a smattering of these over the past year.

[JukkaL]

There are also several already known bugs related to union types, such as #2128, #1855 and #1533. Union types were originally implemented in a few days as part of a Dropbox hack week project, and that is still reflected in the incompleteness of the implementation :-(