dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.37k stars 4.75k forks source link

Microsoft.Extensions.Configuration APIs haven't been AOT-annotated #71654

Closed kant2002 closed 2 years ago

kant2002 commented 2 years ago

Consider this small application

using System.Diagnostics.CodeAnalysis;
using Microsoft.Extensions.Configuration;

var config = new ConfigurationBuilder()
    .AddJsonFile("appsettings.json")
    .Build();

var appSettings = GetAppSettings(config);
Console.WriteLine($"{appSettings.Database}: {appSettings.ConnectionString}");

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern",
    Justification = "AppSettings is consists only from primitive type and thus will never be trimmed")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL3050:UnrecognizedReflectionPattern",
    Justification = "AppSettings is consists only from primitive type and thus will never be trimmed")]
AppSettings GetAppSettings(IConfiguration config)
{
    return config.Get<AppSettings>();
}

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
public class AppSettings
{
    public string? ConnectionString { get; set; }

    public DatabaseServer Database { get; set; } = DatabaseServer.None;
}

public enum DatabaseServer
{
    None,
    SqlServer,
    PostgreSql,
    MySql
}

With following packages

dotnet add package Microsoft.Extensions.Configuration.Binder -v 7.0.0-*
dotnet add package Microsoft.Extensions.Configuration.Json -v 7.0.0-*

