Rework how we refine information from callable() #4343

msullivan · 2017-12-12T01:00:19Z

Don't ever assume that something is not callable; basically anything
could be, and getting it wrong leaves code to not be typechecked.

When operating on an Instance that is not clearly callable, construct
a callable version of it by generating a fake subclass with a __call__
method to be used.

Fixes #3605

This is being posted to solicit feedback, I expect it might need some large changes.
The hack perpetrated here is somewhat grevious, and I suspect I missed something, so I am interested to find out what :).
Also, to reduce the change size during development I threaded a TypeChecker through a bunch of functions; the right thing might be to move all of those functions to be TypeChecker methods.

ilevkivskyi · 2017-12-12T16:01:51Z

test-data/unit/check-callable.test


 if not callable(b):
    5 + 'test'

 if callable(c):
-    reveal_type(c)  # E: Revealed type is '__main__.B'
+    reveal_type(c)  # E: Revealed type is 'Union[<callable subtype of A>, __main__.B]'


Just a random comment: we need to be sure that this type <callable subtype of A> will never appear in error messages (apart from actually reveal_type), since this can be confusing. I think it can be formatted just as A in other cases

Does there exist mechanism for having different internal and external names for instances?

Yes, see messages.py. There is a method MessageBuilder.format that controls most of the formatting. But, see other comments for this code that propose more radical solutions.

ilevkivskyi · 2017-12-12T16:02:47Z

the right thing might be to move all of those functions to be TypeChecker methods.

I remember I also wanted this few times.

JukkaL

This is a reasonable first try at this! I didn't do a detailed review yet but left some comments.

JukkaL · 2017-12-12T15:51:33Z

test-data/unit/check-callable.test


 if not callable(b):
    5 + 'test'

 if callable(c):
-    reveal_type(c)  # E: Revealed type is '__main__.B'
+    reveal_type(c)  # E: Revealed type is 'Union[<callable subtype of A>, __main__.B]'


Hmm this might be pretty surprising for users. I wonder if we should special case unions and only consider items with a known __call__ here, so that the inferred type would continue to be just __main__.B here. Yes, it would be ugly, but I'd say that pragmatism beats purity here, since the whole callable check is a legacy thing.

Random though: One way of thinking about this would be to consider this as similar to hasattr(c, '__call__'). We could perhaps eventually extend the support for callable to handle hasattr, and for hasattr I feel that__main__.B would be the correct type to infer here.

I think nearly all users will be surprised by this. And I think in actual code it is very rare to find a situation where c (defined as Union[A, B]) was an instance of a callable subclass of A. We may see the occasional non-callable class that has a callable subclass, but it's unlikely to figure in type discriminations like this. The realistic scenarios probably involve either a union of callable and non-callable classes, where there is no concern that one of non-callable classes has a callable subclass; or a non-callable class that has a callable subclass (but then the discrimination is just between the base class and the callable subclass).

JukkaL · 2017-12-12T15:52:55Z

test-data/unit/check-callable.test

+    if callable(o):
+        o()
+        1 + 'boom'  # E: Unsupported operand types for + ("int" and "str")
+        o()


Maybe test calling with some randon arguments? Also consider doing reveal_type(o).

JukkaL · 2017-12-12T15:53:21Z

test-data/unit/check-callable.test

+def g(o: Foo) -> None:
+    o.bar()
+    if callable(o):
+        o.bar()


Check that o.foo() is rejected here.

JukkaL · 2017-12-12T15:53:35Z

test-data/unit/check-callable.test

+    o.bar()
+    if callable(o):
+        o.bar()
+        o()


Again, also test call with arguments.

JukkaL · 2017-12-12T15:55:32Z

mypy/checker.py

+    return Instance(info, [])
+
+
+def make_fake_callable(type: Instance, typechecker: TypeChecker) -> Type:


Passing TypeChecker instances around is against the prevailing style in the codebase. Instead, it looks like you could just pass a callable corresponding to typechecker.named_type. However, other changes may introduce additional dependencies to TypeChecker and it may be better to turn this and other functions into type checker methods.

JukkaL · 2017-12-12T15:58:21Z

test-data/unit/check-callable.test

@@ -227,7 +227,7 @@ T = Union[Union[int, Callable[[], int]], Union[str, Callable[[], str]]]

 def f(t: T) -> None:
    if callable(t):
-        reveal_type(t())  # E: Revealed type is 'Union[builtins.int, builtins.str]'
+        reveal_type(t())  # E: Revealed type is 'Union[Any, builtins.int, builtins.str]'


Similar to above, if we special cased unions this would remain as previously, which arguably matches the likely intent of the check.

JukkaL · 2017-12-12T16:00:34Z

test-data/unit/check-callable.test

@@ -253,7 +253,7 @@ T = TypeVar('T', int, Callable[[], int], Union[str, Callable[[], str]])

 def f(t: T) -> None:
    if callable(t):
-        reveal_type(t())  # E: Revealed type is 'builtins.int'  # E: Revealed type is 'builtins.str'
+        reveal_type(t())  # E: Revealed type is 'Any'  # E: Revealed type is 'builtins.int'  # E: Revealed type is 'Union[Any, builtins.str]'


This change seems okay, but can you split the long line using \?

JukkaL · 2017-12-12T16:06:10Z

mypy/checker.py

@@ -2883,7 +2883,45 @@ def conditional_type_map(expr: Expression,
        return {}, {}


-def partition_by_callable(type: Type) -> Tuple[List[Type], List[Type]]:
+def intersect_instance_callable(type: Instance, callable_type: CallableType) -> Type:


Style nit: we generally prefer typ instead of type since type is a builtin :-( This doesn't apply to attribute access, since it's clear that foo.type doesn't refer to the builtin.

JukkaL · 2017-12-12T16:19:12Z

mypy/checker.py

+
+    # Build the fake ClassDef and TypeInfo together.
+    # The ClassDef is full of lies and doesn't actually contain a body.
+    cdef = ClassDef("<callable subtype of {}>".format(type.type.name()), Block([]))


This is the trickiest thing in this PR. However, there generally seems to be no better way.

There are several things that should be handled here, such as:

The original class may be generic.

The original class may inherit from Tuple[...] and in that case we should return TupleType, not Instance.

If the original class inherits from Any, the new class should also do that.

Basically you should check that every TypeInfo attribute is initialized correctly.

You may be able to share code with basic_new_typeinfo in mypy.semanal, but you shouldn't add a dependency from type checker to the semantic analyzer. You could move basic_new_typeinfo to some third module, perhaps.

Additionally, it's possible to leak these types through type inference to class and module top level scopes. This means that the types can be serialized in incremental mode, and you should test that it works. It may be necessary to store the created class in a symbol table (with a generated unique dummy name) for serialization to work, but I'm not sure about this.

gvanrossum · 2017-12-12T16:34:53Z

test-data/unit/check-callable.test

@@ -208,13 +208,13 @@ b = B()  # type: B
 c = A()  # type: Union[A, B]

 if callable(a):
-    5 + 'test'
+    5 + 'test' # E: Unsupported operand types for + ("int" and "str")


Nit of nits: our coding standard has two spaces before #.

gvanrossum · 2017-12-12T22:34:59Z

No, but you can special-case the way types are formatted with various methods/functions in messages.py.

JukkaL · 2017-12-13T16:56:35Z

On second thought, creating a new subclass might be bit of an overkill considering that this is not a very common use case. Here's an idea about a simpler "good enough" fix:

Only do anything with callable if the target type is a union. For unions, use the existing approach to narrow them down.
If the target is not a union, ignore the callable check -- the original type stays unmodified. This will fix the false negatives but can generate unwanted errors, but that's acceptable.

msullivan · 2017-12-13T23:55:34Z

Oops, I wish I had noticed your last comment this morning.
That seems like a reasonable behavior, though this is somewhat nicer behavior to provide?

Pushed changes to address most of the comments. I still need to move the functions into the TypeChecker class, but I've put that off to keep the diffs easy to read.

msullivan · 2017-12-14T01:31:29Z

Another way to structure this would be to return just a Callable from conditional_callable_type_map and push the generation of the intersecting class into narrow_declared_type. That would be a start towards implementing make_fake_intersection as discussed in #3603.

JukkaL

Thanks for the updates! This is looking pretty good (once you refactor the TypeChecker arguments away).

The following TypeInfo attributes might still need processing, though some of these are kind of unlikely to have an effect so just adding a TODO comment may suffice:

declared_metaclass
metaclass_type
is_abstract (say, if one does type(x)())
is_protocol (not sure about this)
runtime_protocol (if one does isinstance(y, type(x)) which would be pretty perverse)
protocol_members (not sure)
is_enum (if it's an enum)
_promote (you can test by doing if callable(x) when x: int and checking that the narrowed type is compatible with float)
tuple_type (discussed previously)

JukkaL · 2017-12-14T17:51:34Z

mypy/checker.py

+
+def intersect_instance_callable(type: Instance, callable_type: CallableType,
+                                typechecker: TypeChecker) -> Type:
+    """Creates a fake type that represents the intersection of an


Style nit: Format multi-line docstrings so that there is an empty lines after the first one and the final """" is on a line by its own. Example:

def foo(x): """Do stuff (a 1-line summary). More elaborate description of stuff. This can span multiple lines. """

JukkaL · 2017-12-14T17:56:14Z

test-data/unit/check-callable.test

+        o.bar()
+
+[builtins fixtures/callable.pyi]
+[case testCallableObjectAny]


Style nit: Add empty line before each test case.

msullivan · 2017-12-14T23:30:48Z

I think the is_abstract and protocol related fields aren't important. Doing type(x)() wouldn't try creating the abstract class, it would create the concrete class that x actually is. Similar with isinstance checks for a protocol type. The other stuff I think is now all either computed or correctly left blank. _promote being set in a parent class seems to do the right thing?

ilevkivskyi · 2017-12-14T23:39:44Z

There are certain checks for protocols. For example, isinstance(x, P) is prohibited if P is a protocol (unless it is decorated with @runtime).

msullivan · 2017-12-14T23:47:41Z

Right, but the concern here was isinstance(y, type(x)) where x is a protocol---but this isn't a problem, because type(x) is a regular concrete type.

JukkaL

LGTM! Just one minor nit. Feel to merge once you've fixed the typo and tested this against internal Dropbox codebases.

JukkaL · 2017-12-18T17:44:25Z

mypy/checker.py

+        # Build the fake ClassDef and TypeInfo together.
+        # The ClassDef is full of lies and doesn't actually contain a body.
+        # Use format_bare to generate a nice name for error messages.
+        # We skip filling fully filling out a handful of TypeInfo fields because they


Typo: 'filling fully filling out'.

Don't ever assume that something is *not* callable; basically anything could be, and getting it wrong leaves code to not be typechecked. When operating on an Instance that is not clearly callable, construct a callable version of it by generating a fake subclass with a __call__ method to be used. Fixes #3605

gvanrossum · 2017-12-21T03:26:10Z

🥇

msullivan requested review from JukkaL and gvanrossum December 12, 2017 01:00

ilevkivskyi reviewed Dec 12, 2017

View reviewed changes

JukkaL reviewed Dec 12, 2017

View reviewed changes

gvanrossum reviewed Dec 12, 2017

View reviewed changes

JukkaL reviewed Dec 14, 2017

View reviewed changes

msullivan force-pushed the callable_func_bad branch from 876087a to f04740d Compare December 15, 2017 00:46

JukkaL approved these changes Dec 18, 2017

View reviewed changes

msullivan added 9 commits December 18, 2017 11:29

Populate symbol table so serializing works

73078b0

Some test improvements

c6cbe8c

Special case unions to allow the expected type discrimination

6a7df8d

Handle Any, tweak naming, add tests for generics

48fa119

Handle tuples, do some other cleanups

94b0a72

Move lots of the helper functions directly into TypeChecker

8507b8b

Tuples are callable now, update test

511720f

fix doc typo

dd881a1

msullivan force-pushed the callable_func_bad branch from f04740d to dd881a1 Compare December 18, 2017 19:32

msullivan merged commit 38337a8 into master Dec 19, 2017

gvanrossum deleted the callable_func_bad branch December 21, 2017 03:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework how we refine information from callable() #4343

Rework how we refine information from callable() #4343

msullivan commented Dec 12, 2017 •

edited by ilevkivskyi

Loading

ilevkivskyi Dec 12, 2017

msullivan Dec 12, 2017

ilevkivskyi Dec 12, 2017

ilevkivskyi commented Dec 12, 2017

JukkaL left a comment

JukkaL Dec 12, 2017

gvanrossum Dec 12, 2017

JukkaL Dec 12, 2017

JukkaL Dec 12, 2017

JukkaL Dec 12, 2017

JukkaL Dec 12, 2017

JukkaL Dec 12, 2017

JukkaL Dec 12, 2017

JukkaL Dec 12, 2017

JukkaL Dec 12, 2017

gvanrossum Dec 12, 2017

gvanrossum commented Dec 12, 2017 via email

JukkaL commented Dec 13, 2017

msullivan commented Dec 13, 2017

msullivan commented Dec 14, 2017

JukkaL left a comment

JukkaL Dec 14, 2017

JukkaL Dec 14, 2017

msullivan commented Dec 14, 2017

ilevkivskyi commented Dec 14, 2017

msullivan commented Dec 14, 2017

JukkaL left a comment

JukkaL Dec 18, 2017

gvanrossum commented Dec 21, 2017

		return Instance(info, [])


		def make_fake_callable(type: Instance, typechecker: TypeChecker) -> Type:

Rework how we refine information from callable() #4343

Rework how we refine information from callable() #4343

Conversation

msullivan commented Dec 12, 2017 • edited by ilevkivskyi Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ilevkivskyi commented Dec 12, 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

gvanrossum commented Dec 12, 2017 via email

JukkaL commented Dec 13, 2017

msullivan commented Dec 13, 2017

msullivan commented Dec 14, 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

msullivan commented Dec 14, 2017

ilevkivskyi commented Dec 14, 2017

msullivan commented Dec 14, 2017

JukkaL left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvanrossum commented Dec 21, 2017

msullivan commented Dec 12, 2017 •

edited by ilevkivskyi

Loading