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

Add KeptByAttribute to validate an item was kept due to a specific dependency #3096

Merged
merged 8 commits into from
Nov 7, 2022
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
// 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;

namespace Mono.Linker.Tests.Cases.Expectations.Assertions
{
[AttributeUsage (AttributeTargets.All, Inherited = false)]
public class KeptByAttribute : KeptAttribute
{
private KeptByAttribute () { }

/// <summary>
/// Place on an type member to indicate that the linker should log that the member is kept as a depenendency of <paramref name="dependencyProvider"/> with reason <paramref name="reason"/>.
/// </summary>
/// <param name="dependencyProvider">Cecil's FullName property of the item that provides the dependency that keeps the item</param>
/// <param name="reason">The string representation of the DependencyKind that is recorded as the reason for the dependency</param>
public KeptByAttribute (string dependencyProvider, string reason) { }

/// <summary>
/// Place on an type member to indicate that the linker should log that the member is kept as a depenendency of <paramref name="dependencyProviderType"/> with reason <paramref name="reason"/>.
/// </summary>
/// <param name="dependencyProviderType">The type that is providing the dependency that keeps the item</param>
/// <param name="reason">The string representation of the DependencyKind that is recorded as the reason for the dependency</param>
public KeptByAttribute (Type dependencyProviderType, string reason) { }

/// <summary>
/// Place on an type member to indicate that the linker should log that the member is kept as a depenendency of <paramref name="dependencyProviderType"/>.<paramref name="memberName"/> with reason <paramref name="reason"/>.
/// </summary>
/// <param name="dependencyProviderType">The declaring type of the member that is providing the dependency that keeps the item</param>
/// <param name="memberName">Cecil's 'Name' property of the member that provides the dependency that keeps the item</param>
/// <param name="reason">The string representation of the DependencyKind that is recorded as the reason for the dependency</param>
public KeptByAttribute (Type dependencyProviderType, string memberName, string reason) { }
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ public virtual void Foo ()
[KeptBaseType (typeof (B))]
class A : B
{
[Kept]
[KeptBy (typeof (A), "OverrideOnInstantiatedType")]
public override void Foo ()
{
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ public static void Main ()
[KeptMember (".ctor()")]
class B
{
[Kept] // TODO: Would be nice to be removed
// TODO: Would be nice to be removed
jtschuster marked this conversation as resolved.
Show resolved Hide resolved
// https://github.com/dotnet/linker/issues/3097
[KeptBy (typeof (A), nameof (A.Foo), "BaseMethod")]
public virtual void Foo ()
{
}
Expand All @@ -22,7 +24,10 @@ public virtual void Foo ()
[KeptBaseType (typeof (B))]
class A : B
{
[Kept]
// Bug: https://github.com/dotnet/linker/issues/3078
Copy link
Member

Choose a reason for hiding this comment

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

What's the bug for this one - just that it isn't treated as a direct call? It might be worth a comment.

// Linker should mark for DirectCall as well as OverrideOnInstantiatedType, not just OverrideOnInstantiatedType
//[KeptBy (typeof(A), nameof(Foo), DependencyKind.DirectCall)]
[KeptBy (typeof (A), "OverrideOnInstantiatedType")]
public override void Foo ()
{
}
Expand Down
76 changes: 74 additions & 2 deletions test/Mono.Linker.Tests/TestCasesRunner/AssemblyChecker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,19 @@ namespace Mono.Linker.Tests.TestCasesRunner
public class AssemblyChecker
{
readonly AssemblyDefinition originalAssembly, linkedAssembly;
readonly LinkedTestCaseResult linkedTestCase;

HashSet<string> linkedMembers;
readonly HashSet<string> verifiedGeneratedFields = new HashSet<string> ();
readonly HashSet<string> verifiedEventMethods = new HashSet<string> ();
readonly HashSet<string> verifiedGeneratedTypes = new HashSet<string> ();
bool checkNames;

public AssemblyChecker (AssemblyDefinition original, AssemblyDefinition linked)
public AssemblyChecker (AssemblyDefinition original, AssemblyDefinition linked, LinkedTestCaseResult linkedTestCase)
{
this.originalAssembly = original;
this.linkedAssembly = linked;
this.linkedTestCase = linkedTestCase;

checkNames = original.MainModule.GetTypeReferences ().Any (attr =>
attr.Name == nameof (RemovedNameValueAttribute));
Expand All @@ -44,6 +46,7 @@ public void Verify ()

VerifyResources (originalAssembly, linkedAssembly);
VerifyReferences (originalAssembly, linkedAssembly);
VerifyKeptByAttributes (originalAssembly, originalAssembly.FullName);

linkedMembers = new HashSet<string> (linkedAssembly.MainModule.AllMembers ().Select (s => {
return s.FullName;
Expand Down Expand Up @@ -139,11 +142,76 @@ protected virtual void VerifyTypeDefinition (TypeDefinition original, TypeDefini
}
}

/// <summary>
/// Validates that all <see cref="KeptByAttribute"/> instances on a member are valid (i.e. the linker recorded a marked dependency described in the attribute)
/// </summary>
void VerifyKeptByAttributes (IMemberDefinition src, IMemberDefinition linked)
{
foreach (var keptByAttribute in src.CustomAttributes.Where (ca => ca.AttributeType.IsTypeOf<KeptByAttribute> ()))
VerifyKeptByAttribute (linked.FullName, keptByAttribute);
}

/// <summary>
/// Validates that all <see cref="KeptByAttribute"/> instances on an attribute provider are valid (i.e. the linker recorded a marked dependency described in the attribute)
/// <paramref name="src"/> is the attribute provider that may have a <see cref="KeptByAttribute"/>, and <paramref name="attributeProviderFullName"/> is the 'FullName' of <paramref name="src"/>.
/// </summary>
void VerifyKeptByAttributes (ICustomAttributeProvider src, string attributeProviderFullName)
{
foreach (var keptByAttribute in src.CustomAttributes.Where (ca => ca.AttributeType.IsTypeOf<KeptByAttribute> ()))
VerifyKeptByAttribute (attributeProviderFullName, keptByAttribute);
}

void VerifyKeptByAttribute (string keptAttributeProviderName, CustomAttribute attribute)
{
// public KeptByAttribute (string dependencyProvider, string reason) { }
// public KeptByAttribute (Type dependencyProvider, string reason) { }
// public KeptByAttribute (Type dependencyProvider, string memberName, string reason) { }

Assert.AreEqual (nameof (KeptByAttribute), attribute.AttributeType.Name);

// Create the expected TestDependencyRecorder.Dependency that should be in the recorded dependencies
TestDependencyRecorder.Dependency expectedDependency = new ();
expectedDependency.Target = keptAttributeProviderName;
expectedDependency.Marked = true;
if (attribute.ConstructorArguments.Count == 2) {
// public KeptByAttribute (string dependencyProvider, string reason) { }
// public KeptByAttribute (Type dependencyProvider, string reason) { }
if (attribute.ConstructorArguments[0].Type.IsTypeOf<string> ())
expectedDependency.Source = (string) attribute.ConstructorArguments[0].Value;
else if (attribute.ConstructorArguments[0].Type.IsTypeOf<Type> ())
expectedDependency.Source = ((TypeDefinition) attribute.ConstructorArguments[0].Value).FullName;
else
throw new NotImplementedException ("Unexpected KeptByAttribute ctor variant");

expectedDependency.DependencyKind = (string) attribute.ConstructorArguments[1].Value;
} else if (attribute.ConstructorArguments.Count == 3) {
// public KeptByAttribute (Type dependencyProvider, string memberName, string reason) { }
if (!attribute.ConstructorArguments[0].Type.IsTypeOf<Type> ())
throw new NotImplementedException ("Unexpected KeptByAttribute ctor variant");
var type = (TypeDefinition) attribute.ConstructorArguments[0].Value;
string memberName = (string) attribute.ConstructorArguments[1].Value;
var memberDefinition = type.AllMembers ().Where (m => m.Name == memberName).Single ();
expectedDependency.Source = memberDefinition.FullName;
expectedDependency.DependencyKind = (string) attribute.ConstructorArguments[2].Value;
} else {
throw new NotImplementedException ("Unexpected KeptByAttribute ctor variant");
}

foreach (var dep in this.linkedTestCase.Customizations.DependencyRecorder.Dependencies) {
if (dep == expectedDependency) {
return;
}
}
string errorMessage = $"{keptAttributeProviderName} was expected to be kept by {expectedDependency.Source} with reason {expectedDependency.DependencyKind.ToString ()}.";
Assert.Fail (errorMessage);
}

protected virtual void VerifyTypeDefinitionKept (TypeDefinition original, TypeDefinition linked)
{
if (linked == null)
Assert.Fail ($"Type `{original}' should have been kept");

VerifyKeptByAttributes (original, linked);
if (!original.IsInterface)
VerifyBaseType (original, linked);

Expand Down Expand Up @@ -313,6 +381,7 @@ void VerifyFieldKept (FieldDefinition src, FieldDefinition linked)

Assert.AreEqual (src?.Constant, linked?.Constant, $"Field `{src}' value");

VerifyKeptByAttributes (src, linked);
VerifyPseudoAttributes (src, linked);
VerifyCustomAttributes (src, linked);
}
Expand All @@ -335,6 +404,7 @@ void VerifyProperty (PropertyDefinition src, PropertyDefinition linked, TypeDefi

Assert.AreEqual (src?.Constant, linked?.Constant, $"Property `{src}' value");

VerifyKeptByAttributes (src, linked);
VerifyPseudoAttributes (src, linked);
VerifyCustomAttributes (src, linked);
}
Expand Down Expand Up @@ -367,6 +437,7 @@ void VerifyEvent (EventDefinition src, EventDefinition linked, TypeDefinition li
linkedMembers.Remove (src.RemoveMethod.FullName);
}

VerifyKeptByAttributes (src, linked);
VerifyPseudoAttributes (src, linked);
VerifyCustomAttributes (src, linked);
}
Expand Down Expand Up @@ -430,6 +501,7 @@ protected virtual void VerifyMethodKept (MethodDefinition src, MethodDefinition
VerifySecurityAttributes (src, linked);
VerifyArrayInitializers (src, linked);
VerifyMethodBody (src, linked);
VerifyKeptByAttributes (src, linked);
}

protected virtual void VerifyMethodBody (MethodDefinition src, MethodDefinition linked)
Expand Down Expand Up @@ -989,7 +1061,7 @@ protected virtual bool ShouldMethodBeKept (MethodDefinition method)

protected virtual bool ShouldBeKept<T> (T member, string signature = null) where T : MemberReference, ICustomAttributeProvider
{
if (member.HasAttribute (nameof (KeptAttribute)))
if (member.HasAttribute (nameof (KeptAttribute)) || member.HasAttribute (nameof (KeptByAttribute)))
return true;

ICustomAttributeProvider cap = (ICustomAttributeProvider) member.DeclaringType;
Expand Down
6 changes: 3 additions & 3 deletions test/Mono.Linker.Tests/TestCasesRunner/ResultChecker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ public virtual void Check (LinkedTestCaseResult linkResult)
PerformOutputSymbolChecks (original, linkResult.OutputAssemblyPath.Parent);

if (!HasAttribute (original.MainModule.GetType (linkResult.TestCase.ReconstructedFullTypeName), nameof (SkipKeptItemsValidationAttribute))) {
CreateAssemblyChecker (original, linked).Verify ();
CreateAssemblyChecker (original, linked, linkResult).Verify ();
}
}

Expand All @@ -106,9 +106,9 @@ public virtual void Check (LinkedTestCaseResult linkResult)
}
}

protected virtual AssemblyChecker CreateAssemblyChecker (AssemblyDefinition original, AssemblyDefinition linked)
protected virtual AssemblyChecker CreateAssemblyChecker (AssemblyDefinition original, AssemblyDefinition linked, LinkedTestCaseResult linkedTestCase)
{
return new AssemblyChecker (original, linked);
return new AssemblyChecker (original, linked, linkedTestCase);
}

void InitializeResolvers (LinkedTestCaseResult linkedResult)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -89,8 +89,10 @@ public virtual TestCaseLinkerOptions GetLinkerOptions (NPath inputPath)

public virtual void CustomizeLinker (LinkerDriver linker, LinkerCustomizations customizations)
{
if (_testCaseTypeDefinition.CustomAttributes.Any (attr =>
attr.AttributeType.Name == nameof (DependencyRecordedAttribute))) {
if (!_testCaseTypeDefinition.CustomAttributes.Any (a => a.AttributeType.IsTypeOf<SkipKeptItemsValidationAttribute> ())
Copy link
Member

Choose a reason for hiding this comment

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

Why the check for SkipKeptItemsValidation - isn't it ok to not record dependencies if there are only [Kept] attributes for example?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since the SkipKeptItemsValidation is only on the entry point class, and KeptBy attributes can be anywhere in the assembly, I thought it would be easier, less error prone, and not significantly slower to assume we need a tracer if there's any kept item validation rather than checking every node in the program for a KeptByAttribute and then deciding to use a tracer or not.

|| _testCaseTypeDefinition.CustomAttributes.Any (attr =>
attr.AttributeType.Name == nameof (DependencyRecordedAttribute)
|| attr.AttributeType.Name == nameof (KeptByAttribute))) {
customizations.DependencyRecorder = new TestDependencyRecorder ();
customizations.CustomizeContext += context => {
context.Tracer.AddRecorder (customizations.DependencyRecorder);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,12 @@ namespace Mono.Linker.Tests.TestCasesRunner
{
public class TestDependencyRecorder : IDependencyRecorder
{
public struct Dependency
public record struct Dependency
{
public string Source;
public string Target;
public bool Marked;
public string DependencyKind;
}

public List<Dependency> Dependencies = new List<Dependency> ();
Expand All @@ -30,7 +31,8 @@ public void RecordDependency (object target, in DependencyInfo reason, bool mark
Dependencies.Add (new Dependency () {
Source = reason.Source?.ToString (),
Target = target.ToString (),
Marked = marked
Marked = marked,
DependencyKind = reason.Kind.ToString ()
});
}

Expand Down