Skip to content

Set TypedDicts to always be their declared type as opposed to the type inferred when instantiated #2621

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
wants to merge 2 commits into from

Conversation

rowillia
Copy link
Contributor

@rowillia rowillia commented Dec 30, 2016

TypedDicts appear to have explicitly decided not to accept subtypes on fields,
but this behavior is counter intuitive. This made it so TypedDicts didn't
respect Any and caused problems with what should have been ducktype
compatible. This also brings TypedDicts more in line with other container
types and with how fields on classes behave. The following code is already OK
with mypy:

from typing import Dict

def foo() -> Dict[float, object]:
    x: object = 50.5
    return {
        1: 32
    }

This fixes #2610

@gvanrossum
Copy link
Member

@davidfstr What do you think of this? It looks reasonable to me -- if you agree I'll merge it.

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 3, 2017

Making the typed dict field types covariant isn't safe, since the fields are mutable. Dictionaries are invariant as well. The dictionary example works because mypy uses "type context" to infer the key/value types for a dict literal. The modified example below won't work, since now there's no type context when we define d:

from typing import Dict

def foo() -> Dict[float, object]:
    d = {   # inferred type is Dict[int, int]
        1: 32
    }
    return d  # error

I think that the correct fix would be for the call to a typed dict type to result in the exact declared typed dict type. Example:

from mypy_extensions import TypedDict

A = TypedDict('A', {'x': object})

def f() -> A:
    a = A(x=int)  # type of a should be TypedDict(x=object)
    return a

@davidfstr
Copy link
Contributor

Making the typed dict field types covariant isn't safe, since the fields are mutable.

+1. Yes this is unfortunate. I originally had the fields support covariance until I noticed they were mutable.

I think that the correct fix would be for the call to a typed dict type to result in the exact declared typed dict type.

Yes this would make sense to me. I'm actually a bit surprised this isn't the current behavior.

@rowillia
Copy link
Contributor Author

rowillia commented Jan 4, 2017

@JukkaL @davidfstr makes sense. So it sounds like a proper fix would be able to pass with all of the test cases I added and the existing test case that enforces invariance. Assuming we fix #2487, how would this interact with dictionary literals?

@davidfstr
Copy link
Contributor

I imagine the following two code fragments would create values of equivalent types, i.e. Point:

d1 = Point({'x': 1, 'y': 2})
d2 = {'x': 1, 'y': 2}  # type: Point

I believe there is still a problem where an unknown type (i.e. Any) is not treated as "equivalent" to a required item type. So I suspect the following would still fail:

def make_point(x, y):
    return Point({'x': x, 'y': y})  # probably fails because x and y are of Any type

@JukkaL
Copy link
Collaborator

JukkaL commented Jan 5, 2017

Any is equivalent with all other types, so it shouldn't be a problem.

#2487 would need to consider the type context when inferring the type of a literal. For example, if the context has a type like {'x': List[int]}, the type of [] in {'x': []} should be List[int] based on the type context.

Roy Williams added 2 commits January 6, 2017 14:06
TypedDicts appear to have explicitly decided not to accept subtypes on fields,
but this behavior is counter intuitive.  This made it so TypedDicts didn't
respect `Any` and caused problems with what should have been ducktype
compatible.  This also brings TypedDicts more in line with other container
types and with how fields on classes behave.

```python
from typing import Dict

def foo() -> Dict[float, object]:
    return {
        1: 32
    }
```

This fixes python#2610
@rowillia rowillia force-pushed the fix_typed_dict_duck_typing branch from abb3de9 to 0ca0ae6 Compare January 6, 2017 23:29
@rowillia rowillia changed the title Allow fields on a TypedDict to be subtypes of their declared types. Set TypedDicts to always be their declared type as opposed to the type inferred when instantiated Jan 6, 2017
@rowillia
Copy link
Contributor Author

rowillia commented Jan 7, 2017

@davidfstr I took @JukkaL 's advice and made the type of the TypedDict always the declared type. This matches what I would expected anyway.

This did break a few existing test cases, I'd love your input on the changes.

@davidfstr
Copy link
Contributor

@rowillia these changes look good to me, including the test changes.

Not related to your commit, but I'm surprised the revealed types of the named types (like Point) don't actually give the named type (i.e. Point) but rather the anonymous type (TypedDict(x=int, y=int)). It's possible the original tests and the output for reveal_type() predate the maintenance of the named type information. Something to look at in a different PR...

@rowillia
Copy link
Contributor Author

rowillia commented Jan 9, 2017

@davidfstr yeah I'd love to see that fixed.

rowillia pushed a commit to rowillia/mypy that referenced this pull request Jan 11, 2017
rowillia pushed a commit to lyft/mypy that referenced this pull request Jan 12, 2017
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good now! Just a few test case related requests.

I'd like to see a type context related test case, where a value type is, say, List[int], and it's given an empty list [] as an argument, e.g. MyTypedDict(items=[]). Use reveal_type on the resulting typed dict.

Another thing below.

from typing import Any, Mapping
Point = TypedDict('Point', {'x': float, 'y': float})
def create_point() -> Point:
return Point(x=1, y=2)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you also do something like reveal_type(Point(x=1, y=2)) in a test case?

@JukkaL
Copy link
Collaborator

JukkaL commented Mar 24, 2017

@rowillia Are you interested in finishing this up? If you are currently busy, I can do the remaining bit of work and get this merged.

JukkaL added a commit that referenced this pull request Mar 31, 2017
…e inferred when instantiated (2) (#3099)

[This was implemented originally by @rowillia in #2621. This PR includes
a few test updates.]

* Allow fields on a TypedDict to be subtypes of their declared types.

TypedDicts appear to have explicitly decided not to accept subtypes on fields,
but this behavior is counter intuitive.  This made it so TypedDicts didn't
respect `Any` and caused problems with what should have been ducktype
compatible.  This also brings TypedDicts more in line with other container
types and with how fields on classes behave.

```python
from typing import Dict

def foo() -> Dict[float, object]:
    return {
        1: 32
    }
```

This fixes #2610

* Take suggestion from Jukka to have calls to typed dicts always result in exact declared type

See #2621 (comment)
@JukkaL
Copy link
Collaborator

JukkaL commented Mar 31, 2017

@rowillia I merged this as #3099 with a few test updates. Thanks for working on this!

@JukkaL JukkaL closed this Mar 31, 2017
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

Successfully merging this pull request may close these issues.

TypedDict doesn't respect subtypes or promoted types.
4 participants