Conditional type-check based on callable call by afrieder · Pull Request #2627 · python/mypy

afrieder · 2017-01-01T06:06:39Z

Fixes #1973

This is my first PR to mypy, so let me know if this is out of scope, poor style, wrong, if it needs more tests, etc, etc. I'm happy to learn.

The naïve thing is to make use of conditional_type_map but the way that works, it basically casts the expr to the proposed_type, so passing in a generic CallableType instance (ie AnyType *args, **kwargs, and return) results in a type map where expr is just a generic CallableType. This new function instead preserves the signature of expr and will make Unions as necessary (see testUnionMultipleReturnTypes, which would not raise an error with a generic CallableType).

ambv

LGTM.

ambv · 2017-01-01T20:05:42Z

+        else:
+            return None, {}
+    else:
+        return {}, {}


nit: This could be factored with less indentation by reversing the order of conditional checks. Also, avoid backslashes. I know you based off conditional_type_map() but given it's already copied and pasted, how about:

if not current_type: return {}, {} if isinstance(current_type, CallableType): return {}, None if isinstance(current_type, UnionType): callables = [item for item in current_type.items if isinstance(item, CallableType)] # type: List[Type] non_callables = [item for item in current_type.items if not isinstance(item, CallableType)] # type: List[Type] return ( {expr: UnionType.make_union(callables)}, {expr: UnionType.make_union(non_callables)}, ) return None, {}

The two list comprehensions are a bit wasteful but I guess that's fine, we don't expect unions to be overly long.

Reformatted. I also changed it to a for loop. I initially thought the comprehensions would be faster, but basic experiments show I'm wrong.

ambv · 2017-01-01T20:15:00Z

@@ -0,0 +1,24 @@
+from typing import builtinclass, Tuple, TypeVar, Generic, Union


@builtinclass is no longer a thing, see #599. Just remove this import, you're not using it anyway.

ambv · 2017-01-01T21:28:33Z

👍

afrieder · 2017-01-02T00:26:57Z

I added more tests for chaining ands/ors with callable.
Otherwise this is good to go.

rwbarton · 2017-01-02T01:51:35Z

I have some doubts about this. Actually the issue is not with your code but the original feature request, but the issue only became clear when I read your implementation. In any case it seems better to comment here for now.

The issue is that there's actually almost no situation where we can correctly conclude that a type is not callable. This even applies to the original program which involved Union[str, Callable[[str], int]]. Technically a value of this type could be an instance of a subclass of str, and that subclass could define a __call__ method. In that case callable would return True at runtime, but the real call signature could be anything. Pretty much the only time we know for sure a value cannot be callable is if it is of a non-subclassable type, like bool. At a minimum, we should be treating type and other classes that are known to have __call__ methods, as well as classes inheriting from Any, as callable or potentially callable.

There's actually a similar issue with normal isinstance checks and generics: if I have a value of type Union[A, List[int]] and isinstance tells me it is a list then technically it could still be a subclass of both A and list and not actually be a list of ints. Mypy makes the simplifying assumption that there is no multiple inheritance in this situation. However there doesn't seem to be a reasonable analogous assumption to make for callable, since being callable is as easy as defining __call__, and doesn't require inheriting from a class.

afrieder · 2017-01-02T02:07:10Z

That's completely true.
Even in the following short example:

class T(int):
    def __call__(self) -> int:
        return self


def f(i: int) -> int:
    if callable(i):
        print("Impossible!")
    return i

f(T(5))

What's considered impossible while type-checking is very clearly possible at run-time.
So I guess I should close this PR and someone else should close the issue?

rwbarton · 2017-01-02T02:28:41Z

Well there are some bits that we can carve out here. For instance if we have a Union[str, Callable[[str], bool]] and we test and it turns out to not be callable then we can reasonably conclude that it's in fact a str (and I think your code will already do this). I'm sure there is some obscure thing you can do at runtime to a Callable[[str], bool] to make callable return False on it, but I feel pretty confident that you'd have to do things that are well beyond mypy's comprehension.

I feel like these examples involving subclassing int and str with a __call__ method are also kind of unreasonable (though clearly much less so). It wouldn't be out of the question for mypy to treat callable(x) as always false when x is an int. But I'm not sure callable is important enough to do something complex about it.

gvanrossum · 2017-01-02T02:36:17Z

I would defer to @JukkaL about this. That said, my own intuition is that the PR is valuable as-is (though I haven't reviewed it). While it's possible to subvert the types by e.g. subclassing str to add a __call__ method, I don't think that's a common scenario at all, while I believe that the scenario from the issue this is addressing (#1973) is common enough to want it to "just work" rather than forcing one to refactor the code just so it will type-check. (In fact I do think it's pretty similar to the "multiple inheritance" scenario that we're already ignoring in other cases.)

rwbarton · 2017-01-02T02:39:34Z

But at a minimum I would expect mypy to treat Any, Type[T], type, and types known to have a __call__ method as callable, rather than definitely not callable.

gvanrossum · 2017-01-02T02:42:30Z

Oh does the PR not implement that part? (Though `Any` should be treated as "perhaps callable perhaps not" I suppose.)

rwbarton · 2017-01-02T02:45:41Z

(Though Any should be treated as "perhaps callable perhaps not" I suppose.)

Er yes, I changed what I was going to say by the time I got to the end of that sentence. :)

I only read the code, I didn't read the tests or test it myself; but currently it appears to treat anything that isn't a Callable or a Union as not callable.

gvanrossum · 2017-01-02T04:02:35Z

Oh, that's definitely too narrow. Probably should look for some other piece of code that already checks for callable and see what cases it distinguishes. (Maybe the checking of a CallExpr?)

afrieder · 2017-01-02T23:57:13Z

Yes, the PR as-is only supports Callable and Unions of Callables. I'll take a look at how CallExpr handles things and improve this.

Edit: It's been improved and should now cover all cases.

gvanrossum · 2017-01-11T00:16:49Z

Status update: I discussed this with @JukkaL and he's in favor (though hasn't reviewed the PR in detail). I also ran tests on our internal codebases and found no problems. If I can wrap my head around the actual code I'll try to merge this ASAP so it will make it into the 0.4.7 release (planned for this Thursday).

