Propertly tree shake factory constructors.

pkg/analyzer2dart/lib/src/tree_shaker.dart

Index: pkg/analyzer2dart/lib/src/tree_shaker.dart
diff --git a/pkg/analyzer2dart/lib/src/tree_shaker.dart b/pkg/analyzer2dart/lib/src/tree_shaker.dart
index 14a90c7009899e1a7678d8547c3da9945cdc7b25..899751c91aa38385594464d9e3cda9cbc730c808 100644
--- a/pkg/analyzer2dart/lib/src/tree_shaker.dart
+++ b/pkg/analyzer2dart/lib/src/tree_shaker.dart
@@ -75,8 +75,24 @@ class LocalReachabilityComputer {
    * Perform local reachability analysis of [method].
    */
   MethodAnalysis analyzeMethod(ExecutableElement method) {
-    MethodAnalysis analysis = new MethodAnalysis(method.node);
-    analysis.declaration.accept(new TreeShakingVisitor(analysis));
+    Declaration declaration = method.node;
+    MethodAnalysis analysis = new MethodAnalysis(declaration);
+    if (declaration != null) {
+      declaration.accept(new TreeShakingVisitor(analysis));
+    } else if (method is ConstructorElement) {
+      // This constructor has no associated declaration in the AST.  Either it
+      // is a default constructor for an ordinary class, or it's a synthetic
+      // constructor associated with a mixin.  For now we assume it's a default
+      // constructor, in which case all we need to do is record the class as
+      // being instantiated by this method.  TODO(paulberry): handle the
+      // mixin case.
+      analysis.instantiates.add(method.enclosingElement);
+    } else {
+      // This is an executable element with no associated declaration in the
+      // AST, and it's not a constructor.  TODO(paulberry): can this ever
+      // happen?
+      throw new UnimplementedError();
+    }
     return analysis;
   }
 
@@ -137,7 +153,8 @@ class TreeShaker {
   Set<Element> _alreadyEnqueued = new HashSet<Element>();
   ClosedWorld _world;
   Set<Selector> _selectors = new HashSet<Selector>();
-  final LocalReachabilityComputer _localComputer = new LocalReachabilityComputer();
+  final LocalReachabilityComputer _localComputer =
+      new LocalReachabilityComputer();
 
   TreeShaker(FunctionElement mainFunction)
       : _world = new ClosedWorld(mainFunction);
@@ -199,7 +216,7 @@ class TreeShaker {
       } else {
         throw new Exception(
             'Unexpected element type while tree shaking: '
-            '$element (${element.runtimeType})');
+                '$element (${element.runtimeType})');
       }
     }
     print('Tree shaking done');
