Skip to content

Commit

Permalink
Remove incr/decr of this.unused and *.prototype.unused
Browse files Browse the repository at this point in the history 
This re-implements a feature currently in RemoveUnusedClassProperties.
Removal of `this.unused++` is only enabled for tests for now.
Removal of `*.prototype.unused` is enabled by the
removeUnusedPrototypeProperties option.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=179857292
  • Loading branch information
@brad4d @blickly
brad4d authored and blickly committed Jan 2, 2018
1 parent 319ba7b commit 7f71c0b
Show file tree
Hide file tree
Showing 3 changed files with 173 additions and 70 deletions.
154 changes: 128 additions & 26 deletions src/com/google/javascript/jscomp/RemoveUnusedCode.java
View file
Expand Up @@ -307,6 +307,11 @@ private void traverseNode(Node n, Scope scope) {
traverseCompoundAssign(n, scope); traverseCompoundAssign(n, scope);
break; break;


case INC:
case DEC:
traverseIncrementOrDecrementOp(n, scope);
break;

case CALL: case CALL:
traverseCall(n, scope); traverseCall(n, scope);
break; break;
Expand Down Expand Up @@ -400,6 +405,47 @@ private void traverseNode(Node n, Scope scope) {
} }
} }


private void traverseIncrementOrDecrementOp(Node incOrDecOp, Scope scope) {
checkArgument(incOrDecOp.isInc() || incOrDecOp.isDec(), incOrDecOp);
Node arg = incOrDecOp.getOnlyChild();
if (NodeUtil.isExpressionResultUsed(incOrDecOp)) {
// If expression result is used, then this expression is definitely not removable.
traverseNode(arg, scope);
} else if (arg.isGetProp()) {
Node getPropObj = arg.getFirstChild();
Node propertyNameNode = arg.getLastChild();
if (getPropObj.isThis()) {
// this.propName++
RemovableBuilder builder = new RemovableBuilder().setIsThisNamedPropertyAssignment(true);
considerForIndependentRemoval(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode));
} else if (isDotPrototype(getPropObj)) {
// someExpression.prototype.propName++
Node exprObj = getPropObj.getFirstChild();
RemovableBuilder builder =
new RemovableBuilder().setIsPrototypeObjectPropertyAssignment(true);
if (exprObj.isName()) {
// varName.prototype.propName++
VarInfo varInfo = traverseNameNode(exprObj, scope);
varInfo.addRemovable(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode));
} else {
// (someExpression).prototype.propName++
if (NodeUtil.mayHaveSideEffects(exprObj)) {
traverseNode(exprObj, scope);
} else {
builder.addContinuation(new Continuation(exprObj, scope));
}
considerForIndependentRemoval(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode));
}
} else {
// someExpression.propName++ is not removable except in the cases covered above
traverseNode(arg, scope);
}
} else {
// TODO(bradfordcsmith): varName++ should be removable if varName is otherwise unused
traverseNode(arg, scope);
}
}

private void traverseCompoundAssign(Node compoundAssignNode, Scope scope) { private void traverseCompoundAssign(Node compoundAssignNode, Scope scope) {
// We'll allow removal of compound assignment to a this property as long as the result of the // We'll allow removal of compound assignment to a this property as long as the result of the
// assignment is unused. // assignment is unused.
Expand Down Expand Up @@ -539,10 +585,11 @@ private void traversePrototypeLiteral(Node objectLiteral, Scope scope) {
} }


private boolean isAssignmentToPrototype(Node n) { private boolean isAssignmentToPrototype(Node n) {
return n.isAssign() && isPrototypeGetProp(n.getFirstChild()); return n.isAssign() && isDotPrototype(n.getFirstChild());
} }


private boolean isPrototypeGetProp(Node n) { /** True for `someExpression.prototype`. */
private static boolean isDotPrototype(Node n) {
return n.isGetProp() && n.getLastChild().getString().equals("prototype"); return n.isGetProp() && n.getLastChild().getString().equals("prototype");
} }


Expand Down Expand Up @@ -717,8 +764,7 @@ private void traverseAssign(Node assignNode, Scope scope) {
RemovableBuilder builder = new RemovableBuilder(); RemovableBuilder builder = new RemovableBuilder();
traverseRemovableAssignValue(valueNode, builder, scope); traverseRemovableAssignValue(valueNode, builder, scope);
varInfo.addRemovable(builder.buildNamedPropertyAssign(assignNode, propNameNode)); varInfo.addRemovable(builder.buildNamedPropertyAssign(assignNode, propNameNode));
} else if (getPropLhs.isGetProp() } else if (isDotPrototype(getPropLhs)) {
&& getPropLhs.getLastChild().getString().equals("prototype")) {
// objExpression.prototype.propertyName = someValue // objExpression.prototype.propertyName = someValue
Node objExpression = getPropLhs.getFirstChild(); Node objExpression = getPropLhs.getFirstChild();
RemovableBuilder builder = RemovableBuilder builder =
Expand Down Expand Up @@ -1499,6 +1545,60 @@ AnonymousPrototypeNamedPropertyAssign buildAnonymousPrototypeNamedPropertyAssign
this.propertyName = propertyName; this.propertyName = propertyName;
return new AnonymousPrototypeNamedPropertyAssign(this, assignNode); return new AnonymousPrototypeNamedPropertyAssign(this, assignNode);
} }

IncOrDecOp buildIncOrDepOp(Node incOrDecOp, Node propertyNode) {
this.propertyName = propertyNode.getString();
return new IncOrDecOp(this, incOrDecOp);
}
}

/** Represents an increment or decrement operation that could be removed. */
private class IncOrDecOp extends Removable {
final Node incOrDecNode;

IncOrDecOp(RemovableBuilder builder, Node incOrDecNode) {
super(builder);
checkArgument(incOrDecNode.isInc() || incOrDecNode.isDec(), incOrDecNode);
Node arg = incOrDecNode.getOnlyChild();
checkState(isThisDotProperty(arg) || isDotPrototypeDotProperty(arg), arg);
this.incOrDecNode = incOrDecNode;
}

@Override
void removeInternal(AbstractCompiler compiler) {
if (!alreadyRemoved(incOrDecNode)) {
Node arg = incOrDecNode.getOnlyChild();
checkState(arg.isGetProp(), arg);

if (isThisDotProperty(arg)) {
removeExpressionCompletely(incOrDecNode);
} else {
checkState(isDotPrototypeDotProperty(arg), arg);
// objExpression.prototype.propertyName
Node objExpression = arg.getFirstFirstChild();
if (NodeUtil.mayHaveSideEffects(objExpression)) {
replaceExpressionWith(incOrDecNode, objExpression.detach());
} else {
removeExpressionCompletely(incOrDecNode);
}
}
}
}

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

/** True for `this.propertyName` */
private static boolean isThisDotProperty(Node n) {
return n.isGetProp() && n.getFirstChild().isThis();
}

/** True for `(something).prototype.propertyName` */
private static boolean isDotPrototypeDotProperty(Node n) {
return n.isGetProp() && isDotPrototype(n.getFirstChild());
} }


