ILLink: Allow interface methods to not have newslot#93662
ILLink: Allow interface methods to not have newslot#93662vitek-karas merged 2 commits intodotnet:mainfrom
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.
vitek-karas
added
the
area-Tools-ILLink
|
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.
|
ivanpovazan
left a comment
ivanpovazan
left a comment
There was a problem hiding this comment.
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: 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 |
sbomer
left a comment
sbomer
left a comment
There was a problem hiding this comment.
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 |
5 participants
Fixes #93008
This comes from F#, where interface method defined in F# doesn't have
newslotflag 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
newslotflag for interface instance methods. The existing behavior must be kept for static interface methods though - as a redefinition of a method with thenewC# keyword will produce an interface method definition without thenewslotflag as well.The change adds a new test and modifies the AOT/analyzer infra to run the test as well.