Skip to content
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

Reject method/function object used as a condition #2073

Closed
JukkaL opened this issue Aug 31, 2016 · 13 comments
Closed

Reject method/function object used as a condition #2073

JukkaL opened this issue Aug 31, 2016 · 13 comments

Comments

@JukkaL
Copy link
Collaborator

JukkaL commented Aug 31, 2016

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
Copy link
Member

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

@gvanrossum gvanrossum added this to the Future milestone Sep 1, 2016
@JukkaL
Copy link
Collaborator Author

JukkaL commented Sep 2, 2016

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
Copy link
Contributor

rowillia commented Sep 2, 2016

@JukkaL @gvanrossum It seems to be that this is related to 9a8a8c0 ... 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
Copy link
Collaborator

ddfisher commented Sep 2, 2016

More generally, we could warn about dead code?

@JukkaL
Copy link
Collaborator Author

JukkaL commented Sep 5, 2016

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
Copy link
Contributor

elazarg commented Sep 11, 2016

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

@gvanrossum gvanrossum removed this from the Future milestone Mar 29, 2017
@rvolgers
Copy link

rvolgers commented Aug 17, 2018

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
Copy link
Member

OK, I am raising priority to normal.

@JelleZijlstra
Copy link
Member

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
Copy link
Contributor

elazarg commented Aug 17, 2018

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
Copy link
Member

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
Copy link
Collaborator Author

JukkaL commented Feb 12, 2019

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.

@huonw
Copy link

huonw commented Oct 7, 2024

It seems like this may be resolved now, via the truthy-function error code: https://mypy-play.net/?mypy=latest&python=3.12&gist=067efa3ee1e7617950d94c90cc60116e

from typing import Optional, Callable

def required(cb: Callable[[], None]) -> None:
    if cb: # error: Function "cb" could always be true in boolean context  [truthy-function]
        pass
    
def optional(cb: Optional[Callable[[], None]]) -> None:
    if cb: # no error
        pass

The missing-await issue above seems better covered by the (much newer, but more specific) #16069

Maybe this one can be closed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants