Re-add static interface trimming with more testing (#2791)

Enables more precise removal of static abstract interface methods and adds more tests than previously.

Co-authored-by: Sven Boemer <[email protected]>

Author: jtschuster

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 kept, as is every implementation method on every kept type.
 
 Example:
 

src/linker/Linker.Steps/MarkStep.cs

@@ -60,6 +60,7 @@ protected LinkContext Context {
 
 		protected Queue<(MethodDefinition, DependencyInfo, MessageOrigin)> _methods;
 		protected List<(MethodDefinition, MarkScopeStack.Scope)> _virtual_methods;
+		protected List<(MethodDefinition, MarkScopeStack.Scope)> _static_interface_methods;
 		protected Queue<AttributeProviderPair> _assemblyLevelAttributes;
 		readonly List<AttributeProviderPair> _ivt_attributes;
 		protected Queue<(AttributeProviderPair, DependencyInfo, MarkScopeStack.Scope)> _lateMarkedAttributes;
@@ -224,6 +225,7 @@ public MarkStep ()
 		{
 			_methods = new Queue<(MethodDefinition, DependencyInfo, MessageOrigin)> ();
 			_virtual_methods = new List<(MethodDefinition, MarkScopeStack.Scope)> ();
+			_static_interface_methods = new List<(MethodDefinition, MarkScopeStack.Scope)> ();
 			_assemblyLevelAttributes = new Queue<AttributeProviderPair> ();
 			_ivt_attributes = new List<AttributeProviderPair> ();
 			_lateMarkedAttributes = new Queue<(AttributeProviderPair, DependencyInfo, MarkScopeStack.Scope)> ();
@@ -476,6 +478,7 @@ bool ProcessPrimaryQueue ()
 			while (!QueueIsEmpty ()) {
 				ProcessQueue ();
 				ProcessVirtualMethods ();
+				ProcessStaticInterfaceMethods ();
 				ProcessMarkedTypesWithInterfaces ();
 				ProcessDynamicCastableImplementationInterfaces ();
 				ProcessPendingBodies ();
@@ -576,6 +579,30 @@ void ProcessVirtualMethods ()
 			}
 		}
 
+		/// <summary>
+		/// Handles marking implementations of static interface methods and the interface implementations of types that implement a static interface method.
+		/// </summary>
+		void ProcessStaticInterfaceMethods ()
+		{
+			foreach ((MethodDefinition method, MarkScopeStack.Scope scope) in _static_interface_methods) {
+				using (ScopeStack.PushScope (scope)) {
+					var overrides = Annotations.GetOverrides (method);
+					if (overrides != null) {
+						foreach (OverrideInformation @override in overrides) {
+							ProcessOverride (@override);
+							// We need to mark the interface implementation for static interface methods
+							// Explicit interface method implementations already mark the interface implementation in ProcessMethod
+							MarkExplicitInterfaceImplementation (@override.Override, @override.Base);
+						}
+					}
+				}
+			}
+		}
+
+		/// <summary>
+		/// Does extra handling of marked types that have interfaces when it's necessary to know what types are marked or instantiated.
+		/// Right now it only 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
@@ -693,6 +720,9 @@ void ProcessVirtualMethod (MethodDefinition method)
 			}
 		}
 
+		/// <summary>
+		/// Handles marking overriding methods if the type with the overriding method is instantiated or if the base method is a static abstract interface method
+		/// </summary>
 		void ProcessOverride (OverrideInformation overrideInformation)
 		{
 			var method = overrideInformation.Override;
@@ -708,12 +738,12 @@ void ProcessOverride (OverrideInformation overrideInformation)
 
 			var isInstantiated = Annotations.IsInstantiated (method.DeclaringType);
 
-			// We don't need to mark overrides until it is possible that the type could be instantiated
+			// We don't need to mark overrides until it is possible that the type could be instantiated or the method is a static interface method
 			// Note : The base type is interface check should be removed once we have base type sweeping
 			if (IsInterfaceOverrideThatDoesNotNeedMarked (overrideInformation, isInstantiated))
 				return;
 
-			// Interface static veitual methods will be abstract and will also by pass this check to get marked
+			// Interface static virtual methods will be abstract and will also bypass this check to get marked
 			if (!isInstantiated && !@base.IsAbstract && Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, method))
 				return;
 
