[classes] Fix enumeration order bugs when accessors shadow dynamic prop.

AddToDictionaryTemplate in literal-objects.cc was missing several
cases when handling the overwriting between properties with statically
known and dynamically computed names. This led to wrong enumeration
orders in class prototypes created from class templates.

Bug: v8:11158
Change-Id: I7381b4680ec533bd307a6c32d75c8a66394869df
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2536464
Commit-Queue: Frank Emrich <emrich@google.com>
Reviewed-by: Igor Sheludko <ishell@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Cr-Commit-Position: refs/heads/master@{#71266}

src/objects/literal-objects.cc

@@ -143,8 +143,10 @@ constexpr int ComputeEnumerationIndex(int value_index) {
                    ClassBoilerplate::kMinimumPrototypePropertiesCount});
 }
 
+constexpr int kAccessorNotDefined = -1;
+
 inline int GetExistingValueIndex(Object value) {
-  return value.IsSmi() ? Smi::ToInt(value) : -1;
+  return value.IsSmi() ? Smi::ToInt(value) : kAccessorNotDefined;
 }
 
 template <typename LocalIsolate, typename Dictionary, typename Key>
@@ -155,13 +157,14 @@ void AddToDictionaryTemplate(LocalIsolate* isolate,
                              Smi value) {
   InternalIndex entry = dictionary->FindEntry(isolate, key);
 
-  if (entry.is_not_found()) {
-    // Entry not found, add new one.
   const bool is_elements_dictionary =
       std::is_same<Dictionary, NumberDictionary>::value;
   STATIC_ASSERT(is_elements_dictionary !=
                 (std::is_same<Dictionary, NameDictionary>::value ||
                  std::is_same<Dictionary, OrderedNameDictionary>::value));
+
+  if (entry.is_not_found()) {
+    // Entry not found, add new one.
     int enum_order =
         Dictionary::kIsOrderedDictionaryType || is_elements_dictionary
             ? kDummyEnumerationIndex
@@ -193,9 +196,15 @@ void AddToDictionaryTemplate(LocalIsolate* isolate,
 
   } else {
     // Entry found, update it.
-    int enum_order = Dictionary::kIsOrderedDictionaryType
+    int enum_order_existing =
+        Dictionary::kIsOrderedDictionaryType
             ? kDummyEnumerationIndex
             : dictionary->DetailsAt(entry).dictionary_index();
+    int enum_order_computed =
+        Dictionary::kIsOrderedDictionaryType || is_elements_dictionary
+            ? kDummyEnumerationIndex
+            : ComputeEnumerationIndex(key_index);
+
     Object existing_value = dictionary->ValueAt(entry);
     if (value_kind == ClassBoilerplate::kData) {
       // Computed value is a normal method.
@@ -207,6 +216,7 @@ void AddToDictionaryTemplate(LocalIsolate* isolate,
         int existing_setter_index =
             GetExistingValueIndex(current_pair.setter());
         // At least one of the accessors must already be defined.
+        STATIC_ASSERT(kAccessorNotDefined < 0);
         DCHECK(existing_getter_index >= 0 || existing_setter_index >= 0);
         if (existing_getter_index < key_index &&
             existing_setter_index < key_index) {
@@ -214,14 +224,12 @@ void AddToDictionaryTemplate(LocalIsolate* isolate,
           // method or just one of them was defined before while the other one
           // was not defined yet, so overwrite property to kData.
           PropertyDetails details(kData, DONT_ENUM, PropertyCellType::kNoCell,
-                                  enum_order);
+                                  enum_order_existing);
           dictionary->DetailsAtPut(entry, details);
           dictionary->ValueAtPut(entry, value);
 
-        } else {
-          // The data property was defined "between" accessors so the one that
-          // was overwritten has to be cleared.
-          if (existing_getter_index < key_index) {
+        } else if (existing_getter_index != kAccessorNotDefined &&
+                   existing_getter_index < key_index) {
           DCHECK_LT(key_index, existing_setter_index);
           // Getter was defined and it was done before the computed method
           // and then it was overwritten by the current computed method which
@@ -229,31 +237,68 @@ void AddToDictionaryTemplate(LocalIsolate* isolate,
           // the getter.
           current_pair.set_getter(*isolate->factory()->null_value());
 
-          } else if (existing_setter_index < key_index) {
+        } else if (existing_setter_index != kAccessorNotDefined &&
+                   existing_setter_index < key_index) {
           DCHECK_LT(key_index, existing_getter_index);
           // Setter was defined and it was done before the computed method
           // and then it was overwritten by the current computed method which
           // in turn was later overwritten by the getter method. So we clear
           // the setter.
           current_pair.set_setter(*isolate->factory()->null_value());
+
+        } else {
+          // One of the following cases holds:
+          // The computed method was defined before ...
+          // 1.) the getter and setter, both of which are defined,
+          // 2.) the getter, and the setter isn't defined,
+          // 3.) the setter, and the getter isn't defined.
+          // Therefore, the computed value is overwritten, receiving the
+          // computed property's enum index.
+          DCHECK(key_index < existing_getter_index ||
+                 existing_getter_index == kAccessorNotDefined);
+          DCHECK(key_index < existing_setter_index ||
+                 existing_setter_index == kAccessorNotDefined);
+          DCHECK(existing_getter_index != kAccessorNotDefined ||
+                 existing_setter_index != kAccessorNotDefined);
+          if (!is_elements_dictionary) {
+            // The enum index is unused by elements dictionaries,
+            // which is why we don't need to update the property details if
+            // |is_elements_dictionary| holds.
+            PropertyDetails details = dictionary->DetailsAt(entry);
+            details = details.set_index(enum_order_computed);
+            dictionary->DetailsAtPut(entry, details);
           }
         }
-      } else {
-        // Overwrite existing value if it was defined before the computed one
-        // (AccessorInfo "length" property is always defined before).
+      } else {  // if (existing_value.IsAccessorPair()) ends here
+        DCHECK(value_kind == ClassBoilerplate::kData);
+
         DCHECK_IMPLIES(!existing_value.IsSmi(),
                        existing_value.IsAccessorInfo());
         DCHECK_IMPLIES(!existing_value.IsSmi(),
                        AccessorInfo::cast(existing_value).name() ==
                            *isolate->factory()->length_string());
         if (!existing_value.IsSmi() || Smi::ToInt(existing_value) < key_index) {
+          // Overwrite existing value because it was defined before the computed
+          // one (AccessorInfo "length" property is always defined before).
           PropertyDetails details(kData, DONT_ENUM, PropertyCellType::kNoCell,
-                                  enum_order);
+                                  enum_order_existing);
           dictionary->DetailsAtPut(entry, details);
           dictionary->ValueAtPut(entry, value);
+        } else {
+          // The computed value appears before the existing one. Set the
+          // existing entry's enum index to that of the computed one.
+          if (!is_elements_dictionary) {
+            // The enum index is unused by elements dictionaries,
+            // which is why we don't need to update the property details if
+            // |is_elements_dictionary| holds.
+
+            PropertyDetails details(kData, DONT_ENUM, PropertyCellType::kNoCell,
+                                    enum_order_computed);
+            dictionary->DetailsAtPut(entry, details);
           }
         }
-    } else {
+      }
+    } else {  // if (value_kind == ClassBoilerplate::kData) ends here
       AccessorComponent component = value_kind == ClassBoilerplate::kGetter
                                         ? ACCESSOR_GETTER
                                         : ACCESSOR_SETTER;
@@ -265,16 +310,51 @@ void AddToDictionaryTemplate(LocalIsolate* isolate,
             GetExistingValueIndex(current_pair.get(component));
         if (existing_component_index < key_index) {
           current_pair.set(component, value);
+        } else {
+          // The existing accessor property overwrites the computed one, update
+          // its enumeration order accordingly.
+
+          if (!is_elements_dictionary) {
+            // The enum index is unused by elements dictionaries,
+            // which is why we don't need to update the property details if
+            // |is_elements_dictionary| holds.
+
+            PropertyDetails details(kAccessor, DONT_ENUM,
+                                    PropertyCellType::kNoCell,
+                                    enum_order_computed);
+            dictionary->DetailsAtPut(entry, details);
+          }
         }
 
       } else {
-        // Overwrite existing value with new AccessorPair.
+        DCHECK(!existing_value.IsAccessorPair());
+        DCHECK(value_kind != ClassBoilerplate::kData);
+
+        if (!existing_value.IsSmi() || Smi::ToInt(existing_value) < key_index) {
+          // Overwrite the existing data property because it was defined before
+          // the computed accessor property.
           Handle<AccessorPair> pair(isolate->factory()->NewAccessorPair());
           pair->set(component, value);
-        PropertyDetails details(kAccessor, DONT_ENUM, PropertyCellType::kNoCell,
-                                enum_order);
+          PropertyDetails details(kAccessor, DONT_ENUM,
+                                  PropertyCellType::kNoCell,
+                                  enum_order_existing);
           dictionary->DetailsAtPut(entry, details);
           dictionary->ValueAtPut(entry, *pair);
+        } else {
+          // The computed accessor property appears before the existing data
+          // property. Set the existing entry's enum index to that of the
+          // computed one.
+
+          if (!is_elements_dictionary) {
+            // The enum index is unused by elements dictionaries,
+            // which is why we don't need to update the property details if
+            // |is_elements_dictionary| holds.
+
+            PropertyDetails details(kData, DONT_ENUM, PropertyCellType::kNoCell,
+                                    enum_order_computed);
+            dictionary->DetailsAtPut(entry, details);
+          }
+        }
       }
     }
   }

test/mjsunit/es6/computed-property-names-classes.js

@@ -509,3 +509,229 @@ function assertIteratorResult(value, done, result) {
     }
   }, ReferenceError);
 })();
