Skip to content

Skip making empty loops for collections of non-bindable elements - #131732

Merged
rosebyte merged 4 commits into
dotnet:mainfrom
rosebyte:rosebyte-scaling-disco
Aug 31, 2026
Merged

rosebyte merged 4 commits into
dotnet:mainfrom
rosebyte:rosebyte-scaling-disco

Conversation

@rosebyte

@rosebyte rosebyte commented Aug 3, 2026

Copy link
Copy Markdown
Member

Fixes #92151.

Problem

For a collection whose element type cannot be constructed, the source generator emits a BindCore that does nothing at all:

public static void BindCore(IConfiguration configuration, ref EndPointCollection instance, bool defaultValueIfNotFound, BinderOptions? binderOptions)
{
    foreach (IConfigurationSection section in configuration.GetChildren())
    {
    }
}

It never had a chance of doing anything. The generator has already decided the element type is unusable and says so, reporting SYSLIB1100 ("The collection element type is not supported"), and then emits binding code for it anyway.

Root cause

TypeIndex.HasBindableMembers asked the same question of every collection:

CollectionSpec collectionSpec => CanBindTo(collectionSpec.ElementTypeRef),

That is the right question for a dictionary. Dictionary entries are populated in place (instance.TryGetValue(key, out element) then BindCore(section, ref element, ...)), so an element type that cannot be constructed is still perfectly bindable.

It is the wrong question for a list, array or set. Those construct a fresh element and then append it, so the element type must be instantiable, not merely bindable. When it is not, the switch in EmitBindingLogicForEnumerableWithAdd (ParsableFromStringSpec / ConfigurationSectionSpec / ComplexTypeSpec when CanInstantiate) matches nothing and the foreach body comes out empty.

The distinction only becomes visible for an element type that is bindable but not instantiable, such as an abstract class with properties. System.Net.EndPoint in the original report is exactly that. An abstract class with no members is already caught earlier, because CanBindTo is false for it, which is why the existing tests never saw this.

Fix

Ask CanConstructElementsOf instead, which requires a complex element type to be instantiable. The collection then genuinely has nothing to bind, so no BindCore is generated for it, and the element type is no longer reachable through it.

Registration still reports the collection as supported (via CanInstantiate), so the call site remains intercepted rather than silently falling back to the reflection binder, which would regress trimming and AOT.

Two knock-on effects needed handling:

  • ConfigurationBinder.EmitMethods gated the entire interceptor body on HasBindableMembers. Flipping that flag left the emitted Bind_X methods completely empty, dropping ArgumentNullException.ThrowIfNull(configuration) and the GetBinderOptions call that throws NotSupportedException for BindNonPublicProperties. Both are observable behaviour of the reflection binder, so argument validation is now emitted regardless of whether there is anything to bind. This accounts for most of the baseline churn below.

  • CoreBindingHelpers.EmitBindCoreMainMethod asserted HasBindableMembers for every registered type. That assert could already fire before this change (services.Configure<TypeWithNoMembers>(config) crashed the generator in a Debug build) and legitimately fires more often now, so it is removed. A new test covers the Configure<T> path so it cannot be reinstated silently.

Known remaining artefact

For a get-only collection member, the fix leaves an empty if where the call to the removed method used to be:

global::EndPointCollection? temp2 = instance.EndPoints;
if (temp2 is not null)
{
}

This is not new to the generator. A get-only property of any type with nothing to bind, such as TypeWithNoMembers, already produces exactly this shape today, and settable members are unaffected because instance.X ??= new() is still meaningful. Tightening it means changing the object-member path as well, which is orthogonal to this issue, so I have left it alone. Happy to do it as a follow-up if you would prefer.

Also unchanged, and worth stating explicitly: with BinderOptions.ErrorOnUnknownConfiguration = true, the reflection binder throws for non-instantiable elements while the generator does not. That divergence is identical before and after this change.

…able elements

Fixes dotnet#92151.

TypeIndex.HasBindableMembers treated every collection the same way as a
dictionary, asking only whether the element type could be bound. That is the
right question for a dictionary, whose entries are populated in place, but not
for a list, array or set: those construct each element and then append it. When
the element type cannot be constructed, the switch in
EmitBindingLogicForEnumerableWithAdd matches nothing and the generator emits

    public static void BindCore(..., ref List<AbstractElement> instance, ...)
    {
        foreach (IConfigurationSection section in configuration.GetChildren())
        {
        }
    }

The generator already reports SYSLIB1100 for this shape, so it was emitting
binding code for a type it had just warned about.

Ask CanConstructElementsOf instead, which requires a complex element type to be
instantiable. The collection then has nothing to bind, so no BindCore is
generated for it, the element type is no longer reachable through it, and the
dead s_configKeys_ caches, parse helpers and stray using directives that existed
only to serve that method disappear with it.

Registration still reports the collection as supported (CanInstantiate), so the
call site remains intercepted rather than silently falling back to the
reflection binder, which would regress trimming and AOT.

Two consequences of flipping that flag needed handling:

- ConfigurationBinder.EmitMethods gated the whole interceptor body on
  HasBindableMembers, so the emitted Bind_X methods became completely empty and
  dropped the ArgumentNullException.ThrowIfNull(configuration) and the
  GetBinderOptions call that throws for unsupported BinderOptions. Argument
  validation is now emitted regardless, matching the reflection binder.
  EmitCheckForNullArgument_WithBlankLine is split so the blank line is optional.

