From a5b37377fe8ad7274ed6f8ff0e61754b1e44a2a2 Mon Sep 17 00:00:00 2001
From: Michael Lee <michael.lee.0x2a@gmail.com>
Date: Thu, 20 Jun 2019 07:44:20 -0700
Subject: [PATCH 1/7] Add flag to warn about unreachable branches and exprs

This diff adds a `--disallow-inferred-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 didn't spend a huge amount of time trying to come up with error
   messages. They could probably do with some polishing/rewording.

2. 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 https://github.com/python/mypy/pull/7048)

3. 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 https://github.com/python/mypy/issues/2395; partially
addresses https://github.com/python/mypy/issues/7008.
---
 mypy/checker.py                            |  26 ++-
 mypy/checkexpr.py                          |  30 +++-
 mypy/main.py                               |   4 +
 mypy/messages.py                           |  24 +++
 mypy/options.py                            |   5 +
 test-data/unit/check-unreachable-code.test | 176 +++++++++++++++++++++
 6 files changed, 260 insertions(+), 5 deletions(-)

diff --git a/mypy/checker.py b/mypy/checker.py
index 8720b4995f36..a2f066749fa6 100644
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -1777,13 +1777,36 @@ 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.disallow_inferred_unreachable and not self.is_raising_or_empty(s):
+                    self.msg.unreachable_branch(s)
                 break
             self.accept(s)
 
+    def is_raising_or_empty(self, s: Statement) -> bool:
+        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.
 
@@ -2956,7 +2979,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.
diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py
index 3c2306de6fdc..f0a460605ad8 100644
--- a/mypy/checkexpr.py
+++ b/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.disallow_inferred_unreachable:
+            if left_map is None:
+                self.msg.always_same_truth_value_left_operand(e.op, e.left)
+            if right_map is None:
+                self.msg.unreachable_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:
@@ -3174,12 +3185,18 @@ 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.disallow_inferred_unreachable:
+                    if true_map is None:
+                        self.msg.comprehension_cond_always_same(False, condition)
+                    elif false_map is None:
+                        self.msg.comprehension_cond_always_same(True, condition)
+
     def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
         self.accept(e.cond)
         ctx = self.type_context[-1]
@@ -3187,6 +3204,11 @@ def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
         # 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.disallow_inferred_unreachable:
+            if if_map is None:
+                self.msg.unreachable_branch_in_inline_if('if', e.cond)
+            elif else_map is None:
+                self.msg.unreachable_branch_in_inline_if('else', e.cond)
 
         if_type = self.analyze_cond_branch(if_map, e.if_expr, context=ctx)
 
diff --git a/mypy/main.py b/mypy/main.py
index 14fbee845516..7bca8c58022c 100644
--- a/mypy/main.py
+++ b/mypy/main.py
@@ -492,6 +492,10 @@ def add_invertible_flag(flag: str,
                         help="Treat imports as private unless aliased",
                         group=strictness_group)
 
+    add_invertible_flag('--disallow-inferred-unreachable', default=False, strict_flag=False,
+                        help="Disallow branches inferred to be unreachable after type analysis",
+                        group=strictness_group)
+
     incremental_group = parser.add_argument_group(
         title='Incremental mode',
         description="Adjust how mypy incrementally type checks and caches modules. "
diff --git a/mypy/messages.py b/mypy/messages.py
index 08812acaabcc..2fdc654aaabb 100644
--- a/mypy/messages.py
+++ b/mypy/messages.py
@@ -1228,6 +1228,30 @@ 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_branch(self, context: Context) -> None:
+        self.fail("This branch is inferred to be unreachable", context)
+
+    def always_same_truth_value_left_operand(self, op_name: str, context: Context) -> None:
+        value = 'true' if op_name == 'and' else 'false'
+        self.fail(
+            "The left operand of this '{}' expression is always {}".format(op_name, value),
+            context,
+        )
+
+    def unreachable_right_operand(self, op_name: str, context: Context) -> None:
+        self.fail(
+            "The right operand of this '{}' expression is never evaluated".format(op_name),
+            context,
+        )
+
+    def comprehension_cond_always_same(self, simplified_result: bool, context: Context) -> None:
+        template = "The conditional check in this comprehension is always {}"
+        self.fail(template.format(str(simplified_result).lower()), context)
+
+    def unreachable_branch_in_inline_if(self, sub_expr_name: str, context: Context) -> None:
+        template = "The '{}' expression in this inline if expression is never evaluated"
+        self.fail(template.format(sub_expr_name), context)
+
     def report_protocol_problems(self, subtype: Union[Instance, TupleType, TypedDictType],
                                  supertype: Instance, context: Context) -> None:
         """Report possible protocol conflicts between 'subtype' and 'supertype'.
diff --git a/mypy/options.py b/mypy/options.py
index 68cf3446d5da..7f83fbc16ffa 100644
--- a/mypy/options.py
+++ b/mypy/options.py
@@ -31,6 +31,7 @@ class BuildType:
     "disallow_any_generics",
     "disallow_any_unimported",
     "disallow_incomplete_defs",
+    "disallow_inferred_unreachable",
     "disallow_subclassing_any",
     "disallow_untyped_calls",
     "disallow_untyped_decorators",
@@ -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.disallow_inferred_unreachable = False
+
         # Variable names considered True
         self.always_true = []  # type: List[str]
 
diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test
index 593f24523242..36f429d1981a 100644
--- a/test-data/unit/check-unreachable-code.test
+++ b/test-data/unit/check-unreachable-code.test
@@ -715,3 +715,179 @@ if sys.version_info[0] >= 2:
     reveal_type('')  # N: Revealed type is 'builtins.str'
 reveal_type('')  # N: Revealed type is 'builtins.str'
 [builtins fixtures/ops.pyi]
+
+[case testUnreachableFlagWithBadControlFlow]
+# flags: --disallow-inferred-unreachable
+a: int
+if isinstance(a, int):
+    reveal_type(a)  # N: Revealed type is 'builtins.int'
+else:
+    reveal_type(a)  # E: This branch is inferred to be unreachable
+
+b: int
+while isinstance(b, int):
+    reveal_type(b)  # N: Revealed type is 'builtins.int'
+else:
+    reveal_type(b)  # E: This branch is inferred to be unreachable
+
+def foo(c: int) -> None:
+    reveal_type(c)  # N: Revealed type is 'builtins.int'
+    assert not isinstance(c, int)
+    reveal_type(c)  # E: This branch is inferred to be unreachable
+
+d: int
+if False:
+    reveal_type(d)  # E: This branch is inferred to be unreachable
+
+e: int
+if True:
+    reveal_type(e)  # N: Revealed type is 'builtins.int'
+else:
+    reveal_type(e)  # E: This branch is inferred to be unreachable
+
+[builtins fixtures/isinstancelist.pyi]
+
+[case testUnreachableFlagTryBlocks]
+# flags: --disallow-inferred-unreachable
+
+def foo(x: int) -> int:
+    try:
+        reveal_type(x)      # N: Revealed type is 'builtins.int'
+        return x
+        reveal_type(x)      # E: This branch is inferred to be unreachable
+    finally:
+        reveal_type(x)      # N: Revealed type is 'builtins.int'
+        if True:
+            reveal_type(x)  # N: Revealed type is 'builtins.int'
+        else:
+            reveal_type(x)  # E: This branch is inferred to be unreachable
+
+def bar(x: int) -> int:
+    try:
+        if True:
+            raise Exception()
+        reveal_type(x)      # E: This branch is inferred to be unreachable
+    except:
+        reveal_type(x)      # N: Revealed type is 'builtins.int'
+        return x
+    else:
+        reveal_type(x)      # E: This branch is inferred to be unreachable
+
+def baz(x: int) -> int:
+    try:
+        reveal_type(x)      # N: Revealed type is 'builtins.int'
+    except:
+        # Mypy assumes all lines could throw an exception
+        reveal_type(x)      # N: Revealed type is 'builtins.int'
+        return x
+    else:
+        reveal_type(x)      # N: Revealed type is 'builtins.int'
+        return x
+[builtins fixtures/exception.pyi]
+
+[case testUnreachableFlagIgnoresSemanticAnalysisUnreachable]
+# flags: --disallow-inferred-unreachable --python-version 3.7 --platform win32 --always-false FOOBAR
+import sys
+from typing import TYPE_CHECKING
+
+x: int
+if TYPE_CHECKING:
+    reveal_type(x)  # N: Revealed type is 'builtins.int'
+
+if sys.platform == 'darwin':
+    reveal_type(x)
+
+if sys.version_info == (2, 7):
+    reveal_type(x)
+
+FOOBAR = ""
+if FOOBAR:
+    reveal_type(x)
+else:
+    reveal_type(x)  # N: Revealed type is 'builtins.int'
+[builtins fixtures/ops.pyi]
+
+[case testUnreachableFlagIgnoresSemanticAnalysisExprUnreachable]
+# flags: --disallow-inferred-unreachable --always-false FOOBAR
+import sys
+from typing import TYPE_CHECKING
+
+FOOBAR = ""
+
+def foo() -> bool: ...
+
+lst = [1, 2, 3]
+
+a = FOOBAR and foo()
+b = (not FOOBAR) or foo()
+c = 1 if FOOBAR else 2
+d = [x for x in lst if FOOBAR]
+[builtins fixtures/list.pyi]
+
+[case testUnreachableFlagOkWithDeadStatements]
+# flags: --disallow-inferred-unreachable
+from typing import NoReturn
+def assert_never(x: NoReturn) -> NoReturn:
+    assert False
+
+def nonthrowing_assert_never(x: NoReturn) -> None: ...
+
+def expect_str(x: str) -> str: pass
+
+x: int
+if False:
+    assert False
+    reveal_type(x)
+
+if False:
+    raise Exception()
+    reveal_type(x)
+
+if False:
+    assert_never(x)
+    reveal_type(x)
+
+if False:
+    nonthrowing_assert_never(x)  # E: This branch is inferred to be unreachable
+    reveal_type(x)
+
+if False:
+    # Ignore obvious type errors
+    assert_never(expect_str(x))
+    reveal_type(x)
+[builtins fixtures/exception.pyi]
+
+[case testUnreachableFlagExpressions]
+# flags: --disallow-inferred-unreachable
+def foo() -> bool: ...
+
+lst = [1, 2, 3, 4]
+
+a = True or foo()                        # E: The right operand of this 'or' expression is never evaluated
+b = False or foo()                       # E: The left operand of this 'or' expression is always false
+c = True and foo()                       # E: The left operand of this 'and' expression is always true
+d = False and foo()                      # E: The right operand of this 'and' expression is never evaluated
+e = True or (True or (True or foo()))    # E: The right operand of this 'or' expression is never evaluated
+f = (True or foo()) or (True or foo())   # E: The right operand of this 'or' expression is never evaluated
+g = 3 if True else 4                     # E: The 'else' expression in this inline if expression is never evaluated
+h = 3 if False else 4                    # E: The 'if' expression in this inline if expression is never evaluated
+i = [x for x in lst if True]             # E: The conditional check in this comprehension is always true
+j = [x for x in lst if False]            # E: The conditional check in this comprehension is always false
+
+k = [x for x in lst if isinstance(x, int) or foo()]  # E: The conditional check in this comprehension is always true \
+                                                     # E: The right operand of this 'or' expression is never evaluated
+[builtins fixtures/isinstancelist.pyi]
+
+[case testUnreachableFlagMiscTestCaseMissingMethod]
+# flags: --disallow-inferred-unreachable
+
+class Case1:
+    def test1(self) -> bool:
+        return False and self.missing()  # E: The right operand of this 'and' expression is never evaluated
+
+    def test2(self) -> bool:
+        return not self.property_decorator_missing and self.missing()  # E: The right operand of this 'and' expression is never evaluated
+
+    def property_decorator_missing(self) -> bool:
+        return True
+[builtins fixtures/bool.pyi]

From 615f3b2806f574c6a964dc603263eae10e29a0dd Mon Sep 17 00:00:00 2001
From: Michael Lee <michael.lee.0x2a@gmail.com>
Date: Tue, 25 Jun 2019 20:38:14 -0700
Subject: [PATCH 2/7] Rename flag to '--warn-unreachable'

---
 mypy/checker.py                            |  2 +-
 mypy/checkexpr.py                          |  6 +++---
 mypy/main.py                               |  7 +++----
 mypy/options.py                            |  4 ++--
 test-data/unit/check-unreachable-code.test | 14 +++++++-------
 5 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/mypy/checker.py b/mypy/checker.py
index a2f066749fa6..1381337c9bbc 100644
--- a/mypy/checker.py
+++ b/mypy/checker.py
@@ -1784,7 +1784,7 @@ def visit_block(self, b: Block) -> None:
             return
         for s in b.body:
             if self.binder.is_unreachable():
-                if self.options.disallow_inferred_unreachable and not self.is_raising_or_empty(s):
+                if self.options.warn_unreachable and not self.is_raising_or_empty(s):
                     self.msg.unreachable_branch(s)
                 break
             self.accept(s)
diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py
index f0a460605ad8..130da3a480c0 100644
--- a/mypy/checkexpr.py
+++ b/mypy/checkexpr.py
@@ -2375,7 +2375,7 @@ def check_boolean_op(self, e: OpExpr, context: Context) -> Type:
         # 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.disallow_inferred_unreachable:
+        if self.chk.options.warn_unreachable:
             if left_map is None:
                 self.msg.always_same_truth_value_left_operand(e.op, e.left)
             if right_map is None:
@@ -3191,7 +3191,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.disallow_inferred_unreachable:
+                if self.chk.options.warn_unreachable:
                     if true_map is None:
                         self.msg.comprehension_cond_always_same(False, condition)
                     elif false_map is None:
@@ -3204,7 +3204,7 @@ def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
         # 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.disallow_inferred_unreachable:
+        if self.chk.options.warn_unreachable:
             if if_map is None:
                 self.msg.unreachable_branch_in_inline_if('if', e.cond)
             elif else_map is None:
diff --git a/mypy/main.py b/mypy/main.py
index 7bca8c58022c..fe9a583c9129 100644
--- a/mypy/main.py
+++ b/mypy/main.py
@@ -463,6 +463,9 @@ 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="Disallow branches inferred to be unreachable after type analysis",
+                        group=lint_group)
 
     # Note: this group is intentionally added here even though we don't add
     # --strict to this group near the end.
@@ -492,10 +495,6 @@ def add_invertible_flag(flag: str,
                         help="Treat imports as private unless aliased",
                         group=strictness_group)
 
-    add_invertible_flag('--disallow-inferred-unreachable', default=False, strict_flag=False,
-                        help="Disallow branches inferred to be unreachable after type analysis",
-                        group=strictness_group)
-
     incremental_group = parser.add_argument_group(
         title='Incremental mode',
         description="Adjust how mypy incrementally type checks and caches modules. "
diff --git a/mypy/options.py b/mypy/options.py
index 7f83fbc16ffa..6cafca7aa62e 100644
--- a/mypy/options.py
+++ b/mypy/options.py
@@ -31,7 +31,6 @@ class BuildType:
     "disallow_any_generics",
     "disallow_any_unimported",
     "disallow_incomplete_defs",
-    "disallow_inferred_unreachable",
     "disallow_subclassing_any",
     "disallow_untyped_calls",
     "disallow_untyped_decorators",
@@ -49,6 +48,7 @@ class BuildType:
     "strict_optional_whitelist",
     "warn_no_return",
     "warn_return_any",
+    "warn_unreachable",
     "warn_unused_ignores",
 }  # type: Final
 
@@ -167,7 +167,7 @@ def __init__(self) -> None:
 
         # Report an error for any branches inferred to be unreachable as a result of
         # type analysis.
-        self.disallow_inferred_unreachable = False
+        self.warn_unreachable = False
 
         # Variable names considered True
         self.always_true = []  # type: List[str]
diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test
index 36f429d1981a..c9fede93b5a6 100644
--- a/test-data/unit/check-unreachable-code.test
+++ b/test-data/unit/check-unreachable-code.test
@@ -717,7 +717,7 @@ reveal_type('')  # N: Revealed type is 'builtins.str'
 [builtins fixtures/ops.pyi]
 
 [case testUnreachableFlagWithBadControlFlow]
-# flags: --disallow-inferred-unreachable
+# flags: --warn-unreachable
 a: int
 if isinstance(a, int):
     reveal_type(a)  # N: Revealed type is 'builtins.int'
@@ -748,7 +748,7 @@ else:
 [builtins fixtures/isinstancelist.pyi]
 
 [case testUnreachableFlagTryBlocks]
-# flags: --disallow-inferred-unreachable
+# flags: --warn-unreachable
 
 def foo(x: int) -> int:
     try:
@@ -786,7 +786,7 @@ def baz(x: int) -> int:
 [builtins fixtures/exception.pyi]
 
 [case testUnreachableFlagIgnoresSemanticAnalysisUnreachable]
-# flags: --disallow-inferred-unreachable --python-version 3.7 --platform win32 --always-false FOOBAR
+# flags: --warn-unreachable --python-version 3.7 --platform win32 --always-false FOOBAR
 import sys
 from typing import TYPE_CHECKING
 
@@ -808,7 +808,7 @@ else:
 [builtins fixtures/ops.pyi]
 
 [case testUnreachableFlagIgnoresSemanticAnalysisExprUnreachable]
-# flags: --disallow-inferred-unreachable --always-false FOOBAR
+# flags: --warn-unreachable --always-false FOOBAR
 import sys
 from typing import TYPE_CHECKING
 
@@ -825,7 +825,7 @@ d = [x for x in lst if FOOBAR]
 [builtins fixtures/list.pyi]
 
 [case testUnreachableFlagOkWithDeadStatements]
-# flags: --disallow-inferred-unreachable
+# flags: --warn-unreachable
 from typing import NoReturn
 def assert_never(x: NoReturn) -> NoReturn:
     assert False
@@ -858,7 +858,7 @@ if False:
 [builtins fixtures/exception.pyi]
 
 [case testUnreachableFlagExpressions]
-# flags: --disallow-inferred-unreachable
+# flags: --warn-unreachable
 def foo() -> bool: ...
 
 lst = [1, 2, 3, 4]
@@ -879,7 +879,7 @@ k = [x for x in lst if isinstance(x, int) or foo()]  # E: The conditional check
 [builtins fixtures/isinstancelist.pyi]
 
 [case testUnreachableFlagMiscTestCaseMissingMethod]
-# flags: --disallow-inferred-unreachable
+# flags: --warn-unreachable
 
 class Case1:
     def test1(self) -> bool:

From ce8c357cb042bc6a84a7aa810162d5e921981b02 Mon Sep 17 00:00:00 2001
From: Michael Lee <michael.lee.0x2a@gmail.com>
Date: Tue, 25 Jun 2019 20:52:39 -0700
Subject: [PATCH 3/7] Apply trivial renames

---
 mypy/checkexpr.py                          |  7 +++--
 mypy/messages.py                           | 12 ++++-----
 test-data/unit/check-unreachable-code.test | 30 +++++++++++-----------
 3 files changed, 26 insertions(+), 23 deletions(-)

diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py
index 130da3a480c0..3c1c94f953c9 100644
--- a/mypy/checkexpr.py
+++ b/mypy/checkexpr.py
@@ -3206,9 +3206,12 @@ def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
         if_map, else_map = self.chk.find_isinstance_check(e.cond)
         if self.chk.options.warn_unreachable:
             if if_map is None:
-                self.msg.unreachable_branch_in_inline_if('if', e.cond)
+                self.msg.unreachable_branch_in_inline_if(
+                    condition_result=False, context=e.cond)
             elif else_map is None:
-                self.msg.unreachable_branch_in_inline_if('else', e.cond)
+                self.msg.unreachable_branch_in_inline_if(
+                    condition_result=True,
+                    context=e.cond)
 
         if_type = self.analyze_cond_branch(if_map, e.if_expr, context=ctx)
 
diff --git a/mypy/messages.py b/mypy/messages.py
index 2fdc654aaabb..67a02d1e573b 100644
--- a/mypy/messages.py
+++ b/mypy/messages.py
@@ -1234,23 +1234,23 @@ def unreachable_branch(self, context: Context) -> None:
     def always_same_truth_value_left_operand(self, op_name: str, context: Context) -> None:
         value = 'true' if op_name == 'and' else 'false'
         self.fail(
-            "The left operand of this '{}' expression is always {}".format(op_name, value),
+            "Left operand of '{}' is always {}".format(op_name, value),
             context,
         )
 
     def unreachable_right_operand(self, op_name: str, context: Context) -> None:
         self.fail(
-            "The right operand of this '{}' expression is never evaluated".format(op_name),
+            "Right operand of '{}' is never evaluated".format(op_name),
             context,
         )
 
     def comprehension_cond_always_same(self, simplified_result: bool, context: Context) -> None:
-        template = "The conditional check in this comprehension is always {}"
+        template = "If condition in comprehension is always {}"
         self.fail(template.format(str(simplified_result).lower()), context)
 
-    def unreachable_branch_in_inline_if(self, sub_expr_name: str, context: Context) -> None:
-        template = "The '{}' expression in this inline if expression is never evaluated"
-        self.fail(template.format(sub_expr_name), context)
+    def unreachable_branch_in_inline_if(self, condition_result: bool, context: Context) -> None:
+        template = "If condition is always {}"
+        self.fail(template.format(str(condition_result).lower()), context)
 
     def report_protocol_problems(self, subtype: Union[Instance, TupleType, TypedDictType],
                                  supertype: Instance, context: Context) -> None:
diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test
index c9fede93b5a6..89ddeb51b0d1 100644
--- a/test-data/unit/check-unreachable-code.test
+++ b/test-data/unit/check-unreachable-code.test
@@ -863,19 +863,19 @@ def foo() -> bool: ...
 
 lst = [1, 2, 3, 4]
 
-a = True or foo()                        # E: The right operand of this 'or' expression is never evaluated
-b = False or foo()                       # E: The left operand of this 'or' expression is always false
-c = True and foo()                       # E: The left operand of this 'and' expression is always true
-d = False and foo()                      # E: The right operand of this 'and' expression is never evaluated
-e = True or (True or (True or foo()))    # E: The right operand of this 'or' expression is never evaluated
-f = (True or foo()) or (True or foo())   # E: The right operand of this 'or' expression is never evaluated
-g = 3 if True else 4                     # E: The 'else' expression in this inline if expression is never evaluated
-h = 3 if False else 4                    # E: The 'if' expression in this inline if expression is never evaluated
-i = [x for x in lst if True]             # E: The conditional check in this comprehension is always true
-j = [x for x in lst if False]            # E: The conditional check in this comprehension is always false
-
-k = [x for x in lst if isinstance(x, int) or foo()]  # E: The conditional check in this comprehension is always true \
-                                                     # E: The right operand of this 'or' expression is never evaluated
+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
 [builtins fixtures/isinstancelist.pyi]
 
 [case testUnreachableFlagMiscTestCaseMissingMethod]
@@ -883,10 +883,10 @@ k = [x for x in lst if isinstance(x, int) or foo()]  # E: The conditional check
 
 class Case1:
     def test1(self) -> bool:
-        return False and self.missing()  # E: The right operand of this 'and' expression is never evaluated
+        return False and self.missing()  # E: Right operand of 'and' is never evaluated
 
     def test2(self) -> bool:
-        return not self.property_decorator_missing and self.missing()  # E: The right operand of this 'and' expression is never evaluated
+        return not self.property_decorator_missing and self.missing()  # E: Right operand of 'and' is never evaluated
 
     def property_decorator_missing(self) -> bool:
         return True

From 74663e8458e20c8a76444b77499e717dff993a7a Mon Sep 17 00:00:00 2001
From: Michael Lee <michael.lee.0x2a@gmail.com>
Date: Wed, 26 Jun 2019 08:39:18 -0700
Subject: [PATCH 4/7] Rename functions; adjust code to handle TypeVars with
 value restrictions

---
 mypy/binder.py                             | 15 +++++
 mypy/checker.py                            | 25 ++++++++-
 mypy/checkexpr.py                          | 15 ++---
 mypy/messages.py                           | 43 ++++++++-------
 test-data/unit/check-unreachable-code.test | 64 ++++++++++++++++++----
 5 files changed, 121 insertions(+), 41 deletions(-)

diff --git a/mypy/binder.py b/mypy/binder.py
index 9cb9aa433de1..e8ad27aed3e9 100644
--- a/mypy/binder.py
+++ b/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)
diff --git a/mypy/checker.py b/mypy/checker.py
index 1381337c9bbc..1db7acdcf44f 100644
--- a/mypy/checker.py
+++ b/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()
 
@@ -1784,12 +1793,22 @@ def visit_block(self, b: Block) -> None:
             return
         for s in b.body:
             if self.binder.is_unreachable():
-                if self.options.warn_unreachable and not self.is_raising_or_empty(s):
-                    self.msg.unreachable_branch(s)
+                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)):
diff --git a/mypy/checkexpr.py b/mypy/checkexpr.py
index 3c1c94f953c9..54fdbca7b227 100644
--- a/mypy/checkexpr.py
+++ b/mypy/checkexpr.py
@@ -2377,9 +2377,9 @@ def check_boolean_op(self, e: OpExpr, context: Context) -> Type:
         # So, we shouldn't report an error.
         if self.chk.options.warn_unreachable:
             if left_map is None:
-                self.msg.always_same_truth_value_left_operand(e.op, e.left)
+                self.msg.redundant_left_operand(e.op, e.left)
             if right_map is None:
-                self.msg.unreachable_right_operand(e.op, e.right)
+                self.msg.redundant_right_operand(e.op, e.right)
 
         if e.right_unreachable:
             right_map = None
@@ -3193,9 +3193,9 @@ def check_for_comp(self, e: Union[GeneratorExpr, DictionaryComprehension]) -> No
 
                 if self.chk.options.warn_unreachable:
                     if true_map is None:
-                        self.msg.comprehension_cond_always_same(False, condition)
+                        self.msg.redundant_condition_in_comprehension(False, condition)
                     elif false_map is None:
-                        self.msg.comprehension_cond_always_same(True, condition)
+                        self.msg.redundant_condition_in_comprehension(True, condition)
 
     def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
         self.accept(e.cond)
@@ -3206,12 +3206,9 @@ def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
         if_map, else_map = self.chk.find_isinstance_check(e.cond)
         if self.chk.options.warn_unreachable:
             if if_map is None:
-                self.msg.unreachable_branch_in_inline_if(
-                    condition_result=False, context=e.cond)
+                self.msg.redundant_condition_in_if(False, e.cond)
             elif else_map is None:
-                self.msg.unreachable_branch_in_inline_if(
-                    condition_result=True,
-                    context=e.cond)
+                self.msg.redundant_condition_in_if(True, e.cond)
 
         if_type = self.analyze_cond_branch(if_map, e.if_expr, context=ctx)
 
diff --git a/mypy/messages.py b/mypy/messages.py
index 67a02d1e573b..9cb279a2d7d0 100644
--- a/mypy/messages.py
+++ b/mypy/messages.py
@@ -1228,29 +1228,34 @@ 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_branch(self, context: Context) -> None:
-        self.fail("This branch is inferred to be unreachable", context)
+    def unreachable_statement(self, context: Context) -> None:
+        self.fail("Statement is unreachable", context)
 
-    def always_same_truth_value_left_operand(self, op_name: str, context: Context) -> None:
-        value = 'true' if op_name == 'and' else 'false'
-        self.fail(
-            "Left operand of '{}' is always {}".format(op_name, value),
-            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 unreachable_right_operand(self, op_name: str, context: Context) -> None:
-        self.fail(
-            "Right operand of '{}' is never evaluated".format(op_name),
-            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 comprehension_cond_always_same(self, simplified_result: bool, context: Context) -> None:
-        template = "If condition in comprehension is always {}"
-        self.fail(template.format(str(simplified_result).lower()), context)
+    def redundant_condition_in_assert(self, truthiness: bool, context: Context) -> None:
+        self.redundant_expr("Condition in assert", truthiness, context)
 
-    def unreachable_branch_in_inline_if(self, condition_result: bool, context: Context) -> None:
-        template = "If condition is always {}"
-        self.fail(template.format(str(condition_result).lower()), 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:
diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test
index 89ddeb51b0d1..c4e7e941685b 100644
--- a/test-data/unit/check-unreachable-code.test
+++ b/test-data/unit/check-unreachable-code.test
@@ -722,28 +722,28 @@ a: int
 if isinstance(a, int):
     reveal_type(a)  # N: Revealed type is 'builtins.int'
 else:
-    reveal_type(a)  # E: This branch is inferred to be unreachable
+    reveal_type(a)  # E: Statement is unreachable
 
 b: int
 while isinstance(b, int):
     reveal_type(b)  # N: Revealed type is 'builtins.int'
 else:
-    reveal_type(b)  # E: This branch is inferred to be unreachable
+    reveal_type(b)  # E: Statement is unreachable
 
 def foo(c: int) -> None:
     reveal_type(c)  # N: Revealed type is 'builtins.int'
     assert not isinstance(c, int)
-    reveal_type(c)  # E: This branch is inferred to be unreachable
+    reveal_type(c)  # E: Statement is unreachable
 
 d: int
 if False:
-    reveal_type(d)  # E: This branch is inferred to be unreachable
+    reveal_type(d)  # E: Statement is unreachable
 
 e: int
 if True:
     reveal_type(e)  # N: Revealed type is 'builtins.int'
 else:
-    reveal_type(e)  # E: This branch is inferred to be unreachable
+    reveal_type(e)  # E: Statement is unreachable
 
 [builtins fixtures/isinstancelist.pyi]
 
@@ -754,24 +754,24 @@ def foo(x: int) -> int:
     try:
         reveal_type(x)      # N: Revealed type is 'builtins.int'
         return x
-        reveal_type(x)      # E: This branch is inferred to be unreachable
+        reveal_type(x)      # E: Statement is unreachable
     finally:
         reveal_type(x)      # N: Revealed type is 'builtins.int'
         if True:
             reveal_type(x)  # N: Revealed type is 'builtins.int'
         else:
-            reveal_type(x)  # E: This branch is inferred to be unreachable
+            reveal_type(x)  # E: Statement is unreachable
 
 def bar(x: int) -> int:
     try:
         if True:
             raise Exception()
-        reveal_type(x)      # E: This branch is inferred to be unreachable
+        reveal_type(x)      # E: Statement is unreachable
     except:
         reveal_type(x)      # N: Revealed type is 'builtins.int'
         return x
     else:
-        reveal_type(x)      # E: This branch is inferred to be unreachable
+        reveal_type(x)      # E: Statement is unreachable
 
 def baz(x: int) -> int:
     try:
@@ -848,7 +848,7 @@ if False:
     reveal_type(x)
 
 if False:
-    nonthrowing_assert_never(x)  # E: This branch is inferred to be unreachable
+    nonthrowing_assert_never(x)  # E: Statement is unreachable
     reveal_type(x)
 
 if False:
@@ -891,3 +891,47 @@ class Case1:
     def property_decorator_missing(self) -> bool:
         return True
 [builtins fixtures/bool.pyi]
+
+[case testUnreachableFlagWithGenerics]
+# flags: --warn-unreachable
+from typing import TypeVar, Generic
+
+T1 = TypeVar('T1', bound=int)
+T2 = TypeVar('T2', int, str)
+
+def test1(x: T1) -> T1:
+    if isinstance(x, int):
+        reveal_type(x)  # N: Revealed type is 'T1`-1'
+    else:
+        reveal_type(x)  # E: Statement is unreachable
+    return x
+
+def test2(x: T2) -> T2:
+    if isinstance(x, int):
+        reveal_type(x)  # N: Revealed type is 'builtins.int*'
+    else:
+        reveal_type(x)  # N: Revealed type is 'builtins.str*'
+
+    if False:
+        # This is unreachable, but we don't report an error, unfortunately.
+        # The presence of the TypeVar with values unfortunately currently shuts
+        # down type-checking for this entire function.
+        # TODO: Find a way of removing this limitation
+        reveal_type(x)
+
+    return x
+
+class Test3(Generic[T2]):
+    x: T2
+
+    def func(self) -> None:
+        if isinstance(self.x, int):
+            reveal_type(self.x)  # N: Revealed type is 'builtins.int*'
+        else:
+            reveal_type(self.x)  # N: Revealed type is 'builtins.str*'
+
+        if False:
+            # Same issue as above
+            reveal_type(self.x)
+
+[builtins fixtures/isinstancelist.pyi]

