Propertly tree shake factory constructors.

pkg/analyzer2dart/lib/src/tree_shaker.dart

 // Copyright (c) 2014, 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.
 
 library analyzer2dart.treeShaker;
 
 import 'dart:collection';
 
 import 'package:analyzer/analyzer.dart';
 import 'package:analyzer/src/generated/element.dart';
@@ skipping 57 matching lines @@
 
 /**
  * This class is responsible for performing local analysis of the source code
  * to provide the information needed to do tree shaking.
  */
 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;
   }
 
   /**
    * Perform local reachability analysis of [classElement].
    */
   ClassAnalysis analyzeClass(ClassElement classElement) {
     return new ClassAnalysis(classElement.node);
   }
 
@@ skipping 40 matching lines @@
  * and performing the global inferences necessary to determine which methods
  * in the source program are reachable.  It makes use of
  * [LocalReachabilityComputer] to do local analysis of individual classes and
  * methods.
  */
 class TreeShaker {
   List<Element> _queue = <Element>[];
   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);
 
   void _addElement(Element element) {
     if (_alreadyEnqueued.add(element)) {
       _queue.add(element);
     }
   }
 
@@ skipping 41 matching lines @@
         }
       } else if (element is FieldElement) {
         VariableDeclaration declaration = element.node;
         _world.fields[element] = declaration;
       } else if (element is TopLevelVariableElement) {
         VariableDeclaration declaration = element.node;
         _world.variables[element] = declaration;
       } else {
         throw new Exception(
             'Unexpected element type while tree shaking: '
-            '$element (${element.runtimeType})');
+                '$element (${element.runtimeType})');
       }
     }
     print('Tree shaking done');
     return _world;
   }
 }
 
 class TreeShakingVisitor extends SemanticVisitor {
   final MethodAnalysis analysis;
 
   TreeShakingVisitor(this.analysis);
 
   Source get currentSource => analysis.declaration.element.source;
 
   @override
   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
       // warning, not an error).
     }
     super.visitInstanceCreationExpression(node);
   }
 
   @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);
   }
 
   void visitDynamicAccess(AstNode node, AccessSemantics semantics) {
     if (semantics.isRead) {
       analysis.invokes.add(
           new Selector.getter(semantics.identifier.name, null));
     }
     if (semantics.isWrite) {
@@ skipping 20 matching lines @@
 
   void visitStaticMethodAccess(AstNode node, AccessSemantics semantics) {
     // Method tear-off.  TODO(paulberry): implement.
     super.visitStaticMethodAccess(node, semantics);
   }
 
   void visitStaticPropertyAccess(AstNode node, AccessSemantics semantics) {
     // 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);
+  }
 }