Address PR comments from PR dotnet#3094 (dotnet#3144)

* Address PR comments from PR dotnet#3094

Commit migrated from dotnet@e816e73

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

@@ -710,11 +710,12 @@ void ProcessVirtualMethod (MethodDefinition method)
 		/// <summary>
 		/// Returns true if the Override in <paramref name="overrideInformation"/> should be marked because it is needed by the base method.
 		/// Does not take into account if the base method is in a preserved scope.
-		/// Assumes the base method is marked.
+		/// Assumes the base method is marked or comes from a preserved scope.
 		/// </summary>
 		// TODO: Move interface method marking logic here https://github.com/dotnet/linker/issues/3090
 		bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation)
 		{
+			Debug.Assert (Annotations.IsMarked (overrideInformation.Base) || IgnoreScope (overrideInformation.Base.DeclaringType.Scope));
 			if (!Annotations.IsMarked (overrideInformation.Override.DeclaringType))
 				return false;
 			if (overrideInformation.IsOverrideOfInterfaceMember) {
@@ -725,8 +726,9 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation)
 			if (!Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override))
 				return true;
 
-			// Methods on instantiated types that override a ov.Override from a base type (not an interface) should be marked
-			// Interface ov.Overrides should only be marked if the interfaceImplementation is marked, which is handled below
+			// In this context, an override needs to be kept if
+			// a) it's an override on an instantiated type (of a marked base) or
+			// b) it's an override of an abstract base (required for valid IL)
 			if (Annotations.IsInstantiated (overrideInformation.Override.DeclaringType))
 				return true;
 
@@ -744,6 +746,7 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation)
 		// TODO: Take into account a base method in preserved scope
 		void MarkOverrideForBaseMethod (OverrideInformation overrideInformation)
 		{
+			Debug.Assert (ShouldMarkOverrideForBase (overrideInformation));
 			if (Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override) && Annotations.IsInstantiated (overrideInformation.Override.DeclaringType)) {
 				MarkMethod (overrideInformation.Override, new DependencyInfo (DependencyKind.OverrideOnInstantiatedType, overrideInformation.Override.DeclaringType), ScopeStack.CurrentScope.Origin);
 			} else {
@@ -2034,12 +2037,14 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in
 				_typesWithInterfaces.Add ((type, ScopeStack.CurrentScope));
 
 			if (type.HasMethods) {
-				// TODO: MarkMethodIfNeededByBaseMethod should include logic for IsMethodNeededBytTypeDueToPreservedScope
+				// TODO: MarkMethodIfNeededByBaseMethod should include logic for IsMethodNeededByTypeDueToPreservedScope: https://github.com/dotnet/linker/issues/3090
 				foreach (var method in type.Methods) {
 					MarkMethodIfNeededByBaseMethod (method);
+					if (IsMethodNeededByTypeDueToPreservedScope (method)) {
+						// For methods that must be preserved, blame the declaring type.
+						MarkMethod (method, new DependencyInfo (DependencyKind.VirtualNeededDueToPreservedScope, type), ScopeStack.CurrentScope.Origin);
+					}
 				}
-				// For methods that must be preserved, blame the declaring type.
-				MarkMethodsIf (type.Methods, IsMethodNeededByTypeDueToPreservedScope, new DependencyInfo (DependencyKind.VirtualNeededDueToPreservedScope, type), ScopeStack.CurrentScope.Origin);
 				if (ShouldMarkTypeStaticConstructor (type) && reason.Kind != DependencyKind.TriggersCctorForCalledMethod) {
 					using (ScopeStack.PopToParent ())
 						MarkStaticConstructor (type, new DependencyInfo (DependencyKind.CctorForType, type), ScopeStack.CurrentScope.Origin);
@@ -3201,7 +3206,7 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
 			MarkBaseMethods (method);
 
 			if (Annotations.GetOverrides (method) is IEnumerable<OverrideInformation> overrides) {
-				foreach (var @override in overrides) {
+				foreach (var @override in overrides.Where (ov => Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope))) {
 					if (ShouldMarkOverrideForBase (@override))
 						MarkOverrideForBaseMethod (@override);
 				}

src/tools/illink/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/BaseProvidesInterfaceMethodCircularReference.cs

@@ -17,8 +17,10 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces.BaseProvidesInterfaceEd
 	/// <see cref="IBar"/>'s method as a base to <see cref="Base"/>'s method, which modifies the collection as it's being iterated, causing an exception.
 	/// </summary>
 	[SetupCompileBefore ("base.dll", new[] { "Dependencies/Base.cs" })] // Base Implements IFoo.Method (psuedo-reference to ifoo.dll)
-	[SetupCompileBefore ("ifoo.dll", new[] { "Dependencies/IFoo.cs" }, references: new[] { "base.dll" })] // Derived2 references base.dll (circular reference)
+	[SetupCompileBefore ("ifoo.dll", new[] { "Dependencies/IFoo.cs" }, references: new[] { "base.dll" })] // Derived2 references Base from base.dll (circular reference)
 	[SetupCompileBefore ("derived1.dll", new[] { "Dependencies/Derived1.cs" }, references: new[] { "ifoo.dll", "base.dll" })]
+	[KeptMemberInAssembly ("base.dll", typeof (Base), "Method()")]
+	[RemovedMemberInAssembly ("ifoo", "Derived2")]
 	public class BaseProvidesInterfaceMethodCircularReference
 	{
 		[Kept]
@@ -32,7 +34,6 @@ public static void Main ()
 		public static void Foo ()
 		{
 			((IFoo) null).Method ();
-			object x = null;
 		}
 	}
 }