Consider method accessibility in interface resolution

Fixes dotnet/corert#1986 (yep, all the way to CoreRT repo). We finally found an instance where this matters - in MAUI. Non-public methods never implement an interface by name+sig combo. We got the `ProtectedDerived` case in the newly added test wrong.
MichalStrehovsky · Jan 31, 2023 · 7d126c7 · 7d126c7
1 parent e76d656
commit 7d126c7
Show file tree

Hide file tree

Showing 6 changed files with 84 additions and 3 deletions.
diff --git a/src/coreclr/tools/Common/TypeSystem/Common/InstantiatedMethod.cs b/src/coreclr/tools/Common/TypeSystem/Common/InstantiatedMethod.cs
@@ -117,6 +117,14 @@ public override bool IsFinal
             }
         }
 
+        public override bool IsPublic
+        {
+            get
+            {
+                return _methodDef.IsPublic;
+            }
+        }
+
         public override bool HasCustomAttribute(string attributeNamespace, string attributeName)
         {
             return _methodDef.HasCustomAttribute(attributeNamespace, attributeName);

diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs b/src/coreclr/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs
@@ -424,6 +424,14 @@ private static bool VerifyMethodsHaveTheSameVirtualSlot(MethodDesc slotDefiningM
             return slotDefiningMethodOfMethodToVerify == slotDefiningMethod;
         }
 
+        private static Func<MethodDesc, MethodDesc, bool> s_VerifyMethodIsPublic = VerifyMethodIsPublic;
+
+        // Return true if the method to verify is public
+        private static bool VerifyMethodIsPublic(MethodDesc slotDefiningMethod, MethodDesc methodToVerify)
+        {
+            return methodToVerify.IsPublic;
+        }
+
         private static void FindBaseUnificationGroup(MetadataType currentType, UnificationGroup unificationGroup)
         {
             MethodDesc originalDefiningMethod = unificationGroup.DefiningMethod;
@@ -615,7 +623,7 @@ private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc
             {
                 MethodDesc foundOnCurrentType = FindMatchingVirtualMethodOnTypeByNameAndSig(interfaceMethod, currentType,
                     reverseMethodSearch: false, /* When searching for name/sig overrides on a type that explicitly defines an interface, search through the type in the forward direction*/
-                    nameSigMatchMethodIsValidCandidate :null);
+                    nameSigMatchMethodIsValidCandidate: s_VerifyMethodIsPublic);
                 foundOnCurrentType = FindSlotDefiningMethodForVirtualMethod(foundOnCurrentType);
 
                 if (baseType == null)
@@ -653,7 +661,7 @@ private static MethodDesc ResolveInterfaceMethodToVirtualMethodOnType(MethodDesc
                 {
                     MethodDesc foundOnCurrentType = FindMatchingVirtualMethodOnTypeByNameAndSig(interfaceMethod, currentType,
                                             reverseMethodSearch: false, /* When searching for name/sig overrides on a type that is the first type in the hierarchy to require the interface, search through the type in the forward direction*/
-                                            nameSigMatchMethodIsValidCandidate: null);
+                                            nameSigMatchMethodIsValidCandidate: s_VerifyMethodIsPublic);
 
                     foundOnCurrentType = FindSlotDefiningMethodForVirtualMethod(foundOnCurrentType);
 
@@ -729,7 +737,7 @@ private static MethodDesc FindNameSigOverrideForInterfaceMethodRecursive(MethodD
 
                 MethodDesc nameSigOverride = FindMatchingVirtualMethodOnTypeByNameAndSig(interfaceMethod, currentType,
                     reverseMethodSearch: true, /* When searching for a name sig match for an interface on parent types search in reverse order of declaration */
-                    nameSigMatchMethodIsValidCandidate:null);
+                    nameSigMatchMethodIsValidCandidate: s_VerifyMethodIsPublic);
 
                 if (nameSigOverride != null)
                 {

diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.cs b/src/coreclr/tools/Common/TypeSystem/Common/MethodDesc.cs
@@ -584,6 +584,14 @@ public virtual bool IsFinal
             }
         }
 
+        public virtual bool IsPublic
+        {
+            get
+            {
+                return false;
+            }
+        }
+
         public abstract bool HasCustomAttribute(string attributeNamespace, string attributeName);
 
         /// <summary>

diff --git a/src/coreclr/tools/Common/TypeSystem/Common/MethodForInstantiatedType.cs b/src/coreclr/tools/Common/TypeSystem/Common/MethodForInstantiatedType.cs
@@ -109,6 +109,14 @@ public override bool IsFinal
             }
         }
 
+        public override bool IsPublic
+        {
+            get
+            {
+                return _typicalMethodDef.IsPublic;
+            }
+        }
+
         public override bool HasCustomAttribute(string attributeNamespace, string attributeName)
         {
             return _typicalMethodDef.HasCustomAttribute(attributeNamespace, attributeName);

diff --git a/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaMethod.cs b/src/coreclr/tools/Common/TypeSystem/Ecma/EcmaMethod.cs
@@ -353,6 +353,14 @@ public override bool IsDefaultConstructor
             }
         }
 
+        public override bool IsPublic
+        {
+            get
+            {
+                return Attributes.IsPublic();
+            }
+        }
+
         public override bool IsStaticConstructor
         {
             get

diff --git a/src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs b/src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs
@@ -35,6 +35,7 @@ public static int Run()
         if (TestIterfaceCallOptimization() == Fail)
             return Fail;
 
+        TestPublicAndNonpublicDifference.Run();
         TestDefaultInterfaceMethods.Run();
         TestDefaultInterfaceVariance.Run();
         TestVariantInterfaceOptimizations.Run();
@@ -477,6 +478,46 @@ private static int TestIterfaceCallOptimization()
 
     #endregion
 
+    class TestPublicAndNonpublicDifference
+    {
+        interface IFrobber
+        {
+            string Frob();
+        }
+        class ProtectedBase : IFrobber
+        {
+            string IFrobber.Frob() => "IFrobber.Frob";
+            protected virtual string Frob() => "Base.Frob";
+        }
+
+        class ProtectedDerived : ProtectedBase, IFrobber
+        {
+            protected override string Frob() => "Derived.Frob";
+        }
+
+        class PublicBase : IFrobber
+        {
+            string IFrobber.Frob() => "IFrobber.Frob";
+            public virtual string Frob() => "Base.Frob";
+        }
+
+        class PublicDerived : PublicBase, IFrobber
+        {
+            public override string Frob() => "Derived.Frob";
+        }
+
+        public static void Run()
+        {
+            IFrobber f1 = new PublicDerived();
+            if (f1.Frob() != "Derived.Frob")
+                throw new Exception();
+
+            IFrobber f2 = new ProtectedDerived();
+            if (f2.Frob() != "IFrobber.Frob")
+                throw new Exception();
+        }
+    }
+
     class TestDefaultInterfaceMethods
     {
         interface IFoo
-Original file line number
+Diff line change
@@ Expand Up / @@ -353,6 +353,14 @@ public override bool IsDefaultConstructor @@
                 }
             }
+            public override bool IsPublic
+            {
+                get
+                {
+                    return Attributes.IsPublic();
+                }
+            }
             public override bool IsStaticConstructor
             {
                 get
@@ Expand Down @@