Replace the binder's breaking_out by an 'unreachable' property of Frame

Also add some comments to the binder.

Author: rwbarton

mypy/binder.py

@@ -10,7 +10,19 @@
 
 
 class Frame(Dict[Any, Type]):
-    pass
+    """A Frame represents a specific point in the execution of a program.
+    It carries information about the current types of expressions at
+    that point, arising either from assignments to those expressions
+    or the result of isinstance checks. It also records whether it is
+    possible to reach that point at all.
+
+    This information is not copied into a new Frame when it is pushed
+    onto the stack, so a given Frame only has information about types
+    that were assigned in that frame.
+    """
+
+    def __init__(self) -> None:
+        self.unreachable = False
 
 
 class Key(AnyType):
@@ -39,13 +51,19 @@ class A:
     """
 
     def __init__(self) -> None:
-        # The set of frames currently used.  These map
+        # The stack of frames currently used.  These map
         # expr.literal_hash -- literals like 'foo.bar' --
-        # to types.
+        # to types. The last element of this list is the
+        # top-most, current frame. Each earlier element
+        # records the state as of when that frame was last
+        # on top of the stack.
         self.frames = [Frame()]
 
         # For frames higher in the stack, we record the set of
-        # Frames that can escape there
+        # Frames that can escape there, either by falling off
+        # the end of the frame or by a loop control construct
+        # or raised exception. The last element of self.frames
+        # has no corresponding element in this list.
         self.options_on_return = []  # type: List[List[Frame]]
 
         # Maps expr.literal_hash] to get_declaration(expr)
@@ -55,16 +73,8 @@ def __init__(self) -> None:
         # Whenever a new key (e.g. x.a.b) is added, we update this
         self.dependencies = {}  # type: Dict[Key, Set[Key]]
 
-        # breaking_out is set to True on return/break/continue/raise
-        # It is cleared on pop_frame() and placed in last_pop_breaking_out
-        # Lines of code after breaking_out = True are unreachable and not
-        # typechecked.
-        self.breaking_out = False
-
         # Whether the last pop changed the newly top frame on exit
         self.last_pop_changed = False
-        # Whether the last pop was necessarily breaking out, and couldn't fall through
-        self.last_pop_breaking_out = False
 
         self.try_frames = set()  # type: Set[int]
         self.loop_frames = []  # type: List[int]
@@ -105,9 +115,15 @@ def push(self, expr: Node, typ: Type) -> None:
             self._add_dependencies(key)
         self._push(key, typ)
 
+    def unreachable(self) -> bool:
+        self.frames[-1].unreachable = True
+
     def get(self, expr: Node) -> Type:
         return self._get(expr.literal_hash)
 
+    def is_unreachable(self) -> bool:
+        return self.frames[-1].unreachable
+
     def cleanse(self, expr: Node) -> None:
         """Remove all references to a Node from the binder."""
         self._cleanse_key(expr.literal_hash)
@@ -126,13 +142,17 @@ def update_from_options(self, frames: List[Frame]) -> bool:
         options are the same.
         """
 
+        frames = [f for f in frames if not f.unreachable]
         changed = False
         keys = set(key for f in frames for key in f)
 
         for key in keys:
             current_value = self._get(key)
             resulting_values = [f.get(key, current_value) for f in frames]
             if any(x is None for x in resulting_values):
+                # We didn't know anything about key before
+                # (current_value must be None), and we still don't
+                # know anything about key in at least one possible frame.
                 continue
 
             if isinstance(self.declarations.get(key), AnyType):
@@ -147,21 +167,26 @@ def update_from_options(self, frames: List[Frame]) -> bool:
                 self._push(key, type)
                 changed = True
 
+        self.frames[-1].unreachable = not frames
+
         return changed
 
-    def pop_frame(self, fall_through: int = 0) -> Frame:
+    def pop_frame(self, can_skip: bool, fall_through: int) -> Frame:
         """Pop a frame and return it.
 
         See frame_context() for documentation of fall_through.
         """
