Skip to content

Fix config binding source generator duplicating collection items bound through a constructor parameter #131092

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

Draft
svick wants to merge 2 commits into
base: main
Choose a base branch
from
+213 −21
Draft

Fix config binding source generator duplicating collection items bound through a constructor parameter #131092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c675785
Fix config binding source generator duplicating collection ctor params
svick Jul 20, 2026
8848f7c
Also skip constructor-bound properties for dictionary elements
svick Jul 20, 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
154 changes: 137 additions & 17 deletions src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -260,10 +260,17 @@ private void EmitBindCoreMethods()
private void EmitBindCoreMethod(ComplexTypeSpec type)
{
string objParameterExpression = $"ref {type.TypeRef.FullyQualifiedName} {Identifier.instance}";
EmitStartBlock(@$"public static void {nameof(MethodsToGen_CoreBindingHelper.BindCore)}({Identifier.IConfiguration} {Identifier.configuration}, {objParameterExpression}, bool defaultValueIfNotFound, {Identifier.BinderOptions}? {Identifier.binderOptions})");

ComplexTypeSpec effectiveType = (ComplexTypeSpec)_typeIndex.GetEffectiveTypeSpec(type);

// Objects created through a parameterized constructor need an extra parameter that tells BindCore
// whether the instance was created through that constructor. When it was, properties that are bound
// by a matching constructor parameter must not be bound again (see EmitBindCoreImplForObject).
string boundThroughConstructorParam = ShouldEmitBoundThroughConstructorParameter(effectiveType)
? $", bool {Identifier.boundThroughConstructor} = false"
: string.Empty;

EmitStartBlock(@$"public static void {nameof(MethodsToGen_CoreBindingHelper.BindCore)}({Identifier.IConfiguration} {Identifier.configuration}, {objParameterExpression}, bool defaultValueIfNotFound, {Identifier.BinderOptions}? {Identifier.binderOptions}{boundThroughConstructorParam})");

switch (effectiveType)
{
case ArraySpec arrayType:
Expand Down Expand Up @@ -819,11 +826,27 @@ void Emit_BindAndAddLogic_ForElement(string parsedKeyExpr)

if (_typeIndex.CanInstantiate(complexElementType))
{
// A reference-type element created through a parameterized constructor must not have
// its constructor-bound properties bound again. Since the element is only constructed
// when it isn't already present, track that at run time and forward it to BindCore.
// Value-type elements are always (re)constructed through the InitializationKind.None
// path below, so they don't need a separate flag.
string? constructedExpr = null;
if (!isValueType && ShouldEmitBoundThroughConstructorParameter(complexElementType))
{
constructedExpr = GetIncrementalIdentifier(Identifier.boundThroughConstructor);
_writer.WriteLine($"bool {constructedExpr} = false;");
}

EmitStartBlock($"if (!({conditionToUseExistingElement}))");
EmitObjectInit(complexElementType, Identifier.element, InitializationKind.SimpleAssignment, Identifier.section);
if (constructedExpr is not null)
{
_writer.WriteLine($"{constructedExpr} = true;");
}
EmitEndBlock();

EmitBindingLogic();
EmitBindingLogic(constructedExpr);
}
else
{
Expand All @@ -832,14 +855,15 @@ void Emit_BindAndAddLogic_ForElement(string parsedKeyExpr)
EmitEndBlock();
}

void EmitBindingLogic()
void EmitBindingLogic(string? constructedExpr = null)
{
this.EmitBindingLogic(
complexElementType,
Identifier.element,
Identifier.section,
InitializationKind.None,
ValueDefaulting.None);
ValueDefaulting.None,
constructedExpr: constructedExpr);

_writer.WriteLine($"{instanceIdentifier}[{parsedKeyExpr}] = {Identifier.element};");
}
Expand All @@ -859,19 +883,87 @@ private void EmitBindCoreImplForObject(ObjectSpec type)
string validateMethodCallExpr = $"{Identifier.ValidateConfigurationKeys}(typeof({type.TypeRef.FullyQualifiedName}), {keyCacheFieldName}, {Identifier.configuration}, {Identifier.binderOptions});";
_writer.WriteLine(validateMethodCallExpr);

List<PropertySpec>? ctorMatchedProperties = null;

foreach (PropertySpec property in type.Properties!)
{
if (_typeIndex.ShouldBindTo(property))
if (!_typeIndex.ShouldBindTo(property))
{
continue;
}

// A property that is bound through a matching constructor parameter is already populated when the
// instance is created through that constructor. Binding it again here would append to collections
// that the constructor already filled, duplicating their items.
// Defer such properties into a block guarded by !boundThroughConstructor so they are only bound when
// the instance was not created through the constructor (e.g. Bind(existingInstance)), matching the
// reflection binder.
if (property.MatchingCtorParam is not null && IsPropertyReboundInBindCore(property))
{
(ctorMatchedProperties ??= new()).Add(property);
continue;
}

EmitBindImplForProperty(property);
}

if (ctorMatchedProperties is not null)
{
EmitStartBlock($"if (!{Identifier.boundThroughConstructor})");
foreach (PropertySpec property in ctorMatchedProperties)
{
string containingTypeRef = property.IsStatic ? type.TypeRef.FullyQualifiedName : Identifier.instance;
EmitBindImplForMember(
property,
memberAccessExpr: $"{containingTypeRef}.{property.Name}",
GetSectionPathFromConfigurationExpression(property.ConfigurationKeyName),
canSet: property.CanSet,
canGet: property.CanGet,
InitializationKind.Declaration);
EmitBindImplForProperty(property);
}
EmitEndBlock();
}

void EmitBindImplForProperty(PropertySpec property)
{
string containingTypeRef = property.IsStatic ? type.TypeRef.FullyQualifiedName : Identifier.instance;
EmitBindImplForMember(
property,
memberAccessExpr: $"{containingTypeRef}.{property.Name}",
GetSectionPathFromConfigurationExpression(property.ConfigurationKeyName),
canSet: property.CanSet,
canGet: property.CanGet,
InitializationKind.Declaration);
}
}

