[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 https://github.com/dotnet/runtime/issues/48512

* oops, disable vtable tracing

* Use explicit variant interface to implement base type interfaces

diff --git a/src/mono/mono/metadata/class-setup-vtable.c b/src/mono/mono/metadata/class-setup-vtable.c
index 0c30269..ae1727e 100644 (file)
--- a/src/mono/mono/metadata/class-setup-vtable.c
+++ b/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) {