@@ -736,8 +766,7 @@ bool IsInterfaceOverrideThatDoesNotNeedMarked (OverrideInformation overrideInfor
 			if (!overrideInformation.IsOverrideOfInterfaceMember || isInstantiated)
 				return false;
 
-			// This is a static interface method and these checks should all be true
-			if (overrideInformation.Override.IsStatic && overrideInformation.Base.IsStatic && overrideInformation.Base.IsAbstract && !overrideInformation.Override.IsVirtual)
+			if (overrideInformation.IsStaticInterfaceMethodPair)
 				return false;
 
 			if (overrideInformation.MatchingInterfaceImplementation != null)
@@ -3054,17 +3083,28 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
 				}
 			}
 
+			// Mark overridden methods and interface implementations except for static interface methods
+			// This will not mark implicit interface methods because they do not have a MethodImpl and aren't in the .Overrides
 			if (method.HasOverrides) {
-				foreach (MethodReference ov in method.Overrides) {
-					MarkMethod (ov, new DependencyInfo (DependencyKind.MethodImplOverride, method), ScopeStack.CurrentScope.Origin);
-					MarkExplicitInterfaceImplementation (method, ov);
+				foreach (MethodReference @base 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 (@base) is MethodDefinition baseDefinition
+						&& new OverrideInformation.OverridePair (baseDefinition, method).IsStaticInterfaceMethodPair ())
+						continue;
+					MarkMethod (@base, new DependencyInfo (DependencyKind.MethodImplOverride, method), ScopeStack.CurrentScope.Origin);
+					MarkExplicitInterfaceImplementation (method, @base);
 				}
 			}
 
 			MarkMethodSpecialCustomAttributes (method);
 			if (method.IsVirtual)
 				_virtual_methods.Add ((method, ScopeStack.CurrentScope));
 
+			if (method.IsStatic && method.IsAbstract && method.DeclaringType.IsInterface)
+				_static_interface_methods.Add ((method, ScopeStack.CurrentScope));
+
 			MarkNewCodeDependencies (method);
 
 			MarkBaseMethods (method);
@@ -3147,9 +3187,9 @@ protected virtual IEnumerable<MethodDefinition> GetRequiredMethodsForInstantiate
 			}
 		}
 
-		void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov)
+		void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference overriddenMethod)
 		{
-			if (Context.Resolve (ov) is not MethodDefinition resolvedOverride)
+			if (Context.Resolve (overriddenMethod) is not MethodDefinition resolvedOverride)
 				return;
 
 			if (resolvedOverride.DeclaringType.IsInterface) {
@@ -3421,7 +3461,7 @@ void MarkInterfacesNeededByBodyStack (MethodBody body)
 		{
 			// If a type could be on the stack in the body and an interface it implements could be on the stack on the body
 			// then we need to mark that interface implementation.  When this occurs it is not safe to remove the interface implementation from the type
-			// even if the type is never instantiated
+			// even if the type is never instantiated. (ex. `Type1 x = null; IFoo y = (IFoo)x;`)
 			var implementations = new InterfacesOnStackScanner (Context).GetReferencedInterfaces (body);
 			if (implementations == null)
 				return;

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,38 @@ 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.
+				// We want to remove a method from the list of Overrides if:
+				//	Resolve() is null
+				//		This can happen for a couple of reasons, but it indicates the method isn't in the final assembly.
+				//		Resolve also may return a removed value if method.Overrides[i] is a MethodDefinition. In this case, Resolve short circuits and returns `this`.
+				//	OR
+				//	ov.DeclaringType is null
+				//		ov.DeclaringType may be null if Resolve short circuited and returned a removed method. In this case, we want to remove the override.
+				//	OR
+				//	ov is in a `link` scope and is unmarked
+				//		ShouldRemove returns true if the method is unmarked, but we also We need to make sure the override is in a link scope.
+				//		Only things in a link scope are marked, so ShouldRemove is only valid for items in a `link` scope.
+				if (method.Overrides[i].Resolve () is not MethodDefinition ov || ov.DeclaringType is null || (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov)))
+					method.Overrides.RemoveAt (i);
+				else
+					i++;
+			}
+		}
+
+		/// <summary>
+		/// Returns true if the assembly of the <paramref name="scope"></paramref> is set to link
+		/// </summary>
+		private bool IsLinkScope (IMetadataScope scope)
+		{
+			AssemblyDefinition? assembly = Context.Resolve (scope);
+			return assembly != null && Annotations.GetAction (assembly) == AssemblyAction.Link;
+		}
+
 		void SweepDebugInfo (Collection<MethodDefinition> methods)
 		{
 			List<ScopeDebugInformation>? sweptScopes = null;

src/linker/Linker/Annotations.cs

@@ -436,6 +436,9 @@ public bool IsPublic (IMetadataTokenProvider provider)
 			return public_api.Contains (provider);
 		}
 
