From: Aleksey Kliger (λgeek) Date: Tue, 17 Aug 2021 14:23:31 +0000 (-0400) Subject: [mono] Check for additional implemented variant interfaces (#57086) X-Git-Tag: accepted/tizen/unified/20220110.054933~339 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=eeee548ba69083cce4c2a8cfb9f7222355b2d1a0;p=platform%2Fupstream%2Fdotnet%2Fruntime.git [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 { T Get(); } public class Foo {} public class Bar : Foo {} public class FooFactory : IFactory { public Foo Get() => new Foo(); } public class BarFactory : FooFactory, IFactory { public new Bar Get() => new Bar(); } In this case, BarFactory explicitly implements IFactory and also IFactory. Conversely for contravariant gparams: interface ITaker { string Consume (T x); } class Foo {} class Bar : Foo {} class BarTaker : ITaker { public string Consume (Bar x) => "consumed Bar"; } class FooTaker : BarTaker, ITaker { public string Consume (Foo x) => "consumed Foo"; } In this case FooTaker implements ITaker but also ITaker. When this happens, the signature of the implementing method 'Bar BarFactory:Get()' doesn't match the signature of the implemented interface method 'Foo IFactory: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 --- 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 { T Get(); } + * public class Foo {} + * public class Bar : Foo {} + * public class FooFactory : IFactory { public Foo Get() => new Foo(); } + * public class BarFactory : FooFactory, IFactory { 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: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) {