diff --git a/src/com/google/javascript/jscomp/AbstractCompiler.java b/src/com/google/javascript/jscomp/AbstractCompiler.java index eecbcd74970..f3fe7e3fa32 100644 --- a/src/com/google/javascript/jscomp/AbstractCompiler.java +++ b/src/com/google/javascript/jscomp/AbstractCompiler.java @@ -138,6 +138,18 @@ static enum MostRecentTypechecker { */ public abstract void reportCodeChange(); + /** + * 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); + + /** + * Make modifications in a scope that is different than the Compiler.currentScope use + * this (eg, InlineVariables and many others) + */ + abstract void reportChangeToChangeScope(Node changeScopeRoot); + /** * Logs a message under a central logger. */ @@ -263,7 +275,7 @@ LifeCycleStage getLifeCycleStage() { abstract void removeChangeHandler(CodeChangeHandler handler); /** Let the PhaseOptimizer know which scope a pass is currently analyzing */ - abstract void setScope(Node n); + abstract void setChangeScope(Node n); /** A monotonically increasing value to identify a change */ abstract int getChangeStamp(); @@ -277,12 +289,6 @@ LifeCycleStage getLifeCycleStage() { /** True iff a function changed since the last time a pass was run */ abstract boolean hasScopeChanged(Node n); - /** - * 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); - /** * Represents the different contexts for which the compiler could have * distinct configurations. diff --git a/src/com/google/javascript/jscomp/Compiler.java b/src/com/google/javascript/jscomp/Compiler.java index 60fddabe042..d10997956fa 100644 --- a/src/com/google/javascript/jscomp/Compiler.java +++ b/src/com/google/javascript/jscomp/Compiler.java @@ -270,7 +270,7 @@ public SourceFile apply(String filename) { // visited by the "current" NodeTraversal. This can't be thread safe so // we should move it into the NodeTraversal and require explicit changed // nodes elsewhere so we aren't blocked from doing this elsewhere. - private Node currentScope = null; + private Node currentChangeScope = null; // Starts at 0, increases as "interesting" things happen. // Nothing happens at time START_TIME, the first pass starts at time 1. @@ -2389,8 +2389,8 @@ public void incrementChangeStamp() { } @Override - void setScope(Node n) { - currentScope = (n.isFunction() || n.isScript()) ? n : getEnclosingChangeScope(n); + void setChangeScope(Node newChangeScopeRoot) { + currentChangeScope = newChangeScopeRoot; } private Node getEnclosingChangeScope(Node n) { @@ -2433,10 +2433,18 @@ public void reportCodeChange() { // TODO(johnlenz): if this is called with a null scope we need to invalidate everything // but this isn't done, so we need to make this illegal or record this as having // invalidated everything. - if (currentScope != null) { - recordChange(currentScope); - notifyChangeHandlers(); + if (currentChangeScope != null) { + Preconditions.checkState(currentChangeScope.isScript() || currentChangeScope.isFunction()); + recordChange(currentChangeScope); } + notifyChangeHandlers(); + } + + @Override + public void reportChangeToChangeScope(Node changeScopeRoot) { + Preconditions.checkState(changeScopeRoot.isScript() || changeScopeRoot.isFunction()); + recordChange(changeScopeRoot); + notifyChangeHandlers(); } @Override diff --git a/src/com/google/javascript/jscomp/NodeTraversal.java b/src/com/google/javascript/jscomp/NodeTraversal.java index 77eb0c6ffa9..c20d1baece2 100644 --- a/src/com/google/javascript/jscomp/NodeTraversal.java +++ b/src/com/google/javascript/jscomp/NodeTraversal.java @@ -38,6 +38,12 @@ public class NodeTraversal { /** Contains the current node*/ private Node curNode; + /** The current script being visited */ + private Node curScript; + + /** The change scope for the current node being visiteds */ + private Node currentChangeScope; + /** * Stack containing the Scopes that have been created. The Scope objects * are lazily created; so the {@code scopeRoots} stack contains the @@ -269,7 +275,6 @@ public NodeTraversal(AbstractCompiler compiler, Callback cb, this.scopeCallback = (ScopedCallback) cb; } this.compiler = compiler; - setInputId(null, ""); this.scopeCreator = scopeCreator; this.useBlockScope = scopeCreator.hasBlockScope(); } @@ -304,7 +309,7 @@ private String formatNodeContext(String label, Node n) { */ public void traverse(Node root) { try { - setInputId(NodeUtil.getInputId(root), ""); + initTraversal(root); curNode = root; pushScope(root); // null parent ensures that the shallow callbacks will traverse root @@ -320,7 +325,7 @@ void traverseRoots(Node externs, Node root) { Node scopeRoot = externs.getParent(); Preconditions.checkNotNull(scopeRoot); - setInputId(NodeUtil.getInputId(scopeRoot), ""); + initTraversal(scopeRoot); curNode = scopeRoot; pushScope(scopeRoot); @@ -360,7 +365,7 @@ private String formatNodePosition(Node n) { void traverseWithScope(Node root, Scope s) { Preconditions.checkState(s.isGlobal() || s.isModuleScope()); try { - setInputId(null, ""); + initTraversal(root); curNode = root; pushScope(s); traverseBranch(root, null); @@ -376,6 +381,7 @@ void traverseWithScope(Node root, Scope s) { */ void traverseAtScope(Scope s) { Node n = s.getRootNode(); + initTraversal(n); curNode = n; Deque parentScopes = new ArrayDeque<>(); Scope temp = s.getParent(); @@ -387,11 +393,6 @@ void traverseAtScope(Scope s) { pushScope(parentScopes.pop(), true); } if (n.isFunction()) { - // We need to do some extra magic to make sure that the scope doesn't - // get re-created when we dive into the function. - if (inputId == null) { - setInputId(NodeUtil.getInputId(n), getSourceName(n)); - } pushScope(s); Node args = n.getSecondChild(); @@ -401,9 +402,6 @@ void traverseAtScope(Scope s) { popScope(); } else if (n.isNormalBlock()) { - if (inputId == null) { - setInputId(NodeUtil.getInputId(n), getSourceName(n)); - } pushScope(s); // traverseBranch is not called here to avoid re-creating the block scope. @@ -428,9 +426,7 @@ public void traverseFunctionOutOfBand(Node node, Scope scope) { Preconditions.checkNotNull(scope); Preconditions.checkState(node.isFunction()); Preconditions.checkState(scope.getRootNode() != null); - if (inputId == null) { - setInputId(NodeUtil.getInputId(node), getSourceName(node)); - } + initTraversal(node); curNode = node.getParent(); pushScope(scope, true /* quietly */); traverseBranch(node, curNode); @@ -449,9 +445,7 @@ public void traverseFunctionOutOfBand(Node node, Scope scope) { */ void traverseInnerNode(Node node, Node parent, Scope refinedScope) { Preconditions.checkNotNull(parent); - if (inputId == null) { - setInputId(NodeUtil.getInputId(node), getSourceName(node)); - } + initTraversal(node); if (refinedScope != null && getScope() != refinedScope) { curNode = node; pushScope(refinedScope); @@ -541,8 +535,8 @@ public Node getCurrentNode() { * doesn't know which scope changed. We keep track of the current scope by * calling Compiler.setScope inside pushScope and popScope. * The automatic tracking can be wrong in rare cases when a pass changes scope - * w/out causing a call to pushScope or popScope. It's very hard to find the - * places where this happens unless a bug is triggered. + * w/out causing a call to pushScope or popScope. + * * Passes that do cross-scope modifications call * Compiler.reportChangeToEnclosingScope(Node n). */ @@ -609,14 +603,44 @@ public static void traverseRootsTyped( t.traverseRoots(externs, root); } + private void handleScript(Node n, Node parent) { + setChangeScope(n); + curScript = n; + setInputId(n.getInputId(), getSourceName(n)); + + curNode = n; + if (callback.shouldTraverse(this, n, parent)) { + traverseChildren(n); + curNode = n; + callback.visit(this, n, parent); + } + curScript = null; + setChangeScope(null); + } + + private void handleFunction(Node n, Node parent) { + Node changeScope = this.currentChangeScope; + setChangeScope(n); + curNode = n; + if (callback.shouldTraverse(this, n, parent)) { + traverseFunction(n, parent); + curNode = n; + callback.visit(this, n, parent); + } + setChangeScope(changeScope); + } + /** * Traverses a branch. */ private void traverseBranch(Node n, Node parent) { Token type = n.getToken(); if (type == Token.SCRIPT) { - setInputId(n.getInputId(), getSourceName(n)); - compiler.setScope(n); + handleScript(n, parent); + return; + } else if (type == Token.FUNCTION) { + handleFunction(n, parent); + return; } curNode = n; @@ -738,7 +762,6 @@ public Node getEnclosingFunction() { /** Sets the given node as the current scope and pushes the relevant frames on the CFG stacks. */ private void recordScopeRoot(Node node) { - compiler.setScope(node); if (NodeUtil.isValidCfgRoot(node)) { cfgs.push(node); } @@ -792,10 +815,6 @@ private void popScope(boolean quietly) { if (NodeUtil.isValidCfgRoot(scopeRoot)) { cfgs.pop(); } - Node newScopeRoot = getScopeRoot(); - if (newScopeRoot != null) { - compiler.setScope(newScopeRoot); - } } /** Gets the current scope. */ @@ -927,11 +946,51 @@ public void report(Node n, DiagnosticType diagnosticType, compiler.report(error); } + public void reportCodeChange() { + Node changeScope = this.currentChangeScope; + Preconditions.checkState(changeScope != null && NodeUtil.isChangeScopeRoot(changeScope)); + compiler.reportChangeToChangeScope(changeScope); + } + + public void reportCodeChange(Node n) { + compiler.reportChangeToEnclosingScope(n); + } + private static String getSourceName(Node n) { String name = n.getSourceFileName(); return nullToEmpty(name); } + /** + * @param n The current change scope, should be null when the traversal is complete. + */ + private void setChangeScope(Node n) { + this.currentChangeScope = n; + // TODO(johnlenz): the compiler is a bad place to store this value + // multiple traversals can interfer with each other + // (even on the same thread). + compiler.setChangeScope(n); + } + + private Node getEnclosingScript(Node n) { + while (n != null && !n.isScript()) { + n = n.getParent(); + } + return n; + } + + private void initTraversal(Node traversalRoot) { + Node changeScope = NodeUtil.getEnclosingChangeScopeRoot(traversalRoot); + setChangeScope(changeScope); + Node script = getEnclosingScript(changeScope); + if (script != null) { + setInputId(script.getInputId(), script.getSourceFileName()); + } else { + setInputId(null, ""); + } + this.curScript = script; + } + private void setInputId(InputId id, String sourceName) { inputId = id; this.sourceName = sourceName; diff --git a/src/com/google/javascript/jscomp/NodeUtil.java b/src/com/google/javascript/jscomp/NodeUtil.java index d4386c9e3e3..cca00e736af 100644 --- a/src/com/google/javascript/jscomp/NodeUtil.java +++ b/src/com/google/javascript/jscomp/NodeUtil.java @@ -4707,6 +4707,26 @@ static boolean isNaN(Node n) { && n.getLastChild().getDouble() == 0); } + /** + * A change scope does not directly correspond to a language scope but is an internal + * grouping of changes. + * + * @return Whether the node represents a change scope root. + */ + static boolean isChangeScopeRoot(Node n) { + return (n.isScript() || n.isFunction()); + } + + /** + * @return the change scope root + */ + static Node getEnclosingChangeScopeRoot(Node n) { + while (n != null && !isChangeScopeRoot(n)) { + n = n.getParent(); + } + return n; + } + /** * Given an AST and its copy, map the root node of each scope of main to the * corresponding root node of clone @@ -4739,34 +4759,38 @@ public static void verifyScopeChanges( final String passNameMsg = passName.isEmpty() ? "" : passName + ": "; Node clone = mtoc.get(main); - if (main.getChangeTime() > clone.getChangeTime()) { - Preconditions.checkState(!isEquivalentToExcludingFunctions(main, clone)); - } else { - Preconditions.checkState(isEquivalentToExcludingFunctions(main, clone)); - } + verifyNodeChange(passNameMsg, main, clone); visitPreOrder(main, new Visitor() { @Override public void visit(Node n) { if ((n.isScript() || n.isFunction()) && mtoc.containsKey(n)) { Node clone = mtoc.get(n); - if (n.getChangeTime() > clone.getChangeTime()) { - Preconditions.checkState( - !isEquivalentToExcludingFunctions(n, clone), - "%sunchanged scope marked as changed", - passNameMsg); - } else { - Preconditions.checkState( - isEquivalentToExcludingFunctions(n, clone), - "%schanged scope not marked as changed", - passNameMsg); - } + verifyNodeChange(passNameMsg, n, clone); } } }, Predicates.alwaysTrue()); } + static void verifyNodeChange(final String passNameMsg, Node n, Node clone) { + if (n.getChangeTime() > clone.getChangeTime()) { + if (isEquivalentToExcludingFunctions(n, clone)) { + throw new IllegalStateException( + passNameMsg + + "unchanged scope marked as changed: " + + n.toStringTree()); + } + } else { + if (!isEquivalentToExcludingFunctions(n, clone)) { + throw new IllegalStateException( + passNameMsg + + "change scope not marked as changed: " + + n.toStringTree()); + } + } + } + static int countAstSizeUpToLimit(Node n, final int limit) { // Java doesn't allow accessing mutable local variables from another class. final int[] wrappedSize = {0}; diff --git a/src/com/google/javascript/jscomp/PhaseOptimizer.java b/src/com/google/javascript/jscomp/PhaseOptimizer.java index b63a855506a..42670d3606d 100644 --- a/src/com/google/javascript/jscomp/PhaseOptimizer.java +++ b/src/com/google/javascript/jscomp/PhaseOptimizer.java @@ -395,9 +395,7 @@ public void process(Node externs, Node root) { scopeHandler = new ScopedChangeHandler(); compiler.addChangeHandler(scopeHandler); - // TODO(johnlenz): It is unclear why "setScope" is called here. Try to remove - // this. - compiler.setScope(root); + compiler.setChangeScope(null); // lastRuns is initialized before each loop. This way, when a pass is run // in the 2nd loop for the 1st time, it looks at all scopes. diff --git a/test/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritanceTest.java b/test/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritanceTest.java index 167d5c09042..e3f4e67a309 100644 --- a/test/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritanceTest.java +++ b/test/com/google/javascript/jscomp/Es6ToEs3ClassSideInheritanceTest.java @@ -28,6 +28,7 @@ public void setUp() { setAcceptedLanguage(LanguageMode.ECMASCRIPT_2015); setLanguageOut(LanguageMode.ECMASCRIPT3); allowExternsChanges(true); + validateAstChangeMarking(false); } @Override diff --git a/test/com/google/javascript/jscomp/NodeTraversalTest.java b/test/com/google/javascript/jscomp/NodeTraversalTest.java index 7cfce03f613..360cb6e6bfb 100644 --- a/test/com/google/javascript/jscomp/NodeTraversalTest.java +++ b/test/com/google/javascript/jscomp/NodeTraversalTest.java @@ -23,10 +23,12 @@ import com.google.common.collect.ImmutableSet; import com.google.javascript.jscomp.CompilerOptions.LanguageMode; import com.google.javascript.jscomp.NodeTraversal.AbstractNodeTypePruningCallback; +import com.google.javascript.rhino.IR; import com.google.javascript.rhino.Node; import com.google.javascript.rhino.Token; import java.util.ArrayList; import java.util.List; +import java.util.Map; import java.util.Set; import junit.framework.TestCase; @@ -157,6 +159,72 @@ public void visit(NodeTraversal t, Node n, Node parent) { ); } + private static class NameChangingCallback implements NodeTraversal.Callback { + @Override + public boolean shouldTraverse(NodeTraversal t, Node n, Node parent) { + return true; + } + + @Override + public void visit(NodeTraversal t, Node n, Node parent) { + if (n.isName() && n.getString().equals("change")) { + n.setString("xx"); + t.reportCodeChange(); + } + } + } + + public void testReportChange1() { + String code = LINE_JOINER.join( + "var change;", + "function foo() {", + " var b", + "}"); + assertChangesRecorded(code, new NameChangingCallback()); + } + + public void testReportChange2() { + String code = LINE_JOINER.join( + "var a;", + "function foo() {", + " var change", + "}"); + assertChangesRecorded(code, new NameChangingCallback()); + } + + public void testReportChange3() { + String code = LINE_JOINER.join( + "var a;", + "function foo() {", + " var b", + "}", + "var change"); + assertChangesRecorded(code, new NameChangingCallback()); + } + + public void testReportChange4() { + String code = LINE_JOINER.join( + "function foo() {", + " function bar() {", + " var change", + " }", + "}"); + assertChangesRecorded(code, new NameChangingCallback()); + } + + private void assertChangesRecorded(String code, NodeTraversal.Callback callback) { + final String externs = ""; + Compiler compiler = new Compiler(); + Node tree = parseRoots(compiler, externs, code); + System.out.println("parse done"); + Node clone = tree.cloneTree(); + Map map = NodeUtil.mapMainToClone(tree, clone); + NodeTraversal.traverseRootsEs6( + compiler, callback, tree.getFirstChild(), tree.getSecondChild()); + NodeUtil.verifyScopeChanges("test", map, tree); + } + + public void testGetLineNoAndGetCharno() { Compiler compiler = new Compiler(); String code = "" @@ -348,4 +416,11 @@ private static Node parse(Compiler compiler, String js) { assertThat(compiler.getErrors()).isEmpty(); return n; } + + private static Node parseRoots(Compiler compiler, String externs, String js) { + Node extern = parse(compiler, externs); + Node main = parse(compiler, js); + + return IR.root(IR.root(extern), IR.root(main)); + } } diff --git a/test/com/google/javascript/jscomp/WhitespaceWrapGoogModulesTest.java b/test/com/google/javascript/jscomp/WhitespaceWrapGoogModulesTest.java index 7acac085230..db824e8356a 100644 --- a/test/com/google/javascript/jscomp/WhitespaceWrapGoogModulesTest.java +++ b/test/com/google/javascript/jscomp/WhitespaceWrapGoogModulesTest.java @@ -31,6 +31,7 @@ public void setUp() { // otherwise "use strict" in the expected output moves, // from where it should be (deliberately to match ClosureBundler), // to the top of the AST and breaks the comparison. + validateAstChangeMarking(false); } @Override