+		/// <summary>
+		/// Returns an IEnumerable of the methods that override this method. Note this is different than <see cref="MethodDefinition.Overrides"/>, which returns the MethodImpl's
+		/// </summary>
 		public IEnumerable<OverrideInformation>? GetOverrides (MethodDefinition method)
 		{
 			return TypeMapInfo.GetOverrides (method);

src/linker/Linker/OverrideInformation.cs

@@ -10,17 +10,22 @@ namespace Mono.Linker
 	public class OverrideInformation
 	{
 		readonly ITryResolveMetadata resolver;
+		readonly OverridePair _pair;
 
 		public OverrideInformation (MethodDefinition @base, MethodDefinition @override, ITryResolveMetadata resolver, InterfaceImplementation? matchingInterfaceImplementation = null)
 		{
-			Base = @base;
-			Override = @override;
+			_pair = new OverridePair (@base, @override);
 			MatchingInterfaceImplementation = matchingInterfaceImplementation;
 			this.resolver = resolver;
 		}
 
-		public MethodDefinition Base { get; }
-		public MethodDefinition Override { get; }
+		public readonly record struct OverridePair (MethodDefinition Base, MethodDefinition Override)
+		{
+			public bool IsStaticInterfaceMethodPair () => Base.DeclaringType.IsInterface && Base.IsStatic && Override.IsStatic;
+		}
+
+		public MethodDefinition Base { get => _pair.Base; }
+		public MethodDefinition Override { get => _pair.Override; }
 		public InterfaceImplementation? MatchingInterfaceImplementation { get; }
 
 		public bool IsOverrideOfInterfaceMember {
@@ -43,5 +48,7 @@ public TypeDefinition? InterfaceType {
 				return Base.DeclaringType;
 			}
 		}
+
+		public bool IsStaticInterfaceMethodPair => _pair.IsStaticInterfaceMethodPair ();
 	}
 }

test/ILLink.RoslynAnalyzer.Tests/Inheritance.Interfaces.StaticInterfaceMethodsTests.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.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 (nameof (StaticAbstractInterfaceMethods));
+		}
+
+		[Fact]
+		public Task StaticAbstractInterfaceMethodsLibrary ()
+		{
+			return RunTest (nameof (StaticAbstractInterfaceMethodsLibrary));
+		}
+	}
+}

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

@@ -71,53 +71,51 @@ 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))]
 			internal class InterfaceMethodUnused : IStaticInterfaceMethodUnused, IStaticInterfaceUnused
 			{
-				[Kept]
 				public static void InterfaceUsedMethodNot () { }
 
-				[Kept]
 				public static void InterfaceAndMethodNoUsed () { }
 			}
 
+			[Kept]
+			// This method keeps InterfaceMethodUnused without making it 'relevant to variant casting' like
+			//	doing a typeof or type argument would do. If the type is relevant to variant casting,
+			//	we will keep all interface implementations for interfaces that are kept
+			internal static void KeepInterfaceMethodUnused (InterfaceMethodUnused x) { }
+
 			[Kept]
 			public static void Test ()
 			{
 				InterfaceMethodUsedThroughImplementation.InterfaceUsedMethodNot ();
 				InterfaceMethodUsedThroughImplementation.InterfaceAndMethodNoUsed ();
 
-				Type t;
-				t = typeof (IStaticInterfaceMethodUnused);
-				t = typeof (InterfaceMethodUnused);
+				// The interface has to be kept this way, because if both the type and the interface may
+				//	appear on the stack then they would be marked as relevant to variant casting and the
+				//	interface implementation would be kept.
+				Type t = typeof (IStaticInterfaceMethodUnused);
+				KeepInterfaceMethodUnused (null);
 			}
 		}