From 631389fb941eb618fef883975ca5f4f5b03a93cf Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Michal=20Strehovsk=C3=BD?=
 <MichalStrehovsky@users.noreply.github.com>
Date: Thu, 31 Mar 2022 22:45:25 +0900
Subject: [PATCH] Fix ordering in default interface method lookup (#67379)

* Fix ordering in default interface method lookup

The previous implementation would find a default implementation in the current type before looking for non-default implementation in the base. Default implementations should be looked at last.

In fact the default implementation in the current type is unreachable if there's a non-default implementation in the base type - the compiler shouldn't even have bothered to emit it into the dispatch map. That can be a separate fix - this fix is still logically correct and necessary to get variant dispatch corner cases correctly.

* Regression test
---
 .../src/System/Runtime/DispatchResolve.cs     | 30 ++++++++++---------
 .../SmokeTests/UnitTests/Interfaces.cs        | 10 +++++++
 2 files changed, 26 insertions(+), 14 deletions(-)

diff --git a/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/DispatchResolve.cs b/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/DispatchResolve.cs
index 85adf27fe5469..aa661feaf6cd1 100644
--- a/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/DispatchResolve.cs
+++ b/src/coreclr/nativeaot/Runtime.Base/src/System/Runtime/DispatchResolve.cs
@@ -30,11 +30,16 @@ public static IntPtr FindInterfaceMethodImplementationTarget(MethodTable* pTgtTy
             if (pItfType->IsCloned)
                 pItfType = pItfType->CanonicalEEType;
 
+            // We first look at non-default implementation. Default implementations are only considered
+            // if the "old algorithm" didn't come up with an answer.
+            bool fDoDefaultImplementationLookup = false;
+
+        again:
             while (pCur != null)
             {
                 ushort implSlotNumber;
                 if (FindImplSlotForCurrentType(
-                        pCur, pItfType, itfSlotNumber, &implSlotNumber))
+                        pCur, pItfType, itfSlotNumber, fDoDefaultImplementationLookup, &implSlotNumber))
                 {
                     IntPtr targetMethod;
                     if (implSlotNumber < pCur->NumVtableSlots)
@@ -63,6 +68,15 @@ public static IntPtr FindInterfaceMethodImplementationTarget(MethodTable* pTgtTy
                 else
                     pCur = pCur->NonArrayBaseType;
             }
+
+            // If we haven't found an implementation, do a second pass looking for a default implementation.
+            if (!fDoDefaultImplementationLookup)
+            {
+                fDoDefaultImplementationLookup = true;
+                pCur = pTgtType;
+                goto again;
+            }
+
             return IntPtr.Zero;
         }
 
@@ -70,6 +84,7 @@ public static IntPtr FindInterfaceMethodImplementationTarget(MethodTable* pTgtTy
         private static bool FindImplSlotForCurrentType(MethodTable* pTgtType,
                                         MethodTable* pItfType,
                                         ushort itfSlotNumber,
+                                        bool fDoDefaultImplementationLookup,
                                         ushort* pImplSlotNumber)
         {
             bool fRes = false;
@@ -87,17 +102,11 @@ private static bool FindImplSlotForCurrentType(MethodTable* pTgtType,
 
             if (pTgtType->HasDispatchMap)
             {
-                // We first look at non-default implementation. Default implementations are only considered
-                // if the "old algorithm" didn't come up with an answer.
-
-                bool fDoDefaultImplementationLookup = false;
-
                 // For variant interface dispatch, the algorithm is to walk the parent hierarchy, and at each level
                 // attempt to dispatch exactly first, and then if that fails attempt to dispatch variantly. This can
                 // result in interesting behavior such as a derived type only overriding one particular instantiation
                 // and funneling all the dispatches to it, but its the algorithm.
 
-            again:
                 bool fDoVariantLookup = false; // do not check variance for first scan of dispatch map
 
                 fRes = FindImplSlotInSimpleMap(
@@ -109,13 +118,6 @@ private static bool FindImplSlotForCurrentType(MethodTable* pTgtType,
                     fRes = FindImplSlotInSimpleMap(
                      pTgtType, pItfType, itfSlotNumber, pImplSlotNumber, fDoVariantLookup, fDoDefaultImplementationLookup);
                 }
-
-                // If we haven't found anything and haven't looked at the default implementations yet, look now
-                if (!fRes && !fDoDefaultImplementationLookup)
-                {
-                    fDoDefaultImplementationLookup = true;
-                    goto again;
-                }
             }
 
             return fRes;
diff --git a/src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs b/src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs
index 572dd8abba59d..616a46827045b 100644
--- a/src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs
+++ b/src/tests/nativeaot/SmokeTests/UnitTests/Interfaces.cs
@@ -495,6 +495,13 @@ interface IFoo<T>
 
         class Foo<T> : IFoo<T> { }
 
+        class Base : IFoo
+        {
+            int IFoo.GetNumber() => 100;
+        }
+
+        class Derived : Base, IBar { }
+
         public static void Run()
         {
             Console.WriteLine("Testing default interface methods...");
@@ -508,6 +515,9 @@ public static void Run()
             if (((IFoo)new Baz()).GetNumber() != 100)
                 throw new Exception();
 
+            if (((IFoo)new Derived()).GetNumber() != 100)
+                throw new Exception();
+
             if (((IFoo<object>)new Foo<object>()).GetInterfaceType() != typeof(IFoo<object>))
                 throw new Exception();