Skip to content

Fix config binder source gen for a sole read-only collection ctor param#131358

Open
TemRevil wants to merge 2 commits into
dotnet:mainfrom
TemRevil:fix/config-binder-solo-readonly-collection-ctor-param
Open

Fix config binder source gen for a sole read-only collection ctor param#131358
TemRevil wants to merge 2 commits into
dotnet:mainfrom
TemRevil:fix/config-binder-solo-readonly-collection-ctor-param

Conversation

@TemRevil

Copy link
Copy Markdown

Fixes #131320

The configuration binder source generator emitted an uncompilable call to an Initialize method that was never generated, for a parameterized-constructor type whose only member is a non-bindable copy-constructor collection parameter (a positional record with a single IReadOnlyList, IReadOnlyCollection, IReadOnlySet, or IEnumerable parameter and no other bindable property). The reflection binder handles this shape correctly, so this was a parity regression that broke the build instead.

Root cause: BindingHelperInfo.Builder.TryRegisterTransitiveTypesForMethodGen only registered a type for Initialize-method generation inside the HasBindableMembers(objectSpec) check. But constructor parameters are bound in Initialize independently of whether the type has any other bindable property, per the comment already on the ctor-param property loop a few lines above. For a type where the only "property" is a ctor param backed by a non-bindable read-only collection type, HasBindableMembers is false, so Initialize never got registered/emitted, while the emitter's EmitBindingLogic/EmitObjectInit still unconditionally calls InitializeXxx(...) for any ParameterizedConstructor type.

Fix: register the Initialize method (and walk the constructor-parameter properties needed to bind it) whenever the type has a parameterized constructor, regardless of HasBindableMembers. BindCore registration stays gated on HasBindableMembers as before, since that part is unrelated.

Verified by running the incremental generator directly against the repro from the issue:

var config = new ConfigurationBuilder().Build();
Options options = config.Get<Options>();
public record Options(IReadOnlyList<string> Values);

Before this change the generated source calls InitializeOptions but never defines it, reproducing CS0103: The name 'InitializeOptions' does not exist in the current context exactly. After this change the method is generated and binds the parameter correctly. Confirmed for all four affected collection interfaces (IReadOnlyList, IReadOnlyCollection, IReadOnlySet, IEnumerable).

Added SoleReadOnlyCollectionConstructorParameterIsBindable next to the existing ReadOnlyCollectionConstructorParameterIsBindable test, covering the case where the collection parameter is the type's only member (the existing test always paired it with a second, ordinarily-bindable property, so it didn't exercise this gap).

The configuration binder source generator emitted a call to an
Initialize method that was never generated for a parameterized-
constructor type whose only member is a non-bindable copy-constructor
collection parameter (IReadOnlyList<T>, IReadOnlyCollection<T>,
IReadOnlySet<T>, or IEnumerable<T>, with no other bindable property).
The reflection binder already handled this shape correctly, so this
was a parity gap that broke the build with CS0103 instead.

The registration pass only registered a type for Initialize-method
generation inside the HasBindableMembers check, but constructor
parameters are bound in Initialize independently of whether the type
has any other bindable property. Register the Initialize method (and
walk the constructor-parameter properties needed to bind it) whenever
the type has a parameterized constructor, regardless of
HasBindableMembers.

Verified by running the incremental generator directly against the
repro from the issue (record with a single IReadOnlyList<string>
constructor parameter): before this change the generated source calls
InitializeOptions but never defines it, reproducing CS0103 exactly;
after this change the method is generated and binds the parameter
correctly. Confirmed for all four affected collection interfaces.

Fixes dotnet#131320
@dotnet-policy-service dotnet-policy-service Bot added the community-contribution Indicates that the PR has been added by a community member label Jul 25, 2026
@azure-pipelines

Copy link
Copy Markdown
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.

@dotnet-policy-service

Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

@tarekgh tarekgh left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few test-hardening notes on the new regression test. The product change itself looks correct and well targeted; these are non-blocking suggestions. One additional stylistic thought: the sibling ReadOnlyCollectionConstructorParameterIsBindable, ComplexReadOnlyListConstructorParameterIsBindable, and this new Sole... test all cover variations of the same shape, so they could eventually be consolidated into a single data-driven theory.

