ConfigurationBinder Source Generator doesn't support IEnumerable types with same functionality as reflection based binder

[eerhardt]

Description

When a class implements IEnumerable, but not ICollection<T>, the ConfigurationBinder source generator is emitting code with different behavior than the reflection based binder uses.

Reproduction Steps

csproj:

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

  <PropertyGroup>
    <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="Confluent.Kafka" Version="2.3.0" />
  </ItemGroup>
</Project>

appsettings.json:

{
  "Config": {
    "DeliveryReportFields": "one,two,three",
    "EnableDeliveryReports": false
  }
}

Program.cs:

using Confluent.Kafka;

var builder = Host.CreateApplicationBuilder(args);
var producerConfig = builder.Configuration.GetSection("Config").Get<ProducerConfig>()!;

Console.WriteLine($"DeliveryReportFields: '{producerConfig.DeliveryReportFields}', should be 'one,two,three'");
Console.WriteLine($"EnableDeliveryReports: '{producerConfig.EnableDeliveryReports}', should be 'False'");

Toggle the EnableConfigurationBindingGenerator setting between true and false.

Expected behavior

The behavior of the app should be the same whether you are setting EnableConfigurationBindingGenerator to false or true.

Actual behavior

When the Source Generator is not used:

DeliveryReportFields: 'one,two,three', should be 'one,two,three'
EnableDeliveryReports: 'False', should be 'False'

When the Source Generator is used:

Unhandled exception. System.NotSupportedException: Unable to bind to type 'Confluent.Kafka.ProducerConfig': generator did not detect the type as input.
   at Microsoft.Extensions.Configuration.Binder.SourceGeneration.<BindingExtensions_g>F40008862DA0CE1CF1C594A476014B455F2B40704BA5739597A906A2663453198__BindingExtensions.GetCore(IConfiguration configuration, Type type, Action`1 configureOptions) in C:\DotNetTest\ConfigBinderTest\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs:line 57
   at Microsoft.Extensions.Configuration.Binder.SourceGeneration.<BindingExtensions_g>F40008862DA0CE1CF1C594A476014B455F2B40704BA5739597A906A2663453198__BindingExtensions.Get[T](IConfiguration configuration) in C:\DotNetTest\ConfigBinderTest\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs:line 35
   at Program.<Main>$(String[] args) in C:\DotNetTest\ConfigBinderTest\Program.cs:line 4

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

It appears the difference is this code:

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

Lines 781 to 782 in f21dc6c

	private bool IsCollection(ITypeSymbol type) =>
	type is INamedTypeSymbol namedType && GetInterface(namedType, _typeSymbols.IEnumerable) is not null;

The source generator considers anything that implements IEnumerable to be a "collection".

vs the reflection-based binder, which only considers IDictionary and ICollection<T>:

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 408 to 426 in f21dc6c

	// At this point we know that we have a non-null bindingPoint.Value, we just have to populate the items
	// using the IDictionary<> or ICollection<> interfaces, or properties using reflection.
	Type? dictionaryInterface = FindOpenGenericInterface(typeof(IDictionary<,>), type);
	if (dictionaryInterface != null)
	{
	BindDictionary(bindingPoint.Value, dictionaryInterface, config, options);
	}
	else
	{
	Type? collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), type);
	if (collectionInterface != null)
	{
	BindCollection(bindingPoint.Value, collectionInterface, config, options);
	}
	else
	{
	BindProperties(bindingPoint.Value, config, options);
	}

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

Issue Details

---

Description

When a class implements IEnumerable, but not ICollection<T>, the ConfigurationBinder source generator is emitting code with different behavior than the reflection based binder uses.

Reproduction Steps

csproj:

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

  <PropertyGroup>
    <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="Confluent.Kafka" Version="2.3.0" />
  </ItemGroup>
</Project>

appsettings.json:

{
  "Config": {
    "DeliveryReportFields": "one,two,three",
    "EnableDeliveryReports": false
  }
}

Program.cs:

using Confluent.Kafka;

var builder = Host.CreateApplicationBuilder(args);
var producerConfig = builder.Configuration.GetSection("Config").Get<ProducerConfig>()!;

Console.WriteLine($"DeliveryReportFields: '{producerConfig.DeliveryReportFields}', should be 'one,two,three'");
Console.WriteLine($"EnableDeliveryReports: '{producerConfig.EnableDeliveryReports}', should be 'False'");

Toggle the EnableConfigurationBindingGenerator setting between true and false.

Expected behavior

The behavior of the app should be the same whether you are setting EnableConfigurationBindingGenerator to false or true.

Actual behavior

When the Source Generator is not used:

DeliveryReportFields: 'one,two,three', should be 'one,two,three'
EnableDeliveryReports: 'False', should be 'False'

When the Source Generator is used:

Unhandled exception. System.NotSupportedException: Unable to bind to type 'Confluent.Kafka.ProducerConfig': generator did not detect the type as input.
   at Microsoft.Extensions.Configuration.Binder.SourceGeneration.<BindingExtensions_g>F40008862DA0CE1CF1C594A476014B455F2B40704BA5739597A906A2663453198__BindingExtensions.GetCore(IConfiguration configuration, Type type, Action`1 configureOptions) in C:\DotNetTest\ConfigBinderTest\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs:line 57
   at Microsoft.Extensions.Configuration.Binder.SourceGeneration.<BindingExtensions_g>F40008862DA0CE1CF1C594A476014B455F2B40704BA5739597A906A2663453198__BindingExtensions.Get[T](IConfiguration configuration) in C:\DotNetTest\ConfigBinderTest\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs:line 35
   at Program.<Main>$(String[] args) in C:\DotNetTest\ConfigBinderTest\Program.cs:line 4

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

