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

Type check typed dict as caller **kwargs #5925

Merged
merged 17 commits into from
Nov 30, 2018
132 changes: 97 additions & 35 deletions mypy/argmap.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
"""Utilities for mapping between actual and formal arguments (and their types)."""

from typing import List, Optional, Sequence, Callable
from typing import List, Optional, Sequence, Callable, Set

from mypy.types import Type, Instance, TupleType, AnyType, TypeOfAny
from mypy.types import Type, Instance, TupleType, AnyType, TypeOfAny, TypedDictType
from mypy import nodes


Expand Down Expand Up @@ -65,13 +65,23 @@ def map_actuals_to_formals(caller_kinds: List[int],
map[callee_kinds.index(nodes.ARG_STAR2)].append(i)
else:
assert kind == nodes.ARG_STAR2
for j in range(ncallee):
# TODO tuple varargs complicate this
no_certain_match = (
not map[j] or caller_kinds[map[j][0]] == nodes.ARG_STAR)
if ((callee_names[j] and no_certain_match)
or callee_kinds[j] == nodes.ARG_STAR2):
map[j].append(i)
argt = caller_arg_type(i)
if isinstance(argt, TypedDictType):
for name, value in argt.items.items():
if name in callee_names:
map[callee_names.index(name)].append(i)
elif nodes.ARG_STAR2 in callee_kinds:
map[callee_kinds.index(nodes.ARG_STAR2)].append(i)
else:
# We don't exactly which **kwargs are provided by the
Copy link
Member

Choose a reason for hiding this comment

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

"know" missing in comment.

# caller. Assume that they will fill the remaining arguments.
for j in range(ncallee):
# TODO tuple varargs complicate this
Copy link
Member

Choose a reason for hiding this comment

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

I this comment still relevant?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think that it means that we don't which args a variable-length tuple *args will match, so we make assumptions. I will update the comment -- this is still an issue.

no_certain_match = (
not map[j] or caller_kinds[map[j][0]] == nodes.ARG_STAR)
if ((callee_names[j] and no_certain_match)
or callee_kinds[j] == nodes.ARG_STAR2):
map[j].append(i)
return map


Expand All @@ -95,35 +105,87 @@ def map_formals_to_actuals(caller_kinds: List[int],
return actual_to_formal


def get_actual_type(arg_type: Type, kind: int,
tuple_counter: List[int]) -> Type:
"""Return the type of an actual argument with the given kind.
class ArgTypeExpander:
"""Utility class for mapping actual argument types to formal arguments.

One of the main responsibilities is to expand caller tuple *args and TypedDict
**kwargs, and to keep track of which tuple/TypedDict items have already been
consumed.

Example:

. def f(x: int, *args: str) -> None: ...
. f(*(1, 'x', 1.1)
Copy link
Member

Choose a reason for hiding this comment

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

I am just curious why these dots are here? (Also missing closing parenthesis)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The dots were an attempt to avoid breaking auto indent in emacs, but apparently they are no longer required.


We'd call expand_actual_type three times:

If the argument is a *arg, return the individual argument item.
1. The first call would provide 'int' as the actual type of 'x' (from '1').
2. The second call would provide 'str' as one of the actual types for '*args'.
2. The third call would provide 'float' as one of the actual types for '*args'.

Construct a separate instance for each call since instances have per-call
state.
Copy link
Member

Choose a reason for hiding this comment

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

Explain that it means per function f call, not per one of the three calls above (IIUC).

"""

if kind == nodes.ARG_STAR:
if isinstance(arg_type, Instance):
if arg_type.type.fullname() == 'builtins.list':
# List *arg.
return arg_type.args[0]
elif arg_type.args:
# TODO try to map type arguments to Iterable
return arg_type.args[0]
def __init__(self) -> None:
# Next tuple *args index to use.
self.tuple_index = 0
# Keyword arguments in TypedDict **kwargs used.
self.kwargs_used = set() # type: Set[str]

def expand_actual_type(self,
actual_type: Type,
actual_kind: int,
formal_name: Optional[str],
formal_kind: int) -> Type:
"""Return the actual (caller) type(s) of a formal argument with the given kinds.

If the actual argument is a tuple *args, return the individual tuple item(s) that
map(s) to the formal arg.

If the actual argument is a TypedDict **kwargs, return the matching typed dict dict
Copy link
Member

Choose a reason for hiding this comment

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

Duplicate word "dict"?

value type(s) based on formal argument name and kind.

This is supposed to be called for each formal, in order. Call multiple times per
formal if multiple actuals map to a formal.
"""
if actual_kind == nodes.ARG_STAR:
if isinstance(actual_type, Instance):
if actual_type.type.fullname() == 'builtins.list':
# List *arg.
return actual_type.args[0]
elif actual_type.args:
Copy link
Member

Choose a reason for hiding this comment

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

This may be a bit confusing if a totally unrelated generic is provided here, I would either fix the TODO below, or just skip this branch as you do for **kwargs.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'd rather keep it as it is to stay closer to "one issue per PR". This PR already does too much for a single PR, arguably. Piggy packing additional fixes to a PR is a bad habit I'm trying to move away from.

Copy link
Member

@ilevkivskyi ilevkivskyi Nov 29, 2018

Choose a reason for hiding this comment

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

I thought this is new code, but now I see it was already here.

# TODO: Try to map type arguments to Iterable
return actual_type.args[0]
else:
return AnyType(TypeOfAny.from_error)
elif isinstance(actual_type, TupleType):
# Get the next tuple item of a tuple *arg.
if self.tuple_index >= len(actual_type.items):
# Exhausted a tuple -- continue to the next *args.
self.tuple_index = 1
else:
self.tuple_index += 1
return actual_type.items[self.tuple_index - 1]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am missing something, but it seems to me the logic is wrong here, if the tuple exhausted, you return the first element of the first tuple, not the second one. It is a bit hard to reason, could you please add a test for something like this:

def f(a: T1, b: T2, c: T3, d: T4) -> Tuple[T1, T2, T3, T4]: ...
x: Tuple[A, B]
y: Tuple[C, D]
reveal_type(f(*x, *y))

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The logic is correct, but added a test case. We do a -1 afterwards.

else:
return AnyType(TypeOfAny.from_error)
elif actual_kind == nodes.ARG_STAR2:
if isinstance(actual_type, TypedDictType):
if formal_kind != nodes.ARG_STAR2 and formal_name in actual_type.items:
# Lookup type based on keyword argument name.
assert formal_name is not None
Copy link
Member

Choose a reason for hiding this comment

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

What if formal_kind is ARG_STAR? Or this can't happen for some reasons?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The in check above guarantees that this is not None.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, I though that we narrow optional type to non-optional after an in check, if it doesn't work here (assert is still needed) it may be a bug.

else:
# Pick an arbitrary item if no specified keyword is expected.
formal_name = (set(actual_type.items.keys()) - self.kwargs_used).pop()
self.kwargs_used.add(formal_name)
return actual_type.items[formal_name]
elif (isinstance(actual_type, Instance)
and (actual_type.type.fullname() == 'builtins.dict')):
# Dict **arg.
# TODO: Handle arbitrary Mapping
return actual_type.args[1]
else:
return AnyType(TypeOfAny.from_error)
elif isinstance(arg_type, TupleType):
# Get the next tuple item of a tuple *arg.
tuple_counter[0] += 1
return arg_type.items[tuple_counter[0] - 1]
else:
return AnyType(TypeOfAny.from_error)
elif kind == nodes.ARG_STAR2:
if isinstance(arg_type, Instance) and (arg_type.type.fullname() == 'builtins.dict'):
# Dict **arg. TODO more general (Mapping)
return arg_type.args[1]
else:
return AnyType(TypeOfAny.from_error)
else:
# No translation for other kinds.
return arg_type
# No translation for other kinds -- 1:1 mapping.
return actual_type
Loading