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

dgelessus · 2018-10-01T00:58:23Z

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

…n#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.

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.

ilevkivskyi

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

Could you please also add tests for your fixes?

ilevkivskyi · 2018-10-03T10:47:17Z

mypy/checkexpr.py

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


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?

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:

Look up the type of the called method (using checkexpr.ExpressionChecker.accept for "real" calls, or checkmember.analyze_member_access for implicit calls)

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.

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.

ilevkivskyi · 2018-10-03T10:52:24Z

mypy/checkexpr.py

                if info:
                    fullname = '{}.{}'.format(info.fullname(), e.callee.name)
                    object_type = callee_expr_type
-                    # Apply plugin signature hook that may generate a better signature.


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

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.

ilevkivskyi · 2018-10-03T11:05:14Z

mypy/checkexpr.py

+        """Type check a call to a named method on an object.

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


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.

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 · 2018-10-26T23:59:24Z

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 · 2018-10-28T23:51:15Z

@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

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

mypy/checkexpr.py

ilevkivskyi · 2018-10-28T23:55:14Z

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


See my comment in the issue.

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

mypy/checkexpr.py

ilevkivskyi · 2018-10-28T23:58:13Z

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:


Could you please add a docstring for this method?

ilevkivskyi · 2018-10-28T23:59:05Z

mypy/checkexpr.py

        The given callee type overrides the type of the callee
        expression.
        """
+


Another redundant newline.

mypy/checkexpr.py

ilevkivskyi · 2018-10-29T00:07:52Z

test-data/unit/check-custom-plugin.test

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


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

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.

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 · 2018-10-29T15:18:19Z

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 · 2018-11-01T11:18:36Z

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 · 2018-11-01T11:23:04Z

(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 · 2018-11-05T12:44:34Z

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.

To hook into class instantiation, a function hook for the class name should be used instead, see discussion in python#5700.

ilevkivskyi

Thanks! Looks perfect now.

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.

dgelessus added 2 commits October 1, 2018 01:54

Fix method signature hooks not being called for dunder methods (pytho…

ab22b38

…n#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.

dgelessus mentioned this pull request Oct 1, 2018

Plugin for ctypes.Array #4869

Merged

5 tasks

Fix flake8 and mypy errors

94425b7

ilevkivskyi reviewed Oct 3, 2018

View reviewed changes

dgelessus added 6 commits October 24, 2018 22:18

Add unit test case for method signature hook plugin

5a526a1

Add helper method to determine qualified method names for plugin hooks

050669c

Support TupleType in ExpressionChecker.method_fullname

654f819

Add test to check that plugins receive fully qualified method names

09f2c0b

Merge branch 'master' into fix_dunder_method_hooks

52b3667

ilevkivskyi reviewed Oct 29, 2018

View reviewed changes

dgelessus added 5 commits October 30, 2018 19:14

Fix some minor formatting issues

c6c3e9f

Document ExpressionChecker.transform_callee_type

9db5f7b

Support class methods in ExpressionChecker.method_fullname

7b17f6d

Fix another flake8 issue

bb8d2b5

Fix None type error in ExpressionChecker.visit_call_expr_inner

7bde377

Don't test that method hooks are called for __init__

3bc56f4

To hook into class instantiation, a function hook for the class name should be used instead, see discussion in python#5700.

ilevkivskyi approved these changes Nov 5, 2018

View reviewed changes

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

Uh oh!

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

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

Uh oh!

Conversation

dgelessus commented Oct 1, 2018

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgelessus commented Oct 26, 2018

Uh oh!

ilevkivskyi commented Oct 28, 2018

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dgelessus commented Oct 29, 2018

Uh oh!

ilevkivskyi commented Nov 1, 2018

Uh oh!

ilevkivskyi commented Nov 1, 2018

Uh oh!

dgelessus commented Nov 5, 2018

Uh oh!

ilevkivskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants