Support multiple isinstance in a single if.

This also squashes a minor weirdness where in order to infer
isinstance for resolution further in an and-expr, we were
parsing for isinstance in the entire expression!

Overall this is pretty self-explanatory. We get rid of the 'kind'
stuff and just return a tuple of optional dicts.

Fixes #900.

Author: jhance

mypy/checker.py

@@ -2,7 +2,9 @@
 
 import itertools
 
-from typing import Any, Dict, Set, List, cast, Tuple, Callable, TypeVar, Union
+from typing import (
+    Any, Dict, Set, List, cast, Tuple, Callable, TypeVar, Union, Optional
+)
 
 from mypy.errors import Errors
 from mypy.nodes import (
@@ -45,11 +47,6 @@
 from mypy.meet import meet_simple, meet_simple_away, nearest_builtin_ancestor, is_overlapping_types
 
 
-# Kinds of isinstance checks.
-ISINSTANCE_OVERLAPPING = 0
-ISINSTANCE_ALWAYS_TRUE = 1
-ISINSTANCE_ALWAYS_FALSE = 2
-
 T = TypeVar('T')
 
 
@@ -1433,16 +1430,21 @@ 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)
-            var, type, elsetype, kind = find_isinstance_check(e, self.type_map,
-                                            self.typing_mode_weak())
-            if kind == ISINSTANCE_ALWAYS_FALSE:
+            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.
                 self.binder.push_frame()
-                if var:
-                    self.binder.push(var, type)
+                if if_map:
+                    for var, type in if_map.items():
+                        self.binder.push(var, type)
+
                 self.accept(b)
                 _, frame = self.binder.pop_frame()
                 if not self.breaking_out:
@@ -1451,9 +1453,10 @@ def visit_if_stmt(self, s: IfStmt) -> Type:
 
                 self.breaking_out = False
 
-                if var:
-                    self.binder.push(var, elsetype)
-            if kind == ISINSTANCE_ALWAYS_TRUE:
+                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.
 
