Skip to content

Commit

Permalink
Added optimization to removeUnusedVar pass for destructuring ES6 feature
Browse files Browse the repository at this point in the history 
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=164043393
  • Loading branch information
@simran-arora @dimvar
simran-arora authored and dimvar committed Aug 3, 2017
1 parent afecc10 commit 37cd66f
Show file tree
Hide file tree
Showing 4 changed files with 284 additions and 3 deletions.
101 changes: 101 additions & 0 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -49,6 +49,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.EnumSet;
import java.util.HashMap;
import java.util.Iterator;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
Expand Down Expand Up @@ -2774,6 +2775,31 @@ public static void removeChild(Node parent, Node node) {
// need something for the condition. Others need to be replaced
// or the structure removed.
parent.replaceChild(node, IR.empty());
} else if (parent.isObjectPattern()) {
// Remove the name from the object pattern

// TODO (simranarora) remove the pattern and declaration when nothing is left and if there is
// an object literal on the right hand side, wrap it in an expression result
// E.g. var { } = { a:foo(), b:bar()} --> ({a:foo(), b:bar()})

parent.removeChild(node);
} else if (parent.isArrayPattern()) {
// Leave an empty slot for the array pattern node that gets removed.
// E.g. var [a, b] = [1, 2] becomes var [a, ,] = [1, 2]

// TODO (simranarora) remove the pattern and declaration when nothing is left and just keep
// the initializer expression.
// E.g. var [ , ,] = [1, 2]; --> [1, 2];

parent.replaceChild(node, IR.empty());
} else if (parent.isDestructuringLhs()) {
// Destructuring is empty so we should remove the node
parent.removeChild(node);
if (parent.getParent().hasChildren()) {
// removing the destructuring could leave an empty variable declaration node, so we would
// want to remove it from the AST
removeChild(parent.getParent(), parent);
}
} else {
throw new IllegalStateException("Invalid attempt to remove node: " + node + " of " + parent);
}
Expand Down Expand Up @@ -3236,6 +3262,39 @@ static Node getObjectLitKeyNode(Node key) {
throw new IllegalStateException("Unexpected node type: " + key);
}

/**
* Determine whether the destructuring object pattern is nested
*
* @param n object pattern node
*/
static boolean isNestedObjectPattern(Node n) {
checkState(n.isObjectPattern());
Map<String, Node> destructuringMap = NodeUtil.getDestructuringMap(n.getParent());
for (Node value : destructuringMap.values()) {
if (value.isObjectLit()
|| value.isArrayLit()
|| (value.isName() && value.getFirstChild() == null)) {
return true;
}
}
return false;
}

/**
* Determine whether the destructuring array pattern is nested
*
* @param n array pattern node
*/
static boolean isNestedArrayPattern(Node n) {
checkState(n.isArrayPattern());
for (Node key = n.getFirstChild(); key != null; key = key.getNext()) {
if (key.hasChildren()) {
return true;
}
}
return false;
}

/**
* Determines whether a node represents an object literal get or set key
* (e.g. key1 in {get key1() {}, set key2(a){}).
Expand Down Expand Up @@ -5189,6 +5248,48 @@ public void visit(NodeTraversal t, Node n, Node parent) {}
t.traverseAtScope(scope);
}

/**
* Matches together a destructuring assignment with the value assigned
*
* @param n a destructuring node where the first child is the pattern array or object and the
* second child is an optional array or object literal
* @return map from the variable to value assigned (value assigned can be null!)
*/
static Map<String, Node> getDestructuringMap(Node n) {
checkState(n.isDestructuringLhs());
Map<String, Node> destructurePairs = new HashMap<>();

boolean objLitRhs = n.getSecondChild().isObjectLit();
boolean arrLitRhs = n.getSecondChild().isArrayLit();
if (objLitRhs) {
Node literal = n.getSecondChild();
for (Node value : literal.children()) {
destructurePairs.put(value.getString(), value.getFirstChild());
}
} else if (arrLitRhs) {
Node pattern = n.getFirstChild();
Node literal = n.getSecondChild();
Node value = literal.getFirstChild();
for (Node key : pattern.children()) {
destructurePairs.put(key.getString(), value);
if (value != null) {
value = value.getNext();
}
}
} else if (n.getSecondChild() != null) {
Node pattern = n.getFirstChild();
for (Node key : pattern.children()) {
destructurePairs.put(key.getString(), n.getSecondChild());
}
} else {
Node pattern = n.getFirstChild();
for (Node key : pattern.children()) {
destructurePairs.put(key.getString(), null);
}
}
return destructurePairs;
}

