ILLink: Sweep .override for interface method if the .interfaceImpl is removed

src/tools/illink/src/linker/Linker.Steps/MarkStep.cs

@@ -35,9 +35,7 @@
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
-using System.Reflection.Metadata.Ecma335;
 using System.Reflection.Runtime.TypeParsing;
-using System.Runtime.CompilerServices;
 using System.Text.RegularExpressions;
 using ILCompiler.DependencyAnalysisFramework;
 using ILLink.Shared;
@@ -718,33 +716,25 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method, MessageOrigin orig
 		/// or if any marked interface implementations on <paramref name="type"/> are interfaces that implement <paramref name="interfaceType"/> and that interface implementation is marked
 		/// </summary>
 		bool IsInterfaceImplementationMarkedRecursively (TypeDefinition type, TypeDefinition interfaceType)
+			=> IsInterfaceImplementationMarkedRecursively (type, interfaceType, Context);
+
+		/// <summary>
+		/// Returns true if <paramref name="type"/> implements <paramref name="interfaceType"/> and the interface implementation is marked,
+		/// or if any marked interface implementations on <paramref name="type"/> are interfaces that implement <paramref name="interfaceType"/> and that interface implementation is marked
+		/// </summary>
+		internal static bool IsInterfaceImplementationMarkedRecursively (TypeDefinition type, TypeDefinition interfaceType, LinkContext context)
 		{
 			if (type.HasInterfaces) {
 				foreach (var intf in type.Interfaces) {
-					TypeDefinition? resolvedInterface = Context.Resolve (intf.InterfaceType);
+					TypeDefinition? resolvedInterface = context.Resolve (intf.InterfaceType);
 					if (resolvedInterface == null)
 						continue;
-
-					if (Annotations.IsMarked (intf) && RequiresInterfaceRecursively (resolvedInterface, interfaceType))
-						return true;
-				}
-			}
-
-			return false;
-		}
-
-		bool RequiresInterfaceRecursively (TypeDefinition typeToExamine, TypeDefinition interfaceType)
-		{
-			if (typeToExamine == interfaceType)
-				return true;
-
-			if (typeToExamine.HasInterfaces) {
-				foreach (var iface in typeToExamine.Interfaces) {
-					var resolved = Context.TryResolve (iface.InterfaceType);
-					if (resolved == null)
+					if (!context.Annotations.IsMarked (intf))
 						continue;
 
-					if (RequiresInterfaceRecursively (resolved, interfaceType))
+					if (resolvedInterface == interfaceType)
+						return true;
+					if (IsInterfaceImplementationMarkedRecursively (resolvedInterface, interfaceType, context))
 						return true;
 				}
 			}
@@ -3197,8 +3187,12 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
 						Context.Resolve (@base) is MethodDefinition baseDefinition
 						&& baseDefinition.DeclaringType.IsInterface && baseDefinition.IsStatic && method.IsStatic)
 						continue;
+					// Instance methods can have overrides on public implementation methods in IL, but C# will usually only have them for private explicit interface implementations.
+					// It is valid IL for a public method to override an interface method and only be called directly. In this case it would be safe to skip marking the .override method.
+					// However, in most cases, the C# compiler will only generate .override for instance methods when it's a private explicit interface implementations which can only be called through the interface.
+					// We can just take a short cut and mark all the overrides on instance methods. We shouldn't miss out on size savings for code generated by Roslyn.
 					MarkMethod (@base, new DependencyInfo (DependencyKind.MethodImplOverride, method), methodOrigin);
-					MarkExplicitInterfaceImplementation (method, @base);
+					MarkRuntimeInterfaceImplementation (method, @base);
 				}
 			}
 
@@ -3314,22 +3308,21 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type
 			DoAdditionalInstantiatedTypeProcessing (type);
 		}
 
