Update function type checking in VM after spec change (issue 444):

runtime/vm/object.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/object.h"
 
 #include "vm/assembler.h"
 #include "vm/assert.h"
 #include "vm/bigint_operations.h"
 #include "vm/bootstrap.h"
@@ skipping 2905 matching lines @@
 bool Function::HasCompatibleParametersWith(const Function& other) const {
   // The default values of optional parameters can differ.
   const intptr_t num_fixed_params = num_fixed_parameters();
   const intptr_t num_opt_params = num_optional_parameters();
   const intptr_t other_num_fixed_params = other.num_fixed_parameters();
   const intptr_t other_num_opt_params = other.num_optional_parameters();
   if ((num_fixed_params != other_num_fixed_params) ||
       (num_opt_params < other_num_opt_params)) {
     return false;
   }
-  // Check that for each optional named parameter of the other function, there
-  // exists an optional named parameter of this function with an identical name.
+  // Check that for each optional named parameter of the other function there is
+  // a corresponding optional named parameter of this function with an identical
+  // name at the same position.
   // Note that SetParameterNameAt() guarantees that names are symbols, so we can
   // compare their raw pointers.
-  const int num_params = num_fixed_params + num_opt_params;
   const int other_num_params = other_num_fixed_params + other_num_opt_params;
-  bool found_param_name;
-  String& other_param_name = String::Handle();
   for (intptr_t i = other_num_fixed_params; i < other_num_params; i++) {
-    other_param_name = other.ParameterNameAt(i);
-    found_param_name = false;
-    for (intptr_t j = num_fixed_params; j < num_params; j++) {
-      if (ParameterNameAt(j) == other_param_name.raw()) {
-        found_param_name = true;
-        break;
-      }
-    }
-    if (!found_param_name) {
+    if (ParameterNameAt(i) != other.ParameterNameAt(i)) {

[srdjan, 2011/11/29 01:01:59]

This is tricky, you are holding onto raw pointers for too long. I'd advise
storing both into handles and then comparing they raw pointers.

[regis, 2011/11/29 01:48:21]

Done.

       return false;
     }
   }
   return true;
 }
+
+
+bool Function::CheckParameters(const TypeArguments& type_arguments,
+                               const Function& other,
+                               const TypeArguments& other_type_arguments,
+                               intptr_t from_parameter,
+                               intptr_t to_parameter,
+                               bool check_names) const {

[srdjan, 2011/11/29 01:01:59]

I think this method has too many arguments. I suggest splitting it in 3,
something like:

CheckParameterAt(parameter_ix, type_arguments,  other_type_arguments);
CheckParameters(type_arguments,  other_type_arguments);
CheckNamedParameters(type_arguments, other_type_arguments,
num_opt_params_to_checks);

[regis, 2011/11/29 01:48:21]

I simplified the code by keeping the loop in TestType and by calling
TestParameterType on each checked parameter.

+  Type& param_type = Type::Handle();
+  Type& other_param_type = Type::Handle();
+  for (intptr_t i = from_parameter; i < to_parameter; i++) {
+    if (check_names && (ParameterNameAt(i) != other.ParameterNameAt(i))) {

[srdjan, 2011/11/29 01:01:59]

ditto about holding onto raw objects.

[regis, 2011/11/29 01:48:21]

Done.

+      return false;
+    }
+    param_type = ParameterTypeAt(i);
+    if (!param_type.IsInstantiated()) {
+      param_type = param_type.InstantiateFrom(type_arguments, 0);
+    }
+    if (param_type.IsDynamicType()) {
+      continue;
+    }
+    other_param_type = other.ParameterTypeAt(i);
+    if (!other_param_type.IsInstantiated()) {
+      other_param_type =
+          other_param_type.InstantiateFrom(other_type_arguments, 0);
+    }
+    if (other_param_type.IsDynamicType()) {
+      continue;
+    }
+    if (!param_type.IsSubtypeOf(other_param_type) &&
+        !other_param_type.IsSubtypeOf(param_type)) {
+      return false;
+    }
+  }
+  return true;
+}
 
 
 bool Function::TestType(TypeTestKind test,
                         const TypeArguments& type_arguments,
                         const Function& other,
                         const TypeArguments& other_type_arguments) const {
   const intptr_t num_fixed_params = num_fixed_parameters();
   const intptr_t num_opt_params = num_optional_parameters();
   const intptr_t other_num_fixed_params = other.num_fixed_parameters();
   const intptr_t other_num_opt_params = other.num_optional_parameters();
   if ((num_fixed_params != other_num_fixed_params) ||
       ((test == Type::kIsSubtypeOf) &&
        (num_opt_params < other_num_opt_params))) {
     return false;
   }
   // Check the result type.
   Type& other_res_type = Type::Handle(other.result_type());
   if (!other_res_type.IsInstantiated()) {
     other_res_type = other_res_type.InstantiateFrom(other_type_arguments, 0);
   }
   if (!other_res_type.IsDynamicType() && !other_res_type.IsVoidType()) {
     Type& res_type = Type::Handle(result_type());
     if (!res_type.IsInstantiated()) {
       res_type = res_type.InstantiateFrom(type_arguments, 0);
     }
     if (!res_type.IsDynamicType() &&
-        (res_type.IsVoidType() || !res_type.IsAssignableTo(other_res_type))) {
+        (res_type.IsVoidType() ||
+         !(res_type.IsSubtypeOf(other_res_type) ||
+           other_res_type.IsSubtypeOf(res_type)))) {
       return false;
     }
   }
   // Check the types of fixed parameters.
-  Type& param_type = Type::Handle();
-  Type& other_param_type = Type::Handle();
-  for (intptr_t i = 0; i < num_fixed_params; i++) {
-    param_type = ParameterTypeAt(i);
-    if (!param_type.IsInstantiated()) {
-      param_type = param_type.InstantiateFrom(type_arguments, 0);
-    }
-    if (param_type.IsDynamicType()) {
-      continue;
-    }
-    other_param_type = other.ParameterTypeAt(i);
-    if (!other_param_type.IsInstantiated()) {
-      other_param_type =
-          other_param_type.InstantiateFrom(other_type_arguments, 0);
-    }
-    if (other_param_type.IsDynamicType()) {
-      continue;
-    }
-    // Subtyping and assignability rules are identical when applied to parameter
-    // types.
-    ASSERT((test == Type::kIsSubtypeOf) || (test == Type::kIsAssignableTo));
-    if (!param_type.IsSubtypeOf(other_param_type) &&
-        !other_param_type.IsSubtypeOf(param_type)) {
-      return false;
-    }
+  if (!CheckParameters(type_arguments,
+                       other,
+                       other_type_arguments,
+                       0,
+                       num_fixed_params,
+                       /* check_names = */ false)) {
+    return false;
   }
   // Check the names and types of optional parameters.
-  // First, check that for each optional named parameter of type T of the other
-  // function type, there exists an optional named parameter of this function
-  // type with an identical name and with a Type S that is a subtype or
-  // supertype of T.
-  // Note that SetParameterNameAt() guarantees that names are symbols, so we can
-  // compare their raw pointers.
-  const int num_params = num_fixed_params + num_opt_params;
-  const int other_num_params = other_num_fixed_params + other_num_opt_params;
-  bool is_subtype = true;
-  bool found_param_name;
-  String& other_param_name = String::Handle();
-  for (intptr_t i = other_num_fixed_params; i < other_num_params; i++) {
-    other_param_name = other.ParameterNameAt(i);
-    found_param_name = false;
-    for (intptr_t j = num_fixed_params; j < num_params; j++) {
-      if (ParameterNameAt(j) == other_param_name.raw()) {
-        found_param_name = true;
-        param_type = ParameterTypeAt(j);
-        if (!param_type.IsInstantiated()) {
-          param_type = param_type.InstantiateFrom(type_arguments, 0);
-        }
-        if (param_type.IsDynamicType()) {
-          break;
-        }
-        other_param_type = other.ParameterTypeAt(i);
-        if (!other_param_type.IsInstantiated()) {
-          other_param_type =
-              other_param_type.InstantiateFrom(other_type_arguments, 0);
-        }
-        if (other_param_type.IsDynamicType()) {
-          break;
-        }
-        if (!param_type.IsSubtypeOf(other_param_type) &&
-            !other_param_type.IsSubtypeOf(param_type)) {
-          is_subtype = false;
-        }
-        break;
-      }
-    }
-    if (!found_param_name) {
-      is_subtype = false;
-      break;
-    }
+  if (num_opt_params >= other_num_opt_params) {
+    // Check that for each optional named parameter of type T of the other
+    // function type, there is a corresponding optional named parameter of this
+    // function at the same position with an identical name and with a Type S
+    // that is a subtype or supertype of T.
+    // Note that SetParameterNameAt() guarantees that names are symbols, so we
+    // can compare their raw pointers.
+    return CheckParameters(type_arguments,
+                           other,
+                           other_type_arguments,
+                           other_num_fixed_params,
+                           other_num_fixed_params + other_num_opt_params,
+                           /* check_names = */ true);
   }
-  // If this first checking step succeeds, return true, otherwise, this function
-  // type is not a subtype of the other function type.
-  if (is_subtype) {
-    return true;
-  }
-  if (test == Type::kIsSubtypeOf) {
-    return false;
-  }
+  ASSERT((test == Type::kIsAssignableTo) &&
+         (num_opt_params < other_num_opt_params));
   // To verify that this function type is assignable to the other function type,
-  // i.e whether the other function type is a subtype of this function type, we
-  // repeat the checking step above after swapping the other function type with
-  // this function type.
-  ASSERT(test == Type::kIsAssignableTo);
-  String& param_name = String::Handle();
-  is_subtype = true;
-  for (intptr_t i = num_fixed_params; i < num_params; i++) {
-    param_name = ParameterNameAt(i);
-    found_param_name = false;
-    for (intptr_t j = other_num_fixed_params; j < other_num_params; j++) {
-      if (other.ParameterNameAt(j) == param_name.raw()) {
-        found_param_name = true;
-        other_param_type = other.ParameterTypeAt(j);
-        if (!other_param_type.IsInstantiated()) {
-          other_param_type =
-              other_param_type.InstantiateFrom(other_type_arguments, 0);
-        }
-        if (other_param_type.IsDynamicType()) {
-          break;
-        }
-        param_type = ParameterTypeAt(i);
-        if (!param_type.IsInstantiated()) {
-          param_type = param_type.InstantiateFrom(type_arguments, 0);
-        }
-        if (param_type.IsDynamicType()) {
-          break;
-        }
-        if (!other_param_type.IsSubtypeOf(param_type) &&
-            !param_type.IsSubtypeOf(other_param_type)) {
-          return false;
-        }
-        break;
-      }
-    }
-    if (!found_param_name) {
-      return false;
-    }
-  }
-  return true;
+  // check that for each optional named parameter of type T of this function
+  // type, there is a corresponding optional named parameter of the other
+  // function at the same position with an identical name and with a Type S that
+  // is a subtype or supertype of T.
+  // Note that SetParameterNameAt() guarantees that names are symbols, so we
+  // can compare their raw pointers.
+  return CheckParameters(type_arguments,
+                         other,
+                         other_type_arguments,
+                         num_fixed_params,
+                         num_fixed_params + num_opt_params,
+                         /* check_names = */ true);
 }
 
 
 bool Function::IsImplicitClosureFunction() const {
   if (!IsClosureFunction()) {
     return false;
   }
   const Function& parent = Function::Handle(parent_function());
   return parent.raw_ptr()->implicit_closure_function_ == raw();
 }
@@ skipping 4157 matching lines @@
   const String& str = String::Handle(pattern());
   const char* format = "JSRegExp: pattern=%s flags=%s";
   intptr_t len = OS::SNPrint(NULL, 0, format, str.ToCString(), Flags());
   char* chars = reinterpret_cast<char*>(
       Isolate::Current()->current_zone()->Allocate(len + 1));
   OS::SNPrint(chars, (len + 1), format, str.ToCString(), Flags());
   return chars;
 }
 
 }  // namespace dart