Flow attributes on generic types to compiler generated constructs

[eerhardt]

Version Used: 3.8.0-1.20361.1 (f24d2c5c)

See the discussions in:

• https://github.com/dotnet/runtime/pull/40431#issuecomment-669768677
• https://github.com/mono/linker/issues/1416

When annotating Type information to make a library linker-friendly, there are times when an annotation needs to flow into a lambda method. For example:

public interface ITypedHttpClientFactory<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors)] TClient>
{
}

...

internal static IHttpClientBuilder AddTypedClientCore<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] TClient>(
    this IHttpClientBuilder builder, bool validateSingleType)
    where TClient : class
{
    ReserveClient(builder, typeof(TClient), builder.Name, validateSingleType);

    builder.Services.AddTransient<TClient>(s =>
    {
        IHttpClientFactory httpClientFactory = s.GetRequiredService<IHttpClientFactory>();
        HttpClient httpClient = httpClientFactory.CreateClient(builder.Name);

        ITypedHttpClientFactory<TClient> typedClientFactory = s.GetRequiredService<ITypedHttpClientFactory<TClient>>();
        return typedClientFactory.CreateClient(httpClient);
    });

    return builder;
}

In this case, the linker is warning:

Trim analysis warning IL2006: Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.<>c__DisplayClass10_0<TClient>.<AddTypedClientCore>b__0(IServiceProvider): The generic parameter 'TClient' from 'Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.<>c__DisplayClass10_0<TClient>' with dynamically accessed member kinds 'None' is passed into the generic parameter 'TClient' from 'Microsoft.Extensions.Http.ITypedHttpClientFactory<TClient>' which requires dynamically accessed member kinds 'PublicConstructors'. To fix this add DynamicallyAccessedMembersAttribute to it and specify at least these member kinds 'PublicConstructors'.
Trim analysis warning IL2006: Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.<>c__DisplayClass10_0<TClient>.<AddTypedClientCore>b__0(IServiceProvider): The generic parameter 'TClient' from 'Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.<>c__DisplayClass10_0<TClient>' with dynamically accessed member kinds 'None' is passed into the generic parameter 'TClient' from 'Microsoft.Extensions.Http.ITypedHttpClientFactory<TClient>' which requires dynamically accessed member kinds 'PublicConstructors'. To fix this add DynamicallyAccessedMembersAttribute to it and specify at least these member kinds 'PublicConstructors'.
Trim analysis warning IL2006: Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.<>c__DisplayClass10_0<TClient>.<AddTypedClientCore>b__0(IServiceProvider): The generic parameter 'TClient' from 'Microsoft.Extensions.DependencyInjection.HttpClientBuilderExtensions.<>c__DisplayClass10_0<TClient>' with dynamically accessed member kinds 'None' is passed into the generic parameter 'TClient' from 'Microsoft.Extensions.Http.ITypedHttpClientFactory<TClient>' which requires dynamically accessed member kinds 'PublicConstructors'. To fix this add DynamicallyAccessedMembersAttribute to it and specify at least these member kinds 'PublicConstructors'.

I have annotated TClient with the correct attribute, but the linker doesn't see the annotation on the generated class, so it is warning.

If the Compiler would flow the [DynamicallyAccessedMembers] attribute from the outer method onto the generated class's TClient generic type, the linker would see that the type is annotated correctly and it wouldn't warn unnecessarily.

Expected Behavior:

The underlying <>c__DisplayClass10_0<TClient> type should have a DynamicallyAccessedMembers attribute on TClient matching the attribute used in the calling method.

Actual Behavior:

<>c__DisplayClass10_0<TClient> has no attributes on the TClient generic type, and so the linker warns that the code is not linker safe.

cc @davidfowl @MichalStrehovsky

[eerhardt]

@jaredpar - any thoughts on the above proposal? In order to drive the ILLinker warnings to zero in 6.0, we would like this addressed in the compiler. That way the ILLinker sees the above TClient type is really annotated, and stops issuing the false warning.

[danmoseley]

@jaredpar?

[jaredpar]

Sorry, missed this first time around.

Bit of context here. The decision to not emit attributes on the type parameters, or anything else that could be relevant to the signature, is a deliberate choice. The rational is that what we generate here is an implementation detail and any attempt to peak at that code is asking for trouble because we can and will change what we generate here.

