From b99fa7e1cb485c7e161dff455410bfd8829d92ec Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Thu, 27 Oct 2022 15:19:52 -0700 Subject: [PATCH 1/5] Check for marking virtual method due to base only when state changes (#3073) Instead of checking every virtual method to see if it should be kept due to a base method every iteration of the MarkStep pipeline, check each method only when its relevant state has changed. Co-authored-by: Sven Boemer --- src/linker/Linker.Steps/MarkStep.cs | 218 ++++++++---------- src/linker/Linker.Steps/SealerStep.cs | 11 +- .../ValidateVirtualMethodAnnotationsStep.cs | 10 +- src/linker/Linker/Annotations.cs | 12 +- src/linker/Linker/TypeMapInfo.cs | 26 ++- ...ceCastableImplementationAttributeIsKept.cs | 4 + .../InterfaceVariants.cs | 23 +- .../UnusedInterfacesInPreservedScope.cs | 7 +- 8 files changed, 160 insertions(+), 151 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 7dbede11d0fd..55cb6e3a6b02 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -607,12 +607,12 @@ void ProcessMarkedTypesWithInterfaces () } // OverrideInformation for interfaces in PreservedScope aren't added yet foreach (var method in type.Methods) { - var bases = Annotations.GetBaseMethods (method); - if (bases is null) + var baseOverrideInformations = Annotations.GetBaseMethods (method); + if (baseOverrideInformations is null) continue; - foreach (var @base in bases) { - if (@base.DeclaringType.IsInterface && IgnoreScope (@base.DeclaringType.Scope)) - _interfaceOverrides.Add ((new OverrideInformation (@base, method, Context), ScopeStack.CurrentScope)); + foreach (var ov in baseOverrideInformations) { + if (ov.Base.DeclaringType is not null && ov.Base.DeclaringType.IsInterface && IgnoreScope (ov.Base.DeclaringType.Scope)) + _interfaceOverrides.Add ((ov, ScopeStack.CurrentScope)); } } } @@ -707,12 +707,6 @@ void ProcessVirtualMethod (MethodDefinition method) { Annotations.EnqueueVirtualMethod (method); - var overrides = Annotations.GetOverrides (method); - if (overrides != null) { - foreach (OverrideInformation @override in overrides) - ProcessOverride (@override); - } - var defaultImplementations = Annotations.GetDefaultInterfaceImplementations (method); if (defaultImplementations != null) { foreach (var defaultImplementationInfo in defaultImplementations) { @@ -721,43 +715,63 @@ void ProcessVirtualMethod (MethodDefinition method) } } - void ProcessOverride (OverrideInformation overrideInformation) + /// + /// 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. + /// + // TODO: Move interface method marking logic here https://github.com/dotnet/linker/issues/3090 + bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation) { - var method = overrideInformation.Override; - var @base = overrideInformation.Base; - if (!Annotations.IsMarked (method.DeclaringType)) - return; - - if (Annotations.IsProcessed (method)) - return; - - if (Annotations.IsMarked (method)) - return; - - var isInstantiated = Annotations.IsInstantiated (method.DeclaringType); - - // Handle interface methods once we know more about whether the type is instantiated or relevant to variant casting if (overrideInformation.IsOverrideOfInterfaceMember) { _interfaceOverrides.Add ((overrideInformation, ScopeStack.CurrentScope)); - return; + return false; } - // Interface static veitual methods will be abstract and will also by pass this check to get marked - if (!isInstantiated && !@base.IsAbstract && Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, method)) - return; + 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 + if (Annotations.IsInstantiated (overrideInformation.Override.DeclaringType)) + return true; + + // Direct overrides of marked abstract ov.Overrides must be marked or we get invalid IL. + // Overrides further in the hierarchy will override the direct override (which will be implemented by the above rule), so we don't need to worry about invalid IL. + if (overrideInformation.Base.IsAbstract) + return true; + + return false; + } - // Only track instantiations if override removal is enabled and the type is instantiated. - // If it's disabled, all overrides are kept, so there's no instantiation site to blame. - if (Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, method) && isInstantiated) { - MarkMethod (method, new DependencyInfo (DependencyKind.OverrideOnInstantiatedType, method.DeclaringType), ScopeStack.CurrentScope.Origin); + /// + /// Marks the Override of with the correct reason. Should be called when returns true. + /// + // TODO: Take into account a base method in preserved scope + void MarkOverrideForBaseMethod (OverrideInformation 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 { // If the optimization is disabled or it's an abstract type, we just mark it as a normal override. - Debug.Assert (!Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, method) || @base.IsAbstract); - MarkMethod (method, new DependencyInfo (DependencyKind.Override, @base), ScopeStack.CurrentScope.Origin); + Debug.Assert (!Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override) || overrideInformation.Base.IsAbstract); + MarkMethod (overrideInformation.Override, new DependencyInfo (DependencyKind.Override, overrideInformation.Base), ScopeStack.CurrentScope.Origin); } + } - if (method.IsVirtual) - ProcessVirtualMethod (method); + void MarkMethodIfNeededByBaseMethod (MethodDefinition method) + { + Debug.Assert (Annotations.IsMarked (method.DeclaringType)); + + var bases = Annotations.GetBaseMethods (method); + if (bases is null) + return; + + var markedBaseMethods = bases.Where (ov => Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)); + foreach (var ov in markedBaseMethods) { + if (ShouldMarkOverrideForBase (ov)) + MarkOverrideForBaseMethod (ov); + } } /// @@ -2027,6 +2041,10 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in _typesWithInterfaces.Add ((type, ScopeStack.CurrentScope)); if (type.HasMethods) { + // TODO: MarkMethodIfNeededByBaseMethod should include logic for IsMethodNeededBytTypeDueToPreservedScope + foreach (var method in type.Methods) { + MarkMethodIfNeededByBaseMethod (method); + } // 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) { @@ -2334,6 +2352,28 @@ void MarkInterfaceImplementations (TypeDefinition type) if (ShouldMarkInterfaceImplementation (type, iface)) MarkInterfaceImplementation (iface, new MessageOrigin (type)); } + + bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementation iface) + { + if (Annotations.IsMarked (iface)) + return false; + + if (!Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, type)) + return true; + + if (Context.Resolve (iface.InterfaceType) is not TypeDefinition resolvedInterfaceType) + return false; + + if (Annotations.IsMarked (resolvedInterfaceType)) + return true; + + // It's hard to know if a com or windows runtime interface will be needed from managed code alone, + // so as a precaution we will mark these interfaces once the type is instantiated + if (resolvedInterfaceType.IsImport || resolvedInterfaceType.IsWindowsRuntime) + return true; + + return IsFullyPreserved (type); + } } void MarkGenericParameterProvider (IGenericParameterProvider provider) @@ -2378,19 +2418,19 @@ bool IsMethodNeededByTypeDueToPreservedScope (MethodDefinition method) if (base_list == null) return false; - foreach (MethodDefinition @base in base_list) { + foreach (OverrideInformation ov in base_list) { // Skip interface methods, they will be captured later by IsInterfaceImplementationMethodNeededByTypeDueToInterface - if (@base.DeclaringType.IsInterface) + if (ov.Base.DeclaringType.IsInterface) continue; - if (!IgnoreScope (@base.DeclaringType.Scope) && !IsMethodNeededByTypeDueToPreservedScope (@base)) + if (!IgnoreScope (ov.Base.DeclaringType.Scope) && !IsMethodNeededByTypeDueToPreservedScope (ov.Base)) continue; // If the type is marked, we need to keep overrides of abstract members defined in assemblies // that are copied to keep the IL valid. // However, if the base method is a non-abstract virtual (has an implementation on the base type), then we don't need to keep the override // until the type could be instantiated - if (!@base.IsAbstract) + if (!ov.Base.IsAbstract) continue; return true; @@ -2440,35 +2480,6 @@ bool IsInterfaceImplementationMethodNeededByTypeDueToInterface (OverrideInformat return Annotations.IsInstantiated (method.DeclaringType); } - /// - /// Returns true if any of the base methods of 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. - /// - /// - /// This is very similar to , - /// but will mark methods from an interface defined in a non-link assembly regardless of the optimization, and does not handle static interface methods. - /// - bool IsMethodNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition method) - { - // Any static interface methods are captured by , which should be called on all relevant methods so no need to check again here. - if (!method.IsVirtual) - return false; - - var base_list = Annotations.GetBaseMethods (method); - if (base_list == null) - return false; - - foreach (MethodDefinition @base in base_list) { - if (IgnoreScope (@base.DeclaringType.Scope)) - return true; - - if (IsMethodNeededByTypeDueToPreservedScope (@base)) - return true; - } - - return false; - } - static bool IsSpecialSerializationConstructor (MethodDefinition method) { if (!method.IsInstanceConstructor ()) @@ -3177,6 +3188,13 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo MarkBaseMethods (method); + if (Annotations.GetOverrides (method) is IEnumerable overrides) { + foreach (var @override in overrides) { + if (ShouldMarkOverrideForBase (@override)) + MarkOverrideForBaseMethod (@override); + } + } + MarkType (method.ReturnType, new DependencyInfo (DependencyKind.ReturnType, method)); MarkCustomAttributes (method.MethodReturnType, new DependencyInfo (DependencyKind.ReturnTypeAttribute, method)); MarkMarshalSpec (method.MethodReturnType, new DependencyInfo (DependencyKind.ReturnTypeMarshalSpec, method)); @@ -3232,29 +3250,16 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type MarkInterfaceImplementations (type); - foreach (var method in GetRequiredMethodsForInstantiatedType (type)) - MarkMethod (method, new DependencyInfo (DependencyKind.MethodForInstantiatedType, type), ScopeStack.CurrentScope.Origin); + // Requires interface implementations to be marked first + foreach (var method in type.Methods) { + MarkMethodIfNeededByBaseMethod (method); + } MarkImplicitlyUsedFields (type); DoAdditionalInstantiatedTypeProcessing (type); } - /// - /// Collect methods that must be marked once a type is determined to be instantiated. - /// - /// This method is virtual in order to give derived mark steps an opportunity to modify the collection of methods that are needed - /// - /// - /// - protected virtual IEnumerable GetRequiredMethodsForInstantiatedType (TypeDefinition type) - { - foreach (var method in type.Methods) { - if (IsMethodNeededByInstantiatedTypeDueToPreservedScope (method)) - yield return method; - } - } - void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov) { if (Context.Resolve (ov) is not MethodDefinition resolvedOverride) @@ -3348,18 +3353,18 @@ void MarkBaseMethods (MethodDefinition method) if (base_methods == null) return; - foreach (MethodDefinition base_method in base_methods) { + foreach (OverrideInformation ov in base_methods) { // We should add all interface base methods to _virtual_methods for virtual override annotation validation // Interfaces from preserved scope will be missed if we don't add them here // This will produce warnings for all interface methods and virtual methods regardless of whether the interface, interface implementation, or interface method is kept or not. - if (base_method.DeclaringType.IsInterface && !method.DeclaringType.IsInterface) { + if (ov.Base.DeclaringType.IsInterface && !method.DeclaringType.IsInterface) { // These are all virtual, no need to check IsVirtual before adding to list - _virtual_methods.Add ((base_method, ScopeStack.CurrentScope)); + _virtual_methods.Add ((ov.Base, ScopeStack.CurrentScope)); continue; } - MarkMethod (base_method, new DependencyInfo (DependencyKind.BaseMethod, method), ScopeStack.CurrentScope.Origin); - MarkBaseMethods (base_method); + MarkMethod (ov.Base, new DependencyInfo (DependencyKind.BaseMethod, method), ScopeStack.CurrentScope.Origin); + MarkBaseMethods (ov.Base); } } @@ -3654,8 +3659,9 @@ protected virtual void MarkInstruction (Instruction instruction, MethodDefinitio var operand = (TypeReference) instruction.Operand; switch (instruction.OpCode.Code) { case Code.Newarr: - if (Context.TryResolve (operand) is TypeDefinition typeDefinition) + if (Context.TryResolve (operand) is TypeDefinition typeDefinition) { Annotations.MarkRelevantToVariantCasting (typeDefinition); + } break; case Code.Isinst: if (operand is TypeSpecification || operand is GenericParameter) @@ -3685,33 +3691,12 @@ protected virtual void MarkInstruction (Instruction instruction, MethodDefinitio } } - protected virtual bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementation iface) - { - if (Annotations.IsMarked (iface)) - return false; - - if (!Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, type)) - return true; - - if (Context.Resolve (iface.InterfaceType) is not TypeDefinition resolvedInterfaceType) - return false; - - if (Annotations.IsMarked (resolvedInterfaceType)) - return true; - - // It's hard to know if a com or windows runtime interface will be needed from managed code alone, - // so as a precaution we will mark these interfaces once the type is instantiated - if (resolvedInterfaceType.IsImport || resolvedInterfaceType.IsWindowsRuntime) - return true; - - - return IsFullyPreserved (type); - } protected internal virtual void MarkInterfaceImplementation (InterfaceImplementation iface, MessageOrigin? origin = null, DependencyInfo? reason = null) { if (Annotations.IsMarked (iface)) return; + Annotations.MarkProcessed (iface, reason ?? new DependencyInfo (DependencyKind.InterfaceImplementationOnType, ScopeStack.CurrentScope.Origin.Provider)); using var localScope = origin.HasValue ? ScopeStack.PushScope (origin.Value) : null; @@ -3719,7 +3704,6 @@ protected internal virtual void MarkInterfaceImplementation (InterfaceImplementa MarkCustomAttributes (iface, new DependencyInfo (DependencyKind.CustomAttribute, iface)); // Blame the interface type on the interfaceimpl itself. MarkType (iface.InterfaceType, reason ?? new DependencyInfo (DependencyKind.InterfaceImplementationInterfaceType, iface)); - Annotations.MarkProcessed (iface, reason ?? new DependencyInfo (DependencyKind.InterfaceImplementationOnType, ScopeStack.CurrentScope.Origin.Provider)); } // diff --git a/src/linker/Linker.Steps/SealerStep.cs b/src/linker/Linker.Steps/SealerStep.cs index 52d6957f239f..dbc64f0f1669 100644 --- a/src/linker/Linker.Steps/SealerStep.cs +++ b/src/linker/Linker.Steps/SealerStep.cs @@ -96,7 +96,7 @@ void ProcessType (TypeDefinition type) // // cannot de-virtualize nor seal methods if something overrides them // - if (IsAnyMarked (overrides)) + if (IsAnyOverrideMarked (overrides)) continue; SealMethod (method); @@ -108,7 +108,7 @@ void ProcessType (TypeDefinition type) var bases = Annotations.GetBaseMethods (method); // Devirtualize if a method is not override to existing marked methods - if (!IsAnyMarked (bases)) + if (!IsAnyBaseMarked (bases)) method.IsVirtual = method.IsFinal = method.IsNewSlot = false; } } @@ -123,7 +123,7 @@ protected virtual void SealMethod (MethodDefinition method) method.IsFinal = true; } - bool IsAnyMarked (IEnumerable? list) + bool IsAnyOverrideMarked (IEnumerable? list) { if (list == null) return false; @@ -135,12 +135,13 @@ bool IsAnyMarked (IEnumerable? list) return false; } - bool IsAnyMarked (List? list) + bool IsAnyBaseMarked (IEnumerable? list) { if (list == null) return false; + foreach (var m in list) { - if (Annotations.IsMarked (m)) + if (Annotations.IsMarked (m.Base)) return true; } return false; diff --git a/src/linker/Linker.Steps/ValidateVirtualMethodAnnotationsStep.cs b/src/linker/Linker.Steps/ValidateVirtualMethodAnnotationsStep.cs index 7470186a2226..a13876880118 100644 --- a/src/linker/Linker.Steps/ValidateVirtualMethodAnnotationsStep.cs +++ b/src/linker/Linker.Steps/ValidateVirtualMethodAnnotationsStep.cs @@ -13,11 +13,11 @@ protected override void Process () { var annotations = Context.Annotations; foreach (var method in annotations.VirtualMethodsWithAnnotationsToValidate) { - var baseMethods = annotations.GetBaseMethods (method); - if (baseMethods != null) { - foreach (var baseMethod in baseMethods) { - annotations.FlowAnnotations.ValidateMethodAnnotationsAreSame (method, baseMethod); - ValidateMethodRequiresUnreferencedCodeAreSame (method, baseMethod); + var baseOverrideInformations = annotations.GetBaseMethods (method); + if (baseOverrideInformations != null) { + foreach (var baseOv in baseOverrideInformations) { + annotations.FlowAnnotations.ValidateMethodAnnotationsAreSame (method, baseOv.Base); + ValidateMethodRequiresUnreferencedCodeAreSame (method, baseOv.Base); } } diff --git a/src/linker/Linker/Annotations.cs b/src/linker/Linker/Annotations.cs index 227468101cc6..4729ec773de8 100644 --- a/src/linker/Linker/Annotations.cs +++ b/src/linker/Linker/Annotations.cs @@ -436,6 +436,9 @@ public bool IsPublic (IMetadataTokenProvider provider) return public_api.Contains (provider); } + /// + /// Returns a list of all known methods that override . The list may be incomplete if other overrides exist in assemblies that haven't been processed by TypeMapInfo yet + /// public IEnumerable? GetOverrides (MethodDefinition method) { return TypeMapInfo.GetOverrides (method); @@ -446,7 +449,14 @@ public bool IsPublic (IMetadataTokenProvider provider) return TypeMapInfo.GetDefaultInterfaceImplementations (method); } - public List? GetBaseMethods (MethodDefinition method) + /// + /// Returns all base methods that overrides. + /// This includes methods on 's declaring type's base type (but not methods higher up in the type hierarchy), + /// methods on an interface that 's delcaring type implements, + /// and methods an interface implemented by a derived type of 's declaring type if the derived type uses as the implementing method. + /// The list may be incomplete if there are derived types in assemblies that havent been processed yet that use to implement an interface. + /// + public List? GetBaseMethods (MethodDefinition method) { return TypeMapInfo.GetBaseMethods (method); } diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index 117df8fad74b..f4b81ce7b76c 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -39,7 +39,7 @@ public class TypeMapInfo { readonly HashSet assemblies = new HashSet (); readonly LinkContext context; - protected readonly Dictionary> base_methods = new Dictionary> (); + protected readonly Dictionary> base_methods = new Dictionary> (); protected readonly Dictionary> override_methods = new Dictionary> (); protected readonly Dictionary> default_interface_implementations = new Dictionary> (); @@ -57,6 +57,9 @@ void EnsureProcessed (AssemblyDefinition assembly) MapType (type); } + /// + /// Returns a list of all known methods that override . The list may be incomplete if other overrides exist in assemblies that haven't been processed by TypeMapInfo yet + /// public IEnumerable? GetOverrides (MethodDefinition method) { EnsureProcessed (method.Module.Assembly); @@ -64,10 +67,17 @@ void EnsureProcessed (AssemblyDefinition assembly) return overrides; } - public List? GetBaseMethods (MethodDefinition method) + /// + /// Returns all base methods that overrides. + /// This includes the closest overridden virtual method on 's base types + /// methods on an interface that 's declaring type implements, + /// and methods an interface implemented by a derived type of 's declaring type if the derived type uses as the implementing method. + /// The list may be incomplete if there are derived types in assemblies that havent been processed yet that use to implement an interface. + /// + public List? GetBaseMethods (MethodDefinition method) { EnsureProcessed (method.Module.Assembly); - base_methods.TryGetValue (method, out List? bases); + base_methods.TryGetValue (method, out List? bases); return bases; } @@ -77,14 +87,14 @@ void EnsureProcessed (AssemblyDefinition assembly) return ret; } - public void AddBaseMethod (MethodDefinition method, MethodDefinition @base) + public void AddBaseMethod (MethodDefinition method, MethodDefinition @base, InterfaceImplementation? matchingInterfaceImplementation) { - if (!base_methods.TryGetValue (method, out List? methods)) { - methods = new List (); + if (!base_methods.TryGetValue (method, out List? methods)) { + methods = new List (); base_methods[method] = methods; } - methods.Add (@base); + methods.Add (new OverrideInformation (@base, method, context, matchingInterfaceImplementation)); } public void AddOverride (MethodDefinition @base, MethodDefinition @override, InterfaceImplementation? matchingInterfaceImplementation = null) @@ -204,7 +214,7 @@ void MapOverrides (MethodDefinition method) void AnnotateMethods (MethodDefinition @base, MethodDefinition @override, InterfaceImplementation? matchingInterfaceImplementation = null) { - AddBaseMethod (@override, @base); + AddBaseMethod (@override, @base, matchingInterfaceImplementation); AddOverride (@base, @override, matchingInterfaceImplementation); } diff --git a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs index 7065930acb96..c6dd27cb5755 100644 --- a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs +++ b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs @@ -16,6 +16,8 @@ namespace Mono.Linker.Tests.Cases.Attributes [KeptMemberInAssembly ("impl", "Mono.Linker.Tests.Cases.Attributes.Dependencies.IReferencedAssemblyImpl", "Foo()")] [KeptInterfaceOnTypeInAssembly ("impl", "Mono.Linker.Tests.Cases.Attributes.Dependencies.IReferencedAssemblyImpl", "interface", "Mono.Linker.Tests.Cases.Attributes.Dependencies.IReferencedAssembly")] + [SetupLinkerTrimMode ("link")] + [IgnoreDescriptors (false)] public class TypeWithDynamicInterfaceCastableImplementationAttributeIsKept { public static void Main () @@ -54,6 +56,7 @@ static IReferencedAssembly GetReferencedInterface (object obj) #if NETCOREAPP [Kept] [KeptMember (".ctor()")] + [KeptInterface (typeof (IDynamicInterfaceCastable))] class Foo : IDynamicInterfaceCastable { [Kept] @@ -74,6 +77,7 @@ public bool IsInterfaceImplemented (RuntimeTypeHandle interfaceType, bool throwI [Kept] [KeptMember (".ctor()")] + [KeptInterface (typeof (IDynamicInterfaceCastable))] class DynamicCastableImplementedInOtherAssembly : IDynamicInterfaceCastable { [Kept] diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs index 347c3a6a0768..b19208bd563d 100644 --- a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs +++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs @@ -21,15 +21,20 @@ public static void Main () t = typeof (UninstantiatedPublicClassWithPrivateInterface); t = typeof (ImplementsUsedStaticInterface.InterfaceMethodUnused); - ImplementsUnusedStaticInterface.Test (); ; + ImplementsUnusedStaticInterface.Test (); GenericMethodThatCallsInternalStaticInterfaceMethod (); // Use all public interfaces - they're marked as public only to denote them as "used" typeof (IPublicInterface).RequiresPublicMethods (); typeof (IPublicStaticInterface).RequiresPublicMethods (); - var ___ = new InstantiatedClassWithInterfaces (); + _ = new InstantiatedClassWithInterfaces (); + MarkIFormattable (null); } + [Kept] + static void MarkIFormattable (IFormattable x) + { } + [Kept] internal static void GenericMethodThatCallsInternalStaticInterfaceMethod () where T : IStaticInterfaceUsed { @@ -113,8 +118,8 @@ public static void Test () } } + // Interfaces are kept despite being uninstantiated because it is relevant to variant casting [Kept] - [KeptInterface (typeof (IEnumerator))] [KeptInterface (typeof (IPublicInterface))] [KeptInterface (typeof (IPublicStaticInterface))] [KeptInterface (typeof (ICopyLibraryInterface))] @@ -151,18 +156,12 @@ public static void InternalStaticInterfaceMethod () { } static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { } - [Kept] - [ExpectBodyModified] bool IEnumerator.MoveNext () { throw new PlatformNotSupportedException (); } - [Kept] object IEnumerator.Current { - [Kept] - [ExpectBodyModified] get { throw new PlatformNotSupportedException (); } } - [Kept] void IEnumerator.Reset () { } [Kept] @@ -198,7 +197,6 @@ public string ToString (string format, IFormatProvider formatProvider) } [Kept] - [KeptInterface (typeof (IEnumerator))] [KeptInterface (typeof (IPublicInterface))] [KeptInterface (typeof (IPublicStaticInterface))] [KeptInterface (typeof (ICopyLibraryInterface))] @@ -235,13 +233,10 @@ public static void InternalStaticInterfaceMethod () { } static void IInternalStaticInterface.ExplicitImplementationInternalStaticInterfaceMethod () { } - [Kept] bool IEnumerator.MoveNext () { throw new PlatformNotSupportedException (); } - [Kept] - object IEnumerator.Current { [Kept] get { throw new PlatformNotSupportedException (); } } + object IEnumerator.Current { get { throw new PlatformNotSupportedException (); } } - [Kept] void IEnumerator.Reset () { } [Kept] diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/UnusedInterfacesInPreservedScope.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/UnusedInterfacesInPreservedScope.cs index a03a28bd24c7..35ae2dda0521 100644 --- a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/UnusedInterfacesInPreservedScope.cs +++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/StaticInterfaceMethods/UnusedInterfacesInPreservedScope.cs @@ -37,10 +37,15 @@ class MyType : IStaticInterfaceWithDefaultImpls public int InstanceMethod () => 0; } + // Keep MyType without marking it relevant to variant casting + [Kept] + static void KeepMyType (MyType x) + { } + [Kept] static void Test () { - var x = typeof (MyType); // The only use of MyType + KeepMyType (null); } } } From eb56d660f694d5db9fad112974f44f0219a49425 Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Wed, 2 Nov 2022 10:37:46 -0500 Subject: [PATCH 2/5] Don't mark override of abstract base if the override's declaring type is not marked (#3098) * Don't mark an override every time the base is abstract, only if the declaring type is also marked Adds a condition to ShouldMarkOverrideForBase to exit early if the declaring type of the method is not marked. --- src/linker/Linker.Steps/MarkStep.cs | 2 + .../Inheritance.VirtualMethodsTests.g.cs | 6 ++ ...rrideOfAbstractInUnmarkedClassIsRemoved.cs | 97 +++++++++++++++++++ 3 files changed, 105 insertions(+) create mode 100644 test/Mono.Linker.Tests.Cases/Inheritance.VirtualMethods/OverrideOfAbstractInUnmarkedClassIsRemoved.cs diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 55cb6e3a6b02..4dd6307c1bae 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -723,6 +723,8 @@ void ProcessVirtualMethod (MethodDefinition method) // TODO: Move interface method marking logic here https://github.com/dotnet/linker/issues/3090 bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation) { + if (!Annotations.IsMarked (overrideInformation.Override.DeclaringType)) + return false; if (overrideInformation.IsOverrideOfInterfaceMember) { _interfaceOverrides.Add ((overrideInformation, ScopeStack.CurrentScope)); return false; diff --git a/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.VirtualMethodsTests.g.cs b/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.VirtualMethodsTests.g.cs index a7cadcdb33e0..5ddfcf4f3b67 100644 --- a/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.VirtualMethodsTests.g.cs +++ b/test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/Inheritance.VirtualMethodsTests.g.cs @@ -27,6 +27,12 @@ public Task NeverInstantiatedTypeWithBaseInCopiedAssembly () return RunTest (allowMissingWarnings: true); } + [Fact] + public Task OverrideInUnmarkedClassIsRemoved () + { + return RunTest (allowMissingWarnings: true); + } + [Fact] public Task UnusedTypeWithOverrideOfVirtualMethodIsRemoved () { diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.VirtualMethods/OverrideOfAbstractInUnmarkedClassIsRemoved.cs b/test/Mono.Linker.Tests.Cases/Inheritance.VirtualMethods/OverrideOfAbstractInUnmarkedClassIsRemoved.cs new file mode 100644 index 000000000000..366fd6a3f34f --- /dev/null +++ b/test/Mono.Linker.Tests.Cases/Inheritance.VirtualMethods/OverrideOfAbstractInUnmarkedClassIsRemoved.cs @@ -0,0 +1,97 @@ +// 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; +using System.Collections.Generic; +using System.Linq; +using System.Text; +using System.Threading.Tasks; +using Mono.Linker.Tests.Cases.Expectations.Assertions; +using Mono.Linker.Tests.Cases.Expectations.Metadata; + +namespace Mono.Linker.Tests.Cases.Inheritance.VirtualMethods +{ + public class OverrideOfAbstractInUnmarkedClassIsRemoved + { + [Kept] + public static void Main () + { + MarkedBase x = new MarkedDerived (); + x.Method (); + + UsedSecondLevelTypeWithAbstractBase y = new (); + y.Method (); + + UsedSecondLevelType z = new (); + z.Method (); + } + + [Kept] + [KeptMember (".ctor()")] + abstract class MarkedBase + { + [Kept] + public abstract int Method (); + } + + [Kept] + [KeptMember (".ctor()")] + [KeptBaseType (typeof (MarkedBase))] + class MarkedDerived : MarkedBase + { + [Kept] + public override int Method () => 1; + } + + class UnmarkedDerived : MarkedBase + { + public override int Method () => 1; + } + + [Kept] + [KeptMember (".ctor()")] + [KeptBaseType (typeof (MarkedBase))] + class UnusedIntermediateType : MarkedBase + { + [Kept] + public override int Method () => 1; + } + + [Kept] + [KeptMember (".ctor()")] + [KeptBaseType (typeof (UnusedIntermediateType))] + class UsedSecondLevelType : UnusedIntermediateType + { + [Kept] + public override int Method () => 1; + } + + [Kept] + [KeptMember (".ctor()")] + [KeptBaseType (typeof (MarkedBase))] + abstract class UnusedIntermediateTypeWithAbstractOverride : MarkedBase + { + [Kept] + public abstract override int Method (); + } + + [Kept] + [KeptMember (".ctor()")] + [KeptBaseType (typeof (UnusedIntermediateTypeWithAbstractOverride))] + class UsedSecondLevelTypeWithAbstractBase : UnusedIntermediateTypeWithAbstractOverride + { + [Kept] + public override int Method () => 1; + } + + class UnusedSecondLevelTypeWithAbstractBase : UnusedIntermediateTypeWithAbstractOverride + { + public override int Method () => 1; + } + + class UnusedSecondLevelType : UnusedIntermediateType + { + public override int Method () => 1; + } + } +} From ea2dd483aec89710e643d1a8556c680405987dc0 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 15 Nov 2022 17:05:18 -0800 Subject: [PATCH 3/5] Add test case for #3112 with pseudo-circular reference with ifaces --- ...rovidesInterfaceMethodCircularReference.cs | 38 +++++++++++++++++++ .../Dependencies/Base.cs | 15 ++++++++ .../Dependencies/Derived1.cs | 9 +++++ .../Dependencies/IFoo.cs | 17 +++++++++ 4 files changed, 79 insertions(+) create mode 100644 test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/BaseProvidesInterfaceMethodCircularReference.cs create mode 100644 test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/Base.cs create mode 100644 test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/Derived1.cs create mode 100644 test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/IFoo.cs diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/BaseProvidesInterfaceMethodCircularReference.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/BaseProvidesInterfaceMethodCircularReference.cs new file mode 100644 index 000000000000..0750274d0891 --- /dev/null +++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/BaseProvidesInterfaceMethodCircularReference.cs @@ -0,0 +1,38 @@ +// 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 Mono.Linker.Tests.Cases.Expectations.Assertions; +using Mono.Linker.Tests.Cases.Expectations.Metadata; +using Mono.Linker.Tests.Cases.Inheritance.Interfaces.BaseProvidesInterfaceEdgeCase.Dependencies; + +namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces.BaseProvidesInterfaceEdgeCase +{ + /// + /// Reproduces the issue found in https://github.com/dotnet/linker/issues/3112. + /// derives from and uses 's method to implement , + /// creating a psuedo-circular assembly reference (but not quite since doesn't implement IFoo itself). + /// In the linker, IsMethodNeededByInstantiatedTypeDueToPreservedScope would iterate through 's method's base methods, + /// and in the process would trigger the assembly of to be processed. Since that assembly also has that + /// inherits from and implements using 's methods, the linker adds + /// '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 ("derived1.dll", new[] { "Dependencies/Derived1.cs" }, references: new[] { "ifoo.dll", "base.dll" })] + public class BaseProvidesInterfaceMethodCircularReference + { + [Kept] + public static void Main () + { + _ = new Derived1 (); + Foo (); + } + + [Kept] + public static void Foo () + { + ((IFoo) null).Method (); + object x = null; + } + } +} diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/Base.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/Base.cs new file mode 100644 index 000000000000..58eebea3aa6c --- /dev/null +++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/Base.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.Inheritance.Interfaces.BaseProvidesInterfaceEdgeCase.Dependencies +{ + public class Base + { + public virtual void Method() + { + throw new NotImplementedException(); + } + } +} diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/Derived1.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/Derived1.cs new file mode 100644 index 000000000000..5cac5a8953d8 --- /dev/null +++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/Derived1.cs @@ -0,0 +1,9 @@ +// 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.BaseProvidesInterfaceEdgeCase.Dependencies +{ + public class Derived1 : Base, IFoo + { + } +} diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/IFoo.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/IFoo.cs new file mode 100644 index 000000000000..9d77d7960e50 --- /dev/null +++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/BaseProvidesInterfaceEdgeCase/Dependencies/IFoo.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.BaseProvidesInterfaceEdgeCase.Dependencies +{ + public interface IFoo + { + void Method(); + } + public interface IBar + { + void Method(); + } + public class Derived2 : Base, IBar + { + } +} From ebe29eef166cebe55b1f94dca66928f1f365c29a Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Mon, 28 Nov 2022 13:43:35 -0600 Subject: [PATCH 4/5] Link issue to TODO --- src/linker/Linker.Steps/MarkStep.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 4dd6307c1bae..526256a18448 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -749,7 +749,7 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation) /// /// Marks the Override of with the correct reason. Should be called when returns true. /// - // TODO: Take into account a base method in preserved scope + // TODO: Take into account a base method in preserved scope https://github.com/dotnet/linker/issues/3090 void MarkOverrideForBaseMethod (OverrideInformation overrideInformation) { if (Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override) && Annotations.IsInstantiated (overrideInformation.Override.DeclaringType)) { From 2c0fd5b16ff24941bc851ca2f3ea10a8e80cf1ca Mon Sep 17 00:00:00 2001 From: vitek-karas <10670590+vitek-karas@users.noreply.github.com> Date: Mon, 12 Dec 2022 04:39:15 -0800 Subject: [PATCH 5/5] Adds a test for recursive generics on interfaces This is a copy of the test added in #3156 --- .../DataFlow/GenericParameterDataFlow.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/test/Mono.Linker.Tests.Cases/DataFlow/GenericParameterDataFlow.cs b/test/Mono.Linker.Tests.Cases/DataFlow/GenericParameterDataFlow.cs index e30b6f1d2c98..5b84ffe0712b 100644 --- a/test/Mono.Linker.Tests.Cases/DataFlow/GenericParameterDataFlow.cs +++ b/test/Mono.Linker.Tests.Cases/DataFlow/GenericParameterDataFlow.cs @@ -318,6 +318,8 @@ static void TestInterfaceTypeGenericRequirements () new InterfaceImplementationTypeWithInstantiationOverSelfOnBase (); new InterfaceImplementationTypeWithOpenGenericOnBase (); new InterfaceImplementationTypeWithOpenGenericOnBaseWithRequirements (); + + RecursiveGenericWithInterfacesRequirement.Test (); } interface IGenericInterfaceTypeWithRequirements<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)] T> @@ -345,6 +347,23 @@ class InterfaceImplementationTypeWithOpenGenericOnBaseWithRequirements<[Dynamica { } + class RecursiveGenericWithInterfacesRequirement + { + interface IFace<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.Interfaces)] T> + { + } + + class TestType : IFace + { + } + + public static void Test () + { + var a = typeof (IFace); + var t = new TestType (); + } + } + static void TestTypeGenericRequirementsOnMembers () { // Basically just root everything we need to test