Trimming warning IL2111 when using dictionary GetOrAdd pattern

[eerhardt]

When using a ConcurrentDictionary<Type, XXX> and using the GetOrAdd method to populate the dictionary, it is not possible to remove all ILLink warnings. With the below code, I'm still getting:

Program.cs(79,9): Trim analysis warning IL2111: DotNetDispatcher.GetCachedMethodInfo(IDotNetObjectReference, String): Method 'DotNetDispatcher.<GetCachedMethodInfo>g__ScanTypeForCallableMethods|2_0(Type)' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the method.

Repro

• dotnet publish -c Release the following app:

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net7.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
    <PublishTrimmed>true</PublishTrimmed>
    <RuntimeIdentifier>win-x64</RuntimeIdentifier>
    <TrimmerDefaultAction>link</TrimmerDefaultAction>
  </PropertyGroup>

</Project>

using System.Collections.Concurrent;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;

MyInterop interop = new();
DotNetDispatcher.Invoke(DotNetObjectReference.Create(interop), "Method1");
DotNetDispatcher.Invoke(DotNetObjectReference.Create(interop), "Method2");
DotNetDispatcher.Invoke(DotNetObjectReference.Create(interop), "Method1");

public class MyInterop
{
    [JSInvokable]
    public void Method1()
    {
        Console.WriteLine("Method1");
    }

    [JSInvokable]
    public void Method2()
    {
        Console.WriteLine("Method2");
    }
}

public static class DotNetObjectReference
{
    public static DotNetObjectReference<TValue> Create<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] TValue>(TValue value) where TValue : class
    {
        if (value is null)
        {
            throw new ArgumentNullException(nameof(value));
        }

        return new DotNetObjectReference<TValue>(value);
    }
}

public sealed class DotNetObjectReference<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] TValue> :
    IDotNetObjectReference where TValue : class
{
    internal DotNetObjectReference(TValue value)
    {
        Value = value;
    }

    public TValue Value { get; }

    object IDotNetObjectReference.Value => Value;

    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
    Type IDotNetObjectReference.Type => typeof(TValue);
}

public interface IDotNetObjectReference
{
    object Value { get; }

    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
    Type Type { get; }
}

[AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
public sealed class JSInvokableAttribute : Attribute { }

public class DotNetDispatcher
{
    private static readonly ConcurrentDictionary<Type, IReadOnlyDictionary<string, MethodInfo>> _cachedMethodsByType = new();

    public static void Invoke(IDotNetObjectReference objectReference, string methodIdentifier)
    {
        MethodInfo methodInfo = GetCachedMethodInfo(objectReference, methodIdentifier);

        methodInfo.Invoke(objectReference.Value, null);
    }

    private static MethodInfo GetCachedMethodInfo(IDotNetObjectReference objectReference, string methodIdentifier)
    {
        var type = objectReference.Type;
        var assemblyMethods = _cachedMethodsByType.GetOrAdd(type, ScanTypeForCallableMethods);
        if (assemblyMethods.TryGetValue(methodIdentifier, out var result))
        {
            return result;
        }
        else
        {
            throw new ArgumentException($"The type '{type.Name}' does not contain a public invokable method with [{nameof(JSInvokableAttribute)}(\"{methodIdentifier}\")].");
        }

        static Dictionary<string, MethodInfo> ScanTypeForCallableMethods([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type)
        {
            var result = new Dictionary<string, MethodInfo>(StringComparer.Ordinal);

            foreach (var method in type.GetMethods(BindingFlags.Instance | BindingFlags.Public))
            {
                if (method.ContainsGenericParameters || !method.IsDefined(typeof(JSInvokableAttribute), inherit: false))
                {
                    continue;
                }

                var identifier = method.Name!;

                if (result.ContainsKey(identifier))
                {
                    throw new InvalidOperationException($"The type {type.Name} contains more than one " +
                        $"[JSInvokable] method with identifier '{identifier}'. All [JSInvokable] methods within the same " +
                        "type must have different identifiers. You can pass a custom identifier as a parameter to " +
                        $"the [JSInvokable] attribute.");
                }

                result.Add(identifier, method);
            }

            return result;
        }
    }
}

Notes

As you can see, I've flown DynamicallyAccessedMembers on the Type all the way down to the call to GetMethods. However, the trimmer is still warning that the nested method ScanTypeForCallableMethods "is accessed via reflection", which it is not. It is being called when the cache isn't populated, but not through reflection.

I'm not sure exactly how this should be handled. The naïve approach would be to specially know about GetOrAdd methods. Maybe some hueristic can be made about methods that are of the form (T1 one, Func<T1, return> func), and if the referenced func has DynamicallyAccessedMembers applied, then the T1 one being passed to the function needs to match - just as if I was calling func and passing in one as a parameter.

cc @vitek-karas @MichalStrehovsky @sbomer

[sbomer]

The GetOrAdd signature (TKey key, Func<TKey, TValue> valueFactory) already seems to express that the key argument type matches the func's first argument type. Would it work to create a wrapper for Type that has the annotation, and make it a ConcurrentDictionary<KeyType, ...> instead? Like this:

struct KeyType
{
    [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)]
    public Type Type;
    public KeyType([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type t) => Type = t;
    // Equals, etc...
}

I agree that the warning message is confusing since your example doesn't access anything via reflection. We should probably find a better way to word it.

[eerhardt]

Would it work to create a wrapper for Type that has the annotation, and make it a ConcurrentDictionary<KeyType, ...> instead?

It might work. But putting in hacks like this to work around the tooling limitations doesn't feel right. I should be able to write the code naturally. For now, I've suppressed the warning - see dotnet/aspnetcore#41610.

[vitek-karas]

I should be able to write the code naturally.

I think it's a bit difficult to say what that means.

• Does it mean that you should be able to add annotation on the dictionary to mark all of the types in it with some annotation?
  • This is tricky as there's no good way to do this - C#/IL doesn't allow attributes on generic arguments. C# had to invent a special attribute on the parameter/variable to encode nullability for these cases (and it can get away with it basically only because it's made invisible to developers).
  • That said - this is ultimately doable - but seems really hard right now.
• Does it mean that the GetOrAdd method should be able to "transfer" the annotation from the type parameter to the func parameter?
  • How would this work. Note that the code in this issue is only "correct" because we KNOW that the type parameter is passed to the func as is, and the func will not be used for anything else. I can easily come up with other methods, which will not behave like this and for which it's not correct to transfer the annotation.
  • To solve this we would probably have to add a way to create "links" between parameters/generic parameters to express addition flow information.

There are other possible solutions:

• Perform cross method analysis - in this case if the linker "inlined" the GetOrAdd method it would see through this and realize it's OK. Eventually we will have a mechanism to do this in the linker, so maybe we could introduce an attribute which would ask linker to do this for a method (InlineForAnalysisPurposes or something like that).
• Special case this method (and probably lot of others) - basically hardcode the knowledge of their semantics. Exactly what we do for other methods mainly around reflection.

Generally I agree that this is something which the tooling should be able to figure out (potentially with some help from annotations), but so far I couldn't come up with a reasonable solution.

Honestly right now I like the "conditional inlining for analysis purposes" as the best solution (but other than increased complexity it would only work in certain cases, for example it would not work on virtuals - that would be really difficult to make work).

[eerhardt]

I should be able to write the code naturally.

I think it's a bit difficult to say what that means.

My thinking was that it means the 2nd choice: "Does it mean that the GetOrAdd method should be able to "transfer" the annotation from the type parameter to the func parameter?"

[MichalStrehovsky]

It took me a while to figure out what this is warning about (we really should improve the warning here). I wrote a simpler repro case just for my own reference that I'm pasting here:

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using System.Runtime.CompilerServices;

Func<Type, IEnumerable<MemberInfo>> func = ListMethods;

foreach (var m in func(typeof(ConditionalWeakTable<,>)))
    Console.WriteLine(m);

static IEnumerable<MemberInfo> ListMethods([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type)
{
    return type.GetMethods();
}

The problem is really that we're creating a delegate that points to an annotated method and at that point trimming has no way to guarantee that all the places that call the delegate actually satisfy the invariants. Running the above repro with trimming does in fact produce different outputs and the delegate creation is the only point where we have a chance to warn about it.

We would basically need to look at what GetOrAdd does, ensure that the value passed to delegate invocation is properly annotated, and also make sure the delegate instance never escapes our analysis (it's not stored in a field, etc.). And then figure out how warnings would look like if the invariants don't hold. I'm not a big fan of trying to do the inlining and especially not across assemblies. There's too much fragility around that e.g. if a new version of the library starts caching the delegate for whatever reason, that would be an escape from the analysis and all bets are off - we would need to start warning. Cross-assembly inlining also doesn't work for the analyzer at all.

I thought about delegates in the past and one could make this work:

delegate IEnumerable<MemberInfo> Foo([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods)] Type type);

But that doesn't work for Func or Action. The problem is really that we need to be able to extend the type system in similar ways that Roslyn did for nullable reference types and custom attributes alone can't do that. The wrapping struct that Sven suggested is the only way how to express it in the type system right now.