Mypy: Reject method/function object used as a condition

[JukkaL]

Mypy could reject this code, which is almost always bad and a reasonably common sort of bug:

def name() -> str: ...

if name:  # Probably meant name()?
    do_something()

(A related issue is if name == 'foo' but there is a separate issue for that already.)

[gvanrossum]

Is that really common? It sounds like more of a linter feature.

[JukkaL]

The example was overly simplistic. This is more realistic, and here we need type information:

class A:
    def name(self) -> Optional[str]: ...

def f(a: A) -> None:
    if a.name:  # Probably meant a.name()?
        do_something(a.name())

I've been bitten by this and I've heard similar bugs mentioned by other people. It's probably not a very big thing, but it should be pretty easy to catch these.

[rowillia]

@JukkaL @gvanrossum It seems to be that this is related to https://github.com/python/mypy/commit/9a8a8c098a592cccc87473019b29f56a5db9c7cb ... The boolean value of a.name in @JukkaL's example above is true_only

Where this will be _super_ useful is with the new async/await support. In Hack-land, we'd _always_ see bugs of the form:

async def is_foo_active() -> bool:
    return await some_database_call_about_foo()

async def do_a_thing() -> None:
    if is_foo_active(): #  BUG!  Engineer forgot to await so this is always truthy!!!!!
        do_foo()
    else:
        do_bar() # Dead code

Hack now throws an error if you branch on something always truth-y to solve this problem.

[ddfisher]

More generally, we could warn about dead code?

[JukkaL]

This doesn't necessarily result in dead code, but sometimes it does. An always-true condition in an if/while statement could be flagged as a potential error, but not always. while True: and if 1: should likely be okay, as it's quite clear that this is what the programmer intended.

I think that a reasonable goal would be to reject code that _looks_ to the reader like it might not be truthy, when we actually know that it will always be truthy.

[elazarg]

I think that such errors are very common, especially when turning a method into a property, and it would be most useful.

[rvolgers]

I recently typed if user.is_admin: instead of if user.is_admin(): and was pretty disappointed none of the linters was able to catch this. From where I'm standing this looks like a pretty important feature.

Btw before anyone says this should have been a property, there are other similar methods that take parameters so it's trying to be consistent.

[ilevkivskyi]

OK, I am raising priority to normal.

[JelleZijlstra]

A possible more general approach would be to give an error if an object is used in a boolean context, but doesn't override object's __bool__ method, which means it will always appear as True. I don't know if that would cause a lot of false positives though.

[elazarg]

You mean if the type does not have any possible instance that overrides object's __bool__(). There aren't many final types in Python.

I think handling (unions of) functions, specifically, is the way to go.

[ilevkivskyi]

An important corner case to consider is:

def run(cb: Optional[Callable[[], None]]) -> None:
    ...
    if cb:
        cb()

I think we should always allow such code.

[JukkaL]

I think that it would be okay to flag an error if a condition is a direct reference to a module-level function or method. Anything else we may want to allow. In particular, variables with an explicit Callable[...] type might have a custom __bool__ (they could be instances or user-defined classes), so perhaps we should always allow them to be used in conditions.