diff --git a/src/com/google/javascript/jscomp/IncrementalScopeCreator.java b/src/com/google/javascript/jscomp/IncrementalScopeCreator.java index 02139a2c7ea..b980a6eb1a0 100644 --- a/src/com/google/javascript/jscomp/IncrementalScopeCreator.java +++ b/src/com/google/javascript/jscomp/IncrementalScopeCreator.java @@ -92,6 +92,7 @@ private void invalidateChangedScopes() { if (root.isScript()) { scripts.add(root); } else { + checkState(!root.isRoot()); invalidateRoot(root); } } @@ -121,6 +122,9 @@ public Scope createScope(Node n, Scope parent) { checkState(parent == null || parent instanceof PersistentScope); checkState(parent == null || ((PersistentScope) parent).isValid(), "parent is not valid"); checkState(frozen, "freeze() must be called before retrieving scopes"); + checkArgument(parent != null || n == compiler.getRoot(), + "the shared persistent scope must always be root at the tip of the AST"); + PersistentScope scope = scopesByScopeRoot.get(n); if (scope == null) { scope = (PersistentScope) delegate.createScope(n, parent); @@ -194,12 +198,20 @@ protected PersistentGlobalScope(Node rootNode) { @Override void addChildScope(PersistentLocalScope scope) { - validChildren.put(getContainingScript(scope.getRootNode()), scope); + // Only track child scopes that should be + // invalidated when a "change scope" is changed, + // not scopes that are themselves change scope roots. + if (!NodeUtil.isChangeScopeRoot(scope.getRootNode())) { + Node script = getContainingScript(scope.getRootNode()); + checkState(script.isScript()); + validChildren.put(script, scope); + } } public void invalidate(List invalidatedScripts) { valid = false; for (Node script : invalidatedScripts) { + checkState(script.isScript()); // invalidate any generated child scopes for (PersistentLocalScope scope : validChildren.removeAll(script)) { scope.invalidate(); @@ -296,7 +308,7 @@ public void redeclare(Node n) { /** * A subclass of the traditional Scope class that knows about its children, - * and has methods for updating the scope heirarchy. + * and has methods for updating the scope hierarchy. */ private static class PersistentLocalScope extends PersistentScope { // A list of Scope within the "change scope" (those not crossing function boundaries) @@ -340,29 +352,25 @@ void invalidate() { @Override void refresh(AbstractCompiler compiler, PersistentScope newParent) { - checkArgument(newParent == null || newParent.isValid()); - checkState((parent == null) == (newParent == null)); + checkArgument(newParent != null && newParent.isValid()); + checkState(parent != null); // Even if this scope hasn't been invalidated, its parent scopes may have, - // so we update the scope chaining. - if (parent != null && (!valid || this.parent != newParent)) { - this.parent = newParent; - if (!valid) { - // TODO(johnlenz): It doesn't really matter which - // parent scope in the "change scope" invalidates this scope, - // but if we were previously invalidated no parent - // has this instance in its list, so add it to the new parent. - getParent().addChildScope(this); - } - // Even if the parent hasn't changed the depth might have, update it now. - this.depth = parent.getDepth() + 1; - } + // so update the scope chaining. + parent = newParent; + + // Even if the parent hasn't changed the depth might have, update it now. + depth = parent.getDepth() + 1; // Update the scope if needed. - if (!this.valid) { + if (!valid) { vars.clear(); new ScopeScanner(compiler, this).populate(); - this.valid = true; + valid = true; + + // NOTE(johnlenz): It doesn't really matter which parent scope in the "change scope" + // invalidates this scope so it doesn't need to update when the parent changes. + getParent().addChildScope(this); } } } diff --git a/test/com/google/javascript/jscomp/IncrementalScopeCreatorTest.java b/test/com/google/javascript/jscomp/IncrementalScopeCreatorTest.java index a3123b980d7..86c9f8551d2 100644 --- a/test/com/google/javascript/jscomp/IncrementalScopeCreatorTest.java +++ b/test/com/google/javascript/jscomp/IncrementalScopeCreatorTest.java @@ -32,28 +32,45 @@ public final class IncrementalScopeCreatorTest extends TestCase { public void testMemoization() throws Exception { - Node root1 = IR.root(); - Node root2 = IR.root(); - Compiler compiler = new Compiler(); - compiler.initOptions(new CompilerOptions()); + + List externs = ImmutableList.of( + SourceFile.fromCode("externs.js", "var symbol;var ext")); + List srcs = ImmutableList.of( + SourceFile.fromCode("testcode1.js", "var a; var b; function foo() { var inside = 1; }"), + SourceFile.fromCode("testcode2.js", "var x;")); + Compiler compiler = initCompiler(externs, srcs); ScopeCreator creator = IncrementalScopeCreator.getInstance(compiler).freeze(); + Node root1 = compiler.getRoot(); + Scope scopeA = creator.createScope(root1, null); assertSame(scopeA, creator.createScope(root1, null)); - assertNotSame(scopeA, creator.createScope(root2, null)); + + IncrementalScopeCreator.getInstance(compiler).thaw(); + + IncrementalScopeCreator.getInstance(compiler).freeze(); + + assertSame(scopeA, creator.createScope(root1, null)); + + try { + Node root2 = IR.root(); + creator.createScope(root2, null); + fail(); + } catch (IllegalArgumentException expected) { + assertTrue(expected.getMessage().contains( + "the shared persistent scope must always " + + "be root at the tip of the AST")); + } } public void testParialGlobalScopeRefresh() throws Exception { - - Compiler compiler = new Compiler(); - CompilerOptions options = new CompilerOptions(); List externs = ImmutableList.of( SourceFile.fromCode("externs.js", "var symbol;var ext")); List srcs = ImmutableList.of( - SourceFile.fromCode("testcode1.js", "var a; var b; function foo() {}"), + SourceFile.fromCode("testcode1.js", "var a; var b; function foo() { var inside = 1; }"), SourceFile.fromCode("testcode2.js", "var x;")); + Compiler compiler = initCompiler(externs, srcs); ScopeCreator creator = IncrementalScopeCreator.getInstance(compiler).freeze(); - compiler.init(externs, srcs, options); - compiler.parseInputs(); + checkState(!compiler.hasErrors()); @@ -63,6 +80,9 @@ public void testParialGlobalScopeRefresh() throws Exception { Scope globalScope = creator.createScope(root, null); Scope globalFunction = creator.createScope(fnFoo, globalScope); + // When refreshing a local scope, the Scope object is preserved but the Var objects are + // recreated, so we need to inspect a Var in the scope to see if it has been freshed or not. + Var inside = globalFunction.getVar("inside"); assertTrue(globalScope.isDeclared("a", true)); assertTrue(globalScope.isDeclared("b", true)); @@ -77,6 +97,7 @@ public void testParialGlobalScopeRefresh() throws Exception { assertSame(globalScope, globalScope2); // unchanged local scopes should be preserved assertSame(globalFunction, creator.createScope(fnFoo, globalScope)); + assertSame(inside, globalFunction.getVar("inside")); assertTrue(globalScope2.isDeclared("a", true)); // still declared, scope creator is frozen assertTrue(globalScope2.isDeclared("b", true)); @@ -94,6 +115,7 @@ public void testParialGlobalScopeRefresh() throws Exception { assertSame(globalScope, globalScope3); // unchanged local scopes should be preserved assertSame(globalFunction, creator.createScope(fnFoo, globalScope)); + assertSame(inside, globalFunction.getVar("inside")); assertFalse(globalScope3.isDeclared("a", true)); // no declared, scope creator has refreshed assertTrue(globalScope3.isDeclared("b", true)); @@ -106,16 +128,13 @@ public void testParialGlobalScopeRefresh() throws Exception { public void testRefreshedGlobalScopeWithRedeclaration() throws Exception { - Compiler compiler = new Compiler(); - CompilerOptions options = new CompilerOptions(); List externs = ImmutableList.of( SourceFile.fromCode("externs.js", "")); List srcs = ImmutableList.of( SourceFile.fromCode("testcode1.js", "var a; var b;"), SourceFile.fromCode("testcode2.js", "var a;")); + Compiler compiler = initCompiler(externs, srcs); ScopeCreator creator = IncrementalScopeCreator.getInstance(compiler).freeze(); - compiler.init(externs, srcs, options); - compiler.parseInputs(); checkState(!compiler.hasErrors()); @@ -144,18 +163,50 @@ public void testRefreshedGlobalScopeWithRedeclaration() throws Exception { IncrementalScopeCreator.getInstance(compiler).thaw(); } - public void testPreconditionCheck() throws Exception { - Compiler compiler = new Compiler(); - compiler.initOptions(new CompilerOptions()); - Node root = IR.root(); + public void testValidScopeReparenting() throws Exception { + List externs = ImmutableList.of( + SourceFile.fromCode("externs.js", "var symbol;var ext")); + List srcs = ImmutableList.of( + SourceFile.fromCode("testcode1.js", "var a; var b; " + + " { function foo() { var inside = 1; } }"), + SourceFile.fromCode("testcode2.js", "var x;")); + Compiler compiler = initCompiler(externs, srcs); ScopeCreator creator = IncrementalScopeCreator.getInstance(compiler).freeze(); - Scope scopeA = creator.createScope(root, null); - try { - creator.createScope(root, scopeA); - fail(); - } catch (IllegalArgumentException expected) { - } + checkState(!compiler.hasErrors()); + + Node root = compiler.getRoot(); + Node fnFoo = findDecl(root, "foo"); + checkState(fnFoo.isFunction()); + + Node block = fnFoo.getParent(); + checkState(block.isNormalBlock()); + + Scope globalScope1 = creator.createScope(root, null); + Scope blockScope1 = creator.createScope(block, globalScope1); + Scope fnScope1 = creator.createScope(fnFoo, blockScope1); + assertSame(blockScope1.getDepth() + 1, fnScope1.getDepth()); + assertSame(blockScope1, fnScope1.getParent()); + + // When refreshing a local scope, the Scope object is preserved but the Var objects are + // recreated, so we need to inspect a Var in the scope to see if it has been freshed or not. + Var inside1 = fnScope1.getVar("inside"); + + compiler.reportChangeToEnclosingScope(block); + block.replaceWith(fnFoo.detach()); + + IncrementalScopeCreator.getInstance(compiler).thaw(); + + IncrementalScopeCreator.getInstance(compiler).freeze(); + + Scope globalScope2 = creator.createScope(root, null); + Scope fnScope2 = creator.createScope(fnFoo, globalScope2); + assertSame(fnScope1, fnScope2); + assertSame(globalScope2, fnScope2.getParent()); + assertSame(globalScope2.getDepth() + 1, fnScope2.getDepth()); + assertSame(inside1, fnScope2.getVar("inside")); + + IncrementalScopeCreator.getInstance(compiler).thaw(); } private void removeFirstDecl(Compiler compiler, Node n, String name) { @@ -192,4 +243,12 @@ private static Node find(Node node, return null; } + + Compiler initCompiler(List externs, List srcs) { + Compiler compiler = new Compiler(); + CompilerOptions options = new CompilerOptions(); + compiler.init(externs, srcs, options); + compiler.parseInputs(); + return compiler; + } }