-        if fall_through and not self.breaking_out:
+
+        if fall_through > 0:
             self.allow_jump(-fall_through)
 
         result = self.frames.pop()
         options = self.options_on_return.pop()
 
+        if can_skip:
+            options.insert(0, self.frames[-1])
+
         self.last_pop_changed = self.update_from_options(options)
-        self.last_pop_breaking_out = self.breaking_out
 
         return result
 
@@ -240,6 +265,8 @@ def allow_jump(self, index: int) -> None:
         frame = Frame()
         for f in self.frames[index + 1:]:
             frame.update(f)
+            if f.unreachable:
+                frame.unreachable = True
         self.options_on_return[index].append(frame)
 
     def push_loop_frame(self) -> None:
@@ -249,16 +276,30 @@ def pop_loop_frame(self) -> None:
         self.loop_frames.pop()
 
     @contextmanager
-    def frame_context(self, fall_through: int = 0) -> Iterator[Frame]:
+    def frame_context(self, *, can_skip: bool, fall_through: int = 1) -> Iterator[Frame]:
         """Return a context manager that pushes/pops frames on enter/exit.
 
-        If fall_through > 0, then it will allow the frame to escape to
-        its ancestor `fall_through` levels higher.
+        If can_skip is True, control flow is allowed to bypass the
+        newly-created frame.
 
-        A simple 'with binder.frame_context(): pass' will change the
-        last_pop_* flags but nothing else.
+        If fall_through > 0, then it will allow control flow that
+        falls off the end of the frame to escape to its ancestor
+        `fall_through` levels higher. Otherwise control flow ends
+        at the end of the frame.
+
+        After the context manager exits, self.last_pop_changed indicates
+        whether any types changed in the newly-topmost frame as a result
+        of popping this frame.
+        """
+        assert len(self.frames) > 1
+        yield self.push_frame()
+        self.pop_frame(can_skip, fall_through)
+
+    @contextmanager
+    def top_frame_context(self) -> Iterator[Frame]:
+        """A variant of frame_context for use at the top level of
+        a namespace (module, function, or class).
         """
-        was_breaking_out = self.breaking_out
+        assert len(self.frames) == 1
         yield self.push_frame()
-        self.pop_frame(fall_through)
-        self.breaking_out = was_breaking_out
+        self.pop_frame(True, 0)

mypy/checker.py

@@ -165,8 +165,9 @@ def visit_file(self, file_node: MypyFile, path: str) -> None:
         # gross, but no other clear way to tell
         self.is_typeshed_stub = self.is_stub and 'typeshed' in os.path.normpath(path).split(os.sep)
 
-        for d in file_node.defs:
-            self.accept(d)
+        with self.binder.top_frame_context():
+            for d in file_node.defs:
+                self.accept(d)
 
         self.leave_partial_types()
 
