Skip to content

Add support for additional TypedDict methods #6011

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

Merged
merged 25 commits into from
Dec 6, 2018
Merged

Add support for additional TypedDict methods #6011

merged 25 commits into from
Dec 6, 2018

Conversation

JukkaL
Copy link
Collaborator

@JukkaL JukkaL commented Dec 5, 2018

This adds support for these methods through additional plugin hooks:

  • pop
  • setdefault
  • update (positional argument only)
  • __delitem__

These methods also work and don't need plugin support:

  • copy
  • has_key (Python 2 only)
  • viewitems (Python 2 only)
  • viewkeys (Python 2 only)
  • viewvalues (Python 2 only)

The base signatures for all of these methods are defined in
mypy_extensions._TypedDict, which is a stub-only class
only used internally by mypy. It becomes the new fallback
type of all TypedDicts.

Fixes #3843. Fixes #3550.

There's some possible follow-up work that I'm leaving to other PRs,
such as optimizing hook lookup through dictionaries in the default
plugin and documenting the supported methods.

@JukkaL JukkaL requested a review from ilevkivskyi December 5, 2018 13:45
JukkaL added a commit that referenced this pull request Dec 5, 2018
Follow-up to #6011 (this must be merged after it).
Copy link
Member

@ilevkivskyi ilevkivskyi left a comment

Choose a reason for hiding this comment

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

Looks great! I have a bunch of small comments.

if (isinstance(ctx.type, TypedDictType)
and len(ctx.arg_types) >= 1
and len(ctx.arg_types[0]) == 1):
if isinstance(ctx.args[0][0], StrExpr):
Copy link
Member

Choose a reason for hiding this comment

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

At some point we should also start using "constant folding", not in this PR however.

arg_type = arg_type.as_anonymous()
arg_type = arg_type.copy_modified(required_keys=set())
return signature.copy_modified(arg_types=[arg_type])
return signature
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a TODO somewhere for .update(x=1, y=2)? Or is this tracked in an issue?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fallback = (self.api.named_type_or_none('typing.Mapping',
[self.api.named_type('__builtins__.str'),
self.api.named_type('__builtins__.object')])
fallback = (self.api.named_type_or_none('mypy_extensions._TypedDict', [])
Copy link
Member

Choose a reason for hiding this comment

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

I would fail soon here if we can't find mypy_extensions._TypedDict. There is some logic than may cause a crash/fail that will be harder to debug.

mypy/types.py Outdated
at runtime.

If a TypedDict is named, its fallback will be an Instance of the named type
(ex: "Point") whose TypeInfo has a typeddict_type that is anonymous.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe mention this is similar to how named tuples work and/or add some motivation for such complex structure?

runtests.py Outdated
@@ -1,6 +1,6 @@
#!/usr/bin/env python3
from os import system
from sys import argv, exit, platform, executable, version_info
from sys import argv, exit, platform, executable, version_info, stdout
Copy link
Member

Choose a reason for hiding this comment

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

A redundant import?


reveal_type(a.copy()) # E: Revealed type is 'TypedDict('__main__.A', {'x': builtins.int, 'y': builtins.list[builtins.int]})'
a.has_key() # E: "A" has no attribute "has_key"
a.clear() # E: "A" has no attribute "clear"
Copy link
Member

Choose a reason for hiding this comment

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

Another possible TODO is to give a better error message here.

A = TypedDict('A', {'x': int})
a = A(x=1)
reveal_type(a.copy()) # E: Revealed type is 'TypedDict('__main__.A', {'x': builtins.int})'
reveal_type(a.has_key()) # E: Revealed type is 'builtins.bool'
Copy link
Member

Choose a reason for hiding this comment

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

Argument is missing here.

Also we can probably infer Literal[True/False] here and for 's' in SomeTypedDict @Michael0x2a you might want to add this to your "wishlist" for literal types.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This wouldn't be safe due to structural subtyping and casts. Users may use a total TypedDict type when some keys are actually missing, and we don't want to break such code even though it's questionable.


reveal_type(a.pop('x')) # E: Revealed type is 'builtins.int'
reveal_type(a.pop('y', [])) # E: Revealed type is 'builtins.list[builtins.int]'
reveal_type(a.pop('x', '')) # E: Revealed type is 'Union[builtins.int, builtins.str]'
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a line here for non-Instance default value (maybe TupleType) just in case?

@JukkaL JukkaL merged commit d34d285 into master Dec 6, 2018
JukkaL added a commit that referenced this pull request Dec 6, 2018
@gvanrossum gvanrossum deleted the tdict-methods branch December 7, 2018 18:07
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.

2 participants