Annotations do not survive call via GroupBy

[kant2002]

Consider this example. I n my understanding that's perfectly valid annotations. What am I missing?

using System.Diagnostics.CodeAnalysis;

var validationParameters = new ValidationParameters();

foreach (var typeGroup in validationParameters.Benchmarks.GroupBy(benchmark => benchmark.Descriptor.Type))
{
    // warning IL2072: 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicParameterlessConstructor' in call to 'TryCreateBenchmarkTypeInstance(Type, out Object)'. The return value of method 'System.Linq.IGrouping<TKey, TElement>.Key.get' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
    if (!TryCreateBenchmarkTypeInstance(typeGroup.Key, out var benchmarkTypeInstance))
    {
        continue;
    }
}

bool TryCreateBenchmarkTypeInstance(
        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
        Type type, out object? instance)
{
    try
    {
        instance = Activator.CreateInstance(type);

        return true;
    }
    catch (Exception)
    {
        instance = null;
        return false;
    }
}

public class ValidationParameters
{
    public IReadOnlyList<BenchmarkCase> Benchmarks { get; } = new List<BenchmarkCase>();
}
public class BenchmarkCase
{
    public Descriptor Descriptor { get; } = new Descriptor();
}
public class Descriptor
{
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]
    public Type Type { get; set; } = null!;
}

This is simplified version of https://github.com/dotnet/BenchmarkDotNet/blob/5e62756c126980ecde9e4779b3c320b3225381ee/src/BenchmarkDotNet/Validators/ExecutionValidatorBase.cs#L25-L27 in my naive attempt to address Michal's suggestion dotnet/BenchmarkDotNet#2020 (comment)

[kant2002]

That's warning when I have <EnableTrimAnalyzer>true</EnableTrimAnalyzer>, did not try run linker.

[kant2002]

I assume that's the same

using System.Diagnostics.CodeAnalysis;
using System.Reflection;

X.TypeToBenchmarks(typeof(int));

public static class X
{
    public static void TypeToBenchmarks(
        [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.NonPublicMethods | DynamicallyAccessedMemberTypes.PublicMethods)]
        Type type)
    {
        MethodsToBenchmarksWithFullConfig(type);
    }

    private static void MethodsToBenchmarksWithFullConfig(Type type)
    {
        // IL2070: X.MethodsToBenchmarksWithFullConfig(Type): 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods', 'DynamicallyAccessedMemberTypes.NonPublicMethods' in call to 'System.Type.GetMethods(BindingFlags)'. The parameter 'type' of method 'X.MethodsToBenchmarksWithFullConfig(Type)' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to.
        var allMethods = type.GetMethods(BindingFlags.Static | BindingFlags.Instance | BindingFlags.Public | BindingFlags.NonPublic);
    }
}

[vitek-karas]

There's an important rule in the DynamicallyAccessedMembers annotations (we refer to these as DAM annotations, or just trim annotations):

• The analysis will only infer them within a single method body!

So in the second example, you need to put DAM on the type parameter of the MethodsToBeBenchmarksWithFullConfig. The analysis will not magically flow the annotation from the caller to the callee. (This was a specific design decision, cross method analysis is very complex and expensive, and more importantly if there's a warning somewhere describing to the user why it happened is very hard - if you ever met the errors which C++ compilers produce due to std template errors, you have an idea - it's a very similar problem).

The first example is basically the same, but unfortunately there's no good solution. In this case the lambda returns annotated value, but the analysis would have to be smart enough to flow it through to the GroupBy, know that it effectively applies to one of the generic arguments, flow that to the return value of the GroupBy and then imply the annotation there. Not only is this really complex to do, but it could be wrong - the body of GroupBy may or may not change the returned value. If it did, the annotation would not flow. So what this would really need is an ability to put DAM annotation on generic arguments in generic instantiation. Something similar to what happens with IEnumerable<string?> - the ? is effectively an annotation which gets propagated. So far we haven't design a way to do this (attributes are not very nice since they don't apply to generic arguments, so one would have to encode it as an attribute on the outer type and then encode which argument it applies to - that's what Roslyn does for nullable, but it hides it from the user, we would not be able to do that unfortunately).

For the first problem, our current suggestion is to wrap the problematic piece of code in a local function, annotate that function and then suppress the warning.

So it might look something like this:

ForEachTypeGroup(validationParameters.Benchmarks, ([DAM(PublicParameterlessConstructor)] Type type, IEnumerable<Benchmark> values) => {
    if (!TryCreateBenchmarkTypeInstance(typeGroup.Key, out var benchmarkTypeInstance))
    {
        return;
    }
});

