Add support for immutable collections with CollectionBuilderAttribute

[StephenMolloy]

Problem

XmlSerializer cannot correctly round-trip immutable collections (ImmutableArray, ImmutableList, ImmutableHashSet, ImmutableSortedSet, ImmutableStack, ImmutableQueue, and — since it also carries [CollectionBuilderAttribute] — ReadOnlyCollection).

Behavior today ranges from bad to worse depending on the type:

ImmutableArray with a nullable-reference or nullable-enum element type (e.g. ImmutableArray<ConsoleColor?>) crashes with an AccessViolationException during deserialization — the original repro in #66264.
ImmutableList, ImmutableStack, ImmutableQueue silently deserialize as empty collections. No exception; data is lost.
ImmutableHashSet, ImmutableSortedSet, ReadOnlyCollection either throw a construction error at serializer creation time or fail at deserialization because there is no usable public parameterless constructor and no Add method to call.
Root cause: XmlSerializer's collection-deserialization pipeline is built around new T() + repeated Add(item), which is a poor match for immutable / read-only collection types.

Fix

Recognize [CollectionBuilderAttribute] (added to these types in .NET 8 as part of the collection-expressions work) and route their construction through the associated static factory method (Create(T[]) or Create(ReadOnlySpan)) instead of trying to new an instance and call Add.

Applied consistently across all three deserialization paths:

