ILLink: Sweep .override for interface method if the .interfaceImpl is removed

[jtschuster]

Fixes #102796.

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.

[dotnet-policy-service]

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

[agocke]

Instance methods shouldn't hit this issue since public interface methods don't have a .override

Is that required by the ECMA specification? I believe Roslyn can generate these cases, for instance:

interface I {
 void Method();
}
class A { }

class B : A, I 
{
    void I.Method() { }
}
class C : B 
{
    public void Method() { }
}
class D : C, I { }

In general, I would say that it is too hard to guess at what Roslyn will or won't generate -- it would require a completely up-to-date, encyclopedic knowledge of the compiler. We should instead follow the ECMA specification.

[agocke]

Never mind, the above example doesn't work -- Roslyn generates a private stub, which has the methodimpl. However, I think the point is the same -- I'm not confident there's no situation that Roslyn won't generate this IL. And it looks perfectly valid from the spec perspective, to me.

[jtschuster]

Instance methods shouldn't hit this issue since public interface methods don't have a .override

Is that required by the ECMA specification?

Thats a good point, it'll require writing tests in IL, but I think it might be possible to break that assumption.

[jtschuster]

I was able to make methods with .overrides on public methods, but I wasn't able to find a way to mark only the method without marking the type as 'RelevantToVariantCasting' or instantiated, so the interfaceImpl is always marked. Am I missing a way to mark the instance method without marking the interfaces too?

[jtschuster]

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.

[MichalStrehovsky]

I'd leave final review for Sven, but this looks reasonable to me. The interface stripping optimization is difficult to think about and source of many bugs (dotnet/linker#1235, dotnet/linker#2056 off the top of my head, but also this one and probably others). I always wondered how much it actually saves.

It's unfortunate we don't mark the overrides directly in the mark step and instead have to rely on indirect signals to decide whether they should be kept. It's a contrast with how e.g. interface lists are modeled (interface list is modeled in Cecil as a list of InterfaceImplementation (not a list of TypeReference) and we mark individual InterfaceImplementations. Override list is modeled as a list of MethodReference and we don't have a good thing to mark, unless we start marking a tuple [TypeDefinition, MethodReference]).

[jtschuster]

The interface stripping optimization is difficult to think about and source of many bugs... I always wondered how much it actually saves.

I looked into this and it looks like with the unusedinterfaces optimization, the trimmer removes an additional ~13kb from ~2.5mb on a hello world and ~16kb from ~26mb in the ASP.NET Core benchmarks application. Maybe it would be worth disabling by default.

[vitek-karas]

Maybe it would be worth disabling by default.

Just an idea - there's other value in this, not just size. This causes failures if somebody incorrectly annotates their code (I'm thinking about the DI problems where this showed up). In a way it helps with making things more trim compatible :-). But maybe it's not worth the pain....