Install class "name" accessor before methods

tc39/ecma262#1490 changed the spec so that the
"name" property of a class should be installed after "length" but before
"prototype". This CL adapts accordingly.

After this change, there is now no need for the separate code path to
set the "name" accessor at runtime. Delete the relevant runtime code as
well.

Bug: v8:8771
Change-Id: I8f809b45bf209c899cf5df76d0ebf6d9a45a6d4e
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2974772
Commit-Queue: Timothy Gu <[email protected]>
Reviewed-by: Marja Hölttä <[email protected]>
Cr-Commit-Position: refs/heads/master@{#75340}

Author: TimothyGu

src/ast/ast.h

@@ -2453,9 +2453,6 @@ class ClassLiteral final : public Expression {
   ZonePtrList<Property>* private_members() const { return private_members_; }
   int start_position() const { return position(); }
   int end_position() const { return end_position_; }
-  bool has_name_static_property() const {
-    return HasNameStaticProperty::decode(bit_field_);
-  }
   bool has_static_computed_names() const {
     return HasStaticComputedNames::decode(bit_field_);
   }
@@ -2491,9 +2488,9 @@ class ClassLiteral final : public Expression {
                FunctionLiteral* static_initializer,
                FunctionLiteral* instance_members_initializer_function,
                int start_position, int end_position,
-               bool has_name_static_property, bool has_static_computed_names,
-               bool is_anonymous, bool has_private_methods,
-               Variable* home_object, Variable* static_home_object)
+               bool has_static_computed_names, bool is_anonymous,
+               bool has_private_methods, Variable* home_object,
+               Variable* static_home_object)
       : Expression(start_position, kClassLiteral),
         end_position_(end_position),
         scope_(scope),
@@ -2506,8 +2503,7 @@ class ClassLiteral final : public Expression {
             instance_members_initializer_function),
         home_object_(home_object),
         static_home_object_(static_home_object) {
-    bit_field_ |= HasNameStaticProperty::encode(has_name_static_property) |
-                  HasStaticComputedNames::encode(has_static_computed_names) |
+    bit_field_ |= HasStaticComputedNames::encode(has_static_computed_names) |
                   IsAnonymousExpression::encode(is_anonymous) |
                   HasPrivateMethods::encode(has_private_methods);
   }
@@ -2520,8 +2516,7 @@ class ClassLiteral final : public Expression {
   ZonePtrList<Property>* private_members_;
   FunctionLiteral* static_initializer_;
   FunctionLiteral* instance_members_initializer_function_;
-  using HasNameStaticProperty = Expression::NextBitField<bool, 1>;
-  using HasStaticComputedNames = HasNameStaticProperty::Next<bool, 1>;
+  using HasStaticComputedNames = Expression::NextBitField<bool, 1>;
   using IsAnonymousExpression = HasStaticComputedNames::Next<bool, 1>;
   using HasPrivateMethods = IsAnonymousExpression::Next<bool, 1>;
   Variable* home_object_;
@@ -3251,16 +3246,14 @@ class AstNodeFactory final {
       ZonePtrList<ClassLiteral::Property>* private_members,
       FunctionLiteral* static_initializer,
       FunctionLiteral* instance_members_initializer_function,
-      int start_position, int end_position, bool has_name_static_property,
-      bool has_static_computed_names, bool is_anonymous,
-      bool has_private_methods, Variable* home_object,
+      int start_position, int end_position, bool has_static_computed_names,
+      bool is_anonymous, bool has_private_methods, Variable* home_object,
       Variable* static_home_object) {
     return zone_->New<ClassLiteral>(
         scope, extends, constructor, public_members, private_members,
         static_initializer, instance_members_initializer_function,
-        start_position, end_position, has_name_static_property,
-        has_static_computed_names, is_anonymous, has_private_methods,
-        home_object, static_home_object);
+        start_position, end_position, has_static_computed_names, is_anonymous,
+        has_private_methods, home_object, static_home_object);
   }
 
   NativeFunctionLiteral* NewNativeFunctionLiteral(const AstRawString* name,

src/objects/literal-objects.cc

@@ -276,10 +276,13 @@ void AddToDictionaryTemplate(IsolateT* isolate, Handle<Dictionary> dictionary,
                        existing_value.IsAccessorInfo());
         DCHECK_IMPLIES(!existing_value.IsSmi(),
                        AccessorInfo::cast(existing_value).name() ==
-                           *isolate->factory()->length_string());
+                               *isolate->factory()->length_string() ||
+                           AccessorInfo::cast(existing_value).name() ==
+                               *isolate->factory()->name_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).
+          // one (AccessorInfo "length" and "name" properties are always defined
+          // before).
           PropertyDetails details(
               kData, DONT_ENUM, PropertyDetails::kConstIfDictConstnessTracking,
               enum_order_existing);
@@ -625,6 +628,14 @@ Handle<ClassBoilerplate> ClassBoilerplate::BuildClassBoilerplate(
     static_desc.AddConstant(isolate, factory->length_string(),
                             factory->function_length_accessor(), attribs);
   }
+  {
+    // Add name_accessor.
+    // All classes, even anonymous ones, have a name accessor.
+    PropertyAttributes attribs =
+        static_cast<PropertyAttributes>(DONT_ENUM | READ_ONLY);
+    static_desc.AddConstant(isolate, factory->name_string(),
+                            factory->function_name_accessor(), attribs);
+  }
   {
     // Add prototype_accessor.
     PropertyAttributes attribs =
@@ -698,18 +709,6 @@ Handle<ClassBoilerplate> ClassBoilerplate::BuildClassBoilerplate(
     }
   }
 
-  // All classes, even anonymous ones, have a name accessor. If static_desc is
-  // in dictionary mode, the name accessor is installed at runtime in
-  // DefineClass.
-  if (!expr->has_name_static_property() &&
-      !static_desc.HasDictionaryProperties()) {
-    // Set class name accessor if the "name" method was not added yet.
-    PropertyAttributes attribs =
-        static_cast<PropertyAttributes>(DONT_ENUM | READ_ONLY);
-    static_desc.AddConstant(isolate, factory->name_string(),
-                            factory->function_name_accessor(), attribs);
-  }
-
   static_desc.Finalize(isolate);
   instance_desc.Finalize(isolate);
 

src/parsing/parser-base.h

@@ -595,7 +595,6 @@ class ParserBase {
           instance_fields(parser->impl()->NewClassPropertyList(4)),
           constructor(parser->impl()->NullExpression()),
           has_seen_constructor(false),
-          has_name_static_property(false),
           has_static_computed_names(false),
           has_static_elements(false),
           has_static_private_methods(false),
@@ -615,7 +614,6 @@ class ParserBase {
     FunctionLiteralT constructor;
 
     bool has_seen_constructor;
-    bool has_name_static_property;
     bool has_static_computed_names;
     bool has_static_elements;
     bool has_static_private_methods;
@@ -2316,11 +2314,6 @@ ParserBase<Impl>::ParseClassPropertyDefinition(ClassInfo* class_info,
     name_expression = ParseProperty(prop_info);
   }
 
-  if (!class_info->has_name_static_property && prop_info->is_static &&
-      impl()->IsName(prop_info->name)) {
-    class_info->has_name_static_property = true;
-  }
-
   switch (prop_info->kind) {
     case ParsePropertyKind::kAssign:
     case ParsePropertyKind::kClassField:

src/parsing/parser.cc

@@ -3132,7 +3132,6 @@ FunctionLiteral* Parser::CreateInitializerFunction(
 //   - proxy
 //   - extends
 //   - properties
-//   - has_name_static_property
 //   - has_static_computed_names
 Expression* Parser::RewriteClassLiteral(ClassScope* block_scope,
                                         const AstRawString* name,
@@ -3183,7 +3182,6 @@ Expression* Parser::RewriteClassLiteral(ClassScope* block_scope,
       block_scope, class_info->extends, class_info->constructor,
       class_info->public_members, class_info->private_members,
       static_initializer, instance_members_initializer_function, pos, end_pos,
-      class_info->has_name_static_property,
       class_info->has_static_computed_names, class_info->is_anonymous,
       class_info->has_private_methods, class_info->home_object_variable,
       class_info->static_home_object_variable);

src/runtime/runtime-classes.cc

@@ -202,19 +202,12 @@ Handle<Dictionary> ShallowCopyDictionaryTemplate(
 
 template <typename Dictionary>
 bool SubstituteValues(Isolate* isolate, Handle<Dictionary> dictionary,
-                      RuntimeArguments& args,
-                      bool* install_name_accessor = nullptr) {
-  Handle<Name> name_string = isolate->factory()->name_string();
-
+                      RuntimeArguments& args) {
   // Replace all indices with proper methods.
   ReadOnlyRoots roots(isolate);
   for (InternalIndex i : dictionary->IterateEntries()) {
     Object maybe_key = dictionary->KeyAt(i);
     if (!Dictionary::IsKey(roots, maybe_key)) continue;
-    if (install_name_accessor && *install_name_accessor &&
-        (maybe_key == *name_string)) {
-      *install_name_accessor = false;
-    }
     Handle<Object> key(maybe_key, isolate);
     Handle<Object> value(dictionary->ValueAt(i), isolate);
     if (value->IsAccessorPair()) {
@@ -400,7 +393,7 @@ bool AddDescriptorsByTemplate(
     Handle<Dictionary> properties_dictionary_template,
     Handle<NumberDictionary> elements_dictionary_template,
     Handle<FixedArray> computed_properties, Handle<JSObject> receiver,
-    bool install_name_accessor, RuntimeArguments& args) {
+    RuntimeArguments& args) {
   int computed_properties_length = computed_properties->length();
 
   // Shallow-copy properties template.
@@ -438,20 +431,9 @@ bool AddDescriptorsByTemplate(
   }
 
   // Replace all indices with proper methods.
-  if (!SubstituteValues<Dictionary>(isolate, properties_dictionary, args,
-                                    &install_name_accessor)) {
+  if (!SubstituteValues<Dictionary>(isolate, properties_dictionary, args)) {
     return false;
   }
-  if (install_name_accessor) {
-    PropertyAttributes attribs =
-        static_cast<PropertyAttributes>(DONT_ENUM | READ_ONLY);
-    PropertyDetails details(kAccessor, attribs,
-                            PropertyDetails::kConstIfDictConstnessTracking);
-    Handle<Dictionary> dict = ToHandle(Dictionary::Add(
-        isolate, properties_dictionary, isolate->factory()->name_string(),
-        isolate->factory()->function_name_accessor(), details));
-    CHECK_EQ(*dict, *properties_dictionary);
-  }
 
   UpdateProtectors(isolate, receiver, properties_dictionary);
 
@@ -520,23 +502,18 @@ bool InitClassPrototype(Isolate* isolate,
     map->set_may_have_interesting_symbols(true);
     map->set_construction_counter(Map::kNoSlackTracking);
 
-    // Class prototypes do not have a name accessor.
-    const bool install_name_accessor = false;
-
     if (V8_ENABLE_SWISS_NAME_DICTIONARY_BOOL) {
       Handle<SwissNameDictionary> properties_dictionary_template =
           Handle<SwissNameDictionary>::cast(properties_template);
       return AddDescriptorsByTemplate(
           isolate, map, properties_dictionary_template,
-          elements_dictionary_template, computed_properties, prototype,
-          install_name_accessor, args);
+          elements_dictionary_template, computed_properties, prototype, args);
     } else {
       Handle<NameDictionary> properties_dictionary_template =
           Handle<NameDictionary>::cast(properties_template);
       return AddDescriptorsByTemplate(
           isolate, map, properties_dictionary_template,
-          elements_dictionary_template, computed_properties, prototype,
-          install_name_accessor, args);
+          elements_dictionary_template, computed_properties, prototype, args);
     }
   }
 }
@@ -582,24 +559,19 @@ bool InitClassConstructor(Isolate* isolate,
     map->set_may_have_interesting_symbols(true);
     map->set_construction_counter(Map::kNoSlackTracking);
 
-    // All class constructors have a name accessor.
-    const bool install_name_accessor = true;
-
     if (V8_ENABLE_SWISS_NAME_DICTIONARY_BOOL) {
       Handle<SwissNameDictionary> properties_dictionary_template =
           Handle<SwissNameDictionary>::cast(properties_template);
 
       return AddDescriptorsByTemplate(
           isolate, map, properties_dictionary_template,
-          elements_dictionary_template, computed_properties, constructor,
-          install_name_accessor, args);
+          elements_dictionary_template, computed_properties, constructor, args);
     } else {
       Handle<NameDictionary> properties_dictionary_template =
           Handle<NameDictionary>::cast(properties_template);
       return AddDescriptorsByTemplate(
           isolate, map, properties_dictionary_template,
-          elements_dictionary_template, computed_properties, constructor,
-          install_name_accessor, args);
+          elements_dictionary_template, computed_properties, constructor, args);
     }
   }
 }

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

@@ -78,10 +78,7 @@ function ID(x) {
   assertEquals('C', C.c());
   assertEquals('D', C.d());
   assertArrayEquals([], Object.keys(C));
-  // TODO(arv): It is not clear that we are adding the "standard" properties
-  // in the right order. As far as I can tell the spec adds them in alphabetical
-  // order.
-  assertArrayEquals(['length', 'prototype', 'a', 'b', 'c', 'd', 'name'],
+  assertArrayEquals(['length', 'name', 'prototype', 'a', 'b', 'c', 'd'],
                     Object.getOwnPropertyNames(C));
 })();
 
@@ -99,7 +96,7 @@ function ID(x) {
   assertEquals('D', C[2]());
   // Array indexes first.
   assertArrayEquals([], Object.keys(C));
-  assertArrayEquals(['1', '2', 'length', 'prototype', 'a', 'c', 'name'],
+  assertArrayEquals(['1', '2', 'length', 'name', 'prototype', 'a', 'c'],
                     Object.getOwnPropertyNames(C));
 })();
 
@@ -118,7 +115,7 @@ function ID(x) {
   assertEquals('C', C.c());
   assertEquals('D', C[sym2]());
   assertArrayEquals([], Object.keys(C));
-  assertArrayEquals(['length', 'prototype', 'a', 'c', 'name'],
+  assertArrayEquals(['length', 'name', 'prototype', 'a', 'c'],
                     Object.getOwnPropertyNames(C));
   assertArrayEquals([sym1, sym2], Object.getOwnPropertySymbols(C));
 })();

test/mjsunit/es6/function-name.js

@@ -394,25 +394,25 @@
 })();
 
 (function testClassNameOrder() {
-  assertEquals(['length', 'prototype', 'name'], Object.getOwnPropertyNames(class {}));
+  assertEquals(['length', 'name', 'prototype'], Object.getOwnPropertyNames(class {}));
 
   var tmp = {'': class {}};
   var Tmp = tmp[''];
-  assertEquals(['length', 'prototype', 'name'], Object.getOwnPropertyNames(Tmp));
+  assertEquals(['length', 'name', 'prototype'], Object.getOwnPropertyNames(Tmp));
 
   var name = () => '';
   var tmp = {[name()]: class {}};
   var Tmp = tmp[name()];
-  assertEquals(['length', 'prototype', 'name'], Object.getOwnPropertyNames(Tmp));
+  assertEquals(['length', 'name', 'prototype'], Object.getOwnPropertyNames(Tmp));
 
   class A { }
-  assertEquals(['length', 'prototype', 'name'], Object.getOwnPropertyNames(A));
+  assertEquals(['length', 'name', 'prototype'], Object.getOwnPropertyNames(A));
 
   class B { static foo() { } }
-  assertEquals(['length', 'prototype', 'foo', 'name'], Object.getOwnPropertyNames(B));
+  assertEquals(['length', 'name', 'prototype', 'foo'], Object.getOwnPropertyNames(B));
 
   class C { static name() { } static foo() { } }
-  assertEquals(['length', 'prototype', 'name', 'foo'], Object.getOwnPropertyNames(C));
+  assertEquals(['length', 'name', 'prototype', 'foo'], Object.getOwnPropertyNames(C));
 })();
 
 (function testStaticName() {

test/test262/test262.status

@@ -71,11 +71,6 @@
   # https://code.google.com/p/v8/issues/detail?id=10958
   'language/module-code/eval-gtbndng-indirect-faux-assertion': [FAIL],
 
-  # https://code.google.com/p/v8/issues/detail?id=8771
-  'language/computed-property-names/class/static/method-number': [FAIL],
-  'language/computed-property-names/class/static/method-string': [FAIL],
-  'language/computed-property-names/class/static/method-symbol': [FAIL],
-
   # https://bugs.chromium.org/p/v8/issues/detail?id=4895
   # Some TypedArray methods throw due to the same bug, from Get
   'built-ins/TypedArray/prototype/every/callbackfn-detachbuffer': [FAIL],