[dart2dart] Optionally cut types in variable declarations:

lib/compiler/implementation/dart_backend/placeholder_collector.dart

Index: lib/compiler/implementation/dart_backend/placeholder_collector.dart
diff --git a/lib/compiler/implementation/dart_backend/placeholder_collector.dart b/lib/compiler/implementation/dart_backend/placeholder_collector.dart
index ed3aef30abc2486db74b1ee3a74a473e9630abed..6cee3c01f308d316479c5f700b894076d6af74f4 100644
--- a/lib/compiler/implementation/dart_backend/placeholder_collector.dart
+++ b/lib/compiler/implementation/dart_backend/placeholder_collector.dart
@@ -22,6 +22,12 @@ class FunctionScope {
   }
 }
 
+class DeclarationTypePlaceholder {
+  final TypeAnnotation typeNode;
+  final bool canOmitType;

[Anton Muhin, 2012/08/28 09:13:37]

canOmitType is somewhat misnomer as in the case of final or const, one must omit
var decl, care provide some other names, something lik requiresVar?

[Roman, 2012/08/28 12:49:30]

Done.

+  DeclarationTypePlaceholder(this.typeNode, this.canOmitType);
+}
+
 class SendVisitor extends ResolvedVisitor {
   final PlaceholderCollector collector;
 
@@ -85,6 +91,7 @@ class PlaceholderCollector extends AbstractVisitor {
   final Map<Element, Set<Node>> elementNodes;
   final Map<FunctionElement, FunctionScope> functionScopes;
   final Map<LibraryElement, Set<Identifier>> privateNodes;
+  final List<DeclarationTypePlaceholder> declarationTypePlaceholders;
   Map<String, LocalPlaceholder> currentLocalPlaceholders;
   Element currentElement;
   TreeElements treeElements;
@@ -97,7 +104,8 @@ class PlaceholderCollector extends AbstractVisitor {
       unresolvedNodes = new Set<Identifier>(),
       elementNodes = new Map<Element, Set<Node>>(),
       functionScopes = new Map<FunctionElement, FunctionScope>(),
-      privateNodes = new Map<LibraryElement, Set<Identifier>>();
+      privateNodes = new Map<LibraryElement, Set<Identifier>>(),
+      declarationTypePlaceholders = new List<DeclarationTypePlaceholder>();
 
   void tryMakeConstructorNamePlaceholder(
       FunctionExpression constructor, ClassElement element) {
@@ -164,6 +172,7 @@ class PlaceholderCollector extends AbstractVisitor {
         unreachable();
       }
     }
+    makeVarDeclarationTypePlaceholder(node);
   }
 
   void collect(Element element, TreeElements elements) {
@@ -217,6 +226,18 @@ class PlaceholderCollector extends AbstractVisitor {
     makeElementPlaceholder(node, type.element);
   }
 
+  void makeDeclarationTypePlaceholder(TypeAnnotation type, bool canOmitType) {

[Anton Muhin, 2012/08/28 09:13:37]

it looks like the only place where canOmitType is true is
makeVarDeclarationPlaceholder.  Given that most probably node.type in this case
cannot be null, can you drop this argument and inline necessary logici nto
makeVDTP?

[Roman, 2012/08/28 12:49:30]

I can, but I don't see any benefits. I will save one argument, but will make
code duplication in makeVarDeclarationTypePlaceholder, also method
makeDeclarationTypePlaceholder would hide that the meaning is that I can omit
type, I will need to rename it.

[Anton Muhin, 2012/08/28 13:06:32]

That's not the matter of saving argument, but the fact that in all other places
types can be omitted which is now immediately seen.

On 2012/08/28 12:49:30, Roman wrote:

[Roman, 2012/08/28 13:27:51]

What about renaming this method to
makeOmitDeclarationTypePlaceholder(TypeAnnotation type) ? I want either this
method name to clearly tell that type can be omitted or have a parameter for
omitting.

[Anton Muhin, 2012/08/28 13:37:23]

I like the idea with renaming


On 2012/08/28 13:27:51, Roman wrote:

[Roman, 2012/08/28 14:48:55]

Done.

+    if (type === null) return;
+    declarationTypePlaceholders.add(
+        new DeclarationTypePlaceholder(type, canOmitType));
+  }
+
+  void makeVarDeclarationTypePlaceholder(VariableDefinitions node) {
+    Element definitionElement = treeElements[node.definitions.nodes.head];
+    bool canOmitType = node.modifiers.isFinalOrConst();
+    makeDeclarationTypePlaceholder(node.type, canOmitType);
+  }
+
   void makeNullPlaceholder(Node node) {
     assert(node is Identifier || node is Send);
     nullNodes.add(node);
@@ -391,6 +412,22 @@ class PlaceholderCollector extends AbstractVisitor {
       }
     }
     node.visitChildren(this);
+    makeDeclarationTypePlaceholder(node.returnType, true);
+    collectFunctionParameters(node.parameters);
+  }
+
+  void collectFunctionParameters(NodeList parameters) {
+    if (parameters === null) return;
+    for (Link<Node> link = parameters.nodes; !link.isEmpty(); link= link.tail) {

[Anton Muhin, 2012/08/28 09:13:37]

for (Node parameter in parameters) ?

[Anton Muhin, 2012/08/28 09:13:37]

nit: space before =

[Roman, 2012/08/28 12:49:30]

Can't do this, the structure is: first several Node-s are VariableDefinitions
representing required parameters, then there's a NodeList that has
VariableDefinitions representing optional parameters.
Another option is to do something like this:

for(Node node in parameters.nodes) {
  if (node is NodeList) {
    // Optional parameters.
    collectFunctionParameters(node);
  } else {
    // Required parameter.
    makeDeclarationTypePlaceholder...
  }
}

[Roman, 2012/08/28 12:49:30]

Done.

[Anton Muhin, 2012/08/28 13:06:32]

Yes, this looks more readable to me.

On 2012/08/28 12:49:30, Roman wrote:

[Roman, 2012/08/28 13:27:51]

Sorry, which one looks more readable?

[Anton Muhin, 2012/08/28 13:37:23]

Sorry, with for loop and inner dispatch, one in your comments.

On 2012/08/28 13:27:51, Roman wrote:

[Roman, 2012/08/28 14:48:55]

Done.

+      Node parameter = link.head;
+      if (parameter is NodeList) {
+        link = parameter.nodes;
+        parameter = link.head;
+      }
+      assert(parameter is VariableDefinitions);
+      makeDeclarationTypePlaceholder(
+          (parameter as VariableDefinitions).type, true);

[Anton Muhin, 2012/08/28 09:13:37]

parameter.asVariableDefinitions() ?

[Roman, 2012/08/28 12:49:30]

Done.

+    }
   }
 
   visitClassNode(ClassNode node) {
@@ -439,5 +476,16 @@ class PlaceholderCollector extends AbstractVisitor {
     assert(currentElement is TypedefElement);
     makeElementPlaceholder(node.name, currentElement);
     node.visitChildren(this);
+    makeDeclarationTypePlaceholder(node.returnType, true);
+    collectFunctionParameters(node.formals);
+  }
+
+  visitBlock(Block node) {
+    node.statements.nodes.forEach((Node statement) {

[Anton Muhin, 2012/08/28 09:13:37]

plain for loop?

[Roman, 2012/08/28 12:49:30]

Done.

+      if (statement is VariableDefinitions) {
+        makeVarDeclarationTypePlaceholder(statement);

[Anton Muhin, 2012/08/28 09:13:37]

why this cannot be a part of visitVariableDefinitions?

[Roman, 2012/08/28 12:49:30]

As discussed, there's a catch statement that I want to skip.
Also, in visitVariableDefinitions I must decide whether declaration requires any
type or 'var', I will have problems if variable definitions node is not resolved
in case of closure function parameters.
Added a todo to maybe move this to visitVariableDefinitions if resolver becomes
better and/or catch syntax changes.

+      }
+    });
+    node.visitChildren(this);
   }
 }