From 7928b1720d1f2a27a4f6ae242fce7c9552e19903 Mon Sep 17 00:00:00 2001 From: bradfordcsmith Date: Fri, 22 Dec 2017 12:53:47 -0800 Subject: [PATCH] RemoveUnusedCode: Remove unused references like this.prop; and *.prototype.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 --- .../javascript/jscomp/RemoveUnusedCode.java | 131 +++++++++++++----- .../RemoveUnusedCodeClassPropertiesTest.java | 9 +- ...moveUnusedCodePrototypePropertiesTest.java | 18 +++ .../jscomp/RemoveUnusedCodeTest.java | 9 +- 4 files changed, 127 insertions(+), 40 deletions(-) diff --git a/src/com/google/javascript/jscomp/RemoveUnusedCode.java b/src/com/google/javascript/jscomp/RemoveUnusedCode.java index 5361a0c3cf9..c47ba494e27 100644 --- a/src/com/google/javascript/jscomp/RemoveUnusedCode.java +++ b/src/com/google/javascript/jscomp/RemoveUnusedCode.java @@ -392,11 +392,7 @@ private void traverseNode(Node n, Scope scope) { break; case GETPROP: - Node objectNode = n.getFirstChild(); - Node propertyNameNode = objectNode.getNext(); - String propertyName = propertyNameNode.getString(); - markPropertyNameReferenced(propertyName); - traverseNode(objectNode, scope); + traverseGetProp(n, scope); break; default: @@ -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) { checkArgument(incOrDecOp.isInc() || incOrDecOp.isDec(), incOrDecOp); Node arg = incOrDecOp.getOnlyChild(); @@ -416,13 +449,12 @@ private void traverseIncrementOrDecrementOp(Node incOrDecOp, Scope scope) { Node propertyNameNode = arg.getLastChild(); if (getPropObj.isThis()) { // this.propName++ - RemovableBuilder builder = new RemovableBuilder().setIsThisNamedPropertyAssignment(true); + RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true); considerForIndependentRemoval(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode)); } else if (isDotPrototype(getPropObj)) { // someExpression.prototype.propName++ Node exprObj = getPropObj.getFirstChild(); - RemovableBuilder builder = - new RemovableBuilder().setIsPrototypeObjectPropertyAssignment(true); + RemovableBuilder builder = new RemovableBuilder().setIsPrototypeDotPropertyReference(true); if (exprObj.isName()) { // varName.prototype.propName++ VarInfo varInfo = traverseNameNode(exprObj, scope); @@ -455,7 +487,7 @@ private void traverseCompoundAssign(Node compoundAssignNode, Scope scope) { if (targetNode.isGetProp() && targetNode.getFirstChild().isThis() && !NodeUtil.isExpressionResultUsed(compoundAssignNode)) { - RemovableBuilder builder = new RemovableBuilder().setIsThisNamedPropertyAssignment(true); + RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true); traverseRemovableAssignValue(valueNode, builder, scope); considerForIndependentRemoval( builder.buildNamedPropertyAssign(compoundAssignNode, targetNode.getLastChild())); @@ -767,8 +799,7 @@ private void traverseAssign(Node assignNode, Scope scope) { } else if (isDotPrototype(getPropLhs)) { // objExpression.prototype.propertyName = someValue Node objExpression = getPropLhs.getFirstChild(); - RemovableBuilder builder = - new RemovableBuilder().setIsPrototypeObjectPropertyAssignment(true); + RemovableBuilder builder = new RemovableBuilder().setIsPrototypeDotPropertyReference(true); traverseRemovableAssignValue(valueNode, builder, scope); if (objExpression.isName()) { // varName.prototype.propertyName = someValue @@ -787,7 +818,7 @@ private void traverseAssign(Node assignNode, Scope scope) { } } else if (getPropLhs.isThis()) { // this.propertyName = someValue - RemovableBuilder builder = new RemovableBuilder().setIsThisNamedPropertyAssignment(true); + RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true); traverseRemovableAssignValue(valueNode, builder, scope); considerForIndependentRemoval(builder.buildNamedPropertyAssign(assignNode, propNameNode)); } else { @@ -1124,7 +1155,7 @@ private void considerForIndependentRemoval(Removable removable) { } else if (removeUnusedPrototypeProperties && removable.isPrototypeProperty()) { // Store for possible removal later. removablesForPropertyNames.put(propertyName, removable); - } else if (removeUnusedThisProperties && removable.isThisNamedPropertyAssignment()) { + } else if (removeUnusedThisProperties && removable.isThisDotPropertyReference()) { // Store for possible removal later. removablesForPropertyNames.put(propertyName, removable); } else { @@ -1354,8 +1385,8 @@ private abstract class Removable { private final List continuations; @Nullable private final String propertyName; @Nullable private final Node assignedValue; - private final boolean isPrototypeObjectPropertyAssignment; - private final boolean isThisNamedPropertyAssignment; + private final boolean isPrototypeDotPropertyReference; + private final boolean isThisDotPropertyReference; private boolean continuationsAreApplied = false; private boolean isRemoved = false; @@ -1364,8 +1395,8 @@ private abstract class Removable { continuations = builder.continuations; propertyName = builder.propertyName; assignedValue = builder.assignedValue; - isPrototypeObjectPropertyAssignment = builder.isPrototypeObjectPropertyAssignment; - isThisNamedPropertyAssignment = builder.isThisNamedPropertyAssignment; + isPrototypeDotPropertyReference = builder.isPrototypeDotPropertyReference; + isThisDotPropertyReference = builder.isThisDotPropertyReference; } String getPropertyName() { @@ -1441,8 +1472,8 @@ boolean isPrototypeAssignment() { } /** Is this an assignment to a property on a prototype object? */ - boolean isPrototypeObjectNamedPropertyAssignment() { - return isPrototypeObjectPropertyAssignment && isNamedPropertyAssignment(); + boolean isPrototypeDotPropertyReference() { + return isPrototypeDotPropertyReference; } boolean isClassOrPrototypeNamedProperty() { @@ -1450,11 +1481,11 @@ boolean isClassOrPrototypeNamedProperty() { } boolean isPrototypeProperty() { - return isPrototypeObjectNamedPropertyAssignment() || isClassOrPrototypeNamedProperty(); + return isPrototypeDotPropertyReference() || isClassOrPrototypeNamedProperty(); } - boolean isThisNamedPropertyAssignment() { - return isThisNamedPropertyAssignment; + boolean isThisDotPropertyReference() { + return isThisDotPropertyReference; } } @@ -1463,8 +1494,8 @@ private class RemovableBuilder { @Nullable String propertyName = null; @Nullable public Node assignedValue = null; - boolean isPrototypeObjectPropertyAssignment = false; - boolean isThisNamedPropertyAssignment = false; + boolean isPrototypeDotPropertyReference = false; + boolean isThisDotPropertyReference = false; RemovableBuilder addContinuation(Continuation continuation) { continuations.add(continuation); @@ -1476,14 +1507,13 @@ RemovableBuilder setAssignedValue(@Nullable Node assignedValue) { return this; } - RemovableBuilder setIsPrototypeObjectPropertyAssignment( - boolean isPrototypeObjectPropertyAssignment) { - this.isPrototypeObjectPropertyAssignment = isPrototypeObjectPropertyAssignment; + RemovableBuilder setIsPrototypeDotPropertyReference(boolean value) { + this.isPrototypeDotPropertyReference = value; return this; } - RemovableBuilder setIsThisNamedPropertyAssignment(boolean value) { - this.isThisNamedPropertyAssignment = value; + RemovableBuilder setIsThisDotPropertyReference(boolean value) { + this.isThisDotPropertyReference = value; return this; } @@ -1550,6 +1580,43 @@ IncOrDecOp buildIncOrDepOp(Node incOrDecOp, Node propertyNode) { this.propertyName = propertyNode.getString(); 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. */ @@ -1560,6 +1627,7 @@ private class IncOrDecOp extends Removable { super(builder); checkArgument(incOrDecNode.isInc() || incOrDecNode.isDec(), incOrDecNode); Node arg = incOrDecNode.getOnlyChild(); + // TODO(bradfordcsmith): handle `name;` and `name.property;` references checkState(isThisDotProperty(arg) || isDotPrototypeDotProperty(arg), arg); this.incOrDecNode = incOrDecNode; } @@ -1584,11 +1652,6 @@ void removeInternal(AbstractCompiler compiler) { } } } - - @Override - boolean isNamedPropertyAssignment() { - return isNamedProperty(); - } } /** True for `this.propertyName` */ diff --git a/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java b/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java index eed0dfb292f..807564720bb 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java @@ -60,6 +60,7 @@ public final class RemoveUnusedCodeClassPropertiesTest extends TypeICompilerTest "var EXT = {};", "EXT.ext;", "var externVar;", + "function externFunction() {}", "/** @type {Function} */", "Object.defineProperties = function() {};", "/** @type {Function} */", @@ -184,11 +185,9 @@ public void testInc2() { testSame("let x = (alert(), --this.a)"); } - // TODO(b/66971163): make this pass - public void disabledTestExprResult() { - test("this.x", "0"); - test("c.prototype.x", "0"); - test("SomeSideEffect().prototype.x", "SomeSideEffect(),0"); + public void testExprResult() { + test("this.x", ""); + test("externFunction().prototype.x", "externFunction()"); } public void testJSCompiler_renameProperty() { diff --git a/test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java b/test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java index 9edae0a5705..6ca1e8a6bf2 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java @@ -30,6 +30,7 @@ public final class RemoveUnusedCodePrototypePropertiesTest extends CompilerTestC "var window;", "var Math = {};", "Math.random = function() {};", + "function alert(x) {}", "function externFunction() {}", "externFunction.prototype.externPropName;", "var mExtern;", @@ -76,6 +77,23 @@ protected void setUp() throws Exception { 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() { test("({}.prototype.x = 5, externFunction())", "0, externFunction();"); diff --git a/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java b/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java index 815c1377a02..1a15d838dab 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java @@ -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() { // We don't need the assignment or the assigned value, but we need to keep the AST valid. test( @@ -139,7 +144,9 @@ public void testUsageBeforeDefinition() { } 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() {