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

Add arguments to callee context in a call expression #3116

Merged
merged 8 commits into from Aug 15, 2017

Conversation

Projects
None yet
2 participants
@elazarg
Contributor

elazarg commented Apr 3, 2017

Fix #3097.

Show outdated Hide outdated mypy/checkexpr.py
Show outdated Hide outdated mypy/checkexpr.py
Show outdated Hide outdated test-data/unit/check-inference.test
Show outdated Hide outdated test-data/unit/check-inference-context.test
@elazarg

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Apr 4, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@elazarg

elazarg Apr 4, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@JukkaL

JukkaL Apr 4, 2017

Collaborator

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.

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

@JukkaL

I realized that this only works reliably with positional arguments.

Show outdated Hide outdated mypy/checkexpr.py
@JukkaL

JukkaL approved these changes Aug 15, 2017

Thanks for the updates! Looks good now.

@JukkaL JukkaL merged commit 69af980 into python:master Aug 15, 2017

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment