Fix config binding source generator duplicating collection items bound through a constructor parameter - #131092
Fix config binding source generator duplicating collection items bound through a constructor parameter#131092svick wants to merge 5 commits into
Conversation
Fixes dotnet#83803. For a type with a parameterized constructor, the generated code ran `Initialize(...)` (invoking the constructor, which binds the constructor parameters into their collections) and then `BindCore(...)`, which re-bound every property - re-appending to collections the constructor had already filled. This duplicated items for collection properties bound through a matching constructor parameter (e.g. `IList<int> Ints` via ctor param `ints`/`Ints`); arrays and scalars were unaffected. The generated `BindCore` now takes an optional `boundThroughConstructor` parameter (emitted only for parameterized-ctor object types that actually have a re-bindable ctor-matched property). Constructor-matched properties are deferred into an `if (!boundThroughConstructor)` block so they are only bound when the instance was not created through the constructor (e.g. `Bind(existingInstance)`), matching the reflection binder. Call sites pass `true` when the instance is freshly constructed, and a runtime `wasNull` flag for the `x ??= Initialize()` case, which only constructs when the value was null. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a0d7ac2-f2a3-4526-93ab-36bf1a23933f
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR updates the Microsoft.Extensions.Configuration.Binder source generator to avoid re-binding constructor-matched collection properties (which can cause additive collections to duplicate items) by adding a boundThroughConstructor flag to generated BindCore overloads and guarding the relevant property-binding paths. Tests are updated to validate the behavior in both reflection and source-gen modes.
Changes:
- Extend generated
BindCoremethods (for affected parameterized-constructor object shapes) with an optionalboundThroughConstructorparameter and skip re-binding ctor-matched properties when it’strue. - Update the emitter call sites to pass
boundThroughConstructor: truewhen an instance is freshly constructed, and a runtimewasNullflag for??=initialization patterns. - Update binder tests: remove the SourceGen-mode
ActiveIssuegate for the case-mismatched ctor-parameter scenario and add coverage for same-cased ctor-parameter collection binding.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.cs | Adds a new test class with ctor-matched collection parameters/properties for same-cased repro coverage. |
| src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.cs | Un-gates an existing test for source-gen mode and adds a new regression test for same-cased ctor parameter/property collection binding. |
| src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs | Adds identifier constants used by the emitter (boundThroughConstructor, wasNull). |
| src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs | Implements the boundThroughConstructor parameter, defers ctor-matched properties behind a guard, and updates key call sites to pass the flag on construction. |
Dictionary value binding instantiates the element through a separate EmitObjectInit and then calls BindCore with InitializationKind.None, which bypassed the boundThroughConstructor flag. A dictionary whose value type is a parameterized-ctor type with a matching additive collection property would therefore still duplicate items. EmitBindingLogic now accepts an explicit run-time flag (constructedExpr) for callers that instantiate separately. The dictionary element path tracks whether the element was newly constructed (it is only created when not already present) and forwards that to BindCore. The enumerable-with-add and array paths already construct each element through InitializationKind.Declaration, so they were unaffected. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a0d7ac2-f2a3-4526-93ab-36bf1a23933f
|
Looks good. One suggestion on test coverage: the added tests cover the top-level Worth adding (true regression test): a container whose property type has a ctor-matched collection, bound via a nested null property. This exercises the public sealed class ContainerWithCtorCollectionChild
{
public GetterOnlyInterfaceCollectionWithCaseMismatchedCtorParameter Child { get; set; }
}
[Fact]
public void CanBind_NestedTypeWithCollectionConstructorParameter()
{
string json = """
{
"Child": { "Instances": [ "a", "b" ] }
}
""";
IConfiguration config = TestHelpers.GetConfigurationFromJsonString(json);
var result = config.Get<ContainerWithCtorCollectionChild>();
Assert.Equal(new[] { "a", "b" }, result.Child.Instances);
}Without the fix this produces Nice to have (guard, not a repro): a [Fact]
public void CanBindExistingInstance_BindsCtorMatchedCollection()
{
string json = """
{
"Instances": [ "first", "second" ]
}
""";
IConfiguration config = TestHelpers.GetConfigurationFromJsonString(json);
var instance = new SettableCollectionWithCaseMismatchedCtorParameter(new List<string>());
config.Bind(instance);
Assert.Equal(new[] { "first", "second" }, instance.Instances);
} |
An init-only or required property that is not backed by a constructor parameter is still bound while the instance is created, through the object initializer in the generated Initialize method. It was then bound again in BindCore, appending to collections that Initialize already filled and duplicating their items - the same class of bug as the constructor-parameter case, but a different root cause. Widen the BindCore deferral (and the boundThroughConstructor parameter emission) from ctor-matched properties to any property bound in Initialize, captured by the new IsBoundInInitialize helper. It is gated on the type being a parameterized-constructor type, since only those emit an Initialize method; parameterless-constructor types bind all their properties in BindCore. Also fix a CS1717 self-assignment (`x = x`) the generator emitted for a complex or collection init/required property in Initialize: the "preserve existing value" self-assignment is meaningful only for a property with a value-mutator setter, so skip it when binding into an Initialize local, which has no accessor to invoke. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a0d7ac2-f2a3-4526-93ab-36bf1a23933f
Address review feedback on PR dotnet#131092 by covering two paths the existing tests missed: - CanBind_NestedTypeWithCollectionConstructorParameter: a container whose property type has a ctor-matched collection, bound through the nested null-check (??=) path. This exercises the runtime `wasNull` branch of boundThroughConstructor and duplicates on the unfixed generator (verified). - CanBindExistingInstance_BindsCtorMatchedCollection: binding into an existing instance still binds a settable ctor-matched collection. This passes with or without the fix; it guards the boundThroughConstructor:false branch so a future change cannot start over-skipping and silently stop binding such properties. Both live in tests/Common so they run under the reflection and source-generator binders. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a0d7ac2-f2a3-4526-93ab-36bf1a23933f
Cover two more paths affected by the fix: - CanBind_ListOfTypeWithCollectionConstructorParameter: a list whose element type has a ctor-matched collection, exercising the enumerable-with-add (and array) element construction path. This duplicates on the unfixed generator (verified), mirroring the dictionary-element regression test. - CanBindOnParametersAndProperties_InitOnlyComplexPropertyWithoutConstructorParameter: an init-only complex (non-collection) property with no ctor param, exercising the CS1717 self-assignment fix for the object case (the existing init-only test covers only the collection case). Verified it fails to compile (CS1717) without the bindingToLocal guard. Both live in tests/Common so they run under the reflection and source-generator binders. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 1a0d7ac2-f2a3-4526-93ab-36bf1a23933f
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Summary
Fixes #83803.
The Configuration Binding source generator duplicated the items of a collection property that is populated while an instance is created, when binding a type with a parameterized constructor. For example, given:
binding
ints: [1, 2, 3, 4, 5]produced1, 2, 3, 4, 5, 1, 2, 3, 4, 5. Arrays and scalars were unaffected; only additive collections (IList<T>,IEnumerable<T>, settable collections) duplicated. The reflection binder was already fixed (#106158, #129775); this brings the source generator in line.Root cause
For a type with a parameterized constructor, the generated code first calls
Initialize(...)- which invokes the constructor and populates the members bound during construction - and then callsBindCore(...), which re-bound every property. Re-binding a collection property appends to the collection that was already filled, so the items appear twice.Fix
The generated
BindCoremethod now takes an optionalboundThroughConstructorparameter, emitted only for parameterized-constructor object types that actually have a re-bindable property bound during construction. Properties bound inInitializeare deferred into anif (!boundThroughConstructor)block, so they are bound only when the instance was not created throughInitialize(e.g.config.Bind(existingInstance)), matching the reflection binder'sBindProperties/ResetPropertyValuebehavior.Call sites pass the flag when the instance is (re)created:
truefor unconditional construction (var x = Initialize(...)),wasNullflag for thex ??= Initialize(...)case, since the constructor only runs when the existing value was null,BindCore(dictionary elements).The fix covers every construct-then-bind path: top-level
Get<T>(), nested properties (??=), dictionary values, and list/array elements."Bound during construction" means both:
required/init-only property assigned in the generated object initializer, even when it has no matching constructor parameter.The latter also fixes a
CS1717self-assignment (x = x) the generator previously emitted for a complex or collectioninit/requiredproperty inInitialize.No baseline
.generated.txtchanges: the extra parameter/guard only appear for the affected type shapes, none of which are in the baseline suite.Out of scope
Two related, pre-existing source-generator issues surfaced during review and are intentionally left for follow-ups:
CS9035for arequiredproperty on a parameterless-constructor type (the generator never emits an object initializer there).initcollection is replaced rather than appended to on theGet<T>()path (reflection appends).Note
This pull request was authored with the assistance of GitHub Copilot.