ExitDetector: Examine continue when determining a 'do'

pkg/analyzer/lib/src/generated/resolver.dart

 // Copyright (c) 2014, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library analyzer.src.generated.resolver;
 
 import 'dart:collection';
 
 import 'package:analyzer/dart/ast/ast.dart';
 import 'package:analyzer/dart/ast/token.dart';
@@ skipping 3481 matching lines @@
  * expression, or simple infinite loop such as `while(true)`.
  */
 class ExitDetector extends GeneralizingAstVisitor<bool> {
   /**
    * Set to `true` when a `break` is encountered, and reset to `false` when a
    * `do`, `while`, `for` or `switch` block is entered.
    */
   bool _enclosingBlockContainsBreak = false;
 
   /**
+   * Set to `true` when a `continue` is encountered, and reset to `false` when a
+   * `do`, `while`, `for` or `switch` block is entered.
+   */
+  bool _enclosingBlockContainsContinue = false;
+
+  /**
    * Add node when a labelled `break` is encountered.
    */
   Set<AstNode> _enclosingBlockBreaksLabel = new Set<AstNode>();
 
   @override
   bool visitArgumentList(ArgumentList node) =>
       _visitExpressions(node.arguments);
 
   @override
   bool visitAsExpression(AsExpression node) => _nodeExits(node.expression);
@@ skipping 87 matching lines @@
     if (_nodeExits(conditionExpression)) {
       return true;
     }
     if (thenStatement == null || elseStatement == null) {
       return false;
     }
     return thenStatement.accept(this) && elseStatement.accept(this);
   }
 
   @override
-  bool visitContinueStatement(ContinueStatement node) => false;
+  bool visitContinueStatement(ContinueStatement node) {
+    _enclosingBlockContainsContinue = true;
+    return false;
+  }
 
   @override
   bool visitDoStatement(DoStatement node) {
     bool outerBreakValue = _enclosingBlockContainsBreak;
+    bool outerContinueValue = _enclosingBlockContainsContinue;
     _enclosingBlockContainsBreak = false;
+    _enclosingBlockContainsContinue = false;
     try {
-      if (_nodeExits(node.body) && !_enclosingBlockContainsBreak) {
+      bool bodyExits = _nodeExits(node.body);
+      bool containsBreakOrContinue =
+          _enclosingBlockContainsBreak || _enclosingBlockContainsContinue;
+      // Even if we determine that the body "exits", there might be break or
+      // continue statements that actually mean it _doesn't_ always exit.
+      if (bodyExits && !containsBreakOrContinue) {
         return true;
       }
       Expression conditionExpression = node.condition;
       if (_nodeExits(conditionExpression)) {
         return true;
       }
       // TODO(jwren) Do we want to take all constant expressions into account?
       if (conditionExpression is BooleanLiteral) {
         // If do {} while (true), and the body doesn't break, then return true.
         if (conditionExpression.value && !_enclosingBlockContainsBreak) {
           return true;
         }
       }
       return false;
     } finally {
       _enclosingBlockContainsBreak = outerBreakValue;
+      _enclosingBlockContainsContinue = outerContinueValue;
     }
   }
 
   @override
   bool visitEmptyStatement(EmptyStatement node) => false;
 
   @override
   bool visitExpressionStatement(ExpressionStatement node) =>
       _nodeExits(node.expression);
 
   @override
   bool visitForEachStatement(ForEachStatement node) {
     bool outerBreakValue = _enclosingBlockContainsBreak;
+    bool outerContinueValue = _enclosingBlockContainsContinue;
     _enclosingBlockContainsBreak = false;
+    _enclosingBlockContainsContinue = false;
     try {
       return _nodeExits(node.iterable);
     } finally {
       _enclosingBlockContainsBreak = outerBreakValue;
+      _enclosingBlockContainsContinue = outerContinueValue;
     }
   }
 
   @override
   bool visitForStatement(ForStatement node) {
     bool outerBreakValue = _enclosingBlockContainsBreak;
+    bool outerContinueValue = _enclosingBlockContainsContinue;
     _enclosingBlockContainsBreak = false;
+    _enclosingBlockContainsContinue = false;
     try {
       if (node.variables != null &&
           _visitVariableDeclarations(node.variables.variables)) {
         return true;
       }
       if (node.initialization != null && _nodeExits(node.initialization)) {
         return true;
       }
       Expression conditionExpression = node.condition;
       if (conditionExpression != null && _nodeExits(conditionExpression)) {
         return true;
       }
       if (_visitExpressions(node.updaters)) {
         return true;
       }
       bool blockReturns = _nodeExits(node.body);
       // TODO(jwren) Do we want to take all constant expressions into account?
       // If for(; true; ) (or for(;;)), and the body doesn't return or the body
       // doesn't have a break, then return true.
       bool implicitOrExplictTrue = conditionExpression == null ||
           (conditionExpression is BooleanLiteral && conditionExpression.value);
       if (implicitOrExplictTrue) {
         if (blockReturns || !_enclosingBlockContainsBreak) {
           return true;
         }
       }
       return false;
     } finally {
       _enclosingBlockContainsBreak = outerBreakValue;
+      _enclosingBlockContainsContinue = outerContinueValue;
     }
   }
 
   @override
   bool visitFunctionDeclarationStatement(FunctionDeclarationStatement node) =>
       false;
 
   @override
   bool visitFunctionExpression(FunctionExpression node) => false;
 
@@ skipping 120 matching lines @@
   @override
   bool visitSwitchCase(SwitchCase node) => _visitStatements(node.statements);
 
   @override
   bool visitSwitchDefault(SwitchDefault node) =>
       _visitStatements(node.statements);
 
   @override
   bool visitSwitchStatement(SwitchStatement node) {
     bool outerBreakValue = _enclosingBlockContainsBreak;
+    bool outerContinueValue = _enclosingBlockContainsContinue;

[Paul Berry, 2016/06/28 19:17:41]

I don't think this method should be modified.  Switch statements only affect the
target of "break" statements, not "continue" statements.  So for instance I
think with your code as written, we would incorrectly classify the following
code as exiting:

do {
  switch (x) {
    case 0:
      continue;
  }
  throw 'foo';
} while (false);

(Assuming I'm right about this, we should add a test case to verify that the
above code is handled correctly).

[srawlins, 2016/06/28 20:00:55]

Done. With testcase.

     _enclosingBlockContainsBreak = false;
+    _enclosingBlockContainsContinue = false;
     try {
       bool hasDefault = false;
       bool hasNonExitingCase = false;
       List<SwitchMember> members = node.members;
       for (int i = 0; i < members.length; i++) {
         SwitchMember switchMember = members[i];
         if (switchMember is SwitchDefault) {
           hasDefault = true;
           // If this is the last member and there are no statements, then it
           // does not exit.
           if (switchMember.statements.isEmpty && i + 1 == members.length) {
             hasNonExitingCase = true;
             continue;
           }
         }
         // For switch members with no statements, don't visit the children.
         // Otherwise, if there children statements don't exit, mark this as a
         // non-exiting case.
         if (!switchMember.statements.isEmpty && !switchMember.accept(this)) {
           hasNonExitingCase = true;
         }
       }
       if (hasNonExitingCase) {
         return false;
       }
       // As all cases exit, return whether that list includes `default`.
       return hasDefault;
     } finally {
       _enclosingBlockContainsBreak = outerBreakValue;
+      _enclosingBlockContainsContinue = outerContinueValue;
     }
   }
 
   @override
   bool visitThisExpression(ThisExpression node) => false;
 
   @override
   bool visitThrowExpression(ThrowExpression node) => true;
 
   @override
@@ skipping 35 matching lines @@
       if (variables[i].accept(this)) {
         return true;
       }
     }
     return false;
   }
 
   @override
   bool visitWhileStatement(WhileStatement node) {
     bool outerBreakValue = _enclosingBlockContainsBreak;
+    bool outerContinueValue = _enclosingBlockContainsContinue;
     _enclosingBlockContainsBreak = false;
+    _enclosingBlockContainsContinue = false;
     try {
       Expression conditionExpression = node.condition;
       if (conditionExpression.accept(this)) {
         return true;
       }
       node.body.accept(this);
       // TODO(jwren) Do we want to take all constant expressions into account?
       if (conditionExpression is BooleanLiteral) {
         // If while(true), and the body doesn't have a break, then return true.
         // The body might be found to exit, but if there are any break
         // statements, then it is a faulty finding. In other words:
         //
         // * If the body exits, and does not contain a break statement, then
         //   it exits.
         // * If the body does not exit, and does not contain a break statement,
         //   then it loops infinitely (also an exit).
         //
         // As both conditions forbid any break statements to be found, the logic
         // just boils down to checking [_enclosingBlockContainsBreak].
         if (conditionExpression.value && !_enclosingBlockContainsBreak) {
           return true;
         }
       }
       return false;
     } finally {
       _enclosingBlockContainsBreak = outerBreakValue;
+      _enclosingBlockContainsContinue = outerContinueValue;
     }
   }
 
   @override
   bool visitYieldStatement(YieldStatement node) => _nodeExits(node.expression);
 
   /**
    * Return `true` if the given node exits.
    *
    * @param node the node being tested
@@ skipping 7091 matching lines @@
       return null;
     }
     if (identical(node.staticElement, variable)) {
       if (node.inSetterContext()) {
         result = true;
       }
     }
     return null;
   }
 }