Overhaul tree IR visitor and rename IR classes.

pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart

Index: pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart
diff --git a/pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart b/pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart
index 16cf74de280e6655628e0fb5ca2fc86bc71fdbeb..d0795f79b49a1438b66b17b1f49524f7b871b058 100644
--- a/pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart
+++ b/pkg/compiler/lib/src/cps_ir/cps_ir_nodes.dart
@@ -729,19 +729,19 @@ class Continuation extends Definition<Continuation> implements InteriorNode {
   accept(Visitor visitor) => visitor.visitContinuation(this);
 }
 
-abstract class ExecutableDefinition implements Node {
-  RunnableBody get body;
+abstract class RootNode extends Node {
   Element get element;
-
-  applyPass(Pass pass);
+  bool get isEmpty;

[Kevin Millikin (Google), 2015/04/08 15:10:15]

This makes me wonder what it means to be an empty root node.  When I look at the
implementations and see that it exactly means that it doesn't have a body.  That
makes me wonder if it wouldn't be better to just go ahead and expose the field
body here in this class, and even to use x.body == null instead of x.isEmpty?

[asgerf, 2015/04/09 09:58:23]

I removed the body getter because constructors have more than one body, and
there is almost no legitimate use of the 'body' that does not look at the
initializer bodies (except emptiness-checking).

I copied the comment from the tree.RootNode in here.

+  List<Definition> get parameters;

[Kevin Millikin (Google), 2015/04/08 15:10:15]

It looks like at the cost of a field per FieldDefinition this could just be a
field in this class.

[asgerf, 2015/04/09 09:58:23]

But then we don't get to use an initializing formal `this.parameters` in the
subclasses, and the field can't be final unless we use a superconstructor call
to initialize it.

 }
 
 // This is basically a function definition with an empty parameter list and a
 // field element instead of a function element and no const declarations, and
 // never a getter or setter, though that's less important.
-class FieldDefinition extends Node implements ExecutableDefinition {
+class FieldDefinition extends RootNode implements DartSpecificNode {
   final FieldElement element;
-  RunnableBody body;
+  List<Definition> get parameters => const <Definition>[];
+  Body body;

[Kevin Millikin (Google), 2015/04/08 15:10:15]

Can this be final?

[asgerf, 2015/04/09 09:58:23]

Yes.

 
   FieldDefinition(this.element, this.body);
 
@@ -749,26 +749,8 @@ class FieldDefinition extends Node implements ExecutableDefinition {
       : this.body = null;
 
   accept(Visitor visitor) => visitor.visitFieldDefinition(this);
-  applyPass(Pass pass) => pass.rewriteFieldDefinition(this);
 
-  /// `true` if this field has no initializer.
-  ///
-  /// If `true` [body] is `null`.
-  ///
-  /// This is different from a initializer that is `null`. Consider this class:
-  ///
-  ///     class Class {
-  ///       final field;
-  ///       Class.a(this.field);
-  ///       Class.b() : this.field = null;
-  ///       Class.c();
-  ///     }
-  ///
-  /// If `field` had an initializer, possibly `null`, constructors `Class.a` and
-  /// `Class.b` would be invalid, and since `field` has no initializer
-  /// constructor `Class.c` is invalid. We therefore need to distinguish the two
-  /// cases.
-  bool get hasInitializer => body != null;
+  bool get isEmpty => body == null;
 }
 
 /// Identifies a mutable variable.
@@ -782,22 +764,21 @@ class MutableVariable extends Definition {
   accept(Visitor v) => v.visitMutableVariable(this);
 }
 
-class RunnableBody extends InteriorNode {
+class Body extends InteriorNode {
   Expression body;

[Kevin Millikin (Google), 2015/04/08 15:10:15]

This is still weird: Body has a field body which is not a Body.  I *would*
suggest that we call this the root, but that's taken.  How about expression?

[asgerf, 2015/04/09 09:58:23]

The field overrides InteriorNode.body so this would force us to also rename
LetCont.body, LetPrim.body, etc. It think we should hold on the 'body' field
name.

As for the class name, I wish something I could think of something better, but
nothing in my vocabulary describes something that has its own return
continuation but doesn't have its own parameter list. It's not even a
ClosedExpression because the parameters are free.

So I just defaulted to Body. I took out the Runnable prefix because it was just
noise. Plenty of others things are "bodies" and they are also runnable.

   final Continuation returnContinuation;
-  RunnableBody(this.body, this.returnContinuation);
-  accept(Visitor visitor) => visitor.visitRunnableBody(this);
+  Body(this.body, this.returnContinuation);
+  accept(Visitor visitor) => visitor.visitBody(this);
 }
 
 /// A function definition, consisting of parameters and a body.  The parameters
 /// include a distinguished continuation parameter (held by the body).
-class FunctionDefinition extends Node
-    implements ExecutableDefinition {
+class FunctionDefinition extends RootNode {
   final FunctionElement element;
   final Parameter thisParameter;
   /// Mixed list of [Parameter]s and [MutableVariable]s.
   final List<Definition> parameters;
-  final RunnableBody body;
+  final Body body;
   final List<ConstDeclaration> localConstants;
 
   /// Values for optional parameters.
@@ -811,26 +792,22 @@ class FunctionDefinition extends Node
       this.defaultParameterValues);
 
   FunctionDefinition.abstract(this.element,
-                              this.thisParameter,
                               this.parameters,
                               this.defaultParameterValues)
       : body = null,
+        thisParameter = null,
         localConstants = const <ConstDeclaration>[];
 
   accept(Visitor visitor) => visitor.visitFunctionDefinition(this);
-  applyPass(Pass pass) => pass.rewriteFunctionDefinition(this);
 
-  /// Returns `true` if this function is abstract or external.
-  ///
-  /// If `true`, [body] is `null` and [localConstants] is empty.
-  bool get isAbstract => body == null;
+  bool get isEmpty => body == null;
 }
 
 abstract class Initializer extends Node implements DartSpecificNode {}
 
 class FieldInitializer extends Initializer {
   final FieldElement element;
-  final RunnableBody body;
+  final Body body;
 
   FieldInitializer(this.element, this.body);
   accept(Visitor visitor) => visitor.visitFieldInitializer(this);
@@ -838,36 +815,46 @@ class FieldInitializer extends Initializer {
 
 class SuperInitializer extends Initializer {
   final ConstructorElement target;
-  final List<RunnableBody> arguments;
+  final List<Body> arguments;
   final Selector selector;
   SuperInitializer(this.target, this.arguments, this.selector);
   accept(Visitor visitor) => visitor.visitSuperInitializer(this);
 }
 
-class ConstructorDefinition extends FunctionDefinition {
+class ConstructorDefinition extends RootNode implements DartSpecificNode {
+  final ConstructorElement element;
+  final Parameter thisParameter;
+  /// Mixed list of [Parameter]s and [MutableVariable]s.
+  final List<Definition> parameters;
+  final Body body;
+  final List<ConstDeclaration> localConstants;
   final List<Initializer> initializers;
 
-  ConstructorDefinition(ConstructorElement element,
-                        Definition thisParameter, // only Dart
-                        List<Definition> parameters,
-                        RunnableBody body,
+  /// Values for optional parameters.
+  final List<ConstantExpression> defaultParameterValues;
+
+  ConstructorDefinition(this.element,
+                        this.thisParameter,
+                        this.parameters,
+                        this.body,
                         this.initializers,
-                        List<ConstDeclaration> localConstants,
-                        List<ConstantExpression> defaultParameterValues)
-  : super(element, thisParameter, parameters, body, localConstants,
-          defaultParameterValues);
+                        this.localConstants,
+                        this.defaultParameterValues);
 
   // 'Abstract' here means "has no body" and is used to represent external
   // constructors.
   ConstructorDefinition.abstract(
-      ConstructorElement element,
-      List<Definition> parameters,
-      List<ConstantExpression> defaultParameterValues)
-      : initializers = null,
-        super.abstract(element, null, parameters, defaultParameterValues);
+      this.element,
+      this.parameters,
+      this.defaultParameterValues)
+      : body = null,
+        initializers = null,
+        thisParameter = null,
+        localConstants = const <ConstDeclaration>[];
 
   accept(Visitor visitor) => visitor.visitConstructorDefinition(this);
-  applyPass(Pass pass) => pass.rewriteConstructorDefinition(this);
+
+  bool get isEmpty => body == null;
 }
 
 /// Converts the internal representation of a type to a Dart object of type
@@ -934,7 +921,7 @@ abstract class Visitor<T> {
   T visitFieldDefinition(FieldDefinition node);
   T visitFunctionDefinition(FunctionDefinition node);
   T visitConstructorDefinition(ConstructorDefinition node);
-  T visitRunnableBody(RunnableBody node);
+  T visitBody(Body node);
 
   // Initializers
   T visitFieldInitializer(FieldInitializer node);
@@ -995,9 +982,9 @@ class RecursiveVisitor implements Visitor {
 
   processReference(Reference ref) {}
 
-  processRunnableBody(RunnableBody node) {}
-  visitRunnableBody(RunnableBody node) {
-    processRunnableBody(node);
+  processBody(Body node) {}
+  visitBody(Body node) {
+    processBody(node);
     visit(node.returnContinuation);
     visit(node.body);
   }
@@ -1005,7 +992,7 @@ class RecursiveVisitor implements Visitor {
   processFieldDefinition(FieldDefinition node) {}
   visitFieldDefinition(FieldDefinition node) {
     processFieldDefinition(node);
-    if (node.hasInitializer) {
+    if (node.body != null) {
       visit(node.body);
     }
   }
@@ -1015,7 +1002,7 @@ class RecursiveVisitor implements Visitor {
     processFunctionDefinition(node);
     if (node.thisParameter != null) visit(node.thisParameter);
     node.parameters.forEach(visit);
-    if (!node.isAbstract) {
+    if (node.body != null) {
       visit(node.body);
     }
   }
@@ -1025,7 +1012,7 @@ class RecursiveVisitor implements Visitor {
     processConstructorDefinition(node);
     if (node.thisParameter != null) visit(node.thisParameter);
     node.parameters.forEach(visit);
-    if (!node.isAbstract) {
+    if (node.body != null) {
       node.initializers.forEach(visit);

[Kevin Millikin (Google), 2015/04/08 15:10:15]

It might be better not to assume that (node.body == null) == (node.initializers
== null) here and write:

if (node.initializers != null) {
  node.initializers.forEach(visit);
}
if (node.body != null) {
  visit(node.body);
}

[asgerf, 2015/04/09 09:58:23]

Done.

There are other places where we depend on a constructor being either empty or
non-empty and nothing in-between.

But I guess the visitor should be a bit more conservative so it can be used for
tracing/sexpressions/integrity/etc.

       visit(node.body);
     }