[UnconditionalSuppressMessage(....)] // Suppress the warning
void ForEachTypeGroup(IEnumerable<Benchmark> benchmarks, Action<[DAM(PublicParameterlessConstructor)] Type, IEnumerable<Benchmark>> action)
{
    foreach (var typeGroup in validationParameters.Benchmarks.GroupBy(benchmark => benchmark.Descriptor.Type))
    {
        // IL... : Warning that typeGroup.Key doesn't have a matching annotation basically
        action(typeGroup.Key, typeGroup.Values);
    }
}

Overall this is a known problem with collections specifically (but it applies to lot of other cases). Unfortunately so far we didn't come up with a good solution.

Side note: Even Roslyn's nullable is not perfect in this case. If the GroupBy lambda returns non-nullable string for example, the code will infer non-nullable string as the Key type, but there's no guarantee that the GroupBy implementation will not return null. The compiler would have to "Recompile" the body of GroupBy and apply the annotation to see if the body is correct - since the nullability is not part of the GroupBy declaration - it's applied "after the fact".

[kant2002]

Thanks for explanation. I would not mind annotate every function which is required to be annotated. I only do not want to annotate unnecessary functions, given that DAM and RUC annotation potential compatibility problems. I assume relaxing them would be fine on non-virtual methods, but otherwise I fear introduce some extra annotations which hinder WinForms/BDN trimmability in the long run.

[vitek-karas]

With regard to WinForms: There might be some tricky things to deal with (resources for example) - we ran into some of these types of problems when we annotated BCL, and sometimes it was not easy to fix. It's case by case I think. In any case, a good approached which worked was to "over annotate" (RUC on methods we would like to not have RUC for example) and log issues for the things we were not sure about (or would like to do better). We then discussed possible solutions to these issues - sometimes it required trimming improvements, lot of times we figures something out in the library itself, we also sometimes relies on feature switches.

With regard to BDN: I'm not sure it's that important to be able to trim BDN. I know that for NativeAOT it's required, but we run xUnit tests with NativeAOT as well without actually trimming the xUnit part (yes, it does produce lot of warnings and so on, but it mostly works). Testing frameworks tend to be dynamic, so that's a problem, but they also tend to lend themselves pretty well to code generation (source generators).

Curious - what problem do you refer to when you say the DAM/RUC can introduce compatibility issues?

[kant2002]

There might be some tricky things to deal with (resources for example)

Here I would start with placing RUC here and there, but! Whole localization story around WinForms are built around resources. And designer rely on resources for storing images and complex data structure. I did not expect that any alternatives to that within WinForms arise by themself. Same can be saying about WPF, and quite possible about WinUI. Even if linker is not directly responsible for solving that solution, but leaving them unsolve bad for whole trimming story outside of web apps.

Maybe you can just shun anything inside DerivedControl.InitializeComponent or DerivedForm.InitializeComponent.

It's case by case I think.

I agree, and there other areas where placing RUC can make whole WinForms spill them all the time and do not actually indicate anything. Good example probably would be DataGridView which is central to LOB apps. I bet that architectural patterns will keep you engaged for some time :)

and log issues for the things we were not sure about (or would like to do better)

I maybe rant about this in separate issue.

With regard to BDN: I'm not sure it's that important to be able to trim BDN.

You may consider this is just learning excercise for me personally. At the same time, I think BDN is pretty small and unlikely I make stupid mistakes, like it possible in WinForms. Also I start reaping rewards in that area, because immidiately I have to ask questions about linking.

yes, it does produce lot of warnings and so on, but it mostly works

I believe you are first person interested in what would not work for BDN. Such patterns probably would be in other codebases, and maybe some issues can be generalized. Let's see how my experiment with BDN unrolls.

Curious - what problem do you refer to when you say the DAM/RUC can introduce compatibility issues?

I do not have hopes that I and WinForms team do not "under annotate" things on first try. That mean if somebody would use these annotations, then in next version we have to raise requirements. Most modern codebases use TrimWarningAsErrors=True and I expect all issues to be fixed, so subsequent release can lead in build breakages for these codebases which are most loyal adopters of the tech. I worry about unknowns which are not clear in that area.
Both I and WinForms team not knowledgeable enough so I expect mistakes.

[vitek-karas]

I do not have hopes that I and WinForms team do not "under annotate" things on first try.

I'd be more than happy to help in any way I can. Feel free to include me on the PRs and ask questions, ...
We know that trimming is tricky and especially when existing codebases use things which are not fully trimmable. Even if I can't figure it out, maybe we could introduce a new trimming feature to help with it.

[kant2002]

@vitek-karas don't worry, I would not be shy from now on. I'm bit overloaded with other work, but definitely want to comeback to WinForms very soon.

[agocke]

Closing as question answered.