Fix handling of non-method callable attribute #3227

elazarg · 2017-04-23T17:18:58Z

Rewrite check_method_type:

1. Reverse the subtyping check. It seems to be backwards. This change alone fixes #3223.

2. Pass original_type for the subtype check, which is more precise - in particular, there seems no need to fallback from TupleType. It also yields better error messages.

3. Make the error message clearer (I think. see the tests)

4. The classmethod case goes backwards too - instead of falling back in the parameter, fall back in the argument. I think it's the straightforward thing.

The PR does not pass stub tests due to the following error:

> typeshed/stdlib/2and3/fractions.pyi:80: error: Invalid self argument "Union[int, float, Decimal, Real]" to attribute function Callable[[Real, Any], bool]

For this line:

python <br /> def __gt__(self, other: _ComparableNum) -> bool: ... <br />

And similarly for __ge__. I did not figure out yet why it happens.

elazarg · 2017-04-24T00:40:26Z

This looks like the minimal repro:

from typing import Union
from abc import abstractmethod

class A:
    @abstractmethod
    def __lt__(self, other) -> bool: ...

class B:
    def __gt__(self, other: Union[int, A]) -> bool: ...  # E: Invalid self argument "Union[str, A]" to attribute function Callable[[B, Any], bool]

I think info.get_method(name) in analyze_member_access does not handle decorated functions properly.

The current fix avoids this case by performing the test for each item of the union separately.

gvanrossum · 2017-04-24T15:20:50Z

Repro of what? That snippet gives no errors for me. How did you run it? (Flags, mypy.ini.)

gvanrossum · 2017-04-24T15:22:36Z

mypy/checker.py

