Skip to content

Commit

Permalink
Add KeptByAttribute to validate an item was kept due to a specific de…
Browse files Browse the repository at this point in the history
…pendency (dotnet/linker#3096)

The tests we have in the linker try to ensure that something can only be kept for a certain reason, but it can be hard to ensure it's not being kept for another reason. For example, sometimes we use typeof(TestType) to mark a type, but that also makes it relevant to variant casting, which can cause parts of TestType to be kept for unintended reasons.

This PR creates the KeptByAttribute which accepts a fully qualified string in Cecil format to indicate depenency provider (the thing that is creating the dependency that marks the item with the attribute), as well as a string representing the DependencyKind to specify the "reason" there was a dependency between the two. It also can accept a System.Type, or a System.Type + name of the member to indicate the dependency provider.

Commit migrated from dotnet/linker@dc5e60f
  • Loading branch information
jtschuster authored Nov 7, 2022
1 parent d67748d commit 381f14b
Show file tree
Hide file tree
Showing 7 changed files with 128 additions and 12 deletions.
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
// 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
// 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
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
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> ())
|| _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

0 comments on commit 381f14b

Please sign in to comment.