@@ -2046,24 +2049,17 @@ def map_type_from_supertype(typ: Type, sub_info: TypeInfo,
 
 def find_isinstance_check(node: Node,
                           type_map: Dict[Node, Type],
-                          weak: bool=False) -> Tuple[Node, Type, Type, int]:
-    """Check if node is an isinstance(variable, type) check.
-
-    If successful, return tuple (variable, target-type, else-type,
-    kind); otherwise, return (None, AnyType, AnyType, -1).
+                          weak: bool=False) \
+        -> Tuple[Optional[Dict[Node, Type]], Optional[Dict[Node, Type]]]:
+    """Find any isinstance checks (within a chain of ands)".
 
-    When successful, the kind takes one of these values:
+    Return value is a map of variables to their types if the condition
+    is true and a map of variables to their types if the condition is false.
 
-      ISINSTANCE_OVERLAPPING: The type of variable and the target type are
-          partially overlapping => the test result can be True or False.
-      ISINSTANCE_ALWAYS_TRUE: The target type at least as general as the
-          variable type => the test is always True.
-      ISINSTANCE_ALWAYS_FALSE: The target type and the variable type are not
-          overlapping => the test is always False.
+    If either of the values in the tuple is None, then that particular
+    branch can never occur.
 
-    If it is an isinstance check, but we don't understand the argument
-    type, then in weak mode it is treated as Any and in non-weak mode
-    it is not treated as an isinstance.
+    Guaranteed to not return None, None. (But may return {}, {})
     """
     if isinstance(node, CallExpr):
         if refers_to_fullname(node.callee, 'builtins.isinstance'):
@@ -2072,46 +2068,45 @@ def find_isinstance_check(node: Node,
                 vartype = type_map[expr]
                 type = get_isinstance_type(node.args[1], type_map)
                 if type:
-                    kind = ISINSTANCE_OVERLAPPING
                     elsetype = vartype
                     if vartype:
                         if is_proper_subtype(vartype, type):
-                            kind = ISINSTANCE_ALWAYS_TRUE
                             elsetype = None
+                            return {expr: type}, None
                         elif not is_overlapping_types(vartype, type):
-                            kind = ISINSTANCE_ALWAYS_FALSE
+                            return None, {expr: elsetype}
                         else:
                             elsetype = restrict_subtype_away(vartype, type)
-                    return expr, type, elsetype, kind
+                    return {expr: type}, {expr: elsetype}
                 else:
                     # An isinstance check, but we don't understand the type
                     if weak:
-                        return expr, AnyType(), vartype, ISINSTANCE_OVERLAPPING
+                        return {expr: AnyType()}, {expr: vartype}
     elif isinstance(node, OpExpr) and node.op == 'and':
-        # XXX We should extend this to support two isinstance checks in the same
-        # expression
-        (var, type, elsetype, kind) = find_isinstance_check(node.left, type_map, weak)
-        if var is None:
-            (var, type, elsetype, kind) = find_isinstance_check(node.left, type_map, weak)
-        if var:
-            if kind == ISINSTANCE_ALWAYS_TRUE:
-                kind = ISINSTANCE_OVERLAPPING
-            return (var, type, AnyType(), kind)
+        left_if_vars, right_else_vars = find_isinstance_check(
+            node.left,
+            type_map,
+            weak,
+        )
+
+        right_if_vars, right_else_vars = find_isinstance_check(
+            node.right,
+            type_map,
+            weak,
+        )
+        if left_if_vars:
+            left_if_vars.update(right_if_vars)
+        else:
+            left_if_vars = right_if_vars
+
+        # Make no claim about the types in else
+        return left_if_vars, {}
     elif isinstance(node, UnaryExpr) and node.op == 'not':
-        (var, type, elsetype, kind) = find_isinstance_check(node.expr, type_map, weak)
-        return (var, elsetype, type, invert_isinstance_kind(kind))
+        left, right = find_isinstance_check(node.expr, type_map, weak)
+        return right, left
 
     # Not a supported isinstance check
-    return None, AnyType(), AnyType(), -1
-
-
-def invert_isinstance_kind(kind: int) -> int:
-    if kind == ISINSTANCE_ALWAYS_TRUE:
-        return ISINSTANCE_ALWAYS_FALSE
-    elif kind == ISINSTANCE_ALWAYS_FALSE:
-        return ISINSTANCE_ALWAYS_TRUE
-    else:
-        return kind
+    return {}, {}
 
 
 def get_isinstance_type(node: Node, type_map: Dict[Node, Type]) -> Type:

mypy/checkexpr.py

@@ -911,17 +911,21 @@ def check_boolean_op(self, e: OpExpr, context: Context) -> Type:
         left_type = self.accept(e.left, ctx)
 
         if e.op == 'and':
-            var, type, elsetype, kind = \
-                mypy.checker.find_isinstance_check(e, self.chk.type_map,
+            # else_map unused
+            if_map, else_map = \
+                mypy.checker.find_isinstance_check(e.left, self.chk.type_map,
                                                    self.chk.typing_mode_weak())
         else:
-            var = None
-        if var:
-            self.chk.binder.push_frame()
-            self.chk.binder.push(var, type)
+            if_map = None
+
+        self.chk.binder.push_frame()
+        if if_map:
+            for var, type in if_map.items():
+                self.chk.binder.push(var, type)
+
         right_type = self.accept(e.right, left_type)
-        if var:
-            self.chk.binder.pop_frame()
+
+        self.chk.binder.pop_frame()
 
         self.check_not_void(left_type, context)
         self.check_not_void(right_type, context)
@@ -1278,22 +1282,30 @@ def visit_conditional_expr(self, e: ConditionalExpr) -> Type:
 
         # Gain type information from isinstance if it is there
         # but only for the current expression
-        variable, inst_if_type, inst_else_type, kind = mypy.checker.find_isinstance_check(
+        if_map, else_map = mypy.checker.find_isinstance_check(
             e.cond,
             self.chk.type_map,
             self.chk.typing_mode_weak())
-        if variable:
-            self.chk.binder.push_frame()
-            self.chk.binder.push(variable, inst_if_type)
-            if_type = self.accept(e.if_expr)
-            self.chk.binder.pop_frame()
-            self.chk.binder.push_frame()
-            self.chk.binder.push(variable, inst_else_type)
-            else_type = self.accept(e.else_expr, context=if_type)
-            self.chk.binder.pop_frame()
-        else:
-            if_type = self.accept(e.if_expr)
-            else_type = self.accept(e.else_expr, context=if_type)
+
+        self.chk.binder.push_frame()
+
+        if if_map:
+            for var, type in if_map.items():
+                self.chk.binder.push(var, type)
+
+        if_type = self.accept(e.if_expr)
+
+        self.chk.binder.pop_frame()
+        self.chk.binder.push_frame()
+
+        if else_map:
+            for var, type in else_map.items():
+                self.chk.binder.push(var, type)
+
+        else_type = self.accept(e.else_expr, context=if_type)
+
+        self.chk.binder.pop_frame()
+
         return join.join_types(if_type, else_type)
 
     #

mypy/test/data/check-isinstance.test

@@ -721,3 +721,20 @@ x.flag if isinstance(x, B) else 0
 0 if not isinstance(x, B) else x.flag
 0 if isinstance(x, B) else x.flag # E: "A" has no attribute "flag"
 [builtins fixtures/isinstancelist.py]
+[case testIsinstanceMultiAnd]
+class A:
+    pass
+
+class B(A):
+    flag = 1
+
+class C(A):
+    glaf = 1
+
+x = B() # type: A
+y = C() # type: A
+
+if isinstance(x, B) and isinstance(y, C):
+    x.flag += 1
+    y.glaf += 1
+[builtins fixtures/isinstancelist.py]