dotnet / runtime

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

[API Proposal]: [ReflectionLessBoundableConfiguration] #108111

Open rmannibucau opened 1 month ago

rmannibucau commented 1 month ago

Background and motivation

Supporting AOT for the binder API is done using a source generator but dotnet compiler has only 1 (2) passes so it makes it impossible to use it within another generator forcing the other generator to reimplement pretty much the same logic.

It would be neat to have a middle-ground solution.

API Proposal

namespace Whatever;

[ReflectionLessBoundableConfiguration]
public class ConfigClass
{
//...
}

side note: i'm not fan of the name but guess it is sufficient to get started, what is important is to have a marker attribute

Then it would generate a ConfigClass.Binder class which would be usable as binder (ConfigClass.Binder.Bind(configuration, instance))

API Usage

// Fancy the value
var c = new MyFancyCollection<int>();
ConfigClass.Binder.Bind(new ConfigurationBuilder()...., c)

Alternative Designs

Enable to call the generator from another generator (but it is fishy as design and will lead to a spaghetti solution)

Risks

no real risk there (except if you already have a binder nested class but if so you don't use this API which is new so risk is really 0)

dotnet-policy-service[bot] commented 1 month ago

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

KalleOlaviNiemitalo commented 1 month ago

This would have to be public partial class ConfigClass, right? So that the source generator can add another part that defines the nested class ConfigClass.Binder.

A potential problem is if the class already has a property named Binder; then it cannot have a nested class with the same name. Could be solved with [ReflectionLessBoundableConfiguration(NestedTypeName = "AlternativeBinder")].

rmannibucau commented 1 month ago

@KalleOlaviNiemitalo not sure it needs partial, if it generates a nested class by convention this is sufficient and less dev work. It can be needed for cases you want to explicitly use that factory from user code (and not generators as my case). For the conflicting name issue I guess starting with what I mentionned, ie there will be no issue and not a single case where it can happen, is sufficient (since it is a new API which enables explicitly the feature it shouldn't occur), but no issue to enable to name it as you proposed too.

KalleOlaviNiemitalo commented 1 month ago

I don't see how the source generator can inject the nested class if the outer class is not partial. What kind of source code would it generate?

rmannibucau commented 1 month ago

@KalleOlaviNiemitalo what I had in mind was something along the line var conf = MyModel.Binder.Bind(iconfiguration, new MyModel()); or even the shortcut var conf = MyModel.Binder.Bind(iconfiguration);. If you want to to explicit the config class you need to add it in the attribute (which must be on the model not the reflectionless "factory" to ensure the classes are identifiable, in particular if you enable to break the binder convention).

side note: happy to get another solution, the current source generator is really good, it just doesn't work with other source generators so a more elegant solution would be welcomed - even if it means using dotnet 10 ;).

KalleOlaviNiemitalo commented 1 month ago

I mean, if the application source code contains

[ReflectionLessBoundableConfiguration]
public class ConfigClass
{
    public string Property1 { get; set; }
}

and the source generator generates code like

partial class ConfigClass
{
    public class Binder
    {
        public static ConfigClass Bind(IConfiguration configuration, ConfigClass? instance)
        {
            instance ??= new ConfigClass();
            instance.Property1 = configuration["Property1"];
            return instance;
        }
    }
}

then the build will fail:

error CS0260: Missing partial modifier on declaration of type 'ConfigClass'; another partial declaration of this type exists

rmannibucau commented 1 month ago

@KalleOlaviNiemitalo I see, had more a static class in mind and thought the partial was avoidable there but if not it is ok for me to add partial for that need, good catch.