• Type classification (Types.cs) — new TypeFlags.UsesCollectionBuilder and TypeDesc.CollectionBuilderMethod. Detection block in ImportTypeDesc finds the factory method and marks the type so downstream code can dispatch on it. Bypass the default-indexer and Add-method requirements when a [CollectionBuilder] is present. IDictionary-implementing types are explicitly excluded so their pre-existing NotSupportedException behavior is preserved.
• Reflection-based reader (ReflectionXmlSerializationReader.cs) — skip up-front Activator.CreateInstance for builder-backed types; accumulate items into a T[] and invoke the factory at end-of-collection. For factories whose only overload takes ReadOnlySpan (a ref struct that can't pass through MethodBase.Invoke), build a small Expression<Func<Array, object>> that performs the Array → ReadOnlySpan implicit conversion and the factory call.
• ILGen reader (XmlSerializationReaderILGen.cs) — Member gets an IsBuilderBacked flag; WriteMemberBegin declares a T[] accumulator local (reusing the existing xml-array EnsureArrayIndex/ShrinkArray growth machinery so per-element append IL is unchanged); WriteMemberEnd emits ShrinkArray → Castclass → optional Array.Reverse (for ImmutableStack to preserve enumeration order across round-trips) → optional op_Implicit conversion → factory Call.
• ILGen writer (XmlSerializationWriterILGen.cs) — types that use [CollectionBuilder] but have no this[int] indexer (e.g. ImmutableHashSet) are routed through the existing IEnumerable/foreach serialization path instead of the for(int i; collection[i]) indexer path.

Out of scope for this PR

IDictionary-implementing types (e.g. ImmutableDictionary<,>, ImmutableSortedDictionary<,>) — XmlSerializer doesn't directly support dictionaries in general (GetDefaultIndexer unconditionally throws), so those keep throwing NotSupportedException.

DataContractSerializer — see #21016; that will be a separate PR that applies the same strategy to the DCS code paths (where dictionary types are in scope).

Tests

src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs:

• Rewrote Xml_ImmutableCollections as a [Theory] covering ImmutableArray, ImmutableArray, ImmutableArray<ConsoleColor?> (the exact [Feature Request] Add xml serialization support for immutable arrays #66264 repro), ImmutableList, ImmutableList, ImmutableHashSet, ImmutableSortedSet, ImmutableStack, ImmutableQueue. All round-trip cleanly.
• Added Xml_UnsupportedImmutableCollections — asserts NotSupportedException still thrown for ImmutableDictionary<,> and ImmutableSortedDictionary<,>.
• Updated Xml_ReadOnlyCollection to expect successful round-trip (it was previously asserting the error case, but ReadOnlyCollection also carries [CollectionBuilder] and now works).

Fixes #66264

[Copilot]

Pull request overview

This PR updates System.Private.Xml’s XmlSerializer pipeline to recognize [CollectionBuilderAttribute] on immutable / read-only collection types and to construct them via their associated static factory method rather than via new T() + repeated Add(...). This is implemented across type classification, reflection-based deserialization, ILGen deserialization, and ILGen serialization, with tests updated to validate round-tripping for common immutable collection types.

Changes:

• Extend type import/classification to detect [CollectionBuilder], record the bound factory MethodInfo, and relax constructor/Add/indexer requirements for builder-backed collections.
• Update reflection-based and ILGen readers to accumulate items into a T[] and finalize by invoking the builder factory (including handling ReadOnlySpan<T> factory parameters).
• Update ILGen writer to use foreach-based serialization for builder-backed collections that don’t have a usable this[int] indexer; update tests accordingly.

Reviewed changes

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

Show a summary per file

File	Description
src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs	Updates tests to expect successful round-trips for immutable collections and ReadOnlyCollection<T>, and adds coverage for unsupported immutable dictionaries.
src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriterILGen.cs	Routes builder-backed, non-indexable collections through the existing IEnumerable serialization path.
src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs	Adds ILGen support for builder-backed deserialization by accumulating into arrays and invoking the builder factory (with optional reversal for stacks).
src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs	Allows array/collection type dispatch in generated reader code when the type is builder-backed (no default ctor required).
src/libraries/System.Private.Xml/src/System/Xml/Serialization/Types.cs	Adds UsesCollectionBuilder + CollectionBuilderMethod plumbing and implements builder discovery during type import.
src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs	Adds reflection-mode builder-backed deserialization, including a compiled-invoker cache for ReadOnlySpan<T> factory methods.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[mconnew]

ConditionalWeakTable performance characteristics last time I checked weren't great. For a pending fix to DispatchProxy we took an approach where if you are currently running in the default ALC, and the type involved is also in the default ALC, we used a regular HashTable for speed (or might have been dictionary with locks, but it was faster), and used the CWT only when needed. Now DispatchProxy has a lot less overhead so the performance might be less impactful here. It's worth either doing some basic A/B performance testing or create an adhoc microbenchmark and see if this shows up in a profile as significant at all. I'm not super concerned as this is a new opt-in feature so no impact to existing customers so depending on other workload you have, this isn't horrendous so I'm okay if it's not a priority and is skipped.

[mconnew]

The on wire format is just an array or list of elements. That has a slight impendence mismatch with a Stack as it's just semantics of how you are using the Stack as to what on wire order should appear as in a Stack. For example, if on wire you had the numbers 1,2,3, if semantically this was intended to directly represent a stack similar to how it's laid out in RAM on a thread stack, you would pop the 3 off first as your thread stack has the most recent value last. But if instead it's semantically saying this is the order I want them to be made available in the Stack as I pop each item, you would want 1 to be popped first. There's nothing about declaring you are using a Stack that says which way around is the correct way. The only thing we absolutely need to be concerned about getting right is if you serialize a Stack, that it deserializes so that they pop in the same order as the original Stack.
Basically this is a long winded way of saying we should document which way up a Stack is represented when serialized with the XmlSerializer. I believe there's an XmlSerializer document all about collections, so we should add it as a note there. I don't think there's a correct way you can say it should be, just needs to round trip so you get back what you originally had.

[mconnew]

You can do this without expressions. You can create a generic static method which does the cast and call. Then in your code you can make it concrete. This is more AOT friendly than expressions which last I was aware (it's possible they improved it) results in an interpreted implementation using lots of reflection api's to achieve the work at runtime. Also personally I hate reading Expression based code, I have to think in IL. Using a static method skips that. Something like this should work:

private delegate TResult CollectionBuilderSpanFactory<TElement, TResult>(ReadOnlySpan<TElement> values);

private static readonly MethodInfo s_createCollectionBuilderSpanInvokerMethod =
    typeof(/* containing type */)
        .GetMethod(nameof(CreateCollectionBuilderSpanInvoker), BindingFlags.NonPublic | BindingFlags.Static)!;

// [1] This method is closed with the actual element type and factory return type.
//     Inside this method, ReadOnlySpan<TElement> is a normal strongly typed local,
//     so it never needs to be boxed or passed through object?[].
private static Func<Array, object?> CreateCollectionBuilderSpanInvoker<TElement, TResult>(MethodInfo factory)
{
    Debug.Assert(factory.IsStatic);

    ParameterInfo[] parameters = factory.GetParameters();
    Debug.Assert(parameters.Length == 1);
    Debug.Assert(parameters[0].ParameterType == typeof(ReadOnlySpan<TElement>));
    Debug.Assert(factory.ReturnType == typeof(TResult));

    var typedFactory =
        (CollectionBuilderSpanFactory<TElement, TResult>)factory.CreateDelegate(
            typeof(CollectionBuilderSpanFactory<TElement, TResult>));

    return arr =>
    {
        // [2] This is the readable replacement for the Expression version:
        //
        //     Expression.Convert(p, typeof(TElement[]))
        //     op_Implicit(TElement[] -> ReadOnlySpan<TElement>)
        //     factory(span)
        //
        // The array cast also preserves the old behavior:
        // if the supplied Array is not actually a TElement[], the call fails.
        TElement[] typedArray = (TElement[])arr;
        ReadOnlySpan<TElement> span = typedArray;

        return typedFactory(span);
    };
}

// If collectionType has a [CollectionBuilder] (e.g. ImmutableArray/List/Stack/Queue/HashSet/SortedSet),
// accumulate items into a typed array and invoke the static factory method instead of trying to
// construct the type directly + call Add (which would fail or silently no-op for immutables).
private static bool TryBuildWithCollectionBuilder(
    [DynamicallyAccessedMembers(TrimmerConstants.AllMethods)] Type collectionType,
    CollectionMember collectionMember,
    out object? built)
{
    built = null;

    Type? elementType = GetEnumerableElementType(collectionType);
    if (elementType == null)
        return false;

    MethodInfo? factory = TypeScope.GetCollectionBuilderMethod(collectionType, elementType);
    if (factory == null)
        return false;

    Array buffer = Array.CreateInstance(elementType, collectionMember.Count);
    for (int i = 0; i < collectionMember.Count; i++)
    {
        buffer.SetValue(collectionMember[i], i);
    }

    // ImmutableStack.Create(arr) pushes arr[0] first so arr[last] ends up on top, which inverts
    // enumeration order on round-trip. Reverse the buffer first so the deserialized stack enumerates
    // in the same order as the serialized one.
    if (collectionType.IsGenericType &&
        collectionType.GetGenericTypeDefinition().FullName == "System.Collections.Immutable.ImmutableStack`1")
    {
        Array.Reverse(buffer);
    }

    object? arg = buffer;

    ParameterInfo[] pars = factory.GetParameters();
    Debug.Assert(pars.Length == 1);

    Type paramType = pars[0].ParameterType;
    if (paramType.IsGenericType && paramType.GetGenericTypeDefinition() == typeof(ReadOnlySpan<>))
    {
        // [4] ReadOnlySpan<T> is a ref struct, so do not use MethodBase.Invoke here.
        //     Instead, close a generic helper over TElement and TResult. That helper uses
        //     MethodInfo.CreateDelegate to bind the factory to:
        //
        //         TResult Factory(ReadOnlySpan<TElement>)
        //
        //     and returns the same non-generic wrapper shape this method wants:
        //
        //         Func<Array, object?>
        //
        //     The wrapper is cached per closed factory MethodInfo, so the delegate binding cost
        //     is paid once for each closed collection builder factory.
        Func<Array, object?> invoker = s_collectionBuilderSpanInvokerCache.GetValue(factory, static staticFactory =>
        {
            Type staticParamType = staticFactory.GetParameters()[0].ParameterType;
            Debug.Assert(staticParamType.IsGenericType);
            Debug.Assert(staticParamType.GetGenericTypeDefinition() == typeof(ReadOnlySpan<>));

            Type staticElementType = staticParamType.GetGenericArguments()[0];
            Type staticReturnType = staticFactory.ReturnType;

            MethodInfo closedInvokerFactory =
                s_createCollectionBuilderSpanInvokerMethod.MakeGenericMethod(
                    staticElementType,
                    staticReturnType);

            return (Func<Array, object?>)closedInvokerFactory.Invoke(
                null,
                new object?[] { staticFactory })!;
        });

        built = invoker(buffer);
        return true;
    }

    built = factory.Invoke(null, new object?[] { arg });
    return true;
}

[mconnew]

The type IDictionary<TKey, TValue> doesn't derive from IDictionary so it's possible to implement the former without implementing the latter. Do we also need to protect against the generic typed dictionary too?

[mconnew]

If you don't want to use the attribute and are just checking for its presence, use collectionType.IsDefined(typeof(CollectionBuilderAttribute), inherit: false). GetCustomAttribute<T> under the hood creates an instance of every attribute declared on the type, which can have quite a few knock on effects. In general, we should be getting the CustomAttributesData to avoid instantiating the instance. IsDefined creates the CustomAttributesData instances under the hood and will be faster, and safer.

[mconnew]

Use GetCustomAttributesData instead, it's safer and faster. You will need to filter to the specific one you want. It doesn't check the inheritance chain so you don't need to specify (and can't).

[mconnew]

The docs specifically say only Create<T>(ReadOnlySpan<T> values). If you are looking for an array parameter first, that might have a different function/behavior and could do the wrong thing. The docs say:

methodName must refer to a static method that accepts a single parameter of type ReadOnlySpan and returns an instance of the collection being built containing a copy of the data from that span.

[mconnew]

It's going to be faster to use:

foreach (PropertyInfo p in typeof(C).GetDefaultMembers().OfType<PropertyInfo>())
{ .... }

You will need to check p.GetMethod is { IsStatic : false } as technically someone can declare a static property as a default member. When you have types with often a large number of properties, this will narrow down the set of properties which are the indexer property to a single property straight away.

[github-actions]

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "cc57837f8a464374140ca299dee493ecbc162aa4",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "6c5849144dc8a23aa0760080d6f7784fed60da37",
  "last_reviewed_commit": "cc57837f8a464374140ca299dee493ecbc162aa4",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "6c5849144dc8a23aa0760080d6f7784fed60da37",
  "last_recorded_worker_run_id": "29683999187",
  "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": "cc57837f8a464374140ca299dee493ecbc162aa4",
      "review_id": 4730637838
    }
  ]
}

