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

When binding to interface collections, config generator shouldn't generate logic for both interface & mapping collection type. #89043

Open
layomia opened this issue Jul 17, 2023 · 2 comments
Labels
area-Extensions-Configuration enhancement Product code improvement that does NOT require public API changes/additions size-reduction Issues impacting final app size primary for size sensitive workloads source-generator Indicates an issue with a source generator feature
Milestone

Comments

@layomia
Copy link
Contributor

layomia commented Jul 17, 2023

Matching concrete type = actual type to instantiate when binding if needed, e.g.
IList -> List
IDictionary<,> -> Dictionary<,>


Using the Geolocation type from @martincostello's repro for #89010, if an interface such as IList<T> or IDictionary<,> is the target binding type, the generator emits logic for both the interfaces and matching config types e.g.

[Fact]
public void Struct_As_Dictionary_Element()
{
    var configuration = TestHelpers.GetConfigurationFromJsonString("""
        {
            "First":
            {
                "Latitude": 3,
                "Longitude": 4,
            }
        }
        """);

    Geolocation obj = configuration.Get<IDictionary<string, Geolocation>>()["First"];
    Assert.Equal(3, obj.Latitude);
    Assert.Equal(4, obj.Longitude);
}

Generated (buggy output per #89010):

public static void BindCore(IConfiguration configuration, ref IDictionary<string, ConfigurationBinderTests.Geolocation> obj, BinderOptions? binderOptions)
{
    if (obj is null)
    {
        throw new ArgumentNullException(nameof(obj));
    }

    foreach (IConfigurationSection section in configuration.GetChildren())
    {
        if (!(obj.TryGetValue(section.Key, out ConfigurationBinderTests.Geolocation element)))
        {
            element = InitializeConfigurationBinderTestsGeolocation(section, binderOptions);
        }
        var temp439 = InitializeConfigurationBinderTestsGeolocation(section, binderOptions);
        BindCore(section, ref temp439, binderOptions);
        element! = temp439;
        obj[section.Key] = element;
    }
}

public static void BindCore(IConfiguration configuration, ref Dictionary<string, ConfigurationBinderTests.Geolocation> obj, BinderOptions? binderOptions)
{
    if (obj is null)
    {
        throw new ArgumentNullException(nameof(obj));
    }

    foreach (IConfigurationSection section in configuration.GetChildren())
    {
        if (!(obj.TryGetValue(section.Key, out ConfigurationBinderTests.Geolocation element)))
        {
            element = InitializeConfigurationBinderTestsGeolocation(section, binderOptions);
        }
        var temp438 = InitializeConfigurationBinderTestsGeolocation(section, binderOptions);
        BindCore(section, ref temp438, binderOptions);
        element! = temp438;
        obj[section.Key] = element;
    }
}

We only need one of these methods.

@layomia layomia added enhancement Product code improvement that does NOT require public API changes/additions area-Extensions-Configuration size-reduction Issues impacting final app size primary for size sensitive workloads labels Jul 17, 2023
@layomia layomia added this to the 8.0.0 milestone Jul 17, 2023
@layomia layomia self-assigned this Jul 17, 2023
@ghost
Copy link

ghost commented Jul 17, 2023

Tagging subscribers to 'size-reduction': @eerhardt, @SamMonoRT, @marek-safar
See info in area-owners.md if you want to be subscribed.

Issue Details

Using the Geolocation type from @martincostello's repro for #89010, if a interface such as IList<T> or IDictionary<,> is the target binding type, the generator emits logic for both the interfaces and mapping config types e.g.

[Fact]
public void Struct_As_Dictionary_Element()
{
    var configuration = TestHelpers.GetConfigurationFromJsonString("""
        {
            "First":
            {
                "Latitude": 3,
                "Longitude": 4,
            }
        }
        """);

    Geolocation obj = configuration.Get<IDictionary<string, Geolocation>>()["First"];
    Assert.Equal(3, obj.Latitude);
    Assert.Equal(4, obj.Longitude);
}

Generated (buggy output per #89010):

public static void BindCore(IConfiguration configuration, ref IDictionary<string, ConfigurationBinderTests.Geolocation> obj, BinderOptions? binderOptions)
{
    if (obj is null)
    {
        throw new ArgumentNullException(nameof(obj));
    }

    foreach (IConfigurationSection section in configuration.GetChildren())
    {
        if (!(obj.TryGetValue(section.Key, out ConfigurationBinderTests.Geolocation element)))
        {
            element = InitializeConfigurationBinderTestsGeolocation(section, binderOptions);
        }
        var temp439 = InitializeConfigurationBinderTestsGeolocation(section, binderOptions);
        BindCore(section, ref temp439, binderOptions);
        element! = temp439;
        obj[section.Key] = element;
    }
}

public static void BindCore(IConfiguration configuration, ref Dictionary<string, ConfigurationBinderTests.Geolocation> obj, BinderOptions? binderOptions)
{
    if (obj is null)
    {
        throw new ArgumentNullException(nameof(obj));
    }

    foreach (IConfigurationSection section in configuration.GetChildren())
    {
        if (!(obj.TryGetValue(section.Key, out ConfigurationBinderTests.Geolocation element)))
        {
            element = InitializeConfigurationBinderTestsGeolocation(section, binderOptions);
        }
        var temp438 = InitializeConfigurationBinderTestsGeolocation(section, binderOptions);
        BindCore(section, ref temp438, binderOptions);
        element! = temp438;
        obj[section.Key] = element;
    }
}

We only need one of these methods.

Author: layomia
Assignees: layomia
Labels:

enhancement, area-Extensions-Configuration, size-reduction

Milestone: 8.0.0

@layomia layomia added the source-generator Indicates an issue with a source generator feature label Jul 17, 2023
@layomia layomia modified the milestones: 8.0.0, Future Jul 21, 2023
@tarekgh tarekgh modified the milestones: Future, 9.0.0 Nov 20, 2023
@ericstj
Copy link
Member

ericstj commented Aug 8, 2024

I looked at this and it will be more than just a small fix - we don't always generated duplicates - instead we should change what bind methods we produce when we know we will treat the concreate type as it's interface and move the interface cast up above the bindcore call.

@ericstj ericstj modified the milestones: 9.0.0, 10.0.0 Aug 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-Extensions-Configuration enhancement Product code improvement that does NOT require public API changes/additions size-reduction Issues impacting final app size primary for size sensitive workloads source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

No branches or pull requests

3 participants