Skip to content

Commit

Permalink
Fixes change tracking in StripCode.
Browse files Browse the repository at this point in the history
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=152965110
  • Loading branch information
stalcup authored and brad4d committed Apr 13, 2017
1 parent 5a1bc0b commit 0670466
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 18 deletions.
5 changes: 4 additions & 1 deletion src/com/google/javascript/jscomp/AbstractCompiler.java
Expand Up @@ -277,7 +277,10 @@ LifeCycleStage getLifeCycleStage() {
/** True iff a function changed since the last time a pass was run */ /** True iff a function changed since the last time a pass was run */
abstract boolean hasScopeChanged(Node n); 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); abstract void reportChangeToEnclosingScope(Node n);


/** /**
Expand Down
11 changes: 11 additions & 0 deletions src/com/google/javascript/jscomp/Compiler.java
Expand Up @@ -2377,6 +2377,17 @@ void setScope(Node n) {
} }


private Node getEnclosingChangeScope(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) { while (n.getParent() != null) {
n = n.getParent(); n = n.getParent();
if (n.isFunction() || n.isScript()) { if (n.isFunction() || n.isScript()) {
Expand Down
2 changes: 1 addition & 1 deletion src/com/google/javascript/jscomp/NodeUtil.java
Expand Up @@ -4758,7 +4758,7 @@ public void visit(Node n) {
} else { } else {
Preconditions.checkState( Preconditions.checkState(
isEquivalentToExcludingFunctions(n, clone), isEquivalentToExcludingFunctions(n, clone),
"%schange scope not marked as changed", "%schanged scope not marked as changed",
passNameMsg); passNameMsg);
} }
} }
Expand Down
22 changes: 13 additions & 9 deletions src/com/google/javascript/jscomp/StripCode.java
Expand Up @@ -261,31 +261,34 @@ void maybeRemoveReferenceToRemovedVariable(NodeTraversal t, Node n,
void replaceHighestNestedCallWithNull(Node node, Node parent) { void replaceHighestNestedCallWithNull(Node node, Node parent) {
Node ancestor = parent; Node ancestor = parent;
Node ancestorChild = node; Node ancestorChild = node;
Node ancestorParent;
while (true) { while (true) {
ancestorParent = ancestor.getParent();

if (ancestor.getFirstChild() != ancestorChild) { if (ancestor.getFirstChild() != ancestorChild) {
replaceWithNull(ancestorChild, ancestor); replaceWithNull(ancestorChild, ancestor);
break; break;
} }
if (ancestor.isExprResult()) { if (ancestor.isExprResult()) {
// Remove the entire expression statement. // Remove the entire expression statement.
Node ancParent = ancestor.getParent(); replaceWithEmpty(ancestor, ancestorParent);
replaceWithEmpty(ancestor, ancParent);
break; break;
} }
if (ancestor.isAssign()) { if (ancestor.isAssign()) {
Node ancParent = ancestor.getParent(); ancestorParent.replaceChild(ancestor, ancestor.getLastChild().detach());
ancParent.replaceChild(ancestor, ancestor.getLastChild().detach());
break; break;
} }
if (!NodeUtil.isGet(ancestor) if (!NodeUtil.isGet(ancestor)
&& !ancestor.isCall()) { && !ancestor.isCall()) {
replaceWithNull(ancestorChild, ancestor); replaceWithNull(ancestorChild, ancestor);
break; break;
} }

// Is not executed on the last iteration so can't be used for change reporting.
ancestorChild = ancestor; ancestorChild = ancestor;
ancestor = ancestor.getParent(); ancestor = ancestorParent;
} }
compiler.reportCodeChange(); compiler.reportChangeToEnclosingScope(ancestorParent);
} }


/** /**
Expand All @@ -312,7 +315,7 @@ void maybeEliminateAssignmentByLvalueName(NodeTraversal t, Node n,
if (parent.isExprResult()) { if (parent.isExprResult()) {
Node grandparent = parent.getParent(); Node grandparent = parent.getParent();
replaceWithEmpty(parent, grandparent); replaceWithEmpty(parent, grandparent);
compiler.reportCodeChange(); compiler.reportChangeToEnclosingScope(grandparent);
} else { } else {
t.report(n, STRIP_ASSIGNMENT_ERROR, lvalue.getQualifiedName()); t.report(n, STRIP_ASSIGNMENT_ERROR, lvalue.getQualifiedName());
} }
Expand Down Expand Up @@ -341,10 +344,11 @@ void maybeEliminateExpressionByName(NodeTraversal t, Node n,
if (parent.isExprResult()) { if (parent.isExprResult()) {
Node grandparent = parent.getParent(); Node grandparent = parent.getParent();
replaceWithEmpty(parent, grandparent); replaceWithEmpty(parent, grandparent);
compiler.reportChangeToEnclosingScope(grandparent);
} else { } else {
replaceWithEmpty(n, parent); replaceWithEmpty(n, parent);
compiler.reportChangeToEnclosingScope(parent);
} }
compiler.reportCodeChange();
} }
} }


Expand Down Expand Up @@ -384,7 +388,7 @@ void eliminateKeysWithStripNamesFromObjLit(NodeTraversal t, Node n) {
Node next = key.getNext(); Node next = key.getNext();
n.removeChild(key); n.removeChild(key);
key = next; key = next;
compiler.reportCodeChange(); compiler.reportChangeToEnclosingScope(n);
} else { } else {
key = key.getNext(); key = key.getNext();
} }
Expand Down
9 changes: 2 additions & 7 deletions test/com/google/javascript/jscomp/StripCodeTest.java
Expand Up @@ -31,12 +31,6 @@ public StripCodeTest() {
super(EXTERNS, true); super(EXTERNS, true);
} }


@Override
protected void setUp() throws Exception {
super.setUp();
validateAstChangeMarking(false);
}

/** /**
* Creates an instance for removing logging code. * Creates an instance for removing logging code.
* *
Expand Down Expand Up @@ -71,7 +65,8 @@ private static StripCode createLoggerInstance(Compiler compiler) {
stripNamePrefixes); stripNamePrefixes);
} }


@Override public CompilerPass getProcessor(Compiler compiler) { @Override
public CompilerPass getProcessor(Compiler compiler) {
return createLoggerInstance(compiler); return createLoggerInstance(compiler);
} }


Expand Down

0 comments on commit 0670466

Please sign in to comment.