Trim static interfaces (#2741)

Don't mark static interface methods when called on a concrete type, and removed the MethodImpl / Override if the interface method is kept.

Co-authored-by: vitek-karas <[email protected]>
Co-authored-by: vitek-karas <[email protected]>

docs/removal-behavior.md

@@ -0,0 +1,69 @@
+# Removal Behavior for interfaces
+
+## `unusedinterfaces` optimization
+
+The `unusedinterfaces` optimization controls whether or not trimmin may remove the `interfaceimpl` annotation which denotes whether a class implements an interface. When the optimization is off, the linker will not remove the annotations regardless of the visibility of the interface (even private interface implementations will be kept). However, unused methods from interfaces may still be removed, as well as `.override` directives from methods that implement an interface method. When the optimization is on and the linker can provably determine that an interface will not be used on a type, the annotation will be removed. In order to know whether it is safe to remove an interface implementation, it is necessary to have a "global" view of the code. In other words, if an assembly is passed without selecting `link` for the `action` option for the linker, all classes that implement interfaces from that assembly will keep those interface implementation annotations. For example, if you implement `System.IFormattable` from the `System.Runtime` assembly, but pass the assembly with `--action copy System.Runtime`, the interface implementation will be kept even if your code doesn't use it.
+
+## Static abstract interface methods
+
+The linker's behavior for methods declared on interfaces as `static abstract` like below are defined in the following cases using the example interface and class below:
+
+```C#
+interface IFoo
+{
+    static abstract int GetNum();
+}
+
+class C : IFoo
+{
+    static int GetNum() => 1;
+}
+```
+
+### Method call on concrete type
+
+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.
+
+Example:
+
+In the following program, `C.GetNum()` would be kept, but `IFoo.GetNum()` would be removed.
+
+```C#
+public class Program
+{
+    public static void Main()
+    {
+        C.GetNum();
+    }
+}
+```
+
+### Method call on a constrained type parameter
+
+On a call to a static abstract interface method that is accessed through a constrained type parameter, the interface method is rooted, as well as every implementation method on every type.
+
+Example:
+
+In the following program, `C.GetNum()`, `IFoo.GetNum()`, and `C2.GetNum()` are all kept.
+
+```C#
+public class C2 : IFoo
+{
+    static int GetNum() => 2;
+}
+public class Program
+{
+    public static void Main()
+    {
+        M<C>();
+    }
+    public static void M<T>() where T : IFoo
+    {
+        T.GetNum();
+    }
+}
+```
+
+# `.override` directive and `MethodImpl` marking
+
+The linker currently does not mark `.override` or `MethodImpl` relationships. It will only remove the relationship if one of the methods is removed. These relationships are not always necessary, and future updates could add the ability to remove these relationships even if both methods are not trimmed.

src/linker/Linker.Steps/MarkStep.cs

@@ -573,6 +573,10 @@ void ProcessVirtualMethods ()
 			}
 		}
 
+		/// <summary>
+		/// Does extra handling of marked types that have interfaces when it's necessary to know what types are marked or instantiated.
+		/// e.g. Marks the "implements interface" annotations and removes override annotations for static interface methods.
+		/// </summary>
 		void ProcessMarkedTypesWithInterfaces ()
 		{
 			// We may mark an interface type later on.  Which means we need to reprocess any time with one or more interface implementations that have not been marked
@@ -2288,7 +2292,7 @@ void MarkGenericParameter (GenericParameter parameter)
 
 		/// <summary>
 		/// Returns true if any of the base methods of the <paramref name="method"/> passed is in an assembly that is not trimmed (i.e. action != trim).
-		/// Meant to be used to determine whether methods should be marked regardless of whether it is instantiated or not. 
+		/// Meant to be used to determine whether methods should be marked regardless of whether it is instantiated or not.
 		/// </summary>
 		/// <remarks>
 		/// When the unusedinterfaces optimization is on, this is used to mark methods that override an abstract method from a non-link assembly and must be kept.
@@ -3049,8 +3053,16 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
 				}
 			}
 
+			// Mark overridden methods except for static interface methods
 			if (method.HasOverrides) {
 				foreach (MethodReference ov in method.Overrides) {
+					// Method implementing a static interface method will have an override to it - note nonstatic methods usually don't unless they're explicit.
+					// 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);
 				}

src/linker/Linker.Steps/SweepStep.cs

@@ -453,6 +453,8 @@ protected virtual void SweepMethods (Collection<MethodDefinition> methods)
 
 				SweepCustomAttributes (method.MethodReturnType);
 
+				SweepOverrides (method);
+
 				if (!method.HasParameters)
 					continue;
 
@@ -467,6 +469,15 @@ protected virtual void SweepMethods (Collection<MethodDefinition> methods)
 			}
 		}
 