Assert.NotNull(result.GeneratedSource);
Assert.Empty(result.Diagnostics);

AssertCanCreateAssemblyImage(result.OutputCompilation);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The assertions stop at compile-clean plus a produced assembly image. Since this fixes a source generator vs reflection parity regression, consider also asserting runtime binding behavior: populate an in-memory configuration with a value for Values, call Get<Options>(), and assert the resulting collection contents. A generator can emit compilable but functionally wrong binding, and only a functional assertion guards against that.

}
}

public record Options({{collectionType}}<string> Values);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two coverage gaps around this shape:

  1. Sole member plus complex element type is not exercised. The existing ComplexReadOnlyListConstructorParameterIsBindable pairs the collection with a bindable Name (so HasBindableMembers is true, the old path). Here the element is string. The intersection, for example record Options(IReadOnlyList<Child> Values) with no other member, is what newly drives the property walk transitive registration and RegisterForGen_AsConfigWithChildrenHelper in the changed code path, and it is untested.
  2. Nested usage is not exercised. Reaching this type as a member of an outer bound type goes through BindCore and EmitObjectInit rather than top level GetCore, which is a distinct emission path. It very likely works, but a nested case would lock it in.

[InlineData("IReadOnlyList")]
[InlineData("IReadOnlyCollection")]
[InlineData("IReadOnlySet")]
[InlineData("IEnumerable")]

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IEnumerable<string> may classify as a bindable member, in which case HasBindableMembers is true and this input routes through the pre-existing path rather than the newly fixed one. It still passes and is harmless, but it may not actually exercise the regression. Worth confirming, or swapping in an element type or shape you have verified takes the new path.

… fix

Compiling proved the Initialize method gets emitted, but never asserted
the generated code binds the right values. Extend the existing theory to
populate real config and check the bound collection contents for all
four interface shapes (including IEnumerable, which still routes through
the same CopyConstructor/HasBindableMembers=false path as the others).
Add a sibling test for a complex (non-string) element type, the other
gap called out in review.
@TemRevil

Copy link
Copy Markdown
Author

Added functional coverage for both gaps, pushed as 16c022b5. SoleReadOnlyCollectionConstructorParameterIsBindable now populates real config, invokes the compiled assembly, and asserts the bound collection contents for all four interface shapes. Added a sibling test, SoleReadOnlyCollectionConstructorParameterOfComplexElementIsBindable, for the complex-element case: record Options(IReadOnlyList<Child> Values) with no other member. The existing ComplexReadOnlyListConstructorParameterIsBindable always pairs the collection with a bindable Name, so it never actually hit this code path.

On whether IEnumerable<string> exercises the fix: it does. I reverted to the parent commit and ran all four shapes through the generator directly. Pre-fix, IEnumerable<string> fails with the identical CS0103: The name 'InitializeOptions' does not exist as the other three, since it also resolves to CollectionInstantiationStrategy.CopyConstructor in the parser (same as IReadOnlyList/IReadOnlyCollection/IReadOnlySet), so IsCollectionAndCannotOverride excludes it from HasBindableMembers the same way. Post-fix, all four compile and bind correctly.

The nested case turned up something real, though not what either of us expected. Binding Outer.Nested (an Inner whose sole member is a read-only collection ctor param) silently produces null instead of the actual value, no exception. Traced it to CoreBindingHelpers.cs's EmitBindImplForMember, the early return around line 961: !HasBindableMembers(complexType) && ... && InstantiationStrategy == ParameterizedConstructor skips emitting the property assignment entirely, even though InitializeInner is generated correctly and CanInstantiate is true for that exact type. git diff confirms that block is untouched by this PR, present verbatim at the parent commit too, so it's a pre-existing gap in the nested/BindCore path rather than something this fix introduced.

Since it's a different root cause in a different file, I didn't fold a fix into this PR without checking first. Happy to open a follow-up issue with the repro, take a shot at fixing it separately, or fold it into this PR if you'd rather keep it together, whichever you prefer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Extensions-Configuration community-contribution Indicates that the PR has been added by a community member

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Configuration SG: type whose only member is a non-bindable constructor parameter emits CS0103

2 participants