Fix member overriding rules in VM according to latest spec.

runtime/vm/class_finalizer.cc

 // Copyright (c) 2011, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 #include "vm/class_finalizer.h"
 
 #include "vm/flags.h"
 #include "vm/heap.h"
 #include "vm/isolate.h"
 #include "vm/longjump.h"
@@ skipping 535 matching lines @@
   const intptr_t num_parameters = function.NumberOfParameters();
   for (intptr_t i = 0; i < num_parameters; i++) {
     type = function.ParameterTypeAt(i);
     type = ResolveType(cls, type);
     function.SetParameterTypeAt(i, type);
     FinalizeType(type);
   }
 }
 
 
-static bool FuncNameExistsInSuper(const Class& cls,
-                                  const String& name) {
+static RawClass* InstanceMemberSuperOwner(const Class& cls,
+                                          const String& name) {

[siva, 2011/10/21 17:13:32]

I would have preferred the name FindSuperOwnerOfInstanceMember,
but that is just my preference. Please feel free to ignore this comment if the
name does not sound right.

[regis, 2011/10/21 17:33:38]

Done.

+  Class& super_class = Class::Handle();
+  Function& function = Function::Handle();
+  Field& field = Field::Handle();
+  super_class = cls.SuperClass();
+  while (!super_class.IsNull()) {
+    // Check if an instance member of same name exists in any super class.
+    function = super_class.LookupFunction(name);
+    if (!function.IsNull() && !function.is_static()) {
+      return super_class.raw();
+    }
+    field = super_class.LookupField(name);
+    if (!field.IsNull() && !field.is_static()) {
+      return super_class.raw();
+    }
+    super_class = super_class.SuperClass();
+  }
+  return Class::null();
+}
+
+
+static RawClass* FunctionSuperOwner(const Class& cls, const String& name) {

[siva, 2011/10/21 17:13:32]

Similarly I would have preferred FindSuperOwnerOfFunction but feel free to
ignore this comment.

[regis, 2011/10/21 17:33:38]

Done.

   Class& super_class = Class::Handle();
   Function& function = Function::Handle();
   super_class = cls.SuperClass();
   while (!super_class.IsNull()) {
-    // Check if a field of same name exists in any super class.
+    // Check if a function of same name exists in any super class.
     function = super_class.LookupFunction(name);
     if (!function.IsNull()) {
-      return true;
+      return super_class.raw();
     }
     super_class = super_class.SuperClass();
   }
-  return false;
-}
-
-
-static bool FieldNameExistsInSuper(const Class& cls, const String& name) {
-  Class& super_class = Class::Handle();
-  Field& field = Field::Handle();
-  super_class = cls.SuperClass();
-  while (!super_class.IsNull()) {
-    // Check if a function of same name exists in any super class.
-    field = super_class.LookupField(name);
-    if (!field.IsNull()) {
-      return true;
-    }
-    super_class = super_class.SuperClass();
-  }
-  return false;
+  return Class::null();
 }
 
 
 void ClassFinalizer::ResolveAndFinalizeMemberTypes(const Class& cls) {
-  // Resolve type of fields.
+  // Note that getters and setters are explicitly listed as such in the list of
+  // functions of a class, so we do not need to consider fields as implicitly
+  // generating getters and setters.
+  // The only compile errors we report are therefore:
+  // - a getter having the same name as a method (but not a getter) in a super
+  //   class or in a subclass.
+  // - a setter having the same name as a method (but not a setter) in a super
+  //   class or in a subclass.
+  // - a static field, instance field, or static method (but not an instance
+  //   method) having the same name as an instance member in a super class.
+
+  // Resolve type of fields and check for conflicts in super classes.
   Array& array = Array::Handle(cls.fields());
   Field& field = Field::Handle();
   Type& type = Type::Handle();
+  String& name = String::Handle();
+  Class& super_class = Class::Handle();
   intptr_t num_fields = array.Length();
-  String& name = String::Handle();
   for (intptr_t i = 0; i < num_fields; i++) {
     field ^= array.At(i);
     type = field.type();
     type = ResolveType(cls, type);
     field.set_type(type);
     FinalizeType(type);
     name = field.name();
-    if (FuncNameExistsInSuper(cls, name)) {
-      ReportError("field '%s' overrides a function in the super class.\n",
-                  name.ToCString());
+    super_class = InstanceMemberSuperOwner(cls, name);
+    if (!super_class.IsNull()) {
+      const String& class_name = String::Handle(cls.Name());
+      const String& super_class_name = String::Handle(super_class.Name());
+      ReportError("field '%s' of class '%s' conflicts with instance "
+                  "member '%s' of super class '%s'.\n",
+                  name.ToCString(),
+                  class_name.ToCString(),
+                  name.ToCString(),
+                  super_class_name.ToCString());
     }
   }
-  // Resolve function signatures.
+  // Resolve function signatures and check for conflicts in super classes.
   array = cls.functions();
   Function& function = Function::Handle();
+  Function& overridden_function = Function::Handle();
   intptr_t num_functions = array.Length();
-  String& func_name = String::Handle();
+  String& function_name = String::Handle();
   for (intptr_t i = 0; i < num_functions; i++) {
     function ^= array.At(i);
     ResolveAndFinalizeSignature(cls, function);
-    func_name = function.name();
-    if (FieldNameExistsInSuper(cls, func_name)) {
-      ReportError("function '%s' overrides a field in the super class.\n",
-                  func_name.ToCString());
-    }
-    name = Field::GetterName(func_name);
-    if (FuncNameExistsInSuper(cls, name)) {
-      ReportError("function '%s' overrides a getter in the super class.\n",
-                  name.ToCString());
-    }
-    name = Field::SetterName(func_name);
-    if (FuncNameExistsInSuper(cls, name)) {
-      ReportError("function '%s' overrides a setter in the super class.\n",
-                  name.ToCString());
+    function_name = function.name();
+    if (function.is_static()) {
+      super_class = InstanceMemberSuperOwner(cls, function_name);
+      if (!super_class.IsNull()) {
+        const String& class_name = String::Handle(cls.Name());
+        const String& super_class_name = String::Handle(super_class.Name());
+        ReportError("static function '%s' of class '%s' conflicts with "
+                    "instance member '%s' of super class '%s'.\n",
+                    function_name.ToCString(),
+                    class_name.ToCString(),
+                    function_name.ToCString(),
+                    super_class_name.ToCString());
+      }
+    } else {
+      // TODO(regis): This arity check is still being debated. Revisit.

[siva, 2011/10/21 17:13:32]

Yes this is still being debated, may have to yank out.

[regis, 2011/10/21 17:33:38]

OK. Keeping the TODO.

+      super_class = cls.SuperClass();
+      while (!super_class.IsNull()) {
+        overridden_function = super_class.LookupDynamicFunction(function_name);
+        if (!overridden_function.IsNull() &&
+          !function.HasCompatibleParametersWith(overridden_function)) {
+          // Function types are purposely not checked for subtyping.
+          const String& class_name = String::Handle(cls.Name());
+          const String& super_class_name = String::Handle(super_class.Name());
+          ReportError("class '%s' overrides function '%s' of super class '%s' "
+                      "with incompatible parameters.\n",
+                      class_name.ToCString(),
+                      function_name.ToCString(),
+                      super_class_name.ToCString());
+        }
+        super_class = super_class.SuperClass();
+      }
     }
     if (function.kind() == RawFunction::kGetterFunction) {
-      name = String::New("get:");
-      name = String::SubString(func_name, name.Length());
-      if (FuncNameExistsInSuper(cls, name)) {
-        ReportError("'%s' overrides a function in the super class.\n",
-                    func_name.ToCString());
+      name = Field::NameFromGetter(function_name);
+      super_class = FunctionSuperOwner(cls, name);
+      if (!super_class.IsNull()) {
+        const String& class_name = String::Handle(cls.Name());
+        const String& super_class_name = String::Handle(super_class.Name());
+        ReportError("getter '%s' of class '%s' conflicts with "
+                    "function '%s' of super class '%s'.\n",
+                    name.ToCString(),
+                    class_name.ToCString(),
+                    name.ToCString(),
+                    super_class_name.ToCString());
       }
-    }
-    if (function.kind() == RawFunction::kSetterFunction) {
-      name = String::New("set:");
-      name = String::SubString(func_name, name.Length());
-      if (FuncNameExistsInSuper(cls, name)) {
-        ReportError("'%s' overrides a function in the super class.\n",
-                    func_name.ToCString());
+    } else if (function.kind() == RawFunction::kSetterFunction) {
+      name = Field::NameFromSetter(function_name);
+      super_class = FunctionSuperOwner(cls, name);
+      if (!super_class.IsNull()) {
+        const String& class_name = String::Handle(cls.Name());
+        const String& super_class_name = String::Handle(super_class.Name());
+        ReportError("setter '%s' of class '%s' conflicts with "
+                    "function '%s' of super class '%s'.\n",
+                    name.ToCString(),
+                    class_name.ToCString(),
+                    name.ToCString(),
+                    super_class_name.ToCString());
+      }
+    } else {
+      name = Field::GetterName(function_name);
+      super_class = FunctionSuperOwner(cls, name);
+      if (!super_class.IsNull()) {
+        const String& class_name = String::Handle(cls.Name());
+        const String& super_class_name = String::Handle(super_class.Name());
+        ReportError("function '%s' of class '%s' conflicts with "
+                    "getter '%s' of super class '%s'.\n",
+                    name.ToCString(),

[siva, 2011/10/21 17:13:32]

I think this should be function_name.ToCString()

[regis, 2011/10/21 17:33:38]

Good catch.

+                    class_name.ToCString(),
+                    name.ToCString(),

[siva, 2011/10/21 17:13:32]

I think this should be function_name.ToCString()

[regis, 2011/10/21 17:33:38]

Done.

+                    super_class_name.ToCString());
+      }
+      name = Field::SetterName(function_name);
+      super_class = FunctionSuperOwner(cls, name);
+      if (!super_class.IsNull()) {
+        const String& class_name = String::Handle(cls.Name());
+        const String& super_class_name = String::Handle(super_class.Name());
+        ReportError("function '%s' of class '%s' conflicts with "
+                    "setter '%s' of super class '%s'.\n",
+                    name.ToCString(),

[siva, 2011/10/21 17:13:32]

I think this should be function_name.ToCString()

[regis, 2011/10/21 17:33:38]

Done.

+                    class_name.ToCString(),
+                    name.ToCString(),

[siva, 2011/10/21 17:13:32]

I think this should be function_name.ToCString()

[regis, 2011/10/21 17:33:38]

Done.

+                    super_class_name.ToCString());
       }
     }
   }
   // Resolve the signature type if this class is a signature class.
   if (cls.IsSignatureClass()) {
     const Type& signature_type = Type::Handle(cls.SignatureType());
     FinalizeType(signature_type);
   }
 }
 
@@ skipping 38 matching lines @@
   Array& interface_types = Array::Handle(cls.interfaces());
   Type& interface_type = Type::Handle();
   for (intptr_t i = 0; i < interface_types.Length(); i++) {
     interface_type ^= interface_types.At(i);
     FinalizeType(interface_type);
   }
   // Mark as finalized before resolving member types in order to break cycles.
   cls.Finalize();
   ResolveAndFinalizeMemberTypes(cls);
   // Run additional checks after all types are finalized.
-  if (!cls.is_interface()) {
-    CheckForLegalOverrides(cls);
-  }
   if (cls.is_const()) {
     CheckForLegalConstClass(cls);
   }
 }
 
 
 bool ClassFinalizer::IsSuperCycleFree(const Class& cls) {
   Class& test1 = Class::Handle(cls.raw());
   Class& test2 = Class::Handle(cls.SuperClass());
   // A finalized class has been checked for cycles.
@@ skipping 94 matching lines @@
     // TODO(regis): Verify that unless cls is in core lib, it cannot implement
     // an instance of Number or String. Any other? bool?
 
     // Now resolve the super interfaces.
     ResolveInterfaces(interface_class, visited);
   }
   visited->RemoveLast();
 }
 
 
-void ClassFinalizer::CheckForLegalOverrides(const Class& cls) {
-  HANDLESCOPE();
-  const Class& super = Class::Handle(cls.SuperClass());
-  if (super.IsNull()) {
-    return;
-  }
-  if (FLAG_enable_type_checks) {
-    // Check functions.
-    const Array& functions_array = Array::Handle(cls.functions());
-    Function& function = Function::Handle();
-    String& function_name = String::Handle();
-    const intptr_t len = functions_array.Length();
-    for (intptr_t i = 0; i < len; i++) {
-      function ^= functions_array.At(i);
-      if (!function.is_static()) {
-        function_name ^= function.name();
-        Function& overridden_function =
-            Function::Handle(super.LookupDynamicFunction(function_name));
-        if (!overridden_function.IsNull() &&
-            !function.IsSubtypeOf(overridden_function)) {
-          const String& class_name = String::Handle(cls.Name());
-          const String& super_class_name = String::Handle(
-              Class::Handle(overridden_function.owner()).Name());
-          ReportError("The type of instance method '%s' in class '%s' is "
-                      "not a subtype of the type of overriden instance "
-                      "method '%s' in class '%s'\n",
-                      function_name.ToCString(),
-                      class_name.ToCString(),
-                      function_name.ToCString(),
-                      super_class_name.ToCString());
-        }
-      }
-    }
-  }
-  // Check fields.
-  const Array& fields_array = Array::Handle(cls.fields());
-  Field& field = Field::Handle();
-  String& field_name = String::Handle();
-  const intptr_t len = fields_array.Length();
-  for (intptr_t i = 0; i < len; i++) {
-    field ^= fields_array.At(i);
-    field_name ^= field.name();
-    Field& super_field = Field::Handle(super.LookupStaticField(field_name));
-    if (super_field.IsNull()) {
-      super_field = super.LookupInstanceField(field_name);
-    }
-    if (!super_field.IsNull()) {
-      // A static field may "override" a static field.
-      if (!super_field.is_static() || !field.is_static()) {
-        const String& class_name = String::Handle(cls.Name());
-        ReportError("class '%s' cannot override field '%s'.\n",
-                    class_name.ToCString(), field_name.ToCString());
-      }
-    }
-  }
-}
-
-
 // A class is marked as constant if it has one constant constructor.
 // A constant class:
 // - may extend only const classes.
 // - has only const instance fields.
 // Note: we must check for cycles before checking for const properties.
 void ClassFinalizer::CheckForLegalConstClass(const Class& cls) {
   ASSERT(cls.is_const());
   const Class& super = Class::Handle(cls.SuperClass());
   if (!super.IsNull() && !super.is_const()) {
     String& name = String::Handle(super.Name());
@@ skipping 97 matching lines @@
   ASSERT(msg_buffer != NULL);
   va_list args;
   va_start(args, format);
   OS::VSNPrint(msg_buffer, kBufferLength, format, args);
   va_end(args);
   isolate->long_jump_base()->Jump(1, msg_buffer);
   UNREACHABLE();
 }
 
 }  // namespace dart