+
+
+// The following tests deal with computed and statically known properties
+// of the same name overwriting each other.
+//
+// More concretely, we consider the cases where:
+// The computed property appears ...
+//   - before an existing data property           (case 1)
+//   - after an existing data property            (case 2)
+//   - before an existing getter and setter pair  (case 3)
+//   - before an existing getter xor setter       (case 4)
+//   - after an existing getter and setter pair   (case 5)
+//   - after an existing getter xor setter        (case 6)
+//   - in between two existing accessors          (case 7)
+//
+// For each of the 7 cases above, there exists A and B variants, indicating
+// whether the computed property refers to an accessor (variant A) or a
+// plain property (variant B).
+
+
+// |expect_getter| and |expect_setter| must be undefined if and only if we are
+// expecting |b| to be a data property.
+function TestOverwritingHelper(clazz, expect_getter, expect_setter) {
+  var proto = clazz.prototype;
+  var desc = Object.getOwnPropertyDescriptor(proto, 'b');
+
+  if (desc.hasOwnProperty('value')) {
+    assertEquals(undefined, expect_getter);
+    assertEquals(undefined, expect_setter);
+
+    assertEquals('B', proto.b());
+  } else {
+    assertEquals("boolean", typeof expect_getter);
+    assertEquals("boolean", typeof expect_setter);
+
+    if (expect_getter) {
+      assertEquals('B', proto.b);
+    } else {
+      assertEquals(undefined, desc.getter);
+    }
+
+      assertEquals(expect_setter, desc.set !== undefined);
+  }
+
+  assertEquals('A', proto.a());
+  assertEquals('C', proto.c());
+  assertEquals('D', proto.d());
+
+  assertArrayEquals([], Object.keys(proto));
+  assertArrayEquals(['constructor', 'a', 'b', 'c', 'd'],
+                    Object.getOwnPropertyNames(proto));
+}
+
+
+(function TestOverwriting1A() {
+  class C {
+    a() { return 'A'}
+    get [ID('b')]() { return 'Bx'; }
+    c() { return 'C'; }
+    b() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C);
+})();
+
+
+(function TestOverwriting1B() {
+  class C {
+    a() { return 'A'}
+    [ID('b')]() { return 'Bx'; }
+    c() { return 'C'; }
+    b() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C);
+})();
+
+
+(function TestOverwriting2A() {
+  class C {
+    a() { return 'A'}
+    b() { return 'Bx'; }
+    c() { return 'C'; }
+    get [ID('b')]() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C, true, false);
+})();
+
+
+(function TestOverwriting2B() {
+  class C {
+    a() { return 'A'}
+    b() { return 'Bx'; }
+    c() { return 'C'; }
+    [ID('b')]() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C);
+})();
+
+
+(function TestOverwriting3A() {
+  class C {
+    a() { return 'A'}
+    get [ID('b')]() { return 'Bx'; }
+    c() { return 'C'; }
+    get b() { return 'B'; }
+    set b(foo) { this.x = foo; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C, true, true);
+})();
+
+
+(function TestOverwriting3B() {
+  class C {
+    a() { return 'A'}
+    [ID('b')]() { return 'Bx'; }
+    c() { return 'C'; }
+    get b() { return 'B'; }
+    set b(foo) { this.x = foo; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C, true, true);
+})();
+
+
+(function TestOverwriting4A() {
+  class C {
+    a() { return 'A'}
+    get [ID('b')]() { return 'Bx'; }
+    c() { return 'C'; }
+    get b() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C, true, false);
+})();
+
+
+(function TestOverwriting4B() {
+  class C {
+    a() { return 'A'}
+    [ID('b')]() { return 'Bx'; }
+    c() { return 'C'; }
+    get b() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C, true, false);
+})();
+
+
+(function TestOverwriting5A() {
+  class C {
+    a() { return 'A'}
+    get b() { return 'Bx'; }
+    set b(foo) { this.x = foo; }
+    c() { return 'C'; }
+    get [ID('b')]() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C, true, true);
+})();
+
+
+(function TestOverwriting5B() {
+  class C {
+    a() { return 'A'}
+    get b() { return 'Bx'; }
+    set b(foo) { this.x = foo; }
+    c() { return 'C'; }
+    [ID('b')]() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C);
+})();
+
+
+(function TestOverwriting6A() {
+  class C {
+    a() { return 'A'}
+    set b(foo) { this.x = foo; }
+    c() { return 'C'; }
+    get [ID('b')]() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C, true, true);
+})();
+
+
+(function TestOverwriting6B() {
+  class C {
+    a() { return 'A'}
+    set b(foo) { this.x = foo; }
+    c() { return 'C'; }
+    [ID('b')]() { return 'B'; }
+    d() { return 'D'; }
+  }
+  TestOverwritingHelper(C);
+})();
+
+
+(function TestOverwriting7A() {
+  class C {
+    a() { return 'A'}
+    get b() { return 'Bx'; }
+    c() { return 'C'; }
+    get [ID('b')]() { return 'B'; }
+    d() { return 'D'; }
+    set b(foo) { this.x = foo; }
+  }
+  TestOverwritingHelper(C, true, true);
+})();
+
+
+(function TestOverwriting7B() {
+  class C {
+    a() { return 'A'}
+    set b(foo) { this.x = foo; }
+    c() { return 'C'; }
+    [ID('b')]() { return 'Bx'; }
+    d() { return 'D'; }
+    get b() { return 'B'; }
+  }
+  TestOverwritingHelper(C, true, false);
+})();