Re-add static interface trimming with more testing #2791
Conversation
Make sure that MethodImpls point to an interface that the type implements or a base type that is inherited from.
Make sure that MethodImpls point to an interface that the type implements or a base type that is inherited from.
src/linker/Linker.Steps/MarkStep.cs
Outdated
| if (Context.Resolve (ov)?.IsStatic == true | ||
| && Context.Resolve (ov.DeclaringType)?.IsInterface == true | ||
| // Public methods can be called directly on the type. If it non-public it is an explicit implementation and if the method is marked we need to mark the override. | ||
| && method.IsPublic) { |
There was a problem hiding this comment.
What goes wrong if we don't mark the interface method for non-public explicit implementations here? I understand that the non-public implementation will only be reached through the interface method, but I would have roughly expected that logic elsewhere, in ProcessMarkedTypesWithInterfaces, would take care of that. This looks like it's doing the right thing but I would like to understand if it's necessary.
There was a problem hiding this comment.
I would also prefer we remove the IsPublic check. What if it's internal and there InternalsVisibleTo applied - then it is effectively public. Visibility should have basically no effect here. The explicit implementation methods should be never accessed directly as C# doesn't let you do that - so the only way is via reflection, and in that case I think it's OK to overmark, definitely not something I would optimize for.
There was a problem hiding this comment.
Technically speaking we should be able to avoid marking overrides for interface methods completely (not just for static interfaces) - but that's a bigger/unrelated change. (And it is unlikely to actually do much since non-static interfaces do not use overrides in C#, so the cases where this would happen are very limitted in real world).
There was a problem hiding this comment.
From what I can tell, a method implementing an interface method must be public to implicitly implement the method or private for an explicit implementation, so I don't think we'd run into the internal with InternalsVisibleTo issue.
I think I could avoid of complication by looking from base -> override instead of override -> base like I do here. I didn't see the AnnotationStore.GetOverrides which does the inverse of Cecil's .Overrides. If I check if a method is a static interface method and kept, I can mark the methods in GetOverrides as well as the interface implementation.
I would have roughly expected that logic elsewhere, in ProcessMarkedTypesWithInterfaces
I agree, that would make sense to me, but at the moment that method returns early if the method isn't instantiated (or relevant to variant casting). If we move the logic for static interface methods, we'd have to iterate through each method to check if any override a static interface method before we can say we don't need an interface implementation. I think it could be worth it to keep the related logic in the same place though.
There was a problem hiding this comment.
I though that the reverse direction (from base to overrides) is already there in the ProcessMarkedTypesWithInterfaces (or similar). The code here is the opposite direction which is from implementation (override) to base.
There was a problem hiding this comment.
I think here we should keep marking interface implementations for explicitly implemented non-static interface methods. I moved the logic to mark static interface method overrides from base -> implementation to ProcessOverrides.
If we move marking the interface implementations for explicit interface method implementations to ProcessMArkedTypesWithInterfaces we would be duplicating work with ProcessMethod because we'd have to look at each method in the type to determine if we should keep the interface impl.
I think the way it is now makes the most sense to me, but I can rework if we think another way would be better.
...Cases/Inheritance.Interfaces/StaticInterfaceMethods/StaticAbstractInterfaceMethodsLibrary.cs
Outdated
Show resolved
Hide resolved
test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs
Outdated
Show resolved
Hide resolved
...slynAnalyzer.Tests.TestCaseGenerator/Inheritance.Interfaces.StaticInterfaceMethodsTests.g.cs
Outdated
Show resolved
Hide resolved
...Cases/Inheritance.Interfaces/StaticInterfaceMethods/StaticAbstractInterfaceMethodsLibrary.cs
Show resolved
Hide resolved
|
Also, great test coverage here! |
Co-authored-by: Sven Boemer <sbomer@gmail.com>
…linker into staticInterfacesBack
|
I've updated the marking of static interface methods to be like the virtual method marking, where we store each static interface method that is marked, then mark the overriding methods later. The interface implementations are then marked in ProcessOverride. |
…)" This reverts commit ff646e0.
…otnet#2791)" (dotnet#2841)" This reverts commit 4bed0da.
…otnet#2791)" (dotnet#2841)" This reverts commit 4bed0da.
…otnet#2791)" (dotnet#2841)" This reverts commit 4bed0da.
Enables more precise removal of static abstract interface methods and adds more tests than previously. Co-authored-by: Sven Boemer <sbomer@gmail.com> Commit migrated from dotnet/linker@ff646e0
…nker#2791)" (dotnet/linker#2841) This reverts commit dotnet/linker@ff646e0. Commit migrated from dotnet/linker@4bed0da
Revert the commit that reverted static interface trimming and add more comprehensive tests.
I also found that sometimes
method.Overrides[i].Resolve()would sometimes return null if it had been removed earlier, in which case we want to remove the override. The code has been updated to account for this case.This is missing library tests, but I wanted to make sure that I don't miss any cases before I move to that. Please let me know if there are any other cases that needs to be tested.