Add flag to warn about unreachable branches and exprs (#7050)

This diff adds a `--warn-unreachable` flag that reports an error if:

1. Any branch is inferred to be unreachable
2. Any subexpression in boolean expressions, inline if statements,
   and list/set/generator/dict comprehensions are always unreachable
   or redundant.

This only takes into account the results of *type* analysis. We do not
report errors for branches that are inferred to be unreachable in the
semantic analysis phase (e.g. due to `sys.platform` checks and the
like).

Those checks are all intentional/deliberately flag certain branches
as unreachable, so error messages would be annoying in those cases.

A few additional notes:

1. I thought about enabling this flag for default with mypy, but
   unfortunately that ended up producing ~40 to 50 (apparently
   legitimate) error messages.

   About a fourth of them were due to runtime checks checking to see
   if some type is None (despite not being declared to be Optional),
   about a fourth seemed to be due to mypy not quite understanding how
   to handle things like traits and Bogus[...], and about a fourth
   seemed to be legitimately unnecessary checks we could safely remove.

   The final checks were a mixture of typeshed bugs and misc errors with
   the reachability checks. (e.g. see #7048)

2. For these reasons, I decided against adding this flag to `--strict`
   and against documenting it yet. We'll probably need to flush out
   a few bugs in mypy first/get mypy to the state where we can at least
   honestly dogfood this.

Resolves #2395; partially
addresses #7008.

Author: Michael0x2a

mypy/binder.py

@@ -32,6 +32,14 @@ def __init__(self) -> None:
         self.types = {}  # type: Dict[Key, Type]
         self.unreachable = False
 
+        # Should be set only if we're entering a frame where it's not
+        # possible to accurately determine whether or not contained
+        # statements will be unreachable or not.
+        #
+        # Long-term, we should improve mypy to the point where we no longer
+        # need this field.
+        self.suppress_unreachable_warnings = False
+
 
 Assigns = DefaultDict[Expression, List[Tuple[Type, Optional[Type]]]]
 
@@ -131,6 +139,9 @@ def put(self, expr: Expression, typ: Type) -> None:
     def unreachable(self) -> None:
         self.frames[-1].unreachable = True
 
+    def suppress_unreachable_warnings(self) -> None:
+        self.frames[-1].suppress_unreachable_warnings = True
+
     def get(self, expr: Expression) -> Optional[Type]:
         key = literal_hash(expr)
         assert key is not None, 'Internal error: binder tried to get non-literal'
@@ -141,6 +152,10 @@ def is_unreachable(self) -> bool:
         # this traversal on every statement?
         return any(f.unreachable for f in self.frames)
 
+    def is_unreachable_warning_suppressed(self) -> bool:
+        # TODO: See todo in 'is_unreachable'
+        return any(f.suppress_unreachable_warnings for f in self.frames)
+
     def cleanse(self, expr: Expression) -> None:
         """Remove all references to a Node from the binder."""
         key = literal_hash(expr)

mypy/checker.py

@@ -783,7 +783,8 @@ def enter_attribute_inference_context(self) -> Iterator[None]:
     def check_func_def(self, defn: FuncItem, typ: CallableType, name: Optional[str]) -> None:
         """Type check a function definition."""
         # Expand type variables with value restrictions to ordinary types.
-        for item, typ in self.expand_typevars(defn, typ):
+        expanded = self.expand_typevars(defn, typ)
+        for item, typ in expanded:
             old_binder = self.binder
             self.binder = ConditionalTypeBinder()
             with self.binder.top_frame_context():
@@ -932,6 +933,14 @@ def check_func_def(self, defn: FuncItem, typ: CallableType, name: Optional[str])
             # Type check body in a new scope.
             with self.binder.top_frame_context():
                 with self.scope.push_function(defn):
+                    # We suppress reachability warnings when we use TypeVars with value
+                    # restrictions: we only want to report a warning if a certain statement is
+                    # marked as being suppressed in *all* of the expansions, but we currently
+                    # have no good way of doing this.
+                    #
+                    # TODO: Find a way of working around this limitation
+                    if len(expanded) >= 2:
+                        self.binder.suppress_unreachable_warnings()
                     self.accept(item.body)
                 unreachable = self.binder.is_unreachable()
 
@@ -1777,13 +1786,46 @@ def check_import(self, node: ImportBase) -> None:
 
     def visit_block(self, b: Block) -> None:
         if b.is_unreachable:
+            # This block was marked as being unreachable during semantic analysis.
+            # It turns out any blocks marked in this way are *intentionally* marked
+            # as unreachable -- so we don't display an error.
             self.binder.unreachable()
             return
         for s in b.body:
             if self.binder.is_unreachable():
+                if (self.options.warn_unreachable
+                        and not self.binder.is_unreachable_warning_suppressed()
+                        and not self.is_raising_or_empty(s)):
+                    self.msg.unreachable_statement(s)
                 break
             self.accept(s)
 
+    def is_raising_or_empty(self, s: Statement) -> bool:
+        """Returns 'true' if the given statement either throws an error of some kind
+        or is a no-op.
+
+        We use this function mostly while handling the '--warn-unreachable' flag. When
+        that flag is present, we normally report an error on any unreachable statement.
+        But if that statement is just something like a 'pass' or a just-in-case 'assert False',
+        reporting an error would be annoying.
+        """
+        if isinstance(s, AssertStmt) and is_false_literal(s.expr):
+            return True
+        elif isinstance(s, (RaiseStmt, PassStmt)):
+            return True
+        elif isinstance(s, ExpressionStmt):
+            if isinstance(s.expr, EllipsisExpr):
+                return True
+            elif isinstance(s.expr, CallExpr):
+                self.expr_checker.msg.disable_errors()
+                typ = self.expr_checker.accept(
+                    s.expr, allow_none_return=True, always_allow_any=True)
+                self.expr_checker.msg.enable_errors()
+
+                if isinstance(typ, UninhabitedType):
+                    return True
+        return False
+
     def visit_assignment_stmt(self, s: AssignmentStmt) -> None:
         """Type check an assignment statement.
 
@@ -2954,7 +2996,8 @@ def visit_try_stmt(self, s: TryStmt) -> None:
             # type checks in both contexts, but only the resulting types
             # from the latter context affect the type state in the code
             # that follows the try statement.)
-            self.accept(s.finally_body)
+            if not self.binder.is_unreachable():
+                self.accept(s.finally_body)
 
     def visit_try_without_finally(self, s: TryStmt, try_frame: bool) -> None:
         """Type check a try statement, ignoring the finally block.

mypy/checkexpr.py

@@ -2367,14 +2367,25 @@ 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 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.
+        #
+        # 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.
+        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)
+
         if e.right_unreachable:
             right_map = None
         elif e.right_always:
             left_map = None
 
-        # 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.
         if right_map is None:
             self.msg.disable_errors()
         try:
@@ -3178,19 +3189,30 @@ def check_for_comp(self, e: Union[GeneratorExpr, DictionaryComprehension]) -> No
                 self.accept(condition)
 
                 # values are only part of the comprehension when all conditions are true
-                true_map, _ = self.chk.find_isinstance_check(condition)
+                true_map, false_map = self.chk.find_isinstance_check(condition)
 
                 if true_map:
                     for var, type in true_map.items():
                         self.chk.binder.put(var, type)
 
+                if self.chk.options.warn_unreachable:
+                    if true_map is None:
+                        self.msg.redundant_condition_in_comprehension(False, condition)
+                    elif false_map is None:
+                        self.msg.redundant_condition_in_comprehension(True, condition)
+
     def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
         self.accept(e.cond)
         ctx = self.type_context[-1]
 
         # 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 if_map is None:
+                self.msg.redundant_condition_in_if(False, e.cond)
+            elif else_map is None:
+                self.msg.redundant_condition_in_if(True, e.cond)
 
         if_type = self.analyze_cond_branch(if_map, e.if_expr, context=ctx)
 

mypy/main.py

@@ -463,6 +463,10 @@ def add_invertible_flag(flag: str,
                         help="Warn about returning values of type Any"
                              " from non-Any typed functions",
                         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",
+                        group=lint_group)
 
     # Note: this group is intentionally added here even though we don't add
     # --strict to this group near the end.

mypy/messages.py

@@ -1228,6 +1228,35 @@ def note_call(self, subtype: Type, call: Type, context: Context) -> None:
         self.note('"{}.__call__" has type {}'.format(self.format_bare(subtype),
                                                      self.format(call, verbosity=1)), context)
 
+    def unreachable_statement(self, context: Context) -> None:
+        self.fail("Statement is unreachable", context)
+
+    def redundant_left_operand(self, op_name: str, context: Context) -> None:
+        """Indicates that the left 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".
+        """
+        self.redundant_expr("Left operand of '{}'".format(op_name), op_name == 'and', context)
+
+    def redundant_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".
+        """
+        self.fail("Right operand of '{}' is never evaluated".format(op_name), context)
+
+    def redundant_condition_in_comprehension(self, truthiness: bool, context: Context) -> None:
+        self.redundant_expr("If condition in comprehension", truthiness, context)
+
+    def redundant_condition_in_if(self, truthiness: bool, context: Context) -> None:
+        self.redundant_expr("If condition", truthiness, context)
+
+    def redundant_condition_in_assert(self, truthiness: bool, context: Context) -> None:
+        self.redundant_expr("Condition in assert", truthiness, context)
+
+    def redundant_expr(self, description: str, truthiness: bool, context: Context) -> None:
+        self.fail("{} is always {}".format(description, str(truthiness).lower()), context)
+
     def report_protocol_problems(self, subtype: Union[Instance, TupleType, TypedDictType],
                                  supertype: Instance, context: Context) -> None:
         """Report possible protocol conflicts between 'subtype' and 'supertype'.

mypy/options.py

@@ -48,6 +48,7 @@ class BuildType:
     "strict_optional_whitelist",
     "warn_no_return",
     "warn_return_any",
+    "warn_unreachable",
     "warn_unused_ignores",
 }  # type: Final
 
@@ -164,6 +165,10 @@ def __init__(self) -> None:
         # This makes 1 == '1', 1 in ['1'], and 1 is '1' errors.
         self.strict_equality = False
 
+        # Report an error for any branches inferred to be unreachable as a result of
+        # type analysis.
+        self.warn_unreachable = False
+
         # Variable names considered True
         self.always_true = []  # type: List[str]