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

Support method plugin hooks on unions #6560

Merged
merged 17 commits into from Jul 4, 2019

Conversation

@ilevkivskyi
Copy link
Collaborator

commented Mar 18, 2019

Fixes #6117
Fixes #5930

Currently both our plugin method hooks don't work with unions. This PR fixes this with three things:

  • Moves a bit of logic from visit_call_expr_inner() (which is a long method already) to check_call_expr_with_callee_type() (which is a short method).
  • Special-cases unions in check_call_expr_with_callee_type() (normal method calls) and check_method_call_by_name() (dunder/operator method calls).
  • Adds some clarifying comments and a docstring.

The week point is interaction with binder, but IMO this is the best we can have for now. I left a comment mentioning that check for overlap should be consistent in two functions.

In general, I don't like special-casing, but I spent several days thinking of other solutions, and it looks like special-casing unions in couple more places is the only reasonable way to fix unions-vs-plugins interactions.

This PR may interfere with #6558 that fixes an "opposite" problem, hopefully they will work together unmodified, so that accessing union of literals on union of typed dicts works. Whatever PR lands second, should add a test for this.

@ilevkivskyi ilevkivskyi requested review from JukkaL and Michael0x2a Mar 18, 2019

@Michael0x2a
Copy link
Collaborator

left a comment

I think looks good to me -- I couldn't really find anything I wanted to give feedback on.

I guess I am a little uncomfortable with how many different check_call_blah methods we're accumulating, but I don't think that's really relevant to this pull request.

I think we should merge this first -- I can add the extra tests to my PR.

@JukkaL
Copy link
Collaborator

left a comment

Thanks for fixing this! Generally looks good, left some ideas about making the relevant code a bit easier to understand.

"""Type check call expression.
The given callee type overrides the type of the callee
expression.

This comment has been minimized.

Copy link
@JukkaL

JukkaL Apr 1, 2019

Collaborator

The above docstring part was a bit confusing. It looks like we aren't actually overriding the callee type but to reusing a previously calculated callee type. This looks like an optimization. Maybe update the docstring to mention that? Otherwise it's not clear whether recalculating the callee type is correct.

"""Type check call expression.
The given callee type overrides the type of the callee
expression.
The 'callable_name' and 'object_type' are used to call plugin hooks.

This comment has been minimized.

Copy link
@JukkaL

JukkaL Apr 1, 2019

Collaborator

Also 'member' is used for plugin hooks only? Maybe be more explicit and say that they are only used for this purpose.

in_literal_context=self.is_literal_context())
self.msg.enable_errors()
item = self.narrow_type_from_binder(e.callee, item)
# TODO: This check is not one-to-one with non-special-cased behavior.

This comment has been minimized.

Copy link
@JukkaL

JukkaL Apr 1, 2019

Collaborator

Do you know what issues this may cause? Currently it's not clear if this is a serious problem or not.

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jun 29, 2019

Author Collaborator

Actually I found a cleaner solution, I will push an update soon.

callable_name, callee_type, e.args, e.arg_kinds, e, e.arg_names, object_type)
# Unions are special-cased to allow plugins to act on each item in the union.
elif member is not None and isinstance(object_type, UnionType):
res = [] # type: List[Type]

This comment has been minimized.

Copy link
@JukkaL

JukkaL Apr 1, 2019

Collaborator

Can you move the elif block to a separate method? Now the largest part of this function handles a rare special case, obscuring the main flows.

@@ -2021,6 +2058,22 @@ def check_method_call_by_name(self,
"""
local_errors = local_errors or self.msg
original_type = original_type or base_type
# Unions are special-cased to allow plugins to act on each element of the union.
if isinstance(base_type, UnionType):
res = [] # type: List[Type]

This comment has been minimized.

Copy link
@JukkaL

JukkaL Apr 1, 2019

Collaborator

Similar to above, it would be better to move this block to a separate function.

@Michael0x2a Michael0x2a referenced this pull request Jul 2, 2019

Closed

Release 0.720 planning #7137

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 3, 2019

@Michael0x2a @JukkaL I addressed the comments from last round, do you have any more comments here?

@JukkaL

JukkaL approved these changes Jul 3, 2019

Copy link
Collaborator

left a comment

Looks good, though I didn't look at all the recent checker updates in much detail. Just a few comments about tests.

if isinstance(x.meth, int):
reveal_type(x.meth) # N: Revealed type is 'builtins.int'
else:
reveal_type(x.meth(int())) # N: Revealed type is 'builtins.int'

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jul 3, 2019

Collaborator

Since the plugin switches types between int/str and the existing types are also int/str, it's a bit hard to see what is going on. What about using more than two separate types?

This comment has been minimized.

Copy link
@ilevkivskyi

ilevkivskyi Jul 4, 2019

Author Collaborator

OK, I now have different return types for two meth methods, and correspondingly different type conversions for the two hooks: str -> int, and float -> int, this way the call would fail if a wrong hook would act, and hopefully this is now easier to read.

class Foo:
def meth(self, x: str) -> str: ...
class Bar:
def meth(self, x: int) -> str: ...

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jul 3, 2019

Collaborator

If each meth variant would have a distinct return type, the test would perhaps catch more issues.

if isinstance(x.meth, int):
reveal_type(x.meth) # N: Revealed type is 'builtins.int'
else:
reveal_type(x.meth(int())) # N: Revealed type is 'builtins.int'

This comment has been minimized.

Copy link
@JukkaL

JukkaL Jul 3, 2019

Collaborator

This is similar to above -- maybe use more than two separate types.

@ilevkivskyi ilevkivskyi merged commit 72734f2 into python:master Jul 4, 2019

2 checks passed

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

@ilevkivskyi ilevkivskyi deleted the ilevkivskyi:union-plugin-other branch Jul 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.