Skip to content

Trim static interfaces #2741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jtschuster merged 32 commits into from
Apr 20, 2022
Merged

Trim static interfaces #2741

Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
0388e48
Consider all public types in a library as instantiated
vitek-karas Aug 31, 2021
39ffb9f
Change the solution to rely on the optimization setting only
vitek-karas Aug 31, 2021
e5b3788
wip
jtschuster Mar 25, 2022
61115f6
Add implicit interface implementation case
jtschuster Mar 28, 2022
c4ebd45
Edit tests to remove issue-specific code and names
jtschuster Mar 28, 2022
d7cc881
Mark members of CollectedType as kept
jtschuster Mar 28, 2022
3a8b76d
Add private interface test and simplify external interface example
jtschuster Mar 31, 2022
1c04ad2
Replace early exit for non-interfaces
jtschuster Apr 1, 2022
be37029
Merge branch 'main' into FixLibraryInterface
jtschuster Apr 1, 2022
7ad5afc
License headers and use IsMethodNeededByTypeDueToPreservedScope inste…
jtschuster Apr 1, 2022
07143dd
Add check for static methods before skipping virtual marking
jtschuster Apr 5, 2022
a7c7e13
Add more clarifying comments, move static method check, rename method
jtschuster Apr 6, 2022
64692a0
Renames and comment cleanup + tests
vitek-karas Apr 7, 2022
4ea2991
More tests for interface behavior
vitek-karas Apr 8, 2022
578c686
wip
jtschuster Apr 11, 2022
798e87f
Remove static interface methods that are only accessed by implemetations
jtschuster Apr 11, 2022
3fe5a52
remove commented out lines
jtschuster Apr 12, 2022
9908282
Lint and add deleted line
jtschuster Apr 12, 2022
3b03b77
Another test using interface through reflection
jtschuster Apr 12, 2022
4ae7888
Merge branch 'main' of https://github.com/dotnet/linker into trimStat…
jtschuster Apr 12, 2022
7df4117
Remove comments
jtschuster Apr 12, 2022
0b51131
Only remove override annotation if the static interface method is rem…
jtschuster Apr 13, 2022
e61826c
Add KeptOverride attribute and test support
jtschuster Apr 14, 2022
31a2447
Add Attribute files
jtschuster Apr 14, 2022
6e1aead
formatting
jtschuster Apr 14, 2022
52c4bb4
Move override removal to sweep step and add doc defining behavior
jtschuster Apr 18, 2022
854d08b
formatting
jtschuster Apr 19, 2022
14f71b9
Rework docs, rename test methods and types
jtschuster Apr 19, 2022
8edb5ac
Resolve overrides before sweeping
jtschuster Apr 19, 2022
0c79f06
formatting...
jtschuster Apr 19, 2022
ca0e253
Clarify wording in docs, move VerifyOverrides, rename vars
jtschuster Apr 20, 2022
c32a329
Add note about MethodImpl/Override marking
jtschuster Apr 20, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 26 additions & 0 deletions src/linker/Linker.Steps/MarkStep.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -592,6 +592,7 @@ void ProcessMarkedTypesWithInterfaces ()

using (ScopeStack.PushScope (scope)) {
MarkInterfaceImplementations (type);
RemoveStaticInterfaceOverrideAnnotations (type);
}
}
}
Expand Down Expand Up @@ -2265,6 +2266,24 @@ void MarkInterfaceImplementations (TypeDefinition type)
}
}

/// <summary>
/// Removes the 'override' annotation for implementation of static interface methods when the interface method is removed.
/// </summary>
void RemoveStaticInterfaceOverrideAnnotations (TypeDefinition type)
Copy link
Member

@vitek-karas vitek-karas Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@jtschuster jtschuster Apr 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jtschuster jtschuster Apr 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
foreach (var method in type.Methods) {
// Modify overrides in place
for (int i = 0; i < method.Overrides.Count;) {
if (Context.Resolve(method.Overrides[i].DeclaringType)?.IsInterface == true
&& Context.Resolve (method.Overrides[i])?.IsStatic == true
&& !Annotations.IsMarked (method.Overrides[i]))
method.Overrides.RemoveAt (i);
else
i++;
}
}
}