+		void SweepOverrides (MethodDefinition method)
+		{
+			for (int i = 0; i < method.Overrides.Count;) {
+				if (Context.Resolve (method.Overrides[i]) is MethodDefinition ov && ShouldRemove (ov))
+					method.Overrides.RemoveAt (i);
+				else
+					i++;
+			}
+		}
 		void SweepDebugInfo (Collection<MethodDefinition> methods)
 		{
 			List<ScopeDebugInformation>? sweptScopes = null;

test/Mono.Linker.Tests.Cases.Expectations/Assertions/KeptOverrideAttribute.cs

@@ -0,0 +1,27 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System;
+
+namespace Mono.Linker.Tests.Cases.Expectations.Assertions
+{
+	/// <summary>
+	/// Used to ensure that a method should keep an 'override' annotation for a method in the supplied base type.
+	/// The absence of this attribute does not enforce that the override is removed -- this is different from other Kept attributes
+	/// To enforce the removal of an override, use <see cref="RemovedOverrideAttribute"/>.
+	/// Fails in tests if the method doesn't have the override method in the original or linked assembly.
+	/// </summary>
+	/// <seealso cref="RemovedOverrideAttribute" />
+	[AttributeUsage (AttributeTargets.Method, AllowMultiple = true, Inherited = false)]
+	public class KeptOverrideAttribute : KeptAttribute
+	{
+		public Type TypeWithOverriddenMethodDeclaration;
+
+		public KeptOverrideAttribute (Type typeWithOverriddenMethod)
+		{
+			if (typeWithOverriddenMethod == null)
+				throw new ArgumentNullException (nameof (typeWithOverriddenMethod));
+			TypeWithOverriddenMethodDeclaration = typeWithOverriddenMethod;
+		}
+	}
+}

test/Mono.Linker.Tests.Cases.Expectations/Assertions/RemovedOverrideAttribute.cs

@@ -0,0 +1,25 @@
+// Copyright (c) .NET Foundation and contributors. All rights reserved.
+// Licensed under the MIT license. See LICENSE file in the project root for full license information.
+
+using System;
+
+namespace Mono.Linker.Tests.Cases.Expectations.Assertions
+{
+	/// <Summary>
+	/// Used to ensure that a method should remove an 'override' annotation for a method in the supplied base type.
+	/// Fails in tests if the method has the override method in the linked assembly,
+	///		or if the override is not found in the original assembly
+	/// </Summary>
+	/// <seealso cref="KeptOverrideAttribute" />
+	[AttributeUsage (AttributeTargets.Method, AllowMultiple = true, Inherited = false)]
+	public class RemovedOverrideAttribute : BaseInAssemblyAttribute
+	{
+		public Type TypeWithOverriddenMethodDeclaration;
+		public RemovedOverrideAttribute (Type typeWithOverriddenMethod)
+		{
+			if (typeWithOverriddenMethod == null)
+				throw new ArgumentException ("Value cannot be null or empty.", nameof (typeWithOverriddenMethod));
+			TypeWithOverriddenMethodDeclaration = typeWithOverriddenMethod;
+		}
+	}
+}

test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs

@@ -17,32 +17,84 @@ public class InterfaceVariants
 		public static void Main ()
 		{
 			Type t = typeof (UninstantiatedPublicClassWithInterface);
-			t = typeof (UninstantiatedPublicClassWithImplicitlyImplementedInterface);
+			t = typeof (UninstantiatedClassWithImplicitlyImplementedInterface);
 			t = typeof (UninstantiatedPublicClassWithPrivateInterface);
+			t = typeof (ImplementsUsedStaticInterface.InterfaceMethodUnused);
+			t = typeof (ImplementsUnusedStaticInterface.InterfaceMethodUnused);
 
-			UninstantiatedPublicClassWithInterface.InternalStaticInterfaceMethodUsed ();
-			InstantiatedClassWithInterfaces.InternalStaticInterfaceMethodUsed ();
-
+			ImplementsUnusedStaticInterface.InterfaceMethodUsedThroughImplementation.InternalStaticInterfaceMethodUsedThroughImplementation ();
+			GenericMethodThatCallsInternalStaticInterfaceMethod
+				<ImplementsUsedStaticInterface.InterfaceMethodUsedThroughInterface> ();
 			// Use all public interfaces - they're marked as public only to denote them as "used"
 			typeof (IPublicInterface).RequiresPublicMethods ();
 			typeof (IPublicStaticInterface).RequiresPublicMethods ();
+			var ___ = new InstantiatedClassWithInterfaces ();
+		}
+
+		[Kept]
+		internal static void GenericMethodThatCallsInternalStaticInterfaceMethod<T> () where T : IStaticInterfaceUsed
+		{
+			T.StaticMethodUsedThroughInterface ();
+		}
+
+		[Kept]
+		class ImplementsUsedStaticInterface
+		{
+
+			[Kept]
+			[KeptInterface (typeof (IStaticInterfaceUsed))]
+			internal class InterfaceMethodUsedThroughInterface : IStaticInterfaceUsed
+			{
+				[Kept]
+				[KeptOverride (typeof (IStaticInterfaceUsed))]
+				public static void StaticMethodUsedThroughInterface ()
+				{
+				}
+				public static void UnusedMethod () { }
+			}
+
+			[Kept]
+			[KeptInterface (typeof (IStaticInterfaceUsed))]
+			internal class InterfaceMethodUnused : IStaticInterfaceUsed
+			{
+				[Kept]
+				[KeptOverride (typeof (IStaticInterfaceUsed))]
+				public static void StaticMethodUsedThroughInterface ()
+				{
+				}
+				public static void UnusedMethod () { }
+			}
+		}
+
+		[Kept]
+		internal class ImplementsUnusedStaticInterface
+		{
+			[Kept]
+			internal class InterfaceMethodUsedThroughImplementation : IStaticInterfaceUnused
+			{
+				[Kept]
+				[RemovedOverride (typeof (IStaticInterfaceUnused))]
+				public static void InternalStaticInterfaceMethodUsedThroughImplementation () { }
+			}
 
-			var a = new InstantiatedClassWithInterfaces ();
+			[Kept]
+			internal class InterfaceMethodUnused : IStaticInterfaceUnused
+			{
+				public static void InternalStaticInterfaceMethodUsedThroughImplementation () { }
+			}
 		}
 
 		[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,
 			IEnumerator,
 			ICopyLibraryInterface,
 			ICopyLibraryStaticInterface
@@ -69,8 +121,6 @@ public static void InternalStaticInterfaceMethod () { }
 
 			static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }
 
-			[Kept]
-			public static void InternalStaticInterfaceMethodUsed () { }
 
 			[Kept]
 			[ExpectBodyModified]
@@ -101,9 +151,9 @@ static void ICopyLibraryStaticInterface.CopyLibraryExplicitImplementationStaticI
 
 		[Kept]
 		[KeptInterface (typeof (IFormattable))]
-		public class UninstantiatedPublicClassWithImplicitlyImplementedInterface : IInternalInterface, IFormattable
+		public class UninstantiatedClassWithImplicitlyImplementedInterface : IInternalInterface, IFormattable
 		{
-			internal UninstantiatedPublicClassWithImplicitlyImplementedInterface () { }
+			internal UninstantiatedClassWithImplicitlyImplementedInterface () { }
 
 			public void InternalInterfaceMethod () { }
 
@@ -122,15 +172,13 @@ 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,
 			IEnumerator,
 			ICopyLibraryInterface,
 			ICopyLibraryStaticInterface
@@ -158,9 +206,6 @@ public static void InternalStaticInterfaceMethod () { }
 
 			static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }
 
-			[Kept]
-			public static void InternalStaticInterfaceMethodUsed () { }
-
 			[Kept]
 			bool IEnumerator.MoveNext () { throw new PlatformNotSupportedException (); }
 
@@ -225,13 +270,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 IStaticInterfaceUnused
+		{
+			static abstract void InternalStaticInterfaceMethodUsedThroughImplementation ();
+		}
+
+		// The interface methods themselves are used through the interface
 		[Kept]
-		internal interface IInternalStaticInterfaceWithUsedMethod
+		internal interface IStaticInterfaceUsed
 		{
-			[Kept] // https://github.com/dotnet/linker/issues/2733
-			static abstract void InternalStaticInterfaceMethodUsed ();
+			[Kept]
+			static abstract void StaticMethodUsedThroughInterface ();
+
+			static abstract void UnusedMethod ();
 		}
 
 		private interface IPrivateInterface

test/Mono.Linker.Tests.Cases/Libraries/RootLibrary.cs

@@ -217,6 +217,7 @@ public void InternalInterfaceMethod () { }
 			void IInternalInterface.ExplicitImplementationInternalInterfaceMethod () { }
 
 			[Kept]
+			[RemovedOverride (typeof (IInternalStaticInterface))]
 			public static void InternalStaticInterfaceMethod () { }
 
 			static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }
@@ -300,6 +301,7 @@ public void InternalInterfaceMethod () { }
 			void IInternalInterface.ExplicitImplementationInternalInterfaceMethod () { }
 
 			[Kept]
+			[RemovedOverride (typeof (IInternalStaticInterface))]
 			public static void InternalStaticInterfaceMethod () { }
 
 			static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }
@@ -366,7 +368,6 @@ internal interface IInternalInterface
 		[Kept]
 		internal interface IInternalStaticInterface
 		{
-			[Kept] // https://github.com/dotnet/linker/issues/2733
 			static abstract void InternalStaticInterfaceMethod ();
 
 			static abstract void ExplicitImplementationInternalStaticInterfaceMethod ();