Skip to content

Commit

Permalink
RemoveUnusedCode: Remove unused references like this.prop; and *.prot…
Browse files Browse the repository at this point in the history 
…otype.prop;

This feature is duplicated from RemoveUnusedClassProperties.
The *.prototype.prop part is enabled by removeUnusedPrototypeProperties.
The this.prop part is enabled only in tests for now.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=179954526
  • Loading branch information
@brad4d @blickly
brad4d authored and blickly committed Jan 2, 2018
1 parent 381fa5e commit 7928b17
Show file tree
Hide file tree
Showing 4 changed files with 127 additions and 40 deletions.
131 changes: 97 additions & 34 deletions src/com/google/javascript/jscomp/RemoveUnusedCode.java
View file
Expand Up @@ -392,11 +392,7 @@ private void traverseNode(Node n, Scope scope) {
break; break;


case GETPROP: case GETPROP:
Node objectNode = n.getFirstChild(); traverseGetProp(n, scope);
Node propertyNameNode = objectNode.getNext();
String propertyName = propertyNameNode.getString();
markPropertyNameReferenced(propertyName);
traverseNode(objectNode, scope);
break; break;


default: default:
Expand All @@ -405,6 +401,43 @@ private void traverseNode(Node n, Scope scope) {
} }
} }


private void traverseGetProp(Node getProp, Scope scope) {
Node objectNode = getProp.getFirstChild();
Node propertyNameNode = objectNode.getNext();
String propertyName = propertyNameNode.getString();

if (NodeUtil.isExpressionResultUsed(getProp)) {
// must record as reference to the property and continue traversal.
markPropertyNameReferenced(propertyName);
traverseNode(objectNode, scope);
} else if (objectNode.isThis()) {
// this.propName;
RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true);
considerForIndependentRemoval(builder.buildUnusedReadReference(getProp, propertyNameNode));
} else if (isDotPrototype(objectNode)) {
// (objExpression).prototype.propName;
RemovableBuilder builder = new RemovableBuilder().setIsPrototypeDotPropertyReference(true);
Node objExpression = objectNode.getFirstChild();
if (objExpression.isName()) {
// name.prototype.propName;
VarInfo varInfo = traverseNameNode(objExpression, scope);
varInfo.addRemovable(builder.buildUnusedReadReference(getProp, propertyNameNode));
} else {
// (objExpression).prototype.propName;
if (NodeUtil.mayHaveSideEffects(objExpression)) {
traverseNode(objExpression, scope);
} else {
builder.addContinuation(new Continuation(objExpression, scope));
}
considerForIndependentRemoval(builder.buildUnusedReadReference(getProp, propertyNameNode));
}
} else {
// TODO(bradfordcsmith): add removal of `varName.propName;`
markPropertyNameReferenced(propertyName);
traverseNode(objectNode, scope);
}
}

private void traverseIncrementOrDecrementOp(Node incOrDecOp, Scope scope) { private void traverseIncrementOrDecrementOp(Node incOrDecOp, Scope scope) {
checkArgument(incOrDecOp.isInc() || incOrDecOp.isDec(), incOrDecOp); checkArgument(incOrDecOp.isInc() || incOrDecOp.isDec(), incOrDecOp);
Node arg = incOrDecOp.getOnlyChild(); Node arg = incOrDecOp.getOnlyChild();
Expand All @@ -416,13 +449,12 @@ private void traverseIncrementOrDecrementOp(Node incOrDecOp, Scope scope) {
Node propertyNameNode = arg.getLastChild(); Node propertyNameNode = arg.getLastChild();
if (getPropObj.isThis()) { if (getPropObj.isThis()) {
// this.propName++ // this.propName++
RemovableBuilder builder = new RemovableBuilder().setIsThisNamedPropertyAssignment(true); RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true);
considerForIndependentRemoval(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode)); considerForIndependentRemoval(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode));
} else if (isDotPrototype(getPropObj)) { } else if (isDotPrototype(getPropObj)) {
// someExpression.prototype.propName++ // someExpression.prototype.propName++
Node exprObj = getPropObj.getFirstChild(); Node exprObj = getPropObj.getFirstChild();
RemovableBuilder builder = RemovableBuilder builder = new RemovableBuilder().setIsPrototypeDotPropertyReference(true);
new RemovableBuilder().setIsPrototypeObjectPropertyAssignment(true);
if (exprObj.isName()) { if (exprObj.isName()) {
// varName.prototype.propName++ // varName.prototype.propName++
VarInfo varInfo = traverseNameNode(exprObj, scope); VarInfo varInfo = traverseNameNode(exprObj, scope);
Expand Down Expand Up @@ -455,7 +487,7 @@ private void traverseCompoundAssign(Node compoundAssignNode, Scope scope) {
if (targetNode.isGetProp() if (targetNode.isGetProp()
&& targetNode.getFirstChild().isThis() && targetNode.getFirstChild().isThis()
&& !NodeUtil.isExpressionResultUsed(compoundAssignNode)) { && !NodeUtil.isExpressionResultUsed(compoundAssignNode)) {
RemovableBuilder builder = new RemovableBuilder().setIsThisNamedPropertyAssignment(true); RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true);
traverseRemovableAssignValue(valueNode, builder, scope); traverseRemovableAssignValue(valueNode, builder, scope);
considerForIndependentRemoval( considerForIndependentRemoval(
builder.buildNamedPropertyAssign(compoundAssignNode, targetNode.getLastChild())); builder.buildNamedPropertyAssign(compoundAssignNode, targetNode.getLastChild()));
Expand Down Expand Up @@ -767,8 +799,7 @@ private void traverseAssign(Node assignNode, Scope scope) {
} else if (isDotPrototype(getPropLhs)) { } else if (isDotPrototype(getPropLhs)) {
// objExpression.prototype.propertyName = someValue // objExpression.prototype.propertyName = someValue
Node objExpression = getPropLhs.getFirstChild(); Node objExpression = getPropLhs.getFirstChild();
RemovableBuilder builder = RemovableBuilder builder = new RemovableBuilder().setIsPrototypeDotPropertyReference(true);
new RemovableBuilder().setIsPrototypeObjectPropertyAssignment(true);
traverseRemovableAssignValue(valueNode, builder, scope); traverseRemovableAssignValue(valueNode, builder, scope);
if (objExpression.isName()) { if (objExpression.isName()) {
// varName.prototype.propertyName = someValue // varName.prototype.propertyName = someValue
Expand All @@ -787,7 +818,7 @@ private void traverseAssign(Node assignNode, Scope scope) {
} }
} else if (getPropLhs.isThis()) { } else if (getPropLhs.isThis()) {
// this.propertyName = someValue // this.propertyName = someValue
RemovableBuilder builder = new RemovableBuilder().setIsThisNamedPropertyAssignment(true); RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true);
traverseRemovableAssignValue(valueNode, builder, scope); traverseRemovableAssignValue(valueNode, builder, scope);
considerForIndependentRemoval(builder.buildNamedPropertyAssign(assignNode, propNameNode)); considerForIndependentRemoval(builder.buildNamedPropertyAssign(assignNode, propNameNode));
} else { } else {
Expand Down Expand Up @@ -1124,7 +1155,7 @@ private void considerForIndependentRemoval(Removable removable) {
} else if (removeUnusedPrototypeProperties && removable.isPrototypeProperty()) { } else if (removeUnusedPrototypeProperties && removable.isPrototypeProperty()) {
// Store for possible removal later. // Store for possible removal later.
removablesForPropertyNames.put(propertyName, removable); removablesForPropertyNames.put(propertyName, removable);
} else if (removeUnusedThisProperties && removable.isThisNamedPropertyAssignment()) { } else if (removeUnusedThisProperties && removable.isThisDotPropertyReference()) {
// Store for possible removal later. // Store for possible removal later.
removablesForPropertyNames.put(propertyName, removable); removablesForPropertyNames.put(propertyName, removable);
} else { } else {
Expand Down Expand Up @@ -1354,8 +1385,8 @@ private abstract class Removable {
private final List<Continuation> continuations; private final List<Continuation> continuations;
@Nullable private final String propertyName; @Nullable private final String propertyName;
@Nullable private final Node assignedValue; @Nullable private final Node assignedValue;
private final boolean isPrototypeObjectPropertyAssignment; private final boolean isPrototypeDotPropertyReference;
private final boolean isThisNamedPropertyAssignment; private final boolean isThisDotPropertyReference;


private boolean continuationsAreApplied = false; private boolean continuationsAreApplied = false;
private boolean isRemoved = false; private boolean isRemoved = false;
Expand All @@ -1364,8 +1395,8 @@ private abstract class Removable {
continuations = builder.continuations; continuations = builder.continuations;
propertyName = builder.propertyName; propertyName = builder.propertyName;
assignedValue = builder.assignedValue; assignedValue = builder.assignedValue;
isPrototypeObjectPropertyAssignment = builder.isPrototypeObjectPropertyAssignment; isPrototypeDotPropertyReference = builder.isPrototypeDotPropertyReference;
isThisNamedPropertyAssignment = builder.isThisNamedPropertyAssignment; isThisDotPropertyReference = builder.isThisDotPropertyReference;
} }


String getPropertyName() { String getPropertyName() {
Expand Down Expand Up @@ -1441,20 +1472,20 @@ boolean isPrototypeAssignment() {
} }


/** Is this an assignment to a property on a prototype object? */ /** Is this an assignment to a property on a prototype object? */
boolean isPrototypeObjectNamedPropertyAssignment() { boolean isPrototypeDotPropertyReference() {
return isPrototypeObjectPropertyAssignment && isNamedPropertyAssignment(); return isPrototypeDotPropertyReference;
} }


boolean isClassOrPrototypeNamedProperty() { boolean isClassOrPrototypeNamedProperty() {
return false; return false;
} }


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


boolean isThisNamedPropertyAssignment() { boolean isThisDotPropertyReference() {
return isThisNamedPropertyAssignment; return isThisDotPropertyReference;
} }
} }


Expand All @@ -1463,8 +1494,8 @@ private class RemovableBuilder {


@Nullable String propertyName = null; @Nullable String propertyName = null;
@Nullable public Node assignedValue = null; @Nullable public Node assignedValue = null;
boolean isPrototypeObjectPropertyAssignment = false; boolean isPrototypeDotPropertyReference = false;
boolean isThisNamedPropertyAssignment = false; boolean isThisDotPropertyReference = false;


RemovableBuilder addContinuation(Continuation continuation) { RemovableBuilder addContinuation(Continuation continuation) {
continuations.add(continuation); continuations.add(continuation);
Expand All @@ -1476,14 +1507,13 @@ RemovableBuilder setAssignedValue(@Nullable Node assignedValue) {
return this; return this;
} }


RemovableBuilder setIsPrototypeObjectPropertyAssignment( RemovableBuilder setIsPrototypeDotPropertyReference(boolean value) {
boolean isPrototypeObjectPropertyAssignment) { this.isPrototypeDotPropertyReference = value;
this.isPrototypeObjectPropertyAssignment = isPrototypeObjectPropertyAssignment;
return this; return this;
} }


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


Expand Down Expand Up @@ -1550,6 +1580,43 @@ IncOrDecOp buildIncOrDepOp(Node incOrDecOp, Node propertyNode) {
this.propertyName = propertyNode.getString(); this.propertyName = propertyNode.getString();
return new IncOrDecOp(this, incOrDecOp); return new IncOrDecOp(this, incOrDecOp);
} }

UnusedReadReference buildUnusedReadReference(Node referenceNode, Node propertyNode) {
this.propertyName = propertyNode.getString();
return new UnusedReadReference(this, referenceNode);
}
}

/** Represents a read reference whose value is not used. */
private class UnusedReadReference extends Removable {
final Node referenceNode;

UnusedReadReference(RemovableBuilder builder, Node referenceNode) {
super(builder);
// TODO(bradfordcsmith): handle `name;` and `name.property;` references
checkState(
isThisDotProperty(referenceNode) || isDotPrototypeDotProperty(referenceNode),
referenceNode);
this.referenceNode = referenceNode;
}

@Override
void removeInternal(AbstractCompiler compiler) {
if (!alreadyRemoved(referenceNode)) {
if (isThisDotProperty(referenceNode)) {
removeExpressionCompletely(referenceNode);
} else {
checkState(isDotPrototypeDotProperty(referenceNode), referenceNode);
// objExpression.prototype.propertyName
Node objExpression = referenceNode.getFirstFirstChild();
if (NodeUtil.mayHaveSideEffects(objExpression)) {
replaceExpressionWith(referenceNode, objExpression.detach());
} else {
removeExpressionCompletely(referenceNode);
}
}
}
}
} }


/** Represents an increment or decrement operation that could be removed. */ /** Represents an increment or decrement operation that could be removed. */
Expand All @@ -1560,6 +1627,7 @@ private class IncOrDecOp extends Removable {
super(builder); super(builder);
checkArgument(incOrDecNode.isInc() || incOrDecNode.isDec(), incOrDecNode); checkArgument(incOrDecNode.isInc() || incOrDecNode.isDec(), incOrDecNode);
Node arg = incOrDecNode.getOnlyChild(); Node arg = incOrDecNode.getOnlyChild();
// TODO(bradfordcsmith): handle `name;` and `name.property;` references
checkState(isThisDotProperty(arg) || isDotPrototypeDotProperty(arg), arg); checkState(isThisDotProperty(arg) || isDotPrototypeDotProperty(arg), arg);
this.incOrDecNode = incOrDecNode; this.incOrDecNode = incOrDecNode;
} }
Expand All @@ -1584,11 +1652,6 @@ void removeInternal(AbstractCompiler compiler) {
} }
} }
} }

