[vm] Correct semantics of bounds-checks on partial instantiation.

Fixes #34267.

Change-Id: Ieb557e186aa457bac3b99b6817050bde8f8e142f
Reviewed-on: https://dart-review.googlesource.com/72161
Commit-Queue: Samir Jindel <[email protected]>
Reviewed-by: Alexander Markov <[email protected]>

Author: sjindel-google

runtime/lib/internal_patch.dart

@@ -102,6 +102,12 @@ class Lists {
 _prependTypeArguments(functionTypeArguments, parentTypeArguments, parentLen,
     totalLen) native "Internal_prependTypeArguments";
 
+// Check that a set of type arguments satisfy the type parameter bounds on a
+// closure.
+@pragma("vm:entry-point")
+_boundsCheckForPartialInstantiation(closure, type_args)
+    native "Internal_boundsCheckForPartialInstantiation";
+
 // Called by IRRegExpMacroAssembler::GrowStack.
 Int32List _growRegExpStack(Int32List stack) {
   final newStack = new Int32List(stack.length * 2);

runtime/lib/object.cc

@@ -473,6 +473,72 @@ DEFINE_NATIVE_ENTRY(Internal_prependTypeArguments, 4) {
       zone, parent_type_arguments, smi_parent_len.Value(), smi_len.Value());
 }
 
+// Check that a set of type arguments satisfy the type parameter bounds on a
+// closure.
+// Arg0: Closure object
+// Arg1: Type arguments to function
+DEFINE_NATIVE_ENTRY(Internal_boundsCheckForPartialInstantiation, 2) {
+  const Closure& closure =
+      Closure::CheckedHandle(zone, arguments->NativeArgAt(0));
+  const Function& target = Function::Handle(zone, closure.function());
+  const TypeArguments& bounds =
+      TypeArguments::Handle(zone, target.type_parameters());
+
+  // Either the bounds are all-dynamic or the function is not generic.
+  if (bounds.IsNull()) return Object::null();
+
+  const TypeArguments& type_args_to_check =
+      TypeArguments::CheckedHandle(zone, arguments->NativeArgAt(1));
+
+  // This should be guaranteed by the front-end.
+  ASSERT(type_args_to_check.IsNull() ||
+         bounds.Length() == type_args_to_check.Length());
+
+  // The bounds on the closure may need instantiation.
+  const TypeArguments& instantiator_type_args =
+      TypeArguments::Handle(zone, closure.instantiator_type_arguments());
+  const TypeArguments& function_type_args =
+      TypeArguments::Handle(zone, closure.function_type_arguments());
+
+  AbstractType& supertype = AbstractType::Handle(zone);
+  AbstractType& subtype = AbstractType::Handle(zone);
+  TypeParameter& parameter = TypeParameter::Handle(zone);
+  for (intptr_t i = 0; i < bounds.Length(); ++i) {
+    parameter ^= bounds.TypeAt(i);
+    supertype = parameter.bound();
+    subtype = type_args_to_check.TypeAt(i);
+
+    ASSERT(!subtype.IsNull() && !subtype.IsMalformedOrMalbounded());
+    ASSERT(!supertype.IsNull() && !supertype.IsMalformedOrMalbounded());
+
+    // The supertype may not be instantiated.
+    Error& bound_error = Error::Handle(zone);
+    if (!AbstractType::InstantiateAndTestSubtype(
+            &subtype, &supertype, &bound_error, instantiator_type_args,
+            function_type_args)) {
+      // Throw a dynamic type error.
+      TokenPosition location;
+      {
+        DartFrameIterator iterator(Thread::Current(),
+                                   StackFrameIterator::kNoCrossThreadIteration);
+        StackFrame* caller_frame = iterator.NextFrame();
+        ASSERT(caller_frame != NULL);
+        location = caller_frame->GetTokenPos();
+      }
+      String& bound_error_message = String::Handle(zone);
+      if (!bound_error.IsNull()) {
+        bound_error_message = String::New(bound_error.ToErrorCString());
+      }
+      String& parameter_name = String::Handle(zone, parameter.Name());
+      Exceptions::CreateAndThrowTypeError(location, subtype, supertype,
+                                          parameter_name, bound_error_message);
+      UNREACHABLE();
+    }
+  }
+
+  return Object::null();
+}
+
 DEFINE_NATIVE_ENTRY(InvocationMirror_unpackTypeArguments, 2) {
   const TypeArguments& type_arguments =
       TypeArguments::CheckedHandle(zone, arguments->NativeArgAt(0));

runtime/vm/bootstrap_natives.h

@@ -313,6 +313,7 @@ namespace dart {
   V(Internal_inquireIs64Bit, 0)                                                \
   V(Internal_extractTypeArguments, 2)                                          \
   V(Internal_prependTypeArguments, 4)                                          \
+  V(Internal_boundsCheckForPartialInstantiation, 2)                            \
   V(InvocationMirror_unpackTypeArguments, 2)                                   \
   V(NoSuchMethodError_existingMethodSignature, 3)                              \
   V(LinkedHashMap_getIndex, 1)                                                 \

runtime/vm/compiler/frontend/kernel_binary_flowgraph.cc

@@ -4745,14 +4745,35 @@ Fragment StreamingFlowGraphBuilder::BuildPartialTearoffInstantiation(
       Class::ZoneHandle(Z, I->object_store()->closure_class()), 0);
   LocalVariable* new_closure = MakeTemporary();
 
-  instructions += LoadLocal(new_closure);
-
   intptr_t num_type_args = ReadListLength();
   const TypeArguments& type_args = T.BuildTypeArguments(num_type_args);
   instructions += TranslateInstantiatedTypeArguments(type_args);
+  LocalVariable* type_args_vec = MakeTemporary();
+
+  // Check the bounds.
+  //
+  // TODO(sjindel): Only perform this check for instance tearoffs, not for
+  // tearoffs against local or top-level functions.
+  instructions += LoadLocal(original_closure);
+  instructions += PushArgument();
+  instructions += LoadLocal(type_args_vec);
+  instructions += PushArgument();
+  const Library& dart_internal = Library::Handle(Z, Library::InternalLibrary());
+  const Function& bounds_check_function = Function::ZoneHandle(
+      Z, dart_internal.LookupFunctionAllowPrivate(
+             Symbols::BoundsCheckForPartialInstantiation()));
+  ASSERT(!bounds_check_function.IsNull());
+  instructions += StaticCall(TokenPosition::kNoSource, bounds_check_function, 2,
+                             ICData::kStatic);
+  instructions += Drop();
+
+  instructions += LoadLocal(new_closure);
+  instructions += LoadLocal(type_args_vec);
   instructions += StoreInstanceField(TokenPosition::kNoSource,
                                      Closure::delayed_type_arguments_offset());
 
+  instructions += Drop();  // Drop type args.
+
   // Copy over the target function.
   instructions += LoadLocal(new_closure);
   instructions += LoadLocal(original_closure);
@@ -4781,7 +4802,7 @@ Fragment StreamingFlowGraphBuilder::BuildPartialTearoffInstantiation(
   instructions +=
       StoreInstanceField(TokenPosition::kNoSource, Closure::context_offset());
 
-  instructions += DropTempsPreserveTop(1);  // drop old closure
+  instructions += DropTempsPreserveTop(1);  // Drop old closure.
 
   return instructions;
 }

runtime/vm/symbols.h

@@ -465,7 +465,8 @@ class ObjectPointerVisitor;
   V(vm_entry_point, "vm:entry-point")                                          \
   V(Get, "get")                                                                \
   V(Set, "set")                                                                \
-  V(vm_trace_entrypoints, "vm:testing.unsafe.trace-entrypoints-fn")
+  V(vm_trace_entrypoints, "vm:testing.unsafe.trace-entrypoints-fn")            \
+  V(BoundsCheckForPartialInstantiation, "_boundsCheckForPartialInstantiation")
 
 // Contains a list of frequently used strings in a canonicalized form. This
 // list is kept in the vm_isolate in order to share the copy across isolates

tests/language_2/language_2_dart2js.status

@@ -67,6 +67,7 @@ mixin_type_parameter_inference_test/13: CompileTimeError
 mixin_type_parameter_inference_test/16: CompileTimeError
 mixin_type_parameter_inference_test/none: CompileTimeError
 number_identity_test: CompileTimeError, OK # Error if web int literal cannot be represented exactly, see http://dartbug.com/33351
+partial_instantiation_eager_bounds_check_test: RuntimeError # Issue #34295
 partial_tearoff_instantiation_test/05: Pass # for the wrong reason.
 partial_tearoff_instantiation_test/06: Pass # for the wrong reason.
 partial_tearoff_instantiation_test/07: Pass # for the wrong reason.

tests/language_2/language_2_dartdevc.status

@@ -495,6 +495,7 @@ number_identity2_test: RuntimeError # Issue 29920; Expect.isTrue(false) fails.
 number_identity_test: CompileTimeError, OK # Error if web int literal cannot be represented exactly, see http://dartbug.com/33351
 numbers_test: RuntimeError # Issue 29920; Expect.equals(expected: <false>, actual: <true>) fails.
 parser_quirks_test: CompileTimeError
+partial_instantiation_eager_bounds_check_test: RuntimeError # Issue 34296
 regress_16640_test: RuntimeError # Issue 29920; Uncaught Error: type arguments should not be null: E => {
 regress_22443_test: RuntimeError # Uncaught Expect.isTrue(false) fails.
 stack_overflow_stacktrace_test: RuntimeError # Issue 29920; RangeError: Maximum call stack size exceeded

tests/language_2/partial_instantiation_eager_bounds_check_test.dart

@@ -0,0 +1,21 @@
+// Copyright (c) 2018, 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.
+//
+// This test checks that necessary type argument bounds checks are performed
+// eagerly during partial instantiation, rather than being delayed until the
+// partially instantiated closure is invoked.
+
+import "package:expect/expect.dart";
+
+class C<T> {
+  void foo<S extends T>(S x) {}
+}
+
+void main() {
+  C<Object> c = C<int>();
+  void Function(String) fn;
+  Expect.throwsTypeError(() {
+    fn = c.foo;
+  });
+}