From f8e657686c864db0554534213d7581a36a5545ef Mon Sep 17 00:00:00 2001 From: tbreisacher Date: Wed, 15 Nov 2017 13:59:07 -0800 Subject: [PATCH] Add a few more tests of ExtractPrototypeMemberDeclarations and do some small code cleanups. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=175875886 --- .../ExtractPrototypeMemberDeclarations.java | 59 +++++++++---------- ...xtractPrototypeMemberDeclarationsTest.java | 34 +++++++++++ 2 files changed, 62 insertions(+), 31 deletions(-) diff --git a/src/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarations.java b/src/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarations.java index 29ba112210c..9096bb94269 100644 --- a/src/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarations.java +++ b/src/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarations.java @@ -13,14 +13,16 @@ * See the License for the specific language governing permissions and * limitations under the License. */ - package com.google.javascript.jscomp; +import static com.google.common.base.Preconditions.checkState; + import com.google.javascript.jscomp.NodeTraversal.AbstractShallowCallback; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; import java.util.LinkedList; import java.util.List; +import javax.annotation.Nullable; /** * When there are multiple prototype member declarations to the same class, @@ -85,7 +87,7 @@ class ExtractPrototypeMemberDeclarations implements CompilerPass { // The name of variable that will temporary hold the pointer to the prototype // object. Of cause, we assume that it'll be renamed by RenameVars. - private final String prototypeAlias = "JSCompiler_prototypeAlias"; + private static final String PROTOTYPE_ALIAS = "JSCompiler_prototypeAlias"; private final AbstractCompiler compiler; @@ -150,7 +152,7 @@ private void doExtraction(GatherExtractionInfo info) { if (pattern == Pattern.USE_GLOBAL_TEMP) { Node injectionPoint = compiler.getNodeForCodeInsertion(null); - Node var = NodeUtil.newVarNode(prototypeAlias, null) + Node var = NodeUtil.newVarNode(PROTOTYPE_ALIAS, null) .useSourceInfoIfMissingFromForTree(injectionPoint); injectionPoint.addChildToFront(var); @@ -173,24 +175,23 @@ private void extractInstance(ExtractionInstance instance) { if (pattern == Pattern.USE_GLOBAL_TEMP) { // Use the temp variable to hold the prototype. Node stmt = - new Node( - first.node.getToken(), - IR.assign( - IR.name(prototypeAlias), - NodeUtil.newQName( - compiler, - className + ".prototype", - instance.parent, - className + ".prototype"))) - .useSourceInfoIfMissingFromForTree(first.node); + IR.exprResult( + IR.assign( + IR.name(PROTOTYPE_ALIAS), + NodeUtil.newQName( + compiler, + className + ".prototype", + instance.parent, + className + ".prototype"))) + .useSourceInfoIfMissingFromForTree(first.node); instance.parent.addChildBefore(stmt, first.node); compiler.reportChangeToEnclosingScope(stmt); - } else if (pattern == Pattern.USE_IIFE){ + } else if (pattern == Pattern.USE_IIFE) { Node block = IR.block(); Node func = IR.function( IR.name(""), - IR.paramList(IR.name(prototypeAlias)), + IR.paramList(IR.name(PROTOTYPE_ALIAS)), block); Node call = IR.call(func, @@ -199,7 +200,7 @@ private void extractInstance(ExtractionInstance instance) { instance.parent, className + ".prototype")); call.putIntProp(Node.FREE_CALL, 1); - Node stmt = new Node(first.node.getToken(), call); + Node stmt = IR.exprResult(call); stmt.useSourceInfoIfMissingFromForTree(first.node); instance.parent.addChildBefore(stmt, first.node); compiler.reportChangeToEnclosingScope(stmt); @@ -208,7 +209,7 @@ private void extractInstance(ExtractionInstance instance) { block.addChildToBack(declar.node.detach()); } } - // Go thought each member declaration and replace it with an assignment + // Go through each member declaration and replace it with an assignment // to the prototype variable. for (PrototypeMemberDeclaration declar : instance.declarations) { replacePrototypeMemberDeclaration(declar); @@ -219,20 +220,17 @@ private void extractInstance(ExtractionInstance instance) { * Replaces a member declaration to an assignment to the temp prototype * object. */ - private void replacePrototypeMemberDeclaration( - PrototypeMemberDeclaration declar) { + private void replacePrototypeMemberDeclaration(PrototypeMemberDeclaration declar) { // x.prototype.y = ... -> t.y = ... Node assignment = declar.node.getFirstChild(); Node lhs = assignment.getFirstChild(); Node name = NodeUtil.newQName( compiler, - prototypeAlias + "." + declar.memberName, declar.node, + PROTOTYPE_ALIAS + "." + declar.memberName, declar.node, declar.memberName); - // Save the full prototype path on the left hand side of the assignment - // for debugging purposes. - // declar.lhs = x.prototype.y so first child of the first child - // is 'x'. + // Save the full prototype path on the left hand side of the assignment for debugging purposes. + // declar.lhs = x.prototype.y so first child of the first child is 'x'. Node accessNode = declar.lhs.getFirstFirstChild(); String originalName = accessNode.getOriginalName(); String className = originalName != null ? originalName : "?"; @@ -267,12 +265,10 @@ public void visit(NodeTraversal t, Node n, Node parent) { // Found a good site here. The constructor will computes the chain of // declarations that is qualified for extraction. - ExtractionInstance instance = - new ExtractionInstance(prototypeMember, n); + ExtractionInstance instance = new ExtractionInstance(prototypeMember, n); cur = instance.declarations.getLast().node; - // Only add it to our work list if the extraction at this instance - // makes the code smaller. + // Only add it to our work list if the extraction at this instance makes the code smaller. if (instance.isFavorable()) { instances.add(instance); totalDelta += instance.delta; @@ -281,7 +277,7 @@ public void visit(NodeTraversal t, Node n, Node parent) { } /** - * @return <@code true> if the sum of all the extraction instance gain + * @return {@code true} if the sum of all the extraction instance gain * outweighs the overhead of the temp variable declaration. */ private boolean shouldExtract() { @@ -303,8 +299,7 @@ private ExtractionInstance(PrototypeMemberDeclaration head, Node parent) { // We can skip over any named functions because they have no effect on // the control flow. In fact, they are lifted to the beginning of the - // block. This happens a lot when devirtualization breaks the whole - // chain. + // block. This happens a lot when devirtualization breaks the whole chain. if (cur.isFunction()) { continue; } @@ -340,6 +335,7 @@ private static class PrototypeMemberDeclaration { final Node lhs; private PrototypeMemberDeclaration(Node lhs, Node node) { + checkState(NodeUtil.isExprAssign(node), node); this.lhs = lhs; this.memberName = NodeUtil.getPrototypePropertyName(lhs); this.node = node; @@ -383,6 +379,7 @@ private static boolean isPrototypePropertyDeclaration(Node n) { * @return A prototype member declaration representation if there is one * else it returns {@code null}. */ + @Nullable private static PrototypeMemberDeclaration extractDeclaration(Node n) { if (!isPrototypePropertyDeclaration(n)) { return null; diff --git a/test/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarationsTest.java b/test/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarationsTest.java index 2d0519dac4c..e14208a23bf 100644 --- a/test/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarationsTest.java +++ b/test/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarationsTest.java @@ -16,6 +16,8 @@ package com.google.javascript.jscomp; +import static com.google.javascript.jscomp.CompilerTestCase.lines; + import com.google.javascript.jscomp.ExtractPrototypeMemberDeclarations.Pattern; /** @@ -56,6 +58,38 @@ public void testNoOpOnEs6Class() { testSame("export class Example { method1() {} method2() {} }"); } + public void testClassDefinedInBlock() { + test( + lines( + "{", + generatePrototypeDeclarations("x", 7), + "}"), + lines( + "var " + TMP + ";", + "{", + TMP + " = x.prototype;", + generateExtractedDeclarations(7), + "}")); + } + + public void testClassDefinedInFunction() { + testSame( + lines( + "function f() {", + generatePrototypeDeclarations("x", 7), + "}")); + } + + /** + * Currently, this does not run on classes defined in ES6 modules. + */ + // TODO(tbreisacher): Make this work on modules. The initial 'var' needs to go into a non-module + // script node. + public void testEs6Module() { + String importStatement = "import {someValue} from './another_module.js';"; + testSame(importStatement + generatePrototypeDeclarations("x", 17)); + } + public void testExtractingTwoClassPrototype() { extract( generatePrototypeDeclarations("x", 6) + generatePrototypeDeclarations("y", 6),