@@ -231,12 +232,10 @@ def accept_loop(self, body: Node, else_body: Node = None) -> Type:
         Then check the else_body.
         """
         # The outer frame accumulates the results of all iterations
-        with self.binder.frame_context(1) as outer_frame:
+        with self.binder.frame_context(can_skip=False):
             self.binder.push_loop_frame()
             while True:
-                with self.binder.frame_context(1):
-                    # We may skip each iteration
-                    self.binder.options_on_return[-1].append(outer_frame)
+                with self.binder.frame_context(can_skip=True):
                     self.accept(body)
                 if not self.binder.last_pop_changed:
                     break
@@ -418,7 +417,7 @@ def check_func_def(self, defn: FuncItem, typ: CallableType, name: str) -> None:
         for item, typ in self.expand_typevars(defn, typ):
             old_binder = self.binder
             self.binder = ConditionalTypeBinder()
-            with self.binder.frame_context():
+            with self.binder.top_frame_context():
                 defn.expanded.append(item)
 
                 # We may be checking a function definition or an anonymous
@@ -505,7 +504,7 @@ def is_implicit_any(t: Type) -> bool:
                         self.accept(init)
 
             # Type check body in a new scope.
-            with self.binder.frame_context():
+            with self.binder.top_frame_context():
                 self.accept(item.body)
 
             self.return_types.pop()
@@ -815,7 +814,7 @@ def visit_class_def(self, defn: ClassDef) -> Type:
         self.enter_partial_types()
         old_binder = self.binder
         self.binder = ConditionalTypeBinder()
-        with self.binder.frame_context():
+        with self.binder.top_frame_context():
             self.accept(defn.defs)
         self.binder = old_binder
         self.check_multiple_inheritance(typ)
@@ -911,9 +910,9 @@ def visit_block(self, b: Block) -> Type:
         if b.is_unreachable:
             return None
         for s in b.body:
-            self.accept(s)
-            if self.binder.breaking_out:
+            if self.binder.is_unreachable():
                 break
+            self.accept(s)
 
     def visit_assignment_stmt(self, s: AssignmentStmt) -> Type:
         """Type check an assignment statement.
@@ -1354,7 +1353,10 @@ def visit_expression_stmt(self, s: ExpressionStmt) -> Type:
 
     def visit_return_stmt(self, s: ReturnStmt) -> Type:
         """Type check a return statement."""
-        self.binder.breaking_out = True
+        self.check_return_stmt(s)
+        self.binder.unreachable()
+
+    def check_return_stmt(self, s: ReturnStmt) -> None:
         if self.is_within_function():
             if self.function_stack[-1].is_generator:
                 return_type = self.get_generator_return_type(self.return_types[-1])
@@ -1422,9 +1424,9 @@ def count_nested_types(self, typ: Instance, check_type: str) -> int:
 
     def visit_if_stmt(self, s: IfStmt) -> Type:
         """Type check an if statement."""
-        breaking_out = True
         # This frame records the knowledge from previous if/elif clauses not being taken.
-        with self.binder.frame_context():
+        # Fall-through to the original frame is handled explicitly in each block.
+        with self.binder.frame_context(can_skip=False, fall_through=0):
             for e, b in zip(s.expr, s.body):
                 t = self.accept(e)
                 self.check_not_void(t, e)
@@ -1438,13 +1440,12 @@ def visit_if_stmt(self, s: IfStmt) -> Type:
                     pass
                 else:
                     # Only type check body if the if condition can be true.
-                    with self.binder.frame_context(2):
+                    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)
-                    breaking_out = breaking_out and self.binder.last_pop_breaking_out
 
                     if else_map:
                         for var, type in else_map.items():
@@ -1457,12 +1458,9 @@ def visit_if_stmt(self, s: IfStmt) -> Type:
                     # 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(2):
+                with self.binder.frame_context(can_skip=False, fall_through=2):
                     if s.else_body:
                         self.accept(s.else_body)
-                breaking_out = breaking_out and self.binder.last_pop_breaking_out
-        if breaking_out:
-            self.binder.breaking_out = True
         return None
 
     def visit_while_stmt(self, s: WhileStmt) -> Type:
@@ -1498,11 +1496,11 @@ def visit_assert_stmt(self, s: AssertStmt) -> Type:
 
     def visit_raise_stmt(self, s: RaiseStmt) -> Type:
         """Type check a raise statement."""
-        self.binder.breaking_out = True
         if s.expr:
             self.type_check_raise(s.expr, s)
         if s.from_expr:
             self.type_check_raise(s.from_expr, s)
+        self.binder.unreachable()
 
     def type_check_raise(self, e: Node, s: RaiseStmt) -> None:
         typ = self.accept(e)
@@ -1531,44 +1529,52 @@ def type_check_raise(self, e: Node, s: RaiseStmt) -> None:
     def visit_try_stmt(self, s: TryStmt) -> Type:
         """Type check a try statement."""
         # Our enclosing frame will get the result if the try/except falls through.
