From 4e03f3665ad197b005b361b3b7cdf7fcac298ef2 Mon Sep 17 00:00:00 2001 From: =?utf8?q?Aleksey=20Kliger=20=28=CE=BBgeek=29?= Date: Tue, 15 Jun 2021 14:56:58 -0400 Subject: [PATCH] Don't return null target methods in GetInterfaceMap for abstract classes, or classes using DIMs (#53972) * Add DIM tests for GetInterfaceMap Regression tests for https://github.com/dotnet/runtime/issues/53933 * Return null target methods from GetInterfaceMap in fewer cases If the iterface method is reabstracted, and either the found implementation method is abstract, or the found implementation method is from another DIM (meaning neither klass nor any of its ancestor classes implemented the method), then say the target method is null. Otherwise return the found implementation method, even if it is abstract, unless we found a reabstracted method in a non-abstract class Fixes https://github.com/dotnet/runtime/issues/53933 --- .../System.Runtime/tests/System/Type/TypeTests.cs | 113 ++++++++++++++++++++- src/mono/mono/metadata/icall.c | 38 ++++++- 2 files changed, 147 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.Runtime/tests/System/Type/TypeTests.cs b/src/libraries/System.Runtime/tests/System/Type/TypeTests.cs index 1464db6..8f2d141 100644 --- a/src/libraries/System.Runtime/tests/System/Type/TypeTests.cs +++ b/src/libraries/System.Runtime/tests/System/Type/TypeTests.cs @@ -938,6 +938,16 @@ namespace System.Tests }; yield return new object[] { + typeof(ISimpleInterface), + typeof(AbstractSimpleType), + new Tuple[] + { + new Tuple(typeof(ISimpleInterface).GetMethod("Method"), typeof(AbstractSimpleType).GetMethod("Method")), + new Tuple(typeof(ISimpleInterface).GetMethod("GenericMethod"), typeof(AbstractSimpleType).GetMethod("GenericMethod")) + } + }; + yield return new object[] + { typeof(IGenericInterface), typeof(DerivedType), new Tuple[] @@ -954,12 +964,64 @@ namespace System.Tests new Tuple(typeof(IGenericInterface).GetMethod("Method"), typeof(DerivedType).GetMethod("Method", new Type[] { typeof(string) })), } }; + yield return new object[] + { + typeof(DIMs.I1), + typeof(DIMs.C1), + new Tuple[] + { + new Tuple(typeof(DIMs.I1).GetMethod("M"), typeof(DIMs.I1).GetMethod("M")) + } + }; + yield return new object[] + { + typeof(DIMs.I2), + typeof(DIMs.C2), + new Tuple[] + { + new Tuple(typeof(DIMs.I2).GetMethod("System.Tests.TypeTestsExtended.DIMs.I1.M", BindingFlags.Instance | BindingFlags.NonPublic), null) + } + }; + yield return new object[] + { + typeof(DIMs.I1), + typeof(DIMs.C2), + new Tuple[] + { + new Tuple(typeof(DIMs.I1).GetMethod("M"), typeof(DIMs.C2).GetMethod("M")) + } + }; + yield return new object[] + { + typeof(DIMs.I3), + typeof(DIMs.C3), + new Tuple[] + { + new Tuple(typeof(DIMs.I3).GetMethod("System.Tests.TypeTestsExtended.DIMs.I1.M", BindingFlags.Instance | BindingFlags.NonPublic), typeof(DIMs.I3).GetMethod("System.Tests.TypeTestsExtended.DIMs.I1.M", BindingFlags.Instance | BindingFlags.NonPublic)) + } + }; + yield return new object[] + { + typeof(DIMs.I4), + typeof(DIMs.C4), + new Tuple[] + { + new Tuple(typeof(DIMs.I4).GetMethod("System.Tests.TypeTestsExtended.DIMs.I1.M", BindingFlags.Instance | BindingFlags.NonPublic), null) + } + }; + yield return new object[] + { + typeof(DIMs.I2), + typeof(DIMs.C4), + new Tuple[] + { + new Tuple(typeof(DIMs.I2).GetMethod("System.Tests.TypeTestsExtended.DIMs.I1.M", BindingFlags.Instance | BindingFlags.NonPublic), null) + } + }; } [Theory] [MemberData(nameof(GetInterfaceMap_TestData))] - // Android-only, change to TestPlatforms.Android once arcade dependency is updated - [ActiveIssue("https://github.com/dotnet/runtime/issues/36653", TestRuntimes.Mono)] public void GetInterfaceMap(Type interfaceType, Type classType, Tuple[] expectedMap) { InterfaceMapping actualMapping = classType.GetInterfaceMap(interfaceType); @@ -991,6 +1053,12 @@ namespace System.Tests public void GenericMethod() { } } + abstract class AbstractSimpleType : ISimpleInterface + { + public abstract void Method(); + public abstract void GenericMethod(); + } + interface IGenericInterface { void Method(T arg); @@ -1005,6 +1073,47 @@ namespace System.Tests { public void Method(string arg) { } } + + static class DIMs + { + + internal interface I1 + { + void M() { throw new Exception("e"); } + } + + internal class C1 : I1 { } + + internal interface I2 : I1 + { + abstract void I1.M(); // reabstracted + } + + internal abstract class C2 : I2 + { + public abstract void M(); + } + + internal interface I3 : I2 + { + void I1.M() + { // unabstacted + throw new Exception ("e"); + } + } + + internal class C3 : I3 { } + + internal interface I4 : I3 + { + abstract void I1.M(); // reabstracted again + } + + internal abstract class C4 : I4 + { + public abstract void M(); + } + } #endregion } diff --git a/src/mono/mono/metadata/icall.c b/src/mono/mono/metadata/icall.c index f9dacf6..f1d0d7c 100644 --- a/src/mono/mono/metadata/icall.c +++ b/src/mono/mono/metadata/icall.c @@ -2527,6 +2527,23 @@ fail: } static gboolean +method_is_reabstracted (MonoMethod *method) +{ + /* only on interfaces */ + /* method is marked "final abstract" */ + /* FIXME: we need some other way to detect reabstracted methods. "final" is an incidental detail of the spec. */ + return method->flags & METHOD_ATTRIBUTE_FINAL && method->flags & METHOD_ATTRIBUTE_ABSTRACT; +} + +static gboolean +method_is_dim (MonoMethod *method) +{ + /* only valid on interface methods*/ + /* method is marked "virtual" but not "virtual abstract" */ + return method->flags & method->flags & METHOD_ATTRIBUTE_VIRTUAL && !(method->flags & METHOD_ATTRIBUTE_ABSTRACT); +} + +static gboolean set_interface_map_data_method_object (MonoMethod *method, MonoClass *iclass, int ioffset, MonoClass *klass, MonoArrayHandle targets, MonoArrayHandle methods, int i, MonoError *error) { HANDLE_FUNCTION_ENTER (); @@ -2559,9 +2576,26 @@ set_interface_map_data_method_object (MonoMethod *method, MonoClass *iclass, int } } - if (foundMethod->flags & METHOD_ATTRIBUTE_ABSTRACT) + /* + * if the iterface method is reabstracted, and either the found implementation method is abstract, or the found + * implementation method is from another DIM (meaning neither klass nor any of its ancestor classes implemented + * the method), then say the target method is null. + */ + if (method_is_reabstracted (method) && + ((foundMethod->flags & METHOD_ATTRIBUTE_ABSTRACT) || + (mono_class_is_interface (foundMethod->klass) && method_is_dim (foundMethod)) + )) MONO_HANDLE_ARRAY_SETREF (targets, i, NULL_HANDLE); - else { + else if (mono_class_is_interface (foundMethod->klass) && method_is_reabstracted (foundMethod) && !m_class_is_abstract (klass)) { + /* if the method we found is a reabstracted DIM method, but the class isn't abstract, return NULL */ + /* + * (C# doesn't seem to allow constructing such types, it requires the whole class to be abstract - in + * which case we are supposed to return the reabstracted interface method. But in IL we can make a + * non-abstract class with reabstracted interface methods - which is supposed to fail with an + * EntryPointNotFoundException at invoke time, but does not prevent the class from loading.) + */ + MONO_HANDLE_ARRAY_SETREF (targets, i, NULL_HANDLE); + } else { MONO_HANDLE_ASSIGN (member, mono_method_get_object_handle (foundMethod, mono_class_is_interface (foundMethod->klass) ? foundMethod->klass : klass, error)); goto_if_nok (error, leave); MONO_HANDLE_ARRAY_SETREF (targets, i, member); -- 2.7.4