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

Conversation

Projects
None yet
2 participants
@JukkaL
Copy link
Collaborator

JukkaL commented Nov 20, 2018

Expand and type check TypedDict types when used as **kwargs in calls.

Also refactored the implementation of checking function arguments and
removed some apparently useless code.

Fixes #5198 and another related issue: type checking calls with multiple
*args arguments.

@JukkaL JukkaL requested a review from ilevkivskyi Nov 20, 2018

messages.invalid_keyword_var_arg(arg_type, is_mapping, context)
# Get the type of an individual actual argument (for *args
# and **args this is the item type, not the collection type).
if (isinstance(arg_type, TupleType)

This comment has been minimized.

@JukkaL

JukkaL Nov 20, 2018

Collaborator

This seemed unnecessary so I removed it.

# There may be some remaining tuple varargs items that haven't
# been checked yet. Handle them.
tuplet = arg_types[actual]
if (callee.arg_kinds[i] == nodes.ARG_STAR and

This comment has been minimized.

@JukkaL

JukkaL Nov 20, 2018

Collaborator

This seemed unnecessary as well so I got rid of it.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! The idea looks good. The logic here is a bit complex, so refactorings are also helpful. I have few comments below.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

"know" missing in comment.

# We don't exactly which **kwargs are provided by the
# caller. Assume that they will fill the remaining arguments.
for j in range(ncallee):
# TODO tuple varargs complicate this

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

I this comment still relevant?

This comment has been minimized.

@JukkaL

JukkaL Nov 29, 2018

Collaborator

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.

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

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

This comment has been minimized.

@JukkaL

JukkaL Nov 29, 2018

Collaborator

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

if actual_type.type.fullname() == 'builtins.list':
# List *arg.
return actual_type.args[0]
elif actual_type.args:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

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.

This comment has been minimized.

@JukkaL

JukkaL Nov 29, 2018

Collaborator

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.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 29, 2018

Collaborator

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

@@ -1062,54 +1063,27 @@ def check_argument_count(self, callee: CallableType, actual_types: List[Type],
Return False if there were any errors. Otherwise return True
"""
if messages:
assert context, "Internal error: messages gives without context"

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

typo "gives" -> "given"

g(**b) # E: Argument "x" to "g" has incompatible type "str"; expected "int"
g(1, **a) # E: "g" gets multiple values for keyword argument "x"
g(1, **b) # E: "g" gets multiple values for keyword argument "x" \
# E: Argument "x" to "g" has incompatible type "str"; expected "int"

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

Does this PR also fix #1969? If yes, then I would update the description and added a test with optional argument.

This comment has been minimized.

@JukkaL

JukkaL Nov 29, 2018

Collaborator

No

"""Check for extra actual arguments.
Return tuple (was everything ok,
was there an extra keyword argument error).

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

Maybe mention that the latter is used to avoid duplicate errors?

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

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

This comment has been minimized.

@JukkaL

JukkaL Nov 29, 2018

Collaborator

The in check above guarantees that this is not None.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 29, 2018

Collaborator

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.

self.tuple_index = 1
else:
self.tuple_index += 1
return actual_type.items[self.tuple_index - 1]

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

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))

This comment has been minimized.

@JukkaL

JukkaL Nov 29, 2018

Collaborator

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

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

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Nov 28, 2018

Collaborator

Duplicate word "dict"?

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! LG now.

@JukkaL JukkaL merged commit dd1c5d0 into master Nov 30, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@JelleZijlstra JelleZijlstra deleted the tdict-kwargs branch Nov 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment