From 0670466eab13c9feeb4aedbb81ae6ed752f98b11 Mon Sep 17 00:00:00 2001 From: stalcup Date: Wed, 12 Apr 2017 11:34:12 -0700 Subject: [PATCH] Fixes change tracking in StripCode. ------------- Created by MOE: https://github.com/google/moe MOE_MIGRATED_REVID=152965110 --- .../javascript/jscomp/AbstractCompiler.java | 5 ++++- .../google/javascript/jscomp/Compiler.java | 11 ++++++++++ .../google/javascript/jscomp/NodeUtil.java | 2 +- .../google/javascript/jscomp/StripCode.java | 22 +++++++++++-------- .../javascript/jscomp/StripCodeTest.java | 9 ++------ 5 files changed, 31 insertions(+), 18 deletions(-) diff --git a/src/com/google/javascript/jscomp/AbstractCompiler.java b/src/com/google/javascript/jscomp/AbstractCompiler.java index 71d3002f58b..eecbcd74970 100644 --- a/src/com/google/javascript/jscomp/AbstractCompiler.java +++ b/src/com/google/javascript/jscomp/AbstractCompiler.java @@ -277,7 +277,10 @@ LifeCycleStage getLifeCycleStage() { /** True iff a function changed since the last time a pass was run */ abstract boolean hasScopeChanged(Node n); - /** Passes that do cross-scope modifications use this (eg, InlineVariables) */ + /** + * Passes that make modifications in a scope that is different than the Compiler.currentScope use + * this (eg, InlineVariables and many others) + */ abstract void reportChangeToEnclosingScope(Node n); /** diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java index a4f1549256b..ffe3a501994 100644 --- a/src/com/google/javascript/jscomp/Compiler.java +++ b/src/com/google/javascript/jscomp/Compiler.java @@ -2377,6 +2377,17 @@ void setScope(Node n) { } private Node getEnclosingChangeScope(Node n) { + /** + * Compiler change reporting usually occurs after the AST change has already occurred. In the + * case of node removals those nodes are already removed from the tree and so have no parent + * chain to walk. In these situations changes are reported instead against what (used to be) + * their parent. If that parent is itself a script node then it's important to be able to + * recognize it as the enclosing scope without first stepping to its parent as well. + */ + if (n.isScript()) { + return n; + } + while (n.getParent() != null) { n = n.getParent(); if (n.isFunction() || n.isScript()) { diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index 507bcdbd74a..d4386c9e3e3 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -4758,7 +4758,7 @@ public void visit(Node n) { } else { Preconditions.checkState( isEquivalentToExcludingFunctions(n, clone), - "%schange scope not marked as changed", + "%schanged scope not marked as changed", passNameMsg); } } diff --git a/src/com/google/javascript/jscomp/StripCode.java b/src/com/google/javascript/jscomp/StripCode.java index cb8ffd84e8c..4632155e55e 100644 --- a/src/com/google/javascript/jscomp/StripCode.java +++ b/src/com/google/javascript/jscomp/StripCode.java @@ -261,20 +261,21 @@ void maybeRemoveReferenceToRemovedVariable(NodeTraversal t, Node n, void replaceHighestNestedCallWithNull(Node node, Node parent) { Node ancestor = parent; Node ancestorChild = node; + Node ancestorParent; while (true) { + ancestorParent = ancestor.getParent(); + if (ancestor.getFirstChild() != ancestorChild) { replaceWithNull(ancestorChild, ancestor); break; } if (ancestor.isExprResult()) { // Remove the entire expression statement. - Node ancParent = ancestor.getParent(); - replaceWithEmpty(ancestor, ancParent); + replaceWithEmpty(ancestor, ancestorParent); break; } if (ancestor.isAssign()) { - Node ancParent = ancestor.getParent(); - ancParent.replaceChild(ancestor, ancestor.getLastChild().detach()); + ancestorParent.replaceChild(ancestor, ancestor.getLastChild().detach()); break; } if (!NodeUtil.isGet(ancestor) @@ -282,10 +283,12 @@ void replaceHighestNestedCallWithNull(Node node, Node parent) { replaceWithNull(ancestorChild, ancestor); break; } + + // Is not executed on the last iteration so can't be used for change reporting. ancestorChild = ancestor; - ancestor = ancestor.getParent(); + ancestor = ancestorParent; } - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(ancestorParent); } /** @@ -312,7 +315,7 @@ void maybeEliminateAssignmentByLvalueName(NodeTraversal t, Node n, if (parent.isExprResult()) { Node grandparent = parent.getParent(); replaceWithEmpty(parent, grandparent); - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(grandparent); } else { t.report(n, STRIP_ASSIGNMENT_ERROR, lvalue.getQualifiedName()); } @@ -341,10 +344,11 @@ void maybeEliminateExpressionByName(NodeTraversal t, Node n, if (parent.isExprResult()) { Node grandparent = parent.getParent(); replaceWithEmpty(parent, grandparent); + compiler.reportChangeToEnclosingScope(grandparent); } else { replaceWithEmpty(n, parent); + compiler.reportChangeToEnclosingScope(parent); } - compiler.reportCodeChange(); } } @@ -384,7 +388,7 @@ void eliminateKeysWithStripNamesFromObjLit(NodeTraversal t, Node n) { Node next = key.getNext(); n.removeChild(key); key = next; - compiler.reportCodeChange(); + compiler.reportChangeToEnclosingScope(n); } else { key = key.getNext(); } diff --git a/test/com/google/javascript/jscomp/StripCodeTest.java b/test/com/google/javascript/jscomp/StripCodeTest.java index 832cdb07ba8..eb8ff843cf8 100644 --- a/test/com/google/javascript/jscomp/StripCodeTest.java +++ b/test/com/google/javascript/jscomp/StripCodeTest.java @@ -31,12 +31,6 @@ public StripCodeTest() { super(EXTERNS, true); } - @Override - protected void setUp() throws Exception { - super.setUp(); - validateAstChangeMarking(false); - } - /** * Creates an instance for removing logging code. * @@ -71,7 +65,8 @@ private static StripCode createLoggerInstance(Compiler compiler) { stripNamePrefixes); } - @Override public CompilerPass getProcessor(Compiler compiler) { + @Override + public CompilerPass getProcessor(Compiler compiler) { return createLoggerInstance(compiler); }