-		void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov)
+		void MarkRuntimeInterfaceImplementation (MethodDefinition method, MethodReference ov)
 		{
 			if (Context.Resolve (ov) is not MethodDefinition resolvedOverride)
 				return;
+			if (!resolvedOverride.DeclaringType.IsInterface)
+				return;
+			var interfaceToBeImplemented = ov.DeclaringType;
 
-			if (resolvedOverride.DeclaringType.IsInterface) {
-				foreach (var ifaceImpl in method.DeclaringType.Interfaces) {
-					var resolvedInterfaceType = Context.Resolve (ifaceImpl.InterfaceType);
-					if (resolvedInterfaceType == null) {
-						continue;
-					}
-
-					if (resolvedInterfaceType == resolvedOverride.DeclaringType) {
-						MarkInterfaceImplementation (ifaceImpl, new MessageOrigin (method.DeclaringType));
-						return;
-					}
+			var ifaces = Annotations.GetRecursiveInterfaces (method.DeclaringType);
+			if (ifaces is null)
+				return;
+			foreach (var iface in ifaces) {
+				if (TypeReferenceEqualityComparer.AreEqual (iface.InterfaceType, interfaceToBeImplemented, Context)) {
+					MarkInterfaceImplementationList (iface.ImplementationChain, new MessageOrigin (method.DeclaringType));
+					return;
 				}
 			}
 		}

src/tools/illink/src/linker/Linker.Steps/SweepStep.cs

@@ -456,14 +456,19 @@ void SweepOverrides (MethodDefinition method)
 				//    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 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
+				//  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.
+				// OR
+				//  ov is an interface method and the interface is not implemented by the type
 #pragma warning disable RS0030 // Cecil's Resolve is banned - it's necessary when the metadata graph isn't stable
-				if (method.Overrides[i].Resolve () is not MethodDefinition ov || ov.DeclaringType is null || (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov)))
+				if (method.Overrides[i].Resolve () is not MethodDefinition ov
+					|| ov.DeclaringType is null
+					|| (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov))
+					|| (ov.DeclaringType.IsInterface && !MarkStep.IsInterfaceImplementationMarkedRecursively (method.DeclaringType, ov.DeclaringType, Context)))
 					method.Overrides.RemoveAt (i);
 				else
 					i++;

src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/StaticInterfaceMethods.cs

@@ -16,6 +16,12 @@ public Task BaseProvidesInterfaceMethod ()
 			return RunTest (allowMissingWarnings: false);
 		}
 
+		[Fact]
+		public Task RemovedInterfaceImplementationRemovedOverride()
+		{
+			return RunTest (allowMissingWarnings: false);
+		}
+
 		[Fact]
 		public Task StaticAbstractInterfaceMethods ()
 		{

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/KeptOverrideAttribute.cs

@@ -15,12 +15,12 @@ namespace Mono.Linker.Tests.Cases.Expectations.Assertions
 	[AttributeUsage (AttributeTargets.Method, AllowMultiple = true, Inherited = false)]
 	public class KeptOverrideAttribute : KeptAttribute
 	{
-		public Type TypeWithOverriddenMethodDeclaration;
-
 		public KeptOverrideAttribute (Type typeWithOverriddenMethod)
 		{
 			ArgumentNullException.ThrowIfNull (typeWithOverriddenMethod);
-			TypeWithOverriddenMethodDeclaration = typeWithOverriddenMethod;
+		}
+		public KeptOverrideAttribute (string typeWithOverriddenMethod)
+		{
 		}
 	}
-}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/KeptOverrideOnMethodInAssemblyAttribute.cs

@@ -0,0 +1,15 @@
+// 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
+{
+	[AttributeUsage (AttributeTargets.All, AllowMultiple = true)]
+	public class KeptOverrideOnMethodInAssemblyAttribute : BaseInAssemblyAttribute
+	{
+		public KeptOverrideOnMethodInAssemblyAttribute (string assemblyName, string typeName, string methodName, string overriddenMethodName)
+		{
+		}
+	}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/RemovedOverrideAttribute.cs

@@ -14,12 +14,14 @@ namespace Mono.Linker.Tests.Cases.Expectations.Assertions
 	[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;
+		}
+
+		public RemovedOverrideAttribute (string nameOfOverriddenMethod)
+		{
 		}
 	}
-}
+}

src/tools/illink/test/Mono.Linker.Tests.Cases.Expectations/Assertions/RemovedOverrideOnMethodInAssemblyAttribute.cs

@@ -0,0 +1,15 @@
+// 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
+{
+	[AttributeUsage (AttributeTargets.All, AllowMultiple = true, Inherited = false)]
+	public class RemovedOverrideOnMethodInAssemblyAttribute : BaseInAssemblyAttribute
+	{
+		public RemovedOverrideOnMethodInAssemblyAttribute (string library, string typeName, string methodName, string overriddenMethodFullName)
+		{
+		}
+	}
+}