From 5e4850cfa179b137ba566bd2a99b19f0a7e90a1e Mon Sep 17 00:00:00 2001 From: moz Date: Tue, 14 Nov 2017 14:44:50 -0800 Subject: [PATCH] Make J2clConstantHoisterPass optimize stub static field initializations e.g.: function clinit() { foo = true; } var foo; Also optimize the case where the initialization comes before the clinit. e.g.: var foo; function clinit() { foo = true; } ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175738294 --- .../jscomp/J2clConstantHoisterPass.java | 58 +++++++++++-------- .../jscomp/J2clConstantHoisterPassTest.java | 46 +++++++++++++++ 2 files changed, 79 insertions(+), 25 deletions(-) diff --git a/src/com/google/javascript/jscomp/J2clConstantHoisterPass.java b/src/com/google/javascript/jscomp/J2clConstantHoisterPass.java index 8012eb21e8e..53caf3e090f 100644 --- a/src/com/google/javascript/jscomp/J2clConstantHoisterPass.java +++ b/src/com/google/javascript/jscomp/J2clConstantHoisterPass.java @@ -87,28 +87,27 @@ private void maybeHoistClassField( return; } - Node firstAssignment = Iterables.get(assignments, 0); - Node secondAssignment = Iterables.get(assignments, 1); - - // One of them is for field initialization in declaration phase: - if (!isClassFieldInitialization(secondAssignment)) { + Node first = Iterables.get(assignments, 0); + Node second = Iterables.get(assignments, 1); + + // One of them is the top level declaration and the other is the assignment in clinit. + Node topLevelDeclaration = isClassFieldDeclaration(first) ? first : second; + Node clinitAssignment = isClinitFieldAssignment(first) ? first : second; + if (!isClassFieldDeclaration(topLevelDeclaration) + || !isClinitFieldAssignment(clinitAssignment)) { return; } - // The other one is the clinit initialization: - if (!isClinitFieldAssignment(firstAssignment)) { - return; - } // And it is assigned to a literal value; hence could be used in static eval and safe to move: - Node firstAssignmentRhs = firstAssignment.getSecondChild(); - if (!NodeUtil.isLiteralValue(firstAssignmentRhs, true /* includeFunctions */) - || (firstAssignmentRhs.isFunction() && !hoistableFunctions.contains(firstAssignmentRhs))) { + Node assignmentRhs = clinitAssignment.getSecondChild(); + if (!NodeUtil.isLiteralValue(assignmentRhs, true /* includeFunctions */) + || (assignmentRhs.isFunction() && !hoistableFunctions.contains(assignmentRhs))) { return; } // And the assignment are in the same script: - if (NodeUtil.getEnclosingScript(firstAssignment) - != NodeUtil.getEnclosingScript(secondAssignment)) { + if (NodeUtil.getEnclosingScript(clinitAssignment) + != NodeUtil.getEnclosingScript(topLevelDeclaration)) { return; } @@ -116,12 +115,12 @@ private void maybeHoistClassField( // cycle between clinits and the field is accessed before initialization; which is almost always // a bug and GWT never assumed this state is observable in its optimization, yet nobody // complained. So it is safe to upgrade it to a constant. - hoistConstantLikeField(firstAssignment, secondAssignment); + hoistConstantLikeField(clinitAssignment, topLevelDeclaration); } - private void hoistConstantLikeField(Node clinitAssignment, Node declarationAssignment) { + private void hoistConstantLikeField(Node clinitAssignment, Node topLevelDeclaration) { Node clinitAssignedValue = clinitAssignment.getSecondChild(); - Node declarationInClass = declarationAssignment.getFirstChild(); + Node declarationInClass = topLevelDeclaration.getFirstChild(); Node declarationAssignedValue = declarationInClass.getFirstChild(); Node clinitChangeScope = NodeUtil.getEnclosingChangeScopeRoot(clinitAssignment); @@ -129,23 +128,32 @@ private void hoistConstantLikeField(Node clinitAssignment, Node declarationAssig NodeUtil.removeChild(clinitAssignment.getParent(), clinitAssignment); - // Replace the assignment in declaration with the value from clinit clinitAssignedValue.detach(); compiler.reportChangeToChangeScope(clinitChangeScope); - if (!declarationAssignedValue.isEquivalentTo(clinitAssignedValue)) { + if (declarationAssignedValue == null) { + // Add the value from clinit to the stub declaration + declarationInClass.addChildToFront(clinitAssignedValue); + compiler.reportChangeToEnclosingScope(topLevelDeclaration); + } else if (!declarationAssignedValue.isEquivalentTo(clinitAssignedValue)) { + checkState(NodeUtil.isLiteralValue(declarationAssignedValue, false /* includeFunctions */)); + // Replace the assignment in declaration with the value from clinit declarationInClass.replaceChild(declarationAssignedValue, clinitAssignedValue); - compiler.reportChangeToEnclosingScope(declarationAssignment); + compiler.reportChangeToEnclosingScope(topLevelDeclaration); } declarationInClass.putBooleanProp(Node.IS_CONSTANT_VAR, true); - - checkState(NodeUtil.isLiteralValue(declarationAssignedValue, false /* includeFunctions */)); } - private static boolean isClassFieldInitialization(Node node) { + /** + * Matches literal value declarations {@code var foo = 3;} or stub declarations like + * {@code var bar;}. + */ + private static boolean isClassFieldDeclaration(Node node) { return node.getParent().isScript() && node.isVar() - && node.getFirstFirstChild() != null - && NodeUtil.isLiteralValue(node.getFirstFirstChild(), false /* includeFunctions */); + && (!node.getFirstChild().hasChildren() + || (node.getFirstFirstChild() != null + && NodeUtil.isLiteralValue( + node.getFirstFirstChild(), false /* includeFunctions */))); } private static boolean isClinitFieldAssignment(Node node) { diff --git a/test/com/google/javascript/jscomp/J2clConstantHoisterPassTest.java b/test/com/google/javascript/jscomp/J2clConstantHoisterPassTest.java index 266402ae816..3be41dd005c 100644 --- a/test/com/google/javascript/jscomp/J2clConstantHoisterPassTest.java +++ b/test/com/google/javascript/jscomp/J2clConstantHoisterPassTest.java @@ -52,6 +52,52 @@ public void testHoistClinitConstantAssignments() { "var someClass$buzz = function(){ return someClass$bar; };")); } + public void testHoistClinitConstantAssignments_assignmentAfterInit() { + test( + LINE_JOINER.join( + "var someClass = /** @constructor */ function() {};", + "var someClass$foo = false;", + "var someClass$bar = null;", + "var someClass$buzz = null;", + "someClass.$clinit = function() {", + " someClass.$clinit = function() {};", + " someClass$foo = true;", + " someClass$bar = 'hey';", + " someClass$buzz = function() { return someClass$bar; };", + "};"), + LINE_JOINER.join( + "var someClass = /** @constructor */ function() {};", + "var someClass$foo = true;", + "var someClass$bar = 'hey';", + "var someClass$buzz = function(){ return someClass$bar; };", + "someClass.$clinit = function() {", + " someClass.$clinit = function() {};", + "};")); + } + + public void testHoistClinitConstantAssignments_stubInit() { + test( + LINE_JOINER.join( + "var someClass = /** @constructor */ function() {};", + "someClass.$clinit = function() {", + " someClass.$clinit = function() {};", + " someClass$foo = true;", + " someClass$bar = 'hey';", + " someClass$buzz = function() { return someClass$bar; };", + "};", + "var someClass$foo = false;", + "var someClass$bar;", + "var someClass$buzz;"), + LINE_JOINER.join( + "var someClass = /** @constructor */ function() {};", + "someClass.$clinit = function() {", + " someClass.$clinit = function() {};", + "};", + "var someClass$foo = true;", + "var someClass$bar = 'hey';", + "var someClass$buzz = function(){ return someClass$bar; };")); + } + public void testHoistClinitConstantAssignments_avoidUnsafe() { testSame( lines(