Skip to content

Commit

Permalink
RemoveUnusedCode: Remove Object.defineProperties(unusedVar, ...
Browse files Browse the repository at this point in the history 
Also do a little bit of code clean up.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=183721363
  • Loading branch information
@brad4d @lauraharker
brad4d authored and lauraharker committed Jan 30, 2018
1 parent e2ff7ef commit d04416a
Show file tree
Hide file tree
Showing 2 changed files with 90 additions and 62 deletions.
85 changes: 59 additions & 26 deletions src/com/google/javascript/jscomp/RemoveUnusedCode.java
View file
Expand Up @@ -593,19 +593,28 @@ private void traverseObjectDefinePropertiesCall(Node callNode, Scope scope) {
Node callee = callNode.getFirstChild(); Node callee = callNode.getFirstChild();
Node targetObject = callNode.getSecondChild(); Node targetObject = callNode.getSecondChild();
Node propertyDefinitions = targetObject.getNext(); 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 if ((targetObject.isName() || isNameDotPrototype(targetObject))
// has no side-effects. && !NodeUtil.isExpressionResultUsed(callNode)) {
// TODO(bradfordcsmith): If the affected object is a variable reference, this should be // NOTE: Object.defineProperties() returns its first argument, so if its return value is used
// considered a variable assignment similar to a goog.inherits() call, so it won't block removal // that counts as a use of the targetObject.
// of the variable. Node nameNode = targetObject.isName() ? targetObject : targetObject.getFirstChild();
traverseNode(callee, scope); VarInfo varInfo = traverseNameNode(nameNode, scope);
traverseNode(targetObject, scope); RemovableBuilder builder = new RemovableBuilder();
if (propertyDefinitions.isObjectLit()) { // TODO(bradfordcsmith): Is it really necessary to traverse the callee
// TODO(bradfordcsmith): Consider restricting special handling of the properties literal to // (aka. Object.defineProperties)?
// cases where the target object is a known class, prototype, or this. builder.addContinuation(new Continuation(callee, scope));
traverseObjectDefinePropertiesLiteral(propertyDefinitions, scope); if (NodeUtil.mayHaveSideEffects(propertyDefinitions)) {
traverseNode(propertyDefinitions, scope);
} else {
builder.addContinuation(new Continuation(propertyDefinitions, scope));
}
varInfo.addRemovable(builder.buildClassSetupCall(callNode));
} else { } else {
// TODO(bradfordcsmith): Is it really necessary to traverse the callee
// (aka. Object.defineProperties)?
traverseNode(callee, scope);
traverseNode(targetObject, scope);
traverseNode(propertyDefinitions, scope); traverseNode(propertyDefinitions, scope);
} }
} }
Expand Down Expand Up @@ -663,11 +672,20 @@ private void traverseObjectLiteral(Node objectLiteral, Scope scope) {
// Is this an object literal that is assigned directly to a 'prototype' property? // Is this an object literal that is assigned directly to a 'prototype' property?
if (isAssignmentToPrototype(objectLiteral.getParent())) { if (isAssignmentToPrototype(objectLiteral.getParent())) {
traversePrototypeLiteral(objectLiteral, scope); 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 { } else {
traverseNonPrototypeObjectLiteral(objectLiteral, scope); 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) { private void traverseNonPrototypeObjectLiteral(Node objectLiteral, Scope scope) {
for (Node propertyNode = objectLiteral.getFirstChild(); for (Node propertyNode = objectLiteral.getFirstChild();
propertyNode != null; propertyNode != null;
Expand Down Expand Up @@ -806,7 +824,6 @@ private void traverseVanillaForNameDeclarations(Node nameDeclaration, Scope scop
} else { } else {
VanillaForNameDeclaration vanillaForNameDeclaration = VanillaForNameDeclaration vanillaForNameDeclaration =
new RemovableBuilder() new RemovableBuilder()
.setAssignedValue(valueNode)
.addContinuation(new Continuation(valueNode, scope)) .addContinuation(new Continuation(valueNode, scope))
.buildVanillaForNameDeclaration(nameNode); .buildVanillaForNameDeclaration(nameNode);
varInfo.addRemovable(vanillaForNameDeclaration); varInfo.addRemovable(vanillaForNameDeclaration);
Expand Down Expand Up @@ -834,7 +851,7 @@ private void traverseDeclarationStatement(Node declarationStatement, Scope scope
builder.addContinuation(new Continuation(valueNode, scope)); builder.addContinuation(new Continuation(valueNode, scope));
} }
NameDeclarationStatement removable = NameDeclarationStatement removable =
builder.setAssignedValue(valueNode).buildNameDeclarationStatement(declarationStatement); builder.buildNameDeclarationStatement(declarationStatement);
varInfo.addRemovable(removable); varInfo.addRemovable(removable);
} }
} }
Expand Down Expand Up @@ -924,7 +941,6 @@ private void traverseAssign(Node assignNode, Scope scope) {
} }


private void traverseRemovableAssignValue(Node valueNode, RemovableBuilder builder, Scope scope) { private void traverseRemovableAssignValue(Node valueNode, RemovableBuilder builder, Scope scope) {
builder.setAssignedValue(valueNode);
if (NodeUtil.mayHaveSideEffects(valueNode) if (NodeUtil.mayHaveSideEffects(valueNode)
|| NodeUtil.isExpressionResultUsed(valueNode.getParent())) { || NodeUtil.isExpressionResultUsed(valueNode.getParent())) {
traverseNode(valueNode, scope); traverseNode(valueNode, scope);
Expand Down Expand Up @@ -1520,7 +1536,6 @@ private abstract class Removable {


private final List<Continuation> continuations; private final List<Continuation> continuations;
@Nullable private final String propertyName; @Nullable private final String propertyName;
@Nullable private final Node assignedValue;
private final boolean isPrototypeDotPropertyReference; private final boolean isPrototypeDotPropertyReference;
private final boolean isThisDotPropertyReference; private final boolean isThisDotPropertyReference;


Expand All @@ -1530,7 +1545,6 @@ private abstract class Removable {
Removable(RemovableBuilder builder) { Removable(RemovableBuilder builder) {
continuations = builder.continuations; continuations = builder.continuations;
propertyName = builder.propertyName; propertyName = builder.propertyName;
assignedValue = builder.assignedValue;
isPrototypeDotPropertyReference = builder.isPrototypeDotPropertyReference; isPrototypeDotPropertyReference = builder.isPrototypeDotPropertyReference;
isThisDotPropertyReference = builder.isThisDotPropertyReference; isThisDotPropertyReference = builder.isThisDotPropertyReference;
} }
Expand Down Expand Up @@ -1640,7 +1654,6 @@ private class RemovableBuilder {
final List<Continuation> continuations = new ArrayList<>(); final List<Continuation> continuations = new ArrayList<>();


@Nullable String propertyName = null; @Nullable String propertyName = null;
@Nullable public Node assignedValue = null;
boolean isPrototypeDotPropertyReference = false; boolean isPrototypeDotPropertyReference = false;
boolean isThisDotPropertyReference = false; boolean isThisDotPropertyReference = false;


Expand All @@ -1649,11 +1662,6 @@ RemovableBuilder addContinuation(Continuation continuation) {
return this; return this;
} }


RemovableBuilder setAssignedValue(@Nullable Node assignedValue) {
this.assignedValue = assignedValue;
return this;
}

RemovableBuilder setIsPrototypeDotPropertyReference(boolean value) { RemovableBuilder setIsPrototypeDotPropertyReference(boolean value) {
this.isPrototypeDotPropertyReference = value; this.isPrototypeDotPropertyReference = value;
return this; return this;
Expand Down Expand Up @@ -1701,12 +1709,10 @@ NameDeclarationStatement buildNameDeclarationStatement(Node declarationStatement


Assign buildNamedPropertyAssign(Node assignNode, Node propertyNode) { Assign buildNamedPropertyAssign(Node assignNode, Node propertyNode) {
this.propertyName = propertyNode.getString(); this.propertyName = propertyNode.getString();
checkNotNull(assignedValue);
return new Assign(this, assignNode, Kind.NAMED_PROPERTY, propertyNode); return new Assign(this, assignNode, Kind.NAMED_PROPERTY, propertyNode);
} }


Assign buildComputedPropertyAssign(Node assignNode, Node propertyNode) { Assign buildComputedPropertyAssign(Node assignNode, Node propertyNode) {
checkNotNull(assignedValue);
return new Assign(this, assignNode, Kind.COMPUTED_PROPERTY, propertyNode); return new Assign(this, assignNode, Kind.COMPUTED_PROPERTY, propertyNode);
} }


Expand Down Expand Up @@ -2313,9 +2319,29 @@ private class ClassSetupCall extends Removable {
@Override @Override
public void removeInternal(AbstractCompiler compiler) { public void removeInternal(AbstractCompiler compiler) {
Node parent = callNode.getParent(); 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, // 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. // 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); NodeUtil.deleteNode(parent, compiler);
} else { } else {
// `(goog.inherits(A, B), something)` -> `something` // `(goog.inherits(A, B), something)` -> `something`
Expand All @@ -2325,6 +2351,13 @@ public void removeInternal(AbstractCompiler compiler) {
parent.replaceWith(rhs.detach()); 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) { private static boolean alreadyRemoved(Node n) {
Expand Down
67 changes: 31 additions & 36 deletions test/com/google/javascript/jscomp/RemoveUnusedCodeNameAnalyzerTest.java
View file
Expand Up @@ -1982,6 +1982,11 @@ public void testSingletonGetter3() {
testSame("function Foo() {} goog$addSingletonGetter(Foo); Foo.getInstance();"); 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() { public void testObjectDefinePropertiesOnNamespaceThatEscapes() {
testSame("var a = doThing1(); Object.defineProperties(a, {'prop': {value: 5}});"); testSame("var a = doThing1(); Object.defineProperties(a, {'prop': {value: 5}});");
} }
Expand All @@ -1999,25 +2004,24 @@ public void testRegularAssignPropOnPropFromAVar() {
testSame("var b = 5; var a = {}; a.prop = b; use(a.prop);"); 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 testUnanalyzableObjectDefineProperties() {
public void disabledTestUnanalyzableObjectDefineProperties() { test("var a = {}; Object.defineProperties(a, externfoo);", "");
testSame("var a = {}; Object.defineProperties(a, externfoo);");
} }


// TODO(b/66971163): implement removal of defineProperties() when object is unused public void testObjectDefinePropertiesOnNamespace1() {
public void disabledTestObjectDefinePropertiesOnNamespace1() {
testSame("var a = {}; Object.defineProperties(a, {prop: {value: 5}}); use(a.prop);"); testSame("var a = {}; Object.defineProperties(a, {prop: {value: 5}}); use(a.prop);");
test("var a = {}; Object.defineProperties(a, {prop: {value: 5}});", ""); test("var a = {}; Object.defineProperties(a, {prop: {value: 5}});", "");
} }


// TODO(b/66971163): implement removal of defineProperties() when object is unused public void testObjectDefinePropertiesOnNamespace2() {
public void disabledTestObjectDefinePropertiesOnNamespace2() { test(
// 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(
lines( lines(
"var a = {};", "var a = {};",
"Object.defineProperties(a, {p1: {value: 5}, p2: {value: 3} });", "Object.defineProperties(a, {p1: {value: 5}, p2: {value: 3} });",
"use(a.p1);"),
lines(
"var a = {};",
"Object.defineProperties(a, {p1: {value: 5} });",
"use(a.p1);")); "use(a.p1);"));


test( test(
Expand All @@ -2031,8 +2035,7 @@ public void testNonAnalyzableObjectDefinePropertiesCall() {
testSame("var a = {}; var z = Object.defineProperties(a, {'prop': {value: 5}}); use(z);"); 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 testObjectDefinePropertiesOnNamespace3() {
public void disabledTestObjectDefinePropertiesOnNamespace3() {
testSame( testSame(
"var b = 5;" "var b = 5;"
+ "var a = {};" + "var a = {};"
Expand All @@ -2047,17 +2050,16 @@ public void disabledTestObjectDefinePropertiesOnNamespace3() {
"var b = 5; use(b);"); "var b = 5; use(b);");
} }


// TODO(b/66971163): implement removal of defineProperties() when object is unused public void testObjectDefinePropertiesOnNamespace4() {
public void disabledTestObjectDefinePropertiesOnNamespace4() {
test( test(
"function b() { alert('hello'); };" lines(
+ "var a = {};" "function b() { alert('hello'); };",
+ "Object.defineProperties(a, {prop: {value: b()}});", "var a = {};",
"function b() { alert('hello'); }; b()"); "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 testObjectDefinePropertiesOnNamespace5() {
public void disabledTestObjectDefinePropertiesOnNamespace5() {
test( test(
lines( lines(
"function b() { alert('hello'); };", // preserve newline "function b() { alert('hello'); };", // preserve newline
Expand All @@ -2068,17 +2070,15 @@ public void disabledTestObjectDefinePropertiesOnNamespace5() {
"function b() { alert('hello'); };", // preserve newline "function b() { alert('hello'); };", // preserve newline
"function c() { alert('world'); };", "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 testObjectDefinePropertiesOnConstructor() {
public void disabledTestObjectDefinePropertiesOnConstructor() {
testSame("function Foo() {} Object.defineProperties(Foo, {prop: {value: 5}}); use(Foo.prop);"); testSame("function Foo() {} Object.defineProperties(Foo, {prop: {value: 5}}); use(Foo.prop);");
test("function Foo() {} Object.defineProperties(Foo, {prop: {value: 5}});", ""); test("function Foo() {} Object.defineProperties(Foo, {prop: {value: 5}});", "");
} }


// TODO(b/66971163): implement removal of defineProperties() when object is unused public void testObjectDefinePropertiesOnPrototype1() {
public void disabledTestObjectDefinePropertiesOnPrototype1() {
testSame( testSame(
"function Foo() {}" "function Foo() {}"
+ "Object.defineProperties(Foo.prototype, {prop: {value: 5}});" + "Object.defineProperties(Foo.prototype, {prop: {value: 5}});"
Expand All @@ -2087,8 +2087,7 @@ public void disabledTestObjectDefinePropertiesOnPrototype1() {
test("function Foo() {} Object.defineProperties(Foo.prototype, {prop: {value: 5}});", ""); test("function Foo() {} Object.defineProperties(Foo.prototype, {prop: {value: 5}});", "");
} }


// TODO(b/66971163): implement removal of defineProperties() when object is unused public void testObjectDefinePropertiesOnPrototype2() {
public void disabledTestObjectDefinePropertiesOnPrototype2() {
test( test(
lines( lines(
"var b = 5;", "var b = 5;",
Expand All @@ -2098,36 +2097,32 @@ public void disabledTestObjectDefinePropertiesOnPrototype2() {
"var b = 5; use(b);"); "var b = 5; use(b);");
} }


// TODO(b/66971163): implement removal of defineProperties() when object is unused public void testObjectDefinePropertiesOnPrototype3() {
public void disabledTestObjectDefinePropertiesOnPrototype3() {
test( test(
lines( lines(
"var b = function() {};", "var b = function() {};",
"function Foo() {}", "function Foo() {}",
"Object.defineProperties(Foo.prototype, {prop: {value: b()}});"), "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 testObjectDefineGetters() {
public void disabledTestObjectDefineGetters() {
test("function Foo() {} Object.defineProperties(Foo, {prop: {get: function() {}}});", ""); test("function Foo() {} Object.defineProperties(Foo, {prop: {get: function() {}}});", "");


test( test(
"function Foo() {} Object.defineProperties(Foo.prototype, {prop: {get: function() {}}});", "function Foo() {} Object.defineProperties(Foo.prototype, {prop: {get: function() {}}});",
""); "");
} }


// TODO(b/66971163): implement removal of defineProperties() when object is unused public void testObjectDefineSetters() {
public void disabledTestObjectDefineSetters() {
test("function Foo() {} Object.defineProperties(Foo, {prop: {set: function() {}}});", ""); test("function Foo() {} Object.defineProperties(Foo, {prop: {set: function() {}}});", "");


test( test(
"function Foo() {} Object.defineProperties(Foo.prototype, {prop: {set: function() {}}});", "function Foo() {} Object.defineProperties(Foo.prototype, {prop: {set: function() {}}});",
""); "");
} }


// TODO(b/66971163): implement removal of defineProperties() when object is unused public void testObjectDefineSetters_global() {
public void disabledTestObjectDefineSetters_global() {
test( test(
lines( lines(
"function Foo() {} ", "function Foo() {} ",
Expand Down

0 comments on commit d04416a

Please sign in to comment.