-        # This one gets all possible intermediate states
-        with self.binder.frame_context():
+        # This one gets all possible states after the try block exited abnormally
+        # (by exception, return, break, etc.)
+        with self.binder.frame_context(can_skip=False, fall_through=0):
             if s.finally_body:
                 self.binder.try_frames.add(len(self.binder.frames) - 1)
-                breaking_out = self.visit_try_without_finally(s)
+                self.visit_try_without_finally(s)
                 self.binder.try_frames.remove(len(self.binder.frames) - 1)
                 # First we check finally_body is type safe for all intermediate frames
                 self.accept(s.finally_body)
-                breaking_out = breaking_out or self.binder.breaking_out
             else:
-                breaking_out = self.visit_try_without_finally(s)
-
-        if not breaking_out and s.finally_body:
-            # Then we try again for the more restricted set of options that can fall through
+                self.visit_try_without_finally(s)
+
+        if s.finally_body:
+            # Then we try again for the more restricted set of options
+            # that can fall through. (Why do we need to check the
+            # finally clause twice? Depending on whether the finally
+            # clause was reached by the try clause falling off the end
+            # or exiting abnormally, after completing the finally clause
+            # either flow will continue to after the entire try statement
+            # or the exception/return/etc. will be processed and control
+            # flow will escape. We need to check that the finally clause
+            # 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)
-        self.binder.breaking_out = breaking_out
+
         return None
 
-    def visit_try_without_finally(self, s: TryStmt) -> bool:
+    def visit_try_without_finally(self, s: TryStmt) -> None:
         """Type check a try statement, ignoring the finally block.
 
-        Return whether we are guaranteed to be breaking out.
         Otherwise, it will place the results possible frames of
         that don't break out into self.binder.frames[-2].
         """
-        breaking_out = True
         # This frame records the possible states that exceptions can leave variables in
         # during the try: block
-        with self.binder.frame_context():
-            with self.binder.frame_context(3):
+        with self.binder.frame_context(can_skip=False, fall_through=0):
+            with self.binder.frame_context(can_skip=False, fall_through=3):
                 self.binder.try_frames.add(len(self.binder.frames) - 2)
+                self.binder.allow_jump(-1)
                 self.accept(s.body)
                 self.binder.try_frames.remove(len(self.binder.frames) - 2)
                 if s.else_body:
                     self.accept(s.else_body)
-            breaking_out = breaking_out and self.binder.last_pop_breaking_out
             for i in range(len(s.handlers)):
-                with self.binder.frame_context(3):
+                with self.binder.frame_context(can_skip=True, fall_through=3):
                     if s.types[i]:
                         t = self.visit_except_handler_test(s.types[i])
                         if s.vars[i]:
@@ -1592,8 +1598,6 @@ def visit_try_without_finally(self, s: TryStmt) -> bool:
                         var = cast(Var, s.vars[i].node)
                         var.type = DeletedType(source=source)
                         self.binder.cleanse(s.vars[i])
-                breaking_out = breaking_out and self.binder.last_pop_breaking_out
-        return breaking_out
 
     def visit_except_handler_test(self, n: Node) -> Type:
         """Type check an exception handler test clause."""
@@ -1811,13 +1815,13 @@ def visit_member_expr(self, e: MemberExpr) -> Type:
         return self.expr_checker.visit_member_expr(e)
 
     def visit_break_stmt(self, s: BreakStmt) -> Type:
-        self.binder.breaking_out = True
         self.binder.allow_jump(self.binder.loop_frames[-1] - 1)
+        self.binder.unreachable()
         return None
 
     def visit_continue_stmt(self, s: ContinueStmt) -> Type:
-        self.binder.breaking_out = True
         self.binder.allow_jump(self.binder.loop_frames[-1])
+        self.binder.unreachable()
         return None
 
     def visit_int_expr(self, e: IntExpr) -> Type: