[WIP] Covariant Returns Feature (#35308)

* Covariant Returns Feature: Allowing return types on MethodImpl to be derived types of the return type on the MethodDecl.

Feature limitations:
  - Only supports MethodImpls and MethodDecls on classes (no interfaces/valuetypes).
  - MethodImpl and MethodDecl cannot be on the same type.
  - Interface/valuetypes not supported as covariant return types.

Changes are mostly a boolean flag being passed around to allow for covariant type checking in method signatures.
Generics are handled by correctly keeping track of the substitution chain while traversing the base type hierarchy for type comparison.

All method signature comparisons uses metadata for checking, without loading any type (with some exceptions).

Validation for the `ValidateMethodImplRemainsInEffectAttribute` is performed at the very last step of `CLASS_LOAD_EXACTPARENTS`.

Includes unit tests to cover positive/negative scenarios:
  - Non-generics
  - Generics, with various levels of complexities on generic instantiations and substitutions
  - GVMs
  - Delegates
  - Tests for validation of the existance/absence of the ValidateMethodImplRemainsInEffectAttribute
  - Interface and valuetype scenarios (negative scenarios)

* Adding the missing .ctor methods to the various types

* Perform covariant return signature matching only after an exact match fails

* Add test coverage for implicit override with less derived return type

* Change ValidateMethodImplRemainsInEffect attribute to apply to methods

* Moving covariant return type checking to the final stage of type loading, and use CanCastTo instead of signature-based checking, to be allow for type compatibility based on ECMA I.8.7.1.

* Rename attribute and reference it from System.Runtime instead of S.P.C

* Add test coverage for interface cases.

* Handling for struct cases, and adding test coverage for it.

* Small test fixes

* Fix consistency issue with the way the RequireMethodImplToRemainInEffectAttribute is declared

* Support covariant returns and slot unifications in crossgen2

These changes fix the behavior of the devirtualization algorithm to not incorrectly devirtualize covariant return methods that have the slot unification attribute

* Add unit test coverage for delegates

* Fix handling of covariant and contravariant generic interfaces

Also add tests for those.

* Added test cases to interfaces unit test

Based on PR feedback, I've added test cases to verify the load level
asserts change.

* Fix Nullable handling and add more tests

The attempt to overload method with int32 return value by a method with
Nullable<int32> was asserting in CanCastTo. This change fixes it by
pre-checking this case before calling into CanCastTo.
I've also slightly refactored the
ValidateMethodsWithCovariantReturnTypes by extracting the
compatible-with stuff into IsCompatibleWith method.

Added tests to test various compatible-with differences from the
originally used CanCastTo

- Change the CompatibleWithTest to call virtual methods on test
classes instead of just instantiating the classes.
- Update crossgen2 similar to the runtime to have IsCompatibleWith
method doing the same pre-filtering before calling CanCastTo.

* Rename the attribute to PreserveBaseOverridesAttribute

* Disable covariant returns tests on Mono for now

The mono doesn't implement the feature yet

Co-authored-by: Fadi Hanna <[email protected]>
Co-authored-by: Jan Vorlicek <[email protected]>

docs/design/features/covariant-return-methods.md

@@ -0,0 +1,98 @@
+# Covariant Return Methods
+
+Covariant return methods is a runtime feature designed to support the [covariant return types](https://github.com/dotnet/csharplang/blob/master/proposals/covariant-returns.md) and [records](https://github.com/dotnet/csharplang/blob/master/proposals/records.md) C# language features posed for C# 9.0.
+
+This feature allows an overriding method to have a return type that is different than the one on the method it overrides, but compatible with it. The type compability rules are defined in ECMA I.8.7.1. Example: using a more derived return type. 
+
+Covariant return methods can only be described through MethodImpl records, and as an initial implementation will only be applicable to methods on reference types. Methods on interfaces and value types will not be supported (may be supported later in the future).
+
+MethodImpl checking will allow a return type to vary as long as the override is compatible with the return type of the method overriden (ECMA I.8.7.1).
+
+If a language wishes for the override to be semantically visible such that users of the more derived type may rely on the covariant return type it shall make the override a newslot method with appropriate visibility AND name to be used outside of the class.
+
+For virtual method slot MethodImpl overrides, each slot shall be checked for compatible signature on type load. (Implementation note: This behavior can be triggered only if the type has a covariant return type override in its hierarchy, so as to make this pay for play.)
+
+A new `PreserveBaseOverridesAttribute` shall be added. The presence of this attribute is to require the type loader to ensure that the MethodImpl records specified on the method have not lost their slot unifying behavior due to other actions. This is used to allow the C# language to require that overrides have the consistent behaviors expected. The expectation is that C# would place this attribute on covariant override methods in classes.
+
+## Implementation Notes
+
+### Return Type Checking
+
+During enumeration of MethodImpls on a type (`MethodTableBuilder::EnumerateMethodImpls()`), if the signatures of the MethodImpl and the MethodDecl do not match:
+1. We repeat the signature comparison a second time, but skip the comparison of the return type signatures. If the signatures for the rest of the method arguments match, we will conditionally treat that MethodImpl as a valid one, but flag it for a closer examination of the return type compatibility at a later stage of type loading (end of `CLASS_LOAD_EXACTPARENTS` stage).
+2. At the end of the `CLASS_LOAD_EXACTPARENTS` type loading stage, examing each virtual method on the type, and if it has been flagged for further return type checking:
+    + Load the `TypeHandle` of the return type of the method on base type.
+    + Load the `TypeHandle` of the return type of the method on the current type being validated.
+    + Verify that the second `TypeHandle` is compatible with the first `TypeHandle` using the `MethodTable::CanCastTo()` API. If they are not compatible, a TypeLoadException is thrown.
+
+The only exception where `CanCastTo()` will return true for an incompatible type according to the ECMA rules is for structs implementing interfaces, so we explicitly check for that case and throw a TypeLoadException if we hit it.
+
+Once a method is flagged for return type checking, every time the vtable slot containing that method gets overridden on a derived type, the new override will also be checked for compatiblity. This is to ensure that no derived type can implicitly override some virtual method that has already been overridden by some MethodImpl with a covariant return type.
+
+### VTable Slot Unification
+
+If a MethodImpl has the `PreserveBaseOverridesAttribute` attribute, it needs to propagate all applicable vtable slots on the type. This is to ensure that if we use the signature of one of the base type methods to call the overriding method, we still execute the overriding method.
+
+Consider this case:
+``` C#
+     class A {
+         RetType VirtualFunction() { }
+     }
+     class B : A {
+         [PreserveBaseOverrides]
+         DerivedRetType VirtualFunction() { .override A.VirtualFuncion }
+     }
+     class C : B {
+         [PreserveBaseOverrides]
+         MoreDerivedRetType VirtualFunction() { .override A.VirtualFunction }
+     }
+```
+
+Given an object of type `C`, the attribute will ensure that:
+``` C#
+     callvirt RetType A::VirtualFunc()               -> executes the MethodImpl on C
+     callvirt DerivedRetType B::VirtualFunc()        -> executes the MethodImpl on C
+     callvirt MoreDerivedRetType C::VirtualFunc()    -> executes the MethodImpl on C
+```
+
+Without the attribute, the second callvirt would normally execute the MethodImpl on `B` (the MethodImpl on `C` does not override the vtable slot of `B`'s MethodImpl, but only overrides the declaring method's vtable slot.
+
+This slot unification step will also take place during the last step of type loading (end of `CLASS_LOAD_EXACTPARENTS` stage).
+
+### [Future] Interface Support
+
+An interface method may be both non-final and have a MethodImpl that declares that it overrides another interface method. If it does, NO other interface method may .override it. Instead further overrides must override the method that it overrode. Also the overriding method may only override 1 method.
+
+The default interface method resolution algorithm shall change from:
+
+``` console
+Given interface method M and type T.
+Let MSearch = M
+Let MFound = Most specific implementation within the interfaces for MSearch within type T. If multiple implementations are found, throw Ambiguous match exception.
+Return MFound
+```
+
+To:
+
+``` console
+Given interface method M and type T.
+Let MSearch = M
+
+If (MSearch overrides another method MBase)
+    Let MSearch = MBase
+
+Let MFound = Most specific implementation within the interfaces for MSearch within type T. If multiple implementations are found, throw Ambiguous match exception.
+Let M Code = NULL
+
+If ((MFound != Msearch) and (MFound is not final))
+    Let M ClassVirtual = ResolveInterfaceMethod for MFound to virtual override on class T without using Default interface method implementation or return NULL if not found.
+    If (M ClassVirtual != NULL)
+        Let M Code= ResolveVirtualMethod for MFound on class T to implementation method
+
+If (M Code != NULL)
+    Let M Code = MFound
+
+Check M Code For signature <compatible-with> interface method M.
+
+Return M Code
+```

src/coreclr/src/dlls/mscorrc/mscorrc.rc

@@ -399,6 +399,7 @@ BEGIN
     IDS_CLASSLOAD_MI_BODY_DECL_MISMATCH     "Signature of the body and declaration in a method implementation do not match.  Type: '%1'.  Assembly: '%2'."
     IDS_CLASSLOAD_MI_MISSING_SIG_BODY       "Signature for the body in a method implementation cannot be found.  Type: '%1'.  Assembly: '%2'."
     IDS_CLASSLOAD_MI_MISSING_SIG_DECL       "Signature for the declaration in a method implementation cannot be found.  Type: '%1'.  Assembly: '%2'."
+    IDS_CLASSLOAD_MI_BADRETURNTYPE          "Return type in method '%1' on type '%2' from assembly '%3' is not compatible with base type method '%4'."
 
     IDS_CLASSLOAD_EQUIVALENTSTRUCTMETHODS   "Could not load the structure '%1' from assembly '%2'. The structure is marked as eligible for type equivalence, but it has a method."
     IDS_CLASSLOAD_EQUIVALENTSTRUCTFIELDS    "Could not load the structure '%1' from assembly '%2'. The structure is marked as eligible for type equivalence, but it has a static or non-public field."

src/coreclr/src/dlls/mscorrc/resource.h

@@ -167,6 +167,7 @@
 #define IDS_CLASSLOAD_MI_BODY_DECL_MISMATCH     0x17a5
 #define IDS_CLASSLOAD_MI_MISSING_SIG_BODY       0x17a6
 #define IDS_CLASSLOAD_MI_MISSING_SIG_DECL       0x17a7
+#define IDS_CLASSLOAD_MI_BADRETURNTYPE          0x17a8
 
 #define IDS_CLASSLOAD_TOOMANYGENERICARGS        0x17ab
 #define IDS_ERROR                               0x17b0

src/coreclr/src/tools/Common/TypeSystem/Common/CastingHelper.cs

@@ -20,6 +20,31 @@ public static bool CanCastTo(this TypeDesc thisType, TypeDesc otherType)
             return thisType.CanCastToInternal(otherType, null);
         }
 
+        public static bool IsCompatibleWith(this TypeDesc thisType, TypeDesc otherType)
+        {
+            // Structs can be cast to the interfaces they implement, but they are not compatible according to ECMA I.8.7.1
+            bool isCastFromValueTypeToReferenceType = otherType.IsValueType && !thisType.IsValueType;
+            if (isCastFromValueTypeToReferenceType)
+            {
+                return false;
+            }
+
+            // Nullable<T> can be cast to T, but this is not compatible according to ECMA I.8.7.1
+            bool isCastFromNullableOfTtoT = thisType.IsNullable && otherType.IsEquivalentTo(thisType.Instantiation[0]);
+            if (isCastFromNullableOfTtoT)
+            {
+                return false;
+            }
+
+            return otherType.CanCastTo(thisType);
+        }
+
+        private static bool IsEquivalentTo(this TypeDesc thisType, TypeDesc otherType)
+        {
+            // TODO: Once type equivalence is implemented, this implementation needs to be enhanced to honor it.
+            return thisType.Equals(otherType);
+        }
+
         private static bool CanCastToInternal(this TypeDesc thisType, TypeDesc otherType, StackOverflowProtect protect)
         {
             if (thisType == otherType)

src/coreclr/src/tools/Common/TypeSystem/Common/MetadataRuntimeInterfacesAlgorithm.cs

@@ -22,13 +22,7 @@ public override DefType[] ComputeRuntimeInterfaces(TypeDesc _type)
             // TODO: need to implement deduplication
             // https://github.com/dotnet/corert/issues/208
 
-            if (type.IsInterface)
-            {
-                // For interfaces, the set of interfaces implemented directly matches the
-                // explicitly implemented interface list
-                return type.ExplicitlyImplementedInterfaces;
-            }
-            else if (type is InstantiatedType)
+            if (type is InstantiatedType)
             {
                 return ComputeRuntimeInterfacesForInstantiatedType((InstantiatedType)type);
             }

src/coreclr/src/tools/Common/TypeSystem/Common/MetadataVirtualMethodAlgorithm.cs

@@ -5,6 +5,7 @@
 using System;
 using System.Collections.Generic;
 using System.Diagnostics;
+using System.Linq;
 
 namespace Internal.TypeSystem
 {
@@ -45,6 +46,9 @@ private class UnificationGroup
             private MethodDesc[] _members = MethodDesc.EmptyMethods;
             private int _memberCount = 0;
 
+            private MethodDesc[] _methodsRequiringSlotUnification = MethodDesc.EmptyMethods;
+            private int _methodsRequiringSlotUnificationCount = 0;
+
             /// <summary>
             /// Custom enumerator struct for Unification group. Makes enumeration require 0 allocations.
             /// </summary>
@@ -86,6 +90,21 @@ public MethodDesc Current
                 }
             }
 
+            public struct Enumerable
+            {
+                private readonly MethodDesc[] _arrayToEnumerate;
+
+                public Enumerable(MethodDesc[] arrayToEnumerate)
+                {
+                    _arrayToEnumerate = arrayToEnumerate;
+                }
+
+                public Enumerator GetEnumerator()
+                {
+                    return new Enumerator(_arrayToEnumerate);
+                }
+            }
+
             public UnificationGroup(MethodDesc definingMethod)
             {
                 DefiningMethod = definingMethod;
@@ -94,9 +113,31 @@ public UnificationGroup(MethodDesc definingMethod)
 
             public MethodDesc DefiningMethod;
 
-            public Enumerator GetEnumerator()
+            public Enumerable Members => new Enumerable(_members);
+
+            public Enumerable MethodsRequiringSlotUnification => new Enumerable(_methodsRequiringSlotUnification);
+
+            public void AddMethodRequiringSlotUnification(MethodDesc method)
+            {
+                if (RequiresSlotUnification(method))
+                    return;
+
+                _methodsRequiringSlotUnificationCount++;
+                if (_methodsRequiringSlotUnificationCount >= _methodsRequiringSlotUnification.Length)
+                {
+                    Array.Resize(ref _methodsRequiringSlotUnification, Math.Max(_methodsRequiringSlotUnification.Length * 2, 2));
+                }
+                _methodsRequiringSlotUnification[_methodsRequiringSlotUnificationCount - 1] = method;
+            }
+
+            public bool RequiresSlotUnification(MethodDesc method)
             {
-                return new Enumerator(_members);
+                for(int i = 0; i < _methodsRequiringSlotUnificationCount; i++)
+                {
+                    if (_methodsRequiringSlotUnification[i] == method)
+                        return true;
+                }
+                return false;
             }
 
             public void SetDefiningMethod(MethodDesc newDefiningMethod)
@@ -395,6 +436,11 @@ private static void FindBaseUnificationGroup(MetadataType currentType, Unificati
             MethodDesc methodImpl = FindImplFromDeclFromMethodImpls(currentType, unificationGroup.DefiningMethod);
             if (methodImpl != null)
             {
+                if(methodImpl.RequiresSlotUnification())
+                {
+                    unificationGroup.AddMethodRequiringSlotUnification(unificationGroup.DefiningMethod);
+                    unificationGroup.AddMethodRequiringSlotUnification(methodImpl);
+                }
                 unificationGroup.SetDefiningMethod(methodImpl);
             }
 
@@ -416,7 +462,7 @@ private static void FindBaseUnificationGroup(MetadataType currentType, Unificati
             // Start with removing methods that seperated themselves from the group via name/sig matches
             MethodDescHashtable separatedMethods = null;
 
-            foreach (MethodDesc memberMethod in unificationGroup)
+            foreach (MethodDesc memberMethod in unificationGroup.Members)
             {
                 MethodDesc nameSigMatchMemberMethod = FindMatchingVirtualMethodOnTypeByNameAndSigWithSlotCheck(memberMethod, currentType, reverseMethodSearch: true);
                 if (nameSigMatchMemberMethod != null && nameSigMatchMemberMethod != memberMethod)
@@ -448,29 +494,68 @@ private static void FindBaseUnificationGroup(MetadataType currentType, Unificati
                     if (separatedMethods == null)
                         separatedMethods = new MethodDescHashtable();
                     separatedMethods.AddOrGetExisting(declSlot);
+
+                    if (unificationGroup.RequiresSlotUnification(declSlot) || implSlot.RequiresSlotUnification())
+                    {
+                        if (implSlot.Signature.EqualsWithCovariantReturnType(unificationGroup.DefiningMethod.Signature))
+                        {
+                            unificationGroup.AddMethodRequiringSlotUnification(declSlot);
+                            unificationGroup.AddMethodRequiringSlotUnification(implSlot);
+                            unificationGroup.SetDefiningMethod(implSlot);
+                        }
+                    }
+
                     continue;
                 }
-                if (!unificationGroup.IsInGroupOrIsDefiningSlot(declSlot) && unificationGroup.IsInGroupOrIsDefiningSlot(implSlot))
+                if (!unificationGroup.IsInGroupOrIsDefiningSlot(declSlot))
                 {
-                    // Add decl to group.
+                    if (unificationGroup.IsInGroupOrIsDefiningSlot(implSlot))
+                    {
+                        // Add decl to group.
 
-                    // To do so, we need to have the Unification Group of the decl slot, as it may have multiple members itself
-                    UnificationGroup addDeclGroup = new UnificationGroup(declSlot);
-                    FindBaseUnificationGroup(baseType, addDeclGroup);
-                    Debug.Assert(addDeclGroup.IsInGroupOrIsDefiningSlot(declSlot));
+                        // To do so, we need to have the Unification Group of the decl slot, as it may have multiple members itself
+                        UnificationGroup addDeclGroup = new UnificationGroup(declSlot);
+                        FindBaseUnificationGroup(baseType, addDeclGroup);
+                        Debug.Assert(
+                            addDeclGroup.IsInGroupOrIsDefiningSlot(declSlot) ||
+                            (addDeclGroup.RequiresSlotUnification(declSlot) && addDeclGroup.DefiningMethod.Signature.EqualsWithCovariantReturnType(declSlot.Signature)));
 
-                    // Add all members from the decl's unification group except for ones that have been seperated by name/sig matches
-                    // or previously processed methodimpls. NOTE: This implies that method impls are order dependent.
-                    if (separatedMethods == null || !separatedMethods.Contains(addDeclGroup.DefiningMethod))
-                    {
-                        unificationGroup.AddToGroup(addDeclGroup.DefiningMethod);
-                    }
+                        foreach(MethodDesc methodImplRequiredToRemainInEffect in addDeclGroup.MethodsRequiringSlotUnification)
+                        {
+                            unificationGroup.AddMethodRequiringSlotUnification(methodImplRequiredToRemainInEffect);
+                        }
+
+                        // Add all members from the decl's unification group except for ones that have been seperated by name/sig matches
+                        // or previously processed methodimpls. NOTE: This implies that method impls are order dependent.
+                        if (separatedMethods == null || !separatedMethods.Contains(addDeclGroup.DefiningMethod))
+                        {
+                            unificationGroup.AddToGroup(addDeclGroup.DefiningMethod);
+                        }
+
+                        foreach (MethodDesc addDeclGroupMemberMethod in addDeclGroup.Members)
+                        {
+                            if (separatedMethods == null || !separatedMethods.Contains(addDeclGroupMemberMethod))
+                            {
+                                unificationGroup.AddToGroup(addDeclGroupMemberMethod);
+                            }
+                        }
 
-                    foreach (MethodDesc addDeclGroupMemberMethod in addDeclGroup)
+                        if(unificationGroup.RequiresSlotUnification(declSlot))
+                        {
+                            unificationGroup.AddMethodRequiringSlotUnification(implSlot);
+                        }
+                        else if (implSlot == unificationGroup.DefiningMethod && implSlot.RequiresSlotUnification())
+                        {
+                            unificationGroup.AddMethodRequiringSlotUnification(declSlot);
+                            unificationGroup.AddMethodRequiringSlotUnification(implSlot);
+                        }
+                    }
+                    else if (unificationGroup.RequiresSlotUnification(declSlot))
                     {
-                        if (separatedMethods == null || !separatedMethods.Contains(addDeclGroupMemberMethod))
+                        if (implSlot.Signature.EqualsWithCovariantReturnType(unificationGroup.DefiningMethod.Signature))
                         {
-                            unificationGroup.AddToGroup(addDeclGroupMemberMethod);
+                            unificationGroup.AddMethodRequiringSlotUnification(implSlot);
+                            unificationGroup.SetDefiningMethod(implSlot);
                         }
                     }
                 }