[ VM / Hot Reload ] Fixed missing NoSuchMethod exceptions when hot reloading in situations with tearoffs.

Change-Id: I43ebfe2d788114d028eebbe675308c272972e06d
Reviewed-on: https://dart-review.googlesource.com/49281
Commit-Queue: Ben Konyi <bkonyi@google.com>
Reviewed-by: Ryan Macnak <rmacnak@google.com>

runtime/lib/errors_patch.dart

@@ -184,6 +184,10 @@ class NoSuchMethodError {
       : _receiver = receiver,
         _invocation = invocation as _InvocationMirror;
 
+  static void _throwNewInvocation(Object receiver, Invocation invocation) {
+    throw new NoSuchMethodError.withInvocation(receiver, invocation);
+  }
+
   // The compiler emits a call to _throwNew when it cannot resolve a static
   // method at compile time. The receiver is actually the literal class of the
   // unresolved method.

runtime/lib/invocation_mirror_patch.dart

@@ -57,15 +57,17 @@ class _InvocationMirror implements Invocation {
   }
 
   void _setMemberNameAndType() {
+    _type ??= 0;
     if (_functionName.startsWith("get:")) {
-      _type = _GETTER;
+      _type |= _GETTER;
       _memberName = new internal.Symbol.unvalidated(_functionName.substring(4));
     } else if (_functionName.startsWith("set:")) {
-      _type = _SETTER;
+      _type |= _SETTER;
       _memberName =
           new internal.Symbol.unvalidated(_functionName.substring(4) + "=");
     } else {
-      _type = _isSuperInvocation ? (_SUPER << _LEVEL_SHIFT) | _METHOD : _METHOD;
+      _type |=
+          _isSuperInvocation ? (_SUPER << _LEVEL_SHIFT) | _METHOD : _METHOD;
       _memberName = new internal.Symbol.unvalidated(_functionName);
     }
   }
@@ -188,14 +190,15 @@ class _InvocationMirror implements Invocation {
   }
 
   _InvocationMirror(this._functionName, this._argumentsDescriptor,
-      this._arguments, this._isSuperInvocation);
+      this._arguments, this._isSuperInvocation, this._type);
 
   _InvocationMirror._withoutType(this._functionName, this._typeArguments,
       this._positionalArguments, this._namedArguments, this._isSuperInvocation);
 
   static _allocateInvocationMirror(String functionName,
-      List argumentsDescriptor, List arguments, bool isSuperInvocation) {
+      List argumentsDescriptor, List arguments, bool isSuperInvocation,
+      [int type = null]) {
     return new _InvocationMirror(
-        functionName, argumentsDescriptor, arguments, isSuperInvocation);
+        functionName, argumentsDescriptor, arguments, isSuperInvocation, type);
   }
 }

runtime/tests/vm/vm.status

@@ -185,9 +185,6 @@ dart/spawn_shutdown_test: SkipSlow
 
 [ $compiler == dartk && $runtime == vm && $system == macos ]
 cc/IsolateReload_LibraryLookup: Fail, Crash
-cc/IsolateReload_TearOff_AddArguments: Fail
-cc/IsolateReload_TearOff_Instance_Equality: Fail
-cc/IsolateReload_TearOff_List_Set: Fail
 
 [ $compiler == dartk && $runtime == vm && $strong ]
 cc/IsolateReload_LibraryHide: Crash
@@ -195,15 +192,9 @@ cc/IsolateReload_LibraryShow: Crash
 
 [ $compiler == dartk && $system == linux ]
 cc/IsolateReload_LibraryLookup: Fail, Crash
-cc/IsolateReload_TearOff_AddArguments: Fail
-cc/IsolateReload_TearOff_Instance_Equality: Fail
-cc/IsolateReload_TearOff_List_Set: Fail
 
 [ $compiler == dartk && $system == windows ]
 cc/IsolateReload_LibraryLookup: Fail, Crash
-cc/IsolateReload_TearOff_AddArguments: Fail
-cc/IsolateReload_TearOff_Instance_Equality: Fail
-cc/IsolateReload_TearOff_List_Set: Fail
 cc/Profiler_ContextAllocation: Fail, Pass # Flaky on Windows --- sometimes give "profiler_test.cc: 1107: error: expected: !walker.Down()"
 cc/Profiler_FunctionTicks: Fail, Pass
 cc/Profiler_ToggleRecordAllocation: Fail, Pass

runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc

@@ -4206,11 +4206,28 @@ Fragment StreamingFlowGraphBuilder::BuildInitializers(
 
 FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfImplicitClosureFunction(
     const Function& function) {
+  const Function& parent = Function::ZoneHandle(Z, function.parent_function());
+  const String& func_name = String::ZoneHandle(Z, parent.name());
+  const Class& owner = Class::ZoneHandle(Z, parent.Owner());
+  Function& target = Function::ZoneHandle(Z, owner.LookupFunction(func_name));
+
+  if (target.raw() != parent.raw()) {
+    DEBUG_ASSERT(Isolate::Current()->HasAttemptedReload());
+    if (target.IsNull() || (target.is_static() != parent.is_static()) ||
+        (target.kind() != parent.kind())) {
+      target = Function::null();
+    }
+  }
+
+  if (target.IsNull() ||
+      (parent.num_fixed_parameters() != target.num_fixed_parameters())) {
+    return BuildGraphOfNoSuchMethodForwarder(function, true,
+                                             parent.is_static());
+  }
+
   // The prologue builder needs the default parameter values.
   SetupDefaultParameterValues();
 
-  const Function& target = Function::ZoneHandle(Z, function.parent_function());
-
   TargetEntryInstr* normal_entry = flow_graph_builder_->BuildTargetEntry();
   PrologueInfo prologue_info(-1, -1);
   BlockEntryInstr* instruction_cursor =
@@ -4294,10 +4311,10 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfImplicitClosureFunction(
       AlternativeReadingScope _(reader_);
       body += BuildArgumentTypeChecks();
     } else {
-      // Check if target function was annotated with no-dynamic-invocations.
+      // Check if parent function was annotated with no-dynamic-invocations.
       const ProcedureAttributesMetadata attrs =
           procedure_attributes_metadata_helper_.GetProcedureAttributes(
-              target.kernel_offset());
+              parent.kernel_offset());
       if (!attrs.has_dynamic_invocations) {
         // If it was then we might need to build some checks in the
         // tear-off.
@@ -4347,9 +4364,11 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfImplicitClosureFunction(
     }
   }
 
-  // Forward them to the target.
+  // Forward them to the parent.
   intptr_t argument_count = positional_argument_count + named_argument_count;
-  if (!target.is_static()) ++argument_count;
+  if (!parent.is_static()) {
+    ++argument_count;
+  }
   body += StaticCall(TokenPosition::kNoSource, target, argument_count,
                      argument_names, ICData::kNoRebind,
                      /* result_type = */ NULL, type_args_len);
@@ -4362,9 +4381,13 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfImplicitClosureFunction(
                 flow_graph_builder_->last_used_block_id_, prologue_info);
 }
 
+// If throw_no_such_method_error is set to true (defaults to false), an
+// instance of NoSuchMethodError is thrown. Otherwise, the instance
+// noSuchMethod is called.
 FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder(
     const Function& function,
-    bool is_implicit_closure_function) {
+    bool is_implicit_closure_function,
+    bool throw_no_such_method_error) {
   // The prologue builder needs the default parameter values.
   SetupDefaultParameterValues();
 
@@ -4515,8 +4538,26 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder(
 
   // Load receiver.
   if (is_implicit_closure_function) {
-    body += LoadLocal(parsed_function()->current_context_var());
-    body += B->LoadField(Context::variable_offset(0));
+    if (throw_no_such_method_error) {
+      const Function& parent =
+          Function::ZoneHandle(Z, function.parent_function());
+      const Class& owner = Class::ZoneHandle(Z, parent.Owner());
+      AbstractType& type = AbstractType::ZoneHandle(Z);
+      type ^= Type::New(owner, TypeArguments::Handle(Z), owner.token_pos(),
+                        Heap::kOld);
+      // If the current class is the result of a mixin application, we must
+      // use the class scope of the class from which the function originates.
+      if (owner.IsMixinApplication()) {
+        ClassFinalizer::FinalizeType(
+            Class::Handle(Z, parsed_function()->function().origin()), type);
+      } else {
+        type ^= ClassFinalizer::FinalizeType(owner, type);
+      }
+      body += Constant(type);
+    } else {
+      body += LoadLocal(parsed_function()->current_context_var());
+      body += B->LoadField(Context::variable_offset(0));
+    }
   } else {
     LocalScope* scope = parsed_function()->node_sequence()->scope();
     body += LoadLocal(scope->VariableAt(0));
@@ -4544,6 +4585,28 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder(
   body += Constant(Bool::False());
   body += PushArgument();
 
+  if (throw_no_such_method_error) {
+    const Function& parent =
+        Function::ZoneHandle(Z, function.parent_function());
+    const Class& owner = Class::ZoneHandle(Z, parent.Owner());
+    InvocationMirror::Level im_level = owner.IsTopLevel()
+                                           ? InvocationMirror::kTopLevel
+                                           : InvocationMirror::kStatic;
+    InvocationMirror::Kind im_kind;
+    if (function.IsImplicitGetterFunction() || function.IsGetterFunction()) {
+      im_kind = InvocationMirror::kGetter;
+    } else if (function.IsImplicitSetterFunction() ||
+               function.IsSetterFunction()) {
+      im_kind = InvocationMirror::kSetter;
+    } else {
+      im_kind = InvocationMirror::kMethod;
+    }
+    body += IntConstant(InvocationMirror::EncodeType(im_level, im_kind));
+  } else {
+    body += NullConstant();
+  }
+  body += PushArgument();
+
   const Class& mirror_class =
       Class::Handle(Z, Library::LookupCoreClass(Symbols::InvocationMirror()));
   ASSERT(!mirror_class.IsNull());
@@ -4552,11 +4615,23 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraphOfNoSuchMethodForwarder(
              Library::PrivateCoreLibName(Symbols::AllocateInvocationMirror())));
   ASSERT(!allocation_function.IsNull());
   body += StaticCall(TokenPosition::kMinSource, allocation_function,
-                     /* argument_count = */ 4, ICData::kStatic);
+                     /* argument_count = */ 5, ICData::kStatic);
   body += PushArgument();  // For the call to noSuchMethod.
 
-  body += InstanceCall(TokenPosition::kNoSource, Symbols::NoSuchMethod(),
-                       Token::kILLEGAL, 2, 1);
+  if (throw_no_such_method_error) {
+    const Class& klass = Class::ZoneHandle(
+        Z, Library::LookupCoreClass(Symbols::NoSuchMethodError()));
+    ASSERT(!klass.IsNull());
+    const Function& throw_function = Function::ZoneHandle(
+        Z,
+        klass.LookupStaticFunctionAllowPrivate(Symbols::ThrowNewInvocation()));
+    ASSERT(!throw_function.IsNull());
+    body += StaticCall(TokenPosition::kNoSource, throw_function, 2,
+                       ICData::kStatic);
+  } else {
+    body += InstanceCall(TokenPosition::kNoSource, Symbols::NoSuchMethod(),
+                         Token::kILLEGAL, 2, 1);
+  }
   body += StoreLocal(TokenPosition::kNoSource, result);
   body += Drop();
 

runtime/vm/compiler/frontend/kernel_binary_flowgraph.h

@@ -1121,7 +1121,8 @@ class StreamingFlowGraphBuilder {
   FlowGraph* BuildGraphOfFunction(bool constructor);
   FlowGraph* BuildGraphOfNoSuchMethodForwarder(
       const Function& function,
-      bool is_implicit_closure_function);
+      bool is_implicit_closure_function,
+      bool throw_no_such_method_error = false);
 
   intptr_t GetOffsetForSourceInfo(intptr_t index);
 

runtime/vm/isolate_reload_test.cc

@@ -1527,6 +1527,99 @@ TEST_CASE(IsolateReload_TearOff_Instance_Equality) {
   EXPECT_NON_NULL(lib);
 }
 
+TEST_CASE(IsolateReload_TearOff_Parameter_Count_Mismatch) {
+  const char* kScript =
+      "import 'file:///test:isolate_reload_helper';\n"
+      "class C {\n"
+      "  static foo() => 'old';\n"
+      "}\n"
+      "main() {\n"
+      "  var f1 = C.foo;\n"
+      "  reloadTest();\n"
+      "  return f1();\n"
+      "}\n";
+
+  Dart_Handle lib = TestCase::LoadTestScript(kScript, NULL);
+  EXPECT_VALID(lib);
+
+  const char* kReloadScript =
+      "import 'file:///test:isolate_reload_helper';\n"
+      "class C {\n"
+      "  static foo(i) => 'new:$i';\n"
+      "}\n"
+      "main() {\n"
+      "  var f1 = C.foo;\n"
+      "  reloadTest();\n"
+      "  return f1();\n"
+      "}\n";
+
+  TestCase::SetReloadTestScript(kReloadScript);
+
+  Dart_Handle error_handle = SimpleInvokeError(lib, "main");
+
+  const char* error;
+  if (TestCase::UsingDartFrontend()) {
+    error =
+        "NoSuchMethodError: Closure call with mismatched arguments: function "
+        "'C.foo'\n"
+        "Receiver: Closure: (dynamic) => dynamic from Function 'foo': static.\n"
+        "Tried calling: C.foo()\n"
+        "Found: C.foo(dynamic) => dynamic\n"
+        "#0      Object.noSuchMethod "
+        "(dart:core/runtime/libobject_patch.dart:46:5)\n"
+        "#1      main (file:///test-lib:8:12)";
+  } else {
+    error =
+        "NoSuchMethodError: Closure call with mismatched arguments: function "
+        "'C.foo'\n"
+        "Receiver: Closure: (dynamic) => dynamic from Function 'foo': static.\n"
+        "Tried calling: C.foo()\n"
+        "Found: C.foo(dynamic) => dynamic\n"
+        "#0      Object.noSuchMethod "
+        "(dart:core-patch/dart:core/object_patch.dart:46)\n"
+        "#1      main (test-lib:8:12)";
+  }
+  EXPECT_ERROR(error_handle, error);
+}
+
+TEST_CASE(IsolateReload_TearOff_Remove) {
+  const char* kScript =
+      "import 'file:///test:isolate_reload_helper';\n"
+      "class C {\n"
+      "  static foo({String bar: 'bar'}) => 'old';\n"
+      "}\n"
+      "main() {\n"
+      "  var f1 = C.foo;\n"
+      "  reloadTest();\n"
+      "  try {\n"
+      "    return f1();\n"
+      "  } catch(e) { return '$e'; }\n"
+      "}\n";
+
+  Dart_Handle lib = TestCase::LoadTestScript(kScript, NULL);
+  EXPECT_VALID(lib);
+
+  const char* kReloadScript =
+      "import 'file:///test:isolate_reload_helper';\n"
+      "class C {\n"
+      "}\n"
+      "main() {\n"
+      "  var f1;\n"
+      "  reloadTest();\n"
+      "  try {\n"
+      "    return f1();\n"
+      "  } catch(e) { return '$e'; }\n"
+      "}\n";
+
+  TestCase::SetReloadTestScript(kReloadScript);
+
+  EXPECT_SUBSTRING(
+      "NoSuchMethodError: No static method 'foo' declared in class 'C'.",
+      SimpleInvokeStr(lib, "main"));
+
+  lib = TestCase::GetReloadLibrary();
+  EXPECT_NON_NULL(lib);
+}
 TEST_CASE(IsolateReload_TearOff_Class_Identity) {
   const char* kScript =
       "import 'file:///test:isolate_reload_helper';\n"

runtime/vm/symbols.h

@@ -76,6 +76,7 @@ class ObjectPointerVisitor;
   V(CyclicInitializationError, "CyclicInitializationError")                    \
   V(_CompileTimeError, "_CompileTimeError")                                    \
   V(ThrowNew, "_throwNew")                                                     \
+  V(ThrowNewInvocation, "_throwNewInvocation")                                 \
   V(ThrowNewIfNotLoaded, "_throwNewIfNotLoaded")                               \
   V(EvaluateAssertion, "_evaluateAssertion")                                   \
   V(Symbol, "Symbol")                                                          \