8000 Distinguish redundant-expr warnings from unreachable warnings by llchan · Pull Request #9125 · python/mypy · GitHub
[go: up one dir, main page]

Skip to content

Distinguish redundant-expr warnings from unreachable warnings #9125

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

Merged
merged 7 commits into from
Sep 24, 2020
Merged
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
21 changes: 21 additions & 0 deletions docs/source/error_code_list2.rst
Original file line number Diff line number Diff line change
Expand Up @@ -193,3 +193,24 @@ incorrect control flow or conditional checks that are accidentally always true o
return
# Error: Statement is unreachable [unreachable]
print('unreachable')

Check that expression is redundant [redundant-expr]
---------------------------------------------------

If you use :option:`--enable-error-code redundant-expr <mypy --enable-error-code>`,
mypy generates an error if it thinks that an expression is redundant.

.. code-block:: python

# mypy: enable-error-code redundant-expr

def example(x: int) -> None:
# Error: Left operand of 'and' is always true [redundant-expr]
if isinstance(x, int) and x > 0:
pass

# Error: If condition is always true [redundant-expr]
1 if isinstance(x, int) else 0

# Error: If condition in comprehension is always true [redundant-expr]
[i for i in range(x) if isinstance(i, int)]
19 changes: 14 additions & 5 deletions mypy/checkexpr.py
Original file line number Diff line number Diff line change
Expand Up @@ -2740,6 +2740,17 @@ def check_boolean_op(self, e: OpExpr, context: Context) -> Type:
restricted_left_type = true_only(left_type)
result_is_left = not left_type.can_be_false

# If left_map is None then we know mypy considers the left expression
# to be redundant.
#
# Note that we perform these checks *before* we take into account
# the analysis from the semanal phase below. We assume that nodes
# marked as unreachable during semantic analysis were done so intentionally.
# So, we shouldn't report an error.
Copy link
Member

Choose a reason for hiding this comment

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

Do you understand what this comment refers to? I wonder if the two nearly-identical comment blocks shouldn’t be refactored (the second could just say “similar as above comment”).

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 don't know what exactly that comment means. When I split the conditions into two, I erred on the side of "explicit is better than implicit" and made sure the comment was copied to both places, so that a future change doesnt inadvertently lead to a dangling comment reference.

if codes.REDUNDANT_EXPR in self.chk.options.enabled_error_codes:
if left_map is None:
self.msg.redundant_left_operand(e.op, e.left)

# If right_map is None then we know mypy considers the right branch
# to be unreachable and therefore any errors found in the right branch
# should be suppressed.
Expand All @@ -2749,10 +2760,8 @@ def check_boolean_op(self, e: OpExpr, context: Context) -> Type:
# marked as unreachable during semantic analysis were done so intentionally.
# So, we shouldn't report an error.
if self.chk.options.warn_unreachable:
if left_map is None:
self.msg.redundant_left_operand(e.op, e.left)
if right_map is None:
self.msg.redundant_right_operand(e.op, e.right)
self.msg.unreachable_right_operand(e.op, e.right)

if e.right_unreachable:
right_map = None
Expand Down Expand Up @@ -3673,7 +3682,7 @@ def check_for_comp(self, e: Union[GeneratorExpr, DictionaryComprehension]) -> No
for var, type in true_map.items():
self.chk.binder.put(var, type)

if self.chk.options.warn_unreachable:
if codes.REDUNDANT_EXPR in self.chk.options.enabled_error_codes:
if true_map is None:
self.msg.redundant_condition_in_comprehension(False, condition)
elif false_map is None:
Expand All @@ -3686,7 +3695,7 @@ def visit_conditional_expr(self, e: ConditionalExpr, allow_none_return: bool = F
# Gain type information from isinstance if it is there
# but only for the current expression
if_map, else_map = self.chk.find_isinstance_check(e.cond)
if self.chk.options.warn_unreachable:
if codes.REDUNDANT_EXPR in self.chk.options.enabled_error_codes:
if if_map is None:
self.msg.redundant_condition_in_if(False, e.cond)
elif else_map is None:
Expand Down
5 changes: 5 additions & 0 deletions mypy/errorcodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@ def __str__(self) -> str:
'General') # type: Final
UNREACHABLE = ErrorCode(
'unreachable', "Warn about unreachable statements or expressions", 'General') # type: Final
REDUNDANT_EXPR = ErrorCode(
'redundant-expr',
"Warn about redundant expressions",
'General',
default_enabled=False) # type: Final

