[release/5.0] Fix covariant returns with generic return types (#45873)

* Fix covariant returns with generic return types

When a covariant return type was an uninstantiated generic type, the
ClassLoader::IsCompatibleWith was not working properly. In debug builds,
it was asserting because there was no MethodTable for that type and in
release builds, it resulted in ExecutionEngineException or an internal
CLR error.

This change fixes it by using TypeHandle::CanCastTo instead of
MethodTable::CanCastTo and adds a regression test for two cases where
the problem was observed (Assembly.GetTypes() and creating an instance
of a type with a covariant return with a problematic kind of type).

* Fix issue 45082 too

There were two issues. First, the
ClassLoader::ValidateMethodsWithCovariantReturnTypes was called before
typeHnd.DoFullyLoad and that resulted in an assert down the call chain
of TypeDesc::CanCastTo due to a wrong load level.
Second, the SigTypeContext generation for the current MD in the
ClassLoader::ValidateMethodsWithCovariantReturnTypes requires the same
change for class instantiation as the one that we had for the parent MD.

* Fix the issue 45082 in a correct way

The call to ClassLoader::ValidateMethodsWithCovariantReturnTypes is now
in MethodTable::DoFullyLoad.
I have also added a test case that verifies a case that David Wrighton
has suggested offline, where there are 3 types... A, B and C.
C derives from B which derives from A. B has a bad override which
should produce an error.  Then, cause C to be fully loaded without
otherwise triggering a load of B.

Co-authored-by: Jan Vorlicek <janvorli@microsoft.com>

src/coreclr/src/vm/class.cpp

@@ -1109,20 +1109,18 @@ bool ClassLoader::IsCompatibleWith(TypeHandle hType1, TypeHandle hType2)
         return false;
     }
 
-    _ASSERTE(hType1.GetMethodTable() != NULL);
-    _ASSERTE(hType2.GetMethodTable() != NULL);
-
-    // Nullable<T> can be cast to T, but this is not compatible according to ECMA I.8.7.1
-    bool isCastFromNullableOfTtoT = hType1.GetMethodTable()->IsNullable() && hType2.IsEquivalentTo(hType1.GetMethodTable()->GetInstantiation()[0]);
-    if (isCastFromNullableOfTtoT)
+    MethodTable* pMT1 = hType1.GetMethodTable();
+    if (pMT1 != NULL)
     {
-        return false;
+        // Nullable<T> can be cast to T, but this is not compatible according to ECMA I.8.7.1
+        bool isCastFromNullableOfTtoT = pMT1->IsNullable() && hType2.IsEquivalentTo(pMT1->GetInstantiation()[0]);
+        if (isCastFromNullableOfTtoT)
+        {
+            return false;
+        }
     }
 
-    {
-        GCX_COOP();
-        return hType2.GetMethodTable()->CanCastTo(hType1.GetMethodTable(), NULL);
-    }
+    return hType2.CanCastTo(hType1, NULL);
 }
 
 /*static*/
@@ -1171,16 +1169,21 @@ void ClassLoader::ValidateMethodsWithCovariantReturnTypes(MethodTable* pMT)
             if (!pMD->RequiresCovariantReturnTypeChecking() && !pParentMD->RequiresCovariantReturnTypeChecking())
                 continue;
 
-            Instantiation classInst = pParentMD->GetClassInstantiation();
-            if (ClassLoader::IsTypicalSharedInstantiation(classInst))
+            Instantiation parentClassInst = pParentMD->GetClassInstantiation();
+            if (ClassLoader::IsTypicalSharedInstantiation(parentClassInst))
             {
-                classInst = pParentMT->GetInstantiation();
+                parentClassInst = pParentMT->GetInstantiation();
             }
-            SigTypeContext context1(classInst, pMD->GetMethodInstantiation());
+            SigTypeContext context1(parentClassInst, pMD->GetMethodInstantiation());
             MetaSig methodSig1(pParentMD);
             TypeHandle hType1 = methodSig1.GetReturnProps().GetTypeHandleThrowing(pParentMD->GetModule(), &context1, ClassLoader::LoadTypesFlag::LoadTypes, CLASS_LOAD_EXACTPARENTS);
 
-            SigTypeContext context2(pMD);
+            Instantiation classInst = pMD->GetClassInstantiation();
+            if (ClassLoader::IsTypicalSharedInstantiation(classInst))
+            {
+                classInst = pMT->GetInstantiation();
+            }
+            SigTypeContext context2(classInst, pMD->GetMethodInstantiation());
             MetaSig methodSig2(pMD);
             TypeHandle hType2 = methodSig2.GetReturnProps().GetTypeHandleThrowing(pMD->GetModule(), &context2, ClassLoader::LoadTypesFlag::LoadTypes, CLASS_LOAD_EXACTPARENTS);
 

src/coreclr/src/vm/clsload.cpp

@@ -3531,17 +3531,11 @@ static void PushFinalLevels(TypeHandle typeHnd, ClassLoadLevel targetLevel, cons
     // and on its transitive dependencies.
     if (targetLevel == CLASS_LOADED)
     {
-        if (!typeHnd.IsTypeDesc())
-        {
-            ClassLoader::ValidateMethodsWithCovariantReturnTypes(typeHnd.AsMethodTable());
-        }
-
         DFLPendingList pendingList;
         BOOL           fBailed = FALSE;
 
         typeHnd.DoFullyLoad(NULL, CLASS_LOADED, &pendingList, &fBailed, pInstContext);
 
-
         // In the case of a circular dependency, one or more types will have
         // had their promotions deferred.
         //

src/coreclr/src/vm/methodtable.cpp

@@ -5282,6 +5282,12 @@ void MethodTable::DoFullyLoad(Generics::RecursionGraph * const pVisited,  const
     CONSISTENCY_CHECK(IsRestored_NoLogging());
     CONSISTENCY_CHECK(!HasApproxParent());
 
+    if ((level == CLASS_LOADED) && !IsSharedByGenericInstantiations())
+    {
+        _ASSERTE(GetLoadLevel() >= CLASS_DEPENDENCIES_LOADED);
+        ClassLoader::ValidateMethodsWithCovariantReturnTypes(this);
+    }
+
     if (IsArray())
     {
         Generics::RecursionGraph newVisited(pVisited, TypeHandle(this));

src/coreclr/src/vm/typehandle.cpp

@@ -560,7 +560,7 @@ BOOL TypeHandle::IsBoxedAndCanCastTo(TypeHandle type, TypeHandlePairList *pPairL
         GC_TRIGGERS;
         INJECT_FAULT(COMPlusThrowOM());
 
-        LOADS_TYPE(CLASS_LOAD_EXACTPARENTS);
+        LOADS_TYPE(CLASS_DEPENDENCIES_LOADED);
 
         // The caller should check for an exact match.
         // That will cover the cast of a (unboxed) valuetype to itself.
@@ -607,7 +607,7 @@ BOOL TypeHandle::CanCastTo(TypeHandle type, TypeHandlePairList *pVisited)  const
         MODE_ANY;
         INJECT_FAULT(COMPlusThrowOM());
 
-        LOADS_TYPE(CLASS_LOAD_EXACTPARENTS);
+        LOADS_TYPE(CLASS_DEPENDENCIES_LOADED);
     }
     CONTRACTL_END
 

src/tests/Loader/classloader/MethodImpl/CovariantReturns/UnitTest/CompatibleWithTest.il

@@ -124,6 +124,17 @@
     }
 }
 
+.class public auto ansi beforefieldinit D2 extends C2
+{
+    .method public hidebysig specialname rtspecialname instance void  .ctor() cil managed { ret }
+
+    .method public hidebysig newslot virtual instance int32 MD2()
+    {
+        ldc.i4.0
+        ret
+    }
+}
+
 .class public auto ansi beforefieldinit C3 extends C1
 {
     .method public hidebysig specialname rtspecialname instance void  .ctor() cil managed { ret } 
@@ -491,6 +502,16 @@
         ret
     }
 
+    .method public static void RunTestD2() noinlining
+    {
+        newobj instance void class D2::.ctor()
+        callvirt instance int32 class D2::MD2()
+        pop
+        ldstr "Unexpectedly succeeded"
+        call void [System.Console]System.Console::WriteLine(string)
+        ret
+    }
+
     .method public static void RunTestC3() noinlining 
     {
         newobj instance void class C3::.ctor()
@@ -796,6 +817,24 @@ CC2:
             call void Main::RunTestC2()
             ldc.i4.0
             stloc.0
+            leave.s CD2
+        }
+        catch [mscorlib]System.TypeLoadException
+        {
+            ldstr "Caught expected TypeLoadException:"
+            call       void [System.Console]System.Console::WriteLine(string)
+            call       void [System.Console]System.Console::WriteLine(object)
+            leave.s CD2
+        }
+CD2:
+        ldstr "D2: call non-overriding method MD2 when base class of D2 has invalid covariant override"
+        call void [System.Console]System.Console::WriteLine(string)
+
+        .try
+        {
+            call void Main::RunTestD2()
+            ldc.i4.0
+            stloc.0
             leave.s CC3
         }
         catch [mscorlib]System.TypeLoadException
@@ -804,7 +843,7 @@ CC2:
             call       void [System.Console]System.Console::WriteLine(string)
             call       void [System.Console]System.Console::WriteLine(object)        
             leave.s CC3
-        }  
+        }
 CC3:
         ldstr "C3: override IList<int32> by int32[]"
         call void [System.Console]System.Console::WriteLine(string)

