From e816e7336f1737248d73d405c49f568fe1db745b Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Wed, 30 Nov 2022 12:25:09 -0800 Subject: [PATCH] Address PR comments from PR #3094 (#3144) * Address PR comments from PR #3094 --- src/linker/Linker.Steps/MarkStep.cs | 19 ++++++++++++------- ...rovidesInterfaceMethodCircularReference.cs | 5 +++-- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 735bf7641e3e..bcb252d4bce4 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -710,11 +710,12 @@ void ProcessVirtualMethod (MethodDefinition method) /// /// Returns true if the Override in 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. /// // 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 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); } diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/BaseProvidesInterfaceMethodCircularReference.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/BaseProvidesInterfaceMethodCircularReference.cs index 0750274d0891..fd37de182df6 100644 --- a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/BaseProvidesInterfaceMethodCircularReference.cs +++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/BaseProvidesInterfaceMethodCircularReference.cs @@ -17,8 +17,10 @@ namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces.BaseProvidesInterfaceEd /// 's method as a base to 's method, which modifies the collection as it's being iterated, causing an exception. /// [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; } } }