diff --git a/src/com/google/javascript/jscomp/RemoveUnusedCode.java b/src/com/google/javascript/jscomp/RemoveUnusedCode.java index 1d700c1d262..44b3519842b 100644 --- a/src/com/google/javascript/jscomp/RemoveUnusedCode.java +++ b/src/com/google/javascript/jscomp/RemoveUnusedCode.java @@ -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); @@ -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); } @@ -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(); @@ -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. @@ -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; @@ -1257,6 +1270,7 @@ private abstract class Removable { propertyName = builder.propertyName; assignedValue = builder.assignedValue; isPrototypeObjectPropertyAssignment = builder.isPrototypeObjectPropertyAssignment; + isThisNamedPropertyAssignment = builder.isThisNamedPropertyAssignment; } String getPropertyName() { @@ -1340,9 +1354,13 @@ boolean isClassOrPrototypeNamedProperty() { return false; } - boolean isIndependentlyRemovableNamedProperty() { + boolean isPrototypeProperty() { return isPrototypeObjectNamedPropertyAssignment() || isClassOrPrototypeNamedProperty(); } + + boolean isThisNamedPropertyAssignment() { + return isThisNamedPropertyAssignment; + } } private class RemovableBuilder { @@ -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); @@ -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); } diff --git a/test/com/google/javascript/jscomp/OptimizeCallsTest.java b/test/com/google/javascript/jscomp/OptimizeCallsTest.java index 30816122bf1..96e8b686d08 100644 --- a/test/com/google/javascript/jscomp/OptimizeCallsTest.java +++ b/test/com/google/javascript/jscomp/OptimizeCallsTest.java @@ -42,6 +42,7 @@ public OptimizeCallsTest() { protected void setUp() throws Exception { super.setUp(); enableNormalize(); + enableGatherExternProperties(); } @Override diff --git a/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java b/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java index 8dac5173473..be05233e7ab 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java @@ -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() { @@ -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 @@ -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() { diff --git a/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java b/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java index c7a4c20d49f..ba91417be9d 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedCodeTest.java @@ -48,6 +48,7 @@ protected int getNumRepetitions() { protected void setUp() throws Exception { super.setUp(); enableNormalize(); + enableGatherExternProperties(); removeGlobal = true; preserveFunctionExpressionNames = false; } @@ -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;", ""); }