private class DestructuringAssign extends Removable { private class DestructuringAssign extends Removable {
Expand Down Expand Up @@ -1788,20 +1888,13 @@ public void removeInternal(AbstractCompiler compiler) {
if (mustPreserveRhs && mustPreserveGetElmExpr) { if (mustPreserveRhs && mustPreserveGetElmExpr) {
Node replacement = Node replacement =
IR.comma(lhs.getLastChild().detach(), rhs.detach()).useSourceInfoFrom(assignNode); IR.comma(lhs.getLastChild().detach(), rhs.detach()).useSourceInfoFrom(assignNode);
assignNode.replaceWith(replacement); replaceExpressionWith(assignNode, replacement);
} else if (mustPreserveGetElmExpr) { } else if (mustPreserveGetElmExpr) {
assignNode.replaceWith(lhs.getLastChild().detach()); replaceExpressionWith(assignNode, lhs.getLastChild().detach());
NodeUtil.markFunctionsDeleted(rhs, compiler);
} else if (mustPreserveRhs) { } else if (mustPreserveRhs) {
assignNode.replaceWith(rhs.detach()); replaceExpressionWith(assignNode, rhs.detach());
NodeUtil.markFunctionsDeleted(lhs, compiler);
} else if (parent.isExprResult()) {
parent.detach();
NodeUtil.markFunctionsDeleted(parent, compiler);
} else { } else {
// value isn't needed, but we need to keep the AST valid. removeExpressionCompletely(assignNode);
assignNode.replaceWith(IR.number(0).useSourceInfoFrom(assignNode));
NodeUtil.markFunctionsDeleted(assignNode, compiler);
} }
} }


Expand Down Expand Up @@ -1846,20 +1939,13 @@ void removeInternal(AbstractCompiler compiler) {
if (mustPreserveRhs && mustPreserveObjExpression) { if (mustPreserveRhs && mustPreserveObjExpression) {
Node replacement = Node replacement =
IR.comma(objExpression.detach(), rhs.detach()).useSourceInfoFrom(assignNode); IR.comma(objExpression.detach(), rhs.detach()).useSourceInfoFrom(assignNode);
assignNode.replaceWith(replacement); replaceExpressionWith(assignNode, replacement);
} else if (mustPreserveObjExpression) { } else if (mustPreserveObjExpression) {
assignNode.replaceWith(objExpression.detach()); replaceExpressionWith(assignNode, objExpression.detach());
NodeUtil.markFunctionsDeleted(rhs, compiler);
} else if (mustPreserveRhs) { } else if (mustPreserveRhs) {
assignNode.replaceWith(rhs.detach()); replaceExpressionWith(assignNode, rhs.detach());
NodeUtil.markFunctionsDeleted(objExpression, compiler);
} else if (parent.isExprResult()) {
parent.detach();
NodeUtil.markFunctionsDeleted(parent, compiler);
} else { } else {
// value isn't needed, but we need to keep the AST valid. removeExpressionCompletely(assignNode);
assignNode.replaceWith(IR.number(0).useSourceInfoFrom(assignNode));
NodeUtil.markFunctionsDeleted(assignNode, compiler);
} }
} }


Expand Down Expand Up @@ -2058,6 +2144,22 @@ void removeInternal(AbstractCompiler compiler) {
} }
NodeUtil.markFunctionsDeleted(nameNode, compiler); NodeUtil.markFunctionsDeleted(nameNode, compiler);
} }
}

void removeExpressionCompletely(Node expression) {
checkState(!NodeUtil.isExpressionResultUsed(expression), expression);
Node parent = expression.getParent();
if (parent.isExprResult()) {
NodeUtil.deleteNode(parent, compiler);
} else {
// value isn't needed, but we need to keep the AST valid.
replaceExpressionWith(expression, IR.number(0).useSourceInfoFrom(expression));
}
}


void replaceExpressionWith(Node expression, Node replacement) {
compiler.reportChangeToEnclosingScope(expression);
expression.replaceWith(replacement);
NodeUtil.markFunctionsDeleted(expression, compiler);
} }
} }
74 changes: 30 additions & 44 deletions test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java
View file
Expand Up @@ -160,35 +160,28 @@ public void testAssignOp2() {
testSame("const x = (alert(1), this.a += 2)"); testSame("const x = (alert(1), this.a += 2)");
} }


// TODO(b/66971163): make this pass public void testInc1() {
public void disabledTestInc1() {
// Increments and Decrements are handled similarly to compound assignments // Increments and Decrements are handled similarly to compound assignments
// but need a placeholder value when replaced. // but need a placeholder value when replaced.
test("this.x++", "0"); test("this.x++", "");
testSame("x = (this.x++)"); testSame("let x = (this.x++)");
testSame("this.x++; x = this.x;"); testSame("this.x++; let x = this.x;");


test("--this.x", "0"); test("--this.x", "");
testSame("x = (--this.x)"); testSame("let x = (--this.x)");
testSame("--this.x; x = this.x;"); testSame("--this.x; let x = this.x;");
} }


// TODO(b/66971163): make this pass public void testInc2() {
public void disabledTestInc2() {
// Increments and Decrements are handled similarly to compound assignments // Increments and Decrements are handled similarly to compound assignments
// but need a placeholder value when replaced. // but need a placeholder value when replaced.
test("this.a++, f()", "0, f()"); test("this.a++, alert()", "0, alert()");
test("x = (this.a++, f())", "x = (0, f())"); test("let x = (this.a++, alert())", "let x = (0, alert())");
testSame("x = (f(), this.a++)"); testSame("let x = (alert(), this.a++)");


test("--this.a, f()", "0, f()"); test("--this.a, alert()", "0, alert()");
test("x = (--this.a, f())", "x = (0, f())"); test("let x = (--this.a, alert())", "let x = (0, alert())");
testSame("x = (f(), --this.a)"); testSame("let x = (alert(), --this.a)");
}

// TODO(b/66971163): make this pass
public void disabledTestIncPrototype() {
test("SomeSideEffect().prototype.x++", "SomeSideEffect(), 0");
} }


// TODO(b/66971163): make this pass // TODO(b/66971163): make this pass
Expand Down Expand Up @@ -243,32 +236,25 @@ public void testIssue730() {
// Partial removal of properties can causes problems if the object is // Partial removal of properties can causes problems if the object is
// sealed. // sealed.
testSame( testSame(
"function A() {this.foo = 0;}\n" + lines(
"function B() {this.a = new A();}\n" + "function A() {this.foo = 0;}",
"B.prototype.dostuff = function() {this.a.foo++;alert('hi');}\n" + "function B() {this.a = new A();}",
"new B().dostuff();\n"); "B.prototype.dostuff = function() { this.a.foo++; alert('hi'); }",
} "new B().dostuff();"));

// TODO(b/66971163): make this pass
public void disabledTestNoRemoveSideEffect2() {
test(
"function A() {alert('me'); return function(){}}\n" +
"A().prototype.foo++;\n",
"function A() {alert('me'); return function(){}}\n" +
"A(),0;\n");
} }


// TODO(b/66971163): make this pass public void testPrototypeProps1() {
public void disabledTestPrototypeProps1() {
test( test(
"function A() {this.foo = 1;}\n" + lines(
"A.prototype.foo = 0;\n" + "function A() {this.foo = 1;}",
"A.prototype.method = function() {this.foo++};\n" + "A.prototype.foo = 0;",
"new A().method()\n", "A.prototype.method = function() {this.foo++};",
"function A() {1;}\n" + "new A().method()"),
"0;\n" + lines(
"A.prototype.method = function() {0;};\n" + "function A() { }",
"new A().method()\n"); " ",
"A.prototype.method = function() { };",
"new A().method()"));
} }


public void testPrototypeProps2() { public void testPrototypeProps2() {
Expand Down
15 changes: 15 additions & 0 deletions test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java
View file
Expand Up @@ -76,6 +76,7 @@ protected void setUp() throws Exception {
allowRemovalOfExternProperties = false; allowRemovalOfExternProperties = false;
} }



public void testAnonymousPrototypePropertyRemoved() { public void testAnonymousPrototypePropertyRemoved() {
test("({}.prototype.x = 5, externFunction())", "0, externFunction();"); test("({}.prototype.x = 5, externFunction())", "0, externFunction();");
test("({}).prototype.x = 5;", ""); test("({}).prototype.x = 5;", "");
Expand Down Expand Up @@ -108,6 +109,20 @@ public void testAnonymousPrototypePropertyNoRemoveSideEffect1() {
"A();")); "A();"));
} }


public void testAnonymousPrototypePropertyNoRemoveSideEffect2() {
test(
"function A() { externFunction('me'); return function(){}; } A().prototype.foo++;",
"function A() { externFunction('me'); return function(){}; } A();");
}

public void testIncPrototype() {
test("function A() {} A.prototype.x = 1; A.prototype.x++;", "");
test(
"function A() {} A.prototype.x = 1; A.prototype.x++; new A();",
"function A() {} new A();");
test("externFunction().prototype.x++", "externFunction()");
}

public void testRenamePropertyFunctionTest() { public void testRenamePropertyFunctionTest() {
test( test(
lines( lines(
Expand Down

0 comments on commit 7f71c0b

Please sign in to comment.