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

elazarg · 2017-09-26T19:45:52Z

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.

Reverse subtyping check. This change alone fixes the issue described above.
For classmethod, fallback on the argument instead of on the parameter
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].
Better error message

Take 2 of #3227, but without the reverse-operator part

1. Reverse subtyping check for self argument 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

JukkaL

Thanks for the PR! Note that this is not a full review, just some quick notes.

Can you update the PR message to describe the problem that this PR fixes, with an example? Currently I need to look up the issue to understand what is going on. Another nit is that the title of the PR doesn't communicate the purpose of this PR very well, but maybe it's hard to summarize it better.

I checked that this doesn't cause any issues with internal Dropbox codebases.

JukkaL · 2017-09-28T17:30:19Z

mypy/checkmember.py

-                      context: Context, msg: MessageBuilder) -> None:
+def check_self_arg(functype: FunctionLike, original_type: Type, is_classmethod: bool,
+                   context: Context, name: str, msg: MessageBuilder) -> None:
+    """Check that the the most precise type of the self argument is compatible


Style nit: in a multi-line docstring there should be an empty line after the first line.

I'll fix it on the next commit or before merge.

The docstring could also be clarified a bit. The meaning of "the most precise type of the self argument" wasn't clear initially. Maybe something about generic self, and using the actual type of the self (based on the receiver type) rather than the declared type in the function signature? Would also be good to describe original_type here -- it would even make sense to rename it. What about actual_self_type? It's always the type of self, and it's the actual instead of the formal/declared type. If you rename it, it would also make sense to rename it elsewhere.

There are 4 potentially distinct types in play here:

The declared type of the first parameter self (potentially generic)

The inferred/declared type of x in x.f, ignoring the function signature

The same type as (2) but after narrowing it using the current class

The type of the current class

(4) is almost irrelevant at this point, but it is used to construct (3), restricts the valid types for (2), and adds to the potential confusion.
So both (2) and (3) are "actual self type". Maybe I should elaborate on the docstring/comment, but it will become lengthy.

elazarg · 2017-09-28T18:06:42Z

Updated the PR description and the title. I hope it's an improvement.

gvanrossum · 2017-09-29T15:54:24Z

Sorry, this is too late to make it into the 0.530 release. It's a minor issue and it touches subtle parts of the codebase. Let's postpone it.

JukkaL

Thanks for fixing this -- it now looks cleaner (plus it also works better!). This concludes my review.

Some of the self type related code is still kind of hard to understand -- I added some ideas below for how to clarify things. If some of the suggested changes would require many additional changes (such as renaming original_type to something else) it's probably better to do that as a separate PR.

JukkaL · 2017-10-10T16:03:02Z

mypy/checkmember.py

@@ -314,7 +316,9 @@ def analyze_var(name: str, var: Var, itype: Instance, info: TypeInfo, node: Cont
                # methods: the former to the instance, the latter to the
                # class.
                functype = t
-                check_method_type(functype, itype, var.is_classmethod, node, msg)
+                # Use meet to simulate dispatch - e.g. reduce Union[A, B] to A on dispatch to A


The comment is unclear. Can you add a comment with an example where this makes a difference?

I added a comment and a test. This kind of dispatch with not-exactly-the-right-type occur in reverse operators.

I believe the same thing should also be done with plain functions, but it's not checked at all now. i.e. there should be a call to check_self_arg before bind_self here. For example:

class A: def f(self: B) -> None: pass # type: ignore - because reasons. probably useful for metaclasses class B(A): pass A().f() # should be an error

but changing plain functions breaks some typeshed code (specifically, AbstractSet operators and the version_info tests break).

JukkaL · 2017-10-10T16:05:21Z

mypy/checkmember.py

-                selfarg = selfarg.fallback
-            if not subtypes.is_subtype(selfarg, itype):
-                msg.invalid_method_type(item, context)
+            msg.fail('Attribute function with type %s does not accept self argument'


Add a method to Messages instead of building the message here.

JukkaL · 2017-10-10T16:10:36Z

test-data/unit/check-selftype.test


 class B(A):
    pass

+reveal_type(X().__members__)  # E: Revealed type is '__main__.X*'


Would it make sense to add a test case similar to this, but so that X would a subtype of Tuple?

JukkaL · 2017-10-10T16:17:01Z

mypy/checkmember.py

-                      context: Context, msg: MessageBuilder) -> None:
+def check_self_arg(functype: FunctionLike, original_type: Type, is_classmethod: bool,
+                   context: Context, name: str, msg: MessageBuilder) -> None:
+    """Check that the the most precise type of the self argument is compatible


The docstring could also be clarified a bit. The meaning of "the most precise type of the self argument" wasn't clear initially. Maybe something about generic self, and using the actual type of the self (based on the receiver type) rather than the declared type in the function signature? Would also be good to describe original_type here -- it would even make sense to rename it. What about actual_self_type? It's always the type of self, and it's the actual instead of the formal/declared type. If you rename it, it would also make sense to rename it elsewhere.

…-selftype

elazarg · 2017-10-11T10:21:29Z

