dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.95k stars 4.65k forks source link

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

Closed AlexDoeringEnvision closed 1 week ago

AlexDoeringEnvision commented 8 months ago

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 commented 8 months ago

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 ```c# 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 commented 8 months ago

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:

https://github.com/dotnet/runtime/blob/6f4f268d0b46fa5dbcee46b7220919defb148127/src/coreclr/tools/Common/Compiler/DevirtualizationManager.cs#L183-L197

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 commented 1 month ago

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 commented 1 month ago

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 https://github.com/dotnet/runtime/issues/10809

MichalStrehovsky commented 1 month ago

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#.