Optional rule suggestion: ban implicit truthy/falsey type conversion in truth value testing

[kkom]

I've started a discussion on this on Ruff's GitHub, but I'm also curious if this is something that Pyright might consider implementing.

I'd like to ban implicit truthy/falsy checks in our codebase, to avoid common bugs like this:

def maybe_int() -> int | None:
    """returns None if data is missing, otherwise the value"""
    return 0

x = maybe_int()

if not x:
    return "early return because we're missing data"

# we now have a bug, because `0` is a perfectly fine value, but it was discarded in the early return above

process_data(x)

More details and broader rationale in this discussion: astral-sh/ruff#8757

@erictraut – what's you view on this? Does this feel like a rule Pyright might implement? (I would imagine it would be optional even in strict mode.)

On one hand it feels opinionated enough to be a linter rule, on the other hand I have a hunch it might be easier and more robust to implement in a type checker (feels a bit similar to e.g. reportUnnecessaryComparison).

[erictraut]

This pattern has legitimate uses and is quite common in Python, so it's not a rule that could be enabled by default, even in strict mode. In general, I don't like adding rules that cannot be enabled by default in strict mode because they are difficult for pyright users to discover and therefore are rarely used. This situation is also not a type violation, which means that it's not obvious that it's a rule a type checker should be enforcing.

There is a precedent for pyright supporting rules that are not type violations but do require type analysis (and are therefore difficult for linters to implement). These rules all involve cases where 1) there's a relatively low chance of a false positive and 2) a reasonably high likelihood of an actual bug. The reportUnnecessaryComparison rule is a good example of such a rule.

This particular check doesn't seem to meet this bar. I'm not convinced that it represents a sufficiently common source of bugs to merit adding such a rule, but I could perhaps be persuaded with some additional evidence.

For comparison, the typescript compiler (which is effectively a static type checker) does not provide an option like the one you're proposing, but eslint (which is more akin to a ruff) does.

[kkom]

Another common case is doing if x: when x is a not-None collection. In such a case writing if len(x) > 0: is also easier to read and less error-prone.

Somewhat analogously to the reportUnnecessaryComparison point above, explicit if len(x) > 0: has the advantage that the type checker highlights when it becomes possible that x could be None. This allows the programmer to properly consider that scenario, which might be to e.g. load a value missing in memory – rather than simply assume that the check should fail.

FWIW, I realise we're entering a bit of PEP 661 territory here. However, I wouldn't speculate about the outcome of that proposal. Today in Python None is commonly used as a special value/type to indicate missing data.

[npip99]

Today I just did a full audit of a codebase I maintain to check the impact of this rule:

---

(backend) npip99@npip99:~/Documents/programming/...$ find . -name '*.py' | xargs wc -l | tail -n1
   15418 total
(backend) npip99@npip99:~/Documents/programming/...$ find . -name '*.py' | xargs grep -E '^\s*if.*:$' | wc -l
   596

15k LOC, with 596 if statements.

---

With regexes and manual inspection, there were 340 irrelevant if statements (random boolean algebra), and 256 relevant if statements (None check, empty string, list non-empty). Of the 256 relevant if statements, here was the breakdown:

@kkom was right, None checks are indeed far more than 50%. They're actually 82.3% of all relevant if statements in my sample. I already invest a lot of mental effort to review every PR and check every if statement for None checks, and despite this, 7% of all None checks were PEP8 violations. Checking for PEP8 violations is crucial, 20% of all PEP8 violations were bugs. Here is the offending code:

The fact that empty strings are falsy, creates a bug in this situation. A benign bug, because empty strings are rarely useful. This code will end up throwing an HTTP 500 error in the empty string case, which still is not ideal when trying to build robust backend applications.

• The fact that empty strings are falsy is frustrating for me because it really has absolutely no benefit - out of 15,000 LOC, I only intentionally check for an empty string once. So, the ability to do if x: instead of if x != "": only saves 4 characters per 15k LOC, and even for that 1 line of code it's a negative impact because how it harms readability. Imo the idea that empty strings are falsy is always bad - an empty string check should always be done with an explicit != "", and if a line of code uses the fact that empty strings are falsy, then it has a virtually 100% chance of being a bug.

