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..cebe7d8a9705b0 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: @@ -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) { @@ -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 { @@ -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};"); } @@ -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? 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); + } + } + + /// + /// Whether 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 BindCore method. Such types receive an extra + /// boundThroughConstructor parameter. + /// + private bool ShouldEmitBoundThroughConstructorParameter(ComplexTypeSpec type) => + type is ObjectSpec { Properties: { } properties } objectType && + properties.Any(property => + IsBoundInInitialize(objectType, property) && + _typeIndex.ShouldBindTo(property) && + IsPropertyReboundInBindCore(property)); + + /// + /// Whether is populated while an instance of 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 BindCore. + /// + private static bool IsBoundInInitialize(ObjectSpec type, PropertySpec property) => + type.InstantiationStrategy is ObjectInstantiationStrategy.ParameterizedConstructor && + (property.MatchingCtorParam is not null || property.SetOnInit); + + /// + /// 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; } } @@ -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); @@ -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) { @@ -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. @@ -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); @@ -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"); @@ -1090,7 +1200,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 +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)) + { + 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 +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); } 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..3ba6742be4153c 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 @@ -260,12 +260,52 @@ public class GetterOnlyInterfaceCollectionWithCaseMismatchedCtorParameter public IList Instances { get; } } + public sealed class ContainerWithCtorCollectionChild + { + public GetterOnlyInterfaceCollectionWithCaseMismatchedCtorParameter Child { get; set; } + } + public class ParamsCollectionCtor { public ParamsCollectionCtor(params List instances) => Instances = instances; 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 sealed class ClassWithInitOnlyCollectionNoCtorParam + { + public ClassWithInitOnlyCollectionNoCtorParam(int Number) => this.Number = Number; + public int Number { get; } + public List Items { get; init; } + } + + public sealed class ClassWithInitOnlyComplexNoCtorParam + { + public ClassWithInitOnlyComplexNoCtorParam(int Number) => this.Number = Number; + public int Number { get; } + public NestedForInitOnly Child { get; init; } + } + + public sealed class NestedForInitOnly + { + public string Value { get; set; } + } + 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..ae16017765ba8b 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,172 @@ 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); + } + + /// + /// A list whose element type has a collection populated through a constructor parameter must bind each + /// element's collection only once. List and array elements are constructed through a separate code path + /// (enumerable-with-add) from top-level, property, and dictionary binding. + /// + [Fact] + public void CanBind_ListOfTypeWithCollectionConstructorParameter() + { + string json = """ + { + "Items": [ { "Instances": [ "a", "b" ] }, { "Instances": [ "c" ] } ] + } + """; + + IConfiguration config = TestHelpers.GetConfigurationFromJsonString(json); + + List result = + config.GetSection("Items").Get>(); + + Assert.Equal(new[] { "a", "b" }, result[0].Instances); + Assert.Equal(new[] { "c" }, result[1].Instances); + } + + /// + /// A nested property whose type has a collection populated through a constructor parameter is bound through the + /// null-check assignment (??=) path, where whether the instance was created through its constructor is + /// decided at run time. The collection must still be bound only once. + /// + [Fact] + public void CanBind_NestedTypeWithCollectionConstructorParameter() + { + string json = """ + { + "Child": { "Instances": [ "a", "b" ] } + } + """; + + IConfiguration config = TestHelpers.GetConfigurationFromJsonString(json); + + ContainerWithCtorCollectionChild result = config.Get(); + + Assert.Equal(new[] { "a", "b" }, result.Child.Instances); + } + + /// + /// When binding into an existing instance (which is not created through its constructor), a settable collection + /// property backed by a constructor parameter must still be bound. This guards against the deferral logic + /// over-skipping and silently not binding such properties. + /// + [Fact] + public void CanBindExistingInstance_BindsCtorMatchedCollection() + { + string json = """ + { + "Instances": [ "first", "second" ] + } + """; + + IConfiguration config = TestHelpers.GetConfigurationFromJsonString(json); + var instance = new SettableCollectionWithCaseMismatchedCtorParameter(new List()); + + config.Bind(instance); + + Assert.Equal(new[] { "first", "second" }, instance.Instances); + } + + /// + /// An init-only (or required) collection property that is not backed by a constructor parameter is bound in the + /// generated Initialize method (through the object initializer). It must not be bound again in BindCore when the + /// instance was created there, or the collection items would be duplicated. + /// + [Fact] + public void CanBindOnParametersAndProperties_InitOnlyCollectionWithoutConstructorParameter() + { + string json = """ + { + "Number": 7, + "Items": [ "a", "b" ] + } + """; + + IConfiguration config = TestHelpers.GetConfigurationFromJsonString(json); + + ClassWithInitOnlyCollectionNoCtorParam result = config.Get(); + + Assert.Equal(7, result.Number); + Assert.Equal(new[] { "a", "b" }, result.Items); + } + + /// + /// An init-only complex (non-collection) property that is not backed by a constructor parameter is bound in the + /// generated Initialize method. The generator must not emit a self-assignment for it (which would produce a + /// CS1717 compile error), and the property must still be bound correctly. + /// + [Fact] + public void CanBindOnParametersAndProperties_InitOnlyComplexPropertyWithoutConstructorParameter() + { + string json = """ + { + "Number": 7, + "Child": { "Value": "hello" } + } + """; + + IConfiguration config = TestHelpers.GetConfigurationFromJsonString(json); + + ClassWithInitOnlyComplexNoCtorParam result = config.Get(); + + Assert.Equal(7, result.Number); + Assert.Equal("hello", result.Child.Value); + } + public static IEnumerable Configuration_TestData() { yield return new object[]