[cdac] Generalize LayoutPair to LayoutSet and move IData.md in-tree

[max-charlamb]

Follow-up to #128356 addressing review feedback from @noahfalk:

Changes

1. Generalize LayoutPair to LayoutSet (review comment)

   • Renamed LayoutPairSource.cs -> LayoutSetSource.cs
   • Backed by TypeInfo?[] array instead of fixed native+managed fields
   • Extensible for future layout sources (e.g. symbols) without structural changes
   • Functionally equivalent -- Select/TrySelect/InstanceSize iterate all slots

2. Move IData.md to src/native/managed/cdac/ (review comment)

   • It is implementation guidance for the generator, not a data contract spec
   • Updated LayoutPair -> LayoutSet references in the doc

[dotnet-policy-service]

Tagging subscribers to this area: @steveisok, @tommcdon, @dotnet/dotnet-diag
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

Follow-up rename/refactor of the cDAC IData<T> source generator's layout helper. The fixed two-field LayoutPair(nativeType, managedType) is replaced with a LayoutSet backed by a Target.TypeInfo?[] array, allowing future layout sources (e.g. symbols) to be added without structural changes. The IData.md design doc that previously lived in docs/design/datacontracts/ is relocated next to the generator since it is implementation guidance, not a data contract spec.

Changes:

• Rename LayoutPairSource → LayoutSetSource (file, type, emitted struct, hint name, fully qualified name) and change storage to a TypeInfo?[] with InstanceSize/TrySelect iterating all slots.
• Update generator emit sites (Emitter.cs, CdacGenerator.cs) and comments to reference LayoutSet instead of LayoutPair.
• Move IData.md in-tree and update LayoutPair → LayoutSet references throughout.

Reviewed changes

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

File	Description
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DataGenerator/LayoutSetSource.cs	Renamed from LayoutPairSource.cs; struct now backed by TypeInfo?[] with loop-based InstanceSize/TrySelect; Resolve still aggregates a native + managed slot into the array.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DataGenerator/Emitter.cs	Updates emitted source and comments from LayoutPair to LayoutSet.
src/native/managed/cdac/Microsoft.Diagnostics.DataContractReader.DataGenerator/CdacGenerator.cs	Renames helper-gating flag and references (EmitLayoutPair → EmitLayoutSet, LayoutPairSource → LayoutSetSource).
src/native/managed/cdac/IData.md	Moved in-tree (was under docs/design/datacontracts/); updates LayoutPair → LayoutSet in prose and code examples.

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

src/native/managed/cdac/IData.md:375

• This section says the ctor throws if LayoutSet.Resolve finds neither source, but the current LayoutSet.Resolve implementation can return an empty set / unresolved managed layout and the ctor will instead fail later on the first field lookup (with a different exception/message). Either update LayoutSet.Resolve to preserve the old fail-fast behavior (preferred), or adjust this doc to match the new behavior.

(Separate doc issue: after moving this file, the ManagedTypeSource.md relative link near the top appears to be broken.)

[max-charlamb]

/ba-g cDAC only change, CI passed before comments only change.

[github-actions]

Note

This review was generated by Copilot.

🤖 Copilot Code Review — PR #128716

Holistic Assessment

Motivation: Justified refactor addressing reviewer feedback from a prior PR. Generalizing LayoutPair to LayoutSet prepares the codebase for additional layout sources without structural changes.

Approach: Reasonable — replaces fixed two-field struct with an array-backed lazy-resolution pattern. The rename is consistently applied across code, comments, and docs.

Summary: ⚠️ Needs Human Review. The code is correct for current usage, but two behavioral differences from the original LayoutPair warrant human confirmation.

---

Detailed Findings

⚠️ Behavioral Change — No early throw when no sources exist

The old LayoutPair.Resolve threw InvalidOperationException immediately if neither native nor managed sources were found. The new LayoutSet.Resolve simply returns a LayoutSet with an empty _layouts array — the error is only surfaced later when Select or InstanceSize is called.

This defers the failure point from construction to first use, which can make debugging harder (the stack trace no longer points to the resolution site). If this is intentional (e.g., to support lazy sources that might resolve later), it should be documented. If not, consider adding a guard:

if (layouts.Count == 0)
    throw new InvalidOperationException($"No descriptor available for names=[{string.Join(",", names)}].");

Merge-blocking? Not strictly — existing callers always call Select immediately after Resolve in the generated constructor, so the error still surfaces. But it's a regression in debuggability.

⚠️ Thread Safety — LazyLayout.Get() is not thread-safe

LazyLayout uses a plain field read/write pattern (if (_factory is not null) { _value = _factory(); _factory = null; }) without synchronization. If a LayoutSet instance were shared across threads, _factory could be invoked multiple times or _value read before fully assigned.

In current usage this is likely fine — LayoutSet is constructed and consumed within a single constructor call. However, the class is internal and has no documented threading contract. A brief comment (e.g., "Not thread-safe; instances must not be shared across threads") or using Lazy<T> would prevent future misuse.

Merge-blocking? No — current usage is single-threaded.

✅ Rename consistency

All references to LayoutPair in code, comments, XML docs, and the IData.md design doc have been updated to LayoutSet. No stale references found in the diff.

✅ Doc relocation

Moving IData.md from docs/design/datacontracts/ to src/native/managed/cdac/ is appropriate — it describes generator implementation details, not a public data contract spec.

💡 Minor — List<LazyLayout> allocation in Resolve

Resolve allocates a List<LazyLayout> with initial capacity 2 then calls .ToArray(). For a hot path this is two allocations (list + array). Since the current code always has at most 2 elements, a stack-allocated approach or direct array construction could avoid the list. Low priority given current usage patterns.

Generated by Code Review for issue #128716 · ● 1.5M · ◷