Fix several bugs in closure conversion.

pkg/kernel/lib/ast.dart

 // Copyright (c) 2016, 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.
 
 /// -----------------------------------------------------------------------
 ///                           ERROR HANDLING
 /// -----------------------------------------------------------------------
 ///
 /// As a rule of thumb, errors that can be detected statically are handled by
 /// the frontend, typically by translating the erroneous code into a 'throw' or
@@ skipping 3160 matching lines @@
   }
 }
 
 // ------------------------------------------------------------------------
 //                              STATEMENTS
 // ------------------------------------------------------------------------
 
 abstract class Statement extends TreeNode {
   accept(StatementVisitor v);
   accept1(StatementVisitor1 v, arg);
+  bool get isLoop => false;

[Dmitry Stefantsov, 2017/08/24 09:41:13]

Personally, I don't like modifying AST definitions because of just one
transformation.  I believe, changes here should be justified universally.
 I would prefer defining a function `bool isLoop(Statement stmt)` in
`pkg/kernel/lib/transformations/closure/converter.dart` (currently, the only
place that needs to distinguish between loop statements and non-loop
statements).

[sjindel, 2017/08/24 11:11:43]

I understand that, but I feel like determining whether a statement represents a
loop is a pretty general operation. It doesn't change the binary format anyway
-- it's just a helper function.

[Dmitry Stefantsov, 2017/08/24 11:49:57]

I agree that it's a general operation and that it doesn't change the binary
representation, but I'd prefer to have it as a part of a bigger update that
brings some kind of systematic categorization to AST nodes.  This file serves as
a reference to Kernel syntax, and auxiliary methods may bring unnecessary
questioning about the design of the syntax.

[sjindel, 2017/08/24 14:50:13]

Done.

 }
 
 /// A statement with a compile-time error.
 ///
 /// Should throw an exception at runtime.
 class InvalidStatement extends Statement {
   accept(StatementVisitor v) => v.visitInvalidStatement(this);
   accept1(StatementVisitor1 v, arg) => v.visitInvalidStatement(this, arg);
 
   visitChildren(Visitor v) {}
@@ skipping 165 matching lines @@
   transformChildren(Transformer v) {
     if (condition != null) {
       condition = condition.accept(v);
       condition?.parent = this;
     }
     if (body != null) {
       body = body.accept(v);
       body?.parent = this;
     }
   }
+
+  bool get isLoop => true;
 }
 
 class DoStatement extends Statement {
   Statement body;
   Expression condition;
 
   DoStatement(this.body, this.condition) {
     body?.parent = this;
     condition?.parent = this;
   }
 
   accept(StatementVisitor v) => v.visitDoStatement(this);
   accept1(StatementVisitor1 v, arg) => v.visitDoStatement(this, arg);
 
   visitChildren(Visitor v) {
     body?.accept(v);
     condition?.accept(v);
   }
 
   transformChildren(Transformer v) {
     if (body != null) {
       body = body.accept(v);
       body?.parent = this;
     }
     if (condition != null) {
       condition = condition.accept(v);
       condition?.parent = this;
     }
   }
+
+  bool get isLoop => true;
 }
 
 class ForStatement extends Statement {
   final List<VariableDeclaration> variables; // May be empty, but not null.
   Expression condition; // May be null.
   final List<Expression> updates; // May be empty, but not null.
   Statement body;
 
   ForStatement(this.variables, this.condition, this.updates, this.body) {
     setParents(variables, this);
@@ skipping 17 matching lines @@
     if (condition != null) {
       condition = condition.accept(v);
       condition?.parent = this;
     }
     transformList(updates, v, this);
     if (body != null) {
       body = body.accept(v);
       body?.parent = this;
     }
   }
+
+  bool get isLoop => true;
 }
 
 class ForInStatement extends Statement {
   /// Offset in the source file it comes from.
   ///
   /// Valid values are from 0 and up, or -1 ([TreeNode.noOffset]) if the file
   /// offset is not available (this is the default if none is specifically set).
   int bodyOffset = TreeNode.noOffset;
 
   VariableDeclaration variable; // Has no initializer.
@@ skipping 24 matching lines @@
     }
     if (iterable != null) {
       iterable = iterable.accept(v);
       iterable?.parent = this;
     }
     if (body != null) {
       body = body.accept(v);
       body?.parent = this;
     }
   }
+
+  bool get isLoop => true;
 }
 
 /// Statement of form `switch (e) { case x: ... }`.
 ///
 /// Adjacent case clauses have been merged into a single [SwitchCase]. A runtime
 /// exception must be thrown if one [SwitchCase] falls through to another case.
 class SwitchStatement extends Statement {
   Expression expression;
   final List<SwitchCase> cases;
 
@@ skipping 1257 matching lines @@
   if (typedef_.canonicalName == null) {
     throw '$typedef_ has no canonical name';
   }
   return typedef_.canonicalName;
 }
 
 /// Annotation describing information which is not part of Dart semantics; in
 /// other words, if this information (or any information it refers to) changes,
 /// static analysis and runtime behavior of the library are unaffected.
 const informative = null;