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
10 changes: 9 additions & 1 deletion src/linker/Linker.Steps/MarkStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3166,6 +3166,7 @@ protected virtual void ProcessMethod (MethodDefinition method, in DependencyInfo
}

MarkMethodSpecialCustomAttributes (method);

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

Expand Down Expand Up @@ -3345,8 +3346,15 @@ void MarkBaseMethods (MethodDefinition method)
return;

foreach (MethodDefinition base_method in base_methods) {
if (base_method.DeclaringType.IsInterface && !method.DeclaringType.IsInterface)
// 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) {
// These are all virtual, no need to check IsVirtual before adding to list
_virtual_methods.Add ((base_method, ScopeStack.CurrentScope));
// _virtual_methods is a list and might have duplicates, but it's mostly just used for override validation, so it shouldn't matter
continue;
}

MarkMethod (base_method, new DependencyInfo (DependencyKind.BaseMethod, method), ScopeStack.CurrentScope.Origin);
MarkBaseMethods (base_method);
Expand Down
18 changes: 17 additions & 1 deletion src/linker/Linker/Annotations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -608,7 +608,7 @@ internal bool IsInRequiresUnreferencedCodeScope (MethodDefinition method)
/// <summary>
/// Determines if a member requires unreferenced code (and thus any usage of such method should be warned about).
/// </summary>
/// <remarks>Unlike <see cref="IsInRequiresUnreferencedCodeScope(MethodDefinition)"/> only static methods
/// <remarks>Unlike <see cref="IsInRequiresUnreferencedCodeScope(MethodDefinition)"/> only static methods
/// and .ctors are reported as requiring unreferenced code when the declaring type has RUC on it.</remarks>
internal bool DoesMemberRequireUnreferencedCode (IMemberDefinition member, [NotNullWhen (returnValue: true)] out RequiresUnreferencedCodeAttribute? attribute)
{
Expand Down Expand Up @@ -673,11 +673,27 @@ internal bool DoesFieldRequireUnreferencedCode (FieldDefinition field, [NotNullW
return TryGetLinkerAttribute (field.DeclaringType, out attribute);
}

/// <Summary>
/// Adds a virtual method to the queue if it is annotated and must have matching annotations on its bases and overrides. It does not check if the method is marked before producing a warning about mismatched annotations.
/// </summary>
public void EnqueueVirtualMethod (MethodDefinition method)
{
if (!method.IsVirtual)
return;

// Implementations of static interface methods are not virtual and won't reach here
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are tracking implementations of static interface methods now in _virtual_methods, so could we adjust the check above (if (!method.IsVirtual)) to allow these through instead?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually removed the implementations of static interface methods from _virtual_methods, so it should only be methods that are actually 'virtual' in IL in the list. Do you think it's worth it to add both the base and the implementations?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

@jtschuster jtschuster Aug 3, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding them in MarkBaseMethods is necessary for static methods in a preserved scope. Otherwise we miss annotation checks and marking the implementations for "Base is in a preserved scope".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - would you mind adding a testcase for that? The tests pass for me without that check.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would make sense to merge your other PR first, then maybe this can be simplified since we are already checking for interface methods in preserved scopes there: https://github.com/dotnet/linker/pull/2868/files#diff-f3ab7d627296ba105613b9cc039ca6f4ddc7a9aa66c6060ca82f6456ae0ede4fR612

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added some tests here that fail when the line is removed. I also found that instance interface methods aren't checked for matching annotations if the base in a preserved scope, so I removed the IsStatic check. This may warn if the interface implementation is removed or if the static interface methodImpl is removed, but I think it makes sense to still warn there.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is some small risk that comes with warning even if the interface impl is removed, but the analyzer should in theory warn about it either way so I am fine with this. Please add a note that this is the case. cc @vitek-karas in case you have opinions on this.

// We'll search through the implementations of static interface methods to find if any need to be enqueued
if (method.IsStatic) {
Debug.Assert (method.DeclaringType.IsInterface);
var overrides = GetOverrides (method);
if (overrides is not null) {
foreach (var @override in overrides) {
if (FlowAnnotations.RequiresVirtualMethodDataFlowAnalysis (@override.Override) || HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (@override.Override))
VirtualMethodsWithAnnotationsToValidate.Add (@override.Override);
}
}
}

if (FlowAnnotations.RequiresVirtualMethodDataFlowAnalysis (method) || HasLinkerAttribute<RequiresUnreferencedCodeAttribute> (method))
VirtualMethodsWithAnnotationsToValidate.Add (method);
}
Expand Down
6 changes: 6 additions & 0 deletions test/ILLink.RoslynAnalyzer.Tests/StaticInterfaceMethods.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ public sealed partial class StaticInterfaceMethodsTests : LinkerTestBase
{
protected override string TestSuiteName => "Inheritance.Interfaces.StaticInterfaceMethods";

[Fact]
public Task BaseProvidesInterfaceMethod ()
{
return RunTest (allowMissingWarnings: false);
}

[Fact]
public Task StaticAbstractInterfaceMethods ()
{
Expand Down
66 changes: 66 additions & 0 deletions test/Mono.Linker.Tests.Cases/DataFlow/Dependencies/Library.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
// Copyright (c) .NET Foundation and contributors. All rights reserved.
// Licensed under the MIT license. See LICENSE file in the project root for full license information.

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace Mono.Linker.Tests.Cases.DataFlow.Dependencies
{
public class Library
{
public interface IAnnotatedMethods
{
static abstract void GenericWithMethodsStatic<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> ();

static abstract void ParamWithMethodsStatic ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t);

[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
static abstract Type ReturnWithMethodsStatic ();

void GenericWithMethods<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> ();

void ParamWithMethods ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t);

[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
Type ReturnWithMethods ();
}

public interface IUnannotatedMethods
{
static abstract void GenericStatic<T> ();

static abstract void ParamStatic (Type t);

static abstract Type ReturnStatic ();

void Generic<T> ();

void Param (Type t);

Type Return ();
}

public abstract class AnnotatedMethods
{
public abstract void GenericWithMethods<[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] T> ();

public abstract void ParamWithMethods ([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)] Type t);

[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
public abstract Type ReturnWithMethods ();
}

public abstract class UnannotatedMethods
{
public abstract void Generic<T> ();

public abstract void Param (Type t);

public abstract Type Return ();
}
}
}
Loading