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

Developers can safely trim apps which need Configuration Binder #44493

Closed
Tracked by #43543 ...
eerhardt opened this issue Nov 10, 2020 · 34 comments
Closed
Tracked by #43543 ...

Developers can safely trim apps which need Configuration Binder #44493

eerhardt opened this issue Nov 10, 2020 · 34 comments
Assignees
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Configuration Priority:2 Work that is important, but not critical for the release Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@eerhardt
Copy link
Member

eerhardt commented Nov 10, 2020

Edited by @layomia.

Background

Application configuration in ASP.NET Core is performed using one or more configuration providers. Configuration providers read data (as key-value pairs) using a variety of sources such as settings files (e.g. appsettings.json), environment variables, Azure Key Vault etc.

At the core of this mechanism is ConfigurationBinder, an extension class in platform extensions that provides Bind and Get methods that maps configuration values (IConfiguration instances) to strongly-typed objects. Bind takes an instance, while Get creates one on behalf of the caller. The current approach currently uses reflection which causes issues for trimming and Native AOT.

Here's example of code that users write to invoke the binder:

using Microsoft.AspNetCore.Builder;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.DependencyInjection;

// ASP.NET Core apps configure and launch a host, responsible for app startup and lifetime management.
// Templates create a WebApplicationBuilder which contains the host and provides default configuration
// for the app that may be specified by users through the mechanisms shown above.
// `args` contains configuration values specified through command line
WebApplicationBuilder builder = WebApplication.CreateBuilder(args);

// Represents a section of application configuration values (key-value pairs).
// Implements `IConfiguration`.
IConfigurationSection section = builder.Configuration.GetSection("MyOptions");

// !! Configure call - to be replaced with source-gen'd implementation
// `builder.Services`: a collection of user or framework-provided services for the application to compose
// `Configure<MyOptions>(section)`: adds the configuration section to the service collection for use in their operations
// `MyOptions`: configuration shape/type that maps to command-line specified config values
builder.Services.Configure<MyOptions>(section);

// !! Get call - to be replaced with source-gen'd implementation
// Get a new instance of your options using values from the config section
MyOptions options0 = section.Get<MyOptions>();

// Bind config options to an existing instance
MyOptions options1 = new MyOptions();
section.Bind(myOptions1);

// Build the application.
WebApplication app = builder.Build();
// Specify a http GET operation
app.MapGet("/", () => "Hello World!");
// Run the app
app.Run();

// Configuration classes mapping to user-provided values
public class MyOptions
{
    public int A { get; set; }
    public string S { get; set; }
    public byte[] Data { get; set; }
    public Dictionary<string, string> Values { get; set; }
    public List<MyClass> Values2 { get; set; }
}

public class MyClass
{
    public int SomethingElse { get; set; }
}

Configuration binding source generator in .NET 8

In .NET 8, as a replacement to reflection, we wish to ship a source generator that generates static code to bind config values to target user types. We are proposing an implicit mechanism where we probe for Configure, Bind, and Get calls that we can retrieve type info from.

Given the sample user code above, here's the code that would be generated:

using System.Collections.Generic;
using Microsoft.AspNetCore.Builder;
using Microsoft.Extensison.DependencyInjection;

public static class GeneratedConfigurationBinder
{
    public static IServiceCollection Configure<T>(this IServiceCollection services, IConfiguration configuration)
    {
        if (typeof(T) == typeof(MyOptions))
        {
            // Redirect to generated Bind method.
            return services.Configure<MyOptions>(obj => Bind(configuration, obj));
        }

        // Repeat if-check for all input types.

        throw new NotSupportedException("The source generator did not detect this type as input.");
    }

    public static T? Get<T>(this global::Microsoft.Extensions.Configuration.IConfiguration configuration)
    {
        if (typeof(T) == typeof(MyOptions))
        {
            MyOptions obj = new();
            Bind(configuration, obj);
            return (T)(object)obj;
        }

        // Repeat if-check for all input types.

        throw new NotSupportedException("The source generator did not detect this type as input.");
    }

    internal static void Bind(this IConfiguration configuration, MyOptions obj)
    {
        if (obj is null)
        {
            throw new ArgumentNullException(nameof(obj));
        }
        
        // Preliminary code to validate input configuration.

        obj.A = configuration["A"] is String stringValue0 ? int.Parse(stringValue4) : default;
        obj.S = configuration["S"] is String stringValue1 ? bool.Parse(stringValue5) : default;
        obj.Values ??= new();
        Bind(configuration.GetSection("Values"), obj.Values);
        obj.Values2 ?? = new();
        Bind(configuration.GetSection("Values2"), obj.Values2);
    }