afrieder · 2017-01-11T00:34:13Z

+        return all(is_callable_type(item) for item in type.items)
+
+    if isinstance(type, TypeVarType):
+        return not is_callable_type(type.erase_to_union_or_bound())


Rereading through this, I have no idea why this is negated. I'll remove it and test to confirm this case is right.

You seem to have no tests involving TypeVar. Also none for Type. So please add some!

gvanrossum · 2017-01-11T00:41:16Z

+        return all(is_callable_type(item) for item in type.items)
+
+    if isinstance(type, TypeVarType):
+        return not is_callable_type(type.erase_to_union_or_bound())


You seem to have no tests involving TypeVar. Also none for Type. So please add some!

gvanrossum · 2017-01-11T00:41:36Z

+def conditional_callable_type_map(expr: Expression,
+                                  current_type: Optional[Type],
+                                  ) -> Tuple[TypeMap, TypeMap]:
+    """Takes in an expression and the current type of the expression


Add period? Perhaps complete the sentence?

gvanrossum · 2017-01-11T00:42:33Z

+    """Takes in an expression and the current type of the expression
+
+    Returns a 2-tuple: The first element is a map from the expression to
+    the restricted type if it must be callable. The second element is a


"must be" feels odd. Perhaps make the phrasing more similar to the docstring of conditional_type_map()?

gvanrossum · 2017-01-11T01:56:26Z

+        uncallable_map = {expr: UnionType.make_union(uncallables)} if len(uncallables) else None
+        return callable_map, uncallable_map
+
+    if is_callable_type(current_type):


This is also missing a case for TypeVar. E.g. this example doesn't work right even if I remove the not from line 2520 above:

T = TypeVar('T', bound=Union[int, Callable[[], int]]) def f(a: T) -> int: if callable(a): reveal_type(a) return a() else: reveal_type(a) return a

The output is

__tmp__.py:8: error: Revealed type is 'T`-1' __tmp__.py:9: error: Incompatible return value type (got "T", expected "int")

gvanrossum · 2017-01-11T02:00:44Z

+                uncallables.append(item)
+
+            else:
+                if is_callable_type(item):


There's a problem here: nested unions (e.g. Union[int, Union[str, float]]) aren't necessarily flattened, so if the inner union contains a callable and a non-callable, that item is binned with the uncallables -- which is wrong. Try e.g.

U = Union[int, Union[str, Callable[[], int]]] def f(a: U) -> int: if callable(a): reveal_type(a) return a() else: reveal_type(a) return a

vs.

U = Union[int, str, Callable[[], int]]

afrieder · 2017-01-11T03:43:37Z

@gvanrossum I rewrote the underlying logic to handle the cases you brought up (and hopefully all other cases) and added various tests.

gvanrossum · 2017-01-11T04:47:34Z

W00t! Thanks! Merging now...

ambv approved these changes Jan 1, 2017

View reviewed changes

afrieder added 2 commits January 3, 2017 21:07

Introduces conditional type inference based on callable

a985400

Add tests for callable type-checker

25c73d0

afrieder commented Jan 11, 2017

View reviewed changes

gvanrossum reviewed Jan 11, 2017

View reviewed changes

afrieder added 2 commits January 10, 2017 21:34

Rewrote callable checking to work properly

4dd7b90

Added more tests

de057d1

gvanrossum merged commit f8ff732 into python:master Jan 11, 2017

ddfisher mentioned this pull request Jan 19, 2017

Type of 'and' expressions may be too general #2716

Closed

msullivan mentioned this pull request Dec 6, 2017

Calling callable in an if-condition makes mypy think branch in unreachable #3605

Closed

JelleZijlstra mentioned this pull request Feb 20, 2021

inspect.isfunction and inspect.ismethod #10117

Closed

		@@ -0,0 +1,24 @@
		from typing import builtinclass, Tuple, TypeVar, Generic, Union

Conversation

afrieder commented Jan 1, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ambv 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!

ambv commented Jan 1, 2017

Uh oh!

afrieder commented Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rwbarton commented Jan 2, 2017

Uh oh!

afrieder commented Jan 2, 2017

Uh oh!

rwbarton commented Jan 2, 2017

Uh oh!

gvanrossum commented Jan 2, 2017

Uh oh!

rwbarton commented Jan 2, 2017

Uh oh!

gvanrossum commented Jan 2, 2017 via email

Uh oh!

rwbarton commented Jan 2, 2017

Uh oh!

gvanrossum commented Jan 2, 2017 via email

Uh oh!

afrieder commented Jan 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gvanrossum commented Jan 11, 2017

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!

Choose a reason for hiding this comment

Uh oh!

afrieder commented Jan 11, 2017

Uh oh!

gvanrossum commented Jan 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

afrieder commented Jan 1, 2017 •

edited

Loading

afrieder commented Jan 2, 2017 •

edited

Loading

afrieder commented Jan 2, 2017 •

edited

Loading