Add KeptByAttribute to validate an item was kept due to a specific dependency #3096
Conversation
...heritance.VirtualMethods/VirtualMethodGetsStrippedIfImplementingMethodGetsInvokedDirectly.cs
Show resolved
Hide resolved
....Tests.Cases/Inheritance.VirtualMethods/VirtualMethodGetsPreservedIfBaseMethodGetsInvoked.cs
Outdated
Show resolved
Hide resolved
Would including |
Unfortunately no, Mono.Linker.Tests.csproj references Mono.Linker.csproj and Mono.Linker.Tests.Cases.Expectations.csproj, so there is duplicate type error with both having DependencyInfo.cs. I agree that having the string version is not ideal, and I would prefer to use DependencyKind, I just wanted to get a simple working version before investing too much time in making the build work. |
|
It would not be pretty, but we could define the enum in the tests in different namespace. The code would still look identical, just the usings would be different. It's obviously not ideal (duplication), but it would solve this. |
| @@ -22,7 +24,9 @@ public virtual void Foo () | |||
| [KeptBaseType (typeof (B))] | |||
| class A : B | |||
| { | |||
| [Kept] | |||
| // Bug: https://github.com/dotnet/linker/issues/3078 | |||
There was a problem hiding this comment.
What's the bug for this one - just that it isn't treated as a direct call? It might be worth a comment.
| @@ -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> ()) | |||
There was a problem hiding this comment.
Why the check for SkipKeptItemsValidation - isn't it ok to not record dependencies if there are only [Kept] attributes for example?
There was a problem hiding this comment.
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.
| { | ||
| // public KeptByAttribute (string dependencyProvider, DependencyKind reason) { } | ||
| // public KeptByAttribute (Type dependencyProvider, DependencyKind reason) { } | ||
| // public KeptByAttribute (Type dependencyProvider, string memberName, DependencyKind reason) { } |
There was a problem hiding this comment.
nit: update these comments to reflect that they take string not DependencyKind now.
…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
As the first part of solving #3078, we need a system to validate that we are marking items for the correct reasons. The tests we have try to ensure that something can only be kept for a certain reason, but that 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.
I tried using a DependencyKind parameter itself instead of a string, but ran into build issues. Maybe we could look into using DependencyKind directly in the future.