    private static void Bind(this IConfiguration configuration, Dictionary<string, string> obj)
    {
        if (obj is null)
        {
            throw new global::System.ArgumentNullException(nameof(obj));
        }

        string key;
        string element;

        foreach (Microsoft.Extensions.Configuration.IConfigurationSection section in configuration.GetChildren())
        {
            key = section["Key"] is string stringValue2 ? stringVal2 : default;
            element = section["Value"] is string stringValue3 ? stringValue3 : default;
            obj[key] = element;
        }
    }

    private static void Bind(this IConfiguration configuration, List<MyClass> obj)
    {
        throw new NotSupportedException("This type is not supported.");
    }
}

Generator kick-off gesture

We want the user experience to be easy and minimal. Starting off, the generator will simply probe for a compiler visible property called UseConfigurationBinderSourceGenerator to determine whether to run. This allows folks that don't want the generator or for which it doesn't work for (yet) to skip using it.

<ItemGroup>
    <CompilerVisibleProperty Include="UseConfigurationBinderSourceGenerator" />
</ItemGroup>

We'll add this as an SDK property later.

Generation strategy:

The operation is to convert IConfiguration instances into target config types. We'll do this by generate bespoke parsing/mapping logic for each target type. This is similar to a "fast-path" deserialization feature that the System.Text.Json source generator could implement (#55043).

PR: #82179

Integration with user apps

The code is generated into the global namespace and the methods are picked over the reflection by the compiler.

  • As a next step we want to use the call site replace feature being developed by Roslyn to replace user calls with generated calls directly.

Technical details

  • This approach satisfies the direct-call scenarios in user apps. As a next step we need to design a solution for indirect usage, e.g. calls the ASP.NET makes on behalf of users. A design isn't fully laid out yet but early ideas are to have some sort of global cache from implicit registration of generated code.

What's next

#79527


Related docs

Initial design

@eerhardt eerhardt added area-Extensions-Configuration User Story A single user-facing feature. Can be grouped under an epic. Priority:2 Work that is important, but not critical for the release labels Nov 10, 2020
@ghost
Copy link

ghost commented Nov 10, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.


Issue meta data

Issue content: Source Generators are a new technology that was added to Roslyn in the .NET 5 timeframe, but we have not yet utilized them by creating generators to assist with runtime and framework features. We will work to define customer scenarios in more detail here with customer development.

Code that uses runtime reflection and reflection.emit causes issues for Trimming and Native AOT. Source generators can be used to generate alternative code at build time that is static so can be used in conjunction with Trimming and AOT. Work on trimming ASP.NET apps has revealed where there are current limitations that can be solved by source generators.

This item tracks creating a Source Generator for ConfigurationBinder, which binds strongly-typed objects to configuration values.

cc @maryamariyan @davidfowl @ericstj @samsp-msft

Issue author: eerhardt
Assignees: -
Milestone: -

@eerhardt
Copy link
Member Author

@danroth27 @davidfowl - I don't see ConfigurationBinder being used on the WASM side of a Blazor application. Would a source generator only be useful for a server side ASP.NET app? Or is it useful for Blazor WASM as well?

I see this work item as being of the same importance as #44432, so I marked it as Priority 2 as well. Any thoughts there?

@danroth27
Copy link
Member

@eerhardt Users could decide to use ConfigurationBinder themselves, but as far as I know it isn't commonly used. So I agree that this is really more about ASP.NET Core than Blazor.

@maryamariyan maryamariyan added the untriaged New issue has not been triaged by the area owner label Nov 11, 2020
@maryamariyan
Copy link
Member

Linking this issue to #43930 and #36130

@maryamariyan maryamariyan added this to the 6.0.0 milestone Nov 11, 2020
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Nov 11, 2020
@7sharp9
Copy link

7sharp9 commented Nov 17, 2020

How would this be consumed by non C# languages?

@marek-safar marek-safar changed the title Source Generator for Configuration Binder so apps can be trimmed, and native AOT compiled Developer can safely trim apps which need Configuration Binder Nov 27, 2020
@marek-safar marek-safar changed the title Developer can safely trim apps which need Configuration Binder Developers can safely trim apps which need Configuration Binder Nov 27, 2020
@eerhardt eerhardt modified the milestones: 6.0.0, Future Jul 21, 2021
@eerhardt eerhardt removed their assignment Aug 4, 2021
@maryamariyan maryamariyan removed their assignment Feb 27, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 15, 2023
@layomia layomia added api-suggestion Early API idea and discussion, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Feb 15, 2023
@bartonjs
Copy link
Member

  • The top post says the generated class is public. It should be internal.
  • The proposed generated class's non-private members all look like the existing methods, so they're all reasonable.
  • The generated type should consider being [EditorBrowsable(Never)]
  • There may be bad UX edge cases that need to be investigated, such as
    • The roslyn analyzers report an unused using-import from where the existing extension methods are (might be topically unlikely)
    • Concerns around callers using static invocation syntax instead of instance/extension syntax.
    • The generator might end up toggling itself on and off (by removing the calls to the existing methods)