@JukkaL I will only be available for a couple of hours from now, so I won't be able to respond to further comments in time, unless ~30 hours from now will be good enough. As I understand it, the behavior is OK, and I hope it will be possible to resolve further issues in docstrings and comments even after the branch is cut.

JukkaL

Just a few minor things. If you can't make the updates before I cut the release branch this Friday, I can merge this as such before cutting the branch.

JukkaL · 2017-10-11T12:50:08Z

test-data/unit/check-selftype.test


+[case testSelfTypeProperSupertypeAttribute]
+from typing import Callable, TypeVar
+T = TypeVar('T', bound=object)


bound=object is redundant -- this is equivalent to T = TypeVar('T'). It would be nice to have a test case with a non-object bound.

I know it's redundant, I just wanted to be clear about the intention of the test. But yes, I should add a non-object bound

JukkaL · 2017-10-11T12:54:28Z

test-data/unit/check-selftype.test

+reveal_type(X.g)  # E: Revealed type is 'builtins.int'
+reveal_type(X.gt)  # E: Revealed type is 'def () -> __main__.X'
+reveal_type(X.f())  # E: Revealed type is 'builtins.int'
+reveal_type(X.ft())  # E: Revealed type is 'def () -> __main__.X'


Can you add a __init__ to X that takes an argument so that it's obvious that the signature is derived from the constructor?

JukkaL · 2017-10-11T14:48:00Z

test-data/unit/check-selftype.test

+a: A[int]
+b: B[str]
+reveal_type(a.g)  # E: Revealed type is 'builtins.int'
+--reveal_type(a.gt)  # E: Revealed type is 'builtins.int'


Why are these commented out?

The instantiation of selftype fails in the commented tests. I think it is not a problem in this PR, and fixing these requires changes in bind_self.

~~I just forced an update that removed two of the comments;~~ I hope it doesn't cause a mess.

Sorry - I didn't force it; I will add as a new commit in a moment.

To elaborate, bind_self does not unify K[T] and A[T] even if it can. It just does not check for this case, and simply trims the first argument. It only performs such unification is for TypeType.

JukkaL

Thanks, looks good.

Fix handling of non-method callable attribute (python#3223)

023219b

1. Reverse subtyping check for self argument 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

elazarg mentioned this pull request Sep 26, 2017

Release 0.530 planning #4009

Closed

5 tasks

JukkaL reviewed Sep 28, 2017

View reviewed changes

elazarg changed the title ~~Fix handling of non-method callable attribute (take 2)~~ Fix checking of self when accessing a non-method callable attribute (take 2) Sep 28, 2017

elazarg mentioned this pull request Oct 10, 2017

Release 0.540 planning #4084

Closed

JukkaL self-assigned this Oct 10, 2017

JukkaL reviewed Oct 10, 2017

View reviewed changes

elazarg added 6 commits October 11, 2017 00:15

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

dd9e609

…-selftype

elaborate dispatch

295d3ff

try to elaborate docstring

abd5426

move and improve error message for 0-arg attr function

3d941a5

add and improve tests: Tuple and Meta

46abe18

demonstrate the need for meet

f2c6d32

JukkaL reviewed Oct 11, 2017

View reviewed changes

test for non-object bound; add explicit __init__

fae22af

JukkaL reviewed Oct 11, 2017

View reviewed changes

add test for generic instantiation

a989775

elazarg force-pushed the fix-only-property-selftype branch from 385a5f6 to a989775 Compare October 11, 2017 14:50

remove commmenting of two lines

eb74f85

JukkaL approved these changes Oct 12, 2017

View reviewed changes

JukkaL merged commit bf9dc4c into python:master Oct 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

elazarg commented Sep 26, 2017 •

edited

Loading

JukkaL left a comment

JukkaL Sep 28, 2017

elazarg Sep 28, 2017

JukkaL Oct 10, 2017

elazarg Oct 10, 2017 •

edited

Loading

elazarg commented Sep 28, 2017

gvanrossum commented Sep 29, 2017

JukkaL left a comment

JukkaL Oct 10, 2017

elazarg Oct 11, 2017

JukkaL Oct 10, 2017

JukkaL Oct 10, 2017

JukkaL Oct 10, 2017

elazarg commented Oct 11, 2017

JukkaL left a comment

JukkaL Oct 11, 2017

elazarg Oct 11, 2017

JukkaL Oct 11, 2017

JukkaL Oct 11, 2017

elazarg Oct 11, 2017 •

edited

Loading

elazarg Oct 11, 2017

elazarg Oct 11, 2017

JukkaL left a comment •

edited

Loading

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

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

Conversation

elazarg commented Sep 26, 2017 • edited Loading

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

elazarg Oct 10, 2017 • edited Loading

Choose a reason for hiding this comment

elazarg commented Sep 28, 2017

gvanrossum commented Sep 29, 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

elazarg commented Oct 11, 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

elazarg Oct 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JukkaL left a comment • edited Loading

Choose a reason for hiding this comment

elazarg commented Sep 26, 2017 •

edited

Loading

elazarg Oct 10, 2017 •

edited

Loading

elazarg Oct 11, 2017 •

edited

Loading

JukkaL left a comment •

edited

Loading