src/tests/Regressions/coreclr/GitHub_45037/test45037.cs

@@ -0,0 +1,54 @@
+using System;
+using System.Collections.Generic;
+using System.Reflection;
+
+public abstract class Base<T>
+{
+   public virtual T Get() => throw new NotImplementedException();
+}
+
+public sealed class CovariantReturn : Base<object>
+{
+   public override string Get() => throw new NotImplementedException();
+}
+
+public abstract class ABase
+{
+    public abstract object this[int index] { get; }
+}
+
+public sealed class Concrete<T> : ABase
+    where T : class
+{
+    public override T this[int index]
+    {
+        get
+        {
+            throw null;
+        }
+    }
+}
+
+class Parent
+{
+    public virtual object Value { get; }
+}
+
+class Child<T> : Parent where T : class
+{
+    public override T Value { get => (T)base.Value; }
+}
+
+class Foo { }
+
+class Program
+{
+    static int Main()
+    {
+        Type[] t = Assembly.GetExecutingAssembly().GetTypes();
+        new Child<Foo>();
+        new CovariantReturn();
+
+        return 100;
+    }
+}

src/tests/Regressions/coreclr/GitHub_45037/test45037.csproj

@@ -0,0 +1,10 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestKind>BuildAndRun</CLRTestKind>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="test45037.cs" />
+  </ItemGroup>
+</Project>

src/tests/Regressions/coreclr/GitHub_45082/test45082.cs

@@ -0,0 +1,26 @@
+using System;
+using System.Collections.Generic;
+
+public abstract class AComponent { }
+public class Component : AComponent { }
+
+public abstract class Abstract
+{
+    public abstract IReadOnlyList<AComponent> New { get; }
+}
+
+public sealed class Concrete<T> : Abstract
+    where T : AComponent
+{
+    public override IReadOnlyList<T> New => throw null;
+}
+
+class Program
+{
+    static int Main()
+    {
+        new Concrete<Component>();
+
+        return 100;
+    }
+}

src/tests/Regressions/coreclr/GitHub_45082/test45082.csproj

@@ -0,0 +1,10 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <CLRTestKind>BuildAndRun</CLRTestKind>
+    <CLRTestPriority>1</CLRTestPriority>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="test45082.cs" />
+  </ItemGroup>
+</Project>