ConfigurationBinder source generator generates non-compilable code with new property and internal setter

[eerhardt]

dotnet build on the following projects results in the generated source from the Configuration.Binder source generator to not compile.

Library

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <TargetFramework>net8.0</TargetFramework>
    <ImplicitUsings>enable</ImplicitUsings>
    <Nullable>enable</Nullable>
  </PropertyGroup>

</Project>

namespace ClassLibrary1;

public class CredentialDescription
{
    public string? Certificate { get; set; }
}

public class CertificateDescription : CredentialDescription
{
    public new string? Certificate
    {
        get
        {
            return base.Certificate;
        }
        protected internal set
        {
            base.Certificate = value;
        }
    }
}

App

<Project Sdk="Microsoft.NET.Sdk">

  <PropertyGroup>
    <OutputType>Exe</OutputType>
    <TargetFramework>net8.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
    <EnableConfigurationBindingGenerator>true</EnableConfigurationBindingGenerator>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Hosting" Version="8.0.0" />
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="8.0.1" />
  </ItemGroup>

  <ItemGroup>
    <ProjectReference Include="..\ClassLibrary1\ClassLibrary1.csproj" />
  </ItemGroup>
</Project>

using ClassLibrary1;
using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Hosting;

var builder = Host.CreateApplicationBuilder(args);

builder.Configuration.Bind(new CertificateDescription());

var host = builder.Build();
host.Run();

Expected result

I expect the code to compile. It may emit warnings if it was unable to bind properties correctly.

Actual result

C:\Users\eerhardt\source\repos\WorkerService20\WorkerService20\obj\Debug\net8.0\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.
Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs(65,17): error CS0272: The property or indexer '
CertificateDescription.Certificate' cannot be used in this context because the set accessor is inaccessible [C:\Users\eerhardt\source\repos\WorkerSer
vice20\WorkerService20\WorkerService20.csproj]
    0 Warning(s)
    1 Error(s)

Generated code

// <auto-generated/>

#nullable enable annotations
#nullable disable warnings

// Suppress warnings about [Obsolete] member usage in generated code.
#pragma warning disable CS0612, CS0618

namespace System.Runtime.CompilerServices
{
    using System;
    using System.CodeDom.Compiler;

    [GeneratedCode("Microsoft.Extensions.Configuration.Binder.SourceGeneration", "8.0.9.7805")]
    [AttributeUsage(AttributeTargets.Method, AllowMultiple = true)]
    file sealed class InterceptsLocationAttribute : Attribute
    {
        public InterceptsLocationAttribute(string filePath, int line, int column)
        {
        }
    }
}

namespace Microsoft.Extensions.Configuration.Binder.SourceGeneration
{
    using Microsoft.Extensions.Configuration;
    using System;
    using System.CodeDom.Compiler;
    using System.Collections.Generic;
    using System.Globalization;
    using System.Runtime.CompilerServices;

    [GeneratedCode("Microsoft.Extensions.Configuration.Binder.SourceGeneration", "8.0.9.7805")]
    file static class BindingExtensions
    {
        #region IConfiguration extensions.
        /// <summary>Attempts to bind the given object instance to configuration values by matching property names against configuration keys recursively.</summary>
        [InterceptsLocation(@"C:\Users\eerhardt\source\repos\WorkerService20\WorkerService20\Program.cs", 7, 23)]
        public static void Bind_CertificateDescription(this IConfiguration configuration, object? instance)
        {
            if (configuration is null)
            {
                throw new ArgumentNullException(nameof(configuration));
            }

            if (instance is null)
            {
                return;
            }

            var typedObj = (global::ClassLibrary1.CertificateDescription)instance;
            BindCore(configuration, ref typedObj, defaultValueIfNotFound: false, binderOptions: null);
        }
        #endregion IConfiguration extensions.

        #region Core binding extensions.
        private readonly static Lazy<HashSet<string>> s_configKeys_CertificateDescription = new(() => new HashSet<string>(StringComparer.OrdinalIgnoreCase) { "Certificate" });

        public static void BindCore(IConfiguration configuration, ref global::ClassLibrary1.CertificateDescription instance, bool defaultValueIfNotFound, BinderOptions? binderOptions)
        {
            ValidateConfigurationKeys(typeof(global::ClassLibrary1.CertificateDescription), s_configKeys_CertificateDescription, configuration, binderOptions);

            if (configuration["Certificate"] is string value0)
            {
                instance.Certificate = value0;
            }
        }


        /// <summary>If required by the binder options, validates that there are no unknown keys in the input configuration object.</summary>
        public static void ValidateConfigurationKeys(Type type, Lazy<HashSet<string>> keys, IConfiguration configuration, BinderOptions? binderOptions)
        {
            if (binderOptions?.ErrorOnUnknownConfiguration is true)
            {
                List<string>? temp = null;
        
                foreach (IConfigurationSection section in configuration.GetChildren())
                {
                    if (!keys.Value.Contains(section.Key))
                    {
                        (temp ??= new List<string>()).Add($"'{section.Key}'");
                    }
                }
        
                if (temp is not null)
                {
                    throw new InvalidOperationException($"'ErrorOnUnknownConfiguration' was set on the provided BinderOptions, but the following properties were not found on the instance of {type}: {string.Join(", ", temp)}");
                }
            }
        }
        #endregion Core binding extensions.
    }
}

