Re-add static interface trimming with more testing

docs/removal-behavior.md

@@ -42,7 +42,7 @@ public class Program
 
 ### 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.
+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 that is rooted.
 
 Example:
 

src/linker/Linker.Steps/MarkStep.cs

@@ -574,6 +574,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
@@ -3043,8 +3047,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), ScopeStack.CurrentScope.Origin);
 					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,32 @@ protected virtual void SweepMethods (Collection<MethodDefinition> methods)
 			}
 		}
 
+		void SweepOverrides (MethodDefinition method)
+		{
+			for (int i = 0; i < method.Overrides.Count;) {
+				// We can't rely on the context resolution cache anymore, since it may remember methods which are already removed
+				// So call the direct Resolve here and avoid the cache.
+				// It can still happen that the Resolve will return a removed method (this happens if the Overrides collection stores actual
+				// MethodDefinition and not MethodReference, in which case the Resolve is just "return this" and doesn't check the existence of the method
+				// in any way). In such case the Declaring type of the method is null. And since it has been removed the override pointing
+				// to it should be removed as well.
+				// Resolve() may also return null if the method has been removed, in which case the override should be removed from the list
+				if (method.Overrides[i].Resolve () is not MethodDefinition ov || ShouldRemove (ov) && (ov.DeclaringType == null || !IgnoreScope (ov.DeclaringType.Scope)))
+					method.Overrides.RemoveAt (i);
+				else
+					i++;
+			}
+		}
+
+		/// <summary>
+		/// Returns true if the assembly of the <paramref name="scope"></paramref> is not set to link (i.e. action=copy is set for that assembly)
+		/// </summary>
+		private bool IgnoreScope (IMetadataScope scope)
+		{
+			AssemblyDefinition? assembly = Context.Resolve (scope);
+			return assembly != null && Annotations.GetAction (assembly) != AssemblyAction.Link;
+		}
+
 		void SweepDebugInfo (Collection<MethodDefinition> methods)
 		{
 			List<ScopeDebugInformation>? sweptScopes = null;

test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.Interfaces.StaticInterfaceMethodsTests.g.cs

@@ -0,0 +1,25 @@
+using System;
+using System.Threading.Tasks;
+using Xunit;
+
+namespace ILLink.RoslynAnalyzer.Tests.Inheritance.Interfaces
+{
+	public sealed partial class StaticInterfaceMethodsTests : LinkerTestBase
+	{
+
+		protected override string TestSuiteName => "Inheritance.Interfaces.StaticInterfaceMethods";
+
+		[Fact]
+		public Task StaticAbstractInterfaceMethods ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
+		[Fact]
+		public Task StaticAbstractInterfaceMethodsLibrary ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
+	}
+}

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

@@ -71,41 +71,32 @@ internal class ImplementsUnusedStaticInterface
 			// The interface methods themselves are not used, but the implementation of these methods is
 			internal interface IStaticInterfaceMethodUnused
 			{
-				// Can be removed with Static Interface trimming optimization
-				[Kept]
 				static abstract void InterfaceUsedMethodNot ();
 			}
 
-			// Can be removed with Static Interface Trimming
-			[Kept]
 			internal interface IStaticInterfaceUnused
 			{
-				// Can be removed with Static Interface Trimming
-				[Kept]
 				static abstract void InterfaceAndMethodNoUsed ();
 			}
 
 			[Kept]
-			[KeptInterface (typeof (IStaticInterfaceUnused))]
-			[KeptInterface (typeof (IStaticInterfaceMethodUnused))]
 			internal class InterfaceMethodUsedThroughImplementation : IStaticInterfaceMethodUnused, IStaticInterfaceUnused
 			{
 				[Kept]
+				[RemovedOverride (typeof (IStaticInterfaceMethodUnused))]
 				public static void InterfaceUsedMethodNot () { }
 
 				[Kept]
+				[RemovedOverride (typeof (IStaticInterfaceUnused))]
 				public static void InterfaceAndMethodNoUsed () { }
 			}
 
 			[Kept]
-			[KeptInterface (typeof (IStaticInterfaceMethodUnused))]
-			[KeptInterface (typeof (IStaticInterfaceUnused))]
+			[KeptInterface (typeof (IStaticInterfaceMethodUnused))] // Bug
 			internal class InterfaceMethodUnused : IStaticInterfaceMethodUnused, IStaticInterfaceUnused
 			{
-				[Kept]
 				public static void InterfaceUsedMethodNot () { }
 
-				[Kept]
 				public static void InterfaceAndMethodNoUsed () { }
 			}