diff --git a/src/com/google/javascript/jscomp/RemoveUnusedCode.java b/src/com/google/javascript/jscomp/RemoveUnusedCode.java index 5039e2bc004..5094c3fcac5 100644 --- a/src/com/google/javascript/jscomp/RemoveUnusedCode.java +++ b/src/com/google/javascript/jscomp/RemoveUnusedCode.java @@ -593,19 +593,28 @@ private void traverseObjectDefinePropertiesCall(Node callNode, Scope scope) { Node callee = callNode.getFirstChild(); Node targetObject = callNode.getSecondChild(); Node propertyDefinitions = targetObject.getNext(); - // TODO(bradfordcsmith): It should be possible to remove the entire Object.defineProperties() - // call if the target object is otherwise unused and evaluating the property definitions object - // has no side-effects. - // TODO(bradfordcsmith): If the affected object is a variable reference, this should be - // considered a variable assignment similar to a goog.inherits() call, so it won't block removal - // of the variable. - traverseNode(callee, scope); - traverseNode(targetObject, scope); - if (propertyDefinitions.isObjectLit()) { - // TODO(bradfordcsmith): Consider restricting special handling of the properties literal to - // cases where the target object is a known class, prototype, or this. - traverseObjectDefinePropertiesLiteral(propertyDefinitions, scope); + + if ((targetObject.isName() || isNameDotPrototype(targetObject)) + && !NodeUtil.isExpressionResultUsed(callNode)) { + // NOTE: Object.defineProperties() returns its first argument, so if its return value is used + // that counts as a use of the targetObject. + Node nameNode = targetObject.isName() ? targetObject : targetObject.getFirstChild(); + VarInfo varInfo = traverseNameNode(nameNode, scope); + RemovableBuilder builder = new RemovableBuilder(); + // TODO(bradfordcsmith): Is it really necessary to traverse the callee + // (aka. Object.defineProperties)? + builder.addContinuation(new Continuation(callee, scope)); + if (NodeUtil.mayHaveSideEffects(propertyDefinitions)) { + traverseNode(propertyDefinitions, scope); + } else { + builder.addContinuation(new Continuation(propertyDefinitions, scope)); + } + varInfo.addRemovable(builder.buildClassSetupCall(callNode)); } else { + // TODO(bradfordcsmith): Is it really necessary to traverse the callee + // (aka. Object.defineProperties)? + traverseNode(callee, scope); + traverseNode(targetObject, scope); traverseNode(propertyDefinitions, scope); } } @@ -663,11 +672,20 @@ private void traverseObjectLiteral(Node objectLiteral, Scope scope) { // Is this an object literal that is assigned directly to a 'prototype' property? if (isAssignmentToPrototype(objectLiteral.getParent())) { traversePrototypeLiteral(objectLiteral, scope); + } else if (isObjectDefinePropertiesSecondArgument(objectLiteral)) { + // TODO(bradfordcsmith): Consider restricting special handling of the properties literal to + // cases where the target object is a known class, prototype, or this. + traverseObjectDefinePropertiesLiteral(objectLiteral, scope); } else { traverseNonPrototypeObjectLiteral(objectLiteral, scope); } } + private boolean isObjectDefinePropertiesSecondArgument(Node n) { + Node parent = n.getParent(); + return NodeUtil.isObjectDefinePropertiesDefinition(parent) && parent.getLastChild() == n; + } + private void traverseNonPrototypeObjectLiteral(Node objectLiteral, Scope scope) { for (Node propertyNode = objectLiteral.getFirstChild(); propertyNode != null; @@ -806,7 +824,6 @@ private void traverseVanillaForNameDeclarations(Node nameDeclaration, Scope scop } else { VanillaForNameDeclaration vanillaForNameDeclaration = new RemovableBuilder() - .setAssignedValue(valueNode) .addContinuation(new Continuation(valueNode, scope)) .buildVanillaForNameDeclaration(nameNode); varInfo.addRemovable(vanillaForNameDeclaration); @@ -834,7 +851,7 @@ private void traverseDeclarationStatement(Node declarationStatement, Scope scope builder.addContinuation(new Continuation(valueNode, scope)); } NameDeclarationStatement removable = - builder.setAssignedValue(valueNode).buildNameDeclarationStatement(declarationStatement); + builder.buildNameDeclarationStatement(declarationStatement); varInfo.addRemovable(removable); } } @@ -924,7 +941,6 @@ private void traverseAssign(Node assignNode, Scope scope) { } private void traverseRemovableAssignValue(Node valueNode, RemovableBuilder builder, Scope scope) { - builder.setAssignedValue(valueNode); if (NodeUtil.mayHaveSideEffects(valueNode) || NodeUtil.isExpressionResultUsed(valueNode.getParent())) { traverseNode(valueNode, scope); @@ -1520,7 +1536,6 @@ private abstract class Removable { private final List continuations; @Nullable private final String propertyName; - @Nullable private final Node assignedValue; private final boolean isPrototypeDotPropertyReference; private final boolean isThisDotPropertyReference; @@ -1530,7 +1545,6 @@ private abstract class Removable { Removable(RemovableBuilder builder) { continuations = builder.continuations; propertyName = builder.propertyName; - assignedValue = builder.assignedValue; isPrototypeDotPropertyReference = builder.isPrototypeDotPropertyReference; isThisDotPropertyReference = builder.isThisDotPropertyReference; } @@ -1640,7 +1654,6 @@ private class RemovableBuilder { final List continuations = new ArrayList<>(); @Nullable String propertyName = null; - @Nullable public Node assignedValue = null; boolean isPrototypeDotPropertyReference = false; boolean isThisDotPropertyReference = false; @@ -1649,11 +1662,6 @@ RemovableBuilder addContinuation(Continuation continuation) { return this; } - RemovableBuilder setAssignedValue(@Nullable Node assignedValue) { - this.assignedValue = assignedValue; - return this; - } - RemovableBuilder setIsPrototypeDotPropertyReference(boolean value) { this.isPrototypeDotPropertyReference = value; return this; @@ -1701,12 +1709,10 @@ NameDeclarationStatement buildNameDeclarationStatement(Node declarationStatement Assign buildNamedPropertyAssign(Node assignNode, Node propertyNode) { this.propertyName = propertyNode.getString(); - checkNotNull(assignedValue); return new Assign(this, assignNode, Kind.NAMED_PROPERTY, propertyNode); } Assign buildComputedPropertyAssign(Node assignNode, Node propertyNode) { - checkNotNull(assignedValue); return new Assign(this, assignNode, Kind.COMPUTED_PROPERTY, propertyNode); } @@ -2313,9 +2319,29 @@ private class ClassSetupCall extends Removable { @Override public void removeInternal(AbstractCompiler compiler) { Node parent = callNode.getParent(); + + Node replacement = null; + // Need to keep call args that have side effects. + // Easiest thing to do is break apart the call node as we go. + // First child is the callee (aka. Object.defineProperties or equivalent) + callNode.removeFirstChild(); + for (Node arg = callNode.getLastChild(); arg != null; arg = callNode.getLastChild()) { + arg.detach(); + if (NodeUtil.mayHaveSideEffects(arg)) { + if (replacement == null) { + replacement = arg; + } else { + replacement = IR.comma(arg, replacement).srcref(callNode); + } + } else { + NodeUtil.markFunctionsDeleted(arg, compiler); + } + } // NOTE: The call must either be its own statement or the LHS of a comma expression, // because it doesn't have a meaningful return value. - if (parent.isExprResult()) { + if (replacement != null) { + replaceNodeWith(callNode, replacement); + } else if (parent.isExprResult()) { NodeUtil.deleteNode(parent, compiler); } else { // `(goog.inherits(A, B), something)` -> `something` @@ -2325,6 +2351,13 @@ public void removeInternal(AbstractCompiler compiler) { parent.replaceWith(rhs.detach()); } } + + @Override + public boolean preventsRemovalOfVariableWithNonLocalValueOrPrototype() { + // If we aren't sure where X comes from and what aliases it might have, we cannot be sure + // it's safe to remove the class setup for it. + return true; + } } private static boolean alreadyRemoved(Node n) { diff --git a/test/com/google/javascript/jscomp/RemoveUnusedCodeNameAnalyzerTest.java b/test/com/google/javascript/jscomp/RemoveUnusedCodeNameAnalyzerTest.java index 1d6bdf1d22a..dfcf0bb2367 100644 --- a/test/com/google/javascript/jscomp/RemoveUnusedCodeNameAnalyzerTest.java +++ b/test/com/google/javascript/jscomp/RemoveUnusedCodeNameAnalyzerTest.java @@ -1982,6 +1982,11 @@ public void testSingletonGetter3() { testSame("function Foo() {} goog$addSingletonGetter(Foo); Foo.getInstance();"); } + public void testObjectDefineProperty() { + // TODO(bradfordcsmith): Remove Object.defineProperty() like we do Object.defineProperties(). + testSame("var a = {}; Object.defineProperty(a, 'prop', {value: 5});"); + } + public void testObjectDefinePropertiesOnNamespaceThatEscapes() { testSame("var a = doThing1(); Object.defineProperties(a, {'prop': {value: 5}});"); } @@ -1999,25 +2004,24 @@ public void testRegularAssignPropOnPropFromAVar() { testSame("var b = 5; var a = {}; a.prop = b; use(a.prop);"); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestUnanalyzableObjectDefineProperties() { - testSame("var a = {}; Object.defineProperties(a, externfoo);"); + public void testUnanalyzableObjectDefineProperties() { + test("var a = {}; Object.defineProperties(a, externfoo);", ""); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefinePropertiesOnNamespace1() { + public void testObjectDefinePropertiesOnNamespace1() { testSame("var a = {}; Object.defineProperties(a, {prop: {value: 5}}); use(a.prop);"); test("var a = {}; Object.defineProperties(a, {prop: {value: 5}});", ""); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefinePropertiesOnNamespace2() { - // Note that since we try to remove the entire Object.defineProperties call whole hog, - // we can't remove a single property from the object literal. - testSame( + public void testObjectDefinePropertiesOnNamespace2() { + test( lines( "var a = {};", "Object.defineProperties(a, {p1: {value: 5}, p2: {value: 3} });", + "use(a.p1);"), + lines( + "var a = {};", + "Object.defineProperties(a, {p1: {value: 5} });", "use(a.p1);")); test( @@ -2031,8 +2035,7 @@ public void testNonAnalyzableObjectDefinePropertiesCall() { testSame("var a = {}; var z = Object.defineProperties(a, {'prop': {value: 5}}); use(z);"); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefinePropertiesOnNamespace3() { + public void testObjectDefinePropertiesOnNamespace3() { testSame( "var b = 5;" + "var a = {};" @@ -2047,17 +2050,16 @@ public void disabledTestObjectDefinePropertiesOnNamespace3() { "var b = 5; use(b);"); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefinePropertiesOnNamespace4() { + public void testObjectDefinePropertiesOnNamespace4() { test( - "function b() { alert('hello'); };" - + "var a = {};" - + "Object.defineProperties(a, {prop: {value: b()}});", - "function b() { alert('hello'); }; b()"); + lines( + "function b() { alert('hello'); };", + "var a = {};", + "Object.defineProperties(a, {prop: {value: b()}});"), + "function b() { alert('hello'); }; ({prop: {value: b()}});"); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefinePropertiesOnNamespace5() { + public void testObjectDefinePropertiesOnNamespace5() { test( lines( "function b() { alert('hello'); };", // preserve newline @@ -2068,17 +2070,15 @@ public void disabledTestObjectDefinePropertiesOnNamespace5() { "function b() { alert('hello'); };", // preserve newline "function c() { alert('world'); };", " ", - "b(); c();")); + " ({p1: {value: b()}, p2: {value: c()}});")); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefinePropertiesOnConstructor() { + public void testObjectDefinePropertiesOnConstructor() { testSame("function Foo() {} Object.defineProperties(Foo, {prop: {value: 5}}); use(Foo.prop);"); test("function Foo() {} Object.defineProperties(Foo, {prop: {value: 5}});", ""); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefinePropertiesOnPrototype1() { + public void testObjectDefinePropertiesOnPrototype1() { testSame( "function Foo() {}" + "Object.defineProperties(Foo.prototype, {prop: {value: 5}});" @@ -2087,8 +2087,7 @@ public void disabledTestObjectDefinePropertiesOnPrototype1() { test("function Foo() {} Object.defineProperties(Foo.prototype, {prop: {value: 5}});", ""); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefinePropertiesOnPrototype2() { + public void testObjectDefinePropertiesOnPrototype2() { test( lines( "var b = 5;", @@ -2098,18 +2097,16 @@ public void disabledTestObjectDefinePropertiesOnPrototype2() { "var b = 5; use(b);"); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefinePropertiesOnPrototype3() { + public void testObjectDefinePropertiesOnPrototype3() { test( lines( "var b = function() {};", "function Foo() {}", "Object.defineProperties(Foo.prototype, {prop: {value: b()}});"), - "var b = function() {}; b();"); + "var b = function() {}; ({prop: {value: b()}});"); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefineGetters() { + public void testObjectDefineGetters() { test("function Foo() {} Object.defineProperties(Foo, {prop: {get: function() {}}});", ""); test( @@ -2117,8 +2114,7 @@ public void disabledTestObjectDefineGetters() { ""); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefineSetters() { + public void testObjectDefineSetters() { test("function Foo() {} Object.defineProperties(Foo, {prop: {set: function() {}}});", ""); test( @@ -2126,8 +2122,7 @@ public void disabledTestObjectDefineSetters() { ""); } - // TODO(b/66971163): implement removal of defineProperties() when object is unused - public void disabledTestObjectDefineSetters_global() { + public void testObjectDefineSetters_global() { test( lines( "function Foo() {} ",