Skip to content

Commit

Permalink
RemoveUnusedCode: handle properties defined with Object.definePropert…
Browse files Browse the repository at this point in the history 
…ies()

Implement removal of unused properties defined with Object.defineProperties().
This is only enabled in tests for now, but will soon replace the implementation
in RemoveUnusedClassProperties.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=180687905
  • Loading branch information
@brad4d @blickly
brad4d authored and blickly committed Jan 3, 2018
1 parent 49281c1 commit 48fa68e
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 15 deletions.
96 changes: 96 additions & 0 deletions src/com/google/javascript/jscomp/RemoveUnusedCode.java
View file
Expand Up @@ -132,6 +132,7 @@ class RemoveUnusedCode implements CompilerPass {
private final boolean allowRemovalOfExternProperties; private final boolean allowRemovalOfExternProperties;
private final boolean removeUnusedThisProperties; private final boolean removeUnusedThisProperties;
private final boolean removeUnusedConstructorProperties; private final boolean removeUnusedConstructorProperties;
private final boolean removeUnusedObjectDefinePropertiesDefinitions;


RemoveUnusedCode(Builder builder) { RemoveUnusedCode(Builder builder) {
this.compiler = builder.compiler; this.compiler = builder.compiler;
Expand All @@ -143,6 +144,8 @@ class RemoveUnusedCode implements CompilerPass {
this.allowRemovalOfExternProperties = builder.allowRemovalOfExternProperties; this.allowRemovalOfExternProperties = builder.allowRemovalOfExternProperties;
this.removeUnusedThisProperties = builder.removeUnusedThisProperties; this.removeUnusedThisProperties = builder.removeUnusedThisProperties;
this.removeUnusedConstructorProperties = builder.removeUnusedConstructorProperties; this.removeUnusedConstructorProperties = builder.removeUnusedConstructorProperties;
this.removeUnusedObjectDefinePropertiesDefinitions =
builder.removeUnusedObjectDefinePropertiesDefinitions;
this.scopeCreator = new Es6SyntacticScopeCreator(builder.compiler); this.scopeCreator = new Es6SyntacticScopeCreator(builder.compiler);


// All Vars that are completely unremovable will share this VarInfo instance. // All Vars that are completely unremovable will share this VarInfo instance.
Expand All @@ -160,6 +163,7 @@ public static class Builder {
private boolean allowRemovalOfExternProperties = false; private boolean allowRemovalOfExternProperties = false;
private boolean removeUnusedThisProperties = false; private boolean removeUnusedThisProperties = false;
private boolean removeUnusedConstructorProperties = false; private boolean removeUnusedConstructorProperties = false;
private boolean removeUnusedObjectDefinePropertiesDefinitions = false;


Builder(AbstractCompiler compiler) { Builder(AbstractCompiler compiler) {
this.compiler = compiler; this.compiler = compiler;
Expand Down Expand Up @@ -200,6 +204,11 @@ Builder removeUnusedConstructorProperties(boolean value) {
return this; return this;
} }


Builder removeUnusedObjectDefinePropertiesDefinitions(boolean value) {
this.removeUnusedObjectDefinePropertiesDefinitions = value;
return this;
}

RemoveUnusedCode build() { RemoveUnusedCode build() {
return new RemoveUnusedCode(this); return new RemoveUnusedCode(this);
} }
Expand Down Expand Up @@ -511,6 +520,9 @@ private void traverseCall(Node callNode, Scope scope) {
markPropertyNameReferenced(propertyNameNode.getString()); markPropertyNameReferenced(propertyNameNode.getString());
} }
traverseChildren(callNode, scope); traverseChildren(callNode, scope);
} else if (NodeUtil.isObjectDefinePropertiesDefinition(callNode)) {
// TODO(bradfordcsmith): Should also handle Object.create() and Object.defineProperty().
traverseObjectDefinePropertiesCall(callNode, scope);
} else { } else {
Node parent = callNode.getParent(); Node parent = callNode.getParent();
String classVarName = null; String classVarName = null;
Expand Down Expand Up @@ -556,6 +568,54 @@ private void traverseCall(Node callNode, Scope scope) {
} }
} }


/** Traverse `Object.defineProperties(someObject, propertyDefinitions);`. */
private void traverseObjectDefinePropertiesCall(Node callNode, Scope scope) {
// First child is Object.defineProperties or some equivalent of it.
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()) {
traverseObjectDefinePropertiesLiteral(propertyDefinitions, scope);
} else {
traverseNode(propertyDefinitions, scope);
}
}

/** Traverse the object literal passed as the second argument to `Object.defineProperties()`. */
private void traverseObjectDefinePropertiesLiteral(Node propertyDefinitions, Scope scope) {
for (Node property = propertyDefinitions.getFirstChild();
property != null;
property = property.getNext()) {
if (property.isQuotedString()) {
// Quoted property name counts as a reference to the property and protects it from removal.
markPropertyNameReferenced(property.getString());
traverseNode(property.getOnlyChild(), scope);
} else if (property.isStringKey()) {
Node definition = property.getOnlyChild();
if (NodeUtil.mayHaveSideEffects(definition)) {
traverseNode(definition, scope);
} else {
considerForIndependentRemoval(
new RemovableBuilder()
.addContinuation(new Continuation(definition, scope))
.buildObjectDefinePropertiesDefinition(property));
}
} else {
// TODO(bradfordcsmith): Maybe report error for anything other than a computed property,
// since getters, setters, and methods don't make much sense in this context.
traverseNode(property, scope);
}
}
}

private void traverseRest(Node restNode, Scope scope) { private void traverseRest(Node restNode, Scope scope) {
Node target = restNode.getOnlyChild(); Node target = restNode.getOnlyChild();
if (!target.isName()) { if (!target.isName()) {
Expand Down Expand Up @@ -1159,6 +1219,10 @@ private void considerForIndependentRemoval(Removable removable) {
|| codingConvention.isExported(propertyName)) { || codingConvention.isExported(propertyName)) {
// Referenced, so not removable. // Referenced, so not removable.
removable.applyContinuations(); removable.applyContinuations();
} else if (removeUnusedObjectDefinePropertiesDefinitions
&& removable.isObjectDefinePropertiesDefinition()) {
// Store for possible removal later.
removablesForPropertyNames.put(propertyName, removable);
} else if (removeUnusedPrototypeProperties && removable.isPrototypeProperty()) { } else if (removeUnusedPrototypeProperties && removable.isPrototypeProperty()) {
// Store for possible removal later. // Store for possible removal later.
removablesForPropertyNames.put(propertyName, removable); removablesForPropertyNames.put(propertyName, removable);
Expand Down Expand Up @@ -1494,6 +1558,10 @@ boolean isPrototypeProperty() {
boolean isThisDotPropertyReference() { boolean isThisDotPropertyReference() {
return isThisDotPropertyReference; return isThisDotPropertyReference;
} }

public boolean isObjectDefinePropertiesDefinition() {
return false;
}
} }


private class RemovableBuilder { private class RemovableBuilder {
Expand Down Expand Up @@ -1546,6 +1614,11 @@ ClassOrPrototypeNamedProperty buildClassOrPrototypeNamedProperty(Node propertyNo
return new ClassOrPrototypeNamedProperty(this, propertyNode); return new ClassOrPrototypeNamedProperty(this, propertyNode);
} }


ObjectDefinePropertiesDefinition buildObjectDefinePropertiesDefinition(Node propertyNode) {
this.propertyName = propertyNode.getString();
return new ObjectDefinePropertiesDefinition(this, propertyNode);
}

FunctionDeclaration buildFunctionDeclaration(Node functionNode) { FunctionDeclaration buildFunctionDeclaration(Node functionNode) {
return new FunctionDeclaration(this, functionNode); return new FunctionDeclaration(this, functionNode);
} }
Expand Down Expand Up @@ -1820,6 +1893,29 @@ void removeInternal(AbstractCompiler compiler) {
} }
} }


/**
* Represents a single property definition in the object literal passed as the second argument to
* e.g. `Object.defineProperties(obj, {p1: {value: 1}, p2: {value: 3}});`.
*/
private class ObjectDefinePropertiesDefinition extends Removable {
final Node propertyNode;

ObjectDefinePropertiesDefinition(RemovableBuilder builder, Node propertyNode) {
super(builder);
this.propertyNode = propertyNode;
}

@Override
public boolean isObjectDefinePropertiesDefinition() {
return true;
}

@Override
void removeInternal(AbstractCompiler compiler) {
NodeUtil.deleteNode(propertyNode, compiler);
}
}

private class FunctionDeclaration extends Removable { private class FunctionDeclaration extends Removable {
final Node functionDeclarationNode; final Node functionDeclarationNode;


Expand Down
37 changes: 22 additions & 15 deletions test/com/google/javascript/jscomp/RemoveUnusedCodeClassPropertiesTest.java
View file
Expand Up @@ -87,6 +87,7 @@ protected CompilerPass getProcessor(Compiler compiler) {
.removeUnusedPrototypeProperties(true) .removeUnusedPrototypeProperties(true)
.removeUnusedThisProperties(true) .removeUnusedThisProperties(true)
.removeUnusedConstructorProperties(true) .removeUnusedConstructorProperties(true)
.removeUnusedObjectDefinePropertiesDefinitions(true)
.build(); .build();
} }


Expand Down Expand Up @@ -296,8 +297,7 @@ public void testObjectDefineProperties1() {
"foo(C)")); "foo(C)"));
} }