[github-actions]

Holistic Review

Motivation: Justified. XmlSerializer genuinely mishandles [CollectionBuilder]-backed immutable/read-only collections today — ranging from an AccessViolationException (the #66264 repro) to silent data loss (empty ImmutableList/Stack/Queue) to hard construction failures. The root cause (a new T() + Add(item) pipeline that is a poor fit for immutable types) is correctly identified.

Approach: Sound in principle. Recognizing [CollectionBuilder] and routing construction through the static factory (Create(T[]) / Create(ReadOnlySpan<T>)) is the right mechanism, and the accumulate-into-T[]-then-invoke design reuses the existing xml-array EnsureArrayIndex/ShrinkArray machinery cleanly. The ReadOnlySpan<T> handling (compiled Expression invoker in the reflection reader, cached via ConditionalWeakTable; op_Implicit in ILGen) is a reasonable way around the ref-struct/MethodBase.Invoke limitation. IDictionary exclusion to preserve existing NotSupportedException behavior is appropriate.

Summary: ⚠️ Needs Human Review. The ILGen and reflection deserialization paths are well-implemented and thoroughly tested, and the writer change is correct. My one substantive concern is the sgen (Microsoft.XmlSerializer.Generator) code-generation path in XmlSerializationReader.cs: the PR flips the enablement condition at line 3166 to admit builder-backed types, but does not add corresponding factory-construction logic to that file's WriteMemberBegin/WriteMemberEnd, and the sgen backend is excluded from the added tests. A human who knows the XmlSerializer generator should confirm whether that path now emits incorrect code for immutable collections or whether the enablement is intentionally inert there.

---

Detailed Findings

⚠️ Scope/Correctness — sgen backend enabled but not implemented (inline on XmlSerializationReader.cs:3166)

The change enables builder-backed array mappings in the source-generating reader (XmlSerializationReaderCodeGen), but only the ILGen and reflection backends received actual factory-construction logic. See the inline comment for the full analysis and suggested resolutions. This is the main reason for the Needs-Human-Review verdict.

✅ ILGen deserialization — correct and well-integrated

XmlSerializationReaderILGen.cs adds an IsBuilderBacked accumulator that reuses EnsureArrayIndex/ShrinkArray, then emits ShrinkArray → Castclass → (optional Array.Reverse for ImmutableStack) → (optional op_Implicit) → factory Call → ConvertValue. The per-element append IL is unchanged, and the stack-reversal correctly preserves enumeration order on round-trip. The comment explaining why T[] is used instead of List<T> is accurate.

✅ Reflection deserialization — correct; good caching hygiene

TryBuildWithCollectionBuilder builds a typed buffer and invokes the factory; the ReadOnlySpan<T> compiled-invoker cache keyed on the bound MethodInfo via ConditionalWeakTable is a nice touch that both amortizes Expression.Compile and avoids pinning a collectible AssemblyLoadContext. Setting o = null for UsesCollectionBuilder members (line ~1228) correctly avoids the pointless up-front Activator.CreateInstance.

✅ Writer path — correct routing for indexer-less immutables

XmlSerializationWriterILGen.WriteArrayItems now routes UsesCollectionBuilder types lacking a usable int indexer (e.g. ImmutableHashSet<T>) through the IEnumerable/foreach path instead of the collection[i] indexer path. HasUsableIntIndexer correctly requires a single int parameter and a public getter.

✅ Type classification — reasonable, with a minor robustness note

Types.cs detection is coherent: TryGetCollectionBuilderElementType gates on a single-arg generic with the attribute; GetCollectionBuilderMethod prefers Create(T[]) then falls back to Create(ReadOnlySpan<T>), validates return-type assignability, and swallows MakeGenericMethod failures. The CheckNeedConstructor relaxation and the IDictionary exclusion are appropriate. The IL3050/RequiresDynamicCode handling is consistent with the surrounding ILGen-based serializer.

💡 Stack/Queue ordering relies on a string type-name match

Both the ILGen and reflection paths special-case ImmutableStack by comparing GetGenericTypeDefinition().FullName == "System.Collections.Immutable.ImmutableStack1". This works and is documented, but a literal type-name string is fragile relative to a typeof(...)comparison against the referenced type. SinceSystem.Collections.Immutable` may not be a hard reference here, the string approach is understandable — just flagging it as a maintenance/robustness consideration, not a blocker.

✅ Tests — good coverage for the supported backends

The rewritten Xml_ImmutableCollections [Theory] covers ImmutableArray(int/string), the exact ImmutableArray(ConsoleColor?) #66264 repro, ImmutableList, ImmutableHashSet, ImmutableSortedSet, ImmutableStack, and ImmutableQueue with order-aware assertions (set-comparison for ImmutableHashSet). Xml_UnsupportedImmutableCollections locks in NotSupportedException/InvalidOperationException for ImmutableDictionary/ImmutableSortedDictionary across both modes, and Xml_ReadOnlyCollection is correctly updated to expect a successful round-trip. The gap is the sgen backend (XMLSERIALIZERGENERATORTESTS), tied to the finding above.

Note

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

Generated by Holistic Review · 166.8 AIC · ⌖ 11.8 AIC · ⊞ 10K

[github-actions]

This enables builder-backed (immutable) collection types in the sgen code-generation backend (XmlSerializationReaderCodeGen, used by Microsoft.XmlSerializer.Generator), but the corresponding construction logic is not implemented in this file. Unlike the ILGen reader (XmlSerializationReaderILGen.cs, which gained a full IsBuilderBacked path in WriteMemberBegin/WriteMemberEnd) and the reflection reader (TryBuildWithCollectionBuilder), the source-generating WriteMemberBegin/WriteMemberEnd here have no builder-factory handling: a builder-backed member is neither IsArray nor IsValueType, so WriteMemberEnd emits no assignment and WriteMemberBegin falls into the new T() + Add path. As a result, enabling the array mapping at this line can cause the generator to emit code that fails to construct the immutable collection (silently empty or non-compiling) for exactly the types this PR intends to fix.

Because the sgen tests are excluded (#if !XMLSERIALIZERGENERATORTESTS), this gap is not covered by the added tests. Please either (a) implement the builder-factory construction in this file's WriteMemberBegin/WriteMemberEnd and add sgen coverage, or (b) leave this condition unchanged for the sgen backend so it continues to throw a clear "unsupported" error rather than generating incorrect code. A human familiar with the XmlSerializer generator should confirm which behavior is intended.