ConfigurationBinder: Skip binding null getter-only collection properties

[Sahithbasani]

Fixes #130879

Summary

The Configuration Binder source generator previously attempted to bind getter-only reference and collection properties even when the current property value was null.

Because getter-only properties cannot be assigned, this could allocate and bind a temporary collection whose value was immediately discarded.

This change reads the current property value into a temporary variable and invokes the binding logic only when that value is non-null.

Testing

• GetterOnlyCollectionProperties on net11.0: passed
• GetterOnlyCollectionProperties on net472: passed
• Full SourceGenerationTests on net11.0: 331 passed, 28 skipped
• Full SourceGenerationTests on net472: 317 passed, 36 skipped
• git diff --check: passed

[azure-pipelines]

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.

[dotnet-policy-service]

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

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "cb40807789903e2fd9d3a8dac7dcd2142db39e7e",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "c1b09d933f7d720f7ae50f80b4ec6980bb93d96f",
  "last_reviewed_commit": "cb40807789903e2fd9d3a8dac7dcd2142db39e7e",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "c1b09d933f7d720f7ae50f80b4ec6980bb93d96f",
  "last_recorded_worker_run_id": "29705659164",
  "review_attempt_commit": "",
  "review_attempt_base_ref": "",
  "review_attempt_count": 0,
  "max_review_attempts": 5,
  "review_history_format": "holistic-review-disclosure-v1",
  "review_history": [
    {
      "commit": "76acc74b65659d49b79ccb56cfa01b980104ffe1",
      "review_id": 4731341490
    },
    {
      "commit": "cb40807789903e2fd9d3a8dac7dcd2142db39e7e",
      "review_id": 4731582598
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Justified. The source generator previously allocated a temporary collection (??= new(...)), fully bound it, then discarded it for getter-only reference/collection properties whose getter returns null — pure wasted work with no observable effect (issue #130879). The reflection binder already short-circuits this case, so closing the parity gap is worthwhile.

Approach: Correct and minimal. In EmitBindingLogicForComplexMember, the reference-type getter branch now special-cases !canSet: it reads the current value into a temp and only invokes binding when it is non-null, then returns early. This mirrors the reflection binder's if (bindingPoint.IsReadOnly) return; short-circuit and avoids the throwaway allocation.

Summary: ✅ LGTM. The change is behavior-preserving: writeOnSuccess was already null when !canSet, so nothing was ever written back; the only difference is skipping allocation and BindCore when the getter returns null, while still binding into an existing non-null instance. Tests exercise both a null and a non-null getter-only collection property, and baselines are regenerated for all four target/version combinations.

---

Detailed Findings

✅ Correctness — Behavior preserved, allocation avoided

For getter-only members the previous code emitted temp ??= new(...) + BindCore(...) with no write-back (since writeOnSuccess = !canSet ? null : ...). The new early-return path binds only when temp is not null, so a caller-initialized collection is still populated, and a null getter no longer triggers a discarded allocation. The generated baselines (e.g. netcoreapp/Version0) confirm the if (temp2 is not null) { BindCore(...); } shape with no ??=.

✅ Scope — Applies to all reference-type getter-only members

The guard sits in the shared member.CanGet reference-type branch, so it also benefits getter-only complex (non-collection) properties, not just collections. This is consistent with the reflection binder and does not change value-type or settable-property paths.

✅ Tests — Adequate coverage

GetterOnlyCollectionProperties covers both a null getter-only List<string>? and an initialized getter-only List<string>, and is validated against regenerated baselines for net462/netcoreapp × Version0/Version1. The baseline diffs match the intended emitted code.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 92.2 AIC · ⌖ 15.8 AIC · ⊞ 10K

[github-actions]

Holistic Review

Motivation: Justified. The configuration source generator previously allocated a temporary collection (??= new(...)), fully bound it, then discarded it for getter-only reference/collection properties whose getter returns null — wasted work with no observable effect (issue #130879). The reflection binder already short-circuits this case, so closing the parity gap is worthwhile.

Approach: Correct and minimal. In EmitBindingLogicForComplexMember, the reference-type getter branch special-cases !canSet: it reads the current value into a temp and only invokes binding when it is non-null, then returns early, mirroring the reflection binder's read-only short-circuit and avoiding the throwaway allocation.

Summary: ✅ LGTM. The change is behavior-preserving: writeOnSuccess was already null when !canSet, so nothing was ever written back; the only difference is skipping allocation and BindCore when the getter returns null, while still binding into an existing non-null instance. Tests exercise both null and non-null getter-only collection properties, with regenerated baselines. Since the initial review, the PR was rebased/retargeted onto a new merge base with no change to the PR patch (patch IDs are identical), so this review adds no new findings and the cumulative assessment is unchanged.

Assessment History

• review 4731341490 reviewed commit 76acc74b65659d49b79ccb56cfa01b980104ffe1 with verdict ✅ LGTM. Current verdict is ✅ LGTM. Unchanged — the PR patch has not changed since the prior review (identical patch IDs after a rebase), so motivation, approach, and risk assessment all remain the same.

Note

This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.

Generated by Holistic Review · 33.3 AIC · ⌖ 15.9 AIC · ⊞ 10K

[svick]

I think adding more tests for this to the tests/Common directory would be nice, to verify that that the source generator binder and the reflection binder still behave the same. These tests should include testing get-only nested objects in addition to collections.

And especially adding a test where the type of the property is abstract. In that case, the old code incorrectly threw an exception, the new code correctly does nothing (just like the reflection binder).

[rosebyte]

LGTM, thank you! Please consider adding the tests @svick mentioned.