[dart2dart] Optionally cut types in variable declarations:

lib/compiler/implementation/dart_backend/placeholder_collector.dart

 // Copyright (c) 2012, 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.
 
 class LocalPlaceholder implements Hashable {
   final String identifier;
   final Set<Node> nodes;
   LocalPlaceholder(this.identifier) : nodes = new Set<Node>();
   int hashCode() => identifier.hashCode();
   String toString() =>
       'local_placeholder[id($identifier), nodes($nodes)]';
 }
 
 class FunctionScope {
   final Set<String> parameterIdentifiers;
   final Set<LocalPlaceholder> localPlaceholders;
   FunctionScope()
       : parameterIdentifiers = new Set<String>(),
       localPlaceholders = new Set<LocalPlaceholder>();
   void registerParameter(Identifier node) {
     parameterIdentifiers.add(node.source.slowToString());
   }
 }
 
+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;
 
   SendVisitor(this.collector, TreeElements elements) : super(elements);
 
   visitDynamicSend(Send node) {}
   visitSuperSend(Send node) {}
   visitOperatorSend(Send node) {}
   visitForeignSend(Send node) {}
 
@@ skipping 43 matching lines @@
   }
 }
 
 class PlaceholderCollector extends AbstractVisitor {
   final Compiler compiler;
   final Set<Node> nullNodes;  // Nodes that should not be in output.
   final Set<Identifier> unresolvedNodes;
   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;
 
   LibraryElement get coreLibrary => compiler.coreLibrary;
   FunctionElement get entryFunction => compiler.mainApp.find(Compiler.MAIN);
 
   PlaceholderCollector(this.compiler) :
       nullNodes = new Set<Node>(),
       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) {
     Node nameNode = constructor.name;
     if (nameNode is Send) nameNode = nameNode.receiver;
     if (nameNode.asIdentifier().token.slowToString()
         == element.name.slowToString()) {
       makeElementPlaceholder(nameNode, element);
     }
   }
@@ skipping 46 matching lines @@
     if (Elements.isStaticOrTopLevel(element)) {
       Node fieldNode = element.parseNode(compiler);
       if (fieldNode is Identifier) {
         makeElementPlaceholder(fieldNode, element);
       } else if (fieldNode is SendSet) {
         makeElementPlaceholder(fieldNode.selector, element);
       } else {
         unreachable();
       }
     }
+    makeVarDeclarationTypePlaceholder(node);
   }
 
   void collect(Element element, TreeElements elements) {
     treeElements = elements;
     Node elementNode;
     if (element is FunctionElement) {
       currentElement = element;
       elementNode = currentElement.parseNode(compiler);
       collectFunctionDeclarationPlaceholders(element, elementNode);
     } else if (element.isField()) {
@@ skipping 33 matching lines @@
           .registerParameter(node);
     } else if (Elements.isLocal(element)) {
       makeLocalPlaceholder(node);
     }
   }
 
   void makeTypePlaceholder(Node node, Type type) {
     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);
   }
 
   void makeElementPlaceholder(Node node, Element element) {
     assert(element !== null);
     if (element === entryFunction) return;
     if (element.getLibrary() === coreLibrary) return;
     if (isDartCoreLib(compiler, element.getLibrary())
@@ skipping 154 matching lines @@
     if (element !== null) {
       // Rename only local functions.
       if (element !== currentElement) {
         if (node.name !== null) {
           assert(node.name is Identifier);
           tryMakeLocalPlaceholder(element, node.name);
         }
       }
     }
     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) {
     ClassElement classElement = currentElement;
     makeElementPlaceholder(node.name, classElement);
     node.visitChildren(this);
     if (node.defaultClause !== null) {
       // Can't just visit class node's default clause because of the bug in the
       // resolver, it just crashes when it meets type variable.
       Type defaultType = classElement.defaultClass;
@@ skipping 28 matching lines @@
   visitTypeVariable(TypeVariable node) {
     assert(currentElement is TypedefElement || currentElement is ClassElement);
     tryResolveAndCollectTypeVariable(currentElement, node.name);
     node.visitChildren(this);
   }
 
   visitTypedef(Typedef node) {
     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);
   }
 }