Add --warn-no-return, and major binder refactoring (#1748)

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

Also add some comments to the binder.

* Minor type cleanup in the binder and explanation of Key

This also fixes a bug where the index expression x['m'] was being treated
by the binder as equivalent to the member expression x.m.

* Add warn-no-return flag to give a note on missing return statements

* Learn that loop condition is False on exit from while loop

* Use bool() in tests when we want an unspecified bool in a conditional

* Handle 'while True', 'assert False'

* 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.

* Process try statements in top-to-bottom order

Fixes #1289 (again).

* Fix return type of binder.unreachable

* Skip docstring when determining whether function body is trivial

* Remove None return values from check_return_stmt

* Explain why two try frames are needed for a finally clause; add a test

* Decouple checker and binder by adding more options to frame_context

No more poking around by the checker in loop_frames and try_frames.

Author: rwbarton

mypy/binder.py

@@ -1,20 +1,28 @@
-from typing import (Any, Dict, List, Set, Iterator, Union)
+from typing import (Dict, List, Set, Iterator, Union)
 from contextlib import contextmanager
 
 from mypy.types import Type, AnyType, PartialType
-from mypy.nodes import (Node, Expression, Var, RefExpr, SymbolTableNode)
+from mypy.nodes import (Key, Node, Expression, Var, RefExpr, SymbolTableNode)
 
 from mypy.subtypes import is_subtype
 from mypy.join import join_simple
 from mypy.sametypes import is_same_type
 
 
-class Frame(Dict[Any, Type]):
-    pass
+class Frame(Dict[Key, Type]):
+    """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.
+    """
 
-class Key(AnyType):
-    pass
+    def __init__(self) -> None:
+        self.unreachable = False
 
 
 class ConditionalTypeBinder:
@@ -39,13 +47,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,27 +69,20 @@ 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]
+        self.break_frames = []  # type: List[int]
+        self.continue_frames = []  # type: List[int]
 
     def _add_dependencies(self, key: Key, value: Key = None) -> None:
         if value is None:
             value = key
         else:
             self.dependencies.setdefault(key, set()).add(value)
-        if isinstance(key, tuple):
-            for elt in key:
+        for elt in key:
+            if isinstance(elt, Key):
                 self._add_dependencies(elt, value)
 
     def push_frame(self) -> Frame:
@@ -105,9 +112,17 @@ def push(self, node: Node, typ: Type) -> None:
             self._add_dependencies(key)
         self._push(key, typ)
 
+    def unreachable(self) -> None:
+        self.frames[-1].unreachable = True
+
     def get(self, expr: Union[Expression, Var]) -> Type:
         return self._get(expr.literal_hash)
 
+    def is_unreachable(self) -> bool:
+        # 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: Expression) -> None:
         """Remove all references to a Node from the binder."""
         self._cleanse_key(expr.literal_hash)
@@ -126,13 +141,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,27 +166,32 @@ 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
 
-    def get_declaration(self, node: Node) -> Type:
-        if isinstance(node, (RefExpr, SymbolTableNode)) and isinstance(node.node, Var):
-            type = node.node.type
+    def get_declaration(self, expr: Node) -> Type:
+        if isinstance(expr, RefExpr) and isinstance(expr.node, Var):
+            type = expr.node.type
             if isinstance(type, PartialType):
                 return None
             return type
@@ -239,25 +263,74 @@ 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:
-        self.loop_frames.append(len(self.frames) - 1)
+    def handle_break(self) -> None:
+        self.allow_jump(self.break_frames[-1])
+        self.unreachable()
 
-    def pop_loop_frame(self) -> None:
-        self.loop_frames.pop()
+    def handle_continue(self) -> None:
+        self.allow_jump(self.continue_frames[-1])
+        self.unreachable()
 
     @contextmanager
-    def frame_context(self, fall_through: int = 0) -> Iterator[Frame]:
+    def frame_context(self, *, can_skip: bool, fall_through: int = 1,
+                      break_frame: int = 0, continue_frame: int = 0,
+                      try_frame: bool = False) -> 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.
+
+        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.
+
+        If break_frame > 0, then 'break' statements within this frame
+        will jump out to the frame break_frame levels higher than the
+        frame created by this call to frame_context. Similarly for
+        continue_frame and 'continue' statements.
+
+        If try_frame is true, then execution is allowed to jump at any
+        point within the newly created frame (or its descendents) to
+        its parent (i.e., to the frame that was on top before this
+        call to frame_context).
 
-        A simple 'with binder.frame_context(): pass' will change the
-        last_pop_* flags but nothing else.
+        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
+
+        if break_frame:
+            self.break_frames.append(len(self.frames) - break_frame)
+        if continue_frame:
+            self.continue_frames.append(len(self.frames) - continue_frame)
+        if try_frame:
+            self.try_frames.add(len(self.frames) - 1)
+
+        new_frame = self.push_frame()
+        if try_frame:
+            # An exception may occur immediately
+            self.allow_jump(-1)
+        yield new_frame
+        self.pop_frame(can_skip, fall_through)
+
+        if break_frame:
+            self.break_frames.pop()
+        if continue_frame:
+            self.continue_frames.pop()
+        if try_frame:
+            self.try_frames.remove(len(self.frames) - 1)
+
+    @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)