// TODO(b/66971163): make this pass public void testObjectDefineProperties2() {
public void disabledTestObjectDefineProperties2() {
this.mode = TypeInferenceMode.BOTH; this.mode = TypeInferenceMode.BOTH;


test( test(
Expand All @@ -309,8 +309,7 @@ public void disabledTestObjectDefineProperties2() {
"Object.defineProperties(C, {});")); "Object.defineProperties(C, {});"));
} }


// TODO(b/66971163): make this pass public void testObjectDefineProperties3() {
public void disabledTestObjectDefineProperties3() {
this.mode = TypeInferenceMode.BOTH; this.mode = TypeInferenceMode.BOTH;


test( test(
Expand All @@ -326,13 +325,16 @@ public void disabledTestObjectDefineProperties3() {
"Object.defineProperties(C, {});")); "Object.defineProperties(C, {});"));
} }


// side-effect in definition retains property // side-effect in definition retains property definition, but doesn't count as a reference
public void testObjectDefineProperties4() { public void testObjectDefineProperties4() {
this.mode = TypeInferenceMode.BOTH; this.mode = TypeInferenceMode.BOTH;


testSame( test(
lines( lines(
"/** @constructor */ function C() {}", "/** @constructor */ function C() { this.prop = 3; }",
"Object.defineProperties(C, {prop:alert('')});"),
lines(
"/** @constructor */ function C() { }",
"Object.defineProperties(C, {prop:alert('')});")); "Object.defineProperties(C, {prop:alert('')});"));
} }


