[mono] Check for additional implemented variant interfaces (#57086)

* [class-setup-vtable] Use flags bitmask in check_interface_method_override

No funcitonal change yet.

* [class-setup-vtable] Check additional slots when explicitly implementing variant interfaces

If a class implements a variant interface, consider whether it is explicitly
implementing (as opposed to obtaining by being a subclass of some base class)
some variant interfaces.

Two examples:
public interface IFactory<out T> { T Get(); }
public class Foo {}
public class Bar : Foo {}
public class FooFactory : IFactory<Foo> { public Foo Get() => new Foo(); }
public class BarFactory : FooFactory, IFactory<Bar> { public new Bar Get() => new Bar(); }

In this case, BarFactory explicitly implements IFactory<Bar> and also
IFactory<Foo>.

Conversely for contravariant gparams:
interface ITaker<in T> { string Consume (T x); }
class Foo {}
class Bar : Foo {}
class BarTaker : ITaker<Bar> { public string Consume (Bar x) => "consumed Bar"; }
class FooTaker : BarTaker, ITaker<Foo> { public string Consume (Foo x) =>
"consumed Foo"; }

In this case FooTaker implements ITaker<Foo> but also ITaker<Bar>.

When this happens, the signature of the implementing method 'Bar
BarFactory:Get()' doesn't match the signature of the implemented interface
method 'Foo IFactory<Foo>:Get()'.

We should check the signature parameters of the candidate method and the
implemented method, but I think the interface setup code already checks this
for us.

Fixes #48512

* oops, disable vtable tracing

* Use explicit variant interface to implement base type interfaces

src/mono/mono/metadata/class-setup-vtable.c

@@ -447,9 +447,42 @@ is_wcf_hack_disabled (void)
 	return disabled == 1;
 }
 
+enum MonoInterfaceMethodOverrideFlags {
+	MONO_ITF_OVERRIDE_REQUIRE_NEWSLOT = 0x01,
+	MONO_ITF_OVERRIDE_EXPLICITLY_IMPLEMENTED = 0x02,
+	MONO_ITF_OVERRIDE_SLOT_EMPTY = 0x04,
+	MONO_ITF_OVERRIDE_VARIANT_ITF = 0x08,
+};
+
+static gboolean
+signature_is_subsumed (MonoMethod *impl_method, MonoMethod *decl_method, MonoError *error);
+
+/*
+ * Returns TRUE if the signature of \c decl is assignable from the signature of \c impl.  That is, if \c sig_impl is more
+ * specific than \c sig_decl.
+ */
 static gboolean
