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

Fail when conditioning on implicit bool #10557

Closed
wants to merge 2 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions docs/source/error_code_list.rst
Original file line number Diff line number Diff line change
Expand Up @@ -665,6 +665,24 @@ consistently when using the call-based syntax. Example:
# Error: First argument to namedtuple() should be "Point2D", not "Point"
Point2D = NamedTuple("Point", [("x", int), ("y", int)])



Implicitly converting to bool always evaluates as True [implicit-bool]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"context of Boolean operation" is also used

----------------------------------------------------------------------

In contexts where an expression has to be converted to a bool, an object
which does not implement __bool__ nor __len__ will always evaluate to True.

.. code-block:: python

class Foo:
pass

foo = Foo()
# Error: "{}" does not implement __bool__ or __len__ so the expression is always truthy
if foo:
...

Report syntax errors [syntax]
-----------------------------

Expand Down
18 changes: 17 additions & 1 deletion mypy/checker.py
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@
try_getting_str_literals_from_type, try_getting_int_literals_from_type,
tuple_fallback, is_singleton_type, try_expanding_enum_to_union,
true_only, false_only, function_type, get_type_vars, custom_special_method,
is_literal_type_like,
is_literal_type_like, type_has_explicit_conversion_to_bool,
)
from mypy import message_registry
from mypy.subtypes import (
Expand Down Expand Up @@ -3240,6 +3240,14 @@ def check_return_stmt(self, s: ReturnStmt) -> None:
if self.in_checked_function():
self.fail(message_registry.RETURN_VALUE_EXPECTED, s, code=codes.RETURN_VALUE)

def _type_has_explicit_conversion_to_bool(self, t: Type) -> bool:
if isinstance(t, Instance):
return t.type.has_readable_member('__bool__') or t.type.has_readable_member('__len__')
elif isinstance(t, FunctionLike):
return False
else:
return True

def visit_if_stmt(self, s: IfStmt) -> None:
"""Type check an if statement."""
# This frame records the knowledge from previous if/elif clauses not being taken.
Expand All @@ -3251,6 +3259,14 @@ def visit_if_stmt(self, s: IfStmt) -> None:
if isinstance(t, DeletedType):
self.msg.deleted_as_rvalue(t, s)

if codes.IMPLICIT_BOOL in self.options.enabled_error_codes:
if isinstance(t, (Instance, FunctionLike)):
if not type_has_explicit_conversion_to_bool(t):
self.msg.type_converts_to_bool_implicitly(t, e)
elif isinstance(t, UnionType):
if not any(type_has_explicit_conversion_to_bool(item) for item in t.items):
self.msg.type_union_converts_to_bool_implicitly(t, e)

if_map, else_map = self.find_isinstance_check(e)

# XXX Issue a warning if condition is always False?
Expand Down
15 changes: 13 additions & 2 deletions mypy/checkexpr.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
from mypy.typeops import (
tuple_fallback, make_simplified_union, true_only, false_only, erase_to_union_or_bound,
function_type, callable_type, try_getting_str_literals, custom_special_method,
is_literal_type_like,
is_literal_type_like, type_has_explicit_conversion_to_bool,
)
import mypy.errorcodes as codes

Expand Down Expand Up @@ -3819,7 +3819,7 @@ def check_for_comp(self, e: Union[GeneratorExpr, DictionaryComprehension]) -> No
self.msg.redundant_condition_in_comprehension(True, condition)

def visit_conditional_expr(self, e: ConditionalExpr, allow_none_return: bool = False) -> Type:
self.accept(e.cond)
cond_type = self.accept(e.cond)
ctx = self.type_context[-1]

# Gain type information from isinstance if it is there
Expand All @@ -3831,6 +3831,17 @@ def visit_conditional_expr(self, e: ConditionalExpr, allow_none_return: bool = F
elif else_map is None:
self.msg.redundant_condition_in_if(True, e.cond)

if codes.IMPLICIT_BOOL in self.chk.options.enabled_error_codes:
cond_proper_type = get_proper_type(cond_type)

if isinstance(cond_proper_type, (Instance, FunctionLike)):
if not type_has_explicit_conversion_to_bool(cond_proper_type):
self.msg.type_converts_to_bool_implicitly(cond_proper_type, e.cond)
elif isinstance(cond_proper_type, UnionType):
if not any(type_has_explicit_conversion_to_bool(item)
for item in cond_proper_type.items):
self.msg.type_union_converts_to_bool_implicitly(cond_proper_type, e.cond)

if_type = self.analyze_cond_branch(if_map, e.if_expr, context=ctx,
allow_none_return=allow_none_return)

Expand Down
7 changes: 5 additions & 2 deletions mypy/errorcodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,11 @@ def __str__(self) -> str:
'General') # type: Final
EXIT_RETURN = ErrorCode(
'exit-return', "Warn about too general return type for '__exit__'", 'General') # type: Final
NAME_MATCH = ErrorCode(
'name-match', "Check that type definition has consistent naming", 'General') # type: Final
IMPLICIT_BOOL = ErrorCode(
'implicit-bool', "Implicitly converting to bool always evaluates as True",
'General') # type: Final

# These error codes aren't enabled by default.
NO_UNTYPED_DEF = ErrorCode(
Expand All @@ -117,8 +122,6 @@ def __str__(self) -> str:
"Warn about redundant expressions",
'General',
default_enabled=False) # type: Final
NAME_MATCH = ErrorCode(
'name-match', "Check that type definition has consistent naming", 'General') # type: Final


# Syntax errors are often blocking.
Expand Down
5 changes: 5 additions & 0 deletions mypy/message_registry.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,11 @@
DESCRIPTOR_SET_NOT_CALLABLE = "{}.__set__ is not callable" # type: Final
DESCRIPTOR_GET_NOT_CALLABLE = "{}.__get__ is not callable" # type: Final
MODULE_LEVEL_GETATTRIBUTE = '__getattribute__ is not valid at the module level' # type: Final
TYPE_CONVERTS_TO_BOOL_IMPLICITLY = \
'"{}" does not implement __bool__ or __len__ so the expression is always truthy' # type: Final
ALL_TYPES_IN_UNION_CONVERT_TO_BOOL_IMPLICITLY = \
'Neither of {} implements __bool__ or __len__ '\
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

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

'so the expression is always truthy' # type: Final

# Generic
GENERIC_INSTANCE_VAR_CLASS_ACCESS = \
Expand Down
8 changes: 8 additions & 0 deletions mypy/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,14 @@ def deleted_as_lvalue(self, typ: DeletedType, context: Context) -> None:
s = ' "{}"'.format(typ.source)
self.fail('Assignment to variable{} outside except: block'.format(s), context)

def type_converts_to_bool_implicitly(self, typ: Type, context: Context) -> str:
return self.fail(message_registry.TYPE_CONVERTS_TO_BOOL_IMPLICITLY
.format(typ), context, code=codes.IMPLICIT_BOOL)

def type_union_converts_to_bool_implicitly(self, typ: UnionType, context: Context) -> strg:
return self.fail(message_registry.ALL_TYPES_IN_UNION_CONVERT_TO_BOOL_IMPLICITLY
.format(typ), context, code=codes.IMPLICIT_BOOL)

def no_variant_matches_arguments(self,
plausible_targets: List[CallableType],
overload: Overloaded,
Expand Down
7 changes: 6 additions & 1 deletion mypy/test/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from typing import List, Iterable, Dict, Tuple, Callable, Any, Optional, Iterator

from mypy import defaults
from mypy import errorcodes as codes
import mypy.api as api

import pytest
Expand Down Expand Up @@ -372,16 +373,19 @@ def assert_type(typ: type, value: object) -> None:
def parse_options(program_text: str, testcase: DataDrivenTestCase,
incremental_step: int) -> Options:
"""Parse comments like '# flags: --foo' in a test case."""
options = Options()
flags = re.search('# flags: (.*)$', program_text, flags=re.MULTILINE)
if incremental_step > 1:
flags2 = re.search('# flags{}: (.*)$'.format(incremental_step), program_text,
flags=re.MULTILINE)
if flags2:
flags = flags2

disabled_error_codes = {codes.IMPLICIT_BOOL}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


if flags:
flag_list = flags.group(1).split()
for code in disabled_error_codes:
flag_list.extend(['--disable-error-code', code.code])
flag_list.append('--no-site-packages') # the tests shouldn't need an installed Python
targets, options = process_options(flag_list, require_targets=False)
if targets:
Expand All @@ -393,6 +397,7 @@ def parse_options(program_text: str, testcase: DataDrivenTestCase,
# TODO: Enable strict optional in test cases by default (requires *many* test case changes)
options.strict_optional = False
options.error_summary = False
options.disabled_error_codes = disabled_error_codes

# Allow custom python version to override testcase_pyversion.
if all(flag.split('=')[0] not in ['--python-version', '-2', '--py2'] for flag in flag_list):
Expand Down
9 changes: 9 additions & 0 deletions mypy/typeops.py
Original file line number Diff line number Diff line change
Expand Up @@ -642,6 +642,15 @@ def get_enum_values(typ: Instance) -> List[str]:
return [name for name, sym in typ.type.names.items() if isinstance(sym.node, Var)]


def type_has_explicit_conversion_to_bool(typ: Type) -> bool:
if isinstance(typ, Instance):
return typ.type.has_readable_member('__bool__') or typ.type.has_readable_member('__len__')
elif isinstance(typ, FunctionLike):
return False
else:
return True


def is_singleton_type(typ: Type) -> bool:
"""Returns 'true' if this type is a "singleton type" -- if there exists
exactly only one runtime value associated with this type.
Expand Down
58 changes: 58 additions & 0 deletions test-data/unit/check-errorcodes.test
Original file line number Diff line number Diff line change
Expand Up @@ -807,3 +807,61 @@ from typing_extensions import TypedDict

Foo = TypedDict("Bar", {}) # E: First argument "Bar" to TypedDict() does not match variable name "Foo" [name-match]
[builtins fixtures/dict.pyi]
[case testImplicitBool]
# flags: --enable-error-code implicit-bool
from typing import List, Union

class Foo:
pass


foo = Foo()
if foo: # E: "__main__.Foo" does not implement __bool__ or __len__ so the expression is always truthy [implicit-bool]
pass

zero = 0
if zero:
pass


false = False
if false:
pass


null = None
if null:
pass


s = ''
if s:
pass


good_union: Union[str, int] = 5
if good_union:
pass


bad_union: Union[Foo, object] = Foo()
if bad_union: # E: Neither of Union[__main__.Foo, builtins.object] implements __bool__ or __len__ so the expression is always truthy [implicit-bool]
pass


def f():
pass


if f: # E: "def () -> Any" does not implement __bool__ or __len__ so the expression is always truthy [implicit-bool]
pass


lst: List[int] = []
if lst:
pass


conditional_result = 'foo' if f else 'bar' # E: "def () -> Any" does not implement __bool__ or __len__ so the expression is always truthy [implicit-bool]

[builtins fixtures/implicit_bool.pyi]
16 changes: 16 additions & 0 deletions test-data/unit/fixtures/implicit_bool.pyi
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
from typing import Generic, Sequence, TypeVar

_T = TypeVar('_T')


class object:
def __init__(self): pass

class type: pass
class int:
def __bool__(self) -> bool: pass
class bool(int): pass
class list(Generic[_T], Sequence[_T]):
def __len__(self) -> int: pass
class str:
def __len__(self) -> int: pass