@Override
boolean isNamedPropertyAssignment() {
return isNamedProperty();
}
} }


/** True for `this.propertyName` */ /** True for `this.propertyName` */
Expand Down
9 changes: 4 additions & 5 deletions test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java
View file
Expand Up @@ -60,6 +60,7 @@ public final class RemoveUnusedCodeClassPropertiesTest extends TypeICompilerTest
"var EXT = {};", "var EXT = {};",
"EXT.ext;", "EXT.ext;",
"var externVar;", "var externVar;",
"function externFunction() {}",
"/** @type {Function} */", "/** @type {Function} */",
"Object.defineProperties = function() {};", "Object.defineProperties = function() {};",
"/** @type {Function} */", "/** @type {Function} */",
Expand Down Expand Up @@ -184,11 +185,9 @@ public void testInc2() {
testSame("let x = (alert(), --this.a)"); testSame("let x = (alert(), --this.a)");
} }


// TODO(b/66971163): make this pass public void testExprResult() {
public void disabledTestExprResult() { test("this.x", "");
test("this.x", "0"); test("externFunction().prototype.x", "externFunction()");
test("c.prototype.x", "0");
test("SomeSideEffect().prototype.x", "SomeSideEffect(),0");
} }


public void testJSCompiler_renameProperty() { public void testJSCompiler_renameProperty() {
Expand Down
18 changes: 18 additions & 0 deletions test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java
View file
Expand Up @@ -30,6 +30,7 @@ public final class RemoveUnusedCodePrototypePropertiesTest extends CompilerTestC
"var window;", "var window;",
"var Math = {};", "var Math = {};",
"Math.random = function() {};", "Math.random = function() {};",
"function alert(x) {}",
"function externFunction() {}", "function externFunction() {}",
"externFunction.prototype.externPropName;", "externFunction.prototype.externPropName;",
"var mExtern;", "var mExtern;",
Expand Down Expand Up @@ -76,6 +77,23 @@ protected void setUp() throws Exception {
allowRemovalOfExternProperties = false; allowRemovalOfExternProperties = false;
} }


public void testUnusedPrototypeFieldReference() {
test(
"function C() {} C.prototype.x; new C();", // x is not actually read
"function C() {} new C();");
}

public void testUnusedReferenceToFieldWithGetter() {
// Reference to a field with a getter should not be removed unless we know it has no side
// effects.
// TODO(bradfordcsmith): Implement removal for the no-side-effect cases.
testSame("function C() {} C.prototype = { get x() {} }; new C().x");
testSame("function C() {} C.prototype = { get x() { alert('x'); } }; new C().x");
testSame("class C { get x() {} } new C().x;");
testSame("class C { get x() { alert('x'); } } new C().x");
testSame("let c = { get x() {} }; c.x;");
testSame("let c = { get x() { alert('x'); } }; c.x;");
}


public void testAnonymousPrototypePropertyRemoved() { public void testAnonymousPrototypePropertyRemoved() {
test("({}.prototype.x = 5, externFunction())", "0, externFunction();"); test("({}.prototype.x = 5, externFunction())", "0, externFunction();");
Expand Down
9 changes: 8 additions & 1 deletion test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java
View file
Expand Up @@ -69,6 +69,11 @@ public void process(Node externs, Node root) {
}; };
} }


public void testUnusedPrototypeFieldReference() {
// Simply mentioning a prototype property without using it doesn't count as a reference.
test("function C() {} C.prototype.x;", "");
}

public void testLeaveZeroBehind() { public void testLeaveZeroBehind() {
// We don't need the assignment or the assigned value, but we need to keep the AST valid. // We don't need the assignment or the assigned value, but we need to keep the AST valid.
test( test(
Expand Down Expand Up @@ -139,7 +144,9 @@ public void testUsageBeforeDefinition() {
} }


public void testReferencedPropertiesOnUnreferencedVar() { public void testReferencedPropertiesOnUnreferencedVar() {
test("var x = {}; x.a = 1; var y = {a: 2}; y.a;", "var y = {a: 2}; y.a;"); test(
"var x = {}; x.a = 1; var y = {a: 2}; y.a;", // preserve format
" var y = {a: 2}; y.a;");
} }


public void testPropertyValuesAddedAfterReferenceAreRemoved() { public void testPropertyValuesAddedAfterReferenceAreRemoved() {
Expand Down

0 comments on commit 7928b17

Please sign in to comment.