Support TypedDicts with missing keys (total=False) #3558

JukkaL · 2017-06-16T13:10:11Z

Support both the functional syntax and the class based syntax.

This will also require a change to the mypy_extensions stub in typeshed.

Implements most of #2632, but binder support ('key' in typed_dict checks)
will need another PR.

Only the functional syntax is supported.

JukkaL · 2017-06-16T13:10:32Z

The Travis CI failure looks like a flake.

There is no support for introspection of `total` yet.

Make TypedDict `total` introspectable.

gvanrossum · 2017-06-20T17:38:07Z

I'm getting second thoughts about this feature. I think insisting on using get() everywhere will be pretty disruptive. I've found several use cases already (without really looking) where there's something like

args = {'p1': int(), 'p2': str()}
if bool():
    args['p3'] = int()

and then later args['p1'] and args['p2'] are used freely. While I don't want to have to implement the full "specify which keys are optional" functionality, I think the "total=False" flag would be more useful if it allowed args['p1'] (and even args['p3']) while preserving the other behavior of TypedDict (e.g. complaining if the key is not a constant or not one of the known keys, and inferring the right type for the values for known keys).

JukkaL · 2017-06-21T11:38:41Z

I agree that the current PR likely isn't flexible enough for a lot of user code. I like your idea, assuming I understood it correctly. The only change seems to be that type checking of td['key'] would remain unchanged for non-total typed dicts. This would imply that, unlike with total typed dicts, non-total typed dicts could generate KeyError exceptions. Otherwise they could be type safe, I think.

This change would also make supporting 'key' in td checks unnecessary, which would save some effort.

JukkaL · 2017-06-21T15:12:14Z

Now td['key'] is supported for non-total TypedDicts.

gvanrossum

