Don't remove unused interfaces in library mode (#2711)

This will make sure that we keep all interfaces and all interface method implementations on such type.

* Change the solution to rely on the optimization setting only

* wip

* Add implicit interface implementation case

* Edit tests to remove issue-specific code and names

* Mark members of CollectedType as kept

* Add private interface test and simplify external interface example

* Replace early exit for non-interfaces

* License headers and use IsMethodNeededByTypeDueToPreservedScope instead of Interface specific version

* Add check for static methods before skipping virtual marking

Check for optimization before skipping marking interface methods for
PreservedScope

Add doc comments

* Add more clarifying comments, move static method check, rename method

Rename IsVirtualNeededByInstantiatedTypeDueToPreservedScope to IsOverrideNeededByInstantiatedTypeDueToPreservedScope
Added more comments describing purpose of methods
Moved the static interface method check to the method that is called on all types regardless of instantiation

* Renames and comment cleanup + tests

This doesn't change product behavior, only renamed methods and improved comments.

Added new tests for the RootLibrary:
- Added a dependency to an "copy" assembly (mainly because I can define a static interface in it)
- Added more combinations to the interfaces/classes in the test
- Since this uses static interface methods I had to enable "preview" language features for the test project and for the test infra.

* More tests for interface behavior

Co-authored-by: vitek-karas <vitek.karas@microsoft.com>
Co-authored-by: vitek-karas <10670590+vitek-karas@users.noreply.github.com>

Author: jtschuster

Directory.Build.props

@@ -29,7 +29,7 @@
   </PropertyGroup>
 
   <PropertyGroup>
-    <LangVersion>latest</LangVersion>
+    <LangVersion>preview</LangVersion>
     <AnalysisLevel>latest</AnalysisLevel>
   </PropertyGroup>
 </Project>

src/linker/Linker.Steps/MarkStep.cs

@@ -584,11 +584,15 @@ void ProcessMarkedTypesWithInterfaces ()
 			foreach ((var type, var scope) in typesWithInterfaces) {
 				// Exception, types that have not been flagged as instantiated yet.  These types may not need their interfaces even if the
 				// interface type is marked
-				if (!Annotations.IsInstantiated (type) && !Annotations.IsRelevantToVariantCasting (type))
+				// UnusedInterfaces optimization is turned off mark all interface implementations
+				bool unusedInterfacesOptimizationEnabled = Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, type);
+				if (!Annotations.IsInstantiated (type) && !Annotations.IsRelevantToVariantCasting (type) &&
+					unusedInterfacesOptimizationEnabled)
 					continue;
 
-				using (ScopeStack.PushScope (scope))
+				using (ScopeStack.PushScope (scope)) {
 					MarkInterfaceImplementations (type);
+				}
 			}
 		}
 
@@ -1719,6 +1723,9 @@ void MarkField (FieldDefinition field, in DependencyInfo reason)
 			}
 		}
 
+		/// <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>
 		protected virtual bool IgnoreScope (IMetadataScope scope)
 		{
 			AssemblyDefinition? assembly = Context.Resolve (scope);
@@ -1915,7 +1922,7 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in
 			MarkGenericParameterProvider (type);
 
 			// There are a number of markings we can defer until later when we know it's possible a reference type could be instantiated
-			// For example, if no instance of a type exist, then we don't need to mark the interfaces on that type
+			// For example, if no instance of a type exist, then we don't need to mark the interfaces on that type -- Note this is not true for static interfaces
 			// However, for some other types there is no benefit to deferring
 			if (type.IsInterface) {
 				// There's no benefit to deferring processing of an interface type until we know a type implementing that interface is marked
@@ -1939,12 +1946,13 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in
 				MarkRequirementsForInstantiatedTypes (type);
 			}
 
+			// Save for later once we know which interfaces are marked and then determine which interface implementations and methods to keep
 			if (type.HasInterfaces)
 				_typesWithInterfaces.Add ((type, ScopeStack.CurrentScope));
 
 			if (type.HasMethods) {
-				// For virtuals that must be preserved, blame the declaring type.
-				MarkMethodsIf (type.Methods, IsVirtualNeededByTypeDueToPreservedScope, new DependencyInfo (DependencyKind.VirtualNeededDueToPreservedScope, type));
+				// For methods that must be preserved, blame the declaring type.
+				MarkMethodsIf (type.Methods, IsMethodNeededByTypeDueToPreservedScope, new DependencyInfo (DependencyKind.VirtualNeededDueToPreservedScope, type));
 				if (ShouldMarkTypeStaticConstructor (type) && reason.Kind != DependencyKind.TriggersCctorForCalledMethod) {
 					using (ScopeStack.PopToParent ())
 						MarkStaticConstructor (type, new DependencyInfo (DependencyKind.CctorForType, type));
@@ -2278,9 +2286,19 @@ void MarkGenericParameter (GenericParameter parameter)
 			}
 		}
 
-		bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method)
-		{
-			if (!method.IsVirtual)
+		/// <summary>
+		/// Returns true if any of the base methods of the <paramref name="method"/> passed is in an assembly that is not trimmed (i.e. action != trim).
+		/// Meant to be used to determine whether methods should be marked regardless of whether it is instantiated or not. 
+		/// </summary>
+		/// <remarks>
+		/// When the unusedinterfaces optimization is on, this is used to mark methods that override an abstract method from a non-link assembly and must be kept.
+		/// When the unusedinterfaces optimization is off, this will do the same as when on but will also mark interface methods from interfaces defined in a non-link assembly.
+		/// If the containing type is instantiated, the caller should also use <see cref="IsMethodNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition)" />
+		/// </remarks>
+		bool IsMethodNeededByTypeDueToPreservedScope (MethodDefinition method)
+		{
+			// Static methods may also have base methods in static interface methods. These methods are not captured by IsVirtual and must be checked separately
+			if (!(method.IsVirtual || method.IsStatic))
 				return false;
 
 			var base_list = Annotations.GetBaseMethods (method);
@@ -2289,8 +2307,8 @@ bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method)
 
 			foreach (MethodDefinition @base in base_list) {
 				// Just because the type is marked does not mean we need interface methods.
-				// if the type is never instantiated, interfaces will be removed
-				if (@base.DeclaringType.IsInterface)
+				// if the type is never instantiated, interfaces will be removed - but only if the optimization is enabled
+				if (@base.DeclaringType.IsInterface && Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, method.DeclaringType))
 					continue;
 
 				// If the type is marked, we need to keep overrides of abstract members defined in assemblies
@@ -2302,15 +2320,24 @@ bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method)
 				if (IgnoreScope (@base.DeclaringType.Scope))
 					return true;
 
-				if (IsVirtualNeededByTypeDueToPreservedScope (@base))
+				if (IsMethodNeededByTypeDueToPreservedScope (@base))
 					return true;
 			}
 
 			return false;
 		}
 
-		bool IsVirtualNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition method)
+		/// <summary>
+		/// Returns true if any of the base methods of <paramref name="method" /> is defined in an assembly that is not trimmed (i.e. action!=trim).
+		/// This is meant to be used on methods from a type that is known to be instantiated.
+		/// </summary>
+		/// <remarks>
+		/// This is very similar to <see cref="IsMethodNeededByTypeDueToPreservedScope (MethodDefinition)"/>,
+		///	but will mark methods from an interface defined in a non-link assembly regardless of the optimization, and does not handle static interface methods.
+		/// </remarks>
+		bool IsMethodNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition method)
 		{
+			// Any static interface methods are captured by <see cref="IsVirtualNeededByTypeDueToPreservedScope">, which should be called on all relevant methods so no need to check again here.
 			if (!method.IsVirtual)
 				return false;
 
@@ -2322,7 +2349,7 @@ bool IsVirtualNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition meth
 				if (IgnoreScope (@base.DeclaringType.Scope))
 					return true;
 
-				if (IsVirtualNeededByTypeDueToPreservedScope (@base))
+				if (IsMethodNeededByTypeDueToPreservedScope (@base))
 					return true;
 			}
 
@@ -3110,7 +3137,7 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type
 		protected virtual IEnumerable<MethodDefinition> GetRequiredMethodsForInstantiatedType (TypeDefinition type)
 		{
 			foreach (var method in type.Methods) {
-				if (IsVirtualNeededByInstantiatedTypeDueToPreservedScope (method))
+				if (IsMethodNeededByInstantiatedTypeDueToPreservedScope (method))
 					yield return method;
 			}
 		}

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

@@ -21,5 +21,11 @@ public Task InterfaceOnUninstantiatedTypeRemoved ()
 			return RunTest (allowMissingWarnings: true);
 		}
 
+		[Fact]
+		public Task InterfaceVariants ()
+		{
+			return RunTest (allowMissingWarnings: true);
+		}
+
 	}
 }

test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/Dependencies/CopyLibrary.cs

@@ -0,0 +1,17 @@
+// 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.
+
+namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces.Dependencies
+{
+	public interface ICopyLibraryInterface
+	{
+		void CopyLibraryInterfaceMethod ();
+		void CopyLibraryExplicitImplementationInterfaceMethod ();
+	}
+
+	public interface ICopyLibraryStaticInterface
+	{
+		static abstract void CopyLibraryStaticInterfaceMethod ();
+		static abstract void CopyLibraryExplicitImplementationStaticInterfaceMethod ();
+	}
+}

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

@@ -3,7 +3,6 @@
 
 namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
 {
-	[SetupLinkerArgument ("--disable-opt", "unusedinterfaces")]
 	public class InterfaceOnUninstantiatedTypeRemoved
 	{
 		public static void Main ()