search

dotnet / linker

388 stars 126 forks source link

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

Closed jtschuster closed 1 year ago

jtschuster commented 1 year ago

As the first part of solving https://github.com/dotnet/linker/issues/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.

mrvoorhe commented 1 year ago

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.

Would including src/linker/Linker/DependencyInfo.cs in the compilation of Mono.Linker.Tests.Cases.Expectations fix the build issue? After looking at the attribute in use during the test in this PR, the string version of the DependencyKind value is confusing to read. It almost reads as a method name given that other attributes follow the pattern of (string type, string methodName)

jtschuster commented 1 year ago

Would including src/linker/Linker/DependencyInfo.cs in the compilation of Mono.Linker.Tests.Cases.Expectations fix the build issue? After looking at the attribute in use during the test in this PR, the string version of the DependencyKind value is confusing to read. It almost reads as a method name given that other attributes follow the pattern of (string type, string methodName)

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.

vitek-karas commented 1 year ago

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. There are ways to handle the compiler's confusion about getting the same type from two different assemblies, but it's probably not worth the trouble here.