Add label support to ExitDetector

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

Index: pkg/analyzer/lib/src/generated/resolver.dart
diff --git a/pkg/analyzer/lib/src/generated/resolver.dart b/pkg/analyzer/lib/src/generated/resolver.dart
index 7dbf6e42c99492dbe9054a776fd77d0869c52b5e..89d2fb832b5eff5327ea2cb0fb5f48e273278c6a 100644
--- a/pkg/analyzer/lib/src/generated/resolver.dart
+++ b/pkg/analyzer/lib/src/generated/resolver.dart
@@ -3470,6 +3470,11 @@ class ExitDetector extends GeneralizingAstVisitor<bool> {
    */
   bool _enclosingBlockContainsBreak = false;
 
+  /**
+   * Add node when a labelled `break` is encountered.
+   */
+  Set<AstNode> _enclosingBlockBreaksLabel = new Set<AstNode>();

[srawlins, 2016/05/25 17:26:03]

This probably isn't a popular decision, whole AstNodes??? But I couldn't find
any better way to uniquely identify labels. Does the Identifier of the label
work? The String name?

Do labels need to be unique across a compilation unit or just a function?

[Brian Wilkerson, 2016/05/25 18:13:23]

As long as the ExitDetector is not long-lived, then it should be fine to store
the nodes.
Nope. You can have multiple labels with the same identifier (unless by
"identifier" you mean the SimpleIdentifier AST node, in which case that's also
unique but also an AST node).
Neither. The following is perfectly valid:

main() {
  label: while (true) {
    label: while (true) {
      break label;
    }
    break label;
  }
}

and is equivalent to:

main() {
  label1: while (true) {
    label2: while (true) {
      break label2;
    }
    break label1;
  }
}

[srawlins, 2016/05/25 20:45:23]

Acknowledged.

+
   @override
   bool visitArgumentList(ArgumentList node) =>
       _visitExpressions(node.arguments);
@@ -3545,6 +3550,9 @@ class ExitDetector extends GeneralizingAstVisitor<bool> {
   @override
   bool visitBreakStatement(BreakStatement node) {
     _enclosingBlockContainsBreak = true;
+    if (node.label != null) {
+      _enclosingBlockBreaksLabel.add(node.target);
+    }
     return false;
   }
 
@@ -3634,13 +3642,13 @@ class ExitDetector extends GeneralizingAstVisitor<bool> {
       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) {
-        bool blockReturns = _nodeExits(node.body);
         if (blockReturns || !_enclosingBlockContainsBreak) {
           return true;
         }
@@ -3680,18 +3688,18 @@ class ExitDetector extends GeneralizingAstVisitor<bool> {
     // TODO(jwren) Do we want to take all constant expressions into account?
     if (conditionExpression is BooleanLiteral) {
       if (conditionExpression.value) {
-        // if(true) ...
+        // if (true) ...
         return _nodeExits(thenStatement);
       } else if (elseStatement != null) {
         // if (false) ...
         return _nodeExits(elseStatement);
       }
     }
-    if (thenStatement == null || elseStatement == null) {
-      return false;
-    }
     bool thenExits = _nodeExits(thenStatement);

[srawlins, 2016/05/25 17:26:03]

Need to always visit `then` and `else`, to find possible break labels.

     bool elseExits = _nodeExits(elseStatement);
+    if (thenStatement == null || elseStatement == null) {
+      return false;
+    }
     return thenExits && elseExits;
   }
 
@@ -3718,8 +3726,16 @@ class ExitDetector extends GeneralizingAstVisitor<bool> {
   bool visitLabel(Label node) => false;
 
   @override
-  bool visitLabeledStatement(LabeledStatement node) =>
-      node.statement.accept(this);
+  bool visitLabeledStatement(LabeledStatement node) {
+    try {
+      bool statementExits = _nodeExits(node.statement);
+      bool neverBrokeFromLabel =
+          !_enclosingBlockBreaksLabel.contains(node.statement);
+      return statementExits && neverBrokeFromLabel;
+    } finally {
+      _enclosingBlockBreaksLabel.remove(node);

[Brian Wilkerson, 2016/05/25 18:13:23]

"node" --> "node.statement"?

[srawlins, 2016/05/25 20:45:23]

Done.

+    }
+  }
 
   @override
   bool visitLiteral(Literal node) => false;
@@ -3873,11 +3889,11 @@ class ExitDetector extends GeneralizingAstVisitor<bool> {
       if (conditionExpression.accept(this)) {
         return true;
       }
+      bool blockReturns = 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 return or the body doesn't have
         // a break, then return true.
-        bool blockReturns = node.body.accept(this);
         if (conditionExpression.value &&
             (blockReturns || !_enclosingBlockContainsBreak)) {
           return true;
@@ -3912,12 +3928,13 @@ class ExitDetector extends GeneralizingAstVisitor<bool> {
   }
 
   bool _visitStatements(NodeList<Statement> statements) {
+    var exits = false;
     for (int i = statements.length - 1; i >= 0; i--) {
       if (statements[i].accept(this)) {
-        return true;
+        exits = true;

[srawlins, 2016/05/25 17:26:03]

This may also be unpopular. We can't short circuit evaluating ALL of the
statements, because we want to catch and break labels inside.

Should this require a new visitor? Something complicated like "Visit the
statements until you find one that exits. Then visit the others with a
BreakLabelVisitor to catch break labels, ...

[Brian Wilkerson, 2016/05/25 18:13:23]

I don't understand why. If a statement always exits, then the code after it is
dead and any break inside it won't be hit:

label: while (true) {
  return;
  break label;
}

[srawlins, 2016/05/25 20:45:23]

OK it walks forwards now.

       }
     }
-    return false;
+    return exits;
   }
 
   bool _visitVariableDeclarations(