From 9153e2b71c1f85c2383a4a9710a636a1372bce54 Mon Sep 17 00:00:00 2001 From: goktug Date: Mon, 19 Dec 2016 18:57:23 -0800 Subject: [PATCH] Correctly mark node as changed when modified by PureFunctionIdentifier. This is necessary since PureFunctionIdentifier runs as part of optimization loop and without nodes marked as changed, peephole passes in the optimization loop ignores them. That results in missed oppurtunities especially for J2CL. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=142511104 --- .../jscomp/MarkNoSideEffectCalls.java | 1 + .../jscomp/PureFunctionIdentifier.java | 6 ++- src/com/google/javascript/rhino/Node.java | 54 +++++++++++++------ .../javascript/jscomp/CompilerTestCase.java | 6 ++- 4 files changed, 48 insertions(+), 19 deletions(-) diff --git a/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java b/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java index b70683e24a7..44a812be833 100644 --- a/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java +++ b/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java @@ -182,6 +182,7 @@ public void visit(NodeTraversal traversal, Node node, Node parent) { if (maybeFunction) { node.setSideEffectFlags(Node.NO_SIDE_EFFECTS); + compiler.reportChangeToEnclosingScope(node); } } } diff --git a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java index 71fd273358c..ac563f96d4a 100644 --- a/src/com/google/javascript/jscomp/PureFunctionIdentifier.java +++ b/src/com/google/javascript/jscomp/PureFunctionIdentifier.java @@ -403,7 +403,11 @@ private void markPureFunctionCalls() { } } - callNode.setSideEffectFlags(flags.valueOf()); + int newSideEffectFlags = flags.valueOf(); + if (callNode.getSideEffectFlags() != newSideEffectFlags) { + callNode.setSideEffectFlags(newSideEffectFlags); + compiler.reportChangeToEnclosingScope(callNode); + } } } diff --git a/src/com/google/javascript/rhino/Node.java b/src/com/google/javascript/rhino/Node.java index 3cad2393c67..378e3d68dff 100644 --- a/src/com/google/javascript/rhino/Node.java +++ b/src/com/google/javascript/rhino/Node.java @@ -284,8 +284,8 @@ public void setDouble(double d) { @Override boolean isEquivalentTo( - Node node, boolean compareType, boolean recur, boolean jsDoc) { - boolean equiv = super.isEquivalentTo(node, compareType, recur, jsDoc); + Node node, boolean compareType, boolean recur, boolean jsDoc, boolean sideEffect) { + boolean equiv = super.isEquivalentTo(node, compareType, recur, jsDoc, sideEffect); if (equiv) { double thisValue = getDouble(); double thatValue = ((NumberNode) node).getDouble(); @@ -348,8 +348,8 @@ public void setString(String str) { @Override boolean isEquivalentTo( - Node node, boolean compareType, boolean recur, boolean jsDoc) { - return (super.isEquivalentTo(node, compareType, recur, jsDoc) + Node node, boolean compareType, boolean recur, boolean jsDoc, boolean sideEffect) { + return (super.isEquivalentTo(node, compareType, recur, jsDoc, sideEffect) && this.str.equals(((StringNode) node).str)); } @@ -1779,32 +1779,48 @@ private NodeMismatch checkTreeEqualsImpl(Node actual, boolean jsDoc) { } /** Returns true if this node is equivalent semantically to another */ - public boolean isEquivalentTo(Node node) { - return isEquivalentTo(node, false, true, false); + public final boolean isEquivalentTo(Node node) { + return isEquivalentTo(node, false, true, false, false); + } + + /** Returns true if this node is equivalent semantically to another including side efffects. */ + public final boolean isEquivalentWithSideEffectsTo(Node node) { + return isEquivalentTo(node, false, true, false, true); } /** Checks equivalence without going into child nodes */ - public boolean isEquivalentToShallow(Node node) { - return isEquivalentTo(node, false, false, false); + public final boolean isEquivalentToShallow(Node node) { + return isEquivalentTo(node, false, false, false, false); } /** - * Returns true if this node is equivalent semantically to another and - * the types are equivalent. + * Returns true if this node is equivalent semantically to another and the types are equivalent. */ - public boolean isEquivalentToTyped(Node node) { - return isEquivalentTo(node, true, true, true); + public final boolean isEquivalentToTyped(Node node) { + return isEquivalentTo(node, true, true, true, false); } /** * @param compareType Whether to compare the JSTypes of the nodes. - * @param recurse Whether to compare the children of the current node, if - * not only the the count of the children are compared. + * @param recurse Whether to compare the children of the current node, if not only the the count + * of the children are compared. * @param jsDoc Whether to check that the JsDoc of the nodes are equivalent. * @return Whether this node is equivalent semantically to the provided node. */ + final boolean isEquivalentTo(Node node, boolean compareType, boolean recurse, boolean jsDoc) { + return isEquivalentTo(node, compareType, recurse, jsDoc, false); + } + + /** + * @param compareType Whether to compare the JSTypes of the nodes. + * @param recurse Whether to compare the children of the current node, if not only the the count + * of the children are compared. + * @param jsDoc Whether to check that the JsDoc of the nodes are equivalent. + * @param sideEffect Whether to check that the side-effect flags of the nodes are equivalent. + * @return Whether this node is equivalent semantically to the provided node. + */ boolean isEquivalentTo( - Node node, boolean compareType, boolean recurse, boolean jsDoc) { + Node node, boolean compareType, boolean recurse, boolean jsDoc, boolean sideEffect) { if (token != node.token || getChildCount() != node.getChildCount() || this.getClass() != node.getClass()) { @@ -1857,11 +1873,17 @@ boolean isEquivalentTo( } } + if (sideEffect) { + if (this.getSideEffectFlags() != node.getSideEffectFlags()) { + return false; + } + } + if (recurse) { for (Node n = first, n2 = node.first; n != null; n = n.next, n2 = n2.next) { - if (!n.isEquivalentTo(n2, compareType, recurse, jsDoc)) { + if (!n.isEquivalentTo(n2, compareType, recurse, jsDoc, sideEffect)) { return false; } } diff --git a/test/com/google/javascript/jscomp/CompilerTestCase.java b/test/com/google/javascript/jscomp/CompilerTestCase.java index eedcbdcd2b3..2fadc8b0271 100644 --- a/test/com/google/javascript/jscomp/CompilerTestCase.java +++ b/test/com/google/javascript/jscomp/CompilerTestCase.java @@ -1319,9 +1319,11 @@ private void test( } if (computeSideEffects && i == 0) { + recentChange.reset(); PureFunctionIdentifier.Driver mark = new PureFunctionIdentifier.Driver(compiler, null); mark.process(externsRoot, mainRoot); + hasCodeChanged = hasCodeChanged || recentChange.hasCodeChanged(); } if (markNoSideEffects && i == 0) { @@ -1416,8 +1418,8 @@ private void test( normalizeActualCode(compiler, externsRootClone, mainRootClone); } - boolean codeChange = !mainRootClone.isEquivalentTo(mainRoot); - boolean externsChange = !externsRootClone.isEquivalentTo(externsRoot); + boolean codeChange = !mainRootClone.isEquivalentWithSideEffectsTo(mainRoot); + boolean externsChange = !externsRootClone.isEquivalentWithSideEffectsTo(externsRoot); // Generally, externs should not be changed by the compiler passes. if (externsChange && !allowExternsChanges) {