Fix nested readonly collection ctor parameters - #131932
Draft
rosebyte wants to merge 2 commits into
Draft
Conversation
… collection ctor param Fixes dotnet#131399 If a bound type had a member whose own type's sole member was a read-only collection constructor parameter (IReadOnlyList<T>, IReadOnlyCollection<T>, IReadOnlySet<T> or IEnumerable<T>, with no other bindable property), the generator left that member at null, with no exception and no diagnostic. The equivalent top-level shape, which goes through GetCore, binds correctly. Root cause: EmitBindImplForMember's early-return guard skipped a complex member whenever HasBindableMembers was false, without accounting for a parameterized-constructor type that is still instantiable. Such a type binds its constructor parameters in its generated Initialize method regardless of whether it has any other bindable member, so Initialize<T> was generated and CanInstantiate<T> was true, but nothing ever called it. Fix: treat such a member as bindable when the type can be instantiated and the member can be assigned the instance Initialize creates. Requiring canSet matters: for a get-only or init-only member there is nothing to assign, and un-skipping it would emit a dead `if (temp is not null) { }` block plus a spurious boundThroughConstructor parameter on BindCore. The guard moves into IsBindableAsMember, which now also backs IsPropertyReboundInBindCore. That method previously duplicated the guard's negation inline and has to stay in sync with it, since it decides whether a property already bound in Initialize is deferred into an `if (!boundThroughConstructor)` block in BindCore to avoid double-binding. The added theory covers every way such a member is reached - a constructor parameter, a settable property, and a settable property with a matching constructor parameter, the last both through Get<T>() and Bind(existingInstance) - for all four collection interfaces. It binds null on each shape without the emitter change. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98d6b6b8-446a-41b8-a59e-0e13f9ea3783
…ection constructor parameter The source generator bound `default` for a nested struct member whose only member is a constructor parameter of a read-only collection type, where the reflection binder binds it correctly. Two conditions kept the value-type path inert: * `EmitBindingLogicForComplexMember` hard-codes `InitializationKind.None` for value types, because a struct behind a property getter is a copy and so cannot be bound in place. * `EmitBindingLogic` then early-returns on `!HasBindableMembers` unless the initialization kind is something other than `None`. Relaxing the member guard alone therefore got as far as the door and no further. Such a type is created outright by its generated `Initialize` method, so the value-type branch now short-circuits to `SimpleAssignment` and assigns that instance straight to the member. The value-type term in `IsBindableAsMember` is also tightened to require a setter. Binding a value type in place would only mutate the copy the getter returns, so a get-only or init-only struct member previously produced a dead block in the generated output; it no longer does. Covered by a shared test in tests/Common, which runs against both the reflection binder and the generator and so pins the two engines together. It fails against the generator without this change, binding null where reflection binds the configured value. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 98d6b6b8-446a-41b8-a59e-0e13f9ea3783
|
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. |
Contributor
|
Tagging subscribers to this area: @dotnet/area-extensions-configuration |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a configuration binding source-generator parity gap where certain nested complex members were skipped during generated binding, leaving them null/default instead of being constructed and populated as the reflection-based binder would.
Changes:
- Refactors the “should we emit binding code for this complex member?” decision into
IsBindableAsMemberand applies it consistently to bothEmitBindImplForMemberandIsPropertyReboundInBindCore. - Updates value-type complex-member binding to directly assign the instance created by
Initialize(...)when the type has no bindable members beyond constructor parameters. - Adds new regression tests covering nested binding for both reference types and value types (including nullable structs), plus a reflection-binder test for the same shape.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs | Adds generator regression tests for nested binding of “sole read-only collection ctor parameter” shapes (class + struct). |
| src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.Collections.cs | Introduces test types used to validate nested binding behavior in reflection-binder tests. |
| src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.Collections.cs | Adds a reflection-binder regression test verifying nested binding for the same type shapes. |
| src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs | Fixes emitter logic so nested members that bind via constructor parameters are no longer skipped; adds a value-type fast path to assign initialized instances. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #131399
Problem
The configuration binder source generator silently bound
null/defaultfor a nested member whose type's sole member is a constructor parameter of a read-only collection type:The reflection binder binds both correctly, so this was a silent divergence between the two engines: no diagnostic, no exception, just missing configuration at runtime.
#131358 fixed the equivalent top-level case (
config.Get<Inner>()). The nested case remained broken, and this is a pre-existing gap rather than a regression from that change.Root cause
Two independent bugs sharing one symptom.
1. Reference types.
EmitBindImplForMemberskipped any complex member with no bindable members. But a type with a parameterized constructor binds its parameters in the generatedInitializemethod regardless of what other members it has. Thanks to #131358InitializeInnerwas being emitted andCanInstantiatewas true; nothing ever called it.The guard is now extracted into
IsBindableAsMember, with the missing term added: a type that can be instantiated is bindable through its constructor even with no bindable members of its own, provided the member can actually be assigned. The near-duplicate logic inIsPropertyReboundInBindCorenow calls the same helper, so the two cannot drift apart.2. Value types. Relaxing the guard alone gets you through the door into an empty room, because two conditions both had to change:
EmitBindingLogicForComplexMemberhard-codesInitializationKind.Nonefor value types, since a struct behind a property getter is a copy and cannot be bound in place.EmitBindingLogicthen early-returns on!HasBindableMembersunless the initialization kind is something other thanNone.Such a type is created outright by
Initialize, so the value-type branch now short-circuits toSimpleAssignmentand assigns that instance directly to the member.The struct case was arguably worse than plain
default: a non-nullable struct parameter threwParameterHasNoMatchingConfigwhen the section was absent, and silently bounddefaultwhen it was present — close to backwards. This corrects the present case; the absent-section throw stays, matching every other non-nullable parameter.canSetBoth terms require the member to be settable. For a get-only or init-only member there is nothing to assign, so un-skipping it emitted only a dead block plus a spurious
boundThroughConstructorparameter onBindCore. For value types this is a small behavioural change worth calling out: dead blocks previously emitted for get-only struct members are gone.Generated code
Before, for
class Outer { public Inner? Nested { get; set; } }:After: