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

Open
svick wants to merge 5 commits into
base: main
Choose a base branch
from
+372 −26
Open

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
5 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
9f56ecf
Also skip init/required properties bound in Initialize
svick Jul 22, 2026
e436669
Add nested-property and Bind(existingInstance) coverage
svick Jul 22, 2026
740d845
Add list-element and init-only-complex-property test coverage
svick Jul 22, 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
182 changes: 160 additions & 22 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 @@ -408,7 +415,8 @@ void EmitBindImplForMember(MemberSpec member)
// Since we're binding to local variables, we can always get and set
canSet: true,
canGet: true,
InitializationKind.None);
InitializationKind.None,
bindingToLocal: true);

if (canBindToMember)
{
Expand Down Expand Up @@ -819,11 +827,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 +856,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 +884,99 @@ private void EmitBindCoreImplForObject(ObjectSpec type)
string validateMethodCallExpr = $"{Identifier.ValidateConfigurationKeys}(typeof({type.TypeRef.FullyQualifiedName}), {keyCacheFieldName}, {Identifier.configuration}, {Identifier.binderOptions});";
_writer.WriteLine(validateMethodCallExpr);

List<PropertySpec>? initializeBoundProperties = null;

foreach (PropertySpec property in type.Properties!)
{
if (_typeIndex.ShouldBindTo(property))
if (!_typeIndex.ShouldBindTo(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);
continue;
}

// A property that is populated while the instance is created through its Initialize method - either
// through a matching constructor parameter or as a required/init-only property assigned in the object
// initializer - is already bound at that point. Binding it again here would append to collections
// that Initialize 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 Initialize (e.g. Bind(existingInstance)), matching the
// reflection binder.
if (IsBoundInInitialize(type, property) && IsPropertyReboundInBindCore(property))
{
(initializeBoundProperties ??= new()).Add(property);
continue;
}

EmitBindImplForProperty(property);
}

if (initializeBoundProperties is not null)
{
EmitStartBlock($"if (!{Identifier.boundThroughConstructor})");
foreach (PropertySpec property in initializeBoundProperties)
{
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 while the instance is created (through its Initialize method) 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 { Properties: { } properties } objectType &&
properties.Any(property =>
IsBoundInInitialize(objectType, property) &&
_typeIndex.ShouldBindTo(property) &&
IsPropertyReboundInBindCore(property));

/// <summary>
/// Whether <paramref name="property"/> is populated while an instance of <paramref name="type"/> is created
/// through its Initialize method: either it flows through a matching constructor parameter, or it is a
/// required/init-only property assigned in the object initializer. Only parameterized-constructor types have
/// an Initialize method; parameterless-constructor types bind all their properties in <c>BindCore</c>.
/// </summary>
private static bool IsBoundInInitialize(ObjectSpec type, PropertySpec property) =>
type.InstantiationStrategy is ObjectInstantiationStrategy.ParameterizedConstructor &&
(property.MatchingCtorParam is not null || property.SetOnInit);

/// <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 All @@ -881,7 +986,8 @@ private bool EmitBindImplForMember(
string sectionPathExpr,
bool canSet,
bool canGet,
InitializationKind initializationKind)
InitializationKind initializationKind,
bool bindingToLocal = false)
{
string sectionParseExpr = GetSectionFromConfigurationExpression(member.ConfigurationKeyName);

Expand Down Expand Up @@ -959,7 +1065,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 All @@ -974,7 +1080,7 @@ complexType is not CollectionSpec &&
string sectionIdentifier = GetIncrementalIdentifier(Identifier.section);

EmitStartBlock($"if ({sectionValidationCall} is {Identifier.IConfigurationSection} {sectionIdentifier})");
EmitBindingLogicForComplexMember(member, memberAccessExpr, sectionIdentifier, canSet);
EmitBindingLogicForComplexMember(member, memberAccessExpr, sectionIdentifier, canSet, bindingToLocal);
EmitEndBlock();

// The current configuration section doesn't have any children, let's check if we are binding to an array and the configuration value is empty string.
Expand All @@ -1000,7 +1106,8 @@ private void EmitBindingLogicForComplexMember(
MemberSpec member,
string memberAccessExpr,
string configArgExpr,
bool canSet)
bool canSet,
bool bindingToLocal = false)
{

TypeSpec memberType = _typeIndex.GetTypeSpec(member.TypeRef);
Expand Down Expand Up @@ -1061,7 +1168,10 @@ private void EmitBindingLogicForComplexMember(
_writer.WriteLine($"{tempIdentifierStoringExpr}");
}

if (member.CanGet && _typeIndex.CanInstantiate(effectiveMemberType))
// When the config section is absent, re-assign the member to itself so any value-mutator
// setter runs. This is unnecessary when binding into an Initialize local (no accessor to
// invoke) and would emit a CS1717 self-assignment, so skip it in that case.
if (!bindingToLocal && member.CanGet && _typeIndex.CanInstantiate(effectiveMemberType))
{
EmitEndBlock();
EmitStartBlock("else");
Expand Down Expand Up @@ -1090,7 +1200,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 +1238,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 +1291,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
Loading
Loading