@@ -796,6 +796,7 @@ def check_reverse_op_method(self, defn: FuncItem, typ: CallableType,
                arg_types = list(arg_type.iter_deep())
            else:
                return
+            # We check that each method is fine when dispatched on a proper self argument


FYI you can make your local commit history more relevant by using git commit --amend for such things (since you did not push to GitHub in between).

I understand I should put more info in the commit messages, but this comment is needed regardless. Or are you referring to the code below it?

Neither. I meant that I would prefer to see a single commit pushed to the PR that included the previous commit plus this one (which just added a comment that you forgot on the previous commit).

You can do that locally by using git commit --amend for the second commit (that adds the comment). You could also do this after the fact using git rebase -i.

elazarg · 2017-04-24T15:23:42Z

Sorry for being unclear. This is repro of the issue this PR had (before last commit). I mention it because I think it indicates an error outside this PR, and because it is the reason for noticeable part of this PR.

elazarg · 2017-04-24T15:37:07Z

All the changes in types.py and checker.py are only needed because of this issue.

JukkaL

I have a bunch of mostly minor things and requests for clarification. Also, maybe you should add a test case that includes the original example from #3223?

JukkaL · 2017-07-21T13:57:07Z

mypy/types.py

@@ -1130,6 +1130,13 @@ def relevant_items(self) -> List[Type]:
        else:
            return [i for i in self.items if not isinstance(i, NoneTyp)]

+    def iter_deep(self) -> Iterator[Type]:


Use union_items() instead of adding this method.

Also currently unions are flattened on creation in __init__ with the help of self.items = flatten_nested_unions(items)

JukkaL · 2017-07-21T14:00:50Z

test-data/unit/check-classes.test

@@ -1975,7 +1975,7 @@ class B:
    a = A
    bad = lambda: 42

-B().bad() # E: Invalid method type
+B().bad() # E: Attribute function of type Callable[[], int] does not accept self argument


I'd tweak message to be Attribute function with type Callable[...] .... Otherwise this could be understood to refer to an attribute of an object, where the object is callable.

JukkaL · 2017-07-21T14:01:59Z

mypy/checker.py

@@ -827,15 +828,17 @@ def check_reverse_op_method(self, defn: FuncItem, typ: CallableType,
            elif isinstance(arg_type, UnionType):
                if not arg_type.has_readable_member(other_method):
                    return
+                arg_types = list(arg_type.iter_deep())


Use union_items() instead of iter_deep().

JukkaL · 2017-07-21T14:03:38Z

mypy/checker.py

-                typ, method, defn.info,
-                typ2, other_method, cast(Instance, arg_type),
-                defn)
+            # We check that each method is fine when dispatched on a proper self argument


This comment is unclear. Can you improve it?

JukkaL · 2017-07-21T14:05:33Z

mypy/checker.py

+                    other_method, arg_type, defn)
+                self.check_overlapping_op_methods(
+                    typ, method, defn.info,
+                    typ2, other_method, cast(Instance, arg_type),


The cast is safe (though the unsafe is not introduced by this PR). Is there an easy way to fix it? If not, can you add a TODO comment explaining that it's unsafe?

I'm not sure how to fix it. Do you think replacing it with an assertion is appropriate, or should we have "get_instance_from(Type)"?

JukkaL · 2017-07-21T14:09:12Z

mypy/checkmember.py

@@ -368,33 +368,23 @@ def lookup_member_var_or_accessor(info: TypeInfo, name: str,
        return None


-def check_method_type(functype: FunctionLike, itype: Instance, is_classmethod: bool,
+def check_method_type(functype: FunctionLike, original_type: Type, is_classmethod: bool,


Add docstring that at least explains the purpose of this method and the original_type argument.

Also, can you explain why you this change helps?

The change is explained by point (2) above:

Pass original_type for the subtype check, which is more precise - in particular, there seems no need to fallback from TupleType. It also yields better error messages.

JukkaL · 2017-07-21T14:13:03Z

mypy/checkmember.py

-                if not subtypes.is_equivalent(clsarg, AnyType()):
-                    msg.invalid_class_method_type(item, context)
+            if is_classmethod:
+                original_type = TypeType(original_type)


This isn't safe, since TypeType.__init__ doesn't accept Type. TypeType.make_normalized would work around that, though it's unclear what's the correct thing to do.

JukkaL · 2017-07-21T14:14:47Z

mypy/checkmember.py

            # (This is sufficient for the current treatment of @classmethod,
            # but probably needs to be revisited when we implement Type[C]
            # or advanced variants of it like Type[<args>, C].)
-            clsarg = item.arg_types[0]
-            if isinstance(clsarg, CallableType) and clsarg.is_type_obj():


Why are these lines no longer needed? Is it because of the changes from itype to original_type?

It's because classmethod is handled differently, falling from the instance to the type (instead of the other way around, bullet (4) above). I think the comment is not needed too.

Actually, I think the entire function can be merged with bind_self() since it performs overlapping checks.

JukkaL · 2017-07-21T14:15:01Z

mypy/checkmember.py

-                    msg.invalid_class_method_type(item, context)
+            if is_classmethod:
+                original_type = TypeType(original_type)
+            if not subtypes.is_subtype(original_type, erase_to_bound(selfarg)):


This line needs a comment.

JukkaL · 2017-08-15T16:25:05Z

Can you fix the merge conflict? (You may want to wait until #3132 has been merged since it might cause more conflicts.)

…type

gvanrossum · 2017-09-26T15:47:31Z

@elazarg Can you at least fix the test failures before we spend more time reviewing this?

elazarg · 2017-09-26T19:31:32Z

Sorry, I was too sure that said failure was also a flake (like this last one) that I didn't take the time to check.

I'm going to close this PR, and open two separate ones, which should be easier to review. The review and commit history will not help too much.

1. Reverse subtyping check 2. For classmethod, fallback on the argument instead of on the parameter 3. Mimic dispatch better: meet original_type with static class 4. Better error message

…take 2) (#4016) Fix #3223. Minimal examples: ``` from typing import Callable class X: f: Callable[[object], None] X().f # E: Invalid method type ``` ``` class X: @Property def g(self: object): pass X().g # E: Invalid method type ``` The problem is that for non-methods the check performed on access to self is whether object <: X instead of the other way around. 1. Reverse subtyping check. This change alone fixes the issue described above. 2. For classmethod, fall back on the argument instead of on the parameter. 3. Mimic dispatch better: meet original_type with static class. This handles the case were instead of X() above there was something of type Union[X, Y]. 4. Better error message. Take 2 of #3227, but without the reverse-operator part.

elazarg added 5 commits April 23, 2017 19:18

reverse the subtyping test, more explicit error message, unify cases

5800819

reverse the direction of type fallback, avoid tuple fallback

a67b78c

lint

a523b1f

check access to reversed methods only per-item

78d0887

comment

3c8e85f

gvanrossum reviewed Apr 24, 2017

View reviewed changes

gvanrossum requested a review from JukkaL May 10, 2017 22:24

JukkaL self-assigned this May 12, 2017

elazarg mentioned this pull request Jun 4, 2017

AttributeError: 'UnionType' object has no attribute 'type' #3468

Closed

Merge branch 'master' into fix-property-selftype

42020fb

elazarg mentioned this pull request Jul 20, 2017

Metaclass type/instance difficulties #3625

Closed

Merge branch 'master' into fix-property-selftype

7aacb4f

JukkaL requested changes Jul 21, 2017

View reviewed changes

elazarg force-pushed the fix-property-selftype branch from aed080c to 950173a Compare July 25, 2017 11:17

rephrase error message, use union_items

6703816

elazarg force-pushed the fix-property-selftype branch from 950173a to 6703816 Compare July 25, 2017 11:19

elazarg added 2 commits July 25, 2017 14:34

add test with an unbound typevar

d7a41f9

Use TypeType.make_normalized

1c58d49

Merge branch 'master' into fix-property-selftype

8c4a8ce

ilevkivskyi mentioned this pull request Sep 4, 2017

Mypy forbids converting a method to a property in some cases #3913

Closed

elazarg added 3 commits September 24, 2017 20:33

Merge remote-tracking branch 'upstream/master' into fix-property-self…

499d596

…type

add docstrings and comments

71faee9

revert uncalled-for fix

2108a03

This was referenced Sep 24, 2017

Adding @abstractmethod __init__ to ABC breaks staticmethods #1706

Closed

Release 0.530 planning #4009

Closed

elazarg added 3 commits September 26, 2017 20:30

use meet to simulate dispatch; cleanup

a8a0386

better phrasing

bd1f0f3

assert instead of cast; iterate over union

733fd8e

elazarg closed this Sep 26, 2017

This was referenced Sep 26, 2017

Fix checking of self when accessing a non-method callable attribute (take 2) #4016

Merged

Cleanup check_reverse_op_method #4017

Merged

elazarg deleted the fix-property-selftype branch November 25, 2017 16:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix handling of non-method callable attribute #3227

Fix handling of non-method callable attribute #3227

elazarg commented Apr 23, 2017 •

edited by emmatyping

Loading

elazarg commented Apr 24, 2017

gvanrossum commented Apr 24, 2017

gvanrossum Apr 24, 2017

elazarg Apr 24, 2017

gvanrossum Apr 24, 2017 •

edited

Loading

elazarg commented Apr 24, 2017

elazarg commented Apr 24, 2017

JukkaL left a comment

JukkaL Jul 21, 2017

ilevkivskyi Jul 21, 2017

JukkaL Jul 21, 2017

JukkaL Jul 21, 2017

JukkaL Jul 21, 2017

JukkaL Jul 21, 2017

elazarg Jul 25, 2017

JukkaL Jul 21, 2017

elazarg Jul 25, 2017

JukkaL Jul 21, 2017

JukkaL Jul 21, 2017

elazarg Jul 25, 2017

JukkaL Jul 21, 2017

JukkaL commented Aug 15, 2017

gvanrossum commented Sep 26, 2017

elazarg commented Sep 26, 2017

Fix handling of non-method callable attribute #3227

Fix handling of non-method callable attribute #3227

Conversation

elazarg commented Apr 23, 2017 • edited by emmatyping Loading

elazarg commented Apr 24, 2017

gvanrossum commented Apr 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum Apr 24, 2017 • edited Loading

Choose a reason for hiding this comment

elazarg commented Apr 24, 2017

elazarg commented Apr 24, 2017

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JukkaL commented Aug 15, 2017

gvanrossum commented Sep 26, 2017

elazarg commented Sep 26, 2017

elazarg commented Apr 23, 2017 •

edited by emmatyping

Loading

gvanrossum Apr 24, 2017 •

edited

Loading