Skip to content

Fix config binder source gen for a sole read-only collection ctor param #131358

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.

Sign up for GitHub

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

Open
TemRevil wants to merge 2 commits into
base: main
Choose a base branch
from
+125 −4
Open

Fix config binder source gen for a sole read-only collection ctor param #131358

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3aa4164
Fix config binder source gen for a sole read-only collection ctor param
TemRevil Jul 24, 2026
16c022b
Add functional runtime-binding coverage for the collection ctor param…
TemRevil Jul 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 17 additions & 4 deletions src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/BindingHelperInfo.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,17 @@ bool TryRegisterCore()
// an error for config properties that don't map to object properties.
_namespaces.Add("System.Collections.Generic");

if (_typeIndex.HasBindableMembers(objectSpec))
bool hasBindableMembers = _typeIndex.HasBindableMembers(objectSpec);

// A type with a parameterized constructor gets its constructor parameters bound
// in the Initialize method regardless of whether it also has other bindable
// members; that binding capability must be registered even when
// HasBindableMembers is false (e.g. the only member is a ctor parameter backed
// by a non-bindable read-only collection type), otherwise the emitter can end up
// calling an Initialize method that was never generated.
bool needsInitializeMethod = objectSpec is { InstantiationStrategy: ObjectInstantiationStrategy.ParameterizedConstructor, InitExceptionMessage: null };

if (hasBindableMembers || needsInitializeMethod)
{
foreach (PropertySpec property in objectSpec.Properties!)
{
Expand All @@ -189,10 +199,13 @@ bool TryRegisterCore()
}
}

bool registeredForBindCore = TryRegisterTypeForBindCoreGen(objectSpec);
Debug.Assert(registeredForBindCore);
if (hasBindableMembers)
{
bool registeredForBindCore = TryRegisterTypeForBindCoreGen(objectSpec);
Debug.Assert(registeredForBindCore);
}

if (objectSpec is { InstantiationStrategy: ObjectInstantiationStrategy.ParameterizedConstructor, InitExceptionMessage: null })
if (needsInitializeMethod)
{
RegisterTypeForMethodGen(MethodsToGen_CoreBindingHelper.Initialize, objectSpec);
}
Expand Down
34 changes: 34 additions & 0 deletions ...s/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -542,6 +542,40 @@ public record Options(string Name, {{collectionType}}<string> Values);
AssertCanCreateAssemblyImage(result.OutputCompilation);
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNetCore))]
[InlineData("IReadOnlyList")]
[InlineData("IReadOnlyCollection")]
[InlineData("IReadOnlySet")]
[InlineData("IEnumerable")]

@tarekgh tarekgh Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IEnumerable<string> may classify as a bindable member, in which case HasBindableMembers is true and this input routes through the pre-existing path rather than the newly fixed one. It still passes and is harmless, but it may not actually exercise the regression. Worth confirming, or swapping in an element type or shape you have verified takes the new path.

public async Task SoleReadOnlyCollectionConstructorParameterIsBindable(string collectionType)
{
// Regression test: a type whose only member is a non-bindable, read-only collection
// constructor parameter (no other bindable property) used to make the generator emit a
// call to an Initialize method that was never generated, producing CS0103 at compile time.
string source = $$"""
using Microsoft.Extensions.Configuration;
using System.Collections.Generic;

public class Program
{
public static void Main()
{
ConfigurationBuilder configurationBuilder = new();
IConfiguration config = configurationBuilder.Build();
Options options = config.Get<Options>();
}
}

public record Options({{collectionType}}<string> Values);

@tarekgh tarekgh Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two coverage gaps around this shape:

  1. Sole member plus complex element type is not exercised. The existing ComplexReadOnlyListConstructorParameterIsBindable pairs the collection with a bindable Name (so HasBindableMembers is true, the old path). Here the element is string. The intersection, for example record Options(IReadOnlyList<Child> Values) with no other member, is what newly drives the property walk transitive registration and RegisterForGen_AsConfigWithChildrenHelper in the changed code path, and it is untested.
  2. Nested usage is not exercised. Reaching this type as a member of an outer bound type goes through BindCore and EmitObjectInit rather than top level GetCore, which is a distinct emission path. It very likely works, but a nested case would lock it in.

""";

ConfigBindingGenRunResult result = await RunGeneratorAndUpdateCompilation(source, assemblyReferences: GetAssemblyRefsWithAdditional(typeof(ConfigurationBuilder), typeof(List<>)));
Assert.NotNull(result.GeneratedSource);
Assert.Empty(result.Diagnostics);

AssertCanCreateAssemblyImage(result.OutputCompilation);

@tarekgh tarekgh Jul 25, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertions stop at compile-clean plus a produced assembly image. Since this fixes a source generator vs reflection parity regression, consider also asserting runtime binding behavior: populate an in-memory configuration with a value for Values, call Get<Options>(), and assert the resulting collection contents. A generator can emit compilable but functionally wrong binding, and only a functional assertion guards against that.

}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNetCore))]
public async Task ComplexReadOnlyListConstructorParameterIsBindable()
{
Expand Down
Loading