Concrete example is between C# 5 and 6 we pretty radically changed how we emitted lambda expressions. There was a shift away from using static method emit strategies to singleton closures with instance methods. The reasoning being that it's measurable, and even observably, faster to do so. This broke a number of customers that depended on us emitting them the old way. Our feedback was essentially "sorry, but this is an implementation detail, we changed it for good reasons, we're not switching it back".

Emitting attributes from the source into the generated code is essentially providing incentive to customers to look here. It's in some ways taking both sides of the argument. It's providing a service that is only valuable if customers take actions we've explicitly asked them to not take :smile:

That all being said recent decisions have started to go the other direction here. Local functions for instance allow attributes and the language guarantees we will emit them to the final binary on what lines up to the same method or type parameter. This was critical for PInvoke and function pointer scenarios. This scenario falls into that general type of category. Further we're considering allowing attributes on lambdas in C# 10 and the emit decision for them would almost certainly follow the decision for local functions.

Will bring this up with LDM.

The situation is a bit more complex than what this issue lays out.

• The decision likely wouldn't be made for type parameters in isolation. It would be made for all places attributes can appear in the context of lambdas.
• The way in which type parameters are emitted doesn't necessarily line up with the way code is written. Consider as a concrete example the following where you can observe methods with single declared arity emitted with arity of two. https://sharplab.io/#v2:EYLgtghgzgLgpgJwD4AEBMBGAsAKBQZgAJ1CBhQgb10JuKJQBZCBZAHgFUA+ACgEpLqtIQBVCAcVbCe/QUJpUccpcQCskzoQAehALyE+ujQBM4AMwgBXADYwA3LOU0UAdi197i5QF8HQ37RQMAE5uCQBLADsYaV4PIR8cLyA

[AlekseyTs]

There is a System.Runtime.CompilerServices.CompilerGeneratedAttribute on the type. Should it be handled specially by the linker?

[Blackclaws]

A similar/the same thing happens for local async functions that have closures. Minimal repro is here: https://github.com/Blackclaws/redundant-trim-warning

@jaredpar Is there any useful way in the meantime to suppress these warnings? We're trying to get rid of these false positives as much as possible so that we see the actual issues.

Interestingly enough it needs to be an async closure, either a closure or an async method alone will not trigger this.

[jaredpar]

@jaredpar Is there any useful way in the meantime to suppress these warnings?

@agocke is a better resource for that question as trim warnings are not produced by the compiler.

Overall though there hasn't been much discussion on this since my last message ~1 year ago. So overall we're in the same place.

[agocke]

Yeah, this is tricky for the compiler, although I think there are some strong guarantees made in current closure conversion that could be formalized, namely that every user type parameter has one exact corresponding type parameter in the output via alpha renaming.

However, we did build some very complicated linker stuff for .NET 7 to try to reverse-engineer the compiler construction and figure all this out. Looks like you've found a case that we missed.

@sbomer could you take a look at the example from @Blackclaws?

[agocke]

Here's my proposal for making the linker's job a lot easier: require a 1:1 relationship between user-defined type parameters and synthesized type parameters, and copy attributes from the user defined to the synthesized during alpha renaming.

This is already how the code works because of constraints -- you have to copy constraints from type parameters during lowering, so you need to keep a complete map. In theory there could be sharing, but the compiler never does that today and this new rule would disallow that optimization, at least in the case attributes are present.

General transformation:

public class C {
    public void M<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>() {
        Action a = () => {
            Console.WriteLine(typeof(T).GetMembers());
        };
    }
}

becomes

public class C
{
    private sealed class <>c__0<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>
    {
        public static readonly <>c__0<T> <>9 = new <>c__0<T>();

        public static Action <>9__0_0;

        internal void <M>b__0_0()
        {
            Console.WriteLine(typeof(T).GetMembers());
        }
    }
    public void M<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] T>()
    {
        if (<>c__0<T>.<>9__0_0 == null)
        {
            <>c__0<T>.<>9__0_0 = new Action(<>c__0<T>.<>9.<M>b__0_0);
        }
    }
}

Currently that attribute doesn't appear on the display class type parameter.

[sbomer]

Closing this - we can continue discussion in the broader proposal that's a superset of this one: https://github.com/dotnet/roslyn/issues/73920. edit: @agocke would you mind closing this for me? I don't have permission.