void MarkGenericParameterProvider (IGenericParameterProvider provider)
{
if (!provider.HasGenericParameters)
Expand Down Expand Up @@ -3049,8 +3068,15 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
}
}

// Mark overrides except for static interface methods
Copy link
Member

@agocke agocke Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@jtschuster jtschuster Apr 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@vitek-karas vitek-karas Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@sbomer sbomer Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (method.HasOverrides) {
foreach (MethodReference ov in method.Overrides) {
// Don't mark overrides for static interface methods.
// Since they can only be called on a concrete type and not the interface, these methods can safely be removed in some cases
Copy link
Member

@vitek-karas vitek-karas Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if (Context.Resolve (ov)?.IsStatic == true
&& Context.Resolve (ov.DeclaringType)?.IsInterface == true) {
continue;
}
MarkMethod (ov, new DependencyInfo (DependencyKind.MethodImplOverride, method));
MarkExplicitInterfaceImplementation (method, ov);
}
Expand Down
12 changes: 12 additions & 0 deletions ....Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/UnreachableBlockTests.g.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ public Task InstanceMethodSubstitutions ()
return RunTest (allowMissingWarnings: true);
}

[Fact]
public Task MethodArgumentPropagation ()
{
return RunTest (allowMissingWarnings: true);
}

[Fact]
public Task MethodWithParametersSubstitutions ()
{
Expand All @@ -55,6 +61,12 @@ public Task ReplacedReturns ()
return RunTest (allowMissingWarnings: true);
}

[Fact]
public Task ResultInliningNotPossible ()
{
return RunTest (allowMissingWarnings: true);
}

[Fact]
public Task SimpleConditionalProperty ()
{
Expand Down
66 changes: 52 additions & 14 deletions test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,29 +20,61 @@ public static void Main ()
t = typeof (UninstantiatedPublicClassWithImplicitlyImplementedInterface);
t = typeof (UninstantiatedPublicClassWithPrivateInterface);

UninstantiatedPublicClassWithInterface.InternalStaticInterfaceMethodUsed ();
InstantiatedClassWithInterfaces.InternalStaticInterfaceMethodUsed ();

UninstantiatedPublicClassWithInterface.InternalStaticInterfaceMethodUsedThroughImplementation ();
Copy link
Member

@agocke agocke Apr 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

InstantiatedClassWithInterfaces.InternalStaticInterfaceMethodUsedThroughImplementation ();
GenericMethodThatCallsInternalStaticInterfaceMethod
<ImplementsInternalStaticInterfaceWithInterfaceDefinitionUsedThroughGeneric> ();
typeof (InterfaceVariants).GetMethod ("GenericMethodThatCallsInternalStaticInterfaceMethod")
.MakeGenericMethod (typeof (ImplementsInternalStaticInterfaceWithInterfaceDefinitionNotUsedThroughGeneric))
.Invoke (null, null);
// Use all public interfaces - they're marked as public only to denote them as "used"
typeof (IPublicInterface).RequiresPublicMethods ();
typeof (IPublicStaticInterface).RequiresPublicMethods ();

var a = new InstantiatedClassWithInterfaces ();
}

[Kept]
internal static void GenericMethodThatCallsInternalStaticInterfaceMethod<T> () where T : IInternalStaticInterfaceWithInterfaceDefinitionUsed
Copy link
Member

@agocke agocke Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

{
T.InternalStaticInterfaceUsedThroughInterface ();
}

[Kept]
[KeptInterface (typeof (IInternalStaticInterfaceWithInterfaceDefinitionUsed))]
internal class ImplementsInternalStaticInterfaceWithInterfaceDefinitionUsedThroughGeneric : IInternalStaticInterfaceWithInterfaceDefinitionUsed
{
[Kept]
public static void InternalStaticInterfaceUsedThroughInterface ()
{
}
public static void UnusedMethod () { }
}

[Kept]
[KeptInterface (typeof (IInternalStaticInterfaceWithInterfaceDefinitionUsed))]
internal class ImplementsInternalStaticInterfaceWithInterfaceDefinitionNotUsedThroughGeneric : IInternalStaticInterfaceWithInterfaceDefinitionUsed
Copy link
Member