/// <summary>
/// Whether <paramref name="type"/> is an object created through a parameterized constructor that has at
/// least one property bound through a matching constructor parameter which would otherwise be re-bound in
/// its <c>BindCore</c> method. Such types receive an extra <c>boundThroughConstructor</c> parameter.
/// </summary>
private bool ShouldEmitBoundThroughConstructorParameter(ComplexTypeSpec type) =>
type is ObjectSpec { InstantiationStrategy: ObjectInstantiationStrategy.ParameterizedConstructor, Properties: { } properties } &&
properties.Any(property =>
property.MatchingCtorParam is not null &&
_typeIndex.ShouldBindTo(property) &&
IsPropertyReboundInBindCore(property));

/// <summary>
/// Whether binding <paramref name="property"/> in a <c>BindCore</c> method emits code that reads from or
/// writes to the property. This mirrors the cases in <see cref="EmitBindImplForMember(MemberSpec, string, string, bool, bool, InitializationKind)"/>
/// that actually emit binding logic; get-only value/string properties, for example, are never re-bound.
/// </summary>
private bool IsPropertyReboundInBindCore(PropertySpec property)
{
switch (_typeIndex.GetEffectiveTypeSpec(property.TypeRef))
{
case ParsableFromStringSpec:
return property.CanGet && property.CanSet;
case ConfigurationSectionSpec:
return property.CanSet;
case ComplexTypeSpec complexType:
// EmitBindImplForMember skips a complex member only when it is a
// parameterized-constructor object with no bindable members. Every other complex member is bound.
return _typeIndex.HasBindableMembers(complexType) ||
complexType.IsValueType ||
complexType is CollectionSpec ||
complexType is not ObjectSpec { InstantiationStrategy: ObjectInstantiationStrategy.ParameterizedConstructor };
default:
return false;
}
}