@@ -218,10 +235,7 @@ class TreeShakingVisitor extends SemanticVisitor {
   void visitInstanceCreationExpression(InstanceCreationExpression node) {
     ConstructorElement staticElement = node.staticElement;
     if (staticElement != null) {
-      // TODO(paulberry): Really we should enqueue the constructor, and then
-      // when we visit it add the class to the class bucket.
-      ClassElement classElement = staticElement.enclosingElement;
-      analysis.instantiates.add(classElement);
+      analysis.calls.add(staticElement);
     } else {
       // TODO(paulberry): deal with this situation.  This can happen, for
       // example, in the case "main() => new Unresolved();" (which is a
@@ -232,45 +246,44 @@ class TreeShakingVisitor extends SemanticVisitor {
 
   @override
   void visitDynamicInvocation(MethodInvocation node,
-                              AccessSemantics semantics) {
+      AccessSemantics semantics) {
     analysis.invokes.add(
         createSelectorFromMethodInvocation(node, node.methodName.name));
   }
 
   @override
   void visitLocalFunctionInvocation(MethodInvocation node,
-                                    AccessSemantics semantics) {
+      AccessSemantics semantics) {
     // Locals don't need to be tree shaken.
   }
 
   @override
   void visitLocalVariableInvocation(MethodInvocation node,
-                                    AccessSemantics semantics) {
+      AccessSemantics semantics) {
     // Locals don't need to be tree shaken.
   }
 
   @override
   void visitParameterInvocation(MethodInvocation node,
-                                AccessSemantics semantics) {
+      AccessSemantics semantics) {
     // Locals don't need to be tree shaken.
   }
 
   @override
   void visitStaticFieldInvocation(MethodInvocation node,
-                                  AccessSemantics semantics) {
+      AccessSemantics semantics) {
     // Invocation of a static field.
     analysis.accesses.add(semantics.element);
-    analysis.invokes.add(
-      createSelectorFromMethodInvocation(node, 'call'));
+    analysis.invokes.add(createSelectorFromMethodInvocation(node, 'call'));
   }
 
   void visitStaticMethodInvocation(MethodInvocation node,
-                                   AccessSemantics semantics) {
+      AccessSemantics semantics) {
     analysis.calls.add(semantics.element);
   }
 
   void visitStaticPropertyInvocation(MethodInvocation node,
-                                     AccessSemantics semantics) {
+      AccessSemantics semantics) {
     // Invocation of a property.  TODO(paulberry): handle this.
     super.visitStaticPropertyInvocation(node, semantics);
   }
@@ -311,4 +324,38 @@ class TreeShakingVisitor extends SemanticVisitor {
     // TODO(paulberry): implement.
     super.visitStaticPropertyAccess(node, semantics);
   }
+
+  @override
+  void visitConstructorDeclaration(ConstructorDeclaration node) {

[Johnni Winther, 2014/10/03 08:03:06]

Currently we only mark classes as instantiated but we need to be more precise
and register directly instantiated type also in the face of redirecting factory
constructors. 

For instance we should know that only Foo<int> has been (directly) instantiated
and Bar<List<int>> has been indirectly instantiated in the following:

class Bar<T> {}
class Foo<T> extends Bar<List<T>> implements Baz<T> {}
class Baz<T> {
  factory Baz() = Foo<T>;
}
main() => new Baz<int>();

[Paul Berry, 2014/10/03 15:50:45]

Ok.  Currently I am only keeping track of what's being directly instantiated.  I
believe this CL moves us into a positive direction since it the behavior without
the CL is to consider Baz<int> to be directly instantiated (which is completely
wrong) and the behavior with the CL is to consider Foo<int> to be directly
instantiated (which is correct, insofar as direct instantiation goes).  So I'm
going to go ahead and commit this CL.

I had been avoiding the issue of "indirectly instantiated" because I don't
completely understand what it means yet.  I'll send you a separate email with
clarifying questions about that.

+    // TODO(paulberry): handle parameter list.
+    node.initializers.accept(this);
+    node.body.accept(this);
+    if (node.factoryKeyword == null) {
+      // This is a generative constructor.  Figure out if it is redirecting.
+      // If it isn't, then the constructor instantiates the class so we need to
+      // add the class to analysis.instantiates.  (If it is redirecting, then
+      // we don't need to, because the redirected-to constructor will take care
+      // of that).
+      if (node.initializers.length != 1 || node.initializers[0] is! RedirectingConstructorInvocation) {
+        analysis.instantiates.add(node.element.enclosingElement);
+      }
+    } else if (node.redirectedConstructor != null) {
+      if (node.redirectedConstructor.staticElement == null) {
+        // Factory constructor redirects to a non-existent constructor.
+        // TODO(paulberry): handle this.
+        throw new UnimplementedError();
+      } else {
+        analysis.calls.add(node.redirectedConstructor.staticElement);
+      }
+    }
+  }
+
+  @override
+  void
+      visitRedirectingConstructorInvocation(RedirectingConstructorInvocation node) {
+    // Note: we don't have to worry about node.staticElement being
+    // null, because that would have been detected by the analyzer and
+    // reported as a compile time error.
+    analysis.calls.add(node.staticElement);
+  }
 }