Skip to content
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

Revert "Revert "Re-add static interface trimming with more testing"" #2859

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/removal-behavior.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ public class Program

### Method call on a constrained type parameter

On a call to a static abstract interface method that is accessed through a constrained type parameter, the interface method is rooted, as well as every implementation method on every type.
On a call to a static abstract interface method that is accessed through a constrained type parameter, the interface method is kept, as is every implementation method on every kept type.

Example:

Expand Down
60 changes: 50 additions & 10 deletions src/linker/Linker.Steps/MarkStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ protected LinkContext Context {

protected Queue<(MethodDefinition, DependencyInfo, MessageOrigin)> _methods;
protected List<(MethodDefinition, MarkScopeStack.Scope)> _virtual_methods;
protected List<(MethodDefinition, MarkScopeStack.Scope)> _static_interface_methods;
protected Queue<AttributeProviderPair> _assemblyLevelAttributes;
readonly List<AttributeProviderPair> _ivt_attributes;
protected Queue<(AttributeProviderPair, DependencyInfo, MarkScopeStack.Scope)> _lateMarkedAttributes;
Expand Down Expand Up @@ -224,6 +225,7 @@ public MarkStep ()
{
_methods = new Queue<(MethodDefinition, DependencyInfo, MessageOrigin)> ();
_virtual_methods = new List<(MethodDefinition, MarkScopeStack.Scope)> ();
_static_interface_methods = new List<(MethodDefinition, MarkScopeStack.Scope)> ();
_assemblyLevelAttributes = new Queue<AttributeProviderPair> ();
_ivt_attributes = new List<AttributeProviderPair> ();
_lateMarkedAttributes = new Queue<(AttributeProviderPair, DependencyInfo, MarkScopeStack.Scope)> ();
Expand Down Expand Up @@ -476,6 +478,7 @@ bool ProcessPrimaryQueue ()
while (!QueueIsEmpty ()) {
ProcessQueue ();
ProcessVirtualMethods ();
ProcessStaticInterfaceMethods ();
ProcessMarkedTypesWithInterfaces ();
ProcessDynamicCastableImplementationInterfaces ();
ProcessPendingBodies ();
Expand Down Expand Up @@ -576,6 +579,30 @@ void ProcessVirtualMethods ()
}
}

/// <summary>
/// Handles marking implementations of static interface methods and the interface implementations of types that implement a static interface method.
/// </summary>
void ProcessStaticInterfaceMethods ()
{
foreach ((MethodDefinition method, MarkScopeStack.Scope scope) in _static_interface_methods) {
using (ScopeStack.PushScope (scope)) {
var overrides = Annotations.GetOverrides (method);
if (overrides != null) {
foreach (OverrideInformation @override in overrides) {
ProcessOverride (@override);
// We need to mark the interface implementation for static interface methods
// Explicit interface method implementations already mark the interface implementation in ProcessMethod
MarkExplicitInterfaceImplementation (@override.Override, @override.Base);
}
}
}
}
}

/// <summary>
/// Does extra handling of marked types that have interfaces when it's necessary to know what types are marked or instantiated.
/// Right now it only marks the "implements interface" annotations and removes override annotations for static interface methods.
/// </summary>
void ProcessMarkedTypesWithInterfaces ()
{
// We may mark an interface type later on. Which means we need to reprocess any time with one or more interface implementations that have not been marked
Expand Down Expand Up @@ -693,6 +720,9 @@ void ProcessVirtualMethod (MethodDefinition method)
}
}

/// <summary>
/// Handles marking overriding methods if the type with the overriding method is instantiated or if the base method is a static abstract interface method
/// </summary>
void ProcessOverride (OverrideInformation overrideInformation)
{
var method = overrideInformation.Override;
Expand All @@ -708,12 +738,12 @@ void ProcessOverride (OverrideInformation overrideInformation)

var isInstantiated = Annotations.IsInstantiated (method.DeclaringType);

// We don't need to mark overrides until it is possible that the type could be instantiated
// We don't need to mark overrides until it is possible that the type could be instantiated or the method is a static interface method
// Note : The base type is interface check should be removed once we have base type sweeping
if (IsInterfaceOverrideThatDoesNotNeedMarked (overrideInformation, isInstantiated))
return;

// Interface static veitual methods will be abstract and will also by pass this check to get marked
// Interface static virtual methods will be abstract and will also bypass this check to get marked
if (!isInstantiated && !@base.IsAbstract && Context.IsOptimizationEnabled (CodeOptimizations.OverrideRemoval, method))
return;

Expand All @@ -736,8 +766,7 @@ bool IsInterfaceOverrideThatDoesNotNeedMarked (OverrideInformation overrideInfor
if (!overrideInformation.IsOverrideOfInterfaceMember || isInstantiated)
return false;

// This is a static interface method and these checks should all be true
if (overrideInformation.Override.IsStatic && overrideInformation.Base.IsStatic && overrideInformation.Base.IsAbstract && !overrideInformation.Override.IsVirtual)
if (overrideInformation.IsStaticInterfaceMethodPair)
return false;

if (overrideInformation.MatchingInterfaceImplementation != null)
Expand Down Expand Up @@ -3049,17 +3078,28 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
}
}

