KEMBAR78
Add arguments to callee context in a call expression by elazarg · Pull Request #3116 · python/mypy · GitHub
Skip to content

Conversation

elazarg
Copy link
Contributor

@elazarg elazarg commented Apr 3, 2017

Fix #3097.

context_type = CallableType([self.accept(a) for a in e.args], e.arg_kinds, e.arg_names,
ret_type=self.type_context[-1] or AnyType(),
fallback=self.named_type('builtins.function'))
callee_type = self.accept(e.callee, context_type)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe it is too big a hammer to use on every call? should I add if isinstance(e.callee, LambdaExpr):?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably best to only do this for lambda expressions, as it could interact badly with generic functions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In such a case we may want to have a test that catches such an interaction (as if this PR is merged and someone opened an issue). But I am not sure how would such an interaction look like.

(lambda *, x, y: x + y)(x=1, y="") # E: Unsupported operand types for + ("int" and "str")
[out]
main:1: error: Incompatible types in assignment (expression has type "int", variable has type "str")
main:1: error: Incompatible return value type (got "int", expected "str")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having two error messages is slightly awkward

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know why we are getting two error messages here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we check the lambda against the full context, including the return type, and it fails to check. Then we check the call expression against the variable, which again fails. Maybe it is possible to check the lambda with return type defined to be object

context_type = CallableType([self.accept(a) for a in e.args], e.arg_kinds, e.arg_names,
ret_type=self.type_context[-1] or AnyType(),
fallback=self.named_type('builtins.function'))
callee_type = self.accept(e.callee, context_type)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably best to only do this for lambda expressions, as it could interact badly with generic functions.

g(['a']) # E: Argument 1 to "g" has incompatible type List[str]; expected List[<uninhabited>]

h(g(['a']))
h(g(['a'])) # E: Argument 1 to "g" has incompatible type List[str]; expected List[<uninhabited>]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like a regression, and I suspect that there are other cases that could be affected. By special casing the fix to lambda expressions this should hopefully be fixed.

(lambda *, x, y: x + y)(x=1, y="") # E: Unsupported operand types for + ("int" and "str")
[out]
main:1: error: Incompatible types in assignment (expression has type "int", variable has type "str")
main:1: error: Incompatible return value type (got "int", expected "str")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you know why we are getting two error messages here?

@elazarg
Copy link
Contributor Author

elazarg commented Apr 4, 2017

Now it's pretty surgical. But it bothers me that I do [self.accept(a) for a in e.args] - this must have happened somewhere else so it's redundant.

@elazarg
Copy link
Contributor Author

elazarg commented Apr 4, 2017

I think limiting the context to LambdaExpr makes this feature much less useful. I would have liked mypy to check

((lambda x: x+1) if condition else (lambda x: x+2))("1")

So I would like to understand what are the potential problems with always propagating the context. Maybe there's another way to avoid these problems.

@JukkaL
Copy link
Collaborator

JukkaL commented Apr 4, 2017

Agreed that the limitation makes this not very useful. The change in the output of a test case indicates that there was a behavior change elsewhere, and it might be because there is no type context when we infer the argument types, but I'm not sure. There is also the fact that the argument types are inferred repeatedly, which might result in poor performance when type checking large expressions, such as those that are machine-generated.

I wouldn't recommend making this much more complicated, since the code around type checking calls is already very difficult to understand. Maybe also special casing ConditionExpr would be reasonable, but it feels pretty ad hoc.

@JukkaL JukkaL self-assigned this Jun 30, 2017
Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

I realized that this only works reliably with positional arguments.

self.try_infer_partial_type(e)
callee_type = self.accept(e.callee, always_allow_any=True)
if isinstance(e.callee, LambdaExpr):
type_context = CallableType([self.accept(a) for a in e.args], e.arg_kinds, e.arg_names,
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to take the argument kinds and names from the lambda expression (not the call expression) and map the actual argument to formal arguments. Currently this doesn't work correctly, for example:

(lambda s, i: s[i])(i=0, s='x')

Copy link
Collaborator

@JukkaL JukkaL left a comment

Choose a reason for hiding this comment

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

Thanks for the updates! Looks good now.

@JukkaL JukkaL merged commit 69af980 into python:master Aug 15, 2017
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