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

This reverts commit ff646e0.

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

src/linker/Linker.Steps/MarkStep.cs

@@ -60,7 +60,6 @@ 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;
@@ -225,7 +224,6 @@ 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)> ();
@@ -478,7 +476,6 @@ bool ProcessPrimaryQueue ()
 			while (!QueueIsEmpty ()) {
 				ProcessQueue ();
 				ProcessVirtualMethods ();
-				ProcessStaticInterfaceMethods ();
 				ProcessMarkedTypesWithInterfaces ();
 				ProcessDynamicCastableImplementationInterfaces ();
 				ProcessPendingBodies ();
@@ -579,30 +576,6 @@ 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
@@ -720,9 +693,6 @@ 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;
@@ -738,12 +708,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 or the method is a static interface method
+			// We don't need to mark overrides until it is possible that the type could be instantiated
 			// Note : The base type is interface check should be removed once we have base type sweeping
 			if (IsInterfaceOverrideThatDoesNotNeedMarked (overrideInformation, isInstantiated))
 				return;
 
-			// Interface static virtual methods will be abstract and will also bypass this check to get marked
+			// Interface static veitual methods will be abstract and will also by pass this check to get marked
 			if (!isInstantiated && !@base.IsAbstract && Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, method))
 				return;
 
@@ -766,7 +736,8 @@ bool IsInterfaceOverrideThatDoesNotNeedMarked (OverrideInformation overrideInfor
 			if (!overrideInformation.IsOverrideOfInterfaceMember || isInstantiated)
 				return false;
 
-			if (overrideInformation.IsStaticInterfaceMethodPair)
+			// 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)
 				return false;
 
 			if (overrideInformation.MatchingInterfaceImplementation != null)
@@ -3083,28 +3054,17 @@ 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 @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);
+				foreach (MethodReference ov in method.Overrides) {
+					MarkMethod (ov, new DependencyInfo (DependencyKind.MethodImplOverride, method), ScopeStack.CurrentScope.Origin);
+					MarkExplicitInterfaceImplementation (method, ov);
 				}
 			}
 
 			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);
@@ -3187,9 +3147,9 @@ protected virtual IEnumerable<MethodDefinition> GetRequiredMethodsForInstantiate
 			}
 		}
 
-		void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference overriddenMethod)
+		void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov)
 		{
-			if (Context.Resolve (overriddenMethod) is not MethodDefinition resolvedOverride)
+			if (Context.Resolve (ov) is not MethodDefinition resolvedOverride)
 				return;
 
 			if (resolvedOverride.DeclaringType.IsInterface) {
@@ -3461,7 +3421,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. (ex. `Type1 x = null; IFoo y = (IFoo)x;`)
+			// even if the type is never instantiated
 			var implementations = new InterfacesOnStackScanner (Context).GetReferencedInterfaces (body);
 			if (implementations == null)
 				return;

src/linker/Linker.Steps/SweepStep.cs

@@ -453,8 +453,6 @@ protected virtual void SweepMethods (Collection<MethodDefinition> methods)
 
 				SweepCustomAttributes (method.MethodReturnType);
 
-				SweepOverrides (method);
-
 				if (!method.HasParameters)
 					continue;
 
@@ -469,38 +467,6 @@ 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,9 +436,6 @@ 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,22 +10,17 @@ namespace Mono.Linker
 	public class OverrideInformation
 	{
 		readonly ITryResolveMetadata resolver;
-		readonly OverridePair _pair;
 
 		public OverrideInformation (MethodDefinition @base, MethodDefinition @override, ITryResolveMetadata resolver, InterfaceImplementation? matchingInterfaceImplementation = null)
 		{
-			_pair = new OverridePair (@base, @override);
+			Base = @base;
+			Override = @override;
 			MatchingInterfaceImplementation = matchingInterfaceImplementation;
 			this.resolver = resolver;
 		}
 
-		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 MethodDefinition Base { get; }
+		public MethodDefinition Override { get; }
 		public InterfaceImplementation? MatchingInterfaceImplementation { get; }
 
 		public bool IsOverrideOfInterfaceMember {
@@ -48,7 +43,5 @@ public TypeDefinition? InterfaceType {
 				return Base.DeclaringType;
 			}
 		}
-
-		public bool IsStaticInterfaceMethodPair => _pair.IsStaticInterfaceMethodPair ();
 	}
 }

test/ILLink.RoslynAnalyzer.Tests/Inheritance.Interfaces.StaticInterfaceMethodsTests.cs

@@ -71,51 +71,53 @@ 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 ();
 
-				// 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);
+				Type t;
+				t = typeof (IStaticInterfaceMethodUnused);
+				t = typeof (InterfaceMethodUnused);
 			}
 		}