cc @ericstj @tarekgh @eiriktsarpalis

[dotnet-policy-service]

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

[ericstj]

Ran some tests for comparison:

protected internal

C:\scratch\ee\app>dotnet build
Restore complete (1.2s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  lib succeeded (2.6s) → C:\scratch\ee\lib\bin\Debug\net8.0\lib.dll
  app failed with errors (0.9s)
    C:\scratch\ee\app\obj\Debug\net8.0\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs(65,17): error CS0272: The property or indexer 'CertificateDescription.Certificate' cannot be used in this context because the set accessor is inaccessible [C:\scratch\ee\app\app.csproj]

Build failed with errors in 5.4s

internal

C:\scratch\ee\app>dotnet build
Restore complete (0.4s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  lib succeeded (0.3s) → C:\scratch\ee\lib\bin\Debug\net8.0\lib.dll
  app failed with errors (0.2s)
    C:\scratch\ee\app\obj\Debug\net8.0\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs(65,17): error CS0200: Property or indexer 'CertificateDescription.Certificate' cannot be assigned to -- it is read only [C:\scratch\ee\app\app.csproj]

Build failed with errors in 1.3s

private

C:\scratch\ee\app>dotnet build
Restore complete (0.4s)
You are using a preview version of .NET. See: https://aka.ms/dotnet-support-policy
  lib succeeded (0.3s) → C:\scratch\ee\lib\bin\Debug\net8.0\lib.dll
  app failed with errors (0.2s)
    C:\scratch\ee\app\obj\Debug\net8.0\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs(65,17): error CS0200: Property or indexer 'CertificateDescription.Certificate' cannot be assigned to -- it is read only [C:\scratch\ee\app\app.csproj]

If I remove the new string? Certificate or change it's name to new string? Certificate2 it builds successfully without warnings.

So the bug here happens to be when a derived type hides a property of a base and changes visibility. Perhaps the source generator should be using the actual defining type of the member when accessing it? I wonder if we could even share logic in a type heirarchy - where a Bind method bound only the members of that type and called another Bind call for the base - down the chain. I think we have established behavior already to use the most derived member.

I recall some other issue where we explicitly considered "new" members and what the behavior should be - if we allow those to have precedence when binding or not. I wonder what our reflection binder does here - I think it would bind against the internal thing without any problem. Yep that's the case:

var cd = new CertificateDescription();
builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
{
    ["Certificate"] = "MyCertificate"
});
builder.Configuration.Bind(cd);
Console.WriteLine(cd.Certificate);

outputs:

MyCertificate

[ericstj]

@eiriktsarpalis - it looks like you addressed a similar issue in #87383

I think the bug in Configuration here is that its overwriting an existing property with one from a more base type:

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs

Line 673 in b621129

(properties ??= new(StringComparer.OrdinalIgnoreCase))[propertyName] = spec;

A minimal fix here would be to just add

if (properties?.ContainsKey(propertyName)) 
    continue;