Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static virtual reabstraction doesn't seem to work #71414

Closed
MichalStrehovsky opened this issue Jun 29, 2022 · 8 comments · Fixed by #88711
Closed

Static virtual reabstraction doesn't seem to work #71414

MichalStrehovsky opened this issue Jun 29, 2022 · 8 comments · Fixed by #88711
Assignees
Milestone

Comments

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Jun 29, 2022

I'm working on implementing this with NativeAOT and was looking at how it works in the VM.

Reproed with 7.0.100-preview.5.22307.18 SDK7.0.100-preview.7.22328.2:

Compile following program:

Call<BarClass>();

static void Call<T>() where T : IFoo => T.Frob();

interface IFoo
{
    static virtual void Frob() => throw null;
}

interface IBar : IFoo
{
    static void IFoo.Frob() => throw null;
}

interface IBaz : IFoo
{
    static abstract void IFoo.Frob();
}

class BarClass : IBar
{
}

This will throw at runtime:

Unhandled exception. System.TypeLoadException: Method implementation on an interface 'IBar' from assembly 'publishaot2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' must be a final method.
   at Program.<Main>$(String[] args)

Sure, looks like a Roslyn bug, but it's not what I'm after. (@AlekseyTs is this known?)

Fix the problem by ildasm-ing the assembly, adding the missing final and ILAsm-ing it back. Now the program works as expected.

Now, go back to the IL and change BarClass to implement IBaz instead of the IBar (Roslyn wouldn't allow compiling that because the method was reabstracted). Rebuild the IL.

Unhandled exception. System.MissingMethodException: Method not found: 'Void IFoo.Frob()'.
   at Program.<Main>$(String[] args)

This doesn't look like the experience we would want for reabstraction. For reference, the exception with instance default interface methods is (you have to go back to the IL this generates and replace IBar with IBaz in BarClass interface list, same as for the static virtual case because Roslyn doesn't allow compiling with the reabstraction):

IFoo foo = new BarClass();
foo.Frob();

interface IFoo
{
    virtual void Frob() => throw null;
}

interface IBar : IFoo
{
    void IFoo.Frob() => throw null;
}

interface IBaz : IFoo
{
    abstract void IFoo.Frob();
}

class BarClass : IBar
{
}

This will print a more descriptive message:

Unhandled exception. System.EntryPointNotFoundException: Could not call method 'IFoo.Frob()' on type 'IFoo' with an instance of 'BarClass' from assembly 'publishaot2, Version=1.0.0.0, Culture=neutral, PublicKeyToken=null' because there is no implementation for the method.
   at IFoo.Frob()
   at Program.<Main>$(String[] args)

Cc @trylek @davidwrighton

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jun 29, 2022
@trylek
Copy link
Member

trylek commented Jun 29, 2022

Thanks Michal for reporting this issue. Just to make sure I understand you correctly, are you only concerned about the insufficiently descriptive error message or do you believe that the actual runtime behavior is incorrect?

@trylek trylek self-assigned this Jun 29, 2022
@trylek trylek removed the untriaged New issue has not been triaged by the area owner label Jun 29, 2022
@trylek trylek added this to the 7.0.0 milestone Jun 29, 2022
@MichalStrehovsky
Copy link
Member Author

MichalStrehovsky commented Jun 29, 2022

Thanks Michal for reporting this issue. Just to make sure I understand you correctly, are you only concerned about the insufficiently descriptive error message or do you believe that the actual runtime behavior is incorrect?

I don't know what the runtime is doing that we hit a MissingMethodException for a method that exists, so I'm concerned about the runtime behavior too. It looks like we're in undefined behavior territories.

@MichalStrehovsky
Copy link
Member Author

The Roslyn issue is already fixed. Somehow I switched to a globally installed Preview 5 SDK mid-experimenting with this.

The MissingMethodException still reproes with the latest daily build of the SDK.

@AlekseyTs
Copy link
Contributor

AlekseyTs commented Jun 29, 2022

The Roslyn issue is already fixed.

I am not sure if there was a Roslyn issue around the final flag. The implementation method is static and not virtual, therefore it is not supposed to have final flag.

@mangod9
Copy link
Member

mangod9 commented Aug 5, 2022

Is this something we need to fix in 7?

@trylek
Copy link
Member

trylek commented Aug 5, 2022

I'm going to try to give it a shot next week; in general I think it's a somewhat corner case, at the very least static virtual method functionality in the .NET 7 runtime libraries hasn't hit this yet.

@MichalStrehovsky
Copy link
Member Author

at the very least static virtual method functionality in the .NET 7 runtime libraries hasn't hit this yet.

It's in the same bucket as the diamond case. Properly authored apps will never hit it at runtime because Roslyn would never emit it. Mismatched dependencies and incorrectly composed apps can hit it. It's more likely the customers would hit it than us.

@mangod9
Copy link
Member

mangod9 commented Aug 15, 2022

moving to 8 at this point.

@mangod9 mangod9 modified the milestones: 7.0.0, 8.0.0 Aug 15, 2022
@davidwrighton davidwrighton self-assigned this Jun 23, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 12, 2023
davidwrighton added a commit that referenced this issue Jul 14, 2023
- Add a new helper for the re-abstraction case when the JIT detects an issue
- In the late bound case, instead of producing an error directly, produce an IL stub and have it make the virtual stub dispatch call itself, this will fall back to the JIT implementation for re-abstraction
- Tweak the late bound case to also actively detect the AmbiguousMatchException case as well and use the same helper there as well.

Fixes #71414
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 14, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants