From 34ce1747953d8d60898278311fbead4709c3a729 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 13 Oct 2022 15:32:26 -0500 Subject: [PATCH 01/34] Check for marking method for base only when state changes 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 it's relevant state has changed. These state changes are: - The override's declaring type is marked as instantiated - The base method is marked - The overrides's declaring type is marked as relevant to variant casting --- src/linker/Linker.Steps/MarkStep.cs | 241 +++++++++++------- src/linker/Linker.Steps/SealerStep.cs | 3 +- .../ValidateVirtualMethodAnnotationsStep.cs | 10 +- src/linker/Linker/Annotations.cs | 7 +- src/linker/Linker/TypeMapInfo.cs | 40 ++- ...ceCastableImplementationAttributeIsKept.cs | 4 + .../InterfaceVariants.cs | 1 + .../UnusedInterfacesInPreservedScope.cs | 7 +- 8 files changed, 205 insertions(+), 108 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 5d4065d35dd9..41b5544e99bb 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 is not null && @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,75 @@ 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. + /// + /// True if the Override is the immediate override method in the type hierarchy. + /// False if Override is further removed (e.g. Override is a method on a derived type of a derived type of Base.DeclaringType) + // TODO: Take into account a base method in preserved scope + bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool isDirectBase) { - var method = overrideInformation.Override; - var @base = overrideInformation.Base; - if (!Annotations.IsMarked (method.DeclaringType)) - return; - - if (Annotations.IsProcessed (method)) - return; + var isInstantiated = Annotations.IsInstantiated (overrideInformation.Override.DeclaringType); - if (Annotations.IsMarked (method)) - return; + bool markForOverride = !Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override); - var isInstantiated = Annotations.IsInstantiated (method.DeclaringType); + // 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 + markForOverride |= !overrideInformation.Base.DeclaringType.IsInterface && isInstantiated; - // 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; + markForOverride |= IsInterfaceImplementationMethodNeededByTypeDueToInterface (overrideInformation); + } else { + // 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. + markForOverride |= overrideInformation.Base.IsAbstract && isDirectBase; } - // 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; + return markForOverride; + } - // 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) + { + if (!Annotations.IsMarked (method.DeclaringType) || Annotations.GetBaseMethods (method) is not List bases) + return; + + foreach (var (ov, isDirectBase) in GetMarkedBaseMethods (bases)) { + if (ShouldMarkOverrideForBase (ov, isDirectBase)) + MarkOverrideForBaseMethod (ov); + } + + IEnumerable<(OverrideInformation Override, bool IsDirectBase)> GetMarkedBaseMethods (IEnumerable overrides) + { + foreach (var ov in overrides) { + // Immediate base methods + if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)) + yield return (ov, true); + + // Bases of bases + else if (Annotations.GetBaseMethods (ov.Base) is IEnumerable baseBases) { + foreach (var baseOv in GetMarkedBaseMethods (baseBases)) { + yield return (baseOv.Override, false); + } + } + } + } } /// @@ -1841,7 +1867,7 @@ protected virtual void MarkSerializable (TypeDefinition type) // If a type is visible to reflection, we need to stop doing optimization that could cause observable difference // in reflection APIs. This includes APIs like MakeGenericType (where variant castability of the produced type // could be incorrect) or IsAssignableFrom (where assignability of unconstructed types might change). - Annotations.MarkRelevantToVariantCasting (definition); + MarkAsRelevantToVariantCasting (definition); Annotations.MarkReflectionUsed (definition); @@ -1904,7 +1930,7 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in if (reference == null) return null; - using var localScope = origin.HasValue ? ScopeStack.PushScope (origin.Value) : null; + using var localScoe = origin.HasValue ? ScopeStack.PushScope (origin.Value) : null; (reference, reason) = GetOriginalType (reference, reason); @@ -2003,6 +2029,16 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in if (type.IsInterface) { // There's no benefit to deferring processing of an interface type until we know a type implementing that interface is marked MarkRequirementsForInstantiatedTypes (type); + // Mark interface implementation on each implementor that requires interfaces. + if (Annotations.GetInterfaceImplementors (type) is List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)> implementors) { + foreach (var implementor in implementors) { + // Knowing that the interface type is marked, we need to mark the interface implementation if + // to make sure that the implementing type is instantiated or relevant to variant casting + if (Annotations.IsMarked (type) || Annotations.IsRelevantToVariantCasting (type)) { + MarkInterfaceImplementation (implementor.InterfaceImplementation); + } + } + } } else if (type.IsValueType) { // Note : Technically interfaces could be removed from value types in some of the same cases as reference types, however, it's harder to know when // a value type instance could exist. You'd have to track initobj and maybe locals types. Going to punt for now. @@ -2027,6 +2063,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 +2374,29 @@ 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) || IgnoreScope (iface.InterfaceType.Scope)) + 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 +2441,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; @@ -2461,11 +2524,11 @@ bool IsMethodNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition metho if (base_list == null) return false; - foreach (MethodDefinition @base in base_list) { - if (IgnoreScope (@base.DeclaringType.Scope)) + foreach (OverrideInformation ov in base_list) { + if (IgnoreScope (ov.Base.DeclaringType.Scope)) return true; - if (IsMethodNeededByTypeDueToPreservedScope (@base)) + if (IsMethodNeededByTypeDueToPreservedScope (ov.Base)) return true; } @@ -2691,7 +2754,7 @@ void MarkGenericArguments (IGenericInstance instance) if (argumentTypeDef == null) continue; - Annotations.MarkRelevantToVariantCasting (argumentTypeDef); + MarkAsRelevantToVariantCasting (argumentTypeDef); if (parameter.HasDefaultConstructorConstraint) MarkDefaultConstructor (argumentTypeDef, new DependencyInfo (DependencyKind.DefaultCtorForNewConstrainedGenericArgument, instance)); @@ -2900,7 +2963,7 @@ protected internal void MarkIndirectlyCalledMethod (MethodDefinition method, in MarkType (reference.DeclaringType, new DependencyInfo (DependencyKind.DeclaringType, reference)); if (reference.Name == ".ctor" && Context.TryResolve (arrayType) is TypeDefinition typeDefinition) { - Annotations.MarkRelevantToVariantCasting (typeDefinition); + MarkAsRelevantToVariantCasting (typeDefinition); } return null; } @@ -3180,6 +3243,15 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo MarkBaseMethods (method); + // TODO: Technically we only need to check the virtual/override pair where `method` is the base method. + // This does extra work checking all other base methods of each `@override` method + if (Annotations.GetOverrides (method) is IEnumerable overrides) { + foreach (var @override in overrides) { + if (ShouldMarkOverrideForBase (@override, true)) + 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)); @@ -3224,6 +3296,26 @@ void MarkImplicitlyUsedFields (TypeDefinition type) MarkFields (type, includeStatic: type.IsEnum, reason: new DependencyInfo (DependencyKind.MemberOfType, type)); } + void MarkAsRelevantToVariantCasting (TypeDefinition type) + { + if (Annotations.IsRelevantToVariantCasting (type)) + return; + Annotations.MarkRelevantToVariantCasting (type); + MarkInterfaceImplementations (type); + // Look at all methods on base types that may implement + MarkMethodsOnTypeIfNeededByBaseMethod (type); + } + + void MarkMethodsOnTypeIfNeededByBaseMethod (TypeDefinition type) + { + do { + foreach (var method in type.Methods) { + MarkMethodIfNeededByBaseMethod (method); + } + type = Context.Resolve (type.BaseType)!; + } while (type is not null); + } + protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type) { if (Annotations.IsInstantiated (type)) @@ -3235,27 +3327,13 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type MarkInterfaceImplementations (type); - foreach (var method in GetRequiredMethodsForInstantiatedType (type)) - MarkMethod (method, new DependencyInfo (DependencyKind.MethodForInstantiatedType, type), ScopeStack.CurrentScope.Origin); + MarkMethodsOnTypeIfNeededByBaseMethod (type); 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; - } + // Need to traverse through all the base type's methods on a type that may implement an interface method } void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov) @@ -3351,18 +3429,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); } } @@ -3657,8 +3735,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) - Annotations.MarkRelevantToVariantCasting (typeDefinition); + if (Context.TryResolve (operand) is TypeDefinition typeDefinition) { + MarkAsRelevantToVariantCasting (typeDefinition); + } break; case Code.Isinst: if (operand is TypeSpecification || operand is GenericParameter) @@ -3688,33 +3767,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; @@ -3722,7 +3780,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..d43ca715e5d4 100644 --- a/src/linker/Linker.Steps/SealerStep.cs +++ b/src/linker/Linker.Steps/SealerStep.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; +using System.Linq; using Mono.Cecil; using Mono.Collections.Generic; @@ -106,7 +107,7 @@ void ProcessType (TypeDefinition type) if (!type.IsSealed) continue; - var bases = Annotations.GetBaseMethods (method); + var bases = Annotations.GetBaseMethods (method)?.Select (static x => x.Base).ToList (); // Devirtualize if a method is not override to existing marked methods if (!IsAnyMarked (bases)) method.IsVirtual = method.IsFinal = method.IsNewSlot = 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..73cf0f3e14ee 100644 --- a/src/linker/Linker/Annotations.cs +++ b/src/linker/Linker/Annotations.cs @@ -446,11 +446,16 @@ public bool IsPublic (IMetadataTokenProvider provider) return TypeMapInfo.GetDefaultInterfaceImplementations (method); } - public List? GetBaseMethods (MethodDefinition method) + public List? GetBaseMethods (MethodDefinition method) { return TypeMapInfo.GetBaseMethods (method); } + public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) + { + return TypeMapInfo.GetInterfaceImplementors (type); + } + public List? GetPreservedMethods (TypeDefinition type) { return GetPreservedMethods (type as IMemberDefinition); diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index 75613e2c0734..922eb39e9fc7 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -29,6 +29,7 @@ // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. // +using System; using System.Collections.Generic; using Mono.Cecil; @@ -39,9 +40,10 @@ 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> (); + protected readonly Dictionary> interface_implementors = new (); public TypeMapInfo (LinkContext context) { @@ -64,10 +66,10 @@ void EnsureProcessed (AssemblyDefinition assembly) return overrides; } - public List? GetBaseMethods (MethodDefinition method) + 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 +79,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) @@ -107,10 +109,20 @@ public void AddDefaultInterfaceImplementation (MethodDefinition @base, TypeDefin implementations.Add ((implementingType, matchingInterfaceImplementation)); } + public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) + { + if (!type.IsInterface) + throw new InvalidOperationException ("Cannot get interface implementors of a type that is not an interface"); + if (interface_implementors.TryGetValue (type, out var interfaces)) + return interfaces; + return null; + } + protected virtual void MapType (TypeDefinition type) { MapVirtualMethods (type); MapInterfaceMethodsInTypeHierarchy (type); + MapInterfaces (type); if (!type.HasNestedTypes) return; @@ -163,6 +175,18 @@ void MapInterfaceMethodsInTypeHierarchy (TypeDefinition type) } } + void MapInterfaces (TypeDefinition type) + { + foreach (var iface in type.GetAllInterfaceImplementations (context, true)) { + if (context.Resolve (iface.InterfaceType) is not TypeDefinition ifaceType) + continue; + if (!interface_implementors.TryGetValue (ifaceType, out var implementors)) { + implementors = new (); + } + implementors.Add ((type, iface)); + } + } + void MapVirtualMethods (TypeDefinition type) { if (!type.HasMethods) @@ -204,7 +228,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..a46832ac4213 100644 --- a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs +++ b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs @@ -54,6 +54,8 @@ static IReferencedAssembly GetReferencedInterface (object obj) #if NETCOREAPP [Kept] [KeptMember (".ctor()")] + // Interface is not from a 'link' scope, so it is kept + [KeptInterface (typeof (IDynamicInterfaceCastable))] class Foo : IDynamicInterfaceCastable { [Kept] @@ -74,6 +76,8 @@ public bool IsInterfaceImplemented (RuntimeTypeHandle interfaceType, bool throwI [Kept] [KeptMember (".ctor()")] + // Interface is not from a 'link' scope, so it is kept + [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..c03e416aa527 100644 --- a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs +++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs @@ -113,6 +113,7 @@ public static void Test () } } + // Interfaces are kept despite being uninstantiated because it is relevant to variant casting [Kept] [KeptInterface (typeof (IEnumerator))] [KeptInterface (typeof (IPublicInterface))] 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 bdca16ac2482120ef60f4f0a5f79b38a71b61d15 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 18 Oct 2022 12:27:35 -0700 Subject: [PATCH 02/34] Special case IDynamicInterfaceCastable and don't keep all interfaces from preserved scope --- src/linker/Linker.Steps/MarkStep.cs | 24 ++++++++++++------- ...ceCastableImplementationAttributeIsKept.cs | 2 -- .../InterfaceVariants.cs | 22 +++++++---------- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 41b5544e99bb..6671ffa52207 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -36,6 +36,7 @@ using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Reflection.Runtime.TypeParsing; +using System.Runtime.InteropServices; using System.Text.RegularExpressions; using ILLink.Shared; using ILLink.Shared.TrimAnalysis; @@ -722,8 +723,14 @@ void ProcessVirtualMethod (MethodDefinition method) /// True if the Override is the immediate override method in the type hierarchy. /// False if Override is further removed (e.g. Override is a method on a derived type of a derived type of Base.DeclaringType) // TODO: Take into account a base method in preserved scope + // TODO: Move interface method marking logic here bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool isDirectBase) { + if (overrideInformation.IsOverrideOfInterfaceMember) { + _interfaceOverrides.Add ((overrideInformation, ScopeStack.CurrentScope)); + return false; + } + var isInstantiated = Annotations.IsInstantiated (overrideInformation.Override.DeclaringType); bool markForOverride = !Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override); @@ -732,14 +739,9 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool is // Interface ov.Overrides should only be marked if the interfaceImplementation is marked, which is handled below markForOverride |= !overrideInformation.Base.DeclaringType.IsInterface && isInstantiated; - if (overrideInformation.IsOverrideOfInterfaceMember) { - _interfaceOverrides.Add ((overrideInformation, ScopeStack.CurrentScope)); - markForOverride |= IsInterfaceImplementationMethodNeededByTypeDueToInterface (overrideInformation); - } else { - // 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. - markForOverride |= overrideInformation.Base.IsAbstract && isDirectBase; - } + // 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. + markForOverride |= overrideInformation.Base.IsAbstract && isDirectBase; return markForOverride; } @@ -2386,7 +2388,7 @@ bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementa if (Context.Resolve (iface.InterfaceType) is not TypeDefinition resolvedInterfaceType) return false; - if (Annotations.IsMarked (resolvedInterfaceType) || IgnoreScope (iface.InterfaceType.Scope)) + 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, @@ -2394,6 +2396,9 @@ bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementa if (resolvedInterfaceType.IsImport || resolvedInterfaceType.IsWindowsRuntime) return true; + if (resolvedInterfaceType.IsTypeOf ()) + return true; + return IsFullyPreserved (type); } @@ -3327,6 +3332,7 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type MarkInterfaceImplementations (type); + // Requires interface implementations to be marked first MarkMethodsOnTypeIfNeededByBaseMethod (type); MarkImplicitlyUsedFields (type); diff --git a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs index a46832ac4213..febb4f26eb6b 100644 --- a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs +++ b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs @@ -54,7 +54,6 @@ static IReferencedAssembly GetReferencedInterface (object obj) #if NETCOREAPP [Kept] [KeptMember (".ctor()")] - // Interface is not from a 'link' scope, so it is kept [KeptInterface (typeof (IDynamicInterfaceCastable))] class Foo : IDynamicInterfaceCastable { @@ -76,7 +75,6 @@ public bool IsInterfaceImplemented (RuntimeTypeHandle interfaceType, bool throwI [Kept] [KeptMember (".ctor()")] - // Interface is not from a 'link' scope, so it is kept [KeptInterface (typeof (IDynamicInterfaceCastable))] class DynamicCastableImplementedInOtherAssembly : IDynamicInterfaceCastable { diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs index c03e416aa527..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 { @@ -115,7 +120,6 @@ 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))] @@ -152,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] @@ -199,7 +197,6 @@ public string ToString (string format, IFormatProvider formatProvider) } [Kept] - [KeptInterface (typeof (IEnumerator))] [KeptInterface (typeof (IPublicInterface))] [KeptInterface (typeof (IPublicStaticInterface))] [KeptInterface (typeof (ICopyLibraryInterface))] @@ -236,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] From e5643fb6fdcb46e6aca167880be0699344b0eab4 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 18 Oct 2022 12:52:19 -0700 Subject: [PATCH 03/34] Remove unused method and redundant while loop --- src/linker/Linker.Steps/MarkStep.cs | 42 +++-------------------------- 1 file changed, 3 insertions(+), 39 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 6671ffa52207..45c579e425d6 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -2511,35 +2511,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 (OverrideInformation ov in base_list) { - if (IgnoreScope (ov.Base.DeclaringType.Scope)) - return true; - - if (IsMethodNeededByTypeDueToPreservedScope (ov.Base)) - return true; - } - - return false; - } - static bool IsSpecialSerializationConstructor (MethodDefinition method) { if (!method.IsInstanceConstructor ()) @@ -3248,8 +3219,6 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo MarkBaseMethods (method); - // TODO: Technically we only need to check the virtual/override pair where `method` is the base method. - // This does extra work checking all other base methods of each `@override` method if (Annotations.GetOverrides (method) is IEnumerable overrides) { foreach (var @override in overrides) { if (ShouldMarkOverrideForBase (@override, true)) @@ -3313,12 +3282,9 @@ void MarkAsRelevantToVariantCasting (TypeDefinition type) void MarkMethodsOnTypeIfNeededByBaseMethod (TypeDefinition type) { - do { - foreach (var method in type.Methods) { - MarkMethodIfNeededByBaseMethod (method); - } - type = Context.Resolve (type.BaseType)!; - } while (type is not null); + foreach (var method in type.Methods) { + MarkMethodIfNeededByBaseMethod (method); + } } protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type) @@ -3338,8 +3304,6 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type MarkImplicitlyUsedFields (type); DoAdditionalInstantiatedTypeProcessing (type); - - // Need to traverse through all the base type's methods on a type that may implement an interface method } void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov) From 3854d3fea27fb8c686ad52f3f8fb15689d3c6d1b Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 18 Oct 2022 16:01:00 -0700 Subject: [PATCH 04/34] Add comments to TypeMapInfo methods --- src/linker/Linker/Annotations.cs | 13 +++++++++++++ src/linker/Linker/TypeMapInfo.cs | 19 ++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/linker/Linker/Annotations.cs b/src/linker/Linker/Annotations.cs index 73cf0f3e14ee..ebbf29a8f5af 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,11 +449,21 @@ public bool IsPublic (IMetadataTokenProvider provider) return TypeMapInfo.GetDefaultInterfaceImplementations (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); } + /// + /// Returns a list of all known types that implement . The list may be incomplete if other implementing types exist in assemblies that haven't been processed by TypeMapInfo yet + /// public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) { return TypeMapInfo.GetInterfaceImplementors (type); diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index 922eb39e9fc7..5b03b1d8c640 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -59,6 +59,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); @@ -66,6 +69,13 @@ void EnsureProcessed (AssemblyDefinition assembly) return overrides; } + /// + /// 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) { EnsureProcessed (method.Module.Assembly); @@ -109,11 +119,14 @@ public void AddDefaultInterfaceImplementation (MethodDefinition @base, TypeDefin implementations.Add ((implementingType, matchingInterfaceImplementation)); } - public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) + /// + /// Returns a list of all known types that implement . The list may be incomplete if other implementing types exist in assemblies that haven't been processed by TypeMapInfo yet + /// + public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition interfaceType) { - if (!type.IsInterface) + if (!interfaceType.IsInterface) throw new InvalidOperationException ("Cannot get interface implementors of a type that is not an interface"); - if (interface_implementors.TryGetValue (type, out var interfaces)) + if (interface_implementors.TryGetValue (interfaceType, out var interfaces)) return interfaces; return null; } From 04801f1f6b6ea0cb83431f92c7713b3c3870512a Mon Sep 17 00:00:00 2001 From: vitek-karas <10670590+vitek-karas@users.noreply.github.com> Date: Wed, 19 Oct 2022 04:37:08 -0700 Subject: [PATCH 05/34] Change analyzer versions such that the repo can be built with .NET 7 RC2 SDK. Took the versions from dotnet/runtime. --- eng/Analyzers.props | 1 + eng/Versions.props | 3 ++- eng/ilasm.ilproj | 2 +- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/eng/Analyzers.props b/eng/Analyzers.props index 37816d62cac7..facf033c323d 100644 --- a/eng/Analyzers.props +++ b/eng/Analyzers.props @@ -2,6 +2,7 @@ + diff --git a/eng/Versions.props b/eng/Versions.props index 4af4503d7758..5d58e7e49e9d 100644 --- a/eng/Versions.props +++ b/eng/Versions.props @@ -21,10 +21,11 @@ 8.0.0-beta.22473.1 6.0.0-beta.21271.1 3.10.0-2.final - 4.4.0-1.final + 4.5.0-1.22517.9 $(MicrosoftCodeAnalysisVersion) 1.0.1-beta1.* 3.3.2 + 7.0.0-preview1.22513.1 7.0.0-preview.7.22375.6 - net5.0 + net7.0 From d406e2b030f7d01a43c940adbeabfe351b612970 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Wed, 19 Oct 2022 09:44:39 -0700 Subject: [PATCH 06/34] PR feedback: Fix typo and return early --- src/linker/Linker.Steps/MarkStep.cs | 13 ++++++++----- src/linker/Linker.Steps/SealerStep.cs | 16 ++++++++-------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 45c579e425d6..0fe4944fdf48 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -733,17 +733,20 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool is var isInstantiated = Annotations.IsInstantiated (overrideInformation.Override.DeclaringType); - bool markForOverride = !Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override); + 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 - markForOverride |= !overrideInformation.Base.DeclaringType.IsInterface && isInstantiated; + if (!overrideInformation.Base.DeclaringType.IsInterface && isInstantiated) + 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. - markForOverride |= overrideInformation.Base.IsAbstract && isDirectBase; + if (overrideInformation.Base.IsAbstract && isDirectBase) + return true; - return markForOverride; + return false; } /// @@ -1932,7 +1935,7 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in if (reference == null) return null; - using var localScoe = origin.HasValue ? ScopeStack.PushScope (origin.Value) : null; + using var localScope = origin.HasValue ? ScopeStack.PushScope (origin.Value) : null; (reference, reason) = GetOriginalType (reference, reason); diff --git a/src/linker/Linker.Steps/SealerStep.cs b/src/linker/Linker.Steps/SealerStep.cs index d43ca715e5d4..f02697e843a7 100644 --- a/src/linker/Linker.Steps/SealerStep.cs +++ b/src/linker/Linker.Steps/SealerStep.cs @@ -3,7 +3,6 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using Mono.Cecil; using Mono.Collections.Generic; @@ -97,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); @@ -107,9 +106,9 @@ void ProcessType (TypeDefinition type) if (!type.IsSealed) continue; - var bases = Annotations.GetBaseMethods (method)?.Select (static x => x.Base).ToList (); - // Devirtualize if a method is not override to existing marked methods - if (!IsAnyMarked (bases)) + var bases = Annotations.GetBaseMethods (method); //?.Select (static x => x.Base); + // Devirtualize if a method is not override to existing marked methods + if (bases is not null && !IsAnyBaseMarked (bases)) method.IsVirtual = method.IsFinal = method.IsNewSlot = false; } } @@ -124,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; @@ -136,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; From 20c52afacff0af39dad0c045e94bea6b69663658 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Wed, 19 Oct 2022 10:30:22 -0700 Subject: [PATCH 07/34] Remove commented code and extra null check --- src/linker/Linker.Steps/SealerStep.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/linker/Linker.Steps/SealerStep.cs b/src/linker/Linker.Steps/SealerStep.cs index f02697e843a7..dbc64f0f1669 100644 --- a/src/linker/Linker.Steps/SealerStep.cs +++ b/src/linker/Linker.Steps/SealerStep.cs @@ -106,9 +106,9 @@ void ProcessType (TypeDefinition type) if (!type.IsSealed) continue; - var bases = Annotations.GetBaseMethods (method); //?.Select (static x => x.Base); - // Devirtualize if a method is not override to existing marked methods - if (bases is not null && !IsAnyBaseMarked (bases)) + var bases = Annotations.GetBaseMethods (method); + // Devirtualize if a method is not override to existing marked methods + if (!IsAnyBaseMarked (bases)) method.IsVirtual = method.IsFinal = method.IsNewSlot = false; } } From 6484a010183ec2289afd2045ea962105f069ce08 Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Thu, 20 Oct 2022 15:36:41 -0700 Subject: [PATCH 08/34] Update src/linker/Linker/TypeMapInfo.cs Co-authored-by: Sven Boemer --- src/linker/Linker/TypeMapInfo.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index 5b03b1d8c640..96713fca1d91 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -71,7 +71,7 @@ void EnsureProcessed (AssemblyDefinition assembly) /// /// 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), + /// This includes the closest overridden virtual method on 's base types /// 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. From e6c0429a19d0c5fb67d15755216eeb3f00bdf939 Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Thu, 20 Oct 2022 15:36:50 -0700 Subject: [PATCH 09/34] Update src/linker/Linker/TypeMapInfo.cs Co-authored-by: Sven Boemer --- src/linker/Linker/TypeMapInfo.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index 96713fca1d91..b340ca5f7264 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -72,7 +72,7 @@ void EnsureProcessed (AssemblyDefinition assembly) /// /// Returns all base methods that overrides. /// This includes the closest overridden virtual method on 's base types - /// methods on an interface that 's delcaring type implements, + /// 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. /// From 324d4cbb069b4782bdb210e1c15884db790ee9a5 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 20 Oct 2022 16:06:36 -0700 Subject: [PATCH 10/34] PR feedback: - use null check instead of type check - Remove redundant interface check --- src/linker/Linker.Steps/MarkStep.cs | 31 +++++++++++++++-------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 0fe4944fdf48..e843bcb3ed81 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -719,6 +719,7 @@ 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. /// /// True if the Override is the immediate override method in the type hierarchy. /// False if Override is further removed (e.g. Override is a method on a derived type of a derived type of Base.DeclaringType) @@ -731,14 +732,12 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool is return false; } - var isInstantiated = Annotations.IsInstantiated (overrideInformation.Override.DeclaringType); - 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 + // 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 (!overrideInformation.Base.DeclaringType.IsInterface && isInstantiated) + if (Annotations.IsInstantiated (overrideInformation.Override.DeclaringType)) return true; // Direct overrides of marked abstract ov.Overrides must be marked or we get invalid IL. @@ -766,7 +765,11 @@ void MarkOverrideForBaseMethod (OverrideInformation overrideInformation) void MarkMethodIfNeededByBaseMethod (MethodDefinition method) { - if (!Annotations.IsMarked (method.DeclaringType) || Annotations.GetBaseMethods (method) is not List bases) + if (!Annotations.IsMarked (method.DeclaringType)) + return; + + var bases = Annotations.GetBaseMethods (method); + if (bases is null) return; foreach (var (ov, isDirectBase) in GetMarkedBaseMethods (bases)) { @@ -778,13 +781,15 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method) { foreach (var ov in overrides) { // Immediate base methods - if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)) + if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)){ yield return (ov, true); - - // Bases of bases - else if (Annotations.GetBaseMethods (ov.Base) is IEnumerable baseBases) { - foreach (var baseOv in GetMarkedBaseMethods (baseBases)) { - yield return (baseOv.Override, false); + } else { + // Bases of bases + var baseBases = Annotations.GetBaseMethods (ov.Base); + if (baseBases is not null) { + foreach (var baseOv in GetMarkedBaseMethods (baseBases)) { + yield return (baseOv.Override, false); + } } } } @@ -2399,10 +2404,6 @@ bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementa if (resolvedInterfaceType.IsImport || resolvedInterfaceType.IsWindowsRuntime) return true; - if (resolvedInterfaceType.IsTypeOf ()) - return true; - - return IsFullyPreserved (type); } } From 756f54724ed2aea408d038d8a464ee4032e1b4c7 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 20 Oct 2022 16:22:34 -0700 Subject: [PATCH 11/34] format --- 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 e843bcb3ed81..8342a0c1f59b 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -781,7 +781,7 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method) { foreach (var ov in overrides) { // Immediate base methods - if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)){ + if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)) { yield return (ov, true); } else { // Bases of bases From e18e5799d4ea548872827a7deb357c1e4a280a46 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 20 Oct 2022 17:15:42 -0700 Subject: [PATCH 12/34] remove using --- src/linker/Linker.Steps/MarkStep.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 8342a0c1f59b..bb6f12ebf988 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -36,7 +36,6 @@ using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Reflection.Runtime.TypeParsing; -using System.Runtime.InteropServices; using System.Text.RegularExpressions; using ILLink.Shared; using ILLink.Shared.TrimAnalysis; From cd38208b46d454d63e3ee2495f2fd6d5bcb1e4ba Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Fri, 21 Oct 2022 15:01:10 -0700 Subject: [PATCH 13/34] Don't ignore descriptors in test --- ...WithDynamicInterfaceCastableImplementationAttributeIsKept.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs index febb4f26eb6b..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 () From c0db7f558dc8678288646be135aa4aa61093f6ce Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 25 Oct 2022 12:08:13 -0700 Subject: [PATCH 14/34] Don't go through bases of bases when in GetMarkedBaseMethods --- src/linker/Linker.Steps/MarkStep.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index bb6f12ebf988..f6d751a18ad5 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -779,17 +779,8 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method) IEnumerable<(OverrideInformation Override, bool IsDirectBase)> GetMarkedBaseMethods (IEnumerable overrides) { foreach (var ov in overrides) { - // Immediate base methods if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)) { yield return (ov, true); - } else { - // Bases of bases - var baseBases = Annotations.GetBaseMethods (ov.Base); - if (baseBases is not null) { - foreach (var baseOv in GetMarkedBaseMethods (baseBases)) { - yield return (baseOv.Override, false); - } - } } } } From 44c4c748a0c8ae25443b10f7b0704f8791e15324 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 13 Oct 2022 15:32:26 -0500 Subject: [PATCH 15/34] Check for marking method for base only when state changes 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 it's relevant state has changed. These state changes are: - The override's declaring type is marked as instantiated - The base method is marked - The overrides's declaring type is marked as relevant to variant casting --- src/linker/Linker.Steps/MarkStep.cs | 241 +++++++++++------- src/linker/Linker.Steps/SealerStep.cs | 3 +- .../ValidateVirtualMethodAnnotationsStep.cs | 10 +- src/linker/Linker/Annotations.cs | 7 +- src/linker/Linker/TypeMapInfo.cs | 40 ++- ...ceCastableImplementationAttributeIsKept.cs | 4 + .../InterfaceVariants.cs | 1 + .../UnusedInterfacesInPreservedScope.cs | 7 +- 8 files changed, 205 insertions(+), 108 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 5d4065d35dd9..41b5544e99bb 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 is not null && @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,75 @@ 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. + /// + /// True if the Override is the immediate override method in the type hierarchy. + /// False if Override is further removed (e.g. Override is a method on a derived type of a derived type of Base.DeclaringType) + // TODO: Take into account a base method in preserved scope + bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool isDirectBase) { - var method = overrideInformation.Override; - var @base = overrideInformation.Base; - if (!Annotations.IsMarked (method.DeclaringType)) - return; - - if (Annotations.IsProcessed (method)) - return; + var isInstantiated = Annotations.IsInstantiated (overrideInformation.Override.DeclaringType); - if (Annotations.IsMarked (method)) - return; + bool markForOverride = !Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override); - var isInstantiated = Annotations.IsInstantiated (method.DeclaringType); + // 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 + markForOverride |= !overrideInformation.Base.DeclaringType.IsInterface && isInstantiated; - // 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; + markForOverride |= IsInterfaceImplementationMethodNeededByTypeDueToInterface (overrideInformation); + } else { + // 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. + markForOverride |= overrideInformation.Base.IsAbstract && isDirectBase; } - // 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; + return markForOverride; + } - // 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) + { + if (!Annotations.IsMarked (method.DeclaringType) || Annotations.GetBaseMethods (method) is not List bases) + return; + + foreach (var (ov, isDirectBase) in GetMarkedBaseMethods (bases)) { + if (ShouldMarkOverrideForBase (ov, isDirectBase)) + MarkOverrideForBaseMethod (ov); + } + + IEnumerable<(OverrideInformation Override, bool IsDirectBase)> GetMarkedBaseMethods (IEnumerable overrides) + { + foreach (var ov in overrides) { + // Immediate base methods + if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)) + yield return (ov, true); + + // Bases of bases + else if (Annotations.GetBaseMethods (ov.Base) is IEnumerable baseBases) { + foreach (var baseOv in GetMarkedBaseMethods (baseBases)) { + yield return (baseOv.Override, false); + } + } + } + } } /// @@ -1841,7 +1867,7 @@ protected virtual void MarkSerializable (TypeDefinition type) // If a type is visible to reflection, we need to stop doing optimization that could cause observable difference // in reflection APIs. This includes APIs like MakeGenericType (where variant castability of the produced type // could be incorrect) or IsAssignableFrom (where assignability of unconstructed types might change). - Annotations.MarkRelevantToVariantCasting (definition); + MarkAsRelevantToVariantCasting (definition); Annotations.MarkReflectionUsed (definition); @@ -1904,7 +1930,7 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in if (reference == null) return null; - using var localScope = origin.HasValue ? ScopeStack.PushScope (origin.Value) : null; + using var localScoe = origin.HasValue ? ScopeStack.PushScope (origin.Value) : null; (reference, reason) = GetOriginalType (reference, reason); @@ -2003,6 +2029,16 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in if (type.IsInterface) { // There's no benefit to deferring processing of an interface type until we know a type implementing that interface is marked MarkRequirementsForInstantiatedTypes (type); + // Mark interface implementation on each implementor that requires interfaces. + if (Annotations.GetInterfaceImplementors (type) is List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)> implementors) { + foreach (var implementor in implementors) { + // Knowing that the interface type is marked, we need to mark the interface implementation if + // to make sure that the implementing type is instantiated or relevant to variant casting + if (Annotations.IsMarked (type) || Annotations.IsRelevantToVariantCasting (type)) { + MarkInterfaceImplementation (implementor.InterfaceImplementation); + } + } + } } else if (type.IsValueType) { // Note : Technically interfaces could be removed from value types in some of the same cases as reference types, however, it's harder to know when // a value type instance could exist. You'd have to track initobj and maybe locals types. Going to punt for now. @@ -2027,6 +2063,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 +2374,29 @@ 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) || IgnoreScope (iface.InterfaceType.Scope)) + 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 +2441,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; @@ -2461,11 +2524,11 @@ bool IsMethodNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition metho if (base_list == null) return false; - foreach (MethodDefinition @base in base_list) { - if (IgnoreScope (@base.DeclaringType.Scope)) + foreach (OverrideInformation ov in base_list) { + if (IgnoreScope (ov.Base.DeclaringType.Scope)) return true; - if (IsMethodNeededByTypeDueToPreservedScope (@base)) + if (IsMethodNeededByTypeDueToPreservedScope (ov.Base)) return true; } @@ -2691,7 +2754,7 @@ void MarkGenericArguments (IGenericInstance instance) if (argumentTypeDef == null) continue; - Annotations.MarkRelevantToVariantCasting (argumentTypeDef); + MarkAsRelevantToVariantCasting (argumentTypeDef); if (parameter.HasDefaultConstructorConstraint) MarkDefaultConstructor (argumentTypeDef, new DependencyInfo (DependencyKind.DefaultCtorForNewConstrainedGenericArgument, instance)); @@ -2900,7 +2963,7 @@ protected internal void MarkIndirectlyCalledMethod (MethodDefinition method, in MarkType (reference.DeclaringType, new DependencyInfo (DependencyKind.DeclaringType, reference)); if (reference.Name == ".ctor" && Context.TryResolve (arrayType) is TypeDefinition typeDefinition) { - Annotations.MarkRelevantToVariantCasting (typeDefinition); + MarkAsRelevantToVariantCasting (typeDefinition); } return null; } @@ -3180,6 +3243,15 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo MarkBaseMethods (method); + // TODO: Technically we only need to check the virtual/override pair where `method` is the base method. + // This does extra work checking all other base methods of each `@override` method + if (Annotations.GetOverrides (method) is IEnumerable overrides) { + foreach (var @override in overrides) { + if (ShouldMarkOverrideForBase (@override, true)) + 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)); @@ -3224,6 +3296,26 @@ void MarkImplicitlyUsedFields (TypeDefinition type) MarkFields (type, includeStatic: type.IsEnum, reason: new DependencyInfo (DependencyKind.MemberOfType, type)); } + void MarkAsRelevantToVariantCasting (TypeDefinition type) + { + if (Annotations.IsRelevantToVariantCasting (type)) + return; + Annotations.MarkRelevantToVariantCasting (type); + MarkInterfaceImplementations (type); + // Look at all methods on base types that may implement + MarkMethodsOnTypeIfNeededByBaseMethod (type); + } + + void MarkMethodsOnTypeIfNeededByBaseMethod (TypeDefinition type) + { + do { + foreach (var method in type.Methods) { + MarkMethodIfNeededByBaseMethod (method); + } + type = Context.Resolve (type.BaseType)!; + } while (type is not null); + } + protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type) { if (Annotations.IsInstantiated (type)) @@ -3235,27 +3327,13 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type MarkInterfaceImplementations (type); - foreach (var method in GetRequiredMethodsForInstantiatedType (type)) - MarkMethod (method, new DependencyInfo (DependencyKind.MethodForInstantiatedType, type), ScopeStack.CurrentScope.Origin); + MarkMethodsOnTypeIfNeededByBaseMethod (type); 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; - } + // Need to traverse through all the base type's methods on a type that may implement an interface method } void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov) @@ -3351,18 +3429,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); } } @@ -3657,8 +3735,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) - Annotations.MarkRelevantToVariantCasting (typeDefinition); + if (Context.TryResolve (operand) is TypeDefinition typeDefinition) { + MarkAsRelevantToVariantCasting (typeDefinition); + } break; case Code.Isinst: if (operand is TypeSpecification || operand is GenericParameter) @@ -3688,33 +3767,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; @@ -3722,7 +3780,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..d43ca715e5d4 100644 --- a/src/linker/Linker.Steps/SealerStep.cs +++ b/src/linker/Linker.Steps/SealerStep.cs @@ -3,6 +3,7 @@ using System.Collections.Generic; using System.Diagnostics; +using System.Linq; using Mono.Cecil; using Mono.Collections.Generic; @@ -106,7 +107,7 @@ void ProcessType (TypeDefinition type) if (!type.IsSealed) continue; - var bases = Annotations.GetBaseMethods (method); + var bases = Annotations.GetBaseMethods (method)?.Select (static x => x.Base).ToList (); // Devirtualize if a method is not override to existing marked methods if (!IsAnyMarked (bases)) method.IsVirtual = method.IsFinal = method.IsNewSlot = 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..73cf0f3e14ee 100644 --- a/src/linker/Linker/Annotations.cs +++ b/src/linker/Linker/Annotations.cs @@ -446,11 +446,16 @@ public bool IsPublic (IMetadataTokenProvider provider) return TypeMapInfo.GetDefaultInterfaceImplementations (method); } - public List? GetBaseMethods (MethodDefinition method) + public List? GetBaseMethods (MethodDefinition method) { return TypeMapInfo.GetBaseMethods (method); } + public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) + { + return TypeMapInfo.GetInterfaceImplementors (type); + } + public List? GetPreservedMethods (TypeDefinition type) { return GetPreservedMethods (type as IMemberDefinition); diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index 75613e2c0734..922eb39e9fc7 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -29,6 +29,7 @@ // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. // +using System; using System.Collections.Generic; using Mono.Cecil; @@ -39,9 +40,10 @@ 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> (); + protected readonly Dictionary> interface_implementors = new (); public TypeMapInfo (LinkContext context) { @@ -64,10 +66,10 @@ void EnsureProcessed (AssemblyDefinition assembly) return overrides; } - public List? GetBaseMethods (MethodDefinition method) + 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 +79,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) @@ -107,10 +109,20 @@ public void AddDefaultInterfaceImplementation (MethodDefinition @base, TypeDefin implementations.Add ((implementingType, matchingInterfaceImplementation)); } + public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) + { + if (!type.IsInterface) + throw new InvalidOperationException ("Cannot get interface implementors of a type that is not an interface"); + if (interface_implementors.TryGetValue (type, out var interfaces)) + return interfaces; + return null; + } + protected virtual void MapType (TypeDefinition type) { MapVirtualMethods (type); MapInterfaceMethodsInTypeHierarchy (type); + MapInterfaces (type); if (!type.HasNestedTypes) return; @@ -163,6 +175,18 @@ void MapInterfaceMethodsInTypeHierarchy (TypeDefinition type) } } + void MapInterfaces (TypeDefinition type) + { + foreach (var iface in type.GetAllInterfaceImplementations (context, true)) { + if (context.Resolve (iface.InterfaceType) is not TypeDefinition ifaceType) + continue; + if (!interface_implementors.TryGetValue (ifaceType, out var implementors)) { + implementors = new (); + } + implementors.Add ((type, iface)); + } + } + void MapVirtualMethods (TypeDefinition type) { if (!type.HasMethods) @@ -204,7 +228,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..a46832ac4213 100644 --- a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs +++ b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs @@ -54,6 +54,8 @@ static IReferencedAssembly GetReferencedInterface (object obj) #if NETCOREAPP [Kept] [KeptMember (".ctor()")] + // Interface is not from a 'link' scope, so it is kept + [KeptInterface (typeof (IDynamicInterfaceCastable))] class Foo : IDynamicInterfaceCastable { [Kept] @@ -74,6 +76,8 @@ public bool IsInterfaceImplemented (RuntimeTypeHandle interfaceType, bool throwI [Kept] [KeptMember (".ctor()")] + // Interface is not from a 'link' scope, so it is kept + [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..c03e416aa527 100644 --- a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs +++ b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs @@ -113,6 +113,7 @@ public static void Test () } } + // Interfaces are kept despite being uninstantiated because it is relevant to variant casting [Kept] [KeptInterface (typeof (IEnumerator))] [KeptInterface (typeof (IPublicInterface))] 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 f45f2d054142fa36ef236d9f7e45d39835d332e7 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 18 Oct 2022 12:27:35 -0700 Subject: [PATCH 16/34] Special case IDynamicInterfaceCastable and don't keep all interfaces from preserved scope --- src/linker/Linker.Steps/MarkStep.cs | 24 ++++++++++++------- ...ceCastableImplementationAttributeIsKept.cs | 2 -- .../InterfaceVariants.cs | 22 +++++++---------- 3 files changed, 23 insertions(+), 25 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 41b5544e99bb..6671ffa52207 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -36,6 +36,7 @@ using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Reflection.Runtime.TypeParsing; +using System.Runtime.InteropServices; using System.Text.RegularExpressions; using ILLink.Shared; using ILLink.Shared.TrimAnalysis; @@ -722,8 +723,14 @@ void ProcessVirtualMethod (MethodDefinition method) /// True if the Override is the immediate override method in the type hierarchy. /// False if Override is further removed (e.g. Override is a method on a derived type of a derived type of Base.DeclaringType) // TODO: Take into account a base method in preserved scope + // TODO: Move interface method marking logic here bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool isDirectBase) { + if (overrideInformation.IsOverrideOfInterfaceMember) { + _interfaceOverrides.Add ((overrideInformation, ScopeStack.CurrentScope)); + return false; + } + var isInstantiated = Annotations.IsInstantiated (overrideInformation.Override.DeclaringType); bool markForOverride = !Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override); @@ -732,14 +739,9 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool is // Interface ov.Overrides should only be marked if the interfaceImplementation is marked, which is handled below markForOverride |= !overrideInformation.Base.DeclaringType.IsInterface && isInstantiated; - if (overrideInformation.IsOverrideOfInterfaceMember) { - _interfaceOverrides.Add ((overrideInformation, ScopeStack.CurrentScope)); - markForOverride |= IsInterfaceImplementationMethodNeededByTypeDueToInterface (overrideInformation); - } else { - // 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. - markForOverride |= overrideInformation.Base.IsAbstract && isDirectBase; - } + // 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. + markForOverride |= overrideInformation.Base.IsAbstract && isDirectBase; return markForOverride; } @@ -2386,7 +2388,7 @@ bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementa if (Context.Resolve (iface.InterfaceType) is not TypeDefinition resolvedInterfaceType) return false; - if (Annotations.IsMarked (resolvedInterfaceType) || IgnoreScope (iface.InterfaceType.Scope)) + 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, @@ -2394,6 +2396,9 @@ bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementa if (resolvedInterfaceType.IsImport || resolvedInterfaceType.IsWindowsRuntime) return true; + if (resolvedInterfaceType.IsTypeOf ()) + return true; + return IsFullyPreserved (type); } @@ -3327,6 +3332,7 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type MarkInterfaceImplementations (type); + // Requires interface implementations to be marked first MarkMethodsOnTypeIfNeededByBaseMethod (type); MarkImplicitlyUsedFields (type); diff --git a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs index a46832ac4213..febb4f26eb6b 100644 --- a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs +++ b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs @@ -54,7 +54,6 @@ static IReferencedAssembly GetReferencedInterface (object obj) #if NETCOREAPP [Kept] [KeptMember (".ctor()")] - // Interface is not from a 'link' scope, so it is kept [KeptInterface (typeof (IDynamicInterfaceCastable))] class Foo : IDynamicInterfaceCastable { @@ -76,7 +75,6 @@ public bool IsInterfaceImplemented (RuntimeTypeHandle interfaceType, bool throwI [Kept] [KeptMember (".ctor()")] - // Interface is not from a 'link' scope, so it is kept [KeptInterface (typeof (IDynamicInterfaceCastable))] class DynamicCastableImplementedInOtherAssembly : IDynamicInterfaceCastable { diff --git a/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs b/test/Mono.Linker.Tests.Cases/Inheritance.Interfaces/InterfaceVariants.cs index c03e416aa527..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 { @@ -115,7 +120,6 @@ 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))] @@ -152,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] @@ -199,7 +197,6 @@ public string ToString (string format, IFormatProvider formatProvider) } [Kept] - [KeptInterface (typeof (IEnumerator))] [KeptInterface (typeof (IPublicInterface))] [KeptInterface (typeof (IPublicStaticInterface))] [KeptInterface (typeof (ICopyLibraryInterface))] @@ -236,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] From cebaff6486f257260c69712599648ee7851b4cd6 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 18 Oct 2022 12:52:19 -0700 Subject: [PATCH 17/34] Remove unused method and redundant while loop --- src/linker/Linker.Steps/MarkStep.cs | 42 +++-------------------------- 1 file changed, 3 insertions(+), 39 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 6671ffa52207..45c579e425d6 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -2511,35 +2511,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 (OverrideInformation ov in base_list) { - if (IgnoreScope (ov.Base.DeclaringType.Scope)) - return true; - - if (IsMethodNeededByTypeDueToPreservedScope (ov.Base)) - return true; - } - - return false; - } - static bool IsSpecialSerializationConstructor (MethodDefinition method) { if (!method.IsInstanceConstructor ()) @@ -3248,8 +3219,6 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo MarkBaseMethods (method); - // TODO: Technically we only need to check the virtual/override pair where `method` is the base method. - // This does extra work checking all other base methods of each `@override` method if (Annotations.GetOverrides (method) is IEnumerable overrides) { foreach (var @override in overrides) { if (ShouldMarkOverrideForBase (@override, true)) @@ -3313,12 +3282,9 @@ void MarkAsRelevantToVariantCasting (TypeDefinition type) void MarkMethodsOnTypeIfNeededByBaseMethod (TypeDefinition type) { - do { - foreach (var method in type.Methods) { - MarkMethodIfNeededByBaseMethod (method); - } - type = Context.Resolve (type.BaseType)!; - } while (type is not null); + foreach (var method in type.Methods) { + MarkMethodIfNeededByBaseMethod (method); + } } protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type) @@ -3338,8 +3304,6 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type MarkImplicitlyUsedFields (type); DoAdditionalInstantiatedTypeProcessing (type); - - // Need to traverse through all the base type's methods on a type that may implement an interface method } void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov) From af16936a5e30df99c6ac96ccccec140345fc81fe Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 18 Oct 2022 16:01:00 -0700 Subject: [PATCH 18/34] Add comments to TypeMapInfo methods --- src/linker/Linker/Annotations.cs | 13 +++++++++++++ src/linker/Linker/TypeMapInfo.cs | 19 ++++++++++++++++--- 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/src/linker/Linker/Annotations.cs b/src/linker/Linker/Annotations.cs index 73cf0f3e14ee..ebbf29a8f5af 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,11 +449,21 @@ public bool IsPublic (IMetadataTokenProvider provider) return TypeMapInfo.GetDefaultInterfaceImplementations (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); } + /// + /// Returns a list of all known types that implement . The list may be incomplete if other implementing types exist in assemblies that haven't been processed by TypeMapInfo yet + /// public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) { return TypeMapInfo.GetInterfaceImplementors (type); diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index 922eb39e9fc7..5b03b1d8c640 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -59,6 +59,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); @@ -66,6 +69,13 @@ void EnsureProcessed (AssemblyDefinition assembly) return overrides; } + /// + /// 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) { EnsureProcessed (method.Module.Assembly); @@ -109,11 +119,14 @@ public void AddDefaultInterfaceImplementation (MethodDefinition @base, TypeDefin implementations.Add ((implementingType, matchingInterfaceImplementation)); } - public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) + /// + /// Returns a list of all known types that implement . The list may be incomplete if other implementing types exist in assemblies that haven't been processed by TypeMapInfo yet + /// + public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition interfaceType) { - if (!type.IsInterface) + if (!interfaceType.IsInterface) throw new InvalidOperationException ("Cannot get interface implementors of a type that is not an interface"); - if (interface_implementors.TryGetValue (type, out var interfaces)) + if (interface_implementors.TryGetValue (interfaceType, out var interfaces)) return interfaces; return null; } From 4ff3fbe2cbd8146a2e555ddcf9006bc16987202e Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Wed, 19 Oct 2022 09:44:39 -0700 Subject: [PATCH 19/34] PR feedback: Fix typo and return early --- src/linker/Linker.Steps/MarkStep.cs | 13 ++++++++----- src/linker/Linker.Steps/SealerStep.cs | 16 ++++++++-------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 45c579e425d6..0fe4944fdf48 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -733,17 +733,20 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool is var isInstantiated = Annotations.IsInstantiated (overrideInformation.Override.DeclaringType); - bool markForOverride = !Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, overrideInformation.Override); + 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 - markForOverride |= !overrideInformation.Base.DeclaringType.IsInterface && isInstantiated; + if (!overrideInformation.Base.DeclaringType.IsInterface && isInstantiated) + 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. - markForOverride |= overrideInformation.Base.IsAbstract && isDirectBase; + if (overrideInformation.Base.IsAbstract && isDirectBase) + return true; - return markForOverride; + return false; } /// @@ -1932,7 +1935,7 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in if (reference == null) return null; - using var localScoe = origin.HasValue ? ScopeStack.PushScope (origin.Value) : null; + using var localScope = origin.HasValue ? ScopeStack.PushScope (origin.Value) : null; (reference, reason) = GetOriginalType (reference, reason); diff --git a/src/linker/Linker.Steps/SealerStep.cs b/src/linker/Linker.Steps/SealerStep.cs index d43ca715e5d4..f02697e843a7 100644 --- a/src/linker/Linker.Steps/SealerStep.cs +++ b/src/linker/Linker.Steps/SealerStep.cs @@ -3,7 +3,6 @@ using System.Collections.Generic; using System.Diagnostics; -using System.Linq; using Mono.Cecil; using Mono.Collections.Generic; @@ -97,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); @@ -107,9 +106,9 @@ void ProcessType (TypeDefinition type) if (!type.IsSealed) continue; - var bases = Annotations.GetBaseMethods (method)?.Select (static x => x.Base).ToList (); - // Devirtualize if a method is not override to existing marked methods - if (!IsAnyMarked (bases)) + var bases = Annotations.GetBaseMethods (method); //?.Select (static x => x.Base); + // Devirtualize if a method is not override to existing marked methods + if (bases is not null && !IsAnyBaseMarked (bases)) method.IsVirtual = method.IsFinal = method.IsNewSlot = false; } } @@ -124,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; @@ -136,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; From 6a131b827868ab760311d3554bc18a5a1f028bda Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Wed, 19 Oct 2022 10:30:22 -0700 Subject: [PATCH 20/34] Remove commented code and extra null check --- src/linker/Linker.Steps/SealerStep.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/linker/Linker.Steps/SealerStep.cs b/src/linker/Linker.Steps/SealerStep.cs index f02697e843a7..dbc64f0f1669 100644 --- a/src/linker/Linker.Steps/SealerStep.cs +++ b/src/linker/Linker.Steps/SealerStep.cs @@ -106,9 +106,9 @@ void ProcessType (TypeDefinition type) if (!type.IsSealed) continue; - var bases = Annotations.GetBaseMethods (method); //?.Select (static x => x.Base); - // Devirtualize if a method is not override to existing marked methods - if (bases is not null && !IsAnyBaseMarked (bases)) + var bases = Annotations.GetBaseMethods (method); + // Devirtualize if a method is not override to existing marked methods + if (!IsAnyBaseMarked (bases)) method.IsVirtual = method.IsFinal = method.IsNewSlot = false; } } From 6e788353e9b0d73557e5af767f82859856a9c4ab Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Thu, 20 Oct 2022 15:36:41 -0700 Subject: [PATCH 21/34] Update src/linker/Linker/TypeMapInfo.cs Co-authored-by: Sven Boemer --- src/linker/Linker/TypeMapInfo.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index 5b03b1d8c640..96713fca1d91 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -71,7 +71,7 @@ void EnsureProcessed (AssemblyDefinition assembly) /// /// 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), + /// This includes the closest overridden virtual method on 's base types /// 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. From e78d069d9786be53c7bf4ba55aaed6cba4d1741a Mon Sep 17 00:00:00 2001 From: Jackson Schuster <36744439+jtschuster@users.noreply.github.com> Date: Thu, 20 Oct 2022 15:36:50 -0700 Subject: [PATCH 22/34] Update src/linker/Linker/TypeMapInfo.cs Co-authored-by: Sven Boemer --- src/linker/Linker/TypeMapInfo.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index 96713fca1d91..b340ca5f7264 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -72,7 +72,7 @@ void EnsureProcessed (AssemblyDefinition assembly) /// /// Returns all base methods that overrides. /// This includes the closest overridden virtual method on 's base types - /// methods on an interface that 's delcaring type implements, + /// 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. /// From f1af79b3cf83a128f12bd3a6d9e05139ac33ffec Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 20 Oct 2022 16:06:36 -0700 Subject: [PATCH 23/34] PR feedback: - use null check instead of type check - Remove redundant interface check --- src/linker/Linker.Steps/MarkStep.cs | 31 +++++++++++++++-------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 0fe4944fdf48..e843bcb3ed81 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -719,6 +719,7 @@ 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. /// /// True if the Override is the immediate override method in the type hierarchy. /// False if Override is further removed (e.g. Override is a method on a derived type of a derived type of Base.DeclaringType) @@ -731,14 +732,12 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool is return false; } - var isInstantiated = Annotations.IsInstantiated (overrideInformation.Override.DeclaringType); - 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 + // 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 (!overrideInformation.Base.DeclaringType.IsInterface && isInstantiated) + if (Annotations.IsInstantiated (overrideInformation.Override.DeclaringType)) return true; // Direct overrides of marked abstract ov.Overrides must be marked or we get invalid IL. @@ -766,7 +765,11 @@ void MarkOverrideForBaseMethod (OverrideInformation overrideInformation) void MarkMethodIfNeededByBaseMethod (MethodDefinition method) { - if (!Annotations.IsMarked (method.DeclaringType) || Annotations.GetBaseMethods (method) is not List bases) + if (!Annotations.IsMarked (method.DeclaringType)) + return; + + var bases = Annotations.GetBaseMethods (method); + if (bases is null) return; foreach (var (ov, isDirectBase) in GetMarkedBaseMethods (bases)) { @@ -778,13 +781,15 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method) { foreach (var ov in overrides) { // Immediate base methods - if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)) + if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)){ yield return (ov, true); - - // Bases of bases - else if (Annotations.GetBaseMethods (ov.Base) is IEnumerable baseBases) { - foreach (var baseOv in GetMarkedBaseMethods (baseBases)) { - yield return (baseOv.Override, false); + } else { + // Bases of bases + var baseBases = Annotations.GetBaseMethods (ov.Base); + if (baseBases is not null) { + foreach (var baseOv in GetMarkedBaseMethods (baseBases)) { + yield return (baseOv.Override, false); + } } } } @@ -2399,10 +2404,6 @@ bool ShouldMarkInterfaceImplementation (TypeDefinition type, InterfaceImplementa if (resolvedInterfaceType.IsImport || resolvedInterfaceType.IsWindowsRuntime) return true; - if (resolvedInterfaceType.IsTypeOf ()) - return true; - - return IsFullyPreserved (type); } } From 077e336b19f11a6f7f3a181f34fdeb5847cf5c82 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 20 Oct 2022 16:22:34 -0700 Subject: [PATCH 24/34] format --- 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 e843bcb3ed81..8342a0c1f59b 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -781,7 +781,7 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method) { foreach (var ov in overrides) { // Immediate base methods - if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)){ + if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)) { yield return (ov, true); } else { // Bases of bases From 20ea8087871947f710a09e758012b8e0bd7af43c Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 20 Oct 2022 17:15:42 -0700 Subject: [PATCH 25/34] remove using --- src/linker/Linker.Steps/MarkStep.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 8342a0c1f59b..bb6f12ebf988 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -36,7 +36,6 @@ using System.Diagnostics.CodeAnalysis; using System.Linq; using System.Reflection.Runtime.TypeParsing; -using System.Runtime.InteropServices; using System.Text.RegularExpressions; using ILLink.Shared; using ILLink.Shared.TrimAnalysis; From 0de4d04bf9c6919b7499cd13f528f0d474d2ec74 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Fri, 21 Oct 2022 15:01:10 -0700 Subject: [PATCH 26/34] Don't ignore descriptors in test --- ...WithDynamicInterfaceCastableImplementationAttributeIsKept.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs b/test/Mono.Linker.Tests.Cases/Attributes/TypeWithDynamicInterfaceCastableImplementationAttributeIsKept.cs index febb4f26eb6b..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 () From 4cef7a73346188d5c6a877ae612cb8771ba1c71b Mon Sep 17 00:00:00 2001 From: Vitek Karas <10670590+vitek-karas@users.noreply.github.com> Date: Tue, 25 Oct 2022 10:42:21 -0700 Subject: [PATCH 27/34] Change analyzer versions such that the repo can be built with .NET 7 RC2 SDK (#3077) Change analyzer versions such that the repo can be built with .NET 7 RC2 SDK. - Took the versions from dotnet/runtime. - Remove CheckAttributeInstantiation method since is no longer necessary - Remove global attributes since they are no longer necessary - Workaround the fact that compiler injects System.Runtime.CompilerServices.RefSafetyRulesAttribute into every assembly to describe the language version Co-authored-by: tlakollo --- .../RequiresAnalyzerBase.cs | 32 ------------------- .../RequiresAssemblyFilesAnalyzerTests.cs | 2 +- .../RequiresDynamicCodeAnalyzerTests.cs | 2 +- .../RequiresUnreferencedCodeAnalyzerTests.cs | 2 +- ...nconditionalSuppressMessageCodeFixTests.cs | 2 +- .../TestCasesRunner/AssemblyChecker.cs | 10 ++++++ 6 files changed, 14 insertions(+), 36 deletions(-) diff --git a/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs b/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs index 893dbeeca4b2..2922c51ca2d7 100644 --- a/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs +++ b/src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs @@ -47,19 +47,11 @@ public override void Initialize (AnalysisContext context) if (methodSymbol.IsStaticConstructor () && methodSymbol.HasAttribute (RequiresAttributeName)) ReportRequiresOnStaticCtorDiagnostic (symbolAnalysisContext, methodSymbol); CheckMatchingAttributesInOverrides (symbolAnalysisContext, methodSymbol); - CheckAttributeInstantiation (symbolAnalysisContext, methodSymbol); - foreach (var typeParameter in methodSymbol.TypeParameters) - CheckAttributeInstantiation (symbolAnalysisContext, typeParameter); - }, SymbolKind.Method); context.RegisterSymbolAction (symbolAnalysisContext => { var typeSymbol = (INamedTypeSymbol) symbolAnalysisContext.Symbol; CheckMatchingAttributesInInterfaces (symbolAnalysisContext, typeSymbol); - CheckAttributeInstantiation (symbolAnalysisContext, typeSymbol); - foreach (var typeParameter in typeSymbol.TypeParameters) - CheckAttributeInstantiation (symbolAnalysisContext, typeParameter); - }, SymbolKind.NamedType); @@ -68,8 +60,6 @@ public override void Initialize (AnalysisContext context) if (AnalyzerDiagnosticTargets.HasFlag (DiagnosticTargets.Property)) { CheckMatchingAttributesInOverrides (symbolAnalysisContext, propertySymbol); } - - CheckAttributeInstantiation (symbolAnalysisContext, propertySymbol); }, SymbolKind.Property); context.RegisterSymbolAction (symbolAnalysisContext => { @@ -77,15 +67,8 @@ public override void Initialize (AnalysisContext context) if (AnalyzerDiagnosticTargets.HasFlag (DiagnosticTargets.Event)) { CheckMatchingAttributesInOverrides (symbolAnalysisContext, eventSymbol); } - - CheckAttributeInstantiation (symbolAnalysisContext, eventSymbol); }, SymbolKind.Event); - context.RegisterSymbolAction (symbolAnalysisContext => { - var fieldSymbol = (IFieldSymbol) symbolAnalysisContext.Symbol; - CheckAttributeInstantiation (symbolAnalysisContext, fieldSymbol); - }, SymbolKind.Field); - context.RegisterOperationAction (operationContext => { var methodInvocation = (IInvocationOperation) operationContext.Operation; CheckCalledMember (operationContext, methodInvocation.TargetMethod, incompatibleMembers); @@ -205,21 +188,6 @@ public override void Initialize (AnalysisContext context) foreach (var extraSymbolAction in ExtraSymbolActions) context.RegisterSymbolAction (extraSymbolAction.Action, extraSymbolAction.SymbolKind); - void CheckAttributeInstantiation ( - SymbolAnalysisContext symbolAnalysisContext, - ISymbol symbol) - { - if (symbol.IsInRequiresScope (RequiresAttributeName)) - return; - - foreach (var attr in symbol.GetAttributes ()) { - if (attr.AttributeConstructor?.DoesMemberRequire (RequiresAttributeName, out var requiresAttribute) == true) { - symbolAnalysisContext.ReportDiagnostic (Diagnostic.Create (RequiresDiagnosticRule, - symbol.Locations[0], attr.AttributeConstructor.GetDisplayName (), GetMessageFromAttribute (requiresAttribute), GetUrlFromAttribute (requiresAttribute))); - } - } - } - void CheckCalledMember ( OperationAnalysisContext operationContext, ISymbol member, diff --git a/test/ILLink.RoslynAnalyzer.Tests/RequiresAssemblyFilesAnalyzerTests.cs b/test/ILLink.RoslynAnalyzer.Tests/RequiresAssemblyFilesAnalyzerTests.cs index cdc94089f686..6d4db46a46a4 100644 --- a/test/ILLink.RoslynAnalyzer.Tests/RequiresAssemblyFilesAnalyzerTests.cs +++ b/test/ILLink.RoslynAnalyzer.Tests/RequiresAssemblyFilesAnalyzerTests.cs @@ -620,7 +620,7 @@ public class C [RequiresAssemblyFiles("Calls Wrapper()")] Action M2() { - [global::System.Diagnostics.CodeAnalysis.RequiresAssemblyFilesAttribute("Calls C.M1()")] void Wrapper () => M1(); + [RequiresAssemblyFiles("Calls C.M1()")] void Wrapper () => M1(); return Wrapper; } } diff --git a/test/ILLink.RoslynAnalyzer.Tests/RequiresDynamicCodeAnalyzerTests.cs b/test/ILLink.RoslynAnalyzer.Tests/RequiresDynamicCodeAnalyzerTests.cs index 2ed2be672a5c..ef8203401a8b 100644 --- a/test/ILLink.RoslynAnalyzer.Tests/RequiresDynamicCodeAnalyzerTests.cs +++ b/test/ILLink.RoslynAnalyzer.Tests/RequiresDynamicCodeAnalyzerTests.cs @@ -200,7 +200,7 @@ public class C [RequiresDynamicCode("Calls Wrapper()")] Action M2() { - [global::System.Diagnostics.CodeAnalysis.RequiresDynamicCodeAttribute("Calls C.M1()")] void Wrapper () => M1(); + [RequiresDynamicCode("Calls C.M1()")] void Wrapper () => M1(); return Wrapper; } } diff --git a/test/ILLink.RoslynAnalyzer.Tests/RequiresUnreferencedCodeAnalyzerTests.cs b/test/ILLink.RoslynAnalyzer.Tests/RequiresUnreferencedCodeAnalyzerTests.cs index 64dbdacfec35..12f839003297 100644 --- a/test/ILLink.RoslynAnalyzer.Tests/RequiresUnreferencedCodeAnalyzerTests.cs +++ b/test/ILLink.RoslynAnalyzer.Tests/RequiresUnreferencedCodeAnalyzerTests.cs @@ -236,7 +236,7 @@ public class C [RequiresUnreferencedCode("Calls Wrapper()")] Action M2() { - [global::System.Diagnostics.CodeAnalysis.RequiresUnreferencedCodeAttribute("Calls C.M1()")] void Wrapper () => M1(); + [RequiresUnreferencedCode("Calls C.M1()")] void Wrapper () => M1(); return Wrapper; } } diff --git a/test/ILLink.RoslynAnalyzer.Tests/UnconditionalSuppressMessageCodeFixTests.cs b/test/ILLink.RoslynAnalyzer.Tests/UnconditionalSuppressMessageCodeFixTests.cs index 4a9c2cf076ca..c59a2763fe4b 100644 --- a/test/ILLink.RoslynAnalyzer.Tests/UnconditionalSuppressMessageCodeFixTests.cs +++ b/test/ILLink.RoslynAnalyzer.Tests/UnconditionalSuppressMessageCodeFixTests.cs @@ -389,7 +389,7 @@ public class C Action M2() { - [global::System.Diagnostics.CodeAnalysis.UnconditionalSuppressMessageAttribute(""Trimming"", ""IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code"", Justification = """")] void Wrapper () => M1(); + [UnconditionalSuppressMessage(""Trimming"", ""IL2026:Members annotated with 'RequiresUnreferencedCodeAttribute' require dynamic access otherwise can break functionality when trimming application code"", Justification = """")] void Wrapper () => M1(); return Wrapper; } }"; diff --git a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs index 12f502b60f2b..41210172f6e4 100644 --- a/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs +++ b/test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs @@ -49,6 +49,11 @@ public void Verify () return s.FullName; }), StringComparer.Ordinal); + // Workaround for compiler injected attribute to describe the language version + linkedMembers.Remove ("System.Void Microsoft.CodeAnalysis.EmbeddedAttribute::.ctor()"); + linkedMembers.Remove ("System.Int32 System.Runtime.CompilerServices.RefSafetyRulesAttribute::Version"); + linkedMembers.Remove ("System.Void System.Runtime.CompilerServices.RefSafetyRulesAttribute::.ctor(System.Int32)"); + var membersToAssert = originalAssembly.MainModule.Types; foreach (var originalMember in membersToAssert) { if (originalMember is TypeDefinition td) { @@ -91,6 +96,10 @@ protected virtual void VerifyModule (ModuleDefinition original, ModuleDefinition protected virtual void VerifyTypeDefinition (TypeDefinition original, TypeDefinition linked) { + // Workaround for compiler injected attribute to describe the language version + verifiedGeneratedTypes.Add ("Microsoft.CodeAnalysis.EmbeddedAttribute"); + verifiedGeneratedTypes.Add ("System.Runtime.CompilerServices.RefSafetyRulesAttribute"); + if (linked != null && verifiedGeneratedTypes.Contains (linked.FullName)) return; @@ -839,6 +848,7 @@ protected virtual IEnumerable FilterLinkedAttributes (ICustomAttributePr case "System.Runtime.CompilerServices.RuntimeCompatibilityAttribute": case "System.Runtime.CompilerServices.CompilerGeneratedAttribute": case "System.Runtime.CompilerServices.IsReadOnlyAttribute": + case "System.Runtime.CompilerServices.RefSafetyRulesAttribute": continue; // When mcs is used to compile the test cases, backing fields end up with this attribute on them From c84326e3630c7686e3fbb85114ab5ee36d429dad Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 25 Oct 2022 12:08:13 -0700 Subject: [PATCH 28/34] Don't go through bases of bases when in GetMarkedBaseMethods --- src/linker/Linker.Steps/MarkStep.cs | 9 --------- 1 file changed, 9 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index bb6f12ebf988..f6d751a18ad5 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -779,17 +779,8 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method) IEnumerable<(OverrideInformation Override, bool IsDirectBase)> GetMarkedBaseMethods (IEnumerable overrides) { foreach (var ov in overrides) { - // Immediate base methods if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)) { yield return (ov, true); - } else { - // Bases of bases - var baseBases = Annotations.GetBaseMethods (ov.Base); - if (baseBases is not null) { - foreach (var baseOv in GetMarkedBaseMethods (baseBases)) { - yield return (baseOv.Override, false); - } - } } } } From 2bc88c03286a7665f41b1cf732fe35834e2e1f3a Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 25 Oct 2022 12:44:51 -0700 Subject: [PATCH 29/34] Remove GetInterfaceImplementors --- src/linker/Linker.Steps/MarkStep.cs | 10 ---------- src/linker/Linker/Annotations.cs | 8 -------- src/linker/Linker/TypeMapInfo.cs | 27 --------------------------- 3 files changed, 45 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index f6d751a18ad5..de7bc6bb0822 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -2029,16 +2029,6 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in if (type.IsInterface) { // There's no benefit to deferring processing of an interface type until we know a type implementing that interface is marked MarkRequirementsForInstantiatedTypes (type); - // Mark interface implementation on each implementor that requires interfaces. - if (Annotations.GetInterfaceImplementors (type) is List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)> implementors) { - foreach (var implementor in implementors) { - // Knowing that the interface type is marked, we need to mark the interface implementation if - // to make sure that the implementing type is instantiated or relevant to variant casting - if (Annotations.IsMarked (type) || Annotations.IsRelevantToVariantCasting (type)) { - MarkInterfaceImplementation (implementor.InterfaceImplementation); - } - } - } } else if (type.IsValueType) { // Note : Technically interfaces could be removed from value types in some of the same cases as reference types, however, it's harder to know when // a value type instance could exist. You'd have to track initobj and maybe locals types. Going to punt for now. diff --git a/src/linker/Linker/Annotations.cs b/src/linker/Linker/Annotations.cs index ebbf29a8f5af..4729ec773de8 100644 --- a/src/linker/Linker/Annotations.cs +++ b/src/linker/Linker/Annotations.cs @@ -461,14 +461,6 @@ public bool IsPublic (IMetadataTokenProvider provider) return TypeMapInfo.GetBaseMethods (method); } - /// - /// Returns a list of all known types that implement . The list may be incomplete if other implementing types exist in assemblies that haven't been processed by TypeMapInfo yet - /// - public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) - { - return TypeMapInfo.GetInterfaceImplementors (type); - } - public List? GetPreservedMethods (TypeDefinition type) { return GetPreservedMethods (type as IMemberDefinition); diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index b340ca5f7264..7bf8cacddbfa 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -29,7 +29,6 @@ // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. // -using System; using System.Collections.Generic; using Mono.Cecil; @@ -43,7 +42,6 @@ public class TypeMapInfo protected readonly Dictionary> base_methods = new Dictionary> (); protected readonly Dictionary> override_methods = new Dictionary> (); protected readonly Dictionary> default_interface_implementations = new Dictionary> (); - protected readonly Dictionary> interface_implementors = new (); public TypeMapInfo (LinkContext context) { @@ -119,23 +117,10 @@ public void AddDefaultInterfaceImplementation (MethodDefinition @base, TypeDefin implementations.Add ((implementingType, matchingInterfaceImplementation)); } - /// - /// Returns a list of all known types that implement . The list may be incomplete if other implementing types exist in assemblies that haven't been processed by TypeMapInfo yet - /// - public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition interfaceType) - { - if (!interfaceType.IsInterface) - throw new InvalidOperationException ("Cannot get interface implementors of a type that is not an interface"); - if (interface_implementors.TryGetValue (interfaceType, out var interfaces)) - return interfaces; - return null; - } - protected virtual void MapType (TypeDefinition type) { MapVirtualMethods (type); MapInterfaceMethodsInTypeHierarchy (type); - MapInterfaces (type); if (!type.HasNestedTypes) return; @@ -188,18 +173,6 @@ void MapInterfaceMethodsInTypeHierarchy (TypeDefinition type) } } - void MapInterfaces (TypeDefinition type) - { - foreach (var iface in type.GetAllInterfaceImplementations (context, true)) { - if (context.Resolve (iface.InterfaceType) is not TypeDefinition ifaceType) - continue; - if (!interface_implementors.TryGetValue (ifaceType, out var implementors)) { - implementors = new (); - } - implementors.Add ((type, iface)); - } - } - void MapVirtualMethods (TypeDefinition type) { if (!type.HasMethods) From fdc6bb316c1d6fa8686db91c8d9f5aa3dffbc6a7 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 25 Oct 2022 12:44:51 -0700 Subject: [PATCH 30/34] Remove GetInterfaceImplementors --- src/linker/Linker.Steps/MarkStep.cs | 10 ---------- src/linker/Linker/Annotations.cs | 8 -------- src/linker/Linker/TypeMapInfo.cs | 27 --------------------------- 3 files changed, 45 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index f6d751a18ad5..de7bc6bb0822 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -2029,16 +2029,6 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in if (type.IsInterface) { // There's no benefit to deferring processing of an interface type until we know a type implementing that interface is marked MarkRequirementsForInstantiatedTypes (type); - // Mark interface implementation on each implementor that requires interfaces. - if (Annotations.GetInterfaceImplementors (type) is List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)> implementors) { - foreach (var implementor in implementors) { - // Knowing that the interface type is marked, we need to mark the interface implementation if - // to make sure that the implementing type is instantiated or relevant to variant casting - if (Annotations.IsMarked (type) || Annotations.IsRelevantToVariantCasting (type)) { - MarkInterfaceImplementation (implementor.InterfaceImplementation); - } - } - } } else if (type.IsValueType) { // Note : Technically interfaces could be removed from value types in some of the same cases as reference types, however, it's harder to know when // a value type instance could exist. You'd have to track initobj and maybe locals types. Going to punt for now. diff --git a/src/linker/Linker/Annotations.cs b/src/linker/Linker/Annotations.cs index ebbf29a8f5af..4729ec773de8 100644 --- a/src/linker/Linker/Annotations.cs +++ b/src/linker/Linker/Annotations.cs @@ -461,14 +461,6 @@ public bool IsPublic (IMetadataTokenProvider provider) return TypeMapInfo.GetBaseMethods (method); } - /// - /// Returns a list of all known types that implement . The list may be incomplete if other implementing types exist in assemblies that haven't been processed by TypeMapInfo yet - /// - public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition type) - { - return TypeMapInfo.GetInterfaceImplementors (type); - } - public List? GetPreservedMethods (TypeDefinition type) { return GetPreservedMethods (type as IMemberDefinition); diff --git a/src/linker/Linker/TypeMapInfo.cs b/src/linker/Linker/TypeMapInfo.cs index b340ca5f7264..7bf8cacddbfa 100644 --- a/src/linker/Linker/TypeMapInfo.cs +++ b/src/linker/Linker/TypeMapInfo.cs @@ -29,7 +29,6 @@ // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. // -using System; using System.Collections.Generic; using Mono.Cecil; @@ -43,7 +42,6 @@ public class TypeMapInfo protected readonly Dictionary> base_methods = new Dictionary> (); protected readonly Dictionary> override_methods = new Dictionary> (); protected readonly Dictionary> default_interface_implementations = new Dictionary> (); - protected readonly Dictionary> interface_implementors = new (); public TypeMapInfo (LinkContext context) { @@ -119,23 +117,10 @@ public void AddDefaultInterfaceImplementation (MethodDefinition @base, TypeDefin implementations.Add ((implementingType, matchingInterfaceImplementation)); } - /// - /// Returns a list of all known types that implement . The list may be incomplete if other implementing types exist in assemblies that haven't been processed by TypeMapInfo yet - /// - public List<(TypeDefinition Implementor, InterfaceImplementation InterfaceImplementation)>? GetInterfaceImplementors (TypeDefinition interfaceType) - { - if (!interfaceType.IsInterface) - throw new InvalidOperationException ("Cannot get interface implementors of a type that is not an interface"); - if (interface_implementors.TryGetValue (interfaceType, out var interfaces)) - return interfaces; - return null; - } - protected virtual void MapType (TypeDefinition type) { MapVirtualMethods (type); MapInterfaceMethodsInTypeHierarchy (type); - MapInterfaces (type); if (!type.HasNestedTypes) return; @@ -188,18 +173,6 @@ void MapInterfaceMethodsInTypeHierarchy (TypeDefinition type) } } - void MapInterfaces (TypeDefinition type) - { - foreach (var iface in type.GetAllInterfaceImplementations (context, true)) { - if (context.Resolve (iface.InterfaceType) is not TypeDefinition ifaceType) - continue; - if (!interface_implementors.TryGetValue (ifaceType, out var implementors)) { - implementors = new (); - } - implementors.Add ((type, iface)); - } - } - void MapVirtualMethods (TypeDefinition type) { if (!type.HasMethods) From 0cf21d019fd00f704952085f2ed3006eddd091fd Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 25 Oct 2022 13:12:47 -0700 Subject: [PATCH 31/34] Remove leftover IsDirectBase code --- src/linker/Linker.Steps/MarkStep.cs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index de7bc6bb0822..c42bf0505b7e 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -724,7 +724,7 @@ void ProcessVirtualMethod (MethodDefinition method) /// False if Override is further removed (e.g. Override is a method on a derived type of a derived type of Base.DeclaringType) // TODO: Take into account a base method in preserved scope // TODO: Move interface method marking logic here - bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool isDirectBase) + bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation) { if (overrideInformation.IsOverrideOfInterfaceMember) { _interfaceOverrides.Add ((overrideInformation, ScopeStack.CurrentScope)); @@ -741,7 +741,7 @@ bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation, bool is // 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 && isDirectBase) + if (overrideInformation.Base.IsAbstract) return true; return false; @@ -771,18 +771,14 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method) if (bases is null) return; - foreach (var (ov, isDirectBase) in GetMarkedBaseMethods (bases)) { - if (ShouldMarkOverrideForBase (ov, isDirectBase)) + foreach (var ov in GetMarkedBaseMethods (bases)) { + if (ShouldMarkOverrideForBase (ov)) MarkOverrideForBaseMethod (ov); } - IEnumerable<(OverrideInformation Override, bool IsDirectBase)> GetMarkedBaseMethods (IEnumerable overrides) + IEnumerable GetMarkedBaseMethods (List overrides) { - foreach (var ov in overrides) { - if (Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)) { - yield return (ov, true); - } - } + return overrides.Where (ov => Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)); } } @@ -3205,7 +3201,7 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo if (Annotations.GetOverrides (method) is IEnumerable overrides) { foreach (var @override in overrides) { - if (ShouldMarkOverrideForBase (@override, true)) + if (ShouldMarkOverrideForBase (@override)) MarkOverrideForBaseMethod (@override); } } From 07c28b53a439d8fdaac3601badf8915b8473960b Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Tue, 25 Oct 2022 14:13:34 -0700 Subject: [PATCH 32/34] Remove call to MarkMethodIfNeededByBaseMethod from MarkAsRelevantToVariantCasting --- src/linker/Linker.Steps/MarkStep.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index c42bf0505b7e..799d9322261d 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -764,8 +764,7 @@ void MarkOverrideForBaseMethod (OverrideInformation overrideInformation) void MarkMethodIfNeededByBaseMethod (MethodDefinition method) { - if (!Annotations.IsMarked (method.DeclaringType)) - return; + Debug.Assert (Annotations.IsMarked (method.DeclaringType)); var bases = Annotations.GetBaseMethods (method); if (bases is null) @@ -3256,8 +3255,6 @@ void MarkAsRelevantToVariantCasting (TypeDefinition type) return; Annotations.MarkRelevantToVariantCasting (type); MarkInterfaceImplementations (type); - // Look at all methods on base types that may implement - MarkMethodsOnTypeIfNeededByBaseMethod (type); } void MarkMethodsOnTypeIfNeededByBaseMethod (TypeDefinition type) From 49d2e0ff70afc4258ffcdbd579715eecfc852619 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 27 Oct 2022 13:53:10 -0700 Subject: [PATCH 33/34] PR Feedback: inline methods and remove changes that are not longer relevant --- src/linker/Linker.Steps/MarkStep.cs | 38 ++++++++--------------------- 1 file changed, 10 insertions(+), 28 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 47c7db3348d2..1a2ef43d6116 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -722,8 +722,7 @@ void ProcessVirtualMethod (MethodDefinition method) /// /// True if the Override is the immediate override method in the type hierarchy. /// False if Override is further removed (e.g. Override is a method on a derived type of a derived type of Base.DeclaringType) - // TODO: Take into account a base method in preserved scope - // TODO: Move interface method marking logic here + // TODO: Move interface method marking logic here https://github.com/dotnet/linker/issues/3090 bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation) { if (overrideInformation.IsOverrideOfInterfaceMember) { @@ -770,15 +769,11 @@ void MarkMethodIfNeededByBaseMethod (MethodDefinition method) if (bases is null) return; - foreach (var ov in GetMarkedBaseMethods (bases)) { + var markedBaseMethods = bases.Where (ov => Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)); + foreach (var ov in markedBaseMethods) { if (ShouldMarkOverrideForBase (ov)) MarkOverrideForBaseMethod (ov); } - - IEnumerable GetMarkedBaseMethods (List overrides) - { - return overrides.Where (ov => Annotations.IsMarked (ov.Base) || IgnoreScope (ov.Base.DeclaringType.Scope)); - } } /// @@ -1861,7 +1856,7 @@ protected virtual void MarkSerializable (TypeDefinition type) // If a type is visible to reflection, we need to stop doing optimization that could cause observable difference // in reflection APIs. This includes APIs like MakeGenericType (where variant castability of the produced type // could be incorrect) or IsAssignableFrom (where assignability of unconstructed types might change). - MarkAsRelevantToVariantCasting (definition); + Annotations.MarkRelevantToVariantCasting (definition); Annotations.MarkReflectionUsed (definition); @@ -2711,7 +2706,7 @@ void MarkGenericArguments (IGenericInstance instance) if (argumentTypeDef == null) continue; - MarkAsRelevantToVariantCasting (argumentTypeDef); + Annotations.MarkRelevantToVariantCasting (argumentTypeDef); if (parameter.HasDefaultConstructorConstraint) MarkDefaultConstructor (argumentTypeDef, new DependencyInfo (DependencyKind.DefaultCtorForNewConstrainedGenericArgument, instance)); @@ -2920,7 +2915,7 @@ protected internal void MarkIndirectlyCalledMethod (MethodDefinition method, in MarkType (reference.DeclaringType, new DependencyInfo (DependencyKind.DeclaringType, reference)); if (reference.Name == ".ctor" && Context.TryResolve (arrayType) is TypeDefinition typeDefinition) { - MarkAsRelevantToVariantCasting (typeDefinition); + Annotations.MarkRelevantToVariantCasting (typeDefinition); } return null; } @@ -3253,21 +3248,6 @@ void MarkImplicitlyUsedFields (TypeDefinition type) MarkFields (type, includeStatic: type.IsEnum, reason: new DependencyInfo (DependencyKind.MemberOfType, type)); } - void MarkAsRelevantToVariantCasting (TypeDefinition type) - { - if (Annotations.IsRelevantToVariantCasting (type)) - return; - Annotations.MarkRelevantToVariantCasting (type); - MarkInterfaceImplementations (type); - } - - void MarkMethodsOnTypeIfNeededByBaseMethod (TypeDefinition type) - { - foreach (var method in type.Methods) { - MarkMethodIfNeededByBaseMethod (method); - } - } - protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type) { if (Annotations.IsInstantiated (type)) @@ -3280,7 +3260,9 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type MarkInterfaceImplementations (type); // Requires interface implementations to be marked first - MarkMethodsOnTypeIfNeededByBaseMethod (type); + foreach (var method in type.Methods) { + MarkMethodIfNeededByBaseMethod (method); + } MarkImplicitlyUsedFields (type); @@ -3689,7 +3671,7 @@ protected virtual void MarkInstruction (Instruction instruction, MethodDefinitio switch (instruction.OpCode.Code) { case Code.Newarr: if (Context.TryResolve (operand) is TypeDefinition typeDefinition) { - MarkAsRelevantToVariantCasting (typeDefinition); + Annotations.MarkRelevantToVariantCasting (typeDefinition); } break; case Code.Isinst: From 4c12f431be0ac723ee1a4567617651de65895e75 Mon Sep 17 00:00:00 2001 From: Jackson Schuster Date: Thu, 27 Oct 2022 14:58:14 -0700 Subject: [PATCH 34/34] Remove outdated comment --- src/linker/Linker.Steps/MarkStep.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/linker/Linker.Steps/MarkStep.cs b/src/linker/Linker.Steps/MarkStep.cs index 1a2ef43d6116..e8c29e29e075 100644 --- a/src/linker/Linker.Steps/MarkStep.cs +++ b/src/linker/Linker.Steps/MarkStep.cs @@ -720,8 +720,6 @@ void ProcessVirtualMethod (MethodDefinition method) /// Does not take into account if the base method is in a preserved scope. /// Assumes the base method is marked. /// - /// True if the Override is the immediate override method in the type hierarchy. - /// False if Override is further removed (e.g. Override is a method on a derived type of a derived type of Base.DeclaringType) // TODO: Move interface method marking logic here https://github.com/dotnet/linker/issues/3090 bool ShouldMarkOverrideForBase (OverrideInformation overrideInformation) {