-
Notifications
You must be signed in to change notification settings - Fork 128
Don't remove unused interfaces in library mode #2711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
0388e48
39ffb9f
e5b3788
61115f6
c4ebd45
d7cc881
3a8b76d
1c04ad2
be37029
7ad5afc
07143dd
a7c7e13
64692a0
4ea2991
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -584,11 +584,15 @@ void ProcessMarkedTypesWithInterfaces () | |
| foreach ((var type, var scope) in typesWithInterfaces) { | ||
| // Exception, types that have not been flagged as instantiated yet. These types may not need their interfaces even if the | ||
| // interface type is marked | ||
| if (!Annotations.IsInstantiated (type) && !Annotations.IsRelevantToVariantCasting (type)) | ||
| // UnusedInterfaces optimization is turned off mark all interface implementations | ||
| bool unusedInterfacesOptimizationEnabled = Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, type); | ||
| if (!Annotations.IsInstantiated (type) && !Annotations.IsRelevantToVariantCasting (type) && | ||
| unusedInterfacesOptimizationEnabled) | ||
| continue; | ||
|
|
||
| using (ScopeStack.PushScope (scope)) | ||
| using (ScopeStack.PushScope (scope)) { | ||
| MarkInterfaceImplementations (type); | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -1719,6 +1723,9 @@ void MarkField (FieldDefinition field, in DependencyInfo reason) | |
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Returns true if the assembly of the <paramref name="scope"></paramref> is not set to link (i.e. action=copy is set for that assembly) | ||
| /// </summary> | ||
| protected virtual bool IgnoreScope (IMetadataScope scope) | ||
| { | ||
| AssemblyDefinition? assembly = Context.Resolve (scope); | ||
|
|
@@ -1915,7 +1922,7 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in | |
| MarkGenericParameterProvider (type); | ||
|
|
||
| // There are a number of markings we can defer until later when we know it's possible a reference type could be instantiated | ||
| // For example, if no instance of a type exist, then we don't need to mark the interfaces on that type | ||
| // For example, if no instance of a type exist, then we don't need to mark the interfaces on that type -- Note this is not true for static interfaces | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. While the added comment is correct, I think it makes this whole section confusing. Maybe you should add that the |
||
| // However, for some other types there is no benefit to deferring | ||
| if (type.IsInterface) { | ||
| // There's no benefit to deferring processing of an interface type until we know a type implementing that interface is marked | ||
|
|
@@ -1939,12 +1946,13 @@ internal void MarkStaticConstructorVisibleToReflection (TypeDefinition type, in | |
| MarkRequirementsForInstantiatedTypes (type); | ||
| } | ||
|
|
||
| // Save for later once we know which interfaces are marked and then determine which interface implementations and methods to keep | ||
| if (type.HasInterfaces) | ||
| _typesWithInterfaces.Add ((type, ScopeStack.CurrentScope)); | ||
|
|
||
| if (type.HasMethods) { | ||
| // For virtuals that must be preserved, blame the declaring type. | ||
| MarkMethodsIf (type.Methods, IsVirtualNeededByTypeDueToPreservedScope, new DependencyInfo (DependencyKind.VirtualNeededDueToPreservedScope, type)); | ||
| // For methods that must be preserved, blame the declaring type. | ||
| MarkMethodsIf (type.Methods, IsMethodNeededByTypeDueToPreservedScope, new DependencyInfo (DependencyKind.VirtualNeededDueToPreservedScope, type)); | ||
| if (ShouldMarkTypeStaticConstructor (type) && reason.Kind != DependencyKind.TriggersCctorForCalledMethod) { | ||
| using (ScopeStack.PopToParent ()) | ||
| MarkStaticConstructor (type, new DependencyInfo (DependencyKind.CctorForType, type)); | ||
|
|
@@ -2278,9 +2286,19 @@ void MarkGenericParameter (GenericParameter parameter) | |
| } | ||
| } | ||
|
|
||
| bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method) | ||
| { | ||
| if (!method.IsVirtual) | ||
| /// <summary> | ||
| /// Returns true if any of the base methods of the <paramref name="method"/> passed is in an assembly that is not trimmed (i.e. action != trim). | ||
| /// Meant to be used to determine whether methods should be marked regardless of whether it is instantiated or not. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// When the unusedinterfaces optimization is on, this is used to mark methods that override an abstract method from a non-link assembly and must be kept. | ||
| /// When the unusedinterfaces optimization is off, this will do the same as when on but will also mark interface methods from interfaces defined in a non-link assembly. | ||
| /// If the containing type is instantiated, the caller should also use <see cref="IsMethodNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition)" /> | ||
| /// </remarks> | ||
| bool IsMethodNeededByTypeDueToPreservedScope (MethodDefinition method) | ||
| { | ||
| // Static methods may also have base methods in static interface methods. These methods are not captured by IsVirtual and must be checked separately | ||
| if (!(method.IsVirtual || method.IsStatic)) | ||
| return false; | ||
|
|
||
| var base_list = Annotations.GetBaseMethods (method); | ||
|
|
@@ -2289,8 +2307,8 @@ bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method) | |
|
|
||
| foreach (MethodDefinition @base in base_list) { | ||
| // Just because the type is marked does not mean we need interface methods. | ||
| // if the type is never instantiated, interfaces will be removed | ||
| if (@base.DeclaringType.IsInterface) | ||
| // if the type is never instantiated, interfaces will be removed - but only if the optimization is enabled | ||
| if (@base.DeclaringType.IsInterface && Context.IsOptimizationEnabled (CodeOptimizations.UnusedInterfaces, method.DeclaringType)) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check here eliminates the necessity for marking methods in IsVirtualMethodNeededByInstantiatedTypeDueToPreservedScope. Since we don't exit early here when the optimization is disabled, we mark methods where the interface comes from an IgnoreScope assembly. |
||
| continue; | ||
|
|
||
| // If the type is marked, we need to keep overrides of abstract members defined in assemblies | ||
|
|
@@ -2302,15 +2320,24 @@ bool IsVirtualNeededByTypeDueToPreservedScope (MethodDefinition method) | |
| if (IgnoreScope (@base.DeclaringType.Scope)) | ||
| return true; | ||
|
|
||
| if (IsVirtualNeededByTypeDueToPreservedScope (@base)) | ||
| if (IsMethodNeededByTypeDueToPreservedScope (@base)) | ||
| return true; | ||
| } | ||
|
|
||
| return false; | ||
| } | ||
|
|
||
| bool IsVirtualNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition method) | ||
| /// <summary> | ||
| /// Returns true if any of the base methods of <paramref name="method" /> is defined in an assembly that is not trimmed (i.e. action!=trim). | ||
| /// This is meant to be used on methods from a type that is known to be instantiated. | ||
| /// </summary> | ||
| /// <remarks> | ||
| /// This is very similar to <see cref="IsMethodNeededByTypeDueToPreservedScope (MethodDefinition)"/>, | ||
| /// but will mark methods from an interface defined in a non-link assembly regardless of the optimization, and does not handle static interface methods. | ||
| /// </remarks> | ||
| bool IsMethodNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition method) | ||
| { | ||
| // Any static interface methods are captured by <see cref="IsVirtualNeededByTypeDueToPreservedScope">, which should be called on all relevant methods so no need to check again here. | ||
| if (!method.IsVirtual) | ||
vitek-karas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return false; | ||
|
|
||
|
|
@@ -2322,7 +2349,7 @@ bool IsVirtualNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition meth | |
| if (IgnoreScope (@base.DeclaringType.Scope)) | ||
| return true; | ||
|
|
||
| if (IsVirtualNeededByTypeDueToPreservedScope (@base)) | ||
| if (IsMethodNeededByTypeDueToPreservedScope (@base)) | ||
| return true; | ||
| } | ||
|
|
||
|
|
@@ -3110,7 +3137,7 @@ protected virtual void MarkRequirementsForInstantiatedTypes (TypeDefinition type | |
| protected virtual IEnumerable<MethodDefinition> GetRequiredMethodsForInstantiatedType (TypeDefinition type) | ||
| { | ||
| foreach (var method in type.Methods) { | ||
| if (IsVirtualNeededByInstantiatedTypeDueToPreservedScope (method)) | ||
| if (IsMethodNeededByInstantiatedTypeDueToPreservedScope (method)) | ||
| yield return method; | ||
| } | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| // Copyright (c) .NET Foundation and contributors. All rights reserved. | ||
| // Licensed under the MIT license. See LICENSE file in the project root for full license information. | ||
|
|
||
| namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces.Dependencies | ||
| { | ||
| public interface ICopyLibraryInterface | ||
| { | ||
| void CopyLibraryInterfaceMethod (); | ||
| void CopyLibraryExplicitImplementationInterfaceMethod (); | ||
| } | ||
|
|
||
| public interface ICopyLibraryStaticInterface | ||
| { | ||
| static abstract void CopyLibraryStaticInterfaceMethod (); | ||
| static abstract void CopyLibraryExplicitImplementationStaticInterfaceMethod (); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like this is marking the
.interfaceimplunconditionally. Is that what we want? It seems like it could result in keeping the impl for private interfaces, which isn't strictly necessary for library mode - it should be ok to remove impls of unused private interfaces.If we want to keep all impls for simplicity, it's worth clarifying whether the intention is to keep all interface methods as well. My gut reaction is that it would be confusing to keep all impls but still allow trimming unused interface methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right - this didn't consider private interfaces.