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

Fix plugin hooks not being called correctly for dunder methods #5700

Merged
merged 15 commits into from Nov 5, 2018

Conversation

Projects
None yet
2 participants
@dgelessus
Copy link
Contributor

dgelessus commented Oct 1, 2018

Fixes #4964. The two commits fix the first and second bullet point, respectively.

Please be aware that I don't the mypy codebase very well. I checked that these changes work for my purposes and pass the test suite, but I don't know if this is the right/best way to fix these issues. (In particular, the second commit does some smallish refactoring that I'm not entirely sure about.) Any feedback is welcome.

Also, I noticed that because of the changes in my first commit, the method signature hooks are now called "too often" for overloaded methods: the hook is first applied to all overloads of the method, then a matching overload is selected, and the hook is applied again to the selected overload. Previously the hook was only applied before overload selection. I have no idea if this change matters in practice, or if it's worth fixing.

@ilevkivskyi

dgelessus added some commits Sep 26, 2018

Fix method signature hooks not being called for dunder methods (#4964)
Previously, method signature hooks were only looked up and applied for
"real" method calls in the source code, but not for implicit dunder
method calls.
Fix handling of plugin hooks for dunder methods (#4964)
Previously, most implicit dunder method calls were checked in two
separate steps: first the dunder method's signature was looked up on
the object's type, then the dunder method call was checked against that
signature. In the second step, the type of the self was not supplied
properly. Because of this, the method call was treated like a function
call by the plugin mechanism, and method hooks weren't looked up
properly.

This bug did not occur for binary operator dunder methods (such as
__add__); the binary operator checking code used helper methods to
supply the type of self correctly. These helper methods have been
generalized to work with any kind of method (not just binary operator
methods), and are now used consistently for all implicit method calls.

@dgelessus dgelessus referenced this pull request Oct 1, 2018

Merged

Plugin for ctypes.Array #4869

5 of 5 tasks complete
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks for working on this! I added some comments below.

Could you please also add tests for your fixes?

signature_hook = self.plugin.get_method_signature_hook(callable_name)
if signature_hook:
callee = self.apply_method_signature_hook(
callee, args, arg_kinds, context, arg_names, object_type, signature_hook)

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 3, 2018

Collaborator

I am not sure moving this inside check_call is a good idea. This function is expect to receive an actual callee type. Since, as you noticed, it is used in overload selection and checks, and IIRC in few other places. Is it possible to fix the issue without moving this here?

This comment has been minimized.

@dgelessus

dgelessus Oct 7, 2018

Contributor

I think I understand what you mean - check_call is supposed to receive a concrete callable type and check that the given argument list can be applied to that callable type. The code that calls check_call has already determined the callable type, so check_call shouldn't try to modify it using plugin hooks.

The reason why I put the plugin hook call into check_call is because of how method calls are checked in mypy. The usual pattern seems to be:

  1. Look up the type of the called method (using checkexpr.ExpressionChecker.accept for "real" calls, or checkmember.analyze_member_access for implicit calls)
  2. Check the actual arguments of the method call against the method type (using checkexpr.ExpressionChecker.check_call)

It's not possible to call method signature hooks in the first step - the functions/methods used there are generic to all attribute lookups, not just method calls, so they don't receive the method call-specific information needed by method signature hooks (specifically, information about the actual argument types/values of the call). Since only the second step (check_call) has all the necessary information, I put the method signature hook call there.

Maybe it would be better to instead refactor the "look up and apply method signature hook" code into a helper method, and call that manually between steps 1 and 2 where appropriate.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 7, 2018

Collaborator

Maybe it would be better to instead refactor the "look up and apply method signature hook" code into a helper method, and call that manually between steps 1 and 2 where appropriate.

This sounds like a promising approach.

@@ -297,12 +297,6 @@ def visit_call_expr_inner(self, e: CallExpr, allow_none_return: bool = False) ->
if info:
fullname = '{}.{}'.format(info.fullname(), e.callee.name)
object_type = callee_expr_type
# Apply plugin signature hook that may generate a better signature.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 3, 2018

Collaborator

Ten lines above (can't comment there) is a TODO item about adding support for other types. I think this is something worth fixing. I would add at least CallableType with .is_type_obj() returning True (for methods accessed on class objects), and TupleType (for named tuples).

This comment has been minimized.

@dgelessus

dgelessus Oct 7, 2018

Contributor

I'll have a look. Agreed with your other comment below, this code should probably go into a helper method that converts a callee type and a method name into a fully qualified name.

Return tuple (result type, inferred operator method type).
Return tuple (result type, inferred method type).

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 3, 2018

Collaborator

Maybe this is a better place to add hook application? If necessary (after adding support for more types in visit_call_expr_inner) you can refactor that logic to a helper method.

dgelessus added some commits Oct 24, 2018

Move method signature hook lookup/call into a helper method
Previously, method signatures were applied unconditionally in
check_call, however this caused them to be applied in places where they
shouldn't be (for example during overload resolution). This code has
been moved into a separate helper method, which is explicitly called in
the appropriate places.
@dgelessus

This comment has been minimized.

Copy link
Contributor

dgelessus commented Oct 26, 2018

Sorry for the long delay, things were busy for me in the last few weeks.

I've mostly implemented the changes discussed above, but I ran into a couple of issues:

  • The "get and apply method signature hook if applicable" code is now in a method called transform_callee_type. I'm not sure if it's the best name - apply_method_signature_hook was already taken, so I used a more generic name.
    • However, this refactoring has re-broken method signature hooks for __init__ methods. (One of the tests I added is failing because of this.) I don't see any good way to apply method signature hooks when the __init__ method is looked up, because that is done in a separate function (checkmember.type_object_type) with no access to the ExpressionChecker.
  • The "object type and method name to fully qualified name" code is in method_fullname, and now also supports TupleType.
    • I also tried supporting CallableTypes that have is_type_obj() == True, but I couldn't figure out a way to get the fully qualified name of the type that a CallableType represents. As far as I can tell, the full name is currently "lost" when a class is converted to a CallableType.
@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Oct 28, 2018

@dgelessus

However, this refactoring has re-broken method signature hooks for __init__ methods

Maybe this is actually OK? We can have some kind of agreement, that class instantiation should be done via function hook applied to the class object. After all "There should be only one way to do it" probably applies here.

but I couldn't figure out a way to get the fully qualified name of the type that a CallableType represents

This is how you do it (not tested but will likely work):

if isinstance(tp, CallableType) and tp.is_type_obj():
    if isinstance(tp.ret_type, Instance):  # Normal class objects
        fullname = tp.ret_type.type.fullname()
    elif isinstance(tp.ret_type, TupleType):  # named tuples and similar type objects
        fullname = tp.ret_type.fallback.type.fullname()
    else:
        assert False, "To the best of my knowledge we don't support other type objects"

I will make another pass of review now.

@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thank you for working on this!
This looks very good! I only have few small comments.

Show resolved Hide resolved mypy/checkexpr.py
it is invoked on. Return `None` if the name of `object_type` cannot be determined.
"""

# TODO: Support CallableType (i. e. class methods) and possibly others

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 29, 2018

Collaborator

See my comment in the issue.

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 29, 2018

Collaborator

Sorry, not in the issue, I left the comment just above in this PR, need to sleep more :-)

Show resolved Hide resolved mypy/checkexpr.py
def transform_callee_type(
self, callable_name: Optional[str], callee: Type, args: List[Expression],
arg_kinds: List[int], context: Context,
arg_names: Optional[Sequence[Optional[str]]] = None, object_type: Optional[Type] = None) -> Type:

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 29, 2018

Collaborator

Could you please add a docstring for this method?

@@ -581,6 +601,10 @@ def check_call_expr_with_callee_type(self,
The given callee type overrides the type of the callee
expression.
"""

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 29, 2018

Collaborator

Another redundant newline.

Show resolved Hide resolved mypy/checkexpr.py

[file mypy.ini]
[[mypy]
plugins=<ROOT>/test-data/unit/plugins/fully_qualified_test_hook.py

This comment has been minimized.

@ilevkivskyi

ilevkivskyi Oct 29, 2018

Collaborator

I think it would be great to add a test for class objects (assuming the fix I suggested works).

This comment has been minimized.

@dgelessus

dgelessus Oct 29, 2018

Contributor

Right, I actually had a test case for that, but removed it while testing. I'll re-add it once I get method signature hooks working on class methods.

This comment has been minimized.

@dgelessus

dgelessus Oct 29, 2018

Contributor

Something else I forgot to mention: I also wanted to add a test case for method calls on TypedDicts (that's why the FullyQualifiedTestTypedDict declaration is there). But it seems that mypy doesn't allow any method calls on TypedDict instances, even standard methods like copy. Am I missing something here?

If method calls on TypedDicts are really not allowed by mypy, it doesn't make much sense to support them in method_fullname (or at least there should be a comment explaining that the TypedDict branch is never called at the moment).

@dgelessus

This comment has been minimized.

Copy link
Contributor

dgelessus commented Oct 29, 2018

We can have some kind of agreement, that class instantiation should be done via function hook applied to the class object.

I agree in general, ideally there shouldn't be two different ways to hook into class instantiation. However I have a case where a function hook is not sufficient: when dealing with the ctypes.Array constructor in #4869. This is because of how arrays are created in ctypes:

from ctypes import *
array_type = c_int * 4
array_obj = array_type(1, 2, 3, 4)

The type of array_type here is Type[Array[c_int]]. The parameter types of the array_type constructor vary depending on the type argument of array_type (in this case c_int). Since I need to modify the parameter types, a normal function hook is not enough.

Basically I'd need a "function signature hook" here, but since the plugin interface doesn't have that, I wrote a method signature hook on __init__ instead.

But aside from that case I agree, there shouldn't be any need to call plugin hooks on __new__ and __init__.

This is how you do it

Ah, I didn't think of looking at the signature's return type. That makes perfect sense, thanks for the hint.

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Nov 1, 2018

Basically I'd need a "function signature hook" here, but since the plugin interface doesn't have that, I wrote a method signature hook on __init__ instead.

Hm, I see. So basically you just want to type-check that arguments are valid for given callable. It looks like we have only two ways to proceed here:

  • Add function_signature_hook, that will essentially restore the symmetry between functions/methods. But will be probably very rarely used.
  • Try using the current function hook instead. It has all the necessary things to generate mypy errors (ctx.api.msg.fail("Error message", ctx.context) will do the things). Then you can get the type argument from ctx.default_return_type and type check the arguments "manually" by importing mypy.subtypes.is_subtype().

The second option looks a bit hacky, but TBH I prefer the second option, because although it may be a bit more tedious, I seems to me the current situation is rather an exception to introduce a new hook only for it. What do you think?

@ilevkivskyi

This comment has been minimized.

Copy link
Collaborator

ilevkivskyi commented Nov 1, 2018

(I am happy will all the changes you made since last review, after we decide on this question I think the PR can be merged.)

@dgelessus

This comment has been minimized.

Copy link
Contributor

dgelessus commented Nov 5, 2018

You're right, in many cases a normal function hook is enough, but there are also some use cases for a function signature hook. An example outside of the ctypes plugin would be map(func, *iters), where a function signature hook could be used to inter the types of the iters based on the type of func. (This is just an example, it's really not necessary in practice, because the typeshed stubs have specialized map overloads for up to 5 iterables.)

Yes, this could also be implemented using the second option with manual type checking and errors, but as you said that seems a bit hacky 😃 But I think I'll use the second option for now in the ctypes plugin, since it's the only option that doesn't require more extra work. If function signature hooks are added to mypy at some point, the ctypes plugin can be changed to use them instead.

Don't test that method hooks are called for __init__
To hook into class instantiation, a function hook for the class name
should be used instead, see discussion in #5700.
@ilevkivskyi
Copy link
Collaborator

ilevkivskyi left a comment

Thanks! Looks perfect now.

@ilevkivskyi ilevkivskyi merged commit 5fda3ed into python:master Nov 5, 2018

2 checks passed

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

dgelessus added a commit to dgelessus/mypy that referenced this pull request Nov 12, 2018

Reimplement ctypes.Array constructor hook as discussed in python#5700
Class instantiation intentionally does not call plugin hooks on
__init__, instead function hooks for the class name are called.
Since plugin hooks cannot modify the signature of a function call
(there is no "function signature hook", unlike for methods), we instead
perform manual argument type checking in a regular function hook.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment