Skip to content

Commit

Permalink
Fixes a bug where dead property assignment elimination would remove w…
Browse files Browse the repository at this point in the history 
…rites that

were used in getters/setters.

There is now a pre-order collector to find any property names that are assigned
to a getter/setter. The later traversal will then treat any reference to
properties with this name as if they were a call (assumes all properties are
read).
-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=123247955
  • Loading branch information
@kevinoconnor7 @blickly
kevinoconnor7 authored and blickly committed May 25, 2016
1 parent cf37595 commit 9889afb
Show file tree
Hide file tree
Showing 4 changed files with 332 additions and 32 deletions.
144 changes: 112 additions & 32 deletions src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java
View file
Expand Up @@ -45,13 +45,15 @@
* are processed.</li>
* <li>Hook nodes are not processed (it's assumed they read everything)</li>
* <li>Switch blocks are not processed (it's assumed they read everything)</li>
* <li>Any reference to a property getter/setter is treated like a call that escapes all props.</li>
* </ul>
*/
public class DeadPropertyAssignmentElimination extends AbstractPostOrderCallback
implements CompilerPass {
public class DeadPropertyAssignmentElimination implements CompilerPass {

private final AbstractCompiler compiler;

// TODO(kevinoconnor): Try to give special treatment to constructor, else remove this field
// and cleanup dead code.
@VisibleForTesting
static final boolean ASSUME_CONSTRUCTORS_HAVENT_ESCAPED = false;

Expand All @@ -61,40 +63,59 @@ public class DeadPropertyAssignmentElimination extends AbstractPostOrderCallback

@Override
public void process(Node externs, Node root) {
NodeTraversal.traverseEs6(compiler, root, this);
GetterSetterCollector getterSetterCollector = new GetterSetterCollector();
NodeTraversal.traverseEs6(compiler, root, getterSetterCollector);
NodeTraversal.traverseEs6(compiler, root,
new FunctionVisitor(compiler, getterSetterCollector.propNames));
}

@Override
public void visit(NodeTraversal t, Node n, Node parent) {
if (!n.isFunction()) {
return;
}
private static class FunctionVisitor extends AbstractPostOrderCallback {

private final AbstractCompiler compiler;

/**
* A set of properties names that are known to be assigned to getter/setters. This is important
* since any reference to these properties needs to be treated as if it were a call.
*/
private final Set<String> getterSetterNames;

Node body = NodeUtil.getFunctionBody(n);
if (!body.hasChildren() || NodeUtil.containsFunction(body)) {
return;
FunctionVisitor(AbstractCompiler compiler, Set<String> getterSetterNames) {
this.compiler = compiler;
this.getterSetterNames = getterSetterNames;
}

FindCandidateAssignmentTraversal traversal =
new FindCandidateAssignmentTraversal(NodeUtil.isConstructor(n));
NodeTraversal.traverseEs6(compiler, body, traversal);
@Override
public void visit(NodeTraversal t, Node n, Node parent) {
if (!n.isFunction()) {
return;
}

// Any candidate property assignment can have a write removed if that write is never read
// and it's written to at least one more time.
for (Property property : traversal.propertyMap.values()) {
if (property.writes.size() <= 1) {
continue;
Node body = NodeUtil.getFunctionBody(n);
if (!body.hasChildren() || NodeUtil.containsFunction(body)) {
return;
}
PeekingIterator<PropertyWrite> iter = Iterators.peekingIterator(property.writes.iterator());
while (iter.hasNext()) {
PropertyWrite propertyWrite = iter.next();
if (iter.hasNext() && propertyWrite.isSafeToRemove(iter.peek())) {
Node lhs = propertyWrite.assignedAt;
Node rhs = lhs.getNext();
Node assignNode = lhs.getParent();
rhs.detachFromParent();
assignNode.getParent().replaceChild(assignNode, rhs);
compiler.reportCodeChange();

FindCandidateAssignmentTraversal traversal =
new FindCandidateAssignmentTraversal(getterSetterNames, NodeUtil.isConstructor(n));
NodeTraversal.traverseEs6(compiler, body, traversal);

// Any candidate property assignment can have a write removed if that write is never read
// and it's written to at least one more time.
for (Property property : traversal.propertyMap.values()) {
if (property.writes.size() <= 1) {
continue;
}
PeekingIterator<PropertyWrite> iter = Iterators.peekingIterator(property.writes.iterator());
while (iter.hasNext()) {
PropertyWrite propertyWrite = iter.next();
if (iter.hasNext() && propertyWrite.isSafeToRemove(iter.peek())) {
Node lhs = propertyWrite.assignedAt;
Node rhs = lhs.getNext();
Node assignNode = lhs.getParent();
rhs.detachFromParent();
assignNode.getParent().replaceChild(assignNode, rhs);
compiler.reportCodeChange();
}
}
}
}
Expand Down Expand Up @@ -191,12 +212,19 @@ private static class FindCandidateAssignmentTraversal implements Callback {
*/
Map<String, Property> propertyMap = new HashMap<>();

/**
* A set of properties names that are known to be assigned to getter/setters. This is important
* since any reference to these properties needs to be treated as if it were a call.
*/
private final Set<String> getterSetterNames;

/**
* Whether or not the function being analyzed is a constructor.
*/
private final boolean isConstructor;

FindCandidateAssignmentTraversal(boolean isConstructor) {
FindCandidateAssignmentTraversal(Set<String> getterSetterNames, boolean isConstructor) {
this.getterSetterNames = getterSetterNames;
this.isConstructor = isConstructor;
}

Expand Down Expand Up @@ -322,6 +350,15 @@ private void visitAssignmentLhs(Node lhs) {
private boolean visitNode(Node n, Node parent) {
switch (n.getType()) {
case Token.GETPROP:
// Handle potential getters/setters.
if (n.isGetProp()
&& n.getLastChild().isString()
&& getterSetterNames.contains(n.getLastChild().getString())) {
// We treat getters/setters as if they were a call, thus we mark all properties as read.
markAllPropsRead();
return true;
}

if (NodeUtil.isAssignmentOp(parent) && parent.getFirstChild() == n) {
// We always visit the LHS assignment in post-order
return false;
Expand All @@ -330,7 +367,13 @@ private boolean visitNode(Node n, Node parent) {
if (property != null) {
// Mark all children properties as read.
property.markLastWriteRead();
property.markChildrenRead();

// Only mark children properties as read if we're at at the end of the referenced
// property chain.
// Ex. A read of "a.b.c" should mark a, a.b, a.b.c, and a.b.c.* as read, but not a.d
if (!parent.isGetProp()) {
property.markChildrenRead();
}
}
return true;
case Token.CALL:
Expand All @@ -347,7 +390,9 @@ private boolean visitNode(Node n, Node parent) {
case Token.NAME:
Property nameProp = Preconditions.checkNotNull(getOrCreateProperty(n));
nameProp.markLastWriteRead();
nameProp.markChildrenRead();
if (!parent.isGetProp()) {
nameProp.markChildrenRead();
}
return true;

case Token.THROW:
Expand Down Expand Up @@ -391,4 +436,39 @@ private void markAllPropsReadHelper(boolean excludeThisProps) {
}
}
}

/**
* A traversal to find all property names that are defined to have a getter and/or setter
* associated with them.
*/
private static class GetterSetterCollector extends AbstractPostOrderCallback {

/**
* A set of properties names that are known to be assigned to getter/setters. This is important
* since any reference to these properties needs to be treated as if it were a call.
*/
private final Set<String> propNames = new HashSet<>();

@Override
public void visit(NodeTraversal t, Node n, Node parent) {
// Keep track of any potential getters/setters.
if (NodeUtil.isGetterOrSetter(n)) {
Node grandparent = parent.getParent();
if (NodeUtil.isGetOrSetKey(n) && n.getString() != null) {
// ES5 getter/setter nodes contain the property name directly on the node.
propNames.add(n.getString());
} else if (NodeUtil.isObjectDefinePropertyDefinition(grandparent)) {
// Handle Object.defineProperties(obj, 'propName', { ... }).
Node propNode = grandparent.getChildAtIndex(2);
if (propNode.isString()) {
propNames.add(propNode.getString());
}
} else if (grandparent.isStringKey()
&& NodeUtil.isObjectDefinePropertiesDefinition(grandparent.getParent().getParent())) {
// Handle Object.defineProperties(obj, {propName: { ... }}).
propNames.add(grandparent.getString());
}
}
}
}
}
20 changes: 20 additions & 0 deletions src/com/google/javascript/jscomp/NodeUtil.java
View file
Expand Up @@ -3388,6 +3388,15 @@ static boolean isObjectDefinePropertiesDefinition(Node n) {
&& n.getFirstChild().matchesQualifiedName("Object.defineProperties");
}

/**
* @return {@code true} if the node is a definition with Object.defineProperty
*/
static boolean isObjectDefinePropertyDefinition(Node n) {
return n.isCall()
&& n.getChildCount() == 4
&& n.getFirstChild().matchesQualifiedName("Object.defineProperty");
}

/**
* @return A list of STRING_KEY properties defined by a Object.defineProperties(o, {...}) call
*/
Expand Down Expand Up @@ -4511,4 +4520,15 @@ private static boolean isEs6Constructor(Node fnNode) {
&& fnNode.getParent().matchesQualifiedName("constructor");
}

static boolean isGetterOrSetter(Node propNode) {
if (isGetOrSetKey(propNode)) {
return true;
}
if (!propNode.isStringKey() || !propNode.getFirstChild().isFunction()) {
return false;
}
String keyName = propNode.getString();
return keyName.equals("get") || keyName.equals("set");
}

}
134 changes: 134 additions & 0 deletions test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java
View file
Expand Up @@ -710,6 +710,140 @@ public void testEs6Constrcutor() {
setAcceptedLanguage(LanguageMode.ECMASCRIPT5);
}

public void testGetter() {
testSame(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; };",
"Object.defineProperties(Foo.prototype, {bar: {",
" get: function () { return this.enabled ? 'enabled' : 'disabled'; }",
"}});",
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
" var f = foo.bar;",
" foo.enabled = false;",
"}"));

testSame(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; };",
"Object.defineProperty(Foo, 'bar', {",
" get: function () { return this.enabled ? 'enabled' : 'disabled'; }",
"});",
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
" var f = foo.bar;",
" foo.enabled = false;",
"}"));
}

public void testGetter_afterDeadAssignment() {
testSame(
LINE_JOINER.join(
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
" var f = foo.bar;",
" foo.enabled = false;",
"}",
"/** @constructor */ function Foo() { this.enabled = false; };",
"Object.defineProperties(Foo.prototype, {bar: {",
" get: function () { return this.enabled ? 'enabled' : 'disabled'; }",
"}});"));
}

public void testGetter_onDifferentType() {
testSame(
LINE_JOINER.join(
"/** @constructor */",
"function Foo() {",
" this.enabled = false;",
"};",
"Object.defineProperties(",
" Foo.prototype, {",
" baz: {",
" get: function () { return this.enabled ? 'enabled' : 'disabled'; }",
" }",
" });",
"/** @constructor */",
"function Bar() {",
" this.enabled = false;",
" this.baz = 123;",
"};",
"function f() {",
" var bar = new Bar();",
" bar.enabled = true;",
" var ret = bar.baz;",
" bar.enabled = false;",
" return ret;",
"};")
);
}

public void testSetter() {
testSame(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
"Object.defineProperties(Foo.prototype, {bar: {",
" set: function (x) { this.x = this.enabled ? x * 2 : x; }",
"}});",
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
" foo.bar = 10;",
" foo.enabled = false;",
"}"));

testSame(
LINE_JOINER.join(
"/** @constructor */ function Foo() { this.enabled = false; this.x = null; };",
"Object.defineProperty(Foo, 'bar', {",
" set: function (x) { this.x = this.enabled ? x * 2 : x; }",
"});",
"function f() {",
" var foo = new Foo()",
" foo.enabled = true;",
" foo.bar = 10;",
" foo.enabled = false;",
"}"));
}

public void testEs5Getter() {
testSame(
LINE_JOINER.join(
"var bar = {",
" enabled: false,",
" get baz() {",
" return this.enabled ? 'enabled' : 'disabled';",
" }",
"};",
"function f() {",
" bar.enabled = true;",
" var ret = bar.baz;",
" bar.enabled = false;",
" return ret;",
"};")
);
}

public void testEs5Setter() {
testSame(
LINE_JOINER.join(
"var bar = {",
" enabled: false,",
" set baz(x) {",
" this.x = this.enabled ? x * 2 : x;",
" }",
"};",
"function f() {",
" bar.enabled = true;",
" bar.baz = 10;",
" bar.enabled = false;",
"};")
);
}


@Override
protected CompilerPass getProcessor(Compiler compiler) {
Expand Down

0 comments on commit 9889afb

Please sign in to comment.