-check_interface_method_override (MonoClass *klass, MonoMethod *im, MonoMethod *cm, gboolean require_newslot, gboolean interface_is_explicitly_implemented_by_class, gboolean slot_is_empty)
+signature_assignable_from (MonoMethod *decl, MonoMethod *impl)
 {
+	ERROR_DECL (error);
+	/* FIXME: the "signature_is_subsumed" check for covariant returns is not good enough:
+	 * 1. It doesn't check that the arguments are contravariant.
+	 * 2. it's too general: we don't want  Foo SomeMethod() to be subsumed by Bar SomeMethod() unless
+	 *    at least of Foo or Bar was a generic parameter.
+	 */
+	if (signature_is_subsumed (impl, decl, error))
+		return TRUE;
+	mono_error_cleanup (error);
+	return FALSE;
+}
+
+static gboolean
+check_interface_method_override (MonoClass *klass, MonoMethod *im, MonoMethod *cm, int flags)
+{
+	gboolean require_newslot = (flags & MONO_ITF_OVERRIDE_REQUIRE_NEWSLOT) != 0;
+	gboolean interface_is_explicitly_implemented_by_class = (flags & MONO_ITF_OVERRIDE_EXPLICITLY_IMPLEMENTED) != 0;
+	gboolean slot_is_empty = (flags & MONO_ITF_OVERRIDE_SLOT_EMPTY) != 0;
+	gboolean variant_itf = (flags & MONO_ITF_OVERRIDE_VARIANT_ITF) != 0;
 	MonoMethodSignature *cmsig, *imsig;
 	if (strcmp (im->name, cm->name) == 0) {
 		if (! (cm->flags & METHOD_ATTRIBUTE_PUBLIC)) {
@@ -477,7 +510,19 @@ check_interface_method_override (MonoClass *klass, MonoMethod *im, MonoMethod *c
 			return FALSE;
 		}
 
-		if (! mono_metadata_signature_equal (cmsig, imsig)) {
+		/* if there's a variant interface, the method could be using the generic param in a
+		 * variant position compared to the interface sig
+		 *
+		 * public interface IFactory<out T> { T Get(); }
+		 * public class Foo {}
+		 * public class Bar : Foo {}
+		 * public class FooFactory : IFactory<Foo> { public Foo Get() => new Foo(); }
+		 * public class BarFactory : FooFactory, IFactory<Bar> { public new Bar Get() => new Bar(); }
+		 *
+		 * In this case, there's an explicit newslot but we want to match up 'Bar BarFactory:Get ()'
+		 * with 'Foo IFactory<Foo>:Get ()'.
+		 */
+		if (! mono_metadata_signature_equal (cmsig, imsig) && !(variant_itf && signature_assignable_from (im, cm))) {
 			TRACE_INTERFACE_VTABLE (printf ("[SIGNATURE CHECK FAILED  "));
 			TRACE_INTERFACE_VTABLE (print_method_signatures (im, cm));
 			TRACE_INTERFACE_VTABLE (printf ("]"));
@@ -1744,6 +1789,7 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
 		MonoClass *parent = klass->parent;
 		int ic_offset;
 		gboolean interface_is_explicitly_implemented_by_class;
+		gboolean variant_itf;
 		int im_index;
 
 		ic = klass->interfaces_packed [i];
@@ -1757,11 +1803,21 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
 		if (parent != NULL) {
 			int implemented_interfaces_index;
 			interface_is_explicitly_implemented_by_class = FALSE;
+			variant_itf = mono_class_has_variant_generic_params (ic);
 			for (implemented_interfaces_index = 0; implemented_interfaces_index < klass->interface_count; implemented_interfaces_index++) {
 				if (ic == klass->interfaces [implemented_interfaces_index]) {
 					interface_is_explicitly_implemented_by_class = TRUE;
 					break;
 				}
+				if (variant_itf) {
+					if (mono_class_is_variant_compatible (ic, klass->interfaces [implemented_interfaces_index], FALSE)) {
+						MonoClass *impl_itf;
+						impl_itf = klass->interfaces [implemented_interfaces_index];
+						TRACE_INTERFACE_VTABLE (printf ("  variant interface '%s' is explicitly implemented by '%s'\n", mono_type_full_name (m_class_get_byval_arg (ic)), mono_type_full_name (m_class_get_byval_arg (impl_itf))));
+						interface_is_explicitly_implemented_by_class = TRUE;
+						break;
+					}
+				}
 			}
 		} else {
 			interface_is_explicitly_implemented_by_class = TRUE;
@@ -1787,8 +1843,16 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
 				for (l = virt_methods; l; l = l->next) {
 					cm = (MonoMethod *)l->data;
 					TRACE_INTERFACE_VTABLE (printf ("    For slot %d ('%s'.'%s':'%s'), trying method '%s'.'%s':'%s'... [EXPLICIT IMPLEMENTATION = %d][SLOT IS NULL = %d]", im_slot, ic->name_space, ic->name, im->name, cm->klass->name_space, cm->klass->name, cm->name, interface_is_explicitly_implemented_by_class, (vtable [im_slot] == NULL)));
-					if (check_interface_method_override (klass, im, cm, TRUE, interface_is_explicitly_implemented_by_class, (vtable [im_slot] == NULL))) {
-						TRACE_INTERFACE_VTABLE (printf ("[check ok]: ASSIGNING"));
+					int flags;
+					flags = MONO_ITF_OVERRIDE_REQUIRE_NEWSLOT;
+					if (interface_is_explicitly_implemented_by_class)
+						flags |= MONO_ITF_OVERRIDE_EXPLICITLY_IMPLEMENTED;
+					if (interface_is_explicitly_implemented_by_class && variant_itf)
+						flags |= MONO_ITF_OVERRIDE_VARIANT_ITF;
+					if (vtable [im_slot] == NULL)
+						flags |= MONO_ITF_OVERRIDE_SLOT_EMPTY;
+					if (check_interface_method_override (klass, im, cm, flags)) {
+						TRACE_INTERFACE_VTABLE (printf ("[check ok]: ASSIGNING\n"));
 						vtable [im_slot] = cm;
 						/* Why do we need this? */
 						if (cm->slot < 0) {
@@ -1811,8 +1875,8 @@ mono_class_setup_vtable_general (MonoClass *klass, MonoMethod **overrides, int o
 						MonoMethod *cm = parent->vtable [cm_index];
 
 						TRACE_INTERFACE_VTABLE ((cm != NULL) && printf ("    For slot %d ('%s'.'%s':'%s'), trying (ancestor) method '%s'.'%s':'%s'... ", im_slot, ic->name_space, ic->name, im->name, cm->klass->name_space, cm->klass->name, cm->name));
-						if ((cm != NULL) && check_interface_method_override (klass, im, cm, FALSE, FALSE, TRUE)) {
-							TRACE_INTERFACE_VTABLE (printf ("[everything ok]: ASSIGNING"));
+						if ((cm != NULL) && check_interface_method_override (klass, im, cm, MONO_ITF_OVERRIDE_SLOT_EMPTY)) {
+							TRACE_INTERFACE_VTABLE (printf ("[everything ok]: ASSIGNING\n"));
 							vtable [im_slot] = cm;
 							/* Why do we need this? */
 							if (cm->slot < 0) {