Skip to content
Merged
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 Directory.Build.props
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
</PropertyGroup>

<PropertyGroup>
<LangVersion>latest</LangVersion>
<LangVersion>preview</LangVersion>
<AnalysisLevel>latest</AnalysisLevel>
</PropertyGroup>
</Project>
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/ArrayValue.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
Expand Down
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/FieldValue.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// 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.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
Expand Down
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/GenericParameterValue.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// 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.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
Expand Down
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/MethodParameterValue.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// 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.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
Expand Down
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/MethodReturnValue.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// 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.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
Expand Down
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/MethodThisParameterValue.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// 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.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
Expand Down
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/RuntimeMethodHandleValue.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// 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 ILLink.Shared.DataFlow;
using Mono.Cecil;
Expand Down
4 changes: 2 additions & 2 deletions src/linker/Linker.Dataflow/SingleValueExtensions.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
Expand Down
55 changes: 41 additions & 14 deletions src/linker/Linker.Steps/MarkStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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 .interfaceimpl unconditionally. 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.

Copy link
Member

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.

continue;

using (ScopeStack.PushScope (scope))
using (ScopeStack.PushScope (scope)) {
MarkInterfaceImplementations (type);
}
}
}

Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The 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 MarkRequiermentsForInstantiatedTypes also works for static interfaces - and maybe we should rename that method then.

// 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
Expand All @@ -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));
Expand Down Expand Up @@ -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);
Expand All @@ -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))
Copy link
Member Author

@jtschuster jtschuster Apr 5, 2022

Choose a reason for hiding this comment

The 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
Expand All @@ -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)
return false;

Expand All @@ -2322,7 +2349,7 @@ bool IsVirtualNeededByInstantiatedTypeDueToPreservedScope (MethodDefinition meth
if (IgnoreScope (@base.DeclaringType.Scope))
return true;

if (IsVirtualNeededByTypeDueToPreservedScope (@base))
if (IsMethodNeededByTypeDueToPreservedScope (@base))
return true;
}

Expand Down Expand Up @@ -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;
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,11 @@ public Task InterfaceOnUninstantiatedTypeRemoved ()
return RunTest (allowMissingWarnings: true);
}

[Fact]
public Task InterfaceVariants ()
{
return RunTest (allowMissingWarnings: true);
}

}
}
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 ();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@

namespace Mono.Linker.Tests.Cases.Inheritance.Interfaces
{
[SetupLinkerArgument ("--disable-opt", "unusedinterfaces")]
public class InterfaceOnUninstantiatedTypeRemoved
{
public static void Main ()
Expand Down
Loading