ConfigurationBinder source generator generates code that doesn't compile when using StringValues

[eerhardt]

Description

When an Options object uses the StringValues type, the ConfigurationBinder Source Generator is generating code that doesn't compile.

Reproduction Steps

Build the following app:

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

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

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="9.0.0-dev" />
    <PackageReference Include="Microsoft.Extensions.Configuration" Version="9.0.0-dev" />
  </ItemGroup>

</Project>

using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Primitives;

IConfigurationSection c = new ConfigurationBuilder().Build().GetSection("Options");
c.Get<MyOptions>();

public class MyOptions
{
    public IDictionary<string, StringValues> DefaultValues { get; set; } = new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase);
}

Expected behavior

The project should build successfully.

Actual behavior

The build fails with:

Severity	Code	Description	Project	File	Line	Suppression State	Details
Error (active)	CS8518	An expression of type 'StringValues' can never match the provided pattern.	ConsoleApp101	C:\Users\eerhardt\source\repos\ConsoleApp101\ConsoleApp101\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs	68		

The generated code that fails looks like:

        public static void BindCore(IConfiguration configuration, ref global::Microsoft.Extensions.Primitives.StringValues instance, bool defaultValueIfNotFound, BinderOptions? binderOptions)
        {
            if (instance is not ICollection<string> temp) // this line has the build error
            {
                return;
            }

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

[ghost]

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 an Options object uses the StringValues type, the ConfigurationBinder Source Generator is generating code that doesn't compile.

Reproduction Steps

Build the following app:

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

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

  <ItemGroup>
    <PackageReference Include="Microsoft.Extensions.Configuration.Binder" Version="9.0.0-dev" />
    <PackageReference Include="Microsoft.Extensions.Configuration" Version="9.0.0-dev" />
  </ItemGroup>

</Project>

using Microsoft.Extensions.Configuration;
using Microsoft.Extensions.Primitives;

IConfigurationSection c = new ConfigurationBuilder().Build().GetSection("Options");
c.Get<MyOptions>();

public class MyOptions
{
    public IDictionary<string, StringValues> DefaultValues { get; set; } = new Dictionary<string, StringValues>(StringComparer.OrdinalIgnoreCase);
}

Expected behavior

The project should build successfully.

Actual behavior

The build fails with:

Severity	Code	Description	Project	File	Line	Suppression State	Details
Error (active)	CS8518	An expression of type 'StringValues' can never match the provided pattern.	ConsoleApp101	C:\Users\eerhardt\source\repos\ConsoleApp101\ConsoleApp101\Microsoft.Extensions.Configuration.Binder.SourceGeneration\Microsoft.Extensions.Configuration.Binder.SourceGeneration.ConfigurationBindingGenerator\BindingExtensions.g.cs	68		

The generated code that fails looks like:

        public static void BindCore(IConfiguration configuration, ref global::Microsoft.Extensions.Primitives.StringValues instance, bool defaultValueIfNotFound, BinderOptions? binderOptions)
        {
            if (instance is not ICollection<string> temp) // this line has the build error
            {
                return;
            }

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

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

[ericstj]

Definitely the generator shouldn't emit code that doesn't compile. It can do that check at compile time since the struct must be non null and implements ICollection.

I wonder if we have an example of someone successfully using a StringValues type with configuration? Actually binding data to it. Maybe it requires collection like data to bind correctly?

[eerhardt]

This scenario was found in https://github.com/dotnet/extensions/blob/33abf0c4b36ac8ce5964938b42f06ca31f48d9f6/src/Libraries/Microsoft.AspNetCore.HeaderParsing/HeaderParsingOptions.cs#L16-L24. I tried different combinations of what the config could look like, but I wasn't able to get the reflection based config binder to bind it correctly. I could get a key added to the dictionary, but the StringValues value would always be empty.

I looked back in source history, and I don't see any indication that this property was supposed to actually be bound to configuration. All the places that touch it manually add it through code. Tagging some of the people on the original PR to see if they remember if HeaderParsingOptions.DefaultValues is supposed to be able to be bound to IConfiguration - @geeknoid @tekian @rafal-mz

[geeknoid]

This was not intended to be configured from JSON. This kind of configuration option is pretty essential to the behavior of the code, so it's not particularly compelling as a config options controlled from JSON.

[ericstj]

Confirmed that this is still reproducing in the latest codebase. I'll see about a fix.

[ericstj]

I see we have the same problem for Dictionary too. It's actually very specific to these two cases:

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

Lines 404 to 407 in ae30cba

	else if (_typeSymbols.GenericIDictionary is not null && GetInterface(type, _typeSymbols.GenericIDictionary_Unbound) is not null)
	{
	populationCastType = CollectionPopulationCastType.IDictionary;
	}

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

Lines 471 to 474 in ae30cba

	else if (_typeSymbols.GenericICollection is not null && GetInterface(type, _typeSymbols.GenericICollection_Unbound) is not null)
	{
	populationCastType = CollectionPopulationCastType.ICollection;
	}

Those are the only two places we identify a type as implementing an interface and need to cast to that interface to bind to it. I think we can fix this by omitting the cast check for a struct.

[ericstj]

In this particular case the reflection based binder will actually treat the type as ICollection and will try to add if data is present:

System.Reflection.TargetInvocationException
  HResult=0x80131604
  Message=Exception has been thrown by the target of an invocation.
  Source=System.Private.CoreLib
  StackTrace:
   at System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(Object obj, Span`1 copyOfArgs, BindingFlags invokeAttr) in System.Reflection\MethodBaseInvoker.cs:line 176
   at System.Reflection.MethodBaseInvoker.InvokeWithOneArg(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture) in System.Reflection\MethodBaseInvoker.cs:line 122
   at System.Reflection.MethodBase.Invoke(Object obj, Object[] parameters) in System.Reflection\MethodBase.cs:line 221
   at Microsoft.Extensions.Configuration.ConfigurationBinder.BindCollection(Object collection, Type collectionType, IConfiguration config, BinderOptions options) in C:\src\dotnet\runtime\src\libraries\Microsoft.Extensions.Configuration.Binder\src\ConfigurationBinder.cs:line 720

  This exception was originally thrown at this call stack:
    Microsoft.Extensions.Primitives.StringValues.System.Collections.Generic.ICollection<string>.Add(string)
    System.Reflection.MethodBaseInvoker.InvokeDirectByRefWithFewArgs(object, System.Span<object>, System.Reflection.BindingFlags) in MethodBaseInvoker.cs

Inner Exception 1:
NotSupportedException: Specified method is not supported.

That doesn't change what we do here, just adding some detail to help understand the scenario. This is a case where a user was never successfully binding to the type, but it was previously a noop and now it's a compile failure when trying to use the generator.

One way we could handle this better is to not even introduce the type-specific BindCore method when we see a type that we want to treat as it's collection interface. Instead we cast at the call-site. That would more closely match what the runtime binder does. It would also reduce code volume and fix #89043