Implement type-aware `get` for TypedDict #2620

rowillia · 2016-12-28T20:41:01Z

Previously, get would simply fallback to the type of the underlying dictionary which made
TypedDicts hard to use with code that's parsing objects where fields may or may not be
present (for example, parsing a response).

This implementation explicitly ignores the default parameter's type as it's quite useful to
chain together get calls (Until something like PEP 505 hits 😄)

foo.get('a', {}).get('b', {}).get('c')

This fixes #2612

JukkaL · 2017-01-03T21:05:17Z

Thanks! This is probably pretty important for a lot of typed dict use cases.

Here are some notes after a quick pass:

The result type with a single argument should likely be Optional[...] if using strict optional checking.
It seems questionable to just ignore the default parameter. Do we still type check it? We should do that at the least so that x.get('a', 1 + 'y') is rejected, for example. Also, if the item type is not a typed dict, we shouldn't need to ignore the type of the second argument?

More generally, we don't have a complete story for incomplete typed dicts (i.e. ones with missing keys). This adds support for them partially, but not when explicitly creating a typed dict. You don't need to fully solve that in this PR, though.

gvanrossum · 2017-01-03T21:42:30Z

I'm a little bit surprised by the use case. Consider this test:

TaggedPoint = TypedDict('TaggedPoint', {'type': str, 'x': int, 'y': int})
PointSet = TypedDict('PointSet', {'first_point': TaggedPoint})
p = PointSet(first_point=TaggedPoint(type='2d', x=42, y=1337))
reveal_type(p.get('first_point', {}).get('x'))  # E: Revealed type is 'builtins.int'

In what real-world variation of this code does the default value (whether explicit or implicit) to either get() call get used? If we take the definition of TypedDict literally, neither get() call should ever return None because all the keys of a TypedDict are mandatory. And in fact get() with a literal key is supposed to be not useful for TypedDict instances at all since the keys are all mandatory.

[Note that my comment and Jukka's crossed each other.]

JukkaL · 2017-01-04T12:35:34Z

Yeah, get() with a literal key is only really useful if some keys can be missing. It's currently possible to have this if the typed dict is read from a file/socket, for example, but then the annotated type doesn't match the runtime type, which can be pretty confusing.

Let's move discussion about missing keys to #2632, since it's more complex than just implementing get().

gvanrossum · 2017-01-04T16:08:38Z

@JukkaL, what do you want to happen to this PR?

JukkaL · 2017-01-04T16:48:11Z

It's still unclear to me. It would be nice to get some idea of what the complete typed dict approach might look like before deciding what still needs to be done here (see my comments in #2632).

rowillia · 2017-01-04T17:59:44Z

I'm on vacation currently, but will be back in on Friday to make any changes. @JukkaL, you're totally correct RE: Optional types. I'll make that change.

For data validation, we use Marshmallow - https://marshmallow.readthedocs.io/en/latest/ . My plan was to build on top of it to generate stub files containing TypedDicts.

TypedDicts have also been super useful for us when dealing with some external API like Github's where honestly it's not super well documented what's required and what isn't, and they occasionally broke us by say omitting a field instead of returning an empty object. We generally are defensive and use get everywhere now and, for better or worse, in places where we haven't adopted Marshmallow yet we pass around the JSON object itself everywhere.

rowillia · 2017-01-07T00:45:21Z

@JukkaL Took your feedback and now return an Optional if the second parameter is empty. Also added a test case to ensure the default parameter is still type checked.

@gvanrossum Fixed up the handling of the default parameter. In most cases we construct a simplified union, but I did special case .get('key', {}) to not require declaring the type of that empty dictionary.

Previously, `get` would simply fallback to the type of the underlying dictionary which made TypedDicts hard to use with code that's parsing objects where fields may or may not be present (for example, parsing a response). This implementation _explicitly_ ignores the default parameter's type as it's quite useful to chain together get calls (Until something like PEP 505 hits 😄) ```python foo.get('a', {}).get('b', {}).get('c') ``` This fixes python#2612

… parameter

…ions. After poking around with this a bunch today I realized it would be much simplier to simply create a context-specific Callable as opposed to attemping to hijack the rest of the typechecking. The original implementation had problems in places, for example where a TypedDict had a List field. A default empty list was not being coerced correctly.

JukkaL

This looks almost ready! One non-trivial issue (Mapping type arguments) and a few minor things.

JukkaL · 2017-01-16T14:51:44Z

test-data/unit/check-typeddict.test

+TaggedPoint = TypedDict('TaggedPoint', {'type': str, 'x': int, 'y': int})
+PointSet = TypedDict('PointSet', {'first_point': TaggedPoint})
+p = PointSet(first_point=TaggedPoint(type='2d', x=42, y=1337))
+p.get('first_point', 32)  # E: Argument 2 to "get" of "Mapping" has incompatible type "int"; expected "Union[TypedDict(type=str, x=int, y=int), Mapping]"


This is a pretty minor thing, but the error message is a little confusing: it says "get" of "Mapping" even though the signature of get in this particular case comes from PointSet. A better message would be ... "get" of "PointSet" ... or `... "get" of a TypedDict ..." (if the name of the typed dict is not available).

(If this seems hard to do, we can create as a separate issue for this.)

JukkaL · 2017-01-16T14:58:19Z

test-data/unit/check-typeddict.test

+from mypy_extensions import TypedDict
+Items = TypedDict('Items', {'name': str, 'values': List[str]})
+def foo(i: Items) -> None:
+  reveal_type(i.get('values', []))  # E: Revealed type is 'builtins.list[builtins.str]'


Style nit: using 2 spaces for indent here.

JukkaL · 2017-01-16T14:59:32Z

test-data/unit/check-typeddict.test

+p = Point(x=42, y=13)
+def invoke_method(method: Callable[[str, int], int]) -> None:
+    pass
+invoke_method(p.get)


Maybe add a test case where p.get does not have a compatible type (e.g. target function expects Callable[[int, str], int] or something).

JukkaL · 2017-01-16T15:01:27Z

mypy/checkexpr.py

+                        # concise way without having to set up exception handlers.
+                        arg_types = [callee.arg_types[0],
+                                     UnionType.make_union([return_type,
+                                                           self.named_type('typing.Mapping')])]


It would better to use the type such as Mapping[str, Any]. Not having type arguments for Mapping may cause trouble in some cases (e.g. index errors). This still wouldn't be quite safe, but let's deal with that as a separate issue.

JukkaL · 2017-03-24T11:11:50Z

@rowillia Are you still interested in working on this? If you are busy, I can make the final changes and get this merged, since this would be a useful thing to have before the next mypy release.

gvanrossum · 2017-04-12T17:07:17Z

@rowillia I'm still hoping that you'll pick up this ball.

gvanrossum · 2017-04-30T17:55:39Z

@JukkaL I wonder if we should find a new owner for this PR? @rowillia seems to have stopped responding.

ilevkivskyi · 2017-05-31T19:57:08Z

This is a PR for a high-priority issue, I am thinking maybe it's time to change the owner?

JukkaL · 2017-05-31T19:59:40Z

I can move this forward. I'm going to use a somewhat different approach (a method 'plugin') to implement this.

@rowillia

Some of the tests are adapted from #2620 by @rowillia.

@rowillia

…#3501) Implement a general-purpose way of extending type inference of methods. Also special case TypedDict get and `int.__pow__`. Implement a new plugin system that can handle both module-level functions and methods. This an alternative to #2620 by @rowillia. I borrowed some test cases from that PR. This PR has a few major differences: * Use the plugin system instead of full special casing. * Don't support `d.get('x', {})` as it's not type safe. Once we have #2632 we can add support for this idiom safely. * Code like `f = foo.get` loses the special casing for get. Fixes #2612. Work towards #1240.

ilevkivskyi · 2017-06-07T21:38:36Z

I think this now can be closed in favor of recently merged #3501

rowillia force-pushed the implement_get_method_on_typed_dicts branch from 0de22fc to 586c1fd Compare December 28, 2016 20:56

rowillia force-pushed the implement_get_method_on_typed_dicts branch 4 times, most recently from f8c9dff to 24d2ae6 Compare January 7, 2017 00:06

Roy Williams added 5 commits January 11, 2017 13:13

Add test to ensure get method is still treated as a callable

9186c52

Create an Optional for calls to get with only one parameter

79a5114

Still typecheck default parameter, add test case with invalid default…

9e94cff

… parameter

Add testcase for get with list default parameter

c5f7481

rowillia force-pushed the implement_get_method_on_typed_dicts branch from 24d2ae6 to c5f7481 Compare January 11, 2017 21:13

rowillia force-pushed the implement_get_method_on_typed_dicts branch from 8c5975e to b1022bf Compare January 12, 2017 17:32

JukkaL requested changes Jan 16, 2017

View reviewed changes

gvanrossum assigned JukkaL May 31, 2017

JukkaL added a commit that referenced this pull request Jun 6, 2017

TypedDict get tweaks

2cf5cd7

Some of the tests are adapted from #2620 by @rowillia.

JukkaL mentioned this pull request Jun 6, 2017

Refactor plugin system and special case TypedDict get and int.__pow__ #3501

Merged

ilevkivskyi closed this Jun 7, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement type-aware `get` for TypedDict #2620

Implement type-aware `get` for TypedDict #2620

rowillia commented Dec 28, 2016 •

edited

Loading

JukkaL commented Jan 3, 2017

gvanrossum commented Jan 3, 2017 •

edited

Loading

JukkaL commented Jan 4, 2017

gvanrossum commented Jan 4, 2017

JukkaL commented Jan 4, 2017

rowillia commented Jan 4, 2017

rowillia commented Jan 7, 2017 •

edited

Loading

JukkaL left a comment

JukkaL Jan 16, 2017

JukkaL Jan 16, 2017

JukkaL Jan 16, 2017

JukkaL Jan 16, 2017

JukkaL commented Mar 24, 2017

gvanrossum commented Apr 12, 2017

gvanrossum commented Apr 30, 2017

ilevkivskyi commented May 31, 2017

JukkaL commented May 31, 2017

ilevkivskyi commented Jun 7, 2017

Implement type-aware get for TypedDict #2620

Implement type-aware get for TypedDict #2620

Conversation

rowillia commented Dec 28, 2016 • edited Loading

JukkaL commented Jan 3, 2017

gvanrossum commented Jan 3, 2017 • edited Loading

JukkaL commented Jan 4, 2017

gvanrossum commented Jan 4, 2017

JukkaL commented Jan 4, 2017

rowillia commented Jan 4, 2017

rowillia commented Jan 7, 2017 • edited Loading

JukkaL left a comment

Choose a reason for hiding this comment

JukkaL Jan 16, 2017

Choose a reason for hiding this comment

JukkaL Jan 16, 2017

Choose a reason for hiding this comment

JukkaL Jan 16, 2017

Choose a reason for hiding this comment

JukkaL Jan 16, 2017

Choose a reason for hiding this comment

JukkaL commented Mar 24, 2017

gvanrossum commented Apr 12, 2017

gvanrossum commented Apr 30, 2017

ilevkivskyi commented May 31, 2017

JukkaL commented May 31, 2017

ilevkivskyi commented Jun 7, 2017

Implement type-aware `get` for TypedDict #2620

Implement type-aware `get` for TypedDict #2620

rowillia commented Dec 28, 2016 •

edited

Loading

gvanrossum commented Jan 3, 2017 •

edited

Loading

rowillia commented Jan 7, 2017 •

edited

Loading