# Syntax errors are often blocking.
SYNTAX = ErrorCode(
Expand Down
2 changes: 1 addition & 1 deletion mypy/main.py
Original file line number Diff line number Diff line change
Expand Up @@ -576,7 +576,7 @@ def add_invertible_flag(flag: str,
group=lint_group)
add_invertible_flag('--warn-unreachable', default=False, strict_flag=False,
help="Warn about statements or expressions inferred to be"
" unreachable or redundant",
" unreachable",
group=lint_group)

# Note: this group is intentionally added here even though we don't add
Expand Down
4 changes: 2 additions & 2 deletions mypy/messages.py
Original file line number Diff line number Diff line change
Expand Up @@ -1280,7 +1280,7 @@ def redundant_left_operand(self, op_name: str, context: Context) -> None:
"""
self.redundant_expr("Left operand of '{}'".format(op_name), op_name == 'and', context)

def redundant_right_operand(self, op_name: str, context: Context) -> None:
def unreachable_right_operand(self, op_name: str, context: Context) -> None:
"""Indicates that the right operand of a boolean expression is redundant:
it does not change the truth value of the entire condition as a whole.
'op_name' should either be the string "and" or the string "or".
Expand All @@ -1299,7 +1299,7 @@ def redundant_condition_in_assert(self, truthiness: bool, context: Context) -> N

def redundant_expr(self, description: str, truthiness: bool, context: Context) -> None:
self.fail("{} is always {}".format(description, str(truthiness).lower()),
context, code=codes.UNREACHABLE)
context, code=codes.REDUNDANT_EXPR)

def impossible_intersection(self,
formatted_base_class_list: str,
Expand Down
15 changes: 15 additions & 0 deletions test-data/unit/check-errorcodes.test
Original file line number Diff line number Diff line change
Expand Up @@ -771,3 +771,18 @@ def f(**x: int) -> None:
f(**1) # type: ignore[arg-type]
[builtins fixtures/dict.pyi]
[typing fixtures/typing-typeddict.pyi]

[case testRedundantExpressions]
# flags: --enable-error-code redundant-expr
def foo() -> bool: ...

lst = [1, 2, 3, 4]

b = False or foo() # E: Left operand of 'or' is always false [redundant-expr]
c = True and foo() # E: Left operand of 'and' is always true [redundant-expr]
g = 3 if True else 4 # E: If condition is always true [redundant-expr]
h = 3 if False else 4 # E: If condition is always false [redundant-expr]
i = [x for x in lst if True] # E: If condition in comprehension is always true [redundant-expr]
j = [x for x in lst if False] # E: If condition in comprehension is always false [redundant-expr]
k = [x for x in lst if isinstance(x, int) or foo()] # E: If condition in comprehension is always true [redundant-expr]
[builtins fixtures/isinstancelist.pyi]
9 changes: 1 addition & 8 deletions test-data/unit/check-unreachable-code.test
Original file line number Diff line number Diff line change
Expand Up @@ -898,18 +898,11 @@ def foo() -> bool: ...
lst = [1, 2, 3, 4]

a = True or foo() # E: Right operand of 'or' is never evaluated
b = False or foo() # E: Left operand of 'or' is always false
c = True and foo() # E: Left operand of 'and' is always true
d = False and foo() # E: Right operand of 'and' is never evaluated
e = True or (True or (True or foo())) # E: Right operand of 'or' is never evaluated
f = (True or foo()) or (True or foo()) # E: Right operand of 'or' is never evaluated
g = 3 if True else 4 # E: If condition is always true
h = 3 if False else 4 # E: If condition is always false
i = [x for x in lst if True] # E: If condition in comprehension is always true
j = [x for x in lst if False] # E: If condition in comprehension is always false

k = [x for x in lst if isinstance(x, int) or foo()] # E: If condition in comprehension is always true \
# E: Right operand of 'or' is never evaluated
k = [x for x in lst if isinstance(x, int) or foo()] # E: Right operand of 'or' is never evaluated
[builtins fixtures/isinstancelist.pyi]

[case testUnreachableFlagMiscTestCaseMissingMethod]
Expand Down
0