Expand Down Expand Up @@ -959,7 +1051,7 @@ private bool EmitBindImplForMember(
{
// Early detection of types we cannot bind to and skip it.
if (!_typeIndex.HasBindableMembers(complexType) &&
!_typeIndex.GetEffectiveTypeSpec(complexType).IsValueType &&
!complexType.IsValueType &&
complexType is not CollectionSpec &&
((ObjectSpec)complexType).InstantiationStrategy == ObjectInstantiationStrategy.ParameterizedConstructor)
{
Expand Down Expand Up @@ -1090,7 +1182,8 @@ private void EmitBindingLogic(
string configArgExpr,
InitializationKind initKind,
ValueDefaulting valueDefaulting,
Action<string, string?>? writeOnSuccess = null)
Action<string, string?>? writeOnSuccess = null,
string? constructedExpr = null)
{
if (!_typeIndex.HasBindableMembers(type))
{
Expand Down Expand Up @@ -1127,14 +1220,40 @@ private void EmitBindingLogic(

void EmitBindingLogic(string instanceToBindExpr, InitializationKind initKind, string? tempIdentifierStoringExpr = null)
{
string bindCoreCall = $@"{nameof(MethodsToGen_CoreBindingHelper.BindCore)}({configArgExpr}, ref {instanceToBindExpr}, defaultValueIfNotFound: {FormatDefaultValueIfNotFound()}, {Identifier.binderOptions});";
string boundThroughConstructorArg = string.Empty;

if (_typeIndex.CanInstantiate(type))
{
if (initKind is not InitializationKind.None)
{
// The instance is (re)created here. If it goes through a parameterized constructor, tell
// BindCore not to bind the properties that the constructor already bound. For a null-check
// assignment the constructor only runs when the existing value was null, so the decision
// has to be made at run time.
if (ShouldEmitBoundThroughConstructorParameter(type))
{
Comment thread
svick marked this conversation as resolved.
if (initKind is InitializationKind.AssignmentWithNullCheck)
{
string wasNullIdentifier = GetIncrementalIdentifier(Identifier.wasNull);
_writer.WriteLine($"bool {wasNullIdentifier} = {instanceToBindExpr} is null;");
boundThroughConstructorArg = $", {Identifier.boundThroughConstructor}: {wasNullIdentifier}";
}
else
{
boundThroughConstructorArg = $", {Identifier.boundThroughConstructor}: true";
}
}

EmitObjectInit(type, instanceToBindExpr, initKind, configArgExpr);
}
else if (constructedExpr is not null)
{
// The caller instantiated the instance separately and provides a run-time flag telling
// whether it was created through its constructor (e.g. dictionary element binding). The
// caller only passes it for types that emit the parameter.
Debug.Assert(ShouldEmitBoundThroughConstructorParameter(type));
boundThroughConstructorArg = $", {Identifier.boundThroughConstructor}: {constructedExpr}";
}

EmitBindCoreCall();
}
Expand All @@ -1154,6 +1273,7 @@ void EmitBindingLogic(string instanceToBindExpr, InitializationKind initKind, st

void EmitBindCoreCall()
{
string bindCoreCall = $@"{nameof(MethodsToGen_CoreBindingHelper.BindCore)}({configArgExpr}, ref {instanceToBindExpr}, defaultValueIfNotFound: {FormatDefaultValueIfNotFound()}, {Identifier.binderOptions}{boundThroughConstructorArg});";
_writer.WriteLine(bindCoreCall);
writeOnSuccess?.Invoke(instanceToBindExpr, tempIdentifierStoringExpr);
}
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ private static class TypeDisplayString
private static class Identifier
{
public const string binderOptions = nameof(binderOptions);
public const string boundThroughConstructor = nameof(boundThroughConstructor);
public const string config = nameof(config);
public const string configureBinder = nameof(configureBinder);
public const string configureOptions = nameof(configureOptions);
Expand All @@ -86,6 +87,7 @@ private static class Identifier
public const string typedObj = nameof(typedObj);
public const string validateKeys = nameof(validateKeys);
public const string value = nameof(value);
public const string wasNull = nameof(wasNull);

public const string Add = nameof(Add);
public const string AddSingleton = nameof(AddSingleton);
Expand Down
16 changes: 16 additions & 0 deletions ...soft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,6 +266,22 @@ public class ParamsCollectionCtor
public List<string> Instances { get; }
}

public sealed class SourceWithCollectionCtorParameters
{
public SourceWithCollectionCtorParameters(string Name, IEnumerable<string> Addresses, IList<int> Ints, string[] Strings)
{
this.Name = Name;
this.Addresses = Addresses;
this.Ints = Ints;
this.Strings = Strings;
}

public string Name { get; }
public IEnumerable<string> Addresses { get; }
public IList<int> Ints { get; }
public string[] Strings { get; }
}

public readonly record struct ReadonlyRecordStructTypeOptions(string Color, int Length);

public class ContainerWithNestedImmutableObject
Expand Down
Loading