From d16dd8e62c402a6467f491586bc68488f3f7314c Mon Sep 17 00:00:00 2001
From: Michael Lee <michael.lee.0x2a@gmail.com>
Date: Wed, 26 Jun 2019 08:41:59 -0700
Subject: [PATCH 5/7] Adjust help text for flag

---
 mypy/main.py | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/mypy/main.py b/mypy/main.py
index fe9a583c9129..c16d32c9333d 100644
--- a/mypy/main.py
+++ b/mypy/main.py
@@ -464,7 +464,8 @@ def add_invertible_flag(flag: str,
                              " from non-Any typed functions",
                         group=lint_group)
     add_invertible_flag('--warn-unreachable', default=False, strict_flag=False,
-                        help="Disallow branches inferred to be unreachable after type analysis",
+                        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

From 0ccb3671e1f46a3f60fa92df11b9d8543b10804d Mon Sep 17 00:00:00 2001
From: Michael Lee <michael.lee.0x2a@gmail.com>
Date: Wed, 26 Jun 2019 08:47:16 -0700
Subject: [PATCH 6/7] Add suggested test cases for platform and version checks

---
 test-data/unit/check-unreachable-code.test | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test
index c4e7e941685b..ac9b885b3d77 100644
--- a/test-data/unit/check-unreachable-code.test
+++ b/test-data/unit/check-unreachable-code.test
@@ -793,12 +793,33 @@ from typing import TYPE_CHECKING
 x: int
 if TYPE_CHECKING:
     reveal_type(x)  # N: Revealed type is 'builtins.int'
+else:
+    reveal_type(x)
+
+if not TYPE_CHECKING:
+    reveal_type(x)
+else:
+    reveal_type(x)  # N: Revealed type is 'builtins.int'
 
 if sys.platform == 'darwin':
     reveal_type(x)
+else:
+    reveal_type(x)  # N: Revealed type is 'builtins.int'
+
+if sys.platform == 'win32':
+    reveal_type(x)  # N: Revealed type is 'builtins.int'
+else:
+    reveal_type(x)
 
 if sys.version_info == (2, 7):
     reveal_type(x)
+else:
+    reveal_type(x)  # N: Revealed type is 'builtins.int'
+
+if sys.version_info == (3, 7):
+    reveal_type(x)  # N: Revealed type is 'builtins.int'
+else:
+    reveal_type(x)
 
 FOOBAR = ""
 if FOOBAR:

From 373c6eced4493f04916c4950382743fcf52062b6 Mon Sep 17 00:00:00 2001
From: Michael Lee <michael.lee.0x2a@gmail.com>
Date: Wed, 26 Jun 2019 09:00:24 -0700
Subject: [PATCH 7/7] Add test case for statement after return

---
 test-data/unit/check-unreachable-code.test | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/test-data/unit/check-unreachable-code.test b/test-data/unit/check-unreachable-code.test
index ac9b885b3d77..e1e99419dc26 100644
--- a/test-data/unit/check-unreachable-code.test
+++ b/test-data/unit/check-unreachable-code.test
@@ -747,6 +747,13 @@ else:
 
 [builtins fixtures/isinstancelist.pyi]
 
+[case testUnreachableFlagStatementAfterReturn]
+# flags: --warn-unreachable
+def foo(x: int) -> None:
+    reveal_type(x)  # N: Revealed type is 'builtins.int'
+    return
+    reveal_type(x)  # E: Statement is unreachable
+
 [case testUnreachableFlagTryBlocks]
 # flags: --warn-unreachable