Fail when conditioning on implicit bool

[ikonst]

Description

Fails conditionals where the expression's type does not implement __bool__ nor __len__ since such expressions would always evaluate as truthy and likely an error (e.g. the common pitfall of conditioning on a callable rather than its result).

This feature was proposed by @pkch in #3195 (comment) as an alternative to the deprecated --strict-bool.

Test Plan

Test case was added.

[ikonst]

I'm not sure whether "implicit-bool" should be the code, and whether "implicit conversion" captures what's happening here. Perhaps there's some accepted terminology for this in cpython?

[hauntsaninja]

The Python docs seem to use the term "boolean operations"

[ikonst]

"context of Boolean operation" is also used

[ikonst]

Too many fixtures would need multiple changes to add __len__ to list, __bool__ to int, etc. and I understand that making fixtures more "complete" across the board is discouraged (to keep tests lean).

[hauntsaninja]

Looks reasonable. This is more #8646 than pkch's comment, since e.g. you don't warn about x: Optional[int]; if x: .... I'd also check for __bool__ that returns a Literal, a la #9297 (although maybe complicated, e.g. for Unions you'd need to check that all members aren't returning the same Literal)

[hauntsaninja]

The Python docs seem to use the term "boolean operations"

[hauntsaninja]

nit:

Suggested change

	'Neither of {} implements __bool__ or __len__ '\
	'No members of {} implement __bool__ or __len__ '\

[ikonst]

I'd also check for bool that returns a Literal

So, this opens the door for a more generic always-true/false check, and this was actually my first approach - to leverage the can_be_true and can_be_false attributes, i.e.

if t.can_be_true and not t.can_be_false:
  self.fail('expression is always true',  ...)
elif not t.can_be_true and t.can_be_false:
  self.fail('expression is always false',  ...)

For this to detect the "implicit" case, I had to teach Instance to flag it:

class Instance(ProperType):
    ...
    def __init__(self, ...):
       ...
       if self.type and not self.type.has_readable_member('__bool__') and \
               not self.type.has_readable_member('__len__'):
           self.can_be_true = True
           self.can_be_false = False

This has the benefit of handling many of the details (literals, unions). However:

1. We'd need to explicitly allow patterns which are obviously intentional like while True. Can you think of other such patterns? Should if True be an error?
2. The current code propagates the can_be_true and can_be_false to the expression's root. To leverage it for errors, we'd need more context to produce different messages for different scenarios, e.g. instead of "expression is always true", have it say "literal value 42 is always true" or "type Foobar has no __bool__ or __len__". For this to happen, perhaps I shouldn't handle it in the "if" and "conditional" visitors, but rather in the code that evaluates the expressions, so that I could flag it right in, say, 42 or get_data() (regardless of whether you end up doing if 42 or get_data():).

Initially I hesitated trying to make a generic "always-true/false" checker simply because I felt the error wouldn't be as useful to flag a very common pitfall of using an object as a boolean.

[ikonst]

I'll try a different approach.

[Akuli]

In the past, I have used if True: when a linter complains about something unless it's hidden in a conditional (lol), and if False: to disable code. But I think that's it: if the condition is always true or always false, and the condition is not literally True or False, then it should be an error.

Mypy already has a flag that makes it warn about code that never runs. If the scope of this PR grows, maybe consider how it interacts with that?

[ikonst]

In the past, I have used if True: when a linter complains about something ...
Mypy already has a flag that makes it warn about code that never runs.

Yes, and there's special treatment for "always true" and "always false" that's established during semantic analysis, e.g. so that if TYPE_CHECKING and if MYPY wouldn't result in errors. True and False literals, however, aren't handled there (unless you pass e.g. --always-true True). We would need to see how we should handle patterns like those.

Right now there's no special handling, so e.g.

if False:
  print('x') 

would result in:

$ mypy --warn-unreachable test.py
test.py:2: error: Statement is unreachable

[Akuli]

Perhaps it's good that mypy warns about if False:, as it's something used for temporarily disabling something, and you shouldn't leave it in when cleaning up the code.

[ikonst]

Merged #10666 instead since it had a cooler number.