search

dotnet / runtime

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

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

Closed eerhardt closed 1 month ago

eerhardt commented 8 months ago

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:

https://github.com/dotnet/runtime/blob/f21dc6c3dceb6ea76bef73e2a026c770aaed3b5e/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs#L781-L782

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

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

https://github.com/dotnet/runtime/blob/f21dc6c3dceb6ea76bef73e2a026c770aaed3b5e/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs#L408-L426

ghost commented 8 months ago

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`, the ConfigurationBinder source generator is emitting code with different behavior than the reflection based binder uses. ### Reproduction Steps csproj: ```xml net8.0 enable enable true ``` appsettings.json: ```json { "Config": { "DeliveryReportFields": "one,two,three", "EnableDeliveryReports": false } } ``` Program.cs: ```C# using Confluent.Kafka; var builder = Host.CreateApplicationBuilder(args); var producerConfig = builder.Configuration.GetSection("Config").Get()!; 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.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.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.
$(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: https://github.com/dotnet/runtime/blob/f21dc6c3dceb6ea76bef73e2a026c770aaed3b5e/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/ConfigurationBindingGenerator.Parser.cs#L781-L782 The source generator considers anything that implements IEnumerable to be a "collection". vs the reflection-based binder, which only considers `IDictionary` and `ICollection`: https://github.com/dotnet/runtime/blob/f21dc6c3dceb6ea76bef73e2a026c770aaed3b5e/src/libraries/Microsoft.Extensions.Configuration.Binder/src/ConfigurationBinder.cs#L408-L426
Author: eerhardt
Assignees: -
Labels: `area-Extensions-Configuration`
Milestone: -
eiriktsarpalis commented 8 months ago

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 commented 8 months ago

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 commented 1 month ago

The reflection binder has the following criteria:

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.