-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
ILLink: Allow interface methods to not have newslot #93662
Conversation
Fixes dotnet#93008 This comes from F#, where interface method defined in F# doesn't have `newslot` flag in its metadata. Trimmer gets confused and doesn't recognize such method as a "base" method for its implementation in the type which implements the interface. As a result, the implementation method is removed from the program because there doesn't seem to be a use for it anywhere. The fix is to basically ignore the `newslot` flag for interface instance methods. The existing behavior must be kept for static interface methods though - as a redefinition of a method with the `new` C# keyword will produce an interface method definition without the `newslot` flag as well. The change adds a new test and modifies the AOT/analyzer infra to run the test as well.
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas Issue DetailsFixes #93008 This comes from F#, where interface method defined in F# doesn't have The fix is to basically ignore the The change adds a new test and modifies the AOT/analyzer infra to run the test as well.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for looking into this!
Is there a test that exercises this? I wasn't able to get C# to produce a static virtual method with a (I'm wondering if the fix shouldn't be to ignore all static methods. It seems like they're handled elsewhere.) |
I made this modification due to a failing test. Specifically this: Line 391 in 04ede0d
We should remove the |
I must admit that I don't know if IL allows for implicit implementation of a static virtual/abstract - that is done via method signature match and not relying on method-impl record. If it only allows this via method-impl records, then I agree we should just skip all static methods in this place (since the code after the if handles the method signature matching). |
Static virtual are only implemented through MethodImpl: https://github.com/dotnet/runtime/blob/main/docs/design/specs/Ecma-335-Augments.md#ii122-implementing-virtual-methods-on-interfaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! LGTM, thank you for looking into this.
@vitek-karas should we backport this to a servicing release? |
Fixes dotnet#93008 This comes from F#, where interface method defined in F# doesn't have `newslot` flag in its metadata. Trimmer gets confused and doesn't recognize such method as a "base" method for its implementation in the type which implements the interface. As a result, the implementation method is removed from the program because there doesn't seem to be a use for it anywhere. The fix is to basically ignore the `newslot` flag for interface instance methods. The existing behavior must be kept for static interface methods though - as a redefinition of a method with the `new` C# keyword will produce an interface method definition without the `newslot` flag as well. The change adds a new test and modifies the AOT/analyzer infra to run the test as well.
That would be very useful - effectively enabling F# support in dotnet8 ios nativeaot. |
We discussed this couple of weeks ago, but then it was a weird time (right around the branches closing for RTM). |
@vitek-karas it should be: |
/backport to release/8.0-staging |
Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7085143215 |
Fixes #93008
This comes from F#, where interface method defined in F# doesn't have
newslot
flag in its metadata. Trimmer gets confused and doesn't recognize such method as a "base" method for its implementation in the type which implements the interface. As a result, the implementation method is removed from the program because there doesn't seem to be a use for it anywhere.The fix is to basically ignore the
newslot
flag for interface instance methods. The existing behavior must be kept for static interface methods though - as a redefinition of a method with thenew
C# keyword will produce an interface method definition without thenewslot
flag as well.The change adds a new test and modifies the AOT/analyzer infra to run the test as well.