Fix xsi:list round-tripping with space-separated serialization#130428
Fix xsi:list round-tripping with space-separated serialization#130428StephenMolloy wants to merge 5 commits into
Conversation
Fixes dotnet#115837 An array-like member marked with [XmlText] (e.g. `[XmlText] string[]`) did not round-trip: on serialization the items were concatenated into the element's text content with no separator, so deserialization read them back as a single value. Per the XML spec (https://www.w3.org/TR/xml/#sec-common-syn) whitespace is one of only four characters (0x20, 0x09, 0x0D, 0x0A), and per the XSD spec a list type is a whitespace-separated sequence of tokens. This change makes [XmlText] on array-like members serialize as a space-separated list and split on the four XML whitespace characters when reading, mirroring the long-standing [XmlAttribute] list behavior. [XmlAttribute] list deserialization is also aligned to split on exactly those four XML whitespace characters (rather than the broader char.IsWhiteSpace set) so that other whitespace can survive inside list items. The new behavior is applied across all serialization engines: the reflection-based reader/writer, the IL-generated reader/writer, and the Microsoft.XmlSerializer.Generator pre-generated output. Because the previous concatenation behavior was broken for round-tripping, the new behavior is on by default. Callers depending on the old behavior can opt out via the AppContext switch `Switch.System.Xml.Serialization.UseLegacyXmlListSeparation`. Notes: - Whitespace is normalized when reading a list, so a null/empty middle item that serializes to a double space is collapsed and dropped on read (empty/null items are not expected to survive a round trip). Callers are responsible for not embedding XML whitespace inside individual list items. Tests: - Added coverage for [XmlText] on arrays, List<string>, and mixed element/text content, plus [XmlAttribute] arrays, verifying exact serialized output and round-tripping. - Added tests for whitespace normalization / null-item collapsing edge cases. - Added tests with the legacy AppContext switch enabled to verify the old concatenation behavior, and a test pinning the switch's default to false. - New runtime-only test types were placed in SerializationTypes.RuntimeOnly.cs so they are excluded from the pre-generated serializer test projects.
There was a problem hiding this comment.
Pull request overview
This PR changes XmlSerializer list handling so that array-like members serialized as text ([XmlText] string[], List<string>, etc.) round-trip by writing a space-separated list and reading it back by splitting on the four XML whitespace characters (' ', '\t', '\n', '\r'). It also introduces an AppContext opt-out switch (Switch.System.Xml.Serialization.UseLegacyXmlListSeparation) and updates generator/expected outputs plus tests accordingly.
Changes:
- Emit space separators when serializing “pure text” array-like
[XmlText]members (and avoid applying list semantics for mixed text+element content). - Update deserialization to split list text (and attribute list values) on XML’s four whitespace characters, with an opt-out legacy path.
- Add tests covering round-tripping, whitespace normalization edge cases, and the legacy switch behavior.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime.Serialization.Xml/tests/SerializationTypes.RuntimeOnly.cs | Adds runtime-only test types used by serializer tests. |
| src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs | Adds/updates tests for new list behavior + legacy switch coverage. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriterILGen.cs | IL-gen writer emits separators for list-style [XmlText] array-like members. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationWriter.cs | Source-gen writer emits separators for list-style [XmlText] array-like members. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs | IL-gen reader adds XML-whitespace splitting for text lists and attribute lists (with legacy switch). |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReader.cs | Source-gen reader adds XML-whitespace splitting for text lists and attribute lists (with legacy switch). |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlReflectionImporter.cs | Controls when [XmlText] array-like members are treated as list text vs. legacy concatenation/mixed content. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationWriter.cs | Reflection writer emits separators for list-style [XmlText] array-like members. |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs | Reflection reader splits list text/attributes using XML whitespace (with legacy switch). |
| src/libraries/System.Private.Xml/src/System/Xml/Serialization/Mappings.cs | Adds TextAccessor.IsList to carry per-accessor list semantics for [XmlText]. |
| src/libraries/System.Private.Xml/src/System/Xml/Core/LocalAppContextSwitches.cs | Adds cached switch accessor for UseLegacyXmlListSeparation. |
| src/libraries/Microsoft.XmlSerializer.Generator/tests/Expected.SerializableAssembly.XmlSerializers.cs | Updates expected generated serializer output to reflect new behavior. |
…-and-XmlAttribute
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "35301936033455c31cf21db11761094b91b45b08",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "8dd1f60f48798789aea5967a7df682e7803468e7",
"last_reviewed_commit": "35301936033455c31cf21db11761094b91b45b08",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "8dd1f60f48798789aea5967a7df682e7803468e7",
"last_recorded_worker_run_id": "29766987730",
"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": "5e91788a1ca21eccb4ef9c407433be2335ec077a",
"review_id": 4730638129
},
{
"commit": "35301936033455c31cf21db11761094b91b45b08",
"review_id": 4737841507
}
]
} |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Holistic Review
Motivation: The problem is real and well-evidenced (issue #115837): an array-like [XmlText] member (e.g. [XmlText] string[]) concatenated its items with no separator on write, so the value could never round-trip — deserialization read it back as a single item. Aligning with the long-standing [XmlAttribute] list behavior and the XSD xs:list model is a reasonable, spec-grounded fix.
Approach: The fix threads a per-accessor TextAccessor.IsList flag (a strict refinement of Mapping.IsList, narrowed by pure-text/no-mixed-content checks and the opt-out switch) and applies the space-separated write / whitespace-split read consistently across all four engines (reflection reader/writer, IL-gen reader/writer) plus the pre-generated Expected.* output. Splitting on exactly the four XML whitespace characters (#x20 #x9 #xA #xD) rather than char.IsWhiteSpace() is a deliberate, well-justified refinement, and the legacy behavior is preserved behind the opt-out Switch.System.Xml.Serialization.UseLegacyXmlListSeparation. The code is careful and internally consistent.
Summary: val1val2 → val1 val2) and to attribute list splitting semantics, which requires maintainer sign-off on the compat process (breaking-change doc, opt-out polarity) and confirmation of the documented lossy round-trip for empty/null items. A human should focus on those policy decisions rather than the mechanics.
Detailed Findings
⚠️ Compatibility — Default-on behavior change to serialized output and attribute splitting
The new list behavior is enabled by default (opt-out switch), so two observable behaviors change for existing consumers without any code change:
[XmlText]array/list members now emit space-separated content (val1 val2) instead of concatenated content (val1val2). Any consumer that today relies on the (admittedly broken) concatenation, or that parses the produced XML with external tooling, will see different output.[XmlAttribute]list deserialization now splits on only the four XML whitespace characters instead ofchar.IsWhiteSpace(). Previously an item separated by e.g. NBSP would be split; now it is preserved. This is a read-side semantic change for attribute lists even though attribute serialization is unchanged.
Both are defensible and gated by the switch, but the polarity (opt-out, new behavior by default) and the fact that #2 changes attribute list reading — not just the [XmlText] bug being fixed — are decisions that warrant explicit maintainer confirmation and a breaking-change doc. The UseLegacyXmlListSeparationSwitch_DefaultsToFalse test correctly pins the polarity, which is good.
⚠️ Round-trip lossiness for empty/null items is asymmetric by design
The writer emits a positional separator even when an item serializes to empty (null/empty string), producing a double space, while the reader normalizes runs and uses RemoveEmptyEntries, so empty/null middle items are silently dropped. This is clearly documented in comments and covered by tests (...NullOrEmptyItemNormalizesOnDeserialize), and matches the XSD list model, so it is acceptable — but it is a genuine data-loss edge for callers who put empty entries in a text list. Worth a human confirming this is the intended contract (it mirrors attribute-list behavior, which supports the decision).
✅ Cross-engine consistency verified
I checked that the flag and the split/join logic are applied consistently in ReflectionXmlSerializationReader/Writer, XmlSerializationReader(ILGen)/Writer(ILGen), and that the regenerated Expected.SerializableAssembly.XmlSerializers.cs matches the IL-gen/source-gen output (space separator on write, split on the four XML whitespace chars with RemoveEmptyEntries on read). The text.IsList gate (Mapping.IsList && XmlElements.Count == 0 && XmlAnyElements.Count == 0 && !switch) and the writer's elements.Length == 0 guard are mutually consistent, so mixed element/text content correctly retains the legacy per-run behavior (validated by XmlTextArray_WithMixedElementContent_PreservesTextRuns).
✅ TextAccessor.IsList scoping is sound
The flag is only set in XmlReflectionImporter (attribute-driven mapping). The schema path (XmlSchemaImporter) leaves it false, so schema-imported mixed-content string[] text keeps its existing run-based behavior. That is the correct conservative choice and avoids changing xsd-driven scenarios.
💡 Minor: runtime allocation of the separator array in IL-gen path
The non-legacy IL-gen paths emit " \t\n\r".ToCharArray() inline, allocating a 4-char array on every deserialize of the member. This is negligible in practice and not worth blocking, but a static cached char[] (as the reflection reader already uses via s_xmlListWhitespace) would avoid the per-call allocation if a follow-up wants parity.
✅ Test quality
Tests are strong: exact serialized-output assertions plus round-trip checks, [Theory]/[MemberData]/[InlineData] for whitespace-normalization and null/empty edge cases, List<string> coverage (not just arrays), legacy-switch coverage for both [XmlText] (concatenation) and [XmlAttribute] (broad splitting), a default-polarity pin, and correct placement of new runtime-only types in SerializationTypes.RuntimeOnly.cs to exclude them from the pre-generated serializer projects.
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.9 AIC · ⌖ 11.2 AIC · ⊞ 10K
The ILGen-emitted deserializer split list values (for [XmlText] and [XmlAttribute] array-like members) by building the four-char XML whitespace separator array at runtime via " \t\n\r".ToCharArray(), allocating a fresh char[] on every deserialization. Cache the separators in a private static field (s_xmlListWhitespace) defined on the generated reader type, lazily initialized on first use, so the array is allocated at most once per reader type. The field lives on the generated subclass (not the public XmlSerializationReader base) to avoid adding public API surface. The lazy-init race is benign since the array is only ever read. The legacy (opt-out) path is unchanged behaviorally: it still passes a null separator to String.Split (broader char.IsWhiteSpace splitting) and now loads null directly at the call site with no local round-trip. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 93c39ba6-3968-432e-97ee-713c2252ed42
There was a problem hiding this comment.
Holistic Review
Motivation: Unchanged from the prior review. The base problem (issue #115837) is real and well-evidenced: an array-like [XmlText] member (e.g. [XmlText] string[]) concatenated its items with no separator on write, so the value could never round-trip. The single new commit in this update is a pure performance refinement of the already-accepted fix and does not alter the motivation.
Approach: The new commit (35301936) implements the exact optimization flagged as a minor follow-up in the previous holistic review: instead of emitting " \t\n\r".ToCharArray() inline (allocating a fresh 4-char array on every deserialize), the IL-gen reader now caches the four XML whitespace separators in a lazily-initialized private static field (s_xmlListWhitespace) on the generated reader, via the new EmitXmlListSeparators helper. Both the [XmlAttribute] list path (WriteAttribute) and the [XmlText] list path (WriteText) share the helper. For the legacy switch the helper returns null and the caller loads null directly, preserving the broad char.IsWhiteSpace() split. This brings the IL-gen path to parity with the reflection reader's existing s_xmlListWhitespace cache.
Summary: Ldsfld), branches on null, populates and stores the array once, and the lazy-init race is benign because the array is only ever read after publication. The escalation is unchanged and rooted in the cumulative PR, not this commit: this remains an intentional default-on behavioral/compat change (space-separated serialized output and attribute-list splitting semantics) that warrants maintainer sign-off on compat policy (breaking-change doc, opt-out polarity) and confirmation of the documented lossy round-trip for empty/null items. Those decisions belong to a human, and no code in this increment changes them.
Assessment History
- review 4730638129 reviewed commit
5e91788a, verdict⚠️ Needs Human Review. Current verdict:⚠️ Needs Human Review — unchanged. Motivation, approach, and risk assessment are all unchanged; the sole new commit (35301936) implements the "💡 Minor: runtime allocation of the separator array in IL-gen path" follow-up from that review by caching the separators in a static field, and introduces no new compat or correctness risk.
Detailed Findings
No new actionable findings. The incremental change (EmitXmlListSeparators and its two call sites in XmlSerializationReaderILGen.cs) is correct and addresses the prior review's minor allocation note. All cumulative compatibility observations from the prior review still stand but concern code unchanged in this increment, so they are not re-raised as new findings.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 104.5 AIC · ⌖ 10.4 AIC · ⊞ 10K
Replace the per-engine list-splitting logic (reflection, IL, and C# pre-gen) with a single protected static helper, XmlSerializationReader.SplitXmlListValues(string, bool), so all three serializer engines share one implementation and one cached separator array. This supersedes the prior per-generated-type static char[] caching in the ILGen reader: instead of emitting a lazily-initialized static field per generated serializer, the ILGen and C# codegen paths now emit a call to the already-compiled framework helper (mirroring the existing ToChar/ToEnum callbacks). This removes the per-call char[] allocation in every engine and sidesteps the safe-publication/race-condition concern raised on the lazy static field. The UseLegacyXmlListSeparation switch is now evaluated at read time inside the helper rather than baked in at serializer-generation time. This fixes a latent bug in pre-generated (sgen) serializers, which previously captured the build machine's switch value, leaving end users unable to opt into the legacy behavior. It also makes the ILGen path robust against serializer caching and test ordering. Adds the protected static SplitXmlListValues member to the System.Xml ref assembly and regenerates the Microsoft.XmlSerializer.Generator golden file to emit the helper call. The new protected API requires API-review sign-off. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 93c39ba6-3968-432e-97ee-713c2252ed42
… API The previous commit routed all three serializer engines through a new `protected static string[] XmlSerializationReader.SplitXmlListValues(string, bool)` helper. That added permanent public (protected) API surface to a long-standing public base class, which requires formal API review and is a lasting compatibility obligation. On reflection the shared helper is a bolt-on either way: - The apparent kinship with the existing `ToChar`/`ToEnum` protected-static family is superficial. Those are scalar lexical->value parsers with real conversion logic; list splitting is a trivial tokenizer that every engine emitted inline before this work. - XmlSerializer is by design three parallel engines (reflection, IL-emit, and C# pre-gen) that each duplicate serialization semantics. A single shared helper cuts against that grain rather than with it. - The one thing that genuinely must happen at runtime -- honoring the UseLegacyXmlListSeparation opt-out in the two compiled engines -- does NOT require the helper. The switch is read via the public `AppContext.TryGetSwitch(string, out bool)`, which both compiled engines can invoke directly. So this reverts the shared-helper design and instead has each engine own its separator set while still evaluating the switch at runtime (once per process), with zero new API: - Reflection reader: restores a private `static readonly char[] s_xmlListWhitespace` and inlines the `UseLegacyXmlListSeparation` check at both the attribute and text list sites. - IL-gen reader: emits a private `static char[] s_xmlListSeparators` field on the generated reader plus a static constructor that initializes it from `AppContext.TryGetSwitch`. The switch is therefore read when the generated type loads rather than baked in at generation time, so a caller's opt-out is honored -- fixing the latent pre-gen bug where the build machine's switch value was captured. The two list sites load the field and call `string.Split` directly. - C# pre-gen (codegen) reader: emits an equivalent per-class `static readonly char[] xmlListSeparators = AppContext.TryGetSwitch(...) ? ...` field, emitted only when a list member is generated, and both sites call `listValues.Split(xmlListSeparators[, RemoveEmptyEntries])`. - Removes the `SplitXmlListValues` entry from the XmlSerializer ref assembly and regenerates the golden pre-gen serializer to match the inline `.Split` output. Behavior is unchanged from the helper version and verified across all four engine x switch combinations (IL-gen and reflection, default and legacy): space-separated text by default with the four XML whitespace characters, NBSP preserved by default and split under legacy, double-space normalization, and legacy concatenation for [XmlText] arrays. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 93c39ba6-3968-432e-97ee-713c2252ed42
Fixes #115837
An array-like member marked with [XmlText] (e.g.
[XmlText] string[]) did not round-trip: on serialization the items were concatenated into the element's text content with no separator, so deserialization read them back as a single value.Per the XML spec (https://www.w3.org/TR/xml/#sec-common-syn) whitespace is one of only four characters (0x20, 0x09, 0x0D, 0x0A), and per the XSD spec a list type is a whitespace-separated sequence of tokens. This change makes [XmlText] on array-like members serialize as a space-separated list and split on the four XML whitespace characters when reading, mirroring the long-standing [XmlAttribute] list behavior. [XmlAttribute] list deserialization is also aligned to split on exactly those four XML whitespace characters (rather than the broader char.IsWhiteSpace set) so that other whitespace can survive inside list items.
The new behavior is applied across all serialization engines: the reflection-based reader/writer, the IL-generated reader/writer, and the Microsoft.XmlSerializer.Generator pre-generated output.
Because the previous concatenation behavior was broken for round-tripping, the new behavior is on by default. Callers depending on the old behavior can opt out via the AppContext switch
Switch.System.Xml.Serialization.UseLegacyXmlListSeparation.Notes:
Tests:
Fixes #115837