[NativeAOT] Method overrides with covariant return type don't work in all cases

[AlexDoeringEnvision]

Description

when NativeAOT finds an covariant override it will pick that type as new return type and ignore further overrides that don't return the same type
tested with NativeAOT net7/8

Reproduction Steps

var impl = new Impl();
var subImpl = new SubImpl();
Console.WriteLine(impl.FromBase());
Console.WriteLine(impl.FromType());

Console.WriteLine(subImpl.FromBase());
Console.WriteLine(subImpl.FromType());


class Base
{
    public Base FromBase()
    {
        return FromType();
    }

    public virtual Base FromType()
    {
        return new Base();
    }
}

class Impl : Base
{
    public override Impl FromType()
    {
        return new Impl();
    }
}

class SubImpl : Impl
{
    // not called from FromBase()
    public override SubImpl FromType()
    {
        return new SubImpl();
    }
}

Expected behavior

Impl
Impl
SubImpl
SubImpl

Actual behavior

Impl
Impl
Impl
SubImpl

Regression?

No response

Known Workarounds

don't use covariant return type

Configuration

.NET 8, Windows 10, x64, NativeAOT.

Other information

No response

[ghost]

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Description

when NativeAOT finds an covariant override it will pick that type as new return type and ignore further overrides that don't return the same type
tested with NativeAOT net7/8

Reproduction Steps

var impl = new Impl();
var subImpl = new SubImpl();
Console.WriteLine(impl.FromBase());
Console.WriteLine(impl.FromType());

Console.WriteLine(subImpl.FromBase());
Console.WriteLine(subImpl.FromType());


class Base
{
    public Base FromBase()
    {
        return FromType();
    }

    public virtual Base FromType()
    {
        return new Base();
    }
}

class Impl : Base
{
    public override Impl FromType()
    {
        return new Impl();
    }
}

class SubImpl : Impl
{
    // not called from FromBase()
    public override SubImpl FromType()
    {
        return new SubImpl();
    }
}

Expected behavior

Impl
Impl
SubImpl
SubImpl

Actual behavior

Impl
Impl
Impl
SubImpl

Regression?

No response

Known Workarounds

don't use covariant return type

Configuration

.NET 8, Windows 10, x64, NativeAOT.

Other information

No response

Author:	AlexDoeringEnvision
Assignees:	-
Labels:	area-NativeAOT-coreclr
Milestone:	-

[MichalStrehovsky]

This is an issue in the virtual method resolution algorithm: when asked "what method on SubImpl implements Base::FromType, it answers Impl::FromType". This is because the algorithm fails to consider the fact that Base::FromType was overriden by Impl::FromType and that one is overriden by SubImpl::FromType. It's likely nobody but @davidwrighton is able to make fixes in this codebase. I'd only make this worse if I touch it. We don't run the targeted covariant returns tests for native AOT because the way they were written (mixing valid cases and invalid cases that expect typeloadexceptions) would require us to have a full fidelity typeloadexception emulator that we don't have.

This has been a bug ever since covariant returns got added to the managed type system in #35308 for crossgen2. It would also show up as a bug in crossgen2, but the compiler (neither crossgen2, nor ilc) actually doesn't devirtualize any covariant returns due to this code:

runtime/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs

Lines 183 to 197 in 6f4f268

	MethodDesc slotDefiningMethodImpl = MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(impl);
	MethodDesc slotDefiningMethodDecl = MetadataVirtualMethodAlgorithm.FindSlotDefiningMethodForVirtualMethod(declMethod);
	if (slotDefiningMethodImpl != slotDefiningMethodDecl)
	{
	// If the derived method's slot does not match the vtable slot,
	// bail on devirtualization, as the method was installed into
	// the vtable slot via an explicit override and even if the
	// method is final, the slot may not be.
	//
	// Note the jit could still safely devirtualize if it had an exact
	// class, but such cases are likely rare.
	devirtualizationDetail = CORINFO_DEVIRTUALIZATION_DETAIL.CORINFO_DEVIRTUALIZATION_FAILED_SLOT;
	impl = null;
	}

That one was also added for crossgen2's sake and I don't actually understand why we do it the way we do it. Fixing it would make this bug also show up in crossgen2.

[filipnavara]

We just accidentally hit this in our app when implementing handlers in MAUI code.

We have CarouselViewHandler and CarouselViewController classes under both the Microsoft.Maui.Controls.Handlers.Items namespace and the MailClient.Mobile.iOS.Handlers namespace. The latter ones are are derived from the former ones. There's complex hierarchy of generic types that originally defines the CreateController method (MAUI code).

We had the following code:

namespace MailClient.Mobile.iOS.Handlers
{
	class CarouselViewHandler : Microsoft.Maui.Controls.Handlers.Items.CarouselViewHandler
	{
		protected override CarouselViewController CreateController(CarouselView newElement, ItemsViewLayout layout)
		{
			return new CarouselViewController(newElement, layout);
		}
	}
}

The CreateController method was never called. Once we figured out what it happening the workaround is trivial:

@@ -8,7 +8,7 @@ namespace MailClient.Mobile.iOS.Handlers
 {
        class CarouselViewHandler : Microsoft.Maui.Controls.Handlers.Items.CarouselViewHandler
        {
-               protected override CarouselViewController CreateController(CarouselView newElement, ItemsViewLayout layout)
+               protected override Microsoft.Maui.Controls.Handlers.Items.CarouselViewController CreateController(CarouselView newElement, ItemsViewLayout layout)
                {
                        return new CarouselViewController(newElement, layout);
                }

[AndyAyersMS]

That one was also added for crossgen2's sake and I don't actually understand why we do it the way we do it. Fixing it would make this bug also show up in crossgen2.

For context on that bit of slot-testing code, see the discussion in #10809

[MichalStrehovsky]

That one was also added for crossgen2's sake and I don't actually understand why we do it the way we do it. Fixing it would make this bug also show up in crossgen2.

For context on that bit of slot-testing code, see the discussion in #10809

If I understand it correctly, the discussion happened when this was an IL-level corner case. These can now be hit with regular C#.