From d455a72d13e5175f47e25b3b01800acffd8edbff Mon Sep 17 00:00:00 2001 From: michaelthomas Date: Thu, 9 Jun 2016 12:10:52 -0700 Subject: [PATCH] Allow hoisting of anonymous functions in J2CL clinits. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=124483541 --- .../jscomp/J2clConstantHoisterPass.java | 45 ++++++++++++---- .../jscomp/J2clConstantHoisterPassTest.java | 52 ++++++++++++++++++- 2 files changed, 86 insertions(+), 11 deletions(-) diff --git a/src/com/google/javascript/jscomp/J2clConstantHoisterPass.java b/src/com/google/javascript/jscomp/J2clConstantHoisterPass.java index 25c46d0eca7..efc6f106f91 100644 --- a/src/com/google/javascript/jscomp/J2clConstantHoisterPass.java +++ b/src/com/google/javascript/jscomp/J2clConstantHoisterPass.java @@ -24,6 +24,8 @@ import com.google.javascript.rhino.Node; import java.util.Collection; +import java.util.HashSet; +import java.util.Set; /** * An optimization pass for J2CL-generated code to hoist some constant assignments out clinit method @@ -40,21 +42,40 @@ public class J2clConstantHoisterPass implements CompilerPass { @Override public void process(Node externs, Node root) { final Multimap fieldAssignments = ArrayListMultimap.create(); + final Set hoistableFunctions = new HashSet<>(); NodeTraversal.traverseEs6(compiler, root, new AbstractPostOrderCallback() { @Override public void visit(NodeTraversal t, Node node, Node parent) { if (parent != null && NodeUtil.isLValue(node)) { fieldAssignments.put(node.getQualifiedName(), parent); + Node rValue = NodeUtil.getRValueOfLValue(node); + if (isHoistableFunction(t, parent, rValue)) { + hoistableFunctions.add(rValue); + } } } }); for (Collection assignments : fieldAssignments.asMap().values()) { - maybeHoistClassField(assignments); + maybeHoistClassField(assignments, hoistableFunctions); } } - private void maybeHoistClassField(Collection assignments) { + /** + * Returns whether the specified rValue is a function which does not receive any variables from + * its containing scope, and is thus 'hoistable'. + */ + private static boolean isHoistableFunction(NodeTraversal t, Node parent, Node rValue) { + // TODO(michaelthomas): This could be improved slightly by not assuming that any variable in the + // outer scope is used in the function. + return rValue != null + && rValue.isFunction() + && NodeUtil.isAssignmentOp(parent) + && t.getScope().getVarCount() == 0; + } + + private void maybeHoistClassField( + Collection assignments, Collection hoistableFunctions) { // The field is only assigned twice: if (assignments.size() != 2) { return; @@ -63,7 +84,7 @@ private void maybeHoistClassField(Collection assignments) { Node firstAssignment = Iterables.get(assignments, 0); Node secondAssignment = Iterables.get(assignments, 1); - // One of them is for field initilization in declaration phase: + // One of them is for field initialization in declaration phase: if (!isClassFieldInitialization(secondAssignment)) { return; } @@ -73,7 +94,9 @@ private void maybeHoistClassField(Collection assignments) { return; } // And it is assigned to a literal value; hence could be used in static eval and safe to move: - if (!isLiteralValue(firstAssignment.getSecondChild())) { + Node firstAssignmentRhs = firstAssignment.getSecondChild(); + if (!NodeUtil.isLiteralValue(firstAssignmentRhs, true /* includeFunctions */) + || (firstAssignmentRhs.isFunction() && !hoistableFunctions.contains(firstAssignmentRhs))) { return; } @@ -105,11 +128,14 @@ private void hoistConstantLikeField(Node clinitAssignment, Node declarationAssig declarationInClass.putBooleanProp(Node.IS_CONSTANT_VAR, true); // Sanity check - checkState(isLiteralValue(declarationAssignedValue)); + checkState(NodeUtil.isLiteralValue(declarationAssignedValue, false /* includeFunctions */)); } private static boolean isClassFieldInitialization(Node node) { - return node.getParent().isScript() && node.isVar(); + return node.getParent().isScript() + && node.isVar() + && node.getFirstFirstChild() != null + && NodeUtil.isLiteralValue(node.getFirstFirstChild(), false /* includeFunctions */); } private static boolean isClinitFieldAssignment(Node node) { @@ -125,10 +151,11 @@ private static boolean isClinitMethod(Node fnNode) { } String fnName = NodeUtil.getName(fnNode); - return fnName != null && (fnName.endsWith("$$0clinit") || fnName.endsWith(".$clinit")); + return fnName != null && isClinitMethodName(fnName); } - private static boolean isLiteralValue(Node node) { - return NodeUtil.isLiteralValue(node, false /* exclude functions */); + private static boolean isClinitMethodName(String methodName) { + return methodName != null + && (methodName.endsWith("$$0clinit") || methodName.endsWith(".$clinit")); } } diff --git a/test/com/google/javascript/jscomp/J2clConstantHoisterPassTest.java b/test/com/google/javascript/jscomp/J2clConstantHoisterPassTest.java index 84a835c4a58..e99412409b4 100644 --- a/test/com/google/javascript/jscomp/J2clConstantHoisterPassTest.java +++ b/test/com/google/javascript/jscomp/J2clConstantHoisterPassTest.java @@ -30,16 +30,19 @@ public void testHoistClinitConstantAssignments() { " someClass.$clinit = function() {};", " someClass$foo = true;", " someClass$bar = 'hey';", + " someClass$buzz = function() { return someClass$bar; };", "};", "var someClass$foo = false;", - "var someClass$bar = null;"), + "var someClass$bar = null;", + "var someClass$buzz = null;"), LINE_JOINER.join( "var someClass = /** @constructor */ function() {};", "someClass.$clinit = function() {", " someClass.$clinit = function() {};", "};", "var someClass$foo = true;", - "var someClass$bar = 'hey';")); + "var someClass$bar = 'hey';", + "var someClass$buzz = function(){ return someClass$bar; };")); } public void testHoistClinitConstantAssignments_avoidUnsafe() { @@ -63,4 +66,49 @@ public void testHoistClinitConstantAssignments_avoidUnsafe() { "var someClass$foo2 = false;", "var someClass$hoo = false;")); } + + /** + * Tests that hoisting works as expected when encountering a clinit that has been devirtualized. + */ + public void testHoistClinitConstantAssignments_devirtualized() { + test( + LINE_JOINER.join( + "var someClass = /** @constructor */ function() {};", + "var someClass$$0clinit = function() {", + " someClass$$0clinit = function() {};", + " someClass$shouldHoist = true;", + " someClass$shouldNotHoist = new Object();", + "};", + "var someClass$shouldHoist = false;", + "var someClass$shouldNotHoist = null;"), + LINE_JOINER.join( + "var someClass = /** @constructor */ function() {};", + "var someClass$$0clinit = function() {", + " someClass$$0clinit = function() {};", + " someClass$shouldNotHoist = new Object();", + "};", + "var someClass$shouldHoist = true;", + "var someClass$shouldNotHoist = null;")); + } + + public void testHoistClinitConstantAssignments_avoidUnsafeFunc() { + testSame( + LINE_JOINER.join( + "var someClass = /** @constructor */ function() {};", + "someClass.$clinit = function() {", + " someClass.$clinit = function() {};", + " var x = 10;", + " someClass$foo = function() { return x; };", // Depends on the scope of the clinit. + "};", + "var someClass$foo = null;")); + testSame( + LINE_JOINER.join( + "var someClass = /** @constructor */ function() {};", + "someClass.$clinit = function() {", + " someClass.$clinit = function() {};", + " var x = 10;", + " someClass$foo = function() { return 1; };", // x is assumed to be used + "};", + "var someClass$foo = null;")); + } }