Repository: quora/pyanalyze
Pull request #494 (2022-03-08 02:16:47)

While Loops: handle exiting a loop from within a conditional block
The existing logic doesn't recognize that you can break out of loops inside of an `if` statement, since it only checks for `LEAVES_LOOP` in the main scope.

JelleZijlstra: The self-check is still failing on 3.6 because the `StackedScopes.loop_scope` annotation will also need to be changed.

I'm going to look into why we don't catch this on 3.7+.
nbdaaron: I can update that. does the type for the base Scope class not need to be changed?
JelleZijlstra: > does the type for the base Scope class not need to be changed

Yes I think so. That's another thing that pyanalyze should catch by itself.

Patch:
diff --git a/docs/changelog.md b/docs/changelog.md
index eccbc20..ac76b41 100644
--- a/docs/changelog.md
+++ b/docs/changelog.md
@@ -2,6 +2,9 @@
 
 ## Unreleased
 
+- Fix bug where code after a `while` loop is considered
+  unreachable if all `break` statements are inside of `if`
+  statements (#494)
 - Remove support for detecting properties that represent
   synchronous equivalents of asynq methods (#493)
 - Enable exhaustive checking of enums and booleans (#492)
diff --git a/pyanalyze/name_check_visitor.py b/pyanalyze/name_check_visitor.py
index 7480c1e..f9b9121 100644
--- a/pyanalyze/name_check_visitor.py
+++ b/pyanalyze/name_check_visitor.py
@@ -3349,7 +3349,7 @@ class NameCheckVisitor(node_visitor.ReplacingNodeVisitor):
             Boolability.type_always_true,
         )
         with self.scopes.subscope() as body_scope:
-            with self.scopes.loop_scope() as loop_scope:
+            with self.scopes.loop_scope() as loop_scopes:
                 # The "node" argument need not be an AST node but must be unique.
                 self.add_constraint((node, 1), constraint)
                 self._generic_visit_list(node.body)
@@ -3363,7 +3363,7 @@ class NameCheckVisitor(node_visitor.ReplacingNodeVisitor):
                 self.add_constraint((node, 2), constraint)
                 self._generic_visit_list(node.body)
 
-        if always_entered and LEAVES_LOOP not in loop_scope:
+        if always_entered and all(LEAVES_LOOP not in scope for scope in loop_scopes):
             # This means the code following the loop is unreachable.
             self._set_name_in_scope(LEAVES_SCOPE, node, AnyValue(AnySource.marker))
 
diff --git a/pyanalyze/stacked_scopes.py b/pyanalyze/stacked_scopes.py
index 898c69e..76b39c7 100644
--- a/pyanalyze/stacked_scopes.py
+++ b/pyanalyze/stacked_scopes.py
@@ -1175,12 +1175,12 @@ class FunctionScope(Scope):
             yield new_name_to_nodes
 
     @contextlib.contextmanager
-    def loop_scope(self) -> Iterator[SubScope]:
+    def loop_scope(self) -> Iterator[List[SubScope]]:
         loop_scopes = []
         with self.subscope() as main_scope:
             loop_scopes.append(main_scope)
             with qcore.override(self, "current_loop_scopes", loop_scopes):
-                yield main_scope
+                yield loop_scopes
         self.combine_subscopes(
             [
                 {name: values for name, values in scope.items() if name != LEAVES_LOOP}
@@ -1434,7 +1434,7 @@ class StackedScopes:
         """Creates a new subscope (see the :class:`FunctionScope` docstring)."""
         return self.scopes[-1].subscope()
 
-    def loop_scope(self) -> ContextManager[None]:
+    def loop_scope(self) -> ContextManager[List[SubScope]]:
         """Creates a new loop scope (see the :class:`FunctionScope` docstring)."""
         return self.scopes[-1].loop_scope()
 


Test patch:
diff --git a/pyanalyze/test_name_check_visitor.py b/pyanalyze/test_name_check_visitor.py
index 1a657ba..24550da 100644
--- a/pyanalyze/test_name_check_visitor.py
+++ b/pyanalyze/test_name_check_visitor.py
@@ -1655,6 +1655,19 @@ class TestUnusedIgnore(TestNameCheckVisitorBase):
             print(x)  # static analysis: ignore[undefined_name]
 
 
+class TestNestedLoop(TestNameCheckVisitorBase):
+    @assert_passes()
+    def test(self):
+        def capybara(x: int):
+            v = 1
+            while x < 2:
+                while True:
+                    if x == 0:
+                        assert_is_value(v, KnownValue(1) | KnownValue(2))
+                        break
+                v = 2
+
+
 class TestMissingF(TestNameCheckVisitorBase):
     @assert_passes()
     def test_no_error(self):