Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Linker warns about generic type flown into lambda method #1416

Closed
eerhardt opened this issue Aug 5, 2020 · 6 comments
Closed

Linker warns about generic type flown into lambda method #1416

eerhardt opened this issue Aug 5, 2020 · 6 comments
Labels

Comments

@eerhardt
Copy link
Member

eerhardt commented Aug 5, 2020

This is similar to #1403, but instead of generated fields, it is for generic types that are flown into lambda methods.

using System;
using System.Diagnostics.CodeAnalysis;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            Create<Person>();
        }

        static Func<T> Create<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] T>()
        {
            return () =>
            {
                return Method<T>();   
            };
        }

        static T Method<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] T>()
        {
            return Activator.CreateInstance<T>();
        }
    }

    public class Person
    {
        public int Age { get; set; }
    }
}

Produces warning:

F:\DotNetTest\HelloWorld\Program.cs(16,13): Trim analysis warning IL2006: ConsoleApp1.Program.<>c__1<T>.<Create>b__1_0(): The generic parameter 'T' from 'ConsoleApp1.Program.<>c__1<T>' with dynamically accessed member kinds 'None' is passed into the generic parameter 'T' from 'ConsoleApp1.Program.Method<T>()' which requires dynamically accessed member kinds 'PublicParameterlessConstructor'. To fix this add DynamicallyAccessedMembersAttribute to it and specify at least these member kinds 'PublicParameterlessConstructor'. [F:\DotNetTest\HelloWorld\HelloWorld.csproj]

This was exposed by the following code:

https://github.com/dotnet/runtime/blob/6072e4d3a7a2a1493f514cdf4be75a3d56580e84/src/libraries/Microsoft.Extensions.Http/src/DependencyInjection/HttpClientBuilderExtensions.cs#L330-L345

        internal static IHttpClientBuilder AddTypedClientCore<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;
        }

Where ITypedHttpClientFactory<TClient> is annotated with PublicConstructors on TClient.

We should flow the generic type annotations into these methods.

cc @MichalStrehovsky @sbomer @marek-safar @vitek-karas

@eerhardt
Copy link
Member Author

eerhardt commented Aug 5, 2020

I tried suppressing the warning, but I'm not sure how to do it. I've tried:

        static Func<T> Create<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] T>()
        {
            return () =>
            {
                return CreateTypedClient();

                [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2006:UnrecognizedReflectionPattern",
                    Justification = "https://github.com/mono/linker/issues/1416")]
                static T CreateTypedClient()
                {
                    return Method<T>();   
                }
            };
        }

But that still produces warnings.

F:\DotNetTest\HelloWorld\Program.cs(16,13): Trim analysis warning IL2006: ConsoleApp1.Program.<>c__1<T>.<Create>b__1_0(): The generic parameter 'T' from 'ConsoleApp1.Program.<>c__1<T>' with dynamically accessed member kinds 'None' is passed into the generic parameter 'T' from 'ConsoleApp1.Program.<Create>g__CreateTypedClient|1_1<T>()' which requires dynamically accessed member kinds 'PublicParameterlessConstructor'. To fix this add DynamicallyAccessedMembersAttribute to it and specify at least these member kinds 'PublicParameterlessConstructor'. [F:\DotNetTest\HelloWorld\HelloWorld.csproj]

@mateoatr
Copy link
Contributor

mateoatr commented Aug 5, 2020

I tried suppressing the warning, but I'm not sure how to do it. I've tried:

Suppressing local functions doesn't work, see #1286.

Please dismiss this^, as that issue is not really related to this one (and to clarify, you can suppress warnings on local methods as long as you're not doing it through the Target property).

I just verified that the method T ConsoleApp1.Program::<Create>g__CreateTypedClient|1_1() is correctly being marked for suppression. However, the MemberDefinition attached to the warning is <>c__1`1'::'<Create>b__1_0.

@MichalStrehovsky
Copy link
Member

You would need to add a custom attribute to the lambda. That's probably refCount++ to dotnet/csharplang#3115.

The workaround should be something along the lines of:

    static Func<T> Create<[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)]T>()
    {
        return CreateTypedClient;

        static T CreateTypedClient()
        {
            return Method<T>();
        }
    }

You might not even need a suppression because the C# compiler seems to be propagating the custom attribute from Create's T to CreateTypedClient's T (they're different T's in IL no matter how it looks like in C#).

@eerhardt
Copy link
Member Author

eerhardt commented Aug 6, 2020

@marek-safar - I don't believe this should be labeled as a question. This is a bug in the linker IMO. It shouldn't be warning in this case. The code I've written is safe. I should not need to add a suppression.

@MichalStrehovsky - that workaround might work for the simplified case, but what about the original code that found this issue? How would I workaround the linker's bug here?

        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;
        }

where 

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

Note that not using a lambda in this case is a performance trap - dotnet/roslyn#5835.

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky - that workaround might work for the simplified case, but what about the original code that found this issue? How would I workaround the linker's bug here?

This is not a linker bug. The linker design as specced requires annotating all fields and methods. Linker doesn't operate on C# source code. The output of the C# compiler are unannotated methods with unannotated instance state. Whole program analysis and automatic propagation of annotations was scoped out of the linker design because it's not possible to do in a single pass; linker operates in a single pass to be fast.

It's possible to work around this multiple ways:

  • Do what the C# compiler is doing for this - create a helper class for the closure that is properly annotated.
  • Or mark all of this linker unfriendly
  • Or rewrite in a way that doesn't require annotations anymore (e.g. pass a factory Func or a ConstructorInfo, or some other mechanism).

I don't believe whole program propagation of annotations in general is feasible. It would result in difficult to reason about warnings (the way methods call each other and store stuff into fields would result in situation where flow of data from A to B results in a warning in spot C that would be difficult to connect to A or B - e.g. you would get a several warnings in List<T>.get_Item because whatever System.Type taken out of the underlying array of the List is passed to CreateInstance somewhere and Type.GetMethods somewhere else).

We could special case the specific shape of closures that C# compiler generates right now, but that's fragile and cannot be done without hardcoding detailed knowledge of how the closures are generated. It also requires a bunch of work on the warning side because we would need to unmangle the "Trim analysis warning IL2006: ConsoleApp1.Program.<>c__1<T>.<Create>b__1_0(): The generic parameter 'T' from 'ConsoleApp1.Program.<>c__1<T>' with dynamically accessed" into a warning that says "Please annotate Program.Create<T>". This requires even more hardcoding of C# compiler implementation details into linker because we not only need to do cross-method dataflow, but also understand the name mangling schemes.

@vitek-karas
Copy link
Member

As of .NET 7 RC1 this is fixed - the repro from above doesn't produce any warnings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants