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

Fix overlap between TypedDict and Mapping #7164

Merged
merged 6 commits into from Jul 8, 2019

Conversation

@ilevkivskyi
Copy link
Collaborator

commented Jul 6, 2019

Fixes #7036

This adds more "fine grained" logic for overlap between TypedDict and Mapping used by overload checks and --strict-equality. Basically the rules are:

  • A TypedDict with some required keys is overlapping with Mapping[str, <some type>] if and only if every key type is overlapping with <some type>.
  • A TypedDict with no required keys overlaps with Mapping[str, <some type>] if and only if at least one of key types overlaps with <some type>.
  • Empty dictionaries are as usual in gray area, so we follow the same logic as in other places: an empty dictionary can't cause an overlap (i.e. TypedDict with no required keys doesn't overlap with Mapping[str, <some type>]), but is itself (i.e. Mapping[<nothing>, <nothing>]) overlapping with a TypedDict with no required keys.

@ilevkivskyi ilevkivskyi requested a review from Michael0x2a Jul 6, 2019

@silviogutierrez

This comment has been minimized.

Copy link

commented Jul 6, 2019

I checked this out eagerly, as I thought it covers a big use case of @cs-cordero and mine's. But now that I read it closely, it seems related but not the actual solution. That is, this still won't work:

from typing import Mapping

from mypy_extensions import TypedDict


def mapping_test(thing: Mapping[str, str]) -> bool:
    return True


class Foo(TypedDict):
    foo: str


a = Foo(foo="a")
b: Foo = {'foo': 'a'}


mapping_test(a)  # Errors out still, with below.
mapping_test(b)  # Errors out still, with below.
# Argument 1 to "mapping_test" has incompatible type "Foo"; expected "Mapping[str, str]"

You mention "overlap" but in this case, it's referring strictly to equality and overloads, not general types matching. Is that right?

Is our use case a big stretch?

Either way, thanks for all the work on mypy!

@silviogutierrez

This comment has been minimized.

Copy link

commented Jul 6, 2019

Adding a more thorough snippet that demonstrates what works and what doesn't as of this PR, in case it's helpful. All running with --strict-equality

from typing import Mapping

from mypy_extensions import TypedDict


def mapping_test(thing: Mapping[str, str]) -> bool:
    return True


class Foo(TypedDict):
    foo: str


a = Foo(foo="a")
b: Foo = {"foo": "a"}
c: Mapping[str, str] = {"foo": "a"}


a == b == c  # Works


mapping_test(c)  # Works, of course
mapping_test(a)  # Does not work
mapping_test(b)  # Does not work
@Michael0x2a
Copy link
Collaborator

left a comment

I had one minor suggestion, but otherwise lgtm!

only if at least one of key types overlaps with <some type>. For example:
- TypedDict(x=str, y=str, total=False) overlaps with Dict[str, str]
- TypedDict(x=str, y=str, total=False) doesn't overlap with Dict[str, int]

This comment has been minimized.

Copy link
@Michael0x2a

Michael0x2a Jul 8, 2019

Collaborator

Maybe a third example could be - TypedDict(x=int, y=str, total=False) overlaps with Dict[str, str]`?

Ivan Levkivskyi
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 8, 2019

@silviogutierrez Your issue is completely unrelated. The code is your example is unsafe because typed dicts use structural subtyping, you can use Mapping[str, object] instead.

@ilevkivskyi ilevkivskyi merged commit 8782d63 into python:master Jul 8, 2019

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:typeddict-overlap branch Jul 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.