Expand All @@ -346,18 +348,16 @@ public void testObjectDefineProperties5() {
"Object.defineProperties(C, {'prop': {value: 1}});")); "Object.defineProperties(C, {'prop': {value: 1}});"));
} }


// TODO(b/66971163): make this pass public void testObjectDefineProperties6() {
public void disabledTestObjectDefineProperties6() {
this.mode = TypeInferenceMode.BOTH; this.mode = TypeInferenceMode.BOTH;


// an unknown destination object doesn't prevent removal. // an unknown destination object doesn't prevent removal.
test( test(
"Object.defineProperties(externVar(), {prop:{value:1}});", "Object.defineProperties(externVar(), {prop:{value:1}});",
"Object.defineProperties(externVar(), {});"); "Object.defineProperties(externVar(), { });");
} }


// TODO(b/66971163): make this pass public void testObjectDefineProperties7() {
public void disabledTestObjectDefineProperties7() {
this.mode = TypeInferenceMode.BOTH; this.mode = TypeInferenceMode.BOTH;


test( test(
Expand All @@ -369,8 +369,7 @@ public void disabledTestObjectDefineProperties7() {
"Object.defineProperties(C, {});")); "Object.defineProperties(C, {});"));
} }


// TODO(b/66971163): make this pass public void testObjectDefineProperties8() {
public void disabledTestObjectDefineProperties8() {
this.mode = TypeInferenceMode.BOTH; this.mode = TypeInferenceMode.BOTH;


test( test(
Expand All @@ -382,6 +381,15 @@ public void disabledTestObjectDefineProperties8() {
"Object.defineProperties(C, {});")); "Object.defineProperties(C, {});"));
} }


public void testObjectDefinePropertiesQuotesPreventRemoval() {
this.mode = TypeInferenceMode.BOTH;

testSame(
lines(
"/** @constructor */ function C() { this.prop = 1; }",
"Object.defineProperties(C, {'prop':{set:function (a) {return alert(a.prop)}}});"));
}

// TODO(b/66971163): make this pass // TODO(b/66971163): make this pass
public void disabledTestObjectDefineProperties_used_setter_removed() { public void disabledTestObjectDefineProperties_used_setter_removed() {
// TODO: Either remove, fix this, or document it as a limitation of advanced mode optimizations. // TODO: Either remove, fix this, or document it as a limitation of advanced mode optimizations.
Expand All @@ -397,7 +405,6 @@ public void disabledTestObjectDefineProperties_used_setter_removed() {
"Object.defineProperties(C, {});2")); "Object.defineProperties(C, {});2"));
} }


// TODO(b/66971163): make this pass
public void testEs6GettersWithoutTranspilation() { public void testEs6GettersWithoutTranspilation() {
test( test(
"class C { get value() { return 0; } }", // preserve newline "class C { get value() { return 0; } }", // preserve newline
Expand Down
8 changes: 8 additions & 0 deletions test/com/google/javascript/jscomp/RemoveUnusedCodePrototypePropertiesTest.java
View file
Expand Up @@ -77,6 +77,14 @@ protected void setUp() throws Exception {
allowRemovalOfExternProperties = false; allowRemovalOfExternProperties = false;
} }


public void testClassPropertiesNotRemoved() {
keepGlobals = true;
// This whole test class runs with removeUnusedClassProperties disabled.
testSame("/** @constructor */ function C() {} C.unused = 3;");
testSame(
"/** @constructor */ function C() {} Object.defineProperties(C, {unused: {value: 3}});");
}

public void testUnusedPrototypeFieldReference() { public void testUnusedPrototypeFieldReference() {
test( test(
"function C() {} C.prototype.x; new C();", // x is not actually read "function C() {} C.prototype.x; new C();", // x is not actually read
Expand Down

0 comments on commit 48fa68e

Please sign in to comment.