Skip to content

Commit

Permalink
Update the AST change verification to make sure that changes to funct…
Browse files Browse the repository at this point in the history
…ion declarations nodes mark the containing change scope as changed as well as the function itself.

This ensures that when scopes are regenerated only when need, they will be when the names change.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=154470179
  • Loading branch information
concavelenz authored and Tyler Breisacher committed Apr 28, 2017
1 parent 1231aa8 commit 22fc218
Show file tree
Hide file tree
Showing 6 changed files with 60 additions and 18 deletions.
Expand Up @@ -163,7 +163,12 @@ public void visit(NodeTraversal t, Node n, Node parent) {
n.removeProp(Node.IS_CONSTANT_NAME);
}
n.setString(newName);
t.getCompiler().reportChangeToEnclosingScope(n);
t.reportCodeChange();
// If we are renaming a function declaration, make sure the containing scope
// has the opporunity to act on the change.
if (parent.isFunction() && NodeUtil.isFunctionDeclaration(parent)) {
t.getCompiler().reportChangeToEnclosingScope(parent);
}
}
break;

Expand Down
12 changes: 11 additions & 1 deletion src/com/google/javascript/jscomp/NodeUtil.java
Expand Up @@ -4832,10 +4832,20 @@ private static boolean isEquivalentToExcludingFunctions(
Node thatChild = thatNode.getFirstChild();
while (thisChild != null && thatChild != null) {
if (thisChild.isFunction() || thisChild.isScript()) {
// don't compare function name, parameters or bodies.
// Don't compare function expression name, parameters or bodies.
// But do check that that the node is there.
if (thatChild.getToken() != thisChild.getToken()) {
return false;
}
// Only compare function names for function declarations (not function expressions)
// as they change the outer scope definition.
if (thisChild.isFunction() && NodeUtil.isFunctionDeclaration(thisChild)) {
String thisName = thisChild.getFirstChild().getString();
String thatName = thatChild.getFirstChild().getString();
if (!thisName.equals(thatName)) {
return false;
}
}
} else if (!isEquivalentToExcludingFunctions(thisChild, thatChild)) {
return false;
}
Expand Down
27 changes: 16 additions & 11 deletions src/com/google/javascript/jscomp/RenameVars.java
Expand Up @@ -364,28 +364,33 @@ public void process(Node externs, Node root) {

// Rename the globals!
for (Node n : globalNameNodes) {
String newName = getNewGlobalName(n);
// Note: if newName is null, then oldName is an extern.
if (newName != null) {
n.setString(newName);
compiler.reportChangeToEnclosingScope(n);
}
setNameAndReport(n, getNewGlobalName(n));
}

// Rename the locals!
for (Node n : localNameNodes) {
String newName = getNewLocalName(n);
if (newName != null) {
n.setString(newName);
compiler.reportChangeToEnclosingScope(n);
}
setNameAndReport(n, getNewLocalName(n));
}

// Lastly, write the name assignments to the debug log.
compiler.addToDebugLog("JS var assignments:\n" + assignmentLog);
assignmentLog = null;
}

private void setNameAndReport(Node n, String newName) {
// A null newName, indicates it should not be renamed.
if (newName != null) {
n.setString(newName);
compiler.reportChangeToEnclosingScope(n);
Node parent = n.getParent();
if (parent.isFunction() && NodeUtil.isFunctionDeclaration(parent)) {
// If we are renaming a function declaration, make sure the containing scope
// has the opporunity to act on the change.
compiler.reportChangeToEnclosingScope(parent);
}
}
}

private String getNewGlobalName(Node n) {
String oldName = n.getString();
Assignment a = assignments.get(oldName);
Expand Down
9 changes: 5 additions & 4 deletions src/com/google/javascript/jscomp/ShadowVariables.java
Expand Up @@ -179,10 +179,11 @@ public void enterScope(NodeTraversal t) {

// Since we don't shadow global, there is nothing to be done in the
// first immediate local scope as well.
if ((t.getScopeRoot().isFunction()
&& NodeUtil.getEnclosingFunction(t.getScopeRoot().getParent()) == null)
|| (NodeUtil.isFunctionBlock(t.getScopeRoot())
&& NodeUtil.getEnclosingFunction(t.getScopeRoot().getGrandparent()) == null)) {
Node scopeRoot = t.getScopeRoot();
if ((scopeRoot.isFunction()
&& NodeUtil.getEnclosingFunction(scopeRoot.getParent()) == null)
|| (NodeUtil.isFunctionBlock(scopeRoot)
&& NodeUtil.getEnclosingFunction(scopeRoot.getGrandparent()) == null)) {
return;
}

Expand Down
Expand Up @@ -497,7 +497,7 @@ public void testConstRemovingRename2() {
"var CONST$jscomp$unique_0 = 3; var b$jscomp$unique_1 = CONST$jscomp$unique_0;");
}

public void testVarParamSameName() {
public void testVarParamSameName0() {
test(
LINE_JOINER.join(
"function f(x) {",
Expand All @@ -507,7 +507,9 @@ public void testVarParamSameName() {
"function f$jscomp$unique_0(x$jscomp$unique_1) {",
" if (!x$jscomp$unique_1) var x$jscomp$unique_1 = 6;",
"}"));
}

public void testVarParamSameName1() {
test(
LINE_JOINER.join(
"function f(x) {",
Expand All @@ -517,7 +519,9 @@ public void testVarParamSameName() {
"function f$jscomp$unique_0(x$jscomp$unique_1) {",
" if (!x$jscomp$unique_1) x$jscomp$unique_1 = 6; ",
"}"));
}

public void testVarParamSameAsLet0() {
testEs6(
LINE_JOINER.join(
"function f(x) {",
Expand Down
17 changes: 17 additions & 0 deletions test/com/google/javascript/jscomp/NodeUtilTest.java
Expand Up @@ -2540,6 +2540,23 @@ static void testFunctionName(String js, String expected) {
NodeUtil.getNearestFunctionName(getFunctionNode(js)));
}

public static void testChangeVerification() {
Node mainScript = IR.script();
Node main = IR.root(mainScript);

Node clone = main.cloneTree();

Map<Node, Node> map = NodeUtil.mapMainToClone(main, clone);

NodeUtil.verifyScopeChanges("", map, main);

mainScript.addChildToFront(
IR.function(IR.name("A"), IR.paramList(), IR.block()));
mainScript.setChangeTime(100);

NodeUtil.verifyScopeChanges("", map, main);
}

static Node getClassNode(String js) {
Node root = parse(js);
return getClassNode(root);
Expand Down

0 comments on commit 22fc218

Please sign in to comment.