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 <vitek.karas@microsoft.com>
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>

8 files changed, 258 insertions, 30 deletions

diff --git a/docs/removal-behavior.md b/docs/removal-behavior.md
new file mode 100644
index 000000000..34a2bc3a7
--- /dev/null
+++ b/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.
\ No newline at end of file
diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs
index 2c9c7e038..e164b6df0 100644
--- a/src/linker/Linker.Steps/MarkStep.cs
+++ b/src/linker/Linker.Steps/MarkStep.cs
@@ -573,6 +573,10 @@ namespace Mono.Linker.Steps
 			}
 		}
 
+		/// <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 @@ namespace Mono.Linker.Steps
 
 		/// <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 @@ namespace Mono.Linker.Steps
 				}
 			}
 
+			// 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);
 				}
diff --git a/src/linker/Linker.Steps/SweepStep.cs b/src/linker/Linker.Steps/SweepStep.cs
index 24a2ccbce..560b9cb59 100644
--- a/src/linker/Linker.Steps/SweepStep.cs
+++ b/src/linker/Linker.Steps/SweepStep.cs
@@ -453,6 +453,8 @@ namespace Mono.Linker.Steps
 
 				SweepCustomAttributes (method.MethodReturnType);
 
+				SweepOverrides (method);
+
 				if (!method.HasParameters)
 					continue;
 
@@ -467,6 +469,15 @@ namespace Mono.Linker.Steps
 			}
 		}
 
+		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;
diff --git a/test/Mono.Linker.Tests.Cases.Expectations/Assertions/KeptOverrideAttribute.cs b/test/Mono.Linker.Tests.Cases.Expectations/Assertions/KeptOverrideAttribute.cs
new file mode 100644
index 000000000..62bfd0c15
--- /dev/null
+++ b/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;
+		}
+	}
+}
\ No newline at end of file
diff --git a/test/Mono.Linker.Tests.Cases.Expectations/Assertions/RemovedOverrideAttribute.cs b/test/Mono.Linker.Tests.Cases.Expectations/Assertions/RemovedOverrideAttribute.cs
new file mode 100644
index 000000000..c15cf9a9e
--- /dev/null
+++ b/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;
+		}
+	}
+}
\ No newline at end of file
diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs
index f9d54d009..9b766b42a 100644
--- a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs
+++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs
@@ -17,24 +17,77 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
 		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 :
@@ -42,7 +95,6 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
 			IPublicStaticInterface,
 			IInternalInterface,
 			IInternalStaticInterface,
-			IInternalStaticInterfaceWithUsedMethod,
 			IEnumerator,
 			ICopyLibraryInterface,
 			ICopyLibraryStaticInterface
@@ -69,8 +121,6 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
 
 			static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }
 
-			[Kept]
-			public static void InternalStaticInterfaceMethodUsed () { }
 
 			[Kept]
 			[ExpectBodyModified]
@@ -101,9 +151,9 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
 
 		[Kept]
 		[KeptInterface (typeof (IFormattable))]
-		public class UninstantiatedPublicClassWithImplicitlyImplementedInterface : IInternalInterface, IFormattable
+		public class UninstantiatedClassWithImplicitlyImplementedInterface : IInternalInterface, IFormattable
 		{
-			internal UninstantiatedPublicClassWithImplicitlyImplementedInterface () { }
+			internal UninstantiatedClassWithImplicitlyImplementedInterface () { }
 
 			public void InternalInterfaceMethod () { }
 
@@ -122,7 +172,6 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
 		[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 :
@@ -130,7 +179,6 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
 			IPublicStaticInterface,
 			IInternalInterface,
 			IInternalStaticInterface,
-			IInternalStaticInterfaceWithUsedMethod,
 			IEnumerator,
 			ICopyLibraryInterface,
 			ICopyLibraryStaticInterface
@@ -159,9 +207,6 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
 			static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }
 
 			[Kept]
-			public static void InternalStaticInterfaceMethodUsed () { }
-
-			[Kept]
 			bool IEnumerator.MoveNext () { throw new PlatformNotSupportedException (); }
 
 			[Kept]
@@ -225,13 +270,20 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
 			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
diff --git a/test/Mono.Linker.Tests.Cases/Libraries/RootLibrary.cs b/test/Mono.Linker.Tests.Cases/Libraries/RootLibrary.cs
index 1534573ea..eebdbf67c 100644
--- a/test/Mono.Linker.Tests.Cases/Libraries/RootLibrary.cs
+++ b/test/Mono.Linker.Tests.Cases/Libraries/RootLibrary.cs
@@ -217,6 +217,7 @@ namespace Mono.Linker.Tests.Cases.Libraries
 			void IInternalInterface.ExplicitImplementationInternalInterfaceMethod () { }
 
 			[Kept]
+			[RemovedOverride (typeof (IInternalStaticInterface))]
 			public static void InternalStaticInterfaceMethod () { }
 
 			static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }
@@ -300,6 +301,7 @@ namespace Mono.Linker.Tests.Cases.Libraries
 			void IInternalInterface.ExplicitImplementationInternalInterfaceMethod () { }
 
 			[Kept]
+			[RemovedOverride (typeof (IInternalStaticInterface))]
 			public static void InternalStaticInterfaceMethod () { }
 
 			static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { }
@@ -366,7 +368,6 @@ namespace Mono.Linker.Tests.Cases.Libraries
 		[Kept]
 		internal interface IInternalStaticInterface
 		{
-			[Kept] // https://github.com/dotnet/linker/issues/2733
 			static abstract void InternalStaticInterfaceMethod ();
 
 			static abstract void ExplicitImplementationInternalStaticInterfaceMethod ();
diff --git a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs
index 1388fc52c..b4b293e1f 100644
--- a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs
+++ b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs
@@ -172,12 +172,13 @@ namespace Mono.Linker.Tests.TestCasesRunner
 				linkedMembers.Remove (f.FullName);
 			}
 
-			foreach (var m in original.Methods) {
-				if (verifiedEventMethods.Contains (m.FullName))
+			foreach (var originalMethod in original.Methods) {
+				if (verifiedEventMethods.Contains (originalMethod.FullName))
 					continue;
-				var msign = m.GetSignature ();
-				VerifyMethod (m, linked?.Methods.FirstOrDefault (l => msign == l.GetSignature ()));
-				linkedMembers.Remove (m.FullName);
+				var methodSignature = originalMethod.GetSignature ();
+				var linkedMethod = linked?.Methods.FirstOrDefault (l => methodSignature == l.GetSignature ());
+				VerifyMethod (originalMethod, linkedMethod);
+				linkedMembers.Remove (originalMethod.FullName);
 			}
 		}
 
@@ -212,6 +213,35 @@ namespace Mono.Linker.Tests.TestCasesRunner
 			}
 		}
 
+		void VerifyOverrides (MethodDefinition original, MethodDefinition linked)
+		{
+			if (linked is null)
+				return;
+			var expectedBaseTypesOverridden = new HashSet<string> (original.CustomAttributes
+				.Where (ca => ca.AttributeType.Name == nameof (KeptOverrideAttribute))
+				.Select (ca => (ca.ConstructorArguments[0].Value as TypeDefinition).FullName));
+			var originalBaseTypesOverridden = new HashSet<string> (original.Overrides.Select (ov => ov.DeclaringType.FullName));
+			var linkedBaseTypesOverridden = new HashSet<string> (linked.Overrides.Select (ov => ov.DeclaringType.FullName));
+			foreach (var expectedBaseType in expectedBaseTypesOverridden) {
+				Assert.IsTrue (originalBaseTypesOverridden.Contains (expectedBaseType),
+					$"Method {linked.FullName} was expected to keep override {expectedBaseType}::{linked.Name}, " +
+					 "but it wasn't in the unlinked assembly");
+				Assert.IsTrue (linkedBaseTypesOverridden.Contains (expectedBaseType),
+					$"Method {linked.FullName} was expected to override {expectedBaseType}::{linked.Name}");
+			}
+
+			var expectedBaseTypesNotOverridden = new HashSet<string> (original.CustomAttributes
+				.Where (ca => ca.AttributeType.Name == nameof (RemovedOverrideAttribute))
+				.Select (ca => (ca.ConstructorArguments[0].Value as TypeDefinition).FullName));
+			foreach (var expectedRemovedBaseType in expectedBaseTypesNotOverridden) {
+				Assert.IsTrue (originalBaseTypesOverridden.Contains (expectedRemovedBaseType),
+					$"Method {linked.FullName} was expected to remove override {expectedRemovedBaseType}::{linked.Name}, " +
+					$"but it wasn't in the unlinked assembly");
+				Assert.IsFalse (linkedBaseTypesOverridden.Contains (expectedRemovedBaseType),
+					$"Method {linked.FullName} was expected to not override {expectedRemovedBaseType}::{linked.Name}");
+			}
+		}
+
 		static string FormatBaseOrInterfaceAttributeValue (CustomAttribute attr)
 		{
 			if (attr.ConstructorArguments.Count == 1)
@@ -373,6 +403,7 @@ namespace Mono.Linker.Tests.TestCasesRunner
 			VerifySecurityAttributes (src, linked);
 			VerifyArrayInitializers (src, linked);
 			VerifyMethodBody (src, linked);
+			VerifyOverrides (src, linked);
 		}
 
 		protected virtual void VerifyMethodBody (MethodDefinition src, MethodDefinition linked)
@@ -555,10 +586,10 @@ namespace Mono.Linker.Tests.TestCasesRunner
 			/*
 			 - The test case will always need to have at least 1 reference.
 			 - Forcing all tests to define their expected references seems tedious
-			 
+
 			 Given the above, let's assume that when no [KeptReference] attributes are present,
 			 the test case does not want to make any assertions regarding references.
-			 
+
 			 Once 1 kept reference attribute is used, the test will need to define all of of it's expected references
 			*/
 			if (expected.Length == 0)