Fix many bugs with closure conversion in checked mode.

runtime/vm/object.cc

Index: runtime/vm/object.cc
diff --git a/runtime/vm/object.cc b/runtime/vm/object.cc
index f339a50fa24771b5cc7a3bda7757e2c88af0b788..6bcb887d6a9432b7ae66bb1ec8dde0d47a35720f 100644
--- a/runtime/vm/object.cc
+++ b/runtime/vm/object.cc
@@ -6008,7 +6008,8 @@ intptr_t Function::NumImplicitParameters() const {
   }
   if ((k == RawFunction::kClosureFunction) ||
       (k == RawFunction::kImplicitClosureFunction) ||
-      (k == RawFunction::kSignatureFunction)) {
+      (k == RawFunction::kSignatureFunction) ||
+      (k == RawFunction::kConvertedClosureFunction)) {
     return 1;  // Closure object.
   }
   if (!is_static()) {
@@ -6332,10 +6333,22 @@ RawFunction* Function::InstantiateSignatureFrom(
   // the original uninstantiated parent signatures. That is not a problem.
   const Function& parent = Function::Handle(zone, parent_function());
   ASSERT(!HasInstantiatedSignature());
-  Function& sig = Function::Handle(
-      zone, Function::NewSignatureFunction(owner, parent,
-                                           TokenPosition::kNoSource, space));
-  sig.set_type_parameters(TypeArguments::Handle(zone, type_parameters()));
+
+  Function& sig = Function::Handle(zone, Function::null());
+  if (IsConvertedClosureFunction()) {
+    sig ^= Function::NewConvertedClosureFunction(

[Vyacheslav Egorov (Google), 2017/08/28 11:55:48]

sig = Function::New...

^= is only used when down-casting (e.g. you have RawObject* and you want to
store it into Function& handle).

+        String::Handle(zone, name()), parent, TokenPosition::kNoSource);
+    // TODO(sjindel): Kernel generic methods undone. Handle type parameters

[Vyacheslav Egorov (Google), 2017/08/28 11:55:48]

In VM code we these days encourage usage of TODO(dartbug.com/issue-number)
instead of TODO(username)

TODO(username) usually proliferate uncontrollably and it's also hard to find
which ones related to which issues. 

e.g. here it should probably be TODO(dartbug.com/30455) or TODO(30455) for
short.

[sjindel, 2017/08/28 15:05:59]

Understood, but this comment follows the pattern of the other places where we
have code that won't work with generic methods; i.e., it starts with:

"Kernel generic methods undone."

I could change all the other places with this pattern (there are several), but I
feel it's out of the scope of this revision.

[Vyacheslav Egorov (Google), 2017/08/28 15:59:12]

I think this comment is the only one that does not follow the convention:

$ git grep "Kernel generic methods undone"
runtime/vm/kernel_binary_flowgraph.cc:    // TODO(30455): Kernel generic methods
undone. Currently we only support
runtime/vm/kernel_binary_flowgraph.cc:  // TODO(30455): Kernel generic methods
undone. Since the frontend can't yet
runtime/vm/kernel_binary_flowgraph.cc:  // TODO(30455): Kernel generic methods
undone. When generic methods are
runtime/vm/kernel_to_il.cc:// TODO(30455): Kernel generic methods undone. This
seems to work for static
runtime/vm/object.cc:    // TODO(sjindel): Kernel generic methods undone. Handle
type parameters

+    // correctly when generic closures are supported. Until then, all type
+    // parameters to this target are used for captured type variables, so they
+    // aren't relevant to the type of the function.
+    sig.set_type_parameters(TypeArguments::Handle(zone, TypeArguments::null()));
+  } else {
+    sig ^= Function::NewSignatureFunction(owner, parent,
+                                          TokenPosition::kNoSource, space);
+    sig.set_type_parameters(TypeArguments::Handle(zone, type_parameters()));
+  }
+
   AbstractType& type = AbstractType::Handle(zone, result_type());
   if (!type.IsInstantiated()) {
     type =
@@ -6406,6 +6419,7 @@ bool Function::TestParameterType(TypeTestKind test_kind,
 bool Function::HasSameTypeParametersAndBounds(const Function& other) const {
   Thread* thread = Thread::Current();
   Zone* zone = thread->zone();
+
   const intptr_t num_type_params = NumTypeParameters(thread);
   if (num_type_params != other.NumTypeParameters(thread)) {
     return false;
@@ -7174,6 +7188,35 @@ RawString* Function::BuildSignature(NameVisibility name_visibility) const {
 bool Function::HasInstantiatedSignature(Genericity genericity,
                                         intptr_t num_free_fun_type_params,
                                         TrailPtr trail) const {
+  // This function works differently for converted closures.
+  //
+  // Unlike regular closures, it's not possible to know which type parameters
+  // are supposed to come from parent functions or classes and which are
+  // actually parameters to the closure it represents. For example, consider:
+  //
+  // class C<T> {

[Dmitry Stefantsov, 2017/08/28 11:38:15]

We try to keep the formatting compatible with Markdown, so that dartdoc may
generate nice HTML code.  Usually code examples in comments are indented with 4
spaces (so that there are 5 spaces in total after "//") to be formatted as code.

[sjindel, 2017/08/28 15:05:59]

Done, but does dartdoc work on C++ code too?

+  //   getf() => (T x) { return x; }
+  // }
+  //
+  // class D {
+  //   getf() {
+  //     dynamic fn<T>(T x) { return x; }
+  //     return fn;
+  //   }
+  // }
+  //
+  // The signature of "fn" as a converted closure will in both cases look like
+  // "<T>(T) => dynamic", because the signaute of the converted closure function

[Dmitry Stefantsov, 2017/08/28 11:38:15]

Another Markdown-related adjustment may be changing "fn" to `fn` and "<T>(T) =>
dynamic" to `<T>(T) => dynamic`.

[sjindel, 2017/08/28 15:05:59]

Done.

+  // is the same as it's top-level target function. However, in the first case
+  // the closure's type is instantiated, and in the second case it's not.
+  //
+  // Since we can never assume a converted closure is instantiated if it has any
+  // type parameters, we always return true in these cases.
+  if (IsConvertedClosureFunction()) {
+    if (genericity == kCurrentClass) return true;

[Vyacheslav Egorov (Google), 2017/08/28 11:55:48]

in general single line if-statements are frowned upon in the VM code. 

usually it is recommended to fully brace the if-statement even if is trivial.

I suggest rewriting this code to avoid if entirely.

return (genericity == kCurrentClass) || (NumTypeParameters() == 0);

[sjindel, 2017/08/28 15:05:59]

Done.

+    return NumTypeParameters() == 0;
+  }
+
   if (genericity != kCurrentClass) {
     // A generic typedef may declare a non-generic function type and get
     // instantiated with unrelated function type parameters. In that case, its
@@ -15361,46 +15404,6 @@ bool Instance::IsInstanceOf(
     Function& other_signature =
         Function::Handle(zone, Type::Cast(instantiated_other).signature());
     Function& sig_fun = Function::Handle(zone, Closure::Cast(*this).function());
-    if (sig_fun.IsConvertedClosureFunction()) {

[Dmitry Stefantsov, 2017/08/28 11:38:15]

Nice to see this code transformed into something more reasonable!

-      const String& closure_name = String::Handle(zone, sig_fun.name());
-      const Function& new_sig_fun = Function::Handle(
-          zone,
-          Function::NewConvertedClosureFunction(
-              closure_name, Function::Handle(zone, sig_fun.parent_function()),
-              TokenPosition::kNoSource));
-
-      new_sig_fun.set_type_parameters(
-          TypeArguments::Handle(zone, sig_fun.type_parameters()));
-      new_sig_fun.set_result_type(
-          AbstractType::Handle(zone, sig_fun.result_type()));
-      new_sig_fun.set_end_token_pos(TokenPosition::kNoSource);
-
-      new_sig_fun.set_is_debuggable(false);
-      new_sig_fun.set_is_visible(false);
-
-      // The converted closed top-level function type should have its first
-      // required optional parameter, i.e. context, removed.
-      const int num_fixed_params = sig_fun.num_fixed_parameters() - 1;
-      const int num_opt_params = sig_fun.NumOptionalParameters();
-      const bool has_opt_pos_params = sig_fun.HasOptionalPositionalParameters();
-      const int num_params = num_fixed_params + num_opt_params;
-      new_sig_fun.set_num_fixed_parameters(num_fixed_params);
-      new_sig_fun.SetNumOptionalParameters(num_opt_params, has_opt_pos_params);
-      new_sig_fun.set_parameter_types(
-          Array::Handle(zone, Array::New(num_params, Heap::kOld)));
-      new_sig_fun.set_parameter_names(
-          Array::Handle(zone, Array::New(num_params, Heap::kOld)));
-      AbstractType& param_type = AbstractType::Handle(zone);
-      String& param_name = String::Handle(zone);
-      for (int i = 0; i < num_params; i++) {
-        param_type = sig_fun.ParameterTypeAt(i + 1);
-        new_sig_fun.SetParameterTypeAt(i, param_type);
-        param_name = sig_fun.ParameterNameAt(i + 1);
-        new_sig_fun.SetParameterNameAt(i, param_name);
-      }
-
-      sig_fun = new_sig_fun.raw();
-    }
     if (!sig_fun.HasInstantiatedSignature()) {
       const TypeArguments& instantiator_type_arguments = TypeArguments::Handle(
           zone, Closure::Cast(*this).instantiator_type_arguments());