It appears the difference is this code:

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

Lines 781 to 782 in f21dc6c

	private bool IsCollection(ITypeSymbol type) =>
	type is INamedTypeSymbol namedType && GetInterface(namedType, _typeSymbols.IEnumerable) is not null;

The source generator considers anything that implements IEnumerable to be a "collection".

vs the reflection-based binder, which only considers IDictionary and ICollection<T>:

runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

Lines 408 to 426 in f21dc6c

	// At this point we know that we have a non-null bindingPoint.Value, we just have to populate the items
	// using the IDictionary<> or ICollection<> interfaces, or properties using reflection.
	Type? dictionaryInterface = FindOpenGenericInterface(typeof(IDictionary<,>), type);
	if (dictionaryInterface != null)
	{
	BindDictionary(bindingPoint.Value, dictionaryInterface, config, options);
	}
	else
	{
	Type? collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), type);
	if (collectionInterface != null)
	{
	BindCollection(bindingPoint.Value, collectionInterface, config, options);
	}
	else
	{
	BindProperties(bindingPoint.Value, config, options);
	}

Author:	eerhardt
Assignees:	-
Labels:	area-Extensions-Configuration
Milestone:	-

[eiriktsarpalis]

For context, this is the API of the class in question: https://docs.confluent.io/platform/7.5/clients/confluent-kafka-dotnet/_site/api/Confluent.Kafka.ProducerConfig.html

The source generator considers anything that implements IEnumerable to be a "collection".

vs the reflection-based binder, which only considers IDictionary and ICollection:

It appears then that the source generator is emulating convention already followed in STJ where everything deriving from IEnumerable is regarded as a collection type. Apart from it being inconsistent with the reflection-based implementation, I don't think that this is an incorrect convention: collection initializers/collection literals in C# follow a similar convention wherein a type must implement IEnumerable (+ have a default constructor and a public Add method) for it to be regarded as a collection type.

It seems to me that whether we decided to have the SG implementation mirror reflection semantics or vice versa, wouldn't it be a breaking change to users relying on the current behavior in each case?

[eerhardt]

Apart from it being inconsistent with the reflection-based implementation, I don't think that this is an incorrect convention

Being inconsistent with the reflection-based implementation is what makes it incorrect. The source generator's behavior should match the reflection-based binder.

must implement IEnumerable (+ have a default constructor and a public Add method)

Note that the Type in the repro steps doesn't have an Add method.

It seems to me that whether we decided to have the SG implementation mirror reflection semantics or vice versa, wouldn't it be a breaking change to users relying on the current behavior in each case?

I don't think we should change any behavior of the reflection-based binder.

Changing the SG behavior might be a breaking change, but I think it is the right change to match the reflection-based binder. That was the intention of the SG from the beginning. We could do it so if the SG doesn't find an Add method (which today it doesn't generate any code for the type), only then fall back to the "Object/Properties" non-collection behavior. That shouldn't be a breaking change. It would go from "not supported" to "supported".

[ericstj]

The reflection binder has the following criteria:

• IsArray

  runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

  Line 323 in f21dc6c

  if (type.IsArray || IsImmutableArrayCompatibleInterface(type))

• Is (not implements) IEnumerable<>, IList<>, or IReadOnlyList<>

  runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

  Lines 941 to 943 in f21dc6c

  	return genericTypeDefinition == typeof(IEnumerable<>)
  	|| genericTypeDefinition == typeof(IReadOnlyCollection<>)
  	|| genericTypeDefinition == typeof(IReadOnlyList<>);

• Is ISet<> or IReadOnlySet<>

  runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

  Lines 951 to 953 in f21dc6c

  	return genericTypeDefinition == typeof(ISet<>)
  	#if NETCOREAPP
  	|| genericTypeDefinition == typeof(IReadOnlySet<>)

• Is IDictionary<,> or IReadOnlyDictionary<,>

  runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

  Lines 932 to 933 in f21dc6c

  	return genericTypeDefinition == typeof(IDictionary<,>)
  	|| genericTypeDefinition == typeof(IReadOnlyDictionary<,>);

• Is IList<> or ICollection<>

  runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

  Line 394 in f21dc6c

  (interfaceGenericType == typeof(ICollection<>) || interfaceGenericType == typeof(IList<>)))

• Runtime type implements IDictionary<,>

  runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

  Line 410 in f21dc6c

  Type? dictionaryInterface = FindOpenGenericInterface(typeof(IDictionary<,>), type);

• Runtime type implements ICollection<>

  runtime/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs

  Line 418 in f21dc6c

  Type? collectionInterface = FindOpenGenericInterface(typeof(ICollection<>), type);

Ultimately I think the source generator has similar criteria, but the reflection binder will fall back to trying to treat the type as a POCO if it can't find the ICollection/IDictionary or Add method whereas the source generator does not.

I'll see if it's possible to have the source generator fallback to treating the type as a poco.