Arrays are more debatable; 16.4% of relevant if statements were len checks. Personally, I do if len(x) > 0:, because it's more readable. if x: would save 7 characters, or 294 characters total across the codebase. I don't consider the loss in readability to be worth it, and using the if x: shorthand would definitely be considered "not strict" programming imo.

• Also, if you accidentally write if x: where x is an Optional[list], then nothing will complain, even though it has a virtually 100% chance of being a bug. So, if len(x) > 0: is the only way to get the typechecker to correctly complain if len-checking an Optional variable.

---

Addendum:

PS: If such a rule were to be added, objects implementing the bool method should naturally be excluded from it.

This never occurred in my codebase, I'm not sure in what situation this would happen in. if bool(x): seems fine for those rare situations, it's always good practice to be explicit in rare situations imo. (For example, even the always-truthy __bool__ of a regex's match object, is still handled better with if x is not None:.)

[marcuslimdw]

My two cents: implicit boolean conversions have caused at least two bugs at my workplace that I know of in the last 6 months, and I would 100% use this rule.

[erictraut]

PS: If such a rule were to be added, objects implementing the bool method should naturally be excluded from it.

I'll note that the int and float classes implement the __bool__ method, so if such a check were to exempt classes that implement the __bool__ method, it wouldn't apply to some of the cases mentioned above. It would still apply to str, list, tuple and other instances of Sequence because they implement __len__ rather than __bool__.

[kkom]

Ah, thanks for pointing it out @erictraut!

That's unfortunate (because I think that practically implicit bool conversions for numbers are undesirable as well), but I think still better do what you say for the sake of consistency / soundness.

Seeing the rule fire in most situations would still build some awareness of dangerous conditionals in a codebase or team.

Edit: if x: would still be captured in the most common undesirable case where x: int | None, because None doesn't implement __bool__, right?

Edit 2: However, if you call would be to go even stricter, and ban even relying on an existing __bool__ method, I personally wouldn't be bothered by it! I agree with @npip99 that wrapping things with an explicit bool(...) would lead to clearer code in these scenarios.

I've changed my position here after a bit of research. It turns out that None also implements __bool__():

>>> None.__bool__()
False
>>> object().__bool__()
Traceback (most recent call last):
  File "<python-input-5>", line 1, in <module>
    object().__bool__()
    ^^^^^^^^^^^^^^^^^
AttributeError: 'object' object has no attribute '__bool__'. Did you mean: '__doc__'?

This means that neither of the examples below:

x: int

if x:
    ...

y: int | None

if y:
    ...

would be disallowed by the rule we're discussing here. And the second is in my experience a very common source of bugs.

I think then practically it would be desirable to lean towards strictness, and ban both the if __len__() > 0 and if __bool__() truthiness heuristics. On second though, that feels consistent as well – we're disregarding all heuristics, and require only explicit type matches.

As @npip99 pointed out, there is always the option to wrap a statement in bool(...) if someone desires type coercion to a boolean.

[nurpax]

I have a case where an API some functions return a direct bool and some functions tuple[bool, bool]. A user might write:

if some_func():  # return value of some_func is tuple[bool, bool]
    # ...

when they actually meant:

if some_func()[0]:
    # ...

It feels unfortunate that pyright doesn't seem to have an option to warn about "truthy tuples", esp. as the type of this tuple is statically determined and known to always evaluate to True.

Ruff has a rule for this (https://docs.astral.sh/ruff/rules/if-tuple/) but unfortunately it only works for literal values, as Ruff is not a type checker.

[shartzog]

I'd support an optional rule to flag this sort of usage, but I'd like to reiterate that a 'fast and loose association with the truth' has been a part of python since its inception (for good or ill), and it can be a perfectly legitimate shorthand. E.g. regex usage:

animal_pattern = re.compile(r"(?P<phrase>(?P<prefix>It's|This is an? )(?P<animal>\w+)!)")
animals: list[dict[str, str]] = []

phrase = "This is a walrus!"

if _animal_match := animal_pattern.match(phrase):  # seems legit...
    animals.append(_animal_match.groupdict())

if (_animal_match := animal_pattern.match(phrase)) is not None:  # feels clunky and unnecessary...
    animals.append(_animal_match.groupdict())

Strict container truthiness might also encourage the introduction of bugs from simple type system frustration. For example, a frustrating or cryptic flag might lead to bad decisions like omitting the my_list and from a conditional such as:

my_list: list[int]
if my_list and my_list[0] < 10:
    my_list[0] += 10

You'd want to be very explicit about why this rule exists and how to fix it in the error message.

[yamplum]

A lack of such a check could lead to silent introduction of bugs during refactoring.

Suppose I have the following code:

def try_frobnicate() -> bool:
    # ...

# elsewhere
success = try_frobnicate()

if success: # correct usage - success is a bool
    # ...

Then later I refactor it to provide more information:

def try_frobnicate() -> Result[Success, Error]: # Result is Union[Ok, Err]
    # ...

# elsewhere
success = try_frobnicate()

if success: # uh-oh, Err is truthy but no warning raised, hard to spot manually
    # ...

I think any codebase that takes type safety seriously will (or at least should) want to ban non-bool values from condition checks. I'm not sure if it is feasible at a linter level.

[kkom]

@erictraut – I remember you saying that you could revisit decisions to implement new rules if there is evidence of community demand (upvotes, comments). Could I ask what's your view on this rule now?

It's among the top open discussions at the moment. Especially if, in addition to "upvotes", you also count "thumbs up" – which this discussion seems to have more relatively to others.

Edit: I think a succinct, Pythonic justification for this rule could be that it prevents PEP 8 violations, which states:

beware of writing if x when you really mean if x is not None – e.g. when testing whether a variable or argument that defaults to None was set to some other value. The other value might have a type (such as a container) that could be false in a boolean context!

[Timmmm]

Just ran into a bug that this would have caught. I was refactoring some code and they had used an implicit bool coersion like this:

id = get_value_from_file()
if id:
    ...

During the refactoring I change the type of id from string | None to int. That caused no type errors at this bit of code, but it silently broke the case when id is 0.

I'm surprised this isn't a rule already. It's a pretty common footgun in my experience, and very bad style so nobody should be writing code like this anyway (so IMO it should be enabled by default in strict mode). It's also difficult for tools like Pylint to implement because it requires precise type knowledge.

Typescript has had this rule available for a long time.

[shartzog]

The problem in your case isn't necessarily the implicit __bool__ call. Would you still expect a typing error if your original code had been written as follows?

id = get_value_from_file()
if bool(id):
    ...

Long story short, this rule would more or less invalidate Python's __bool__ methodology entirely. Long time python devs are very aware of this pitfall and approach str --> int conversions with care, and many if not most consider the implicit case you've raised to be the preferred styling. IMHO, the biggest issue with your sample code is that it masks the builtin id function, not the implicit __bool__ call. 🤷

In any case, I definitely support an optional rule as stated earlier in this thread.

[Timmmm]

The problem in your case isn't necessarily the implicit bool call.

Yes it is. If implicit bools were not allowed then the author would likely have written if id is not None and id != 0:.

if bool(id):

I would not expect a typing error in that case, but I have also never seen anyone write this, whereas implicit bool coersion from str, int, None and list is an extremely common mistake.

Long time python devs are very aware of this pitfall and approach str --> int conversions with care

Indeed. I am not working with long-time Python devs. Also that is very much a "I don't need seatbelts; I'm a good driver" attitude.

many if not most consider the implicit case you've raised to be the preferred styling

Then they are misguided. Many (if not most) Python devs consider static typing to be a waste of time entirely.

it masks the builtin id function

It was just an example. The actual code doesn't use id.

[shartzog]

I would not expect a typing error in that case, but I have also never seen anyone write this

I've come across a fair bit of bool(str) and bool(list), but not in simple conditionals to your point. It's usually for type narrowing a function input or output when a type checker complains and usually in a functional(ish) programming context. Not saying it's "right" but cheap and dirty is occasionally the right call.

Also that is very much a "I don't need seatbelts; I'm a good driver" attitude.

Lol, fair point... Again, I'm in favor of this as an optional rule and would use it in some projects.

Then they are misguided. Many (if not most) Python devs consider static typing to be a waste of time entirely.

I think that's changing? But that could be because I adopted it years ago and now take it for granted. And I agree the if id: is not only misguided but just plain wrong in any use case where a future int conversion is an obvious possibility (e.g. when the original str value is already a numeric).