From e96ce45d5727eed08820a637f293ce0f646bba62 Mon Sep 17 00:00:00 2001 From: Ilya Konstantinov Date: Sun, 30 May 2021 01:46:38 -0400 Subject: [PATCH 1/2] Fail when conditioning on implicit bool --- docs/source/error_code_list.rst | 18 +++++++ mypy/checker.py | 18 ++++++- mypy/checkexpr.py | 15 +++++- mypy/errorcodes.py | 7 ++- mypy/message_registry.py | 5 ++ mypy/messages.py | 8 ++++ mypy/test/helpers.py | 7 ++- mypy/typeops.py | 9 ++++ test-data/unit/check-errorcodes.test | 58 +++++++++++++++++++++++ test-data/unit/fixtures/implicit_bool.pyi | 16 +++++++ 10 files changed, 155 insertions(+), 6 deletions(-) create mode 100644 test-data/unit/fixtures/implicit_bool.pyi diff --git a/docs/source/error_code_list.rst b/docs/source/error_code_list.rst index 3afde02e3ee68..4eab15e6c045e 100644 --- a/docs/source/error_code_list.rst +++ b/docs/source/error_code_list.rst @@ -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] +---------------------------------------------------------------------- + +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] ----------------------------- diff --git a/mypy/checker.py b/mypy/checker.py index dde8a3441c35d..4262dcd801255 100644 --- a/mypy/checker.py +++ b/mypy/checker.py @@ -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 ( @@ -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. @@ -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? diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py index 3bfe0a306446f..735690cf54016 100644 --- a/mypy/checkexpr.py +++ b/mypy/checkexpr.py @@ -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 @@ -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 @@ -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) diff --git a/mypy/errorcodes.py b/mypy/errorcodes.py index 01b946c467477..54459fe768e5c 100644 --- a/mypy/errorcodes.py +++ b/mypy/errorcodes.py @@ -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( @@ -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. diff --git a/mypy/message_registry.py b/mypy/message_registry.py index b434acf943085..59cd961b63bcd 100644 --- a/mypy/message_registry.py +++ b/mypy/message_registry.py @@ -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__ '\ + 'so the expression is always truthy' # type: Final # Generic GENERIC_INSTANCE_VAR_CLASS_ACCESS = \ diff --git a/mypy/messages.py b/mypy/messages.py index 0d10b9bef5d6e..6f751dd7ffc68 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -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): + 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): + 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, diff --git a/mypy/test/helpers.py b/mypy/test/helpers.py index 077c2f369cda3..e100dc70d9707 100644 --- a/mypy/test/helpers.py +++ b/mypy/test/helpers.py @@ -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 @@ -372,7 +373,6 @@ 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, @@ -380,8 +380,12 @@ def parse_options(program_text: str, testcase: DataDrivenTestCase, if flags2: flags = flags2 + disabled_error_codes = {codes.IMPLICIT_BOOL} + 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: @@ -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): diff --git a/mypy/typeops.py b/mypy/typeops.py index 56a6002d1e406..f1d41805b0ce5 100644 --- a/mypy/typeops.py +++ b/mypy/typeops.py @@ -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. diff --git a/test-data/unit/check-errorcodes.test b/test-data/unit/check-errorcodes.test index 7479b15b7bc4d..11b7701e931f1 100644 --- a/test-data/unit/check-errorcodes.test +++ b/test-data/unit/check-errorcodes.test @@ -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] diff --git a/test-data/unit/fixtures/implicit_bool.pyi b/test-data/unit/fixtures/implicit_bool.pyi new file mode 100644 index 0000000000000..40d97b8773f27 --- /dev/null +++ b/test-data/unit/fixtures/implicit_bool.pyi @@ -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 From 2280b7e238fe31900dec23dbd7e619cc6eb22f46 Mon Sep 17 00:00:00 2001 From: Ilya Konstantinov Date: Sun, 30 May 2021 03:21:50 -0400 Subject: [PATCH 2/2] add missing type annotation --- mypy/messages.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mypy/messages.py b/mypy/messages.py index 6f751dd7ffc68..9e9c2a15d77f9 100644 --- a/mypy/messages.py +++ b/mypy/messages.py @@ -718,11 +718,11 @@ 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): + 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): + 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)