/** Returns true if the node is a property of an object literal. */
public static boolean isObjLitProperty(Node node) {
return node.isStringKey()
Expand Down
44 changes: 43 additions & 1 deletion src/com/google/javascript/jscomp/RemoveUnusedVars.java
View file
Expand Up @@ -310,6 +310,39 @@ private void traverseNode(Node n, Node parent, Scope scope) {
}
return;

case ARRAY_PATTERN:
// VAR or LET or CONST
// DESTRUCTURING_LHS
// ARRAY_PATTERN
// NAME

// back off if there are nested array patterns
if (n.getParent().isDestructuringLhs()) {
if (NodeUtil.isNestedArrayPattern(n)) {
break;
} else {
return;
}
}
break;

case OBJECT_PATTERN:
// VAR or LET or CONST
// DESTRUCTURING_LHS
// OBJECT_PATTERN
// STRING
// NAME

// back off if there are nested object patterns
if (n.getParent().isDestructuringLhs()) {
if (NodeUtil.isNestedObjectPattern(n)) {
break;
} else {
return;
}
}
break;

case NAME:
var = scope.getVar(n.getString());
if (NodeUtil.isNameDeclaration(parent)) {
Expand Down Expand Up @@ -775,7 +808,6 @@ private Definition getFunctionDefinition(Node function) {
}
}


/**
* Look at all the property assigns to all variables.
* These may or may not count as references. For example,
Expand Down Expand Up @@ -893,10 +925,14 @@ private void removeUnreferencedVars() {
Node nameNode = var.nameNode;
Node toRemove = nameNode.getParent();
Node parent = toRemove.getParent();
Node grandParent = toRemove.getGrandparent();
checkState(
NodeUtil.isNameDeclaration(toRemove)
|| toRemove.isFunction()
|| (toRemove.isParamList() && parent.isFunction())
|| NodeUtil.isDestructuringDeclaration(grandParent) // Array Pattern
|| (parent.isObjectPattern()
&& NodeUtil.isDestructuringDeclaration(grandParent.getParent())) // Object Pattern
|| toRemove.isClass(),
"We should only declare Vars and functions and function args and classes");

Expand All @@ -913,6 +949,12 @@ private void removeUnreferencedVars() {
// Don't remove bleeding functions.
} else if (parent.isForIn()) {
// foreach iterations have 3 children. Leave them alone.
} else if (parent.isDestructuringPattern()) {
compiler.reportChangeToEnclosingScope(toRemove);
NodeUtil.removeChild(parent, toRemove);
} else if (parent.isDestructuringLhs()) {
compiler.reportChangeToEnclosingScope(nameNode);
NodeUtil.removeChild(toRemove, nameNode);
} else if (NodeUtil.isNameDeclaration(toRemove)
&& nameNode.hasChildren()
&& NodeUtil.mayHaveSideEffects(nameNode.getFirstChild(), compiler)) {
Expand Down
101 changes: 101 additions & 0 deletions test/com/google/javascript/jscomp/NodeUtilTest.java
View file
Expand Up @@ -1059,6 +1059,41 @@ public void testRemoveLabelChild2() {
}
}

public void testRemovePatternChild() {
// remove variable declaration from object pattern
Node actual = parse("var {a, b, c} = {a:1, b:2, c:3}");
Node varNode = actual.getFirstChild();
Node destructure = varNode.getFirstChild();
Node pattern = destructure.getFirstChild();
Node a = pattern.getFirstChild();
Node b = a.getNext();
Node c = b.getNext();

NodeUtil.removeChild(pattern, a);
String expected = "var {b, c} = {a:1, b:2, c:3};";
String difference = parse(expected).checkTreeEquals(actual);
assertNull("Nodes do not match:\n" + difference, difference);

// Remove all entries in object pattern
NodeUtil.removeChild(pattern, b);
NodeUtil.removeChild(pattern, c);
expected = "var { } = {a:1, b:2, c:3};";
difference = parse(expected).checkTreeEquals(actual);
assertNull("Nodes do not match:\n" + difference, difference);

// remove variable declaration from array pattern
actual = parse("var [a, b] = [1, 2]");
varNode = actual.getFirstChild();
destructure = varNode.getFirstChild();
pattern = destructure.getFirstChild();
a = pattern.getFirstChild();

NodeUtil.removeChild(pattern, a);
expected = "var [ , b] = [1, 2];";
difference = parse(expected).checkTreeEquals(actual);
assertNull("Nodes do not match:\n" + difference, difference);
}

public void testRemoveForChild() {
// Test removing the initializer.
Node actual = parse("for(var a=0;a<0;a++)foo()");
Expand Down Expand Up @@ -2052,6 +2087,25 @@ public void testIsNotLValue() {
assertNotLValueNamedX(x);
}

public void testIsNestedObjectPattern() {
Node root = parse("var {a, b} = {a:1, b:2}");
Node destructuring = root.getFirstFirstChild();
Node objPattern = destructuring.getFirstChild();
assertFalse(NodeUtil.isNestedObjectPattern(objPattern));

root = parse("var {a, b:{c}} = {a:{}, b:{c:5}};");
destructuring = root.getFirstFirstChild();
objPattern = destructuring.getFirstChild();
assertTrue(NodeUtil.isNestedObjectPattern(objPattern));
}

public void testIsNestedArrayPattern() {
Node root = parse("var [a, b] = [1, 2]");
Node destructuring = root.getFirstFirstChild();
Node arrayPattern = destructuring.getFirstChild();
assertFalse(NodeUtil.isNestedArrayPattern(arrayPattern));
}

public void testLhsByDestructuring1() {
Node root = parse("var [a, b] = obj;");
Node destructLhs = root.getFirstFirstChild();
Expand Down Expand Up @@ -2641,6 +2695,53 @@ public void testIsVarArgs() {
assertFalse(NodeUtil.isVarArgsFunction(getNode("() => arguments")));
}

public void testDestructuringMap1() {
String code = "var {a, b, c} = {a: 1, b: 2, c: 3};";
Node ast = parse(code);
Node varNode = ast.getFirstChild();
Node destructuringNode = varNode.getFirstChild();
Map<String, Node> destructuringMap = NodeUtil.getDestructuringMap(destructuringNode);
assertEquals(3, destructuringMap.size());
}

public void testDestructuringMap2() {
String code = "var {a} = {a:1, b:2};";
Node ast = parse(code);
Node varNode = ast.getFirstChild();
Node destructuringNode = varNode.getFirstChild();
Map<String, Node> destructuringMap = NodeUtil.getDestructuringMap(destructuringNode);
assertEquals(2, destructuringMap.size());
}

public void testDestructuringMap3() {
String code = "var [a, b] = [1, 2];";
Node ast = parse(code);
Node varNode = ast.getFirstChild();
Node destructuringNode = varNode.getFirstChild();
Map<String, Node> destructuringMap = NodeUtil.getDestructuringMap(destructuringNode);
assertEquals(2, destructuringMap.size());
}

public void testDestructuringMap4() {
// Destructuring pattern and object literal are in different orders
String code = "var {a, b} = { b:2, a:1 }";
Node ast = parse(code);
Node varNode = ast.getFirstChild();
Node destructuringNode = varNode.getFirstChild();
Map<String, Node> destructuringMap = NodeUtil.getDestructuringMap(destructuringNode);
assertEquals(1, (int) destructuringMap.get("a").getDouble());
}

public void testDestructuringMap5() {
String code = "var {a, b} = foo();";
Node ast = parse(code);
Node varNode = ast.getFirstChild();
Node destructuringNode = varNode.getFirstChild();
Map<String, Node> destructuringMap = NodeUtil.getDestructuringMap(destructuringNode);
assertTrue(destructuringMap.get("a").isCall());
assertTrue(destructuringMap.get("b").isCall());
}

private boolean executedOnceTestCase(String code) {
Node ast = parse(code);
Node nameNode = getNameNode(ast, "x");
Expand Down
41 changes: 39 additions & 2 deletions test/com/google/javascript/jscomp/RemoveUnusedVarsTest.java
View file
Expand Up @@ -1108,7 +1108,7 @@ public void testTemplateStrings() {
);
}

public void testDestructuring() {
public void testDestructuringArrayPattern() {
testSame(
LINE_JOINER.join(
"var a; var b",
Expand All @@ -1123,8 +1123,45 @@ public void testDestructuring() {
"[a] = [1]"
));

testSame("var [a, b] = [1, 2]");
testSame("var [a, b] = [1, 2]; f(a); f(b);");
}

public void testDestructuringObjectPattern() {
/*testSame(LINE_JOINER.join("var a; var b;", "({a, b} = {a:1, b:2})"));
test(
LINE_JOINER.join("var a; var b;", "({a} = {a:1})"),
LINE_JOINER.join("var a; ", "({a} = {a:1})"));
testSame("var {a, b} = {a:1, b:2}; f(a); f(b);");
testSame("var {a} = {p:{}, q:{}}; a.q = 4;");*/

// Nested Destructuring
testSame("const someObject = {a:{ b:1 }}; var {a: {b}} = someObject; someObject.a.b;");
}

public void testRemoveUnusedVarsDeclaredInDestructuring() {
// Array destructuring
test("var [a, b] = [1, 2]; f(a);", "var [a, ,] = [1, 2]; f(a);");

test("var [a, b] = [1, 2];", "var [,,] = [1, 2];");

// Object pattern destructuring
test("var {a, b} = {a:1, b:2}; f(a);", "var {a, } = {a:1, b:2}; f(a);");

test("var {a, b} = {a:1, b:2};", "var { } = {a:1, b:2};");

// Nested pattern - pass backs off for now
// TODO (simranarora) Implement support for nested destructures
testSame("var {a, b:{c}} = {a:1, b:{c:5}}; f(a);");

// Value may have side effects
test("var {a, b} = {a:foo(), b:bar()};", "var { } = {a:foo(), b:bar()};");

test("var {a, b} = foo();", "var {} = foo();");

// Same as above case without the destructuring declaration
test("var a, b = foo();", "foo();");
}
}

0 comments on commit 37cd66f

Please sign in to comment.