Reduce duplication with find_isinstance_check and binder.push

The resulting simplification of visit_if_expr revealed a bug in the
binder: if a frame was marked as unreachable, but then a new frame
context was begun immediately, the new frame was not marked as
unreachable, and a block visited inside the new frame would be type
checked. For now we check all frames for reachability; probably there
is a more efficient way to do this.

This commit also cleans up check_boolean_op slightly.

Author: rwbarton

mypy/binder.py

@@ -118,7 +118,9 @@ def get(self, expr: Node) -> Type:
         return self._get(expr.literal_hash)
 
     def is_unreachable(self) -> bool:
-        return self.frames[-1].unreachable
+        # TODO: Copy the value of unreachable into new frames to avoid
+        # this traversal on every statement?
+        return any(f.unreachable for f in self.frames)
 
     def cleanse(self, expr: Node) -> None:
         """Remove all references to a Node from the binder."""

mypy/checker.py

@@ -243,14 +243,8 @@ def accept_loop(self, body: Node, else_body: Node = None, *,
                     break
             self.binder.pop_loop_frame()
             if exit_condition:
-                _, else_map = find_isinstance_check(
-                    exit_condition, self.type_map, self.typing_mode_weak()
-                )
-                if else_map is None:
-                    self.binder.unreachable()
-                else:
-                    for var, type in else_map.items():
-                        self.binder.push(var, type)
+                _, else_map = self.find_isinstance_check(exit_condition)
+                self.push_type_map(else_map)
             if else_body:
                 self.accept(else_body)
 
@@ -1441,37 +1435,19 @@ def visit_if_stmt(self, s: IfStmt) -> Type:
             for e, b in zip(s.expr, s.body):
                 t = self.accept(e)
                 self.check_not_void(t, e)
-                if_map, else_map = find_isinstance_check(
-                    e, self.type_map,
-                    self.typing_mode_weak()
-                )
-                if if_map is None:
-                    # The condition is always false
-                    # XXX should issue a warning?
-                    pass
-                else:
-                    # Only type check body if the if condition can be true.
-                    with self.binder.frame_context(can_skip=True, fall_through=2):
-                        if if_map:
-                            for var, type in if_map.items():
-                                self.binder.push(var, type)
-
-                        self.accept(b)
-
-                    if else_map:
-                        for var, type in else_map.items():
-                            self.binder.push(var, type)
-                if else_map is None:
-                    # The condition is always true => remaining elif/else blocks
-                    # can never be reached.
-
-                    # Might also want to issue a warning
-                    # print("Warning: isinstance always true")
-                    break
-            else:  # Didn't break => can't prove one of the conditions is always true
-                with self.binder.frame_context(can_skip=False, fall_through=2):
-                    if s.else_body:
-                        self.accept(s.else_body)
+                if_map, else_map = self.find_isinstance_check(e)
+
+                # XXX Issue a warning if condition is always False?
+                with self.binder.frame_context(can_skip=True, fall_through=2):
+                    self.push_type_map(if_map)
+                    self.accept(b)
+
+                # XXX Issue a warning if condition is always True?
+                self.push_type_map(else_map)
+
+            with self.binder.frame_context(can_skip=False, fall_through=2):
+                if s.else_body:
+                    self.accept(s.else_body)
         return None
 
     def visit_while_stmt(self, s: WhileStmt) -> Type:
@@ -1497,16 +1473,9 @@ def visit_assert_stmt(self, s: AssertStmt) -> Type:
         self.accept(s.expr)
 
         # If this is asserting some isinstance check, bind that type in the following code
-        true_map, _ = find_isinstance_check(
-            s.expr, self.type_map,
-            self.typing_mode_weak()
-        )
+        true_map, _ = self.find_isinstance_check(s.expr)
 
-        if true_map is None:
-            self.binder.unreachable()
-        else:
-            for var, type in true_map.items():
-                self.binder.push(var, type)
+        self.push_type_map(true_map)
 
     def visit_raise_stmt(self, s: RaiseStmt) -> Type:
         """Type check a raise statement."""
@@ -2135,6 +2104,17 @@ def function_type(self, func: FuncBase) -> FunctionLike:
     def method_type(self, func: FuncBase) -> FunctionLike:
         return method_type_with_fallback(func, self.named_type('builtins.function'))
 
+    def find_isinstance_check(self, n: Node) -> Tuple[Optional[Dict[Node, Type]],
+                                                      Optional[Dict[Node, Type]]]:
+        return find_isinstance_check(n, self.type_map, self.typing_mode_weak())
+
+    def push_type_map(self, type_map: Optional[Dict[Node, Type]]) -> None:
+        if type_map is None:
+            self.binder.unreachable()
+        else:
+            for expr, type in type_map.items():
+                self.binder.push(expr, type)
+
 
 # Data structure returned by find_isinstance_check representing
 # information learned from the truth or falsehood of a condition.  The

mypy/checkexpr.py

@@ -1080,22 +1080,13 @@ def check_boolean_op(self, e: OpExpr, context: Context) -> Type:
         left_type = self.accept(e.left, ctx)
 
         if e.op == 'and':
-            right_map, _ = \
-                mypy.checker.find_isinstance_check(e.left, self.chk.type_map,
-                                                   self.chk.typing_mode_weak())
+            right_map, _ = self.chk.find_isinstance_check(e.left)
         elif e.op == 'or':
-            _, right_map = \
-                mypy.checker.find_isinstance_check(e.left, self.chk.type_map,
-                                                   self.chk.typing_mode_weak())
+            _, right_map = self.chk.find_isinstance_check(e.left)
         else:
-            right_map = None
+            assert False, "check_boolean_op only supports 'and' and 'or' expressions"
 
-        with self.chk.binder.frame_context(can_skip=True, fall_through=0):
-            if right_map:
-                for var, type in right_map.items():
-                    self.chk.binder.push(var, type)
-
-            right_type = self.accept(e.right, left_type)
+        right_type = self.analyze_cond_branch(right_map, e.right, left_type)
 
         self.check_not_void(left_type, context)
         self.check_not_void(right_type, context)
@@ -1503,10 +1494,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 = mypy.checker.find_isinstance_check(
-            e.cond,
-            self.chk.type_map,
-            self.chk.typing_mode_weak())
+        if_map, else_map = self.chk.find_isinstance_check(e.cond)
 
         if_type = self.analyze_cond_branch(if_map, e.if_expr, context=ctx)
 
@@ -1534,9 +1522,12 @@ def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
     def analyze_cond_branch(self, map: Optional[Dict[Node, Type]],
                             node: Node, context: Optional[Type]) -> Type:
         with self.chk.binder.frame_context(can_skip=True, fall_through=0):
-            if map:
-                for var, type in map.items():
-                    self.chk.binder.push(var, type)
+            if map is None:
+                # We still need to type check node, in case we want to
+                # process it for isinstance checks later
+                self.accept(node, context=context)
+                return UninhabitedType()
+            self.chk.push_type_map(map)
             return self.accept(node, context=context)
 
     def visit_backquote_expr(self, e: BackquoteExpr) -> Type: