Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
ilinum opened this issue Jun 23, 2017 · 6 comments
Closed
Labels
bug mypy got something wrong priority-0-high

Comments

@ilinum
Copy link
Collaborator

ilinum commented Jun 23, 2017

In the following code, mypy thinking callable(o) always returns False. This might be related to #3603.

def f(o: object) -> None:
    if callable(o):
        # this branch is not typechecked
        o()
        1 + 'boom'  # no error from mypy
        o()

f(lambda: print('hello'))

Here is the output from mypy and python.

$ mypy n.py 
$ python3 n.py 
hello
Traceback (most recent call last):
  File "n.py", line 8, in <module>
    f(lambda: print('hello'))
  File "n.py", line 5, in f
    1 + 'boom'
TypeError: unsupported operand type(s) for +: 'int' and 'str'
@ilinum ilinum added the bug mypy got something wrong label Jun 23, 2017
@msullivan
Copy link
Collaborator

The key problem here is that the conditional type-checking based on callable (as introduced to fix #2627) is pretty fundamentally unsound.

We can't really ever conclude that something can't be callable, short of using whole-program information about the entire class hierarchy. Even in the example given in that bug of testing a type Union[str, Callable[[str], int]], there can be subclasses of str that are callable, so it isn't safe to conclude that callable being true implies the type is Callable[[str], int].

I'm not sure if there is anything sound that we can do in response to callable.

We can soundly assume that the type is Callable, but that has its own brand of unsoundness by introducing a bunch of Anys into play. That might still be the right thing; I think the only other option is to do nothing for callable.

@JukkaL
Copy link
Collaborator

JukkaL commented Dec 6, 2017

For constructs that we can't handle safely, we sometimes try to do something that avoids false positives and still does some useful type checking, even if it's not type safe. In the original example, inferring type Callable (that is, Callable[..., Any]) as the type after the callable check would be my pick. At least we would catch some unsafe code and wouldn't require a cast. Casts are best avoided if possible, since they aren't checked at runtime and often mask bugs.

In cases like Union[str, Callable[[str], int]] the reasoning could go like this:

def f(o: Union[str, Callable[[str], int]]) -> None:
    if callable(o):
        # Could be a subclass of 'str' that is callable or Callable[[str], int].
        # We don't know the precise type in the first case, so we could fall
        # back to Callable.
        #
        # We could then infer type Union[Callable[[str], int], Callable].
        ...
    else:
        # Can only be 'str' here.
        ...

However, elsewhere we sometimes make unsafe assumptions about inheritance. For example, we sometimes assume that there is no multiple inheritance from some classes A and B, even though we cannot be sure that it's the case. In analogy it would be reasonable in the latter example to infer just Callable[[str], int] as the type of o after the callable check and assume that no str subclass is callable.

Eventually I'd like to have optional strictness flags that reject this kind of type unsafety at the cost of some extra required refactoring. I don't think that this is urgent right now, however. There still tends to be a lot of Any types in large type checked programs due to missing or incompletely typed typeshed stubs (and various other reasons). Edge cases such as the ones discussed here only add a very little extra unsafety in practice. My experience suggests that users dislike having to refactor their existing code, so introducing some unsafety is occasionally okay -- but these are often very delicate design decisions.

@ilevkivskyi
Copy link
Member

For example, we sometimes assume that there is no multiple inheritance from some classes A and B, even though we cannot be sure that it's the case.

It looks like Intersection may be a more long-term solution for such cases and also for callable(). It doesn't have many use cases for a user facing API, but IIRC there are lots of corner cases that will be fixed by having internal intersection types. I am sure this was already proposed somewhere, but can't remember where.

@ilevkivskyi
Copy link
Member

OK, found it #3603 (comment)

@msullivan
Copy link
Collaborator

I started hacking up the beginnings of a fix to this based on assuming the type of o is a Callable as in the example Jukka gave.

My naive implementation of this didn't work for what I think turns out is actually a pretty fundamental reason: the type maps returned by find_isinstance_check are restrictions that are used to narrow_declared_type, which computes the meet of object and Callable, which is None.

I had had it in my head that I wanted to just override the previous type with Callable, which would work fine for object but probably be the wrong thing for more specific types.

So I think that gaining any useful information from callable being true is going to require some form of intersection? I think that if it was only callable we cared about, a super special cased version of the proposal in #3603 that just made up a new TypeInfo that added a __call__ method would probably suffice.

@gvanrossum
Copy link
Member

gvanrossum commented Dec 8, 2017 via email

msullivan added a commit that referenced this issue Dec 12, 2017
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
msullivan added a commit that referenced this issue Dec 15, 2017
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
msullivan added a commit that referenced this issue Dec 18, 2017
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
msullivan added a commit that referenced this issue Dec 19, 2017
Don't assume that something is *not* callable except when operating
on Union types.
Normally, 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. When operating on Union types, continue treating
types that are not clearly callable as if they cannot be callable,
so type discrimination using callable() behaves as expected.

Fixes #3605
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug mypy got something wrong priority-0-high
Projects
None yet
Development

No branches or pull requests

5 participants