Backport fix for MethodImpl removal when interfaceImpl is removed #103432
Conversation
… removed (dotnet#102857) When static interface methods are kept but an implementation of the method is only accessed via a direct call on the implementing type, the implementation method and the .override pointing to the interface method are both kept, but not the .interfaceImpl. This causes a TypeLoadException when the .override points to a method on an interface that the type doesn't implement. To fix this, we needed to update the condition for sweeping overrides to include the case where both the interface and the interface method are kept, but the .interfaceImpl is not kept. Tests are for static interface methods, but concrete and generic to test type resolution in SweepStep, which can be finicky. Instance methods shouldn't hit this issue since public interface methods don't have a .override and private methods can't be marked without instantiating the type or reflecting over the type, both of which mark the type's .interfaceImpls, and making the .override valid. For instance methods, we will always mark any methods referenced in a .override as well as their corresponding interface implementation, so we won't end up in a scenario where an instance method would have dangling references in a .override. I did fix up the interface implementation marking (MarkRuntimeInterfaceImplementation) to make sure it can find a recursive interface, and marks the .interfaceImpl with the exact same TypeReference rather than just the same TypeDefinition (for example, IGeneric<int> should be marked, not just the first IGeneric<> interfaceImpl). This was one of the examples where the trimmer would end up doing the right thing (all IGeneric<> implementations would end up marked anyway), but it would do it for a different reason than we expect.
jtschuster
added
Servicing-consider
dotnet-policy-service
bot
added
the
linkable-framework
|
Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas |
There was a problem hiding this comment.
Code change LGTM.
To me this feels riskier than the average backport we've taken for illink, since it touches an area that has been a source of bugs (as you mentioned) - and especially because that area has evolved in .NET 9 and we're not getting as much test coverage of the old behavior in the net8 branch. Personally I would wait to see if there's another customer report before we take this.
...heritance.Interfaces/StaticInterfaceMethods/RemovedInterfaceImplementationRemovedOverride.cs
Show resolved
Hide resolved
I agree, I'd be okay leaving this as a draft for now. |
|
Yeah, I'd probably hold off for now. Alternatively, we could try to find a bigger test suite that we could run against that we think would have a reasonable chance of revealing the bug. |
jeffschwMSFT
removed
the
Servicing-consider
|
@jtschuster friendly reminder that code complete for the August Release is July 15th. If you want this fix to be included in that release, please merge this PR before that date. |
|
Friendly reminder that Monday July 15th is Code Complete day, that's the deadline to get this included in the August Release. |
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
Backports #102857 to .NET 8.
Customer Impact:
The issue was first reported in #102796. This affects situations where a static interface method definition is kept, an implementing type is called only directly on the declaring type (
TypeName.Method()is called rather thanT.Method()where T : IFoo), and the InterfaceImpl is removed. The MethodImpl pointing to the interface method is kept, but since the type doesn't implement the interface anymore, a TypeLoadException is thrown at runtime.There is a workaround that requires creating an explicit interface implementation method that forwards to the public method (see #102796 (comment)). It's not a bad workaround, but finding the workaround wouldn't be easy.
Testing:
A new test is added which exercises this scenario and checks for the proper removal of the MethodImpl. Assertions related to MethodImpl removal in other tests have been reenabled.
Risk:
There is a small chance that this change introduces a new bug. Interface and override trimming have been the primary source of issues reported in the trimmer. However, this change is fairly small and hasn't caused issues in main. It seems worth the risk to fix this scenario.