Marking as needs-work because a lot of this is contingent on a non-released compiler feature; but that doesn't block experimenting with it as-adjusted in previews.

@bartonjs bartonjs added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Feb 21, 2023
@chsienki
Copy link
Contributor

Generator kick-off gesture

Want to call this out as probably not the right approach. It's pretty difficult to 'disable' a generator based on a property it reads in, and you end up with a lot of complicated logic (we do this in Razor for other reasons and it's not pleasant). From a performance POV you're also still doing a (small) amount of work even when it doesn't run.

Instead, you should adjust the logic in ResolveTargetingPackAssets (or add another target that runs after it) to add/remove the generator being passed as part of the Analyzers collection altogether, based on the property provided by the user. This keeps the generator simpler, and ensure the least amount of work is done when it's not in use.

@layomia
Copy link
Contributor

layomia commented Feb 21, 2023

Thanks @chsienki I'll look into this approach. FYI @ericerhardt

Want to call this out as probably not the right approach

Does the source generator cookbook need to be updated then?

we do this in Razor for other reasons and it's not pleasant

Can you pls point me to where this is done? (or cc @captainsafia if you know...)

@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 21, 2023
@chsienki
Copy link
Contributor

@layomia It's totally valid to use CompilerVisibleProperty as described in the cookbook, I'm just saying for this scenario it's not very useful. It's better to just not pass the generator in, instead of trying to supress it in code.

The razor generator does suppression via CompilerVisibleProperty but it can't switch to the other mechanism due to some hot reload weirdness that we're fixing. When thats done, we'll use the 'don't pass it' strategy too.

@captainsafia has been working on a different generator which uses the 'don't pass it' strategy that you can probably re-use almost wholesale.

@captainsafia
Copy link
Member

@captainsafia has been working on a different generator which uses the 'don't pass it' strategy that you can probably re-use almost wholesale.

You can find a sample of the strategy used for the Minimal APIs generator at this PR: dotnet/sdk#29982

@ericstj
Copy link
Member

ericstj commented Feb 22, 2023

I think one of the challenges faced with this one is which targets should get the removal: https://github.com/dotnet/sdk/blob/45a14239002eb0a6f8ea6096d756d882a0cb8208/src/WebSdk/Web/Sdk/Sdk.props#L71-L73. This generator is not part of an SDK. It's associated with a library that's in the ASP.NET shared framework and also part of a NuGet package (to be used outside of ASP.NET). Do you recommend we put them in the .NET SDK and the Microsoft.Extensions.Configuration.Binder nuget package?

@andrewlock
Copy link
Contributor

andrewlock commented May 16, 2023

One potential issue I can foresee with the design of the generated code (because I've run into a similar issue with my own generators) is around internal classes and InternalsVisibleTo.

The fact that the generated code is always a fixed type (GeneratedConfigurationBinder) in the global namespace, means that something like this will no longer compile:

Project 1:

[assembly:InternalsVisible2(Project2)]
var config = new ConfigurationBuilder().Build();
var settings = new SomeOptions();
section.Bind(settings);

Project 2: (has a <ProjectReference> to Project1)

var config = new ConfigurationBuilder().Build();
var settings = new MoreOptions();
section.Bind(settings); // 👈 error

Gives

[CS0121] The call is ambiguous between the following methods or properties: 'GeneratedConfigurationBinder.Configure<T>(Microsoft.Extensions.DependencyInjection.IServiceCollection, Microsoft.Extensions.Configuration.IConfiguration)' and 'GeneratedConfigurationBinder.Configure<T>(Microsoft.Extensions.DependencyInjection.IServiceCollection, Microsoft.Extensions.Configuration.IConfiguration)'

You might consider it an edge case, but it was one of the first issues I ran into when building my first source generator.

Note: I've just seen that preview 4 bits are going onto NuGet, so apologies if this is no longer relevant! :D (Edit: looks like the preview 3 package is broken for me)

@layomia
Copy link
Contributor

layomia commented May 17, 2023

Thanks @andrewlock - I've opened #86363 to address this.

@davidfowl
Copy link
Member

This was the only edge case I found and it does feel like an edge case. That said, we can figure out a cleaner way to handle it.

@layomia
Copy link
Contributor

layomia commented Jul 21, 2023

Closing this issue given the feature is largely implemented and is getting integrated up-stack. Remaining work is tracked in #79527 and this query.

@layomia layomia closed this as completed Jul 21, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Configuration Priority:2 Work that is important, but not critical for the release Team:Libraries User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet
Development

No branches or pull requests