diff --git a/src/com/google/javascript/jscomp/RemoveUnusedCode.java b/src/com/google/javascript/jscomp/RemoveUnusedCode.java index 089253cf841..5361a0c3cf9 100644 --- a/src/com/google/javascript/jscomp/RemoveUnusedCode.java +++ b/src/com/google/javascript/jscomp/RemoveUnusedCode.java @@ -307,6 +307,11 @@ private void traverseNode(Node n, Scope scope) { traverseCompoundAssign(n, scope); break; + case INC: + case DEC: + traverseIncrementOrDecrementOp(n, scope); + break; + case CALL: traverseCall(n, scope); break; @@ -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) { // We'll allow removal of compound assignment to a this property as long as the result of the // assignment is unused. @@ -539,10 +585,11 @@ private void traversePrototypeLiteral(Node objectLiteral, Scope scope) { } 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"); } @@ -717,8 +764,7 @@ private void traverseAssign(Node assignNode, Scope scope) { RemovableBuilder builder = new RemovableBuilder(); traverseRemovableAssignValue(valueNode, builder, scope); varInfo.addRemovable(builder.buildNamedPropertyAssign(assignNode, propNameNode)); - } else if (getPropLhs.isGetProp() - && getPropLhs.getLastChild().getString().equals("prototype")) { + } else if (isDotPrototype(getPropLhs)) { // objExpression.prototype.propertyName = someValue Node objExpression = getPropLhs.getFirstChild(); RemovableBuilder builder = @@ -1499,6 +1545,60 @@ AnonymousPrototypeNamedPropertyAssign buildAnonymousPrototypeNamedPropertyAssign this.propertyName = propertyName; 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 { @@ -1788,20 +1888,13 @@ public void removeInternal(AbstractCompiler compiler) { if (mustPreserveRhs && mustPreserveGetElmExpr) { Node replacement = IR.comma(lhs.getLastChild().detach(), rhs.detach()).useSourceInfoFrom(assignNode); - assignNode.replaceWith(replacement); + replaceExpressionWith(assignNode, replacement); } else if (mustPreserveGetElmExpr) { - assignNode.replaceWith(lhs.getLastChild().detach()); - NodeUtil.markFunctionsDeleted(rhs, compiler); + replaceExpressionWith(assignNode, lhs.getLastChild().detach()); } else if (mustPreserveRhs) { - assignNode.replaceWith(rhs.detach()); - NodeUtil.markFunctionsDeleted(lhs, compiler); - } else if (parent.isExprResult()) { - parent.detach(); - NodeUtil.markFunctionsDeleted(parent, compiler); + replaceExpressionWith(assignNode, rhs.detach()); } else { - // value isn't needed, but we need to keep the AST valid. - assignNode.replaceWith(IR.number(0).useSourceInfoFrom(assignNode)); - NodeUtil.markFunctionsDeleted(assignNode, compiler); + removeExpressionCompletely(assignNode); } } @@ -1846,20 +1939,13 @@ void removeInternal(AbstractCompiler compiler) { if (mustPreserveRhs && mustPreserveObjExpression) { Node replacement = IR.comma(objExpression.detach(), rhs.detach()).useSourceInfoFrom(assignNode); - assignNode.replaceWith(replacement); + replaceExpressionWith(assignNode, replacement); } else if (mustPreserveObjExpression) { - assignNode.replaceWith(objExpression.detach()); - NodeUtil.markFunctionsDeleted(rhs, compiler); + replaceExpressionWith(assignNode, objExpression.detach()); } else if (mustPreserveRhs) { - assignNode.replaceWith(rhs.detach()); - NodeUtil.markFunctionsDeleted(objExpression, compiler); - } else if (parent.isExprResult()) { - parent.detach(); - NodeUtil.markFunctionsDeleted(parent, compiler); + replaceExpressionWith(assignNode, rhs.detach()); } else { - // value isn't needed, but we need to keep the AST valid. - assignNode.replaceWith(IR.number(0).useSourceInfoFrom(assignNode)); - NodeUtil.markFunctionsDeleted(assignNode, compiler); + removeExpressionCompletely(assignNode); } } @@ -2058,6 +2144,22 @@ void removeInternal(AbstractCompiler 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); } } diff --git a/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java b/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java index 3d5b1599bf1..eed0dfb292f 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java @@ -160,35 +160,28 @@ public void testAssignOp2() { testSame("const x = (alert(1), this.a += 2)"); } - // TODO(b/66971163): make this pass - public void disabledTestInc1() { + public void testInc1() { // Increments and Decrements are handled similarly to compound assignments // but need a placeholder value when replaced. - test("this.x++", "0"); - testSame("x = (this.x++)"); - testSame("this.x++; x = this.x;"); + test("this.x++", ""); + testSame("let x = (this.x++)"); + testSame("this.x++; let x = this.x;"); - test("--this.x", "0"); - testSame("x = (--this.x)"); - testSame("--this.x; x = this.x;"); + test("--this.x", ""); + testSame("let x = (--this.x)"); + testSame("--this.x; let x = this.x;"); } - // TODO(b/66971163): make this pass - public void disabledTestInc2() { + public void testInc2() { // Increments and Decrements are handled similarly to compound assignments // but need a placeholder value when replaced. - test("this.a++, f()", "0, f()"); - test("x = (this.a++, f())", "x = (0, f())"); - testSame("x = (f(), this.a++)"); + test("this.a++, alert()", "0, alert()"); + test("let x = (this.a++, alert())", "let x = (0, alert())"); + testSame("let x = (alert(), this.a++)"); - test("--this.a, f()", "0, f()"); - test("x = (--this.a, f())", "x = (0, f())"); - testSame("x = (f(), --this.a)"); - } - - // TODO(b/66971163): make this pass - public void disabledTestIncPrototype() { - test("SomeSideEffect().prototype.x++", "SomeSideEffect(), 0"); + test("--this.a, alert()", "0, alert()"); + test("let x = (--this.a, alert())", "let x = (0, alert())"); + testSame("let x = (alert(), --this.a)"); } // TODO(b/66971163): make this pass @@ -243,32 +236,25 @@ public void testIssue730() { // Partial removal of properties can causes problems if the object is // sealed. testSame( - "function A() {this.foo = 0;}\n" + - "function B() {this.a = new A();}\n" + - "B.prototype.dostuff = function() {this.a.foo++;alert('hi');}\n" + - "new B().dostuff();\n"); - } - - // 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"); + lines( + "function A() {this.foo = 0;}", + "function B() {this.a = new A();}", + "B.prototype.dostuff = function() { this.a.foo++; alert('hi'); }", + "new B().dostuff();")); } - // TODO(b/66971163): make this pass - public void disabledTestPrototypeProps1() { + public void testPrototypeProps1() { test( - "function A() {this.foo = 1;}\n" + - "A.prototype.foo = 0;\n" + - "A.prototype.method = function() {this.foo++};\n" + - "new A().method()\n", - "function A() {1;}\n" + - "0;\n" + - "A.prototype.method = function() {0;};\n" + - "new A().method()\n"); + lines( + "function A() {this.foo = 1;}", + "A.prototype.foo = 0;", + "A.prototype.method = function() {this.foo++};", + "new A().method()"), + lines( + "function A() { }", + " ", + "A.prototype.method = function() { };", + "new A().method()")); } public void testPrototypeProps2() { diff --git a/test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java b/test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java index 170ab20bcee..9edae0a5705 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java @@ -76,6 +76,7 @@ protected void setUp() throws Exception { allowRemovalOfExternProperties = false; } + public void testAnonymousPrototypePropertyRemoved() { test("({}.prototype.x = 5, externFunction())", "0, externFunction();"); test("({}).prototype.x = 5;", ""); @@ -108,6 +109,20 @@ public void testAnonymousPrototypePropertyNoRemoveSideEffect1() { "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() { test( lines(