Propertly tree shake factory constructors.

An invocation of a factory constructor on class A does not necessarily
cause class A to be instantiated--it may redirect to a related or
unrelated class, or instantiate nothing at all.

We handle this by treating constructors similar to static function
calls--when a constructor invocation is found, the constructor element
is added to the queue; later, when the constructor element is
analyzed, we determine whether it instantiates the class or not based
on what kind of constructor it is.

R=brianwilkerson@google.com

Review URL: https://codereview.chromium.org//611153003

git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@40899 260f80e4-7a28-3924-810f-c04153c831b5

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) {
+    // 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);
+  }
 }

pkg/analyzer2dart/test/tree_shaker_test.dart

@@ -179,6 +179,74 @@ main() {
     helper.assertNoField('B.f1');
     helper.assertNoField('B.f2');
   });
+
+  test('Ordinary constructor with initializer list', () {
+    var helper = new TreeShakerTestHelper('''
+class A {
+  A() : x = f();
+  var x;
+  foo() {}
+}
+f() {}
+main() {
+  new A().foo();
+}
+''');
+    helper.assertHasMethod('A.foo');
+    helper.assertHasFunction('f');
+  });
+
+  test('Redirecting constructor', () {
+    var helper = new TreeShakerTestHelper('''
+class A {
+  A.a1() : this.a2();
+  A.a2();
+  foo() {}
+}
+main() {
+  new A.a1().foo();
+}
+''');
+    helper.assertHasMethod('A.foo');
+  });
+
+  test('Factory constructor', () {
+    var helper = new TreeShakerTestHelper('''
+class A {
+  factory A() {
+    return new B();
+  }
+  foo() {}
+}
+class B {
+  B();
+  foo() {}
+}
+main() {
+  new A().foo();
+}
+''');
+    helper.assertHasMethod('B.foo');
+    helper.assertNoMethod('A.foo');
+  });
+
+  test('Redirecting factory constructor', () {
+    var helper = new TreeShakerTestHelper('''
+class A {
+  factory A() = B;
+  foo() {}
+}
+class B {
+  B();
+  foo() {}
+}
+main() {
+  new A().foo();
+}
+''');
+    helper.assertHasMethod('B.foo');
+    helper.assertNoMethod('A.foo');
+  });
 }
 
 class TreeShakerTestHelper {