Skip to content

Commit

Permalink
Consider side-effects of getters and setters when tracking property r…
Browse files Browse the repository at this point in the history 
…ead/writes in `RemoveUnusedCode`.

Previously we could assume that if a property was never read it never needed to be written to. The possibility of a setter being assigned to undermines this assumption.

Additionally, assumptions were being made about when parts of expressions were implicitly safe to remove. The possibility that getters could have side-effects changes this.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=258450431
  • Loading branch information
@nreid260 @tjgq
nreid260 authored and tjgq committed Jul 17, 2019
1 parent c86e148 commit 1ba6e19
Show file tree
Hide file tree
Showing 4 changed files with 422 additions and 51 deletions.
1 change: 1 addition & 0 deletions src/com/google/javascript/jscomp/DefaultPassConfig.java
View file
Expand Up @@ -2704,6 +2704,7 @@ protected CompilerPass create(AbstractCompiler compiler) {
.removeUnusedObjectDefinePropertiesDefinitions(options.isRemoveUnusedClassProperties())
.removeUnusedConstructorProperties(options.isRemoveUnusedConstructorProperties())
.removeUnusedPolyfills(options.rewritePolyfills)
.assumeGettersArePure(options.getAssumeGettersArePure())
.build();
}

Expand Down
134 changes: 92 additions & 42 deletions src/com/google/javascript/jscomp/RemoveUnusedCode.java
View file
Expand Up @@ -24,6 +24,7 @@
import com.google.common.collect.HashMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Multimap;
import com.google.javascript.jscomp.AccessorSummary.PropertyAccessKind;
import com.google.javascript.jscomp.CodingConvention.SubclassRelationship;
import com.google.javascript.rhino.IR;
import com.google.javascript.rhino.Node;
Expand Down Expand Up @@ -119,7 +120,7 @@ class RemoveUnusedCode implements CompilerPass {

private final Map<Var, VarInfo> varInfoMap = new HashMap<>();

private final Set<String> referencedPropertyNames = new HashSet<>(IMPLICITLY_USED_PROPERTIES);
private final Set<String> pinnedPropertyNames = new HashSet<>(IMPLICITLY_USED_PROPERTIES);

/** Stores Removable objects for each property name that is currently considered removable. */
private final Multimap<String, Removable> removablesForPropertyNames = HashMultimap.create();
Expand Down Expand Up @@ -148,11 +149,14 @@ class RemoveUnusedCode implements CompilerPass {
private final boolean removeUnusedStaticProperties;
private final boolean removeUnusedObjectDefinePropertiesDefinitions;
private final boolean removeUnusedPolyfills;
private final boolean assumeGettersArePure;

RemoveUnusedCode(Builder builder) {
this.compiler = builder.compiler;
this.astAnalyzer = compiler.getAstAnalyzer();
this.codingConvention = builder.compiler.getCodingConvention();
this.scopeCreator = new SyntacticScopeCreator(builder.compiler);

this.removeLocalVars = builder.removeLocalVars;
this.removeGlobals = builder.removeGlobals;
this.preserveFunctionExpressionNames = builder.preserveFunctionExpressionNames;
Expand All @@ -163,7 +167,7 @@ class RemoveUnusedCode implements CompilerPass {
this.removeUnusedObjectDefinePropertiesDefinitions =
builder.removeUnusedObjectDefinePropertiesDefinitions;
this.removeUnusedPolyfills = builder.removeUnusedPolyfills;
this.scopeCreator = new SyntacticScopeCreator(builder.compiler);
this.assumeGettersArePure = builder.assumeGettersArePure;

// All Vars that are completely unremovable will share this VarInfo instance.
canonicalUnremovableVarInfo = new VarInfo();
Expand All @@ -182,6 +186,7 @@ public static class Builder {
private boolean removeUnusedStaticProperties = false;
private boolean removeUnusedObjectDefinePropertiesDefinitions = false;
private boolean removeUnusedPolyfills = false;
private boolean assumeGettersArePure = false;

Builder(AbstractCompiler compiler) {
this.compiler = compiler;
Expand Down Expand Up @@ -232,6 +237,11 @@ Builder removeUnusedPolyfills(boolean value) {
return this;
}

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

RemoveUnusedCode build() {
return new RemoveUnusedCode(this);
}
Expand All @@ -245,7 +255,7 @@ RemoveUnusedCode build() {
public void process(Node externs, Node root) {
checkState(compiler.getLifeCycleStage().isNormalized());
if (!allowRemovalOfExternProperties) {
referencedPropertyNames.addAll(compiler.getExternProperties());
pinnedPropertyNames.addAll(compiler.getExternProperties());
}
traverseAndRemoveUnusedReferences(root);
// This pass may remove definitions of getter or setter properties.
Expand Down Expand Up @@ -458,9 +468,10 @@ private void traverseGetProp(Node getProp, Scope scope) {
}
}

if (NodeUtil.isExpressionResultUsed(getProp)) {
if (NodeUtil.isExpressionResultUsed(getProp)
|| considerForAccessorSideEffects(getProp, PropertyAccessKind.GETTER_ONLY)) {
// must record as reference to the property and continue traversal.
markPropertyNameReferenced(propertyName);
markPropertyNameAsPinned(propertyName);
traverseNode(objectNode, scope);
} else if (objectNode.isThis()) {
// this.propName;
Expand All @@ -485,11 +496,12 @@ private void traverseGetProp(Node getProp, Scope scope) {
}
} else {
// TODO(bradfordcsmith): add removal of `varName.propName;`
markPropertyNameReferenced(propertyName);
markPropertyNameAsPinned(propertyName);
traverseNode(objectNode, scope);
}
}

// TODO(b/137380742): Combine with `traverseCompoundAssign`.
private void traverseIncrementOrDecrementOp(Node incOrDecOp, Scope scope) {
checkArgument(incOrDecOp.isInc() || incOrDecOp.isDec(), incOrDecOp);
Node arg = incOrDecOp.getOnlyChild();
Expand All @@ -499,26 +511,32 @@ private void traverseIncrementOrDecrementOp(Node incOrDecOp, Scope scope) {
} else if (arg.isGetProp()) {
Node getPropObj = arg.getFirstChild();
Node propertyNameNode = arg.getLastChild();
if (getPropObj.isThis()) {

if (considerForAccessorSideEffects(arg, PropertyAccessKind.GETTER_AND_SETTER)) {
traverseNode(getPropObj, scope); // Don't re-traverse the GETPROP as a read.
} else if (getPropObj.isThis()) {
// this.propName++
RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true);
considerForIndependentRemoval(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode));
considerForIndependentRemoval(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode, null));
} else if (isDotPrototype(getPropObj)) {
// someExpression.prototype.propName++
Node exprObj = getPropObj.getFirstChild();
RemovableBuilder builder = new RemovableBuilder().setIsPrototypeDotPropertyReference(true);
if (exprObj.isName()) {
// varName.prototype.propName++
VarInfo varInfo = traverseNameNode(exprObj, scope);
varInfo.addRemovable(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode));
varInfo.addRemovable(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode, null));
} else {
// (someExpression).prototype.propName++
Node toPreserve = null;
if (astAnalyzer.mayHaveSideEffects(exprObj)) {
toPreserve = exprObj;
traverseNode(exprObj, scope);
} else {
builder.addContinuation(new Continuation(exprObj, scope));
}
considerForIndependentRemoval(builder.buildIncOrDepOp(incOrDecOp, propertyNameNode));
considerForIndependentRemoval(
builder.buildIncOrDepOp(incOrDecOp, propertyNameNode, toPreserve));
}
} else {
// someExpression.propName++ is not removable except in the cases covered above
Expand All @@ -530,19 +548,28 @@ private void traverseIncrementOrDecrementOp(Node incOrDecOp, Scope scope) {
}
}

// TODO(b/137380742): Combine with `traverseIncrementOrDecrement`.
private void traverseCompoundAssign(Node compoundAssignNode, Scope scope) {
// We'll allow removal of compound assignment to a this property as long as the result of the
// We'll allow removal of compound assignment to a `this` property as long as the result of the
// assignment is unused.
// e.g. `this.x += 3;`
// TODO(nickreid): Why do we treat `this` properties specially? It it because `this` is const?
Node targetNode = compoundAssignNode.getFirstChild();
Node valueNode = compoundAssignNode.getLastChild();
if (targetNode.isGetProp()
&& targetNode.getFirstChild().isThis()
&& !NodeUtil.isExpressionResultUsed(compoundAssignNode)) {
if (targetNode.isGetProp()) {
if (considerForAccessorSideEffects(targetNode, PropertyAccessKind.GETTER_AND_SETTER)) {
traverseNode(targetNode.getFirstChild(), scope); // Don't re-traverse the GETPROP as a read.
traverseNode(valueNode, scope);
} else if (targetNode.getFirstChild().isThis()
&& !NodeUtil.isExpressionResultUsed(compoundAssignNode)) {
RemovableBuilder builder = new RemovableBuilder().setIsThisDotPropertyReference(true);
traverseRemovableAssignValue(valueNode, builder, scope);
considerForIndependentRemoval(
builder.buildNamedPropertyAssign(compoundAssignNode, targetNode.getLastChild()));
} else {
traverseNode(targetNode, scope);
traverseNode(valueNode, scope);
}
} else {
traverseNode(targetNode, scope);
traverseNode(valueNode, scope);
Expand All @@ -566,7 +593,7 @@ private void traverseCall(Node callNode, Scope scope) {
&& codingConvention.isPropertyRenameFunction(callee.getOriginalQualifiedName())) {
Node propertyNameNode = checkNotNull(callee.getNext());
if (propertyNameNode.isString()) {
markPropertyNameReferenced(propertyNameNode.getString());
markPropertyNameAsPinned(propertyNameNode.getString());
}
traverseChildren(callNode, scope);
} else if (NodeUtil.isObjectDefinePropertiesDefinition(callNode)) {
Expand Down Expand Up @@ -680,7 +707,7 @@ private void traverseObjectDefinePropertiesLiteral(Node propertyDefinitions, Sco
property = property.getNext()) {
if (property.isQuotedString()) {
// Quoted property name counts as a reference to the property and protects it from removal.
markPropertyNameReferenced(property.getString());
markPropertyNameAsPinned(property.getString());
traverseNode(property.getOnlyChild(), scope);
} else if (property.isStringKey()) {
Node definition = property.getOnlyChild();
Expand Down Expand Up @@ -732,7 +759,7 @@ private void traverseNonPrototypeObjectLiteral(Node objectLiteral, Scope scope)
// because of some reflection patterns.
// Note that we are intentionally treating both quoted and unquoted keys as
// references.
markPropertyNameReferenced(propertyNode.getString());
markPropertyNameAsPinned(propertyNode.getString());
traverseNode(propertyNode.getFirstChild(), scope);
} else {
traverseNode(propertyNode, scope);
Expand Down Expand Up @@ -938,7 +965,13 @@ private void traverseAssign(Node assignNode, Scope scope) {
Node getPropLhs = lhs.getFirstChild();
Node propNameNode = lhs.getLastChild();

if (getPropLhs.isName()) {
if (considerForAccessorSideEffects(lhs, PropertyAccessKind.SETTER_ONLY)) {
// And the possible side-effects mean we can't do any removal. We don't use the
// `AstAnalyzer` because we only want to consider side-effect from the assignment, not the
// entire l-value subtree.
traverseNode(getPropLhs, scope); // Don't re-traverse the GETPROP as a read.
traverseNode(valueNode, scope);
} else if (getPropLhs.isName()) {
// varName.propertyName = someValue
VarInfo varInfo = traverseNameNode(getPropLhs, scope);
RemovableBuilder builder = new RemovableBuilder();
Expand Down Expand Up @@ -1006,7 +1039,7 @@ private void traverseObjectPattern(Node pattern, Scope scope) {

case STRING_KEY:
if (!elem.isQuotedString()) {
markPropertyNameReferenced(elem.getString());
markPropertyNameAsPinned(elem.getString());
}
traverseIndirectAssignment(elem, elem.getOnlyChild(), scope);
break;
Expand Down Expand Up @@ -1069,6 +1102,10 @@ private void traverseIndirectAssignment(Node root, Node target, Scope scope) {
target = target.getFirstChild();
}

if (target.isGetProp()) {
considerForAccessorSideEffects(target, PropertyAccessKind.SETTER_ONLY);
}

RemovableBuilder builder =
new RemovableBuilder().addContinuation(new Continuation(root, scope));

Expand Down Expand Up @@ -1275,8 +1312,8 @@ private void removeUnreferencedFunctionArgs(Scope fparamScope) {
markUnusedParameters(argList, fparamScope);
}

private void markPropertyNameReferenced(String propertyName) {
if (referencedPropertyNames.add(propertyName)) {
private void markPropertyNameAsPinned(String propertyName) {
if (pinnedPropertyNames.add(propertyName)) {
// Continue traversal of all of the property name's values and no longer consider them for
// removal.
for (Removable removable : removablesForPropertyNames.removeAll(propertyName)) {
Expand All @@ -1289,8 +1326,7 @@ private void considerForIndependentRemoval(Removable removable) {
if (removable.isNamedProperty()) {
String propertyName = removable.getPropertyName();

if (referencedPropertyNames.contains(propertyName)
|| codingConvention.isExported(propertyName)) {
if (pinnedPropertyNames.contains(propertyName) || codingConvention.isExported(propertyName)) {
// Referenced or exported, so not removable.
removable.applyContinuations();
} else if (isIndependentlyRemovable(removable)) {
Expand All @@ -1300,13 +1336,28 @@ private void considerForIndependentRemoval(Removable removable) {
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.
markPropertyNameReferenced(propertyName);
markPropertyNameAsPinned(propertyName);
}
} else {
removable.applyContinuations();
}
}

/** @return Whether or not accessor side-effect are a possibility. */
private boolean considerForAccessorSideEffects(Node getprop, PropertyAccessKind usage) {
checkState(getprop.isGetProp(), getprop); // Other node types may make sense in the future.

String propName = getprop.getSecondChild().getString();
PropertyAccessKind recorded = compiler.getAccessorSummary().getKind(propName);
if ((recorded.hasGetter() && usage.hasGetter() && !assumeGettersArePure)
|| (recorded.hasSetter() && usage.hasSetter())) {
markPropertyNameAsPinned(propName);
return true;
}

return false;
}

private boolean isIndependentlyRemovable(Removable removable) {
return (removeUnusedPrototypeProperties && removable.isPrototypeProperty())
|| (removeUnusedThisProperties && removable.isThisDotPropertyReference())
Expand Down Expand Up @@ -1754,9 +1805,9 @@ AnonymousPrototypeNamedPropertyAssign buildAnonymousPrototypeNamedPropertyAssign
return new AnonymousPrototypeNamedPropertyAssign(this, assignNode);
}

IncOrDecOp buildIncOrDepOp(Node incOrDecOp, Node propertyNode) {
IncOrDecOp buildIncOrDepOp(Node incOrDecOp, Node propertyNode, @Nullable Node toPreseve) {
this.propertyName = propertyNode.getString();
return new IncOrDecOp(this, incOrDecOp);
return new IncOrDecOp(this, incOrDecOp, toPreseve);
}

UnusedReadReference buildUnusedReadReference(Node referenceNode, Node propertyNode) {
Expand Down Expand Up @@ -1850,34 +1901,33 @@ public String toString() {
/** Represents an increment or decrement operation that could be removed. */
private class IncOrDecOp extends Removable {
final Node incOrDecNode;
@Nullable final Node toPreserve;

IncOrDecOp(RemovableBuilder builder, Node incOrDecNode) {
IncOrDecOp(RemovableBuilder builder, Node incOrDecNode, @Nullable Node toPreserve) {
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;
this.toPreserve = toPreserve;
}

@Override
void removeInternal(AbstractCompiler compiler) {
if (!alreadyRemoved(incOrDecNode)) {
Node arg = incOrDecNode.getOnlyChild();
checkState(arg.isGetProp(), arg);
if (alreadyRemoved(incOrDecNode)) {
return;
}

if (isThisDotProperty(arg)) {
removeExpressionCompletely(incOrDecNode);
} else {
checkState(isDotPrototypeDotProperty(arg), arg);
// objExpression.prototype.propertyName
Node objExpression = arg.getFirstFirstChild();
if (astAnalyzer.mayHaveSideEffects(objExpression)) {
replaceNodeWith(incOrDecNode, objExpression.detach());
} else {
removeExpressionCompletely(incOrDecNode);
}
}
Node arg = incOrDecNode.getOnlyChild();
checkState(arg.isGetProp(), arg);

if (this.toPreserve == null) {
removeExpressionCompletely(incOrDecNode);
} else {
replaceNodeWith(incOrDecNode, toPreserve.detach());
}
}

Expand Down

0 comments on commit 1ba6e19

Please sign in to comment.