Overhaul tree IR visitor and rename IR classes.

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

 // Copyright (c) 2013, 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.
 
 // IrNodes are kept in a separate library to have precise control over their
 // dependencies on other parts of the system.
 library dart2js.ir_nodes;
 
 import '../constants/expressions.dart';
 import '../constants/values.dart' as values show ConstantValue;
@@ skipping 711 matching lines @@
 
   bool get isReturnContinuation => body == null;
 
   Continuation(this.parameters);
 
   Continuation.retrn() : parameters = <Parameter>[new Parameter(null)];
 
   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);
 
   FieldDefinition.withoutInitializer(this.element)
       : 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.
 class MutableVariable extends Definition {
   /// Body of source code that declares this mutable variable.
   ExecutableElement host;
   Entity hint;
 
   MutableVariable(this.host, this.hint);
 
   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.
   final List<ConstantExpression> defaultParameterValues;
 
   FunctionDefinition(this.element,
       this.thisParameter,
       this.parameters,
       this.body,
       this.localConstants,
       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);
 }
 
 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
 /// [Type].
 class ReifyRuntimeType extends Primitive implements JsSpecificNode {
   /// Reference to the internal representation of a type (as produced, for
   /// example, by [ReadTypeVariable]).
   final Reference<Primitive> value;
   ReifyRuntimeType(Primitive value)
     : this.value = new Reference<Primitive>(value);
@@ skipping 46 matching lines @@
 
 abstract class Visitor<T> {
   const Visitor();
 
   T visit(Node node);
 
   // Concrete classes.
   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);
   T visitSuperInitializer(SuperInitializer node);
 
   // Expressions.
   T visitLetPrim(LetPrim node);
   T visitLetCont(LetCont node);
   T visitLetHandler(LetHandler node);
   T visitLetMutable(LetMutable node);
@@ skipping 40 matching lines @@
 
 /// Recursively visits the entire CPS term, and calls abstract `process*`
 /// (i.e. `processLetPrim`) functions in pre-order.
 class RecursiveVisitor implements Visitor {
   const RecursiveVisitor();
 
   visit(Node node) => node.accept(this);
 
   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);
   }
 
   processFieldDefinition(FieldDefinition node) {}
   visitFieldDefinition(FieldDefinition node) {
     processFieldDefinition(node);
-    if (node.hasInitializer) {
+    if (node.body != null) {
       visit(node.body);
     }
   }
 
   processFunctionDefinition(FunctionDefinition node) {}
   visitFunctionDefinition(FunctionDefinition node) {
     processFunctionDefinition(node);
     if (node.thisParameter != null) visit(node.thisParameter);
     node.parameters.forEach(visit);
-    if (!node.isAbstract) {
+    if (node.body != null) {
       visit(node.body);
     }
   }
 
   processConstructorDefinition(ConstructorDefinition node) {}
   visitConstructorDefinition(ConstructorDefinition node) {
     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);
     }
   }
 
   processFieldInitializer(FieldInitializer node) {}
   visitFieldInitializer(FieldInitializer node) {
     processFieldInitializer(node);
     visit(node.body);
   }
 
@@ skipping 219 matching lines @@
     processReference(node.target);
   }
 
   processTypeExpression(TypeExpression node) {}
   @override
   visitTypeExpression(TypeExpression node) {
     processTypeExpression(node);
     node.arguments.forEach(processReference);
   }
 }