Support deserializing read-only collections in XmlSerializer via [CollectionBuilder] - #132356
Draft
StephenMolloy wants to merge 3 commits into
Draft
Support deserializing read-only collections in XmlSerializer via [CollectionBuilder]#132356StephenMolloy wants to merge 3 commits into
StephenMolloy wants to merge 3 commits into
Conversation
XmlSerializer could not deserialize collections that cannot be created empty and then added to one element at a time. Types like ImmutableArray<T>, ImmutableList<T>, ReadOnlyCollection<T> and FrozenSet<T> either threw at import or silently deserialized as empty. Add a secondary "accumulate then hydrate" path for these. When a collection cannot be constructed and populated the usual way, but carries a [CollectionBuilder] attribute, elements are accumulated into an ephemeral array and the real instance is created once at the end of the collection by calling the builder factory method. Collections without the attribute keep their existing behavior exactly. The path is implemented in all three readers: the RefEmit reader, the reflection reader, and the pregenerated source generator. Detection uses GetCustomAttributesData so no extra types or assemblies are loaded, and the span-based builder overload is preferred, with the T[] overload as a fallback where dynamic code is unavailable. Nil and absent members keep the long-standing behavior of producing an empty collection rather than null, which required pre-creating the collection in WriteMemberBegin as well as hydrating in WriteMemberEnd. Get-only collection members are left alone rather than assigned, since a collection built from its complete contents has nowhere to be stored without a setter. SOAP encoding is out of scope and guarded so builder-backed types report a clear error, and IDictionary remains unsupported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f550422-5e7b-45fe-9467-28d84726d702
Member level coverage only used properties, so add a field to TypeWithReadOnlyCollections. Fields are assigned through a different path in all three readers. That exposed a divergence for struct collections. The reflection reader guarantees a collection member is never left uninitialized by checking whether it is null at the end of the read, but ImmutableArray<T> boxes to a non-null default, so an absent member kept its default value where the IL generating and pregenerated readers produced an empty collection. Treat a boxed default as uninitialized as well. The existing member test missed this because it exercised a nil element rather than an absent one, and every other member was a reference type. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f550422-5e7b-45fe-9467-28d84726d702
CollectionBuilderAttribute's contract requires a Create(ReadOnlySpan<T>) overload, so CollectionBuilderInfo now requires one and drops the Factory and FactoryTakesSpan members that let callers ask which shape they got. The generating readers always call the span overload; the array overload is kept only as the reflection-based reader's fallback for when dynamic code is unavailable and it cannot close a generic type over the element type. The two invoker classes collapse into a single Func<Array, object>, and the doc comments are trimmed to match an internal helper. Fold the encoded-serialization read-only and collection builder throws in WriteAddCollectionFixup together, since they raise the same exception. Combine the member-level nil and absent tests into one theory covering both cases, and cover a get-only collection declared as repeated elements rather than a wrapped array. That shape reaches a different path in the reflection-based reader, which throws NotSupportedException without the guard that skips it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: f550422-5e7b-45fe-9467-28d84726d702
|
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. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends System.Xml.Serialization.XmlSerializer to support deserializing read-only / immutable collections that opt into construction via System.Runtime.CompilerServices.CollectionBuilderAttribute, by accumulating elements into an array and hydrating the final collection via the builder factory when in-place population (new + Add/indexer) isn’t possible.
Changes:
- Add
CollectionBuilderInforesolution and plumbTypeDesc.UsesCollectionBuilderthrough type classification to enable an alternate “accumulate then build” read path. - Update the reflection reader and both codegen backends (ILGen + sgen) to deserialize builder-backed collections using the existing array accumulation machinery, then build the final instance.
- Add tests and new serialization test types covering root/member scenarios, nil/absent semantics, get-only members, and negative cases (e.g., dictionaries).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime.Serialization.Xml/tests/SerializationTypes.cs | Adds new test types with immutable/read-only collection members to exercise builder-backed deserialization scenarios. |
| src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs | Adds/updates test coverage for root and member deserialization of builder-backed collections, nil/absent semantics, and unsupported shapes. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs | Treats builder-backed collections as array-like during read, and emits hydration via the resolved builder at end-of-collection. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs | Updates the sgen code generator to emit builder-backed collection hydration and to guard unsupported encoded/fixup paths. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/Types.cs | Introduces TypeDesc.CollectionBuilder/UsesCollectionBuilder and updates import/element-type resolution to use the builder fallback when needed. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs | Implements reflection-path accumulation + builder hydration; updates “ensure collection” semantics for struct-backed collections (e.g., ImmutableArray<T>). |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/CollectionBuilderInfo.cs | New helper for discovering and invoking [CollectionBuilder] factories (span-first, optional array fallback). |
| src/libraries/System.Private.Xml/src/System.Private.Xml.csproj | Includes the new CollectionBuilderInfo.cs in the build. |
Comment on lines
+131
to
+139
| // Only keep the array overload when it agrees with the span overload, so the two paths cannot disagree | ||
| // about what the collection contains. | ||
| if (arrayFactory != null && arrayFactory.GetParameters()[0].ParameterType.GetElementType() != elementType) | ||
| { | ||
| arrayFactory = null; | ||
| } | ||
|
|
||
| return new CollectionBuilderInfo(elementType, spanFactory, spanConversion, arrayFactory); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes: #66264
Supersedes #130455, which took a narrower approach. This branch is a fresh implementation.
Problem
XmlSerializerpopulates a collection by constructing it and then callingAdd()(orthis[int]) once per element. Read-only and immutable collections cannot be populated that way, so today they fall into one of three buckets:ImmutableHashSet<T>,FrozenSet<T>,ReadOnlySet<T>,ReadOnlyCollection<T>,ImmutableStack<T>,ImmutableQueue<T>InvalidOperationException, "...does not implement Add(System.Object)" / "...does not have a default accessor"ImmutableList<T>,ImmutableSortedSet<T>Addreturns a new instance which is discarded, so the member always deserializes emptyImmutableArray<T>NotSupportedExceptionfromIList.Add, or an NRE for a nil rootApproach
This adds a secondary construction path driven by
System.Runtime.CompilerServices.CollectionBuilderAttribute. When a collection type carries the attribute and cannot be constructed or populated the normal way, the readers accumulate elements into an ephemeralT[], exactly like the existing array path, and at end-of-collection hydrate the real instance by calling the builder factory method.This is deliberately not an "Immutable-namespace" special case. It is a generic fallback for any type that opts in via the attribute, including user-defined ones.
Gating
The builder path is used only when both hold:
[CollectionBuilder]and a usable factory method resolves; andTypeDesc.CannotNew, or no usablethis[int]indexer, or no matchingAdd.Types that work today are untouched: with no attribute present, behavior is byte-identical to before.
Attribute discovery uses
GetCustomAttributesData()rather thanGetCustomAttribute<T>(), so no attribute is instantiated and no extra types or assemblies are loaded during import.Factory resolution and invocation
[CollectionBuilder]requires aCreate(ReadOnlySpan<T>)overload and permits aT[]overload, soCollectionBuilderInforequires the span overload and treats the array overload as optional. The ILGen and sgen backends always emit the span call directly.Because
MethodInfo.Invokecannot pass aReadOnlySpan<T>, the reflection reader binds the closedMethodInfoto a private delegate type viaMethodInfo.CreateDelegate(no expression trees). When!RuntimeFeature.IsDynamicCodeSupportedit cannot close that generic delegate, so it falls back to theT[]overload with a plainInvoke. Note thatReadOnlyCollection.CreateCollection<T>,ReadOnlyCollection.CreateSet<T>andFrozenSet.Create<T>are span-only, so there is no fallback for those three.Types that begin working
ImmutableArray<T>,ImmutableList<T>,ImmutableHashSet<T>,ImmutableSortedSet<T>,ImmutableStack<T>,ImmutableQueue<T>,FrozenSet<T>,ReadOnlyCollection<T>,ReadOnlySet<T>.Behavioral changes
These are the observable differences for code that compiles today:
ImmutableList<T>andImmutableSortedSet<T>members previously deserialized silently empty and now round-trip with their content. Anyone relying on the empty result was relying on data loss, but it is a behavior change nonetheless.ImmutableArray<T>previously threwNotSupportedExceptionduring deserialization and now succeeds.Everything else here turns a thrown exception into working serialization, which is additive.
Known limitations
ImmutableStack<T>andImmutableQueue<T>reverse element order on round-trip. Their builders match C# collection-expression semantics (ImmutableStack.Create(a, b, c)enumerates asc, b, a). This is documented in the tests rather than worked around, so the behavior stays consistent with the language. Reversing on hydration is a reasonable follow-up.XmlSerializerrejectsIDictionaryup front and that guard is unchanged, soImmutableDictionary<,>andFrozenDictionary<,>still throw.ImmutableSortedDictionary<,>has no[CollectionBuilder]at all.IImmutableList<T>are still rejected earlier as interfaces.ImmutableList<T>, and it is why the fix skips the assignment rather than throwing: such a property imports, serializes, and deserializes (as empty) today, so throwing at deserialize would regress it, and throwing at import would break serialize-only scenarios since import is shared by both directions. All three readers agree on this, including for a get-onlyImmutableArray<T>whose getter returns an uninitialized struct.CollectionFixup/WriteAddCollectionFixupmodel defers population until after the object graph is read, which is fundamentally incompatible with hydrate-once construction. Builder-backed types are explicitly guarded there so they throw a clear error rather than producing garbage.Notes for reviewers
UsesCollectionBuilderis not aTypeFlagsbit. It is derived from whether the resolvedCollectionBuilderInfois non-null.GetNullableTypeDescandCreateArrayTypeDesccopy_flagsbut not the builder object, so a flag bit would leak the capability onto derivedTypeDescs that have no builder.CannotNewis not redefined. A builder has no bearing on whether a type can benew-ed; every downstream site that cares checksUsesCollectionBuilderexplicitly.CheckNeedConstructor()is itself builder-aware rather than each call site being patched.null, whether the XML saysxsi:nil="true"or omits the element entirely, while arrays come backnull. That "absent means empty" behavior comes fromWriteMemberBeginpre-creating the collection, not fromWriteMemberEnd, so builder-backed members do both: create empty up front and hydrate at the end.ShrinkArrayis called withisNullable: falseso a nil collection yields an empty array and therefore an empty hydrated collection.Xml_CollectionAsMember_NilOrAbsentBecomesEmptypins this against a mutable control type.TypeScope.ImportTypeDescfor classification andTypeScope.GetArrayElementType->GetCollectionElementTypeused byArrayModel.ElementandXmlReflectionImporter.ImportAccessorMapping. Both had to be taught the same rule or they disagree and the second one re-throws.[UnconditionalSuppressMessage]rather than[RequiresDynamicCode]on theMakeGenericMethodchoke point. PropagatingRequiresDynamicCodewould cascade across every caller ofTypeScope.GetTypeDesc, and everyXmlSerializerpublic entry point is already annotated[RequiresDynamicCode(XmlSerializer.AotSerializationWarning)].Expected.SerializableAssembly.XmlSerializers.csis machine-generated and was regenerated from sgen output. Adding a type toSerializationTypes.csrenumbers everyWrite##_/Read##_method, which is why that part of the diff is large. The%%ParentAssemblyId%%placeholder is preserved.SR.XmlReadOnlyCollection.Testing
New tests live in
XmlSerializerTests.csso they run in all three flavors. They cover root-level and member-level usage, empty and nil collections,ImmutableArray<ConsoleColor?>(the original AV repro from the issue), and sequence-equality comparisons sinceReadOnlyCollection<T>does not overrideEquals. Member-level coverage includes a field as well as properties, since the readers assign fields through a different path. Unsupported shapes are covered by negative tests asserting they still throw.System.Private.Xml.Tests(ILGen)System.Xml.XmlSerializer.ReflectionOnly.TestsMicrosoft.XmlSerializer.Generator.Tests(sgen)System.Runtime.Serialization.Xml.Tests(sharesSerializationTypes.cs)Three divergences between the backends were found by tests rather than by inspection, and are worth knowing about when reviewing this area:
Xml_GetOnlyReadOnlyCollectionAsMember_KeepsGetterValuefailed without the fix in two distinct ways. The ILGen reader hitDebug.Assert(setMethod != null)inCodeGenerator.StoreMemberand took the test host down, becauseMember._isArrayis nowIsArray || UsesCollectionBuilderand get-only collection properties are admitted at import (Models.cs:238) where get-only array properties are not. The reflection reader failed separately with aNullReferenceException: for the[XmlArray]-wrapped shape it never takes themember.Collectionpath and instead installed a reflectiveAdddelegate that calledImmutableArray<T>.Addon an uninitialized struct. The regenerated sgen output was inspected directly to confirm the read method for that type contains zero member assignments.Adding a field to
TypeWithReadOnlyCollectionsexposed the reflection reader'sEnsureCollectionfinalizer, the thing that guarantees a collection member is never left uninitialized, testingGetMemberValue(...) == null. A struct collection such asImmutableArray<T>boxes to a non-null default, so an absent member kept its default value where the other two readers produced an empty collection.EnsureCollectionnow also treats a boxed default as uninitialized.The reflection reader's flat
[XmlElement]shape reachesAddObjectsIntoTargetCollectionand itsIList.Add, which throwsNotSupportedExceptionfor a builder-backed type. The guard that skips it was verified by adding a repeated-element get-only member and confirming the test fails when the guard is removed.The
!RuntimeFeature.IsDynamicCodeSupportedarray fallback inCollectionBuilderInfo.CreateBuilderis not covered by an executed test. It was verified by inspection only, as the libraries test suites all run with dynamic code available.Unrelated pre-existing behavior worth knowing: the reflection reader cannot deserialize into a
readonlyfield of any collection type, failing withArgumentException: Expression must be writeablefrom theExpression.Assignit builds. This reproduces identically with a plainreadonly List<int>field, so it is not caused by this change and is not addressed here. Filed separately as #132357.Note
This pull request description was drafted with GitHub Copilot.