PeepholeCollectPropertyAssignments: Make sure we don't crash when pro…

…cessing objects with computed properties.

-------------
Created by MOE: https://github.com/google/moe
MOE_MIGRATED_REVID=186836038

src/com/google/javascript/jscomp/PeepholeCollectPropertyAssignments.java

@@ -104,7 +104,9 @@ static boolean isInterestingValue(Node n) {
   }
 
   private static boolean isPropertyAssignmentToName(Node propertyCandidate) {
-    if (propertyCandidate == null) { return false; }
+    if (propertyCandidate == null) {
+      return false;
+    }
     // Must be an assignment...
     if (!NodeUtil.isExprAssign(propertyCandidate)) {
       return false;
@@ -123,8 +125,7 @@ private static boolean isPropertyAssignmentToName(Node propertyCandidate) {
     return obj.isName();
   }
 
-  private boolean collectProperty(
-      Node propertyCandidate, String name, Node value) {
+  private boolean collectProperty(Node propertyCandidate, String name, Node value) {
     if (!isPropertyAssignmentToName(propertyCandidate)) {
       return false;
     }
@@ -165,8 +166,7 @@ && mightContainForwardReference(rhs, name)) {
   }
 
 
-  private static boolean collectArrayProperty(
-      Node arrayLiteral, Node propertyCandidate) {
+  private static boolean collectArrayProperty(Node arrayLiteral, Node propertyCandidate) {
     Node assignment = propertyCandidate.getFirstChild();
     final int sizeOfArrayAtStart = arrayLiteral.getChildCount();
     int maxIndexAssigned = sizeOfArrayAtStart - 1;
@@ -221,18 +221,15 @@ private static boolean collectArrayProperty(
     return true;
   }
 
-  private static boolean collectObjectProperty(
-      Node objectLiteral, Node propertyCandidate) {
+  private boolean collectObjectProperty(Node objectLiteral, Node propertyCandidate) {
     Node assignment = propertyCandidate.getFirstChild();
     Node lhs = assignment.getFirstChild();
     Node rhs = lhs.getNext();
     Node obj = lhs.getFirstChild();
     Node property = obj.getNext();
 
     // The property must be statically known.
-    if (lhs.isGetElem()
-        && (!property.isString()
-            && !property.isNumber())) {
+    if (lhs.isGetElem() && !property.isString() && !property.isNumber()) {
       return false;
     }
 
@@ -247,22 +244,29 @@ private static boolean collectObjectProperty(
     // Note: Duplicate keys are invalid in strict mode
     Node existingProperty = null;
     for (Node currentProperty : objectLiteral.children()) {
-      // Get the name of the current property
-      String currentPropertyName = currentProperty.getString();
-      // Get the value of the property
-      Node currentValue = currentProperty.getFirstChild();
-      // Compare the current property name with the new property name
-      if (currentPropertyName.equals(propertyName)) {
-        existingProperty = currentProperty;
-        // Check if the current value and the new value are side-effect
-        boolean isCurrentValueSideEffect = NodeUtil.canBeSideEffected(currentValue);
-        boolean isNewValueSideEffect = NodeUtil.canBeSideEffected(rhs);
-        // If they are side-effect free then replace the current value with the new one
-        if (isCurrentValueSideEffect || isNewValueSideEffect) {
+      if (currentProperty.isStringKey() || currentProperty.isMemberFunctionDef()) {
+        // Get the name of the current property
+        String currentPropertyName = currentProperty.getString();
+        // Get the value of the property
+        Node currentValue = currentProperty.getFirstChild();
+        // Compare the current property name with the new property name
+        if (currentPropertyName.equals(propertyName)) {
+          existingProperty = currentProperty;
+          // Check if the current value and the new value are side-effect
+          boolean isCurrentValueSideEffect = NodeUtil.canBeSideEffected(currentValue);
+          boolean isNewValueSideEffect = NodeUtil.canBeSideEffected(rhs);
+          // If they are side-effect free then replace the current value with the new one
+          if (isCurrentValueSideEffect || isNewValueSideEffect) {
+            return false;
+          }
+          // Break the loop if the property exists
+          break;
+        }
+      } else if (currentProperty.isGetterDef() || currentProperty.isSetterDef()) {
+        String currentPropertyName = currentProperty.getString();
+        if (currentPropertyName.equals(propertyName)) {
           return false;
         }
-        // Break the loop if the property exists
-        break;
       }
     }
 
@@ -276,7 +280,7 @@ private static boolean collectObjectProperty(
     newProperty.addChildToBack(newValue);
 
     if (existingProperty != null) {
-       objectLiteral.removeChild(existingProperty);
+       NodeUtil.deleteNode(existingProperty, compiler);
     }
     // If the property does not already exist we can safely add it
     objectLiteral.addChildToBack(newProperty);
@@ -285,8 +289,7 @@ private static boolean collectObjectProperty(
   }
 
 
-  private static boolean mightContainForwardReference(
-      Node node, String varName) {
+  private static boolean mightContainForwardReference(Node node, String varName) {
     if (node.isName()) {
       return varName.equals(node.getString());
     }

test/com/google/javascript/jscomp/PeepholeCollectPropertyAssignmentsTest.java

@@ -238,28 +238,85 @@ public final void testObjectFunctionRollup5() {
          "z:{a:function () {return o}}};");
   }
 
-  public final void testObjectPropertyReassigned(){
+  public final void testObjectPropertyReassigned() {
     test("var a = {b:''};" +
         "a.b='c';",
         "var a={b:'c'};");
   }
 
-  public final void testObjectPropertyReassigned2(){
+  public final void testObjectPropertyReassigned2() {
     test("var a = {b:'', x:10};" +
         "a.b='c';",
         "var a={x:10, b:'c'};");
   }
 
-  public final void testObjectPropertyReassigned3(){
+  public final void testObjectPropertyReassigned3() {
     test("var a = {x:10};" +
         "a.b = 'c';",
         "var a = {x:10, b:'c'};");
   }
 
-  public final void testObjectPropertyReassigned4(){
+  public final void testObjectPropertyReassigned4() {
     testSame(
         "var a = {b:10};" +
         "var x = 1;" +
         "a.b = x+10;");
   }
+
+  public final void testObjectComputedProp1() {
+    testSame(
+        lines(
+            "var a = {['computed']: 10};",
+            "var alsoComputed = 'someValue';",
+            "a[alsoComputed] = 20;"));
+  }
+
+  public final void testObjectComputedProp2() {
+    test(
+        lines(
+            "var a = {['computed']: 10};",
+            "a.prop = 20;"),
+        lines(
+            "var a = {",
+            "  ['computed']: 10,",
+            "  prop: 20,",
+            "};"));
+  }
+
+  public final void testObjectMemberFunction1() {
+    test(
+        lines(
+            "var a = { member() {} };",
+            "a.prop = 20;"),
+        lines(
+            "var a = {",
+            "  member() {},",
+            "  prop: 20,",
+            "};"));
+  }
+
+  public final void testObjectMemberFunction2() {
+    test(
+        lines(
+            "var a = { member() {} };",
+            "a.member = 20;"),
+        lines(
+            "var a = {",
+            "  member: 20,",
+            "};"));
+  }
+
+  public final void testObjectGetter() {
+    testSame(
+        lines(
+            "var a = { get x() {} };",
+            "a.x = 20;"));
+  }
+
+  public final void testObjectSetter() {
+    testSame(
+        lines(
+            "var a = { set x(value) {} };",
+            "a.x = 20;"));
+  }
 }