From cfad150267d26a4f0a99104775f04b46fdbbd485 Mon Sep 17 00:00:00 2001 From: stalcup Date: Thu, 20 Apr 2017 15:18:36 -0700 Subject: [PATCH] Fixes change tracking in more passes and reenables tests. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=153763813 --- .../jscomp/AggressiveInlineAliases.java | 3 +-- .../jscomp/CrossModuleMethodMotion.java | 16 +++++++++++++--- .../jscomp/ExportTestFunctions.java | 11 +++++------ .../ExtractPrototypeMemberDeclarations.java | 6 +++++- .../jscomp/MarkNoSideEffectCalls.java | 6 ++++-- .../jscomp/NameAnonymousFunctions.java | 2 +- .../google/javascript/jscomp/Normalize.java | 19 +++++++++++-------- .../javascript/jscomp/OptimizeParameters.java | 11 +++++++---- .../jscomp/PolymerClassRewriter.java | 7 ++++--- .../google/javascript/jscomp/PolymerPass.java | 3 +-- .../jscomp/PolymerPassStaticUtils.java | 2 +- .../jscomp/CrossModuleMethodMotionTest.java | 1 - .../jscomp/ExportTestFunctionsTest.java | 1 - .../FlowSensitiveInlineVariablesTest.java | 1 - .../jscomp/NameAnonymousFunctionsTest.java | 7 +------ .../javascript/jscomp/NormalizeTest.java | 13 ------------- .../jscomp/OptimizeParametersTest.java | 1 - .../javascript/jscomp/PolymerPassTest.java | 1 - 18 files changed, 54 insertions(+), 57 deletions(-) diff --git a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java index 18d1fc2b269..08cc20eac07 100644 --- a/src/com/google/javascript/jscomp/AggressiveInlineAliases.java +++ b/src/com/google/javascript/jscomp/AggressiveInlineAliases.java @@ -53,8 +53,7 @@ class AggressiveInlineAliases implements CompilerPass { * @param depth The chain depth. * @param newNodes Expression nodes that have been updated. */ - private void rewriteAliasProps( - Name name, Node value, int depth, Set newNodes) { + private void rewriteAliasProps(Name name, Node value, int depth, Set newNodes) { if (name.props == null) { return; } diff --git a/src/com/google/javascript/jscomp/CrossModuleMethodMotion.java b/src/com/google/javascript/jscomp/CrossModuleMethodMotion.java index b4617cd4e53..460967fa74f 100644 --- a/src/com/google/javascript/jscomp/CrossModuleMethodMotion.java +++ b/src/com/google/javascript/jscomp/CrossModuleMethodMotion.java @@ -21,7 +21,6 @@ import com.google.javascript.jscomp.AnalyzePrototypeProperties.Symbol; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; - import java.io.Serializable; import java.util.Collection; import java.util.Iterator; @@ -161,6 +160,16 @@ private void moveMethods(Collection allNameInfo) { } Node valueParent = value.getParent(); + /** + * The logic here moves methods from some starting script node to some other script node. + * Both scripts need to be marked as changed. Locally the removal point in the starting + * script node is called 'valueParent' and the insertion point in the destination script + * is sometimes called 'unstubParent' and sometimes 'destParent'. The change on + * 'valueParent' is being reported before the change occurs since the change is guaranteed + * to occur and since after the change the 'valueParent' node has sometimes already been + * detached. + */ + compiler.reportChangeToEnclosingScope(valueParent); Node proto = prop.getPrototype(); int stubId = idGenerator.newId(); @@ -222,8 +231,9 @@ private void moveMethods(Collection allNameInfo) { .hasGeneratedAnyIds()) { // Declare stub functions in the top-most module. Node declarations = compiler.parseSyntheticCode(STUB_DECLARATIONS); - compiler.getNodeForCodeInsertion(null).addChildrenToFront( - declarations.removeChildren()); + Node firstScript = compiler.getNodeForCodeInsertion(null); + firstScript.addChildrenToFront(declarations.removeChildren()); + compiler.reportChangeToEnclosingScope(firstScript); } } diff --git a/src/com/google/javascript/jscomp/ExportTestFunctions.java b/src/com/google/javascript/jscomp/ExportTestFunctions.java index 119b7081623..3a7e9c0bb07 100644 --- a/src/com/google/javascript/jscomp/ExportTestFunctions.java +++ b/src/com/google/javascript/jscomp/ExportTestFunctions.java @@ -19,7 +19,6 @@ import com.google.javascript.jscomp.parsing.parser.util.format.SimpleFormat; import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; - import java.util.regex.Pattern; /** @@ -96,7 +95,7 @@ && isCallTargetQName(n.getParent(), "goog.testing.testSuite")) { for (Node c : n.children()) { if (c.isStringKey() && !c.isQuotedString()) { c.setQuotedString(); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(c); } else if (c.isMemberFunctionDef()) { rewriteMemberDefInObjLit(c, n); } @@ -130,7 +129,7 @@ private void exportClass(Node scriptNode, Node classNode) { Node expression = IR.exprResult(call); scriptNode.addChildAfter(expression, classNode); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(expression); } } } @@ -141,7 +140,7 @@ private void rewriteMemberDefInObjLit(Node memberDef, Node objLit) { Node stringKey = IR.stringKey(name, memberDef.getFirstChild().detach()); objLit.replaceChild(memberDef, stringKey); stringKey.setQuotedString(); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(objLit); } // TODO(johnlenz): move test suite declaration into the @@ -192,7 +191,7 @@ private void exportTestFunctionAsSymbol(String testFunctionName, Node node, Node expression = IR.exprResult(call); scriptNode.addChildAfter(expression, node); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(expression); } @@ -216,7 +215,7 @@ private void exportTestFunctionAsProperty(String fullyQualifiedFunctionName, exportCall.useSourceInfoFromForTree(scriptNode); scriptNode.addChildrenAfter(exportCall, parent); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(exportCall); } diff --git a/src/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarations.java b/src/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarations.java index 83e077b59f8..27b74929f82 100644 --- a/src/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarations.java +++ b/src/com/google/javascript/jscomp/ExtractPrototypeMemberDeclarations.java @@ -147,7 +147,6 @@ public void process(Node externs, Node root) { * through all ExtractInstance and performs extraction there. */ private void doExtraction(GatherExtractionInfo info) { - // Insert a global temp if we are using the USE_GLOBAL_TEMP pattern. if (pattern == Pattern.USE_GLOBAL_TEMP) { Node injectionPoint = compiler.getNodeForCodeInsertion(null); @@ -156,6 +155,7 @@ private void doExtraction(GatherExtractionInfo info) { .useSourceInfoIfMissingFromForTree(injectionPoint); injectionPoint.addChildToFront(var); + compiler.reportChangeToEnclosingScope(var); } // Go through all extraction instances and extract each of them. for (ExtractionInstance instance : info.instances) { @@ -186,6 +186,7 @@ private void extractInstance(ExtractionInstance instance) { .useSourceInfoIfMissingFromForTree(first.node); instance.parent.addChildBefore(stmt, first.node); + compiler.reportChangeToEnclosingScope(stmt); } else if (pattern == Pattern.USE_IIFE){ Node block = IR.block(); Node func = IR.function( @@ -202,7 +203,9 @@ private void extractInstance(ExtractionInstance instance) { Node stmt = new Node(first.node.getToken(), call); stmt.useSourceInfoIfMissingFromForTree(first.node); instance.parent.addChildBefore(stmt, first.node); + compiler.reportChangeToEnclosingScope(stmt); for (PrototypeMemberDeclaration declar : instance.declarations) { + compiler.reportChangeToEnclosingScope(declar.node); block.addChildToBack(declar.node.detach()); } } @@ -238,6 +241,7 @@ private void replacePrototypeMemberDeclaration( name.getFirstChild().setOriginalName(className + ".prototype"); assignment.replaceChild(lhs, name); + compiler.reportChangeToEnclosingScope(name); } /** diff --git a/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java b/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java index 44a812be833..60d87e831f6 100644 --- a/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java +++ b/src/com/google/javascript/jscomp/MarkNoSideEffectCalls.java @@ -181,8 +181,10 @@ public void visit(NodeTraversal traversal, Node node, Node parent) { } if (maybeFunction) { - node.setSideEffectFlags(Node.NO_SIDE_EFFECTS); - compiler.reportChangeToEnclosingScope(node); + if (node.getSideEffectFlags() != Node.NO_SIDE_EFFECTS) { + node.setSideEffectFlags(Node.NO_SIDE_EFFECTS); + compiler.reportChangeToEnclosingScope(node); + } } } } diff --git a/src/com/google/javascript/jscomp/NameAnonymousFunctions.java b/src/com/google/javascript/jscomp/NameAnonymousFunctions.java index dca4d26b678..2816f741202 100644 --- a/src/com/google/javascript/jscomp/NameAnonymousFunctions.java +++ b/src/com/google/javascript/jscomp/NameAnonymousFunctions.java @@ -88,7 +88,7 @@ public final void setFunctionName(String name, Node fnNode) { Node fnNameNode = fnNode.getFirstChild(); String uniqueName = getLikelyNonConflictingName(name); fnNameNode.setString(uniqueName); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(fnNameNode); namedCount++; bytesUsed += uniqueName.length(); } diff --git a/src/com/google/javascript/jscomp/Normalize.java b/src/com/google/javascript/jscomp/Normalize.java index b6acb5ffe82..824edcf06f9 100644 --- a/src/com/google/javascript/jscomp/Normalize.java +++ b/src/com/google/javascript/jscomp/Normalize.java @@ -89,12 +89,12 @@ static Node parseAndNormalizeTestCode( return js; } - private void reportCodeChange(String changeDescription) { + private void reportCodeChange(String changeDescription, Node n) { if (assertOnChange) { throw new IllegalStateException( "Normalize constraints violated:\n" + changeDescription); } - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(n); } @Override @@ -158,20 +158,21 @@ private class RewriteExposedProperties this.exposedProperties = exposedProperties; } - @Override public void visit(NodeTraversal t, Node n, Node parent) { + @Override + public void visit(NodeTraversal t, Node n, Node parent) { if (n.isGetProp()) { String propName = n.getLastChild().getString(); if (exposedProperties.contains(propName)) { Node obj = n.removeFirstChild(); Node prop = n.removeFirstChild(); + compiler.reportChangeToEnclosingScope(n); n.replaceWith(IR.getelem(obj, prop)); - compiler.reportCodeChange(); } } else if (n.isStringKey()) { String propName = n.getString(); if (exposedProperties.contains(propName)) { n.setQuotedString(); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(n); } } } @@ -669,7 +670,7 @@ private void normalizeAssignShorthand(Node shorthand) { assign.setJSDocInfo(shorthand.getJSDocInfo()); shorthand.setJSDocInfo(null); parent.replaceChild(insertPoint, assign); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(assign); } } @@ -770,7 +771,9 @@ private void replaceVarWithAssignment(Node n, Node parent, Node grandparent) { Node replacement = IR.assign(n, value); replacement.setJSDocInfo(parent.getJSDocInfo()); replacement.useSourceInfoIfMissingFrom(parent); - grandparent.replaceChild(parent, NodeUtil.newExpr(replacement)); + Node statement = NodeUtil.newExpr(replacement); + grandparent.replaceChild(parent, statement); + reportCodeChange("Duplicate VAR declaration", statement); } else { // It is an empty reference remove it. if (NodeUtil.isStatementBlock(grandparent)) { @@ -787,8 +790,8 @@ private void replaceVarWithAssignment(Node n, Node parent, Node grandparent) { // already have been normalized to have a BLOCK. throw new IllegalStateException("Unexpected LABEL"); } + reportCodeChange("Duplicate VAR declaration", grandparent); } - reportCodeChange("Duplicate VAR declaration"); } } diff --git a/src/com/google/javascript/jscomp/OptimizeParameters.java b/src/com/google/javascript/jscomp/OptimizeParameters.java index 91e4e2f7791..1c57f873302 100644 --- a/src/com/google/javascript/jscomp/OptimizeParameters.java +++ b/src/com/google/javascript/jscomp/OptimizeParameters.java @@ -485,7 +485,7 @@ private void addVariableToFunction(Node function, Node varName, Node value) { stmt = IR.exprResult(value).useSourceInfoFrom(value); } block.addChildToFront(stmt); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(stmt); } /** @@ -506,10 +506,11 @@ private boolean eliminateParamsAfter(Node fnNode, Node argNode) { if (argNode != null) { // Keep the args in the same order, do the last first. eliminateParamsAfter(fnNode, argNode.getNext()); + compiler.reportChangeToEnclosingScope(argNode); argNode.detach(); Node var = IR.var(argNode).useSourceInfoIfMissingFrom(argNode); fnNode.getLastChild().addChildToFront(var); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(var); return true; } return false; @@ -517,11 +518,12 @@ private boolean eliminateParamsAfter(Node fnNode, Node argNode) { /** * Eliminates the parameter from a function definition. + * * @param function The function node * @param argIndex The index of the the argument to remove. * @return The Node of the argument removed. */ - private static Node eliminateFunctionParamAt(Node function, int argIndex) { + private Node eliminateFunctionParamAt(Node function, int argIndex) { Preconditions.checkArgument(function.isFunction(), "Node must be a function."); @@ -529,6 +531,7 @@ private static Node eliminateFunctionParamAt(Node function, int argIndex) { function, argIndex); if (formalArgPtr != null) { + compiler.reportChangeToEnclosingScope(formalArgPtr); function.getSecondChild().removeChild(formalArgPtr); } return formalArgPtr; @@ -552,13 +555,13 @@ private Node eliminateCallParamAt( if (formalArgPtr != null) { call.removeChild(formalArgPtr); + compiler.reportChangeToEnclosingScope(call); // The value in the parameter object is the one that is being moved into // function definition leave that one's references. For everything else, // remove any references. if (p.getArg() != formalArgPtr) { removedNodes.add(formalArgPtr); } - compiler.reportCodeChange(); } return formalArgPtr; } diff --git a/src/com/google/javascript/jscomp/PolymerClassRewriter.java b/src/com/google/javascript/jscomp/PolymerClassRewriter.java index 3f6f8f74437..fa1886515c8 100644 --- a/src/com/google/javascript/jscomp/PolymerClassRewriter.java +++ b/src/com/google/javascript/jscomp/PolymerClassRewriter.java @@ -138,6 +138,7 @@ void rewritePolymerClass( parent.addChildrenAfter(statements, beforeRoot); } } + compiler.reportChangeToEnclosingScope(statements); // Since behavior files might contain more language features than the class file, we need to // update the feature sets. @@ -146,14 +147,14 @@ void rewritePolymerClass( Node scriptNode = NodeUtil.getEnclosingScript(parent); FeatureSet oldFeatures = (FeatureSet) scriptNode.getProp(Node.FEATURE_SET); scriptNode.putProp(Node.FEATURE_SET, oldFeatures.union(newFeatures)); + compiler.reportChangeToEnclosingScope(scriptNode); } if (NodeUtil.isNameDeclaration(exprRoot)) { Node assignExpr = varToAssign(exprRoot); parent.replaceChild(exprRoot, assignExpr); + compiler.reportChangeToEnclosingScope(assignExpr); } - - compiler.reportCodeChange(); } /** @@ -402,7 +403,7 @@ private void addInterfaceExterns( Node stmts = block.removeChildren(); parent.addChildrenToBack(stmts); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(stmts); } private static boolean hasShorthandAssignment(Node objLit) { diff --git a/src/com/google/javascript/jscomp/PolymerPass.java b/src/com/google/javascript/jscomp/PolymerPass.java index 9867fa9960b..a7ec2f9951a 100644 --- a/src/com/google/javascript/jscomp/PolymerPass.java +++ b/src/com/google/javascript/jscomp/PolymerPass.java @@ -29,7 +29,6 @@ import com.google.javascript.rhino.JSTypeExpression; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; - import java.util.HashSet; import java.util.Map; import java.util.Set; @@ -165,7 +164,7 @@ private void appendPolymerElementExterns(final PolymerClassDefinition def) { Node stmts = block.removeChildren(); parent.addChildrenAfter(stmts, polymerElementExterns); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(stmts); } /** diff --git a/src/com/google/javascript/jscomp/PolymerPassStaticUtils.java b/src/com/google/javascript/jscomp/PolymerPassStaticUtils.java index ba4d2e4e0dd..ee14f3a3d6c 100644 --- a/src/com/google/javascript/jscomp/PolymerPassStaticUtils.java +++ b/src/com/google/javascript/jscomp/PolymerPassStaticUtils.java @@ -50,7 +50,7 @@ public void visit(Node n) { && n.getGrandparent().isGetProp()) { Node dollarChildProp = n.getGrandparent(); dollarChildProp.setToken(Token.GETELEM); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(dollarChildProp); } } }, diff --git a/test/com/google/javascript/jscomp/CrossModuleMethodMotionTest.java b/test/com/google/javascript/jscomp/CrossModuleMethodMotionTest.java index 02fc61b5d34..51cf38216c2 100644 --- a/test/com/google/javascript/jscomp/CrossModuleMethodMotionTest.java +++ b/test/com/google/javascript/jscomp/CrossModuleMethodMotionTest.java @@ -45,7 +45,6 @@ protected CompilerPass getProcessor(Compiler compiler) { public void setUp() { canMoveExterns = false; noStubs = false; - validateAstChangeMarking(false); } public void testMovePrototypeMethod1() { diff --git a/test/com/google/javascript/jscomp/ExportTestFunctionsTest.java b/test/com/google/javascript/jscomp/ExportTestFunctionsTest.java index ec0552c293b..f0d9430cabd 100644 --- a/test/com/google/javascript/jscomp/ExportTestFunctionsTest.java +++ b/test/com/google/javascript/jscomp/ExportTestFunctionsTest.java @@ -39,7 +39,6 @@ public ExportTestFunctionsTest() { @Override public void setUp() { super.enableLineNumberCheck(false); - validateAstChangeMarking(false); } @Override diff --git a/test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java b/test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java index d0442be3043..f12c040e8fe 100644 --- a/test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java +++ b/test/com/google/javascript/jscomp/FlowSensitiveInlineVariablesTest.java @@ -33,7 +33,6 @@ public final class FlowSensitiveInlineVariablesTest extends CompilerTestCase { @Override public void setUp() { enableNormalize(); - validateAstChangeMarking(false); } @Override diff --git a/test/com/google/javascript/jscomp/NameAnonymousFunctionsTest.java b/test/com/google/javascript/jscomp/NameAnonymousFunctionsTest.java index 93f4d3d6422..71ef7b68440 100644 --- a/test/com/google/javascript/jscomp/NameAnonymousFunctionsTest.java +++ b/test/com/google/javascript/jscomp/NameAnonymousFunctionsTest.java @@ -29,12 +29,7 @@ public NameAnonymousFunctionsTest() { } @Override - protected void setUp() throws Exception { - super.setUp(); - validateAstChangeMarking(false); - } - - @Override public CompilerPass getProcessor(Compiler compiler) { + public CompilerPass getProcessor(Compiler compiler) { return new NameAnonymousFunctions(compiler); } diff --git a/test/com/google/javascript/jscomp/NormalizeTest.java b/test/com/google/javascript/jscomp/NormalizeTest.java index 191bdedd40b..c0bd92e5550 100644 --- a/test/com/google/javascript/jscomp/NormalizeTest.java +++ b/test/com/google/javascript/jscomp/NormalizeTest.java @@ -37,12 +37,6 @@ public NormalizeTest() { super(EXTERNS); } - @Override - protected void setUp() throws Exception { - super.setUp(); - validateAstChangeMarking(false); - } - @Override public CompilerPass getProcessor(final Compiler compiler) { return new Normalize(compiler, false); @@ -755,7 +749,6 @@ public void testRenamingConstantProperties() throws Exception { // TODO(johnlenz): fix this so it is just another test case. WithCollapse testCase = new WithCollapse(); - testCase.setUp(); testCase.testConstantProperties(); testCase.tearDown(); } @@ -765,12 +758,6 @@ public static class WithCollapse extends CompilerTestCase { enableNormalize(); } - @Override - protected void setUp() throws Exception { - super.setUp(); - validateAstChangeMarking(false); - } - private void testConstantProperties() { test("var a={}; a.ACONST = 4;var b = 1; b = a.ACONST;", "var a$ACONST = 4; var b = 1; b = a$ACONST;"); diff --git a/test/com/google/javascript/jscomp/OptimizeParametersTest.java b/test/com/google/javascript/jscomp/OptimizeParametersTest.java index 056e011d678..90c62e46baa 100644 --- a/test/com/google/javascript/jscomp/OptimizeParametersTest.java +++ b/test/com/google/javascript/jscomp/OptimizeParametersTest.java @@ -30,7 +30,6 @@ public CompilerPass getProcessor(Compiler compiler) { @Override public void setUp() { enableNormalize(); - validateAstChangeMarking(false); } public void testNoRemoval() { diff --git a/test/com/google/javascript/jscomp/PolymerPassTest.java b/test/com/google/javascript/jscomp/PolymerPassTest.java index 6df8c7468ce..009f204cb00 100644 --- a/test/com/google/javascript/jscomp/PolymerPassTest.java +++ b/test/com/google/javascript/jscomp/PolymerPassTest.java @@ -159,7 +159,6 @@ protected CompilerPass getProcessor(Compiler compiler) { @Override protected void setUp() throws Exception { super.setUp(); - validateAstChangeMarking(false); allowExternsChanges(true); enableTypeCheck(); enableCheckAccessControls(false);