Skip to content

Commit

Permalink
RemoveUnusedCode: implement removal of this.propName assignments
Browse files Browse the repository at this point in the history 
This feature is not yet enabled outside of tests.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=179698768
  • Loading branch information
@brad4d
brad4d authored and Tyler Breisacher committed Dec 21, 2017
1 parent 4811545 commit d454bec
Show file tree
Hide file tree
Showing 4 changed files with 118 additions and 92 deletions.
168 changes: 96 additions & 72 deletions src/com/google/javascript/jscomp/RemoveUnusedCode.java
View file
Expand Up @@ -212,7 +212,7 @@ RemoveUnusedCode build() {
@Override
public void process(Node externs, Node root) {
checkState(compiler.getLifeCycleStage().isNormalized());
if (removeUnusedPrototypeProperties && !allowRemovalOfExternProperties) {
if (!allowRemovalOfExternProperties) {
referencedPropertyNames.addAll(compiler.getExternProperties());
}
traverseAndRemoveUnusedReferences(root);
Expand All @@ -238,15 +238,13 @@ private void traverseAndRemoveUnusedReferences(Node root) {
}

removeUnreferencedVars();
if (removeUnusedPrototypeProperties) {
removeUnreferencedProperties();
}
removeIndependentlyRemovableProperties();
for (Scope fparamScope : allFunctionParamScopes) {
removeUnreferencedFunctionArgs(fparamScope);
}
}

private void removeUnreferencedProperties() {
private void removeIndependentlyRemovableProperties() {
for (Removable removable : removablesForPropertyNames.values()) {
removable.remove(compiler);
}
Expand Down Expand Up @@ -642,78 +640,88 @@ private void traverseAssign(Node assignNode, Scope scope) {
checkState(NodeUtil.isAssignmentOp(assignNode));

Node lhs = assignNode.getFirstChild();
Node nameNode = null;
Node propertyNode = null;
boolean isVariableAssign = false;
boolean isComputedPropertyAssign = false;
boolean isNamedPropertyAssign = false;
boolean isPrototypeObjectPropertyAssignment = false;

Node valueNode = assignNode.getLastChild();
if (lhs.isName()) {
isVariableAssign = true;
nameNode = lhs;
} else if (NodeUtil.isGet(lhs)) {
propertyNode = lhs.getLastChild();
Node possibleNameNode = lhs.getFirstChild();
// Handle assignments to properties of a variable or its prototype property.
// However, don't handle any longer qualified names, because it gets hard to track
// properties of properties.
if (possibleNameNode.isGetProp()
&& possibleNameNode.getSecondChild().getString().equals("prototype")) {
isPrototypeObjectPropertyAssignment = true;
possibleNameNode = possibleNameNode.getFirstChild();
}
if (possibleNameNode.isName()) {
nameNode = possibleNameNode;
if (lhs.isGetProp()) {
isNamedPropertyAssign = true;
// varName = something
VarInfo varInfo = traverseNameNode(lhs, scope);
RemovableBuilder builder = new RemovableBuilder();
traverseRemovableAssignValue(valueNode, builder, scope);
varInfo.addRemovable(builder.buildVariableAssign(assignNode));
} else if (lhs.isGetElem()) {
Node getElemObj = lhs.getFirstChild();
Node getElemKey = lhs.getLastChild();
Node varNameNode =
getElemObj.isName()
? getElemObj
: isNameDotPrototype(getElemObj) ? getElemObj.getFirstChild() : null;

if (varNameNode != null) {
// varName[someExpression] = someValue
// OR
// varName.prototype[someExpression] = someValue
VarInfo varInfo = traverseNameNode(varNameNode, scope);
RemovableBuilder builder = new RemovableBuilder();
if (NodeUtil.mayHaveSideEffects(getElemKey)) {
traverseNode(getElemKey, scope);
} else {
checkState(lhs.isGetElem());
isComputedPropertyAssign = true;
builder.addContinuation(new Continuation(getElemKey, scope));
}
}
}
// else LHS is something else, like a destructuring pattern, which will be handled by
// traverseChildren() below
// TODO(bradfordcsmith): Handle destructuring at this level for better clarity and so we can
// do a better job with removal.

// If we successfully identified a name node & there is a corresponding Var,
// then we have a removable assignment.
if (nameNode == null) {
// Not an assignment we need to track
traverseChildren(assignNode, scope);
} else {
VarInfo varInfo = traverseNameNode(nameNode, scope);

Node valueNode = assignNode.getLastChild();
RemovableBuilder builder =
new RemovableBuilder()
.setAssignedValue(valueNode)
.setIsPrototypeObjectPropertyAssignment(isPrototypeObjectPropertyAssignment);
if (NodeUtil.isExpressionResultUsed(assignNode) || NodeUtil.mayHaveSideEffects(valueNode)) {
traverseNode(valueNode, scope);
traverseRemovableAssignValue(valueNode, builder, scope);
varInfo.addRemovable(builder.buildComputedPropertyAssign(assignNode, getElemKey));
} else {
builder.addContinuation(new Continuation(valueNode, scope));
traverseNode(getElemObj, scope);
traverseNode(getElemKey, scope);
traverseNode(valueNode, scope);
}
} else if (lhs.isGetProp()) {
Node getPropLhs = lhs.getFirstChild();
Node propNameNode = lhs.getLastChild();

if (isNamedPropertyAssign) {
varInfo.addRemovable(builder.buildNamedPropertyAssign(assignNode, propertyNode));
} else if (isVariableAssign) {
varInfo.addRemovable(builder.buildVariableAssign(assignNode));
if (getPropLhs.isName()) {
// varName.propertyName = someValue
VarInfo varInfo = traverseNameNode(getPropLhs, scope);
RemovableBuilder builder = new RemovableBuilder();
traverseRemovableAssignValue(valueNode, builder, scope);
varInfo.addRemovable(builder.buildNamedPropertyAssign(assignNode, propNameNode));
} else if (isNameDotPrototype(getPropLhs)) {
// varName.prototype.propertyName = someValue
VarInfo varInfo = traverseNameNode(getPropLhs.getFirstChild(), scope);
RemovableBuilder builder =
new RemovableBuilder().setIsPrototypeObjectPropertyAssignment(true);
traverseRemovableAssignValue(valueNode, builder, scope);
varInfo.addRemovable(builder.buildNamedPropertyAssign(assignNode, propNameNode));
} else if (getPropLhs.isThis()) {
// this.propertyName = someValue
RemovableBuilder builder = new RemovableBuilder().setIsThisNamedPropertyAssignment(true);
traverseRemovableAssignValue(valueNode, builder, scope);
considerForIndependentRemoval(builder.buildNamedPropertyAssign(assignNode, propNameNode));
} else {
checkState(isComputedPropertyAssign);
if (NodeUtil.mayHaveSideEffects(propertyNode)) {
traverseNode(propertyNode, scope);
} else {
builder.addContinuation(new Continuation(propertyNode, scope));
}
varInfo.addRemovable(
builder.buildComputedPropertyAssign(assignNode, propertyNode));
traverseNode(lhs, scope);
traverseNode(valueNode, scope);
}
} else {
// no other cases are removable
traverseNode(lhs, scope);
traverseNode(valueNode, scope);
}
}

private void traverseRemovableAssignValue(Node valueNode, RemovableBuilder builder, Scope scope) {
builder.setAssignedValue(valueNode);
if (NodeUtil.mayHaveSideEffects(valueNode)
|| NodeUtil.isExpressionResultUsed(valueNode.getParent())) {
traverseNode(valueNode, scope);
} else {
builder.addContinuation(new Continuation(valueNode, scope));
}
}

private boolean isNameDotPrototype(Node n) {
return n.isGetProp()
&& n.getFirstChild().isName()
&& n.getLastChild().getString().equals("prototype");
}

private void traverseDefaultValue(Node defaultValueNode, Scope scope) {
Var var;
Node target = defaultValueNode.getFirstChild();
Expand Down Expand Up @@ -1011,19 +1019,23 @@ private void markPropertyNameReferenced(String propertyName) {
}

private void considerForIndependentRemoval(Removable removable) {
if (removeUnusedPrototypeProperties && removable.isNamedProperty()) {
if (removable.isNamedProperty()) {
String propertyName = removable.getPropertyName();

if (referencedPropertyNames.contains(propertyName)
|| codingConvention.isExported(propertyName)) {
// Referenced, so not removable.
removable.applyContinuations();
} else if (removable.isIndependentlyRemovableNamedProperty()) {
} else if (removeUnusedPrototypeProperties && removable.isPrototypeProperty()) {
// Store for possible removal later.
removablesForPropertyNames.put(removable.getPropertyName(), removable);
removablesForPropertyNames.put(propertyName, removable);
} else if (removeUnusedThisProperties && removable.isThisNamedPropertyAssignment()) {
// Store for possible removal later.
removablesForPropertyNames.put(propertyName, removable);
} else {
// TODO(bradfordcsmith): Maybe allow removal of non-prototype property assignments if we
// can be sure the variable's value is defined as a literal value that does not escape.
// TODO(bradfordcsmith): Maybe allow removal of `varName.propertyName = something`
// assignments if we can be sure the variable's value is defined as a literal value that
// does not escape.
removable.applyContinuations();
// This assignment counts as a reference, since we won't be removing it.
// This is necessary in order to preserve getters and setters for the property.
Expand Down Expand Up @@ -1248,6 +1260,7 @@ private abstract class Removable {
@Nullable private final String propertyName;
@Nullable private final Node assignedValue;
private final boolean isPrototypeObjectPropertyAssignment;
private final boolean isThisNamedPropertyAssignment;

private boolean continuationsAreApplied = false;
private boolean isRemoved = false;
Expand All @@ -1257,6 +1270,7 @@ private abstract class Removable {
propertyName = builder.propertyName;
assignedValue = builder.assignedValue;
isPrototypeObjectPropertyAssignment = builder.isPrototypeObjectPropertyAssignment;
isThisNamedPropertyAssignment = builder.isThisNamedPropertyAssignment;
}

String getPropertyName() {
Expand Down Expand Up @@ -1340,9 +1354,13 @@ boolean isClassOrPrototypeNamedProperty() {
return false;
}

boolean isIndependentlyRemovableNamedProperty() {
boolean isPrototypeProperty() {
return isPrototypeObjectNamedPropertyAssignment() || isClassOrPrototypeNamedProperty();
}

boolean isThisNamedPropertyAssignment() {
return isThisNamedPropertyAssignment;
}
}

private class RemovableBuilder {
Expand All @@ -1351,6 +1369,7 @@ private class RemovableBuilder {
@Nullable String propertyName = null;
@Nullable public Node assignedValue = null;
boolean isPrototypeObjectPropertyAssignment = false;
boolean isThisNamedPropertyAssignment = false;

RemovableBuilder addContinuation(Continuation continuation) {
continuations.add(continuation);
Expand All @@ -1368,6 +1387,11 @@ RemovableBuilder setIsPrototypeObjectPropertyAssignment(
return this;
}

RemovableBuilder setIsThisNamedPropertyAssignment(boolean value) {
this.isThisNamedPropertyAssignment = value;
return this;
}

DestructuringAssign buildDestructuringAssign(Node removableNode, Node nameNode) {
return new DestructuringAssign(this, removableNode, nameNode);
}
Expand Down
1 change: 1 addition & 0 deletions test/com/google/javascript/jscomp/OptimizeCallsTest.java
View file
Expand Up @@ -42,6 +42,7 @@ public OptimizeCallsTest() {
protected void setUp() throws Exception {
super.setUp();
enableNormalize();
enableGatherExternProperties();
}

@Override
Expand Down
36 changes: 16 additions & 20 deletions test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java
View file
Expand Up @@ -97,21 +97,19 @@ protected void setUp() throws Exception {
this.mode = TypeInferenceMode.NEITHER;
}

// TODO(b/66971163): make this pass
public void disabledTestSimple1() {
public void testSimple1() {
// A property defined on "this" can be removed
test("this.a = 2", "2");
test("x = (this.a = 2)", "x = 2");
testSame("this.a = 2; x = this.a;");
test("this.a = 2", "");
test("let x = (this.a = 2)", "let x = 2");
testSame("this.a = 2; let x = this.a;");
}

// TODO(b/66971163): make this pass
public void disabledTestSimple2() {
public void testSimple2() {
// A property defined on "this" can be removed, even when defined
// as part of an expression
test("this.a = 2, f()", "2, f()");
test("x = (this.a = 2, f())", "x = (2, f())");
test("x = (f(), this.a = 2)", "x = (f(), 2)");
test("this.a = 2, alert(1);", "0, alert(1);");
test("const x = (this.a = 2, alert(1));", "const x = (0, alert(1));");
test("const x = (alert(1), this.a = 2);", "const x = (alert(1), 2);");
}

public void testSimple3() {
Expand Down Expand Up @@ -149,10 +147,10 @@ public void disabledTestAssignOp1() {
// Properties defined using a compound assignment can be removed if the
// result of the assignment expression is not immediately used.
test("this.x += 2", "2");
testSame("x = (this.x += 2)");
testSame("this.x += 2; x = this.x;");
testSame("const x = (this.x += 2)");
testSame("this.x += 2; const x = this.x;");
// But, of course, a later use prevents its removal.
testSame("this.x += 2; x.x;");
testSame("this.x += 2; let x = {}; x.x;");
}

// TODO(b/66971163): make this pass
Expand Down Expand Up @@ -213,20 +211,18 @@ public void testJSCompiler_renameProperty() {
testSame("this.a = 2; JSCompiler_renameProperty('a')");
}

// TODO(b/66971163): make this pass
public void disabledTestForIn() {
public void testForIn() {
// This is the basic assumption that this pass makes:
// it can remove properties even when the object is used in a FOR-IN loop
test("this.y = 1;for (var a in x) { alert(x[a]) }",
"1;for (var a in x) { alert(x[a]) }");
test("let x = {}; this.y = 1;for (var a in x) { alert(x[a]) }",
"let x = {}; for (var a in x) { alert(x[a]) }");
}

// TODO(b/66971163): make this pass
public void disabledTestObjectKeys() {
public void testObjectKeys() {
// This is the basic assumption that this pass makes:
// it can remove properties even when the object are referenced
test("this.y = 1;alert(Object.keys(this))",
"1;alert(Object.keys(this))");
" alert(Object.keys(this))");
}

public void testObjectReflection1() {
Expand Down
5 changes: 5 additions & 0 deletions test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java
View file
Expand Up @@ -48,6 +48,7 @@ protected int getNumRepetitions() {
protected void setUp() throws Exception {
super.setUp();
enableNormalize();
enableGatherExternProperties();
removeGlobal = true;
preserveFunctionExpressionNames = false;
}
Expand Down Expand Up @@ -844,6 +845,10 @@ public void testUnusedPropAssign6b() {
test("function x() {} x.prototype.bar = function() {};", "");
}

public void testUnusedPropAssign6c() {
test("function x() {} x.prototype['bar'] = function() {};", "");
}

public void testUnusedPropAssign7() {
test("var x = {}; x[x.foo] = x.bar;", "");
}
Expand Down

0 comments on commit d454bec

Please sign in to comment.