Skip to content

Commit

Permalink
IncrementalScopeCreator:
Browse files Browse the repository at this point in the history 
* Fix inappropriate invalidations of global functions when the containing script was invalidated.
* fix scope reparenting when a valid scope is moved to another scope.
* add a precondition check that a valid global root is provided.
and add associated tests.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=163405537
  • Loading branch information
@concavelenz @brad4d
concavelenz authored and brad4d committed Jul 28, 2017
1 parent 05603ff commit 2ed43d9
Show file tree
Hide file tree
Showing 2 changed files with 111 additions and 44 deletions.
46 changes: 27 additions & 19 deletions src/com/google/javascript/jscomp/IncrementalScopeCreator.java
View file
Expand Up @@ -92,6 +92,7 @@ private void invalidateChangedScopes() {
if (root.isScript()) {
scripts.add(root);
} else {
checkState(!root.isRoot());
invalidateRoot(root);
}
}
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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<Node> invalidatedScripts) {
valid = false;
for (Node script : invalidatedScripts) {
checkState(script.isScript());
// invalidate any generated child scopes
for (PersistentLocalScope scope : validChildren.removeAll(script)) {
scope.invalidate();
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
}
}
Expand Down
109 changes: 84 additions & 25 deletions test/com/google/javascript/jscomp/IncrementalScopeCreatorTest.java
View file
Expand Up @@ -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<SourceFile> externs = ImmutableList.of(
SourceFile.fromCode("externs.js", "var symbol;var ext"));
List<SourceFile> 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<SourceFile> externs = ImmutableList.of(
SourceFile.fromCode("externs.js", "var symbol;var ext"));
List<SourceFile> 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());

Expand All @@ -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));
Expand All @@ -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));
Expand All @@ -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));
Expand All @@ -106,16 +128,13 @@ public void testParialGlobalScopeRefresh() throws Exception {

public void testRefreshedGlobalScopeWithRedeclaration() throws Exception {

Compiler compiler = new Compiler();
CompilerOptions options = new CompilerOptions();
List<SourceFile> externs = ImmutableList.of(
SourceFile.fromCode("externs.js", ""));
List<SourceFile> 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());

Expand Down Expand Up @@ -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<SourceFile> externs = ImmutableList.of(
SourceFile.fromCode("externs.js", "var symbol;var ext"));
List<SourceFile> 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) {
Expand Down Expand Up @@ -192,4 +243,12 @@ private static Node find(Node node,

return null;
}

Compiler initCompiler(List<SourceFile> externs, List<SourceFile> srcs) {
Compiler compiler = new Compiler();
CompilerOptions options = new CompilerOptions();
compiler.init(externs, srcs, options);
compiler.parseInputs();
return compiler;
}
}

0 comments on commit 2ed43d9

Please sign in to comment.