// Mark overridden methods and interface implementations except for static interface methods
// This will not mark implicit interface methods because they do not have a MethodImpl and aren't in the .Overrides
if (method.HasOverrides) {
foreach (MethodReference ov in method.Overrides) {
MarkMethod (ov, new DependencyInfo (DependencyKind.MethodImplOverride, method), ScopeStack.CurrentScope.Origin);
MarkExplicitInterfaceImplementation (method, ov);
foreach (MethodReference @base in method.Overrides) {
// Method implementing a static interface method will have an override to it - note nonstatic methods usually don't unless they're explicit.
// Calling the implementation method directly has no impact on the interface, and as such it should not mark the interface or its method.
// Only if the interface method is referenced, then all the methods which implemented must be kept, but not the other way round.
if (Context.Resolve (@base) is MethodDefinition baseDefinition
&& new OverrideInformation.OverridePair (baseDefinition, method).IsStaticInterfaceMethodPair ())
continue;
MarkMethod (@base, new DependencyInfo (DependencyKind.MethodImplOverride, method), ScopeStack.CurrentScope.Origin);
MarkExplicitInterfaceImplementation (method, @base);
}
}

MarkMethodSpecialCustomAttributes (method);
if (method.IsVirtual)
_virtual_methods.Add ((method, ScopeStack.CurrentScope));

if (method.IsStatic && method.IsAbstract && method.DeclaringType.IsInterface)
_static_interface_methods.Add ((method, ScopeStack.CurrentScope));

MarkNewCodeDependencies (method);

MarkBaseMethods (method);
Expand Down Expand Up @@ -3142,9 +3182,9 @@ protected virtual IEnumerable<MethodDefinition> GetRequiredMethodsForInstantiate
}
}

void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference ov)
void MarkExplicitInterfaceImplementation (MethodDefinition method, MethodReference overriddenMethod)
{
if (Context.Resolve (ov) is not MethodDefinition resolvedOverride)
if (Context.Resolve (overriddenMethod) is not MethodDefinition resolvedOverride)
return;

if (resolvedOverride.DeclaringType.IsInterface) {
Expand Down Expand Up @@ -3416,7 +3456,7 @@ void MarkInterfacesNeededByBodyStack (MethodBody body)
{
// If a type could be on the stack in the body and an interface it implements could be on the stack on the body
// then we need to mark that interface implementation. When this occurs it is not safe to remove the interface implementation from the type
// even if the type is never instantiated
// even if the type is never instantiated. (ex. `Type1 x = null; IFoo y = (IFoo)x;`)
var implementations = new InterfacesOnStackScanner (Context).GetReferencedInterfaces (body);
if (implementations == null)
return;
Expand Down
34 changes: 34 additions & 0 deletions src/linker/Linker.Steps/SweepStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,8 @@ protected virtual void SweepMethods (Collection<MethodDefinition> methods)

SweepCustomAttributes (method.MethodReturnType);

SweepOverrides (method);

if (!method.HasParameters)
continue;

Expand All @@ -467,6 +469,38 @@ protected virtual void SweepMethods (Collection<MethodDefinition> methods)
}
}

void SweepOverrides (MethodDefinition method)
{
for (int i = 0; i < method.Overrides.Count;) {
// We can't rely on the context resolution cache anymore, since it may remember methods which are already removed
// So call the direct Resolve here and avoid the cache.
// We want to remove a method from the list of Overrides if:
// Resolve() is null
// This can happen for a couple of reasons, but it indicates the method isn't in the final assembly.
// Resolve also may return a removed value if method.Overrides[i] is a MethodDefinition. In this case, Resolve short circuits and returns `this`.
// OR
// ov.DeclaringType is null
// ov.DeclaringType may be null if Resolve short circuited and returned a removed method. In this case, we want to remove the override.
// OR
// ov is in a `link` scope and is unmarked
// ShouldRemove returns true if the method is unmarked, but we also We need to make sure the override is in a link scope.
// Only things in a link scope are marked, so ShouldRemove is only valid for items in a `link` scope.
if (method.Overrides[i].Resolve () is not MethodDefinition ov || ov.DeclaringType is null || (IsLinkScope (ov.DeclaringType.Scope) && ShouldRemove (ov)))
method.Overrides.RemoveAt (i);
else
i++;
}
}

/// <summary>
/// Returns true if the assembly of the <paramref name="scope"></paramref> is set to link
/// </summary>
private bool IsLinkScope (IMetadataScope scope)
{
AssemblyDefinition? assembly = Context.Resolve (scope);
return assembly != null && Annotations.GetAction (assembly) == AssemblyAction.Link;
}

void SweepDebugInfo (Collection<MethodDefinition> methods)
{
List<ScopeDebugInformation>? sweptScopes = null;
Expand Down
3 changes: 3 additions & 0 deletions src/linker/Linker/Annotations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -436,6 +436,9 @@ public bool IsPublic (IMetadataTokenProvider provider)
return public_api.Contains (provider);
}

/// <summary>
/// Returns an IEnumerable of the methods that override this method. Note this is different than <see cref="MethodDefinition.Overrides"/>, which returns the MethodImpl's
/// </summary>
public IEnumerable<OverrideInformation>? GetOverrides (MethodDefinition method)
{
return TypeMapInfo.GetOverrides (method);
Expand Down
15 changes: 11 additions & 4 deletions src/linker/Linker/OverrideInformation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,22 @@ namespace Mono.Linker
public class OverrideInformation
{
readonly ITryResolveMetadata resolver;
readonly OverridePair _pair;

public OverrideInformation (MethodDefinition @base, MethodDefinition @override, ITryResolveMetadata resolver, InterfaceImplementation? matchingInterfaceImplementation = null)
{
Base = @base;
Override = @override;
_pair = new OverridePair (@base, @override);
MatchingInterfaceImplementation = matchingInterfaceImplementation;
this.resolver = resolver;
}

public MethodDefinition Base { get; }
public MethodDefinition Override { get; }
public readonly record struct OverridePair (MethodDefinition Base, MethodDefinition Override)
{
public bool IsStaticInterfaceMethodPair () => Base.DeclaringType.IsInterface && Base.IsStatic && Override.IsStatic;
}

public MethodDefinition Base { get => _pair.Base; }
public MethodDefinition Override { get => _pair.Override; }
public InterfaceImplementation? MatchingInterfaceImplementation { get; }

public bool IsOverrideOfInterfaceMember {
Expand All @@ -43,5 +48,7 @@ public TypeDefinition? InterfaceType {
return Base.DeclaringType;
}
}

public bool IsStaticInterfaceMethodPair => _pair.IsStaticInterfaceMethodPair ();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System.Threading.Tasks;
using Xunit;

namespace ILLink.RoslynAnalyzer.Tests.Inheritance.Interfaces
{
public sealed partial class StaticInterfaceMethodsTests : LinkerTestBase
{
protected override string TestSuiteName => "Inheritance.Interfaces.StaticInterfaceMethods";

[Fact]
public Task StaticAbstractInterfaceMethods ()
{
return RunTest (nameof (StaticAbstractInterfaceMethods));
}

[Fact]
public Task StaticAbstractInterfaceMethodsLibrary ()
{
return RunTest (nameof (StaticAbstractInterfaceMethodsLibrary));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -71,53 +71,51 @@ internal class ImplementsUnusedStaticInterface
// The interface methods themselves are not used, but the implementation of these methods is
internal interface IStaticInterfaceMethodUnused
{
// Can be removed with Static Interface trimming optimization
[Kept]
static abstract void InterfaceUsedMethodNot ();
}

// Can be removed with Static Interface Trimming
[Kept]
internal interface IStaticInterfaceUnused
{
// Can be removed with Static Interface Trimming
[Kept]
static abstract void InterfaceAndMethodNoUsed ();
}

[Kept]
[KeptInterface (typeof (IStaticInterfaceUnused))]
[KeptInterface (typeof (IStaticInterfaceMethodUnused))]
internal class InterfaceMethodUsedThroughImplementation : IStaticInterfaceMethodUnused, IStaticInterfaceUnused
{
[Kept]
[RemovedOverride (typeof (IStaticInterfaceMethodUnused))]
public static void InterfaceUsedMethodNot () { }

[Kept]
[RemovedOverride (typeof (IStaticInterfaceUnused))]
public static void InterfaceAndMethodNoUsed () { }
}

[Kept]
[KeptInterface (typeof (IStaticInterfaceMethodUnused))]
[KeptInterface (typeof (IStaticInterfaceUnused))]
internal class InterfaceMethodUnused : IStaticInterfaceMethodUnused, IStaticInterfaceUnused
{
[Kept]
public static void InterfaceUsedMethodNot () { }

[Kept]
public static void InterfaceAndMethodNoUsed () { }
}

[Kept]
// This method keeps InterfaceMethodUnused without making it 'relevant to variant casting' like
// doing a typeof or type argument would do. If the type is relevant to variant casting,
// we will keep all interface implementations for interfaces that are kept
internal static void KeepInterfaceMethodUnused (InterfaceMethodUnused x) { }

[Kept]
public static void Test ()
{
InterfaceMethodUsedThroughImplementation.InterfaceUsedMethodNot ();
InterfaceMethodUsedThroughImplementation.InterfaceAndMethodNoUsed ();

Type t;
t = typeof (IStaticInterfaceMethodUnused);
t = typeof (InterfaceMethodUnused);
// The interface has to be kept this way, because if both the type and the interface may
// appear on the stack then they would be marked as relevant to variant casting and the
// interface implementation would be kept.
Type t = typeof (IStaticInterfaceMethodUnused);
KeepInterfaceMethodUnused (null);
}
}

Expand Down
Loading