- CoreBindingHelpers.EmitBindCoreMainMethod asserted HasBindableMembers for
  every registered type. That assert could already fire before this change
  (services.Configure<TypeWithNoMembers>(config) crashed the generator in a
  Debug build), and legitimately fires more often now. Removed.

MinimalGenerationIfNoBindableMembers gains a List<AbstractType_CannotInit_WithMembers>,
an abstract element type that is bindable but not instantiable. The existing
AbstractType_CannotInit has no members, so it never reached this path. Its
baseline interceptor now contains argument validation and nothing else.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: a02a8e4e-7f3e-4c0f-96e0-4206f8b0355a
Copilot AI lite review requested due to automatic review settings August 3, 2026 11:27
@rosebyte rosebyte changed the title Rosebyte scaling disco Skip making empty loops for collections of non-bindable elements Aug 3, 2026
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines:
Successfully started running 4 pipeline(s).
12 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the ConfigurationBinder source generator so it no longer emits “empty” BindCore methods for non-dictionary collections whose element type can’t be constructed (e.g., abstract element types), while preserving interceptor behavior (argument validation / unsupported-option checks) and updating tests/baselines accordingly.

Changes:

  • Adjusts generator type analysis so collection HasBindableMembers reflects “can construct elements” for list/array/set-like collections, avoiding generation of no-op BindCore loops.
  • Preserves interception semantics by emitting argument validation (and GetBinderOptions checks where applicable) even when there’s nothing to bind.
  • Adds regression tests for the new scenarios and updates generator baselines.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/TypeIndex.cs Changes collection bindability to require constructible elements; adds helper to compute that.
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Specs/BindingHelperInfo.cs Avoids registering/recursing into element types when a non-dictionary collection would have no binding logic; preserves “supported” status via instantiation.
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/Helpers.cs Refactors null-check emission to optionally include a blank line (formatting support for new emitter paths).
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/CoreBindingHelpers.cs Removes an assert that can legitimately fail for “nothing to bind” types and documents the no-op binding path.
src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/ConfigurationBinder.cs Ensures intercepted bind methods still validate arguments / options even when no binding logic is emitted.
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.cs Adds generator regression tests for non-instantiable element collections and Configure<T> no-op binding.
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/GeneratorTests.Baselines.cs Updates baseline expectations (notably diagnostic counts).
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/Version*/EmptyConfigType.generated.txt Baseline updates reflecting new emitted validation/no-op paths and reduced usings.
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/netcoreapp/ConfigurationBinder/Version*/Bind_ParseTypeFromMethodParam.generated.txt Baseline updates for argument validation and binder-options checks in intercepted methods.
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/SourceGenerationTests/Baselines/net462/** Same baseline updates for net462 output shape.
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.TestClasses.Collections.cs Adds test model types for non-instantiable element scenarios.
src/libraries/Microsoft.Extensions.Configuration.Binder/tests/Common/ConfigurationBinderTests.Collections.cs Adds runtime behavior tests for non-instantiable element collections and null-handling validation.

Copilot AI review requested due to automatic review settings August 6, 2026 14:22

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/libraries/Microsoft.Extensions.Configuration.Binder/gen/Emitter/ConfigurationBinder.cs:147

  • For the key-based Bind overload, the generated interceptor currently checks instance for null and returns before evaluating configuration.GetSection(key) (because configExpression is only used in the subsequent BindCore(...) call). This diverges from the reflection binder (configuration.GetSection(key).Bind(instance)), which resolves the section (and validates key) even when instance is null. Observable impact: configuration.Bind(nullKey, (T)null) should throw ArgumentNullException, but the interceptor would return silently.

Since you already computed resolvesSection, consider using it in the bindable-members path by forcing section resolution before the null-instance early return, and then passing the resolved section to BindCore.

                    // Only the key-based overload resolves a section; the others bind the configuration itself.
                    bool resolvesSection = configExpression != Identifier.configuration;

@rosebyte
rosebyte marked this pull request as ready for review August 24, 2026 12:16
Copilot AI review requested due to automatic review settings August 24, 2026 12:16
Resolves an adjacent-insertion conflict in GeneratorTests.cs, where main's
PropertyExcludedFromBindingDoesNotReportItsType test and this branch's two
generator tests were added at the same location. All three are kept.

Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Copilot-Session: fe18c0ab-0d99-4094-a963-419b2febb077

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 24, 2026 12:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

Copilot AI review requested due to automatic review settings August 24, 2026 13:07
@rosebyte
rosebyte force-pushed the rosebyte-scaling-disco branch from bcd0a72 to df83476 Compare August 24, 2026 13:09

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated 1 comment.

@rosebyte
rosebyte merged commit e977050 into dotnet:main Aug 31, 2026
78 of 80 checks passed
@dotnet-milestone-bot dotnet-milestone-bot Bot added this to the 12.0-preview1 milestone Aug 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-Extensions-Configuration source-generator Indicates an issue with a source generator feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ConfigurationBinder source generator shouldn't generate empty methods

4 participants