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

API Proposal: Add ConfigurationBinder.Bind<T> overloads #43930

Closed
davidfowl opened this issue Oct 28, 2020 · 12 comments
Closed

API Proposal: Add ConfigurationBinder.Bind<T> overloads #43930

davidfowl opened this issue Oct 28, 2020 · 12 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Configuration
Milestone

Comments

@davidfowl
Copy link
Member

davidfowl commented Oct 28, 2020

Background and Motivation

We need to enable annotating liker friendly attributes to be applied to the generic argument to preserve things and to allow source generators to replace the implementation of this method by adding generic overloads specialized to a specific T.

Proposed API

namespace Microsoft.Extensions.Configuration
{
    public class ConfigurationBinder 
    {
+        public static void Bind<T>(this IConfiguration configuration, string key, T instance);
+        public static void Bind<T>(this IConfiguration configuration, T instance);
+        public static void Bind<T>(this IConfiguration configuration, T instance, Action<BinderOptions> configureOptions);
    }

Usage Examples

using System;
using System.Collections.Generic;
using Microsoft.Extensions.Configuration;

var configuration = new ConfigurationBuilder()
    .AddInMemoryCollection(new Dictionary<string, string>
    {
        { "Name", "David" }
    })
    .Build();

var p = new Person();
// This will infer the new generic overloads instead of the object overload
configuration.Bind(p);

Console.WriteLine(p.Name);

public class Person
{
    public string Name { get; init; }
}

Risks

None.

PS: These will basically win over the non-generic overloads when the instance isn't typed as object.

cc @jaredpar to check my compiler math.

@davidfowl davidfowl added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Oct 28, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-Extensions-Configuration extensions-port untriaged New issue has not been triaged by the area owner labels Oct 28, 2020
@ghost
Copy link

ghost commented Oct 28, 2020

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

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Oct 28, 2020
@maryamariyan maryamariyan added this to the 6.0.0 milestone Oct 28, 2020
@maryamariyan maryamariyan added 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 Oct 29, 2020
@maryamariyan
Copy link
Member

maryamariyan commented Nov 19, 2020

cc @jaredpar to check my compiler math.

I believe given the concrete class:

public class Person
{
    public string Name { get; set; }
}

the two overloads below in combination

// The APIs proposed in this issue. So when source gen is not enabled, config binding would remain linker friendly
public static class ConfigurationBinder
{
    public static void Bind<T>(this IConfiguration configuration, T instance, Action<BinderOptions> configureOptions)
        => configuration.Bind((object)instance, configureOptions);
...
}

// source generated code for the concrete types, with compiler ideally picking this overload rather than ambiguous error
public static class PersonConfigurationExensions
{
    public static void Bind<Person>(this IConfiguration configuration, Person instance, Action<BinderOptions> configureOptions)
        => configuration.Bind((object)instance, configureOptions);
...
}

would cause ambiguous compile error:

The call is ambiguous between the following methods or properties: 'ConfigurationBinder.Bind<T>(IConfiguration, T, Action<BinderOptions>)' and 'PersonConfigurationExensions.Bind<Person>(IConfiguration, Person, Action<BinderOptions>)'

@davidfowl would it be enough for configuration binding to be linker friendly only when source generation is enabled? If yes, then perhaps we could skip these proposed API additions (additions drafted here) and source gen drafted here.

@davidfowl
Copy link
Member Author

This works for me:

// The APIs proposed in this issue. So when source gen is not enabled, config binding would remain linker friendly
public static class ConfigurationBinder
{
    public static void Bind<T>(this IConfiguration configuration, T instance, Action<BinderOptions> configureOptions)
        => configuration.Bind((object)instance, configureOptions);
}

// source generated code for the concrete types, with compiler ideally picking this overload rather than ambiguous error
public static class PersonConfigurationExensions
{
    public static void Bind<T>(this IConfiguration configuration, T instance, Action<BinderOptions> configureOptions) where T : Person
    {
             // Source generated implementation
    }
}

@maryamariyan
Copy link
Member

Thanks, I missed adding the where clause in the code blob above, that is needed.

What I noticed is that we'd still get the ambiguous error even if the two APIs are in two different namespaces and both are added as using statements. Minimal repro here

@jaredpar
Copy link
Member

Yes this should be fine assuming all of the overloads today are non-generic.

@maryamariyan
Copy link
Member

maryamariyan commented Nov 19, 2020

UPDATE:

Note on trimming:

As described in the issue here, we'd still need the three APIs above for enabling linker friendly attributes.
But from what I understood from @eerhardt, seem like the linker today would have limitations (and needs additional work) to allow support for properly trimming bind for ComplexOptions below:

public class ComplexOptions
{
    public int Integer { get; set; }
    public NestedOptions Nested { get; set; }
}
public class NestedOptions
{
    public FurtherNestingOptions FurtherNesting { get; set; }
}

Where the linker needs to understand not to trim FurtherNestingOptions in this case.

Note on source generation

But seems like for the source gen code, we don't really need to conflict with the above three APIs. Instead we could generate something like:

        public static void Bind(this IConfiguration configuration, Person instance, Action<BinderOptions> configureOptions)

This way we can avoid the ambiguous match compile error.

@eerhardt
Copy link
Member

from what I understood from @eerhardt, seem like the linker today would have limitations (and needs additional work) to allow support for properly trimming bind for ComplexOptions below

See dotnet/linker#1087 for more information on the issue with the linker in this area.

@davidfowl
Copy link
Member Author

This is the case with JsonDeserialize as well. We just need to be able to annotate things (this should have the same annotations as JSON even if they are not fully capable today) since today we can't even do that because of the object overload.

@GrabYourPitchforks
Copy link
Member

If this API is approved, consider having the new overloads annotated where T : class, or if this is not practical, their implementations should fail if T is a value type. Passing a value type T will end up making a copy, so binding the configuration to that instance will essentially be an expensive no-op, and the user probably didn't intend for this to happen.

@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Nov 24, 2020
@terrajobst
Copy link
Member

terrajobst commented Nov 24, 2020

Video

  • What are the linker annotations?
  • Why do we need generic overloads? If we believe the compiler can always implicitly inferred, then it seems maybe we should able to do this inference by the linker as well. In other words, can we annotate the existing object based overloads? It seems local tracking is something the linker could be doing.
  • How many APIs like this do we have in the framework? If it's just a couple, adding generic overloads is fine, but if that's a pervasive pattern, than a change in the linker seems more useful.
namespace Microsoft.Extensions.Configuration
{
    public partial class ConfigurationBinder
    {
        public static void Bind<T>(this IConfiguration configuration, string key, T instance);
        public static void Bind<T>(this IConfiguration configuration, T instance);
        public static void Bind<T>(this IConfiguration configuration, T instance, Action<BinderOptions> configureOptions);
    }
}

@davidfowl
Copy link
Member Author

If this API is approved, consider having the new overloads annotated where T : class, or if this is not practical, their implementations should fail if T is a value type. Passing a value type T will end up making a copy, so binding the configuration to that instance will essentially be an expensive no-op, and the user probably didn't intend for this to happen.

@GrabYourPitchforks great observation!

@maryamariyan maryamariyan modified the milestones: 6.0.0, 7.0.0 Jul 23, 2021
@eerhardt eerhardt modified the milestones: 7.0.0, Future Jul 14, 2022
@davidfowl
Copy link
Member Author

Closing this in favor of the new source generator.

cc @layomia @ericstj

@eerhardt eerhardt closed this as not planned Won't fix, can't repro, duplicate, stale Jul 10, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 14, 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
Projects
None yet
Development

No branches or pull requests

7 participants