diff --git a/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java b/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java index f22795416f4..7bc9bf136df 100644 --- a/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java +++ b/src/com/google/javascript/jscomp/DeadPropertyAssignmentElimination.java @@ -314,14 +314,14 @@ private void visitBlock(Node blockNode) { } private static boolean isConditionalExpression(Node n) { - switch (n.getType()) { + switch (n.getKind()) { case AND: case OR: case HOOK: return true; + default: + return false; } - - return false; } private void visitAssignmentLhs(Node lhs) { @@ -363,7 +363,7 @@ private void visitAssignmentLhs(Node lhs) { } private boolean visitNode(Node n, Node parent) { - switch (n.getType()) { + switch (n.getKind()) { case GETPROP: // Handle potential getters/setters. if (n.isGetProp() @@ -480,22 +480,22 @@ public boolean shouldTraverse(NodeTraversal nodeTraversal, Node n, Node parent) @Override public void visit(NodeTraversal t, Node n, Node parent) { if (NodeUtil.isObjectDefinePropertyDefinition(n)) { - // We must assume any property in the compilation can be a getter/setter if any of the - // params are potentially aliasing the real values. - if (!NodeUtil.isPrototypeProperty(n.getSecondChild()) - || !n.getChildAtIndex(2).isString() - || !n.getChildAtIndex(3).isObjectLit()) { - unknownGetterSetterPresent = true; - return; - } - } else if (NodeUtil.isObjectDefinePropertiesDefinition(n)) { - // If the first param is not a prototype reference or the second param is not an object - // literal then we must assume any property in the compilation can be a getter/setter. - if (!NodeUtil.isPrototypeProperty(n.getSecondChild()) - || !n.getChildAtIndex(2).isObjectLit()) { + // We must assume any property in the compilation can be a getter/setter if the property + // name and what is being assigned to are aliased. + if (!n.getChildAtIndex(2).isString() && !n.getLastChild().isObjectLit()) { unknownGetterSetterPresent = true; - return; + } else if (!n.getLastChild().isObjectLit()) { + // If know the property name but not what it's being assigned to then we need to blackist + // the property name. + propNames.add(n.getChildAtIndex(2).getString()); } + return; + } else if (NodeUtil.isObjectDefinePropertiesDefinition(n) + && !n.getChildAtIndex(2).isObjectLit()) { + // If the second param is not an object literal then we must assume any property in the + // compilation can be a getter/setter. + unknownGetterSetterPresent = true; + return; } // Keep track of any potential getters/setters. @@ -509,13 +509,39 @@ public void visit(NodeTraversal t, Node n, Node parent) { Node propNode = grandparent.getChildAtIndex(2); if (propNode.isString()) { propNames.add(propNode.getString()); + } else { + // Putting a getter/setter on an aliased property means any property can be a getter or + // setter. + unknownGetterSetterPresent = true; } } else if (grandparent.isStringKey() && NodeUtil.isObjectDefinePropertiesDefinition(grandparent.getParent().getParent())) { // Handle Object.defineProperties(obj, {propName: { ... }}). propNames.add(grandparent.getString()); } + } else if (isAliasedPropertySet(n)) { + // If we know this property is being injected but don't know if there's a getter/setter + // then the property still must be blacklisted. + propNames.add(n.getString()); } } + + /** + * Determines if the given keyNode contains an aliased property set. In particular this is only + * true if the grandparent is an {@code Object.defineProperties} call. + * + *

Ex. {@code Object.defineProperties(Foo.prototype, {bar: someObj}}. + */ + private static boolean isAliasedPropertySet(Node keyNode) { + if (keyNode == null || !keyNode.isStringKey() || keyNode.getParent() == null) { + return false; + } + + Node objectLit = keyNode.getParent(); + return objectLit.getParent() != null + && NodeUtil.isObjectDefinePropertiesDefinition(objectLit.getParent()) + && objectLit.getParent().getLastChild() == objectLit + && !keyNode.getFirstChild().isObjectLit(); + } } } diff --git a/test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java b/test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java index 5f547ef6d48..3e9b40e8161 100644 --- a/test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java +++ b/test/com/google/javascript/jscomp/DeadPropertyAssignmentEliminationTest.java @@ -849,7 +849,7 @@ public void testEs5Setter() { ); } - public void testObjectDefineProperty_aliasedVars() { + public void testObjectDefineProperty_aliasedParams() { testSame( LINE_JOINER.join( "function addGetter(obj, propName) {", @@ -891,7 +891,9 @@ public void testObjectDefineProperty_aliasedVars() { " x.bar = 10;", " x.bar = 20;", "}")); + } + public void testObjectDefineProperty_aliasedObject() { testSame( LINE_JOINER.join( "/** @constructor */ function Foo() { this.enabled = false; this.x = null; };", @@ -910,7 +912,9 @@ public void testObjectDefineProperty_aliasedVars() { " x.bar = 10;", " x.bar = 20;", "}")); + } + public void testObjectDefineProperty_aliasedPropName() { testSame( LINE_JOINER.join( "/** @constructor */ function Foo() { this.enabled = false; this.x = null; };", @@ -930,6 +934,44 @@ public void testObjectDefineProperty_aliasedVars() { " x.bar = 20;", "}")); + test( + LINE_JOINER.join( + "/** @constructor */ function Foo() { this.enabled = false; this.x = null; };", + "var x = 'bar';", + "Object.defineProperty(Foo.prototype, x, {", + " value: 10", + "});", + "function f() {", + " var foo = new Foo()", + " foo.enabled = true;", + " foo.bar = 10;", + " foo.enabled = false;", + "}", + "function z() {", + " var x = {};", + " x.bar = 10;", + " x.bar = 20;", + "}"), + LINE_JOINER.join( + "/** @constructor */ function Foo() { this.enabled = false; this.x = null; };", + "var x = 'bar';", + "Object.defineProperty(Foo.prototype, x, {", + " value: 10", + "});", + "function f() {", + " var foo = new Foo()", + " true;", + " foo.bar = 10;", + " foo.enabled = false;", + "}", + "function z() {", + " var x = {};", + " 10;", + " x.bar = 20;", + "}")); + } + + public void testObjectDefineProperty_aliasedPropSet() { testSame( LINE_JOINER.join( "/** @constructor */ function Foo() { this.enabled = false; this.x = null; };", @@ -950,17 +992,14 @@ public void testObjectDefineProperty_aliasedVars() { "}")); } - public void testObjectDefineProperties_aliasedVars() { + public void testObjectDefineProperties_aliasedPropertyMap() { testSame( LINE_JOINER.join( "/** @constructor */ function Foo() { this.enabled = false; this.x = null; };", "var properties = {bar: {", " set: function (x) { this.x = this.enabled ? x * 2 : x; }", "}};", - "var x = Foo.prototype;", - "Object.defineProperties(x, {bar: {", - " set: function (x) { this.x = this.enabled ? x * 2 : x; }", - "}});", + "Object.defineProperties(Foo.prototype, properties);", "function f() {", " var foo = new Foo()", " foo.enabled = true;", @@ -974,17 +1013,63 @@ public void testObjectDefineProperties_aliasedVars() { "}")); testSame( + LINE_JOINER.join( + "/** @constructor */ function Foo() { this.enabled = false; this.x = null; };", + "var properties = {", + " set: function (x) { this.x = this.enabled ? x * 2 : x; }", + "};", + "Object.defineProperties(Foo.prototype, {bar: properties});", + "function f() {", + " var foo = new Foo()", + " foo.enabled = true;", + " foo.bar = 10;", + " foo.enabled = false;", + "}", + "function z() {", + " var x = {};", + " x.bar = 10;", + " x.bar = 20;", + "}")); + } + + public void testObjectDefineProperties_aliasedObject() { + test( LINE_JOINER.join( "/** @constructor */ function Foo() { this.enabled = false; this.x = null; };", "var properties = {bar: {", " set: function (x) { this.x = this.enabled ? x * 2 : x; }", "}};", - "Object.defineProperties(Foo.prototype, properties);", + "var x = Foo.prototype;", + "Object.defineProperties(x, {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;", + " foo.enabled = true;", + "}", + "function z() {", + " var x = {};", + " x.bar = 10;", + " x.bar = 20;", + "}"), + LINE_JOINER.join( + "/** @constructor */ function Foo() { this.enabled = false; this.x = null; };", + "var properties = {bar: {", + " set: function (x) { this.x = this.enabled ? x * 2 : x; }", + "}};", + "var x = Foo.prototype;", + "Object.defineProperties(x, {bar: {", + " set: function (x) { this.x = this.enabled ? x * 2 : x; }", + "}});", + "function f() {", + " var foo = new Foo()", + " foo.enabled = true;", + " foo.bar = 10;", + " false;", + " foo.enabled = true;", "}", "function z() {", " var x = {};",