diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs index e89bad8ded56f7..e483cb6bbd1cc7 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs @@ -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: @@ -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 { @@ -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};"); } @@ -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? 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); + } + } + + /// + /// Whether 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 BindCore method. Such types receive an extra boundThroughConstructor parameter. + /// + 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)); + + /// + /// Whether binding in a BindCore method emits code that reads from or + /// writes to the property. This mirrors the cases in + /// that actually emit binding logic; get-only value/string properties, for example, are never re-bound. + /// + 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; } } @@ -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) { @@ -1090,7 +1182,8 @@ private void EmitBindingLogic( string configArgExpr, InitializationKind initKind, ValueDefaulting valueDefaulting, - Action? writeOnSuccess = null) + Action? writeOnSuccess = null, + string? constructedExpr = null) { if (!_typeIndex.HasBindableMembers(type)) { @@ -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)) + { + 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(); } @@ -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); } diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs index f80edafeced652..c0279e826f61c4 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs @@ -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); @@ -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); diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs index bd2353717596f0..2e16b110e5a6b5 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs @@ -266,6 +266,22 @@ public class ParamsCollectionCtor public List Instances { get; } } + public sealed class SourceWithCollectionCtorParameters + { + public SourceWithCollectionCtorParameters(string Name, IEnumerable Addresses, IList Ints, string[] Strings) + { + this.Name = Name; + this.Addresses = Addresses; + this.Ints = Ints; + this.Strings = Strings; + } + + public string Name { get; } + public IEnumerable Addresses { get; } + public IList Ints { get; } + public string[] Strings { get; } + } + public readonly record struct ReadonlyRecordStructTypeOptions(string Color, int Length); public class ContainerWithNestedImmutableObject diff --git a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs index 034bea66dfb1bd..5f5fde593be85e 100644 --- a/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs +++ b/src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs @@ -1686,12 +1686,12 @@ public void CanBindOnParametersAndProperties_RecordWithArrayConstructorParameter } /// - /// When a constructor parameter name differs only by case from a matching collection property, - /// the binder must bind the collection once (through the constructor) and must not bind it again - /// through the property, which would otherwise duplicate the collection items. + /// When a constructor parameter populates a matching collection property, the binder must bind the collection + /// once (through the constructor) and must not bind it again through the property, which would otherwise + /// duplicate the collection items. This must hold whether the parameter name matches the property name exactly + /// or differs only by case. This test checks the different case scenario, the next one the same name scenario. /// [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/83803", typeof(TestHelpers), nameof(TestHelpers.SourceGenMode))] public void CanBindOnParametersAndProperties_GetterOnlyCollectionWithCaseMismatchedConstructorParameter() { string json = """ @@ -1709,6 +1709,60 @@ public void CanBindOnParametersAndProperties_GetterOnlyCollectionWithCaseMismatc Assert.Equal(expected, config.Get().Instances); } + /// + /// A type whose constructor parameters have the same name as the matching collection properties (as in the + /// canonical record/primary-constructor pattern) must also bind each collection only once. + /// + [Fact] + public void CanBindOnParametersAndProperties_CollectionsWithSameCasedConstructorParameters() + { + string json = """ + { + "source": { + "name": "DemoService", + "addresses": [ "127.0.0.1" ], + "ints": [ 1, 2, 3, 4, 5 ], + "strings": [ "one", "two", "three" ] + } + } + """; + + IConfiguration config = TestHelpers.GetConfigurationFromJsonString(json); + + SourceWithCollectionCtorParameters source = config.GetSection("source").Get(); + + Assert.Equal("DemoService", source.Name); + Assert.Equal(new[] { "127.0.0.1" }, source.Addresses); + Assert.Equal(new[] { 1, 2, 3, 4, 5 }, source.Ints); + Assert.Equal(new[] { "one", "two", "three" }, source.Strings); + } + + /// + /// A dictionary whose value type is created through a parameterized constructor that populates a collection + /// property must bind each element's collection only once, even though dictionary elements are bound through a + /// separate code path from top-level and property binding. + /// + [Fact] + public void CanBindOnParametersAndProperties_DictionaryValueWithCollectionConstructorParameter() + { + string json = """ + { + "map": { + "first": { "Instances": [ "a", "b" ] }, + "second": { "Instances": [ "c" ] } + } + } + """; + + IConfiguration config = TestHelpers.GetConfigurationFromJsonString(json); + + Dictionary map = + config.GetSection("map").Get>(); + + Assert.Equal(new[] { "a", "b" }, map["first"].Instances); + Assert.Equal(new[] { "c" }, map["second"].Instances); + } + public static IEnumerable Configuration_TestData() { yield return new object[]