From e4e554e65ccca44abe766f209debbc98077ac5d4 Mon Sep 17 00:00:00 2001 From: "paulberry@google.com" Date: Fri, 3 Oct 2014 15:53:48 +0000 Subject: [PATCH] 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 | 81 ++++++++++++++++---- pkg/analyzer2dart/test/tree_shaker_test.dart | 68 ++++++++++++++++ 2 files changed, 132 insertions(+), 17 deletions(-) diff --git a/pkg/analyzer2dart/lib/src/tree_shaker.dart b/pkg/analyzer2dart/lib/src/tree_shaker.dart index 14a90c70098..899751c91aa 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 _alreadyEnqueued = new HashSet(); ClosedWorld _world; Set _selectors = new HashSet(); - 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); + } } diff --git a/pkg/analyzer2dart/test/tree_shaker_test.dart b/pkg/analyzer2dart/test/tree_shaker_test.dart index 938b334e3df..c094f09c76d 100644 --- a/pkg/analyzer2dart/test/tree_shaker_test.dart +++ b/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 {