and `appsettings.json

{
    "ConnectionString": "Server=tfb-database;Database=hello_world;User Id=benchmarkdbuser;Password=benchmarkdbpass;Maximum Pool Size=1024;SslMode=None;ConnectionReset=false;ConnectionIdlePingTime=900;ConnectionIdleTimeout=0;AutoEnlist=false;DefaultCommandTimeout=0;ConnectionTimeout=0;IgnorePrepare=false;",
    "Database": "mysql"
}

ILC spill warnings

ILC : AOT analysis warning IL3050: Microsoft.Extensions.Configuration.ConfigurationBinder.AttemptBindToCollectionInterfaces(Type,IConfiguration,BinderOptions): Using member 'System.Type.MakeGenericType(Type[]
)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj]
ILC : AOT analysis warning IL3050: Microsoft.Extensions.Configuration.ConfigurationBinder.AttemptBindToCollectionInterfaces(Type,IConfiguration,BinderOptions): Using member 'System.Type.MakeGenericType(Type[]
)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj]
ILC : AOT analysis warning IL3050: Microsoft.Extensions.Configuration.ConfigurationBinder.BindArray(Type,IEnumerable,IConfiguration,BinderOptions): Using member 'System.Array.CreateInstance(Type,Int32)' which
 has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj]
ILC : AOT analysis warning IL3050: Microsoft.Extensions.Configuration.ConfigurationBinder.BindToCollection(Type,IConfiguration,BinderOptions): Using member 'System.Type.MakeGenericType(Type[])' which has 'Req
uiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj]
/_/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs(174): AOT analysis warning IL3050: System.ComponentModel.EnumConverter.ConvertTo(ITypeDescriptorContext,CultureI
nfo,Object,Type): Using member 'System.Enum.GetValues(Type)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. It might not be possible to create an array of the enum type a
t runtime. Use the GetValues<TEnum> overload instead. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj]

From my understanding AppSettings is safe to map IConfiguration and I want express that. But as you can see all my silly attempts fails.

kant2002 commented 2 years ago

This is boils down to patterns like this

https://github.com/dotnet/runtime/blob/338db1a1ba498f308797ce7a54c0ed54e7286d50/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs#L289-L304

which looks very similar to what FSharp do with printf specialization at runtime. I can give only indirect hint on this in form of these declarations required to make Printf works in NativeAOT https://github.com/kant2002/RdXmlLibrary/blob/625eaf01310f2e098b8a17cfe9658e140d37262e/FSharp.Core.xml#L25-L39

agocke commented 2 years ago

I assume from the ILC prefix that this is from NativeAOT -- moving back to the runtime repo.

ghost commented 2 years ago

Tagging subscribers to this area: @dotnet/area-extensions-configuration See info in area-owners.md if you want to be subscribed.

Issue Details
Consider this small application ```csharp using System.Diagnostics.CodeAnalysis; using Microsoft.Extensions.Configuration; var config = new ConfigurationBuilder() .AddJsonFile("appsettings.json") .Build(); var appSettings = GetAppSettings(config); Console.WriteLine($"{appSettings.Database}: {appSettings.ConnectionString}"); [UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026:UnrecognizedReflectionPattern", Justification = "AppSettings is consists only from primitive type and thus will never be trimmed")] [UnconditionalSuppressMessage("ReflectionAnalysis", "IL3050:UnrecognizedReflectionPattern", Justification = "AppSettings is consists only from primitive type and thus will never be trimmed")] AppSettings GetAppSettings(IConfiguration config) { return config.Get(); } [DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] public class AppSettings { public string? ConnectionString { get; set; } public DatabaseServer Database { get; set; } = DatabaseServer.None; } public enum DatabaseServer { None, SqlServer, PostgreSql, MySql } ``` With following packages ``` dotnet add package Microsoft.Extensions.Configuration.Binder -v 7.0.0-* dotnet add package Microsoft.Extensions.Configuration.Json -v 7.0.0-* ``` and `appsettings.json ```json { "ConnectionString": "Server=tfb-database;Database=hello_world;User Id=benchmarkdbuser;Password=benchmarkdbpass;Maximum Pool Size=1024;SslMode=None;ConnectionReset=false;ConnectionIdlePingTime=900;ConnectionIdleTimeout=0;AutoEnlist=false;DefaultCommandTimeout=0;ConnectionTimeout=0;IgnorePrepare=false;", "Database": "mysql" } ``` ILC spill warnings ``` ILC : AOT analysis warning IL3050: Microsoft.Extensions.Configuration.ConfigurationBinder.AttemptBindToCollectionInterfaces(Type,IConfiguration,BinderOptions): Using member 'System.Type.MakeGenericType(Type[] )' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj] ILC : AOT analysis warning IL3050: Microsoft.Extensions.Configuration.ConfigurationBinder.AttemptBindToCollectionInterfaces(Type,IConfiguration,BinderOptions): Using member 'System.Type.MakeGenericType(Type[] )' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj] ILC : AOT analysis warning IL3050: Microsoft.Extensions.Configuration.ConfigurationBinder.BindArray(Type,IEnumerable,IConfiguration,BinderOptions): Using member 'System.Array.CreateInstance(Type,Int32)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. The code for an array of the specified type might not be available. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj] ILC : AOT analysis warning IL3050: Microsoft.Extensions.Configuration.ConfigurationBinder.BindToCollection(Type,IConfiguration,BinderOptions): Using member 'System.Type.MakeGenericType(Type[])' which has 'Req uiresDynamicCodeAttribute' can break functionality when AOT compiling. The native code for this instantiation might not be available at runtime. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj] /_/src/libraries/System.ComponentModel.TypeConverter/src/System/ComponentModel/EnumConverter.cs(174): AOT analysis warning IL3050: System.ComponentModel.EnumConverter.ConvertTo(ITypeDescriptorContext,CultureI nfo,Object,Type): Using member 'System.Enum.GetValues(Type)' which has 'RequiresDynamicCodeAttribute' can break functionality when AOT compiling. It might not be possible to create an array of the enum type a t runtime. Use the GetValues overload instead. [D:\d\scratch\ConsoleApp7\ConsoleApp7.csproj] ``` From my understanding `AppSettings` is safe to map `IConfiguration` and I want express that. But as you can see all my silly attempts fails.
Author: kant2002
Assignees: -
Labels: `area-Extensions-Configuration`
Milestone: -
agocke commented 2 years ago

I assume this is just because the APIs haven't been annotated with RequiresDynamicCode to bubble the requirements up to the public API, right?

kant2002 commented 2 years ago

I assume from the ILC prefix that this is from NativeAOT -- moving back to the runtime repo.

You are right. I really misinterpret results and regular trimming works fine.

eerhardt commented 2 years ago

I assume this is just because the APIs haven't been annotated with RequiresDynamicCode

They have been annotated with RequiresUnreferencedCode already. Doesn't that imply that they are not safe for NativeAOT?

return config.Get();

This API is marked as RequiresUnreferencedCode:

https://github.com/dotnet/runtime/blob/0cb001f233681fba9135aec9a4d7bdf6eb875e33/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs#L34-L48

agocke commented 2 years ago

Good question. So far we haven’t had RUC suppress RDC, but maybe we should @tlakollo @MichalStrehovsky

tlakollo commented 2 years ago

The way I see it is: PublishTrimmed requires to be trim safe. Therefore any RequiresUnreferencedCode attribute makes the app not safe PublishSingleFile requires to be single file safe. Therefore any RequiresAssemblyFiles attribute makes the app not safe PublishAOT requires being trim, single file and dynamic code safe. Therefore any RequiresUnreferencedCode, RequiresAssemblyFiles, RequiresDynamicCode make the app not safe

They have been annotated with RequiresUnreferencedCode already. Doesn't that imply that they are not safe for NativeAOT?

RequiresUnreferencedCode means the result of the publishing the NativeAOT app is not safe, since trimming is not safe. Do we need to annotate also with RequiresDynamicCode because it might also be DynamicCode? The app is not going to be safe anyway. It's already trim unsafe and making it dynamic code unsafe won't make a difference for the final NativeAOT publishing. On the other hand the attributes are independent of the publishing type, there could be a theoretical Publishing in the future that only requires being dynamic code safe and doesn't care about trimming or single file. In that case if you didn't annotate with RequiresDynamicCode you have a problem.

So far we haven’t had RUC suppress RDC, but maybe we should

My opinion as expresed before is that the attributes are independent, they even have different behaviors depending on the attribute (for example RequiresUnreferencedCode interacts with Reflection and DynamicallyAccessedMembers and the other attributes don't). They also can evolve and go completely different paths. Also, it would be difficult to document, RUC does not suppress RAF, RDC does not suppress RUC and to be honest Im not sure RUC suppresses RDC. It kind of implies that all dynamic code is a trimming problem too, and again wouldnt hold up for a hipotetical scenario that only cares about dynamic code.

MichalStrehovsky commented 2 years ago

Good question. So far we haven’t had RUC suppress RDC, but maybe we should @tlakollo @MichalStrehovsky

Two comments on that. Basically +1 on what Tlaka just wrote.

  1. It is true that we don't currently have a SKU that can't generate code at runtime and wouldn't be trimmed at the same time. Are we sure there won't be such SKU in the future?
  2. The message from RUC in this specific case is "In case the type is non-primitive, the trimmer cannot statically analyze the object's type so its members may be trimmed." and the user is suppressing the warning. The RDC warning from MakeGenericType is about a very different problem (we're trying to MakeGeneric a Dictionary). If you want to MakeGeneric a dictionary from double to float (both "primitive types") you'll run into a runtime exception. I think we'll want a RequiresDynamicCode that specifically describes the situations when MakeGeneric could be called and surface that to the user in addition to the trimming warning.
eerhardt commented 2 years ago

It would be good to annotate these APIs for RequiresDynamicCode in 7.0.