@agocke agocke Apr 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
[Kept]
public static void InternalStaticInterfaceUsedThroughInterface ()
{
}

public static void UnusedMethod () { }
}

[Kept]
[KeptInterface (typeof (IEnumerator))]
[KeptInterface (typeof (IPublicInterface))]
[KeptInterface (typeof (IPublicStaticInterface))]
[KeptInterface (typeof (IInternalStaticInterfaceWithUsedMethod))] // https://github.com/dotnet/linker/issues/2733
[KeptInterface (typeof (ICopyLibraryInterface))]
[KeptInterface (typeof (ICopyLibraryStaticInterface))]
public class UninstantiatedPublicClassWithInterface :
IPublicInterface,
IPublicStaticInterface,
IInternalInterface,
IInternalStaticInterface,
IInternalStaticInterfaceWithUsedMethod,
IInternalStaticInterfaceWithUsedMethodImplementation,
IEnumerator,
ICopyLibraryInterface,
ICopyLibraryStaticInterface
Expand Down Expand Up @@ -70,7 +102,7 @@ public static void InternalStaticInterfaceMethod () { }
static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }

[Kept]
public static void InternalStaticInterfaceMethodUsed () { }
public static void InternalStaticInterfaceMethodUsedThroughImplementation () { }

[Kept]
[ExpectBodyModified]
Expand Down Expand Up @@ -122,15 +154,14 @@ public string ToString (string format, IFormatProvider formatProvider)
[KeptInterface (typeof (IEnumerator))]
[KeptInterface (typeof (IPublicInterface))]
[KeptInterface (typeof (IPublicStaticInterface))]
[KeptInterface (typeof (IInternalStaticInterfaceWithUsedMethod))] // https://github.com/dotnet/linker/issues/2733
[KeptInterface (typeof (ICopyLibraryInterface))]
[KeptInterface (typeof (ICopyLibraryStaticInterface))]
public class InstantiatedClassWithInterfaces :
IPublicInterface,
IPublicStaticInterface,
IInternalInterface,
IInternalStaticInterface,
IInternalStaticInterfaceWithUsedMethod,
IInternalStaticInterfaceWithUsedMethodImplementation,
IEnumerator,
ICopyLibraryInterface,
ICopyLibraryStaticInterface
Expand Down Expand Up @@ -159,7 +190,7 @@ public static void InternalStaticInterfaceMethod () { }
static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }

[Kept]
public static void InternalStaticInterfaceMethodUsed () { }
public static void InternalStaticInterfaceMethodUsedThroughImplementation () { }

[Kept]
bool IEnumerator.MoveNext () { throw new PlatformNotSupportedException (); }
Expand Down Expand Up @@ -225,13 +256,20 @@ internal interface IInternalStaticInterface
static abstract void ExplicitImplementationInternalStaticInterfaceMethod ();
}

// The interface methods themselves are not used, but the implentation of these methods is
// https://github.com/dotnet/linker/issues/2733
// The interface methods themselves are not used, but the implementation of these methods is
internal interface IInternalStaticInterfaceWithUsedMethodImplementation
{
static abstract void InternalStaticInterfaceMethodUsedThroughImplementation ();
}

// The interface methods themselves are used through the interface
[Kept]
internal interface IInternalStaticInterfaceWithUsedMethod
internal interface IInternalStaticInterfaceWithInterfaceDefinitionUsed
{
[Kept] // https://github.com/dotnet/linker/issues/2733
static abstract void InternalStaticInterfaceMethodUsed ();
[Kept]
static abstract void InternalStaticInterfaceUsedThroughInterface ();

static abstract void UnusedMethod ();
}

private interface IPrivateInterface
Expand Down
2 changes: 1 addition & 1 deletion test/Mono.Linker.Tests.Cases/Libraries/RootLibrary.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ internal interface IInternalInterface
[Kept]
internal interface IInternalStaticInterface
{
[Kept] // https://github.com/dotnet/linker/issues/2733
//[Kept] // https://github.com/dotnet/linker/issues/2733
static abstract void InternalStaticInterfaceMethod ();

static abstract void ExplicitImplementationInternalStaticInterfaceMethod ();
Expand Down