For a later PR: I just came up with an idea for specifying whether individual fields are required or not -- use '-x' in the dict. (But this doesn't work for the class-based syntax.)

gvanrossum · 2017-06-21T18:14:51Z

mypy/types.py

            else:
-                return 'TypedDict({}, _fallback={})'.format(s, t.fallback.accept(self))
+                return 'TypedDict({}, _fallback={}{})'.format(s, t.fallback.accept(self), keys_str)
        return 'TypedDict({})'.format(s)


It's a bit disturbing that the repr of a TypedDict is so different from the actual syntax used to create one, but let's deal with that some other time.

Created #3590 to track this.

gvanrossum · 2017-06-21T18:29:51Z

test-data/unit/check-typeddict.test

+f({'x': 1})
+f({'y': ''})
+f({'x': 1, 'y': ''})
+f({'x': 1, 'z': ''}) # E: Expected TypedDict key 'x' but found keys ('x', 'z')


This seems inconsistent -- isn't {'x': 1, 'z': ''} an instance of a subtype of D? See testTypedDictSubtypingWithTotalFalse below.

I want to catch misspelled key names (or totally invalid items). If we didn't complain about z, there would be a lot of potential for false negatives. TypedDicts are treated kind of like structs by mypy so we don't allow extra keys during creation. Structural subtyping can't produce new keys, so it's less of a problem.

gvanrossum · 2017-06-21T20:09:15Z

test-data/unit/check-typeddict.test

+T = TypeVar('T')
+def f(x: Callable[[T, T], None]) -> T: pass
+def g(x: XY, y: YZ) -> None: pass
+reveal_type(f(g))  # E: Revealed type is 'TypedDict(x=builtins.int, y=builtins.int, z=builtins.int, _fallback=typing.Mapping[builtins.str, builtins.int], _required_keys=[y, z])'


The repr of a TypedDict is rather large, with the fallback and required_keys...

Let's continue this in #3590. I think that it's best to discuss all the aspects of the representation at the same time.

gvanrossum · 2017-06-21T20:17:44Z

test-data/unit/check-typeddict.test

+b: B
+c: C
+reveal_type(j(a, b)) \
+    # E: Revealed type is 'TypedDict(_fallback=typing.Mapping[builtins.str, <nothing>])'


It's a little subtle that this is how a TypedDict with no keys is rendered.

Another issue for #3590.

ilevkivskyi · 2017-06-21T20:54:12Z

@gvanrossum

For a later PR: I just came up with an idea for specifying whether individual fields are required or not -- use '-x' in the dict. (But this doesn't work for the class-based syntax.)

How about

class Maybe2DPoint(TypedDict):
    x: int
    (y): int

There will be some difficulties with runtime introspection, expressions that are not simple names are not stored, so that Maybe2DPoint.__annotations__ will be just {'x': int}. But the above example actually works and is supported by the typed_ast parser, i.e., it will detect and mark assignment nodes that have parentheses.

gvanrossum · 2017-06-21T20:58:15Z

Very thin ice there, and the runtime introspection issue kills it for me. :-(

ilevkivskyi · 2017-06-21T21:17:00Z

OK, another two crazy ideas that probably have not been discussed yet:

from mypy_extensions import TypedDict, optional

class Maybe2DPoint(TypedDict):
    x: int
    y: (int, optional)  # The syntax is invalid without parentheses here

class Maybe2DPoint(TypedDict):
    x: int
    y: int; optional

This is quite similar to Optional but still it could be different enough due to being lowercase and appearing after the type, so that it reads not like the type is optional but the whole key is optional.
(Both look a bit ugly however, and the introspection problem still appears with the second one.)

@JukkaL Sorry for off-topic.

gvanrossum

(Sorry, hit SEND prematurely. Here's the restthe review. Honest.)

gvanrossum · 2017-06-21T20:33:28Z

mypy/checkexpr.py

-            callee_item_names = callee.items.keys()
+        if not (callee.required_keys <= set(kwargs.keys()) <= set(callee.items.keys())):
+            callee_item_names = [key for key in callee.items.keys()
+                                 if key in callee.required_keys or key in kwargs.keys()]
            kwargs_item_names = kwargs.keys()



This blank line irks me.

gvanrossum · 2017-06-21T20:35:17Z

mypy/checkexpr.py

-        if callee.items.keys() != kwargs.keys():
-            callee_item_names = callee.items.keys()
+        if not (callee.required_keys <= set(kwargs.keys()) <= set(callee.items.keys())):
+            callee_item_names = [key for key in callee.items.keys()


Naming these two variables expected_xxx and actual_xxx (matching the error message call below) would help in understanding what they mean.

A good idea -- done.

gvanrossum · 2017-06-21T20:35:40Z

mypy/checkexpr.py

-                rvalue_name='expression')
+            if item_name in kwargs:
+                item_value = kwargs[item_name]
+


Again why a blank line?

gvanrossum · 2017-06-21T20:54:32Z

mypy/meet.py

@@ -266,7 +267,8 @@ def visit_typeddict_type(self, t: TypedDictType) -> Type:
            items = OrderedDict(item_list)
            mapping_value_type = join_type_list(list(items.values()))
            fallback = self.s.create_anonymous_fallback(value_type=mapping_value_type)
-            return TypedDictType(items, fallback)
+            required_keys = set(items.keys()) & (t.required_keys | self.s.required_keys)


Again, the '&' with items.keys() shouldn't ever do anything right? Assuming s.required_keys is a subset of s.items etc.

You are correct. Updated.

gvanrossum · 2017-06-21T21:04:44Z

mypy/join.py

            ])
            mapping_value_type = join_type_list(list(items.values()))
            fallback = self.s.create_anonymous_fallback(value_type=mapping_value_type)
-            return TypedDictType(items, fallback)
+            required_keys = set(items.keys()) & t.required_keys & self.s.required_keys


Why is the set(items.keys()) & part needed? Is there ever a TypedDict whose required_keys is not a subset of its items?

We filter out earlier keys that exist in both t and self.s but have incompatible value types, so this is necessary, I think.

gvanrossum · 2017-06-21T21:10:11Z

mypy/plugin.py

+            if (isinstance(value_type, TypedDictType)
+                    and isinstance(default_arg, DictExpr)
+                    and len(default_arg.items) == 0):
+                # Caller has empty dict {} as default for typed dict.


Why do we have to special-case this? Assuming the value type is some TypedDict, and the default passed to get() is some dict literal, isn't the natural union type resulting from the two an appropriate TypedDict? Even if it isn't, shouldn't we use the value type as a context for inferring the type of the dict literal? ISTM that d.get('x', {'y': 1}) ought to work too.

We special this so that the context will be a non-total TypedDict in case the default is {}. If we don't do that, {} won't be accepted for total TypedDicts since it has missing keys (all keys are missing).

Any subset of keys should work but this will be harder to implement and likely not very common, so I think it's okay to postpone it until later. I can create an issue to track that.

gvanrossum · 2017-06-21T21:34:29Z

mypy/subtypes.py

                if not is_equivalent(l, r, self.check_type_parameter):
                    return False
+                # Non-required key is not compatible with a required key since indexing


Hm, I wonder if there's something to say for making at least one of the directions work here??? It's hard to keep things straight (see my notes on some of the tests).

Update the comment to be more specific. Here's my argument why required key shouldn't be compatible with a non-required key (assuming we had #3550 implemented):

A = TypedDict('A', {'x': int}) B = TypedDict('B', {'x': int}, total=False) def f(b: B) -> None: del b['x'] # Should be accepted (but is not until we implement #3550) a: A = {'x': 0} f(a) # Error: if we allow this, the next line can fail a['x'] # Should not fail with KeyError

gvanrossum · 2017-06-21T22:00:58Z

mypy/subtypes.py

                if not is_equivalent(l, r, self.check_type_parameter):
                    return False
+                # Non-required key is not compatible with a required key since indexing
+                # may fail.  Required key is not compatible with a non-required key
+                # since the prior doesn't support 'del' but the latter supports it.


When I try to delete an item I always get "A" has no attribute "__delitem__", e.g.

A = TypedDict('A', {'x': int, 'y': str}) a: A = {'x': 0, 'y': ''} del a['x']

That's not implemented yet (#3550). Updated comment to reflect that.

gvanrossum

LGTM!

See python/mypy#3558 for context.

JukkaL added 8 commits June 15, 2017 16:08

Basic support for TypedDicts with missing keys (total=False)

ccfc4ad

Only the functional syntax is supported.

Support get(key, {}) and fix construction of partial typed dict

44f53a9

Fix subtyping of non-total typed dicts

6bb872e

Fix join with non-total typed dict

4df39fc

Fix meet with non-total typed dicts

39f0d38

Add serialization test case

7e042b8

Support TypedDict total keyword argument with class syntax

981023a

Attempt to fix Python 3.3

6223d2a

JukkaL added 4 commits June 16, 2017 18:37

Add minimal runtime total support to mypy_extensions

5ecafb2

There is no support for introspection of `total` yet.

Merge branch 'master' into typeddict-total

0d16271

Fix tests on pre-3.6 Python and improve introspection

1c068e0

Make TypedDict `total` introspectable.

Fix lint

4429ce1

ilevkivskyi mentioned this pull request Jun 21, 2017

Add typing_extensions module for new / optional typing features to PyPI python/typing#435

Closed

JukkaL added 5 commits June 21, 2017 15:35

Merge branch 'master' into typeddict-total

1c2d327

Fix problems caused by merge

5359a98

Allow td['key'] even if td is not total

a09c5b9

Fix lint

059bc21

Add test case

b47857e

gvanrossum reviewed Jun 21, 2017

View reviewed changes

gvanrossum mentioned this pull request Jun 21, 2017

Document TypedDict #3583

Merged

Merge branch 'master' into typeddict-total

6b922b5

JukkaL added 2 commits June 22, 2017 13:05

Address review feedback

31b6696

Update comment

d32fd68

gvanrossum approved these changes Jun 23, 2017

View reviewed changes

ilevkivskyi merged commit f0e8288 into master Jun 23, 2017

ilevkivskyi deleted the typeddict-total branch June 23, 2017 06:41

JukkaL added a commit to python/typeshed that referenced this pull request Jun 29, 2017

Add TypedDict total argument

24deecc

See python/mypy#3558 for context.

JukkaL added a commit to JukkaL/typeshed that referenced this pull request Jun 29, 2017

Add TypedDict total argument

bb815db

See python/mypy#3558 for context.

JukkaL mentioned this pull request Jun 29, 2017

Add TypedDict total argument python/typeshed#1443

Merged

JelleZijlstra pushed a commit to python/typeshed that referenced this pull request Jun 29, 2017

Add TypedDict total argument (#1443)

9b612c9

See python/mypy#3558 for context.

JukkaL mentioned this pull request Sep 11, 2017

Typed dicts with missing keys #2632

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support TypedDicts with missing keys (total=False) #3558

Support TypedDicts with missing keys (total=False) #3558

JukkaL commented Jun 16, 2017

JukkaL commented Jun 16, 2017

gvanrossum commented Jun 20, 2017

JukkaL commented Jun 21, 2017

JukkaL commented Jun 21, 2017

gvanrossum left a comment

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

ilevkivskyi commented Jun 21, 2017

gvanrossum commented Jun 21, 2017 via email

ilevkivskyi commented Jun 21, 2017 •

edited

Loading

gvanrossum left a comment

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum Jun 21, 2017

JukkaL Jun 22, 2017

gvanrossum left a comment

Support TypedDicts with missing keys (total=False) #3558

Support TypedDicts with missing keys (total=False) #3558

Conversation

JukkaL commented Jun 16, 2017

JukkaL commented Jun 16, 2017

gvanrossum commented Jun 20, 2017

JukkaL commented Jun 21, 2017

JukkaL commented Jun 21, 2017

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Jun 21, 2017

gvanrossum commented Jun 21, 2017 via email

ilevkivskyi commented Jun 21, 2017 • edited Loading

gvanrossum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum left a comment

Choose a reason for hiding this comment

ilevkivskyi commented Jun 21, 2017 •

edited

Loading