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

… removed (#102857)

When static interface methods are kept but an implementation of the method is only accessed via a direct call on the implementing type, the implementation method and the .override pointing to the interface method are both kept, but not the .interfaceImpl. This causes a TypeLoadException when the .override points to a method on an interface that the type doesn't implement. To fix this, we needed to update the condition for sweeping overrides to include the case where both the interface and the interface method are kept, but the .interfaceImpl is not kept.

Tests are for static interface methods, but concrete and generic to test type resolution in SweepStep, which can be finicky. Instance methods shouldn't hit this issue since public interface methods don't have a .override and private methods can't be marked without instantiating the type or reflecting over the type, both of which mark the type's .interfaceImpls, and making the .override valid.

For instance methods, we will always mark any methods referenced in a .override as well as their corresponding interface implementation, so we won't end up in a scenario where an instance method would have dangling references in a .override. I did fix up the interface implementation marking (MarkRuntimeInterfaceImplementation) to make sure it can find a recursive interface, and marks the .interfaceImpl with the exact same TypeReference rather than just the same TypeDefinition (for example, IGeneric<int> should be marked, not just the first IGeneric<> interfaceImpl). This was one of the examples where the trimmer would end up doing the right thing (all IGeneric<> implementations would end up marked anyway), but it would do it for a different reason than we expect.

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)
+		{
+		}
+	}
+}