Trim static interfaces

[jtschuster]

This removes a static interface method from the list of Overrides of the implementation method. Since the interface method is removed as an override, it will not mark the interface method by default. However, if the interface method is called through the interface on a constrained type parameter (T.StaticInterfaceMethodImplementation() where T: IInterfaceWithStaticMethod).

Note we have to completely remove the interface method from the list of overrides. If we only skip the override from the marking, Cecil will fail to write the assembly because the overridden method can't be found anywhere.

[vitek-karas]

What happens if we decide to keep the method because it's called? It would probably still remain removed from the overrides, right? That sounds problematic. I don't know the exact reasons why the compiler generates the explicit override, but I assume there is a reason - and linker should not go against it.

[jtschuster]

Great point, I changed it to only remove the annotation if the interface method is also removed

[vitek-karas]

Is there a way to write a test for this?
I don't know if the test infra actually verifies the "overrides" (maybe it should at least as an opt-in)

[jtschuster]

It doesn't look like there's anything in the test infra yet, but I'll look into it more and add a KeptOverride/NoOverride attribute to check for that

[jtschuster]

I've added a couple attribute to check for the presence/removal of the override annotations. They only perform a check if there is a KeptOverride or RemovedOverride, so it doesn't affect any other tests.

[jtschuster]

I'm not sure how this implementation would affect a derived class that inherits from a base class that implements a static interface when the derived class overrides the method. I can't think of any issues that this would cause, though.

[agocke]

I'm not sure I understand the goal here. Just to clarify some terminology, in C#, interface methods are considered implemented, but not overridden. My understanding of the intent here is that you want to trim particular interface implementations for static virtual interface methods. I'm just not sure of the conditions when.

If we take the example:

interface I1
{
    static abstract int M1();
}
class A : I1
{
    public static int M1() => 0;
}
class B : A, I1
{
    static int I1.M1() => 2;
}

In what circumstances would I1.M1 implementations be trimmed? Or, in other words, what would need to be done to mark their implementations?

[agocke]

Don't we want to mark the MethodImpl for these calls if and only if there's a constrained call to the target method?

[jtschuster]

Yes, but here we are marking the methods themselves, not the MethodImpl. The static abstract interface methods themselves are marked elsewhere when the linker finds constrained calls to them. At the moment there's still no real "marking" of the MethodImpl, it just checks if the overridden method is marked, and if it's not, it removes the MethodImpl / Override.

We could separate marking the Overrides list from marking the Overridden methods themselves, but I feel like that would lead to more (issues where the Override / MethodImpl is incorrectly marked and the override method is correctly removed) than (issues where the Override / MethodImpl is correctly marked and the override method is incorrectly removed).

@sbomer do you have any thoughts on whether to mark overrides separately?

[vitek-karas]

I actually like that we don't mark overrides/methodimpl explicitly:

• With your change in the sweep step - there's no way to make the IL inconsistent - it will fix itself and linker just needs to decide if it keeps a certain method or on.
• Linker in general doesn't "mutate" things, it only "deletes" them. So it should not be linker's decision if two methods should have the methodimpl between them or not - that decision should come from the input. If linker decides to keep both methods - the methodimpl between should be there if and only if it was there on the input.

[sbomer]

Agreed, I like handling this in SweepStep and I like @vitek-karas's reasoning here.

[agocke]

Personally, I think these names are getting long enough that they're not very readable, but I'll accede 😊

[agocke]

I can't quite tell if this is meant to mean that the type is never used through a generic substitution. If so, that would mean that the InternalStaticInterfaceUsedThroughInterface method below has its override kept regardless.

I don't necessarily think this is the wrong behavior, but it's good to note. We might want to file a bug about a potential improvement in the future, if that's something we're interested in.

[agocke]

I'd follow a consistent style here of

[rule]
[example]

Instead of something more like prose. Basically, this is code, but in English.

I think the phrasing could also be more specific in what's rooted and what's not.

Something like:

On a direct call to a static method which implements a static interface method, only the body is rooted, not its associated MethodImpl. Similarly, the interface method which it implements is not rooted.

[agocke]

Overall, if we imagine we had a complete spec here, I expect it would look something like:

[IL instruction]
[description of things it roots]
[example]

[agocke]

What's the significance of Internal here? It's repeated a lot throughout the code. I can't think of a reason why that would make a difference.

[vitek-karas]

Please add a comment that this behaves differently from the other Kept attributes - almost all the other ones test both sides, that is if the attribute is not there it means the thing should NOT be in the output. For example if a certain type doesn't have a Kept attribute on it, we expected the type to not survive linking.
We do have some other attributes which are like this, but I think it's a good idea to at least document it - and maybe mention the other attribute RemoveOverrideAttribute as its counterpart.

[vitek-karas]

As discussed offline I wonder if this can be done for all interface methods (static and non-static) - but that's a separate discussion.
I think the comment here is a bit misleading, maybe a better way to describe this would be something like:

// Method implementing a static interface method will have an override to it.
// Calling the implementation method directly has no impact on the interface, and as such it should not mark the interface or its method.
// Only if the interface method is referenced, then all the methods which implemented must be kept, but not the other way round.

[vitek-karas]

I actually like that we don't mark overrides/methodimpl explicitly:

• With your change in the sweep step - there's no way to make the IL inconsistent - it will fix itself and linker just needs to decide if it keeps a certain method or on.
• Linker in general doesn't "mutate" things, it only "deletes" them. So it should not be linker's decision if two methods should have the methodimpl between them or not - that decision should come from the input. If linker decides to keep both methods - the methodimpl between should be there if and only if it was there on the input.

[agocke]

If linker decides to keep both methods - the methodimpl between should be there if and only if it was there on the input.

Interesting approach -- this is slightly more conservative than necessary, right? For instance, if the given type is never substituted for a generic parameter, we can be certain that the methodimpl isn't necessary for that method.

[jtschuster]

If linker decides to keep both methods - the methodimpl between should be there if and only if it was there on the input.

Interesting approach -- this is slightly more conservative than necessary, right? For instance, if the given type is never substituted for a generic parameter, we can be certain that the methodimpl isn't necessary for that method.

I wonder if we could get into trouble with something like

void Method (IFoo x)
{
MethodInfo method = (TypeConstrainedGenericMethod);
method.MakeGenericMethod(x.GetType());
method.Invoke();
}

I think we could perhaps delay marking all implementations of static interface methods, then only mark the override if the InterfaceImpl is marked by something else / remove the MethodImpl if the interfaceImpl is not marked. From there we could even remove the implementation method if it's not called directly and there's no MethodImpl. I'm just not sure the size reductions would be worth the complexity required, though.

[sbomer]

Thank you!

[agocke]

I wonder if we could get into trouble with something like

void Method (IFoo x)
{
MethodInfo method = (TypeConstrainedGenericMethod);
method.MakeGenericMethod(x.GetType());
method.Invoke();
}

Yeah, I didn't really go into reflection here. I guess my more general point was: it sounds like what was being described was that the presence of an abstract interface method and an associated implementation would entail the MethodImpl between them. I'm not sure this is required, simply from a general "rule-based" point of view. However, I'm fine with going down that path if it's safer, or conceptually simpler, or just easier.

Overall, I'm just trying to determine what's necessary vs. what's chosen. In this case it sounds like we're choosing to keep the MethodImpl, but I'm not sure it's necessary and I like to record these things somewhere in case someone comes along later and asks either why we're doing it or whether it could be removed.

[vitek-karas]

@agocke you're right. I was implicitly including the fact that linker currently doesn't have the infra to mark specific overrides and so I assumed we would not be adding that. It's definitely doable though. I don't think it's worth it right now, but it's something we should mention in the doc since we now have one.

[agocke]

LGTM