Implement support for derived IXmlSerializable types in XmlSerializer by StephenMolloy · Pull Request #129492 · dotnet/runtime

StephenMolloy · Jun 16, 2026

This pull request improves support for deserializing derived types that implement IXmlSerializable in the reflection-based XML serializer. It fixes a long-standing issue by enabling proper handling of xsi:type for derived serializable types, and moves the corresponding test to the main test suite. The most important changes are:

ReflectionXmlSerializationReader improvements:

Implements logic to walk the SerializableMapping derivation tree and correctly deserialize objects when an xsi:type refers to a derived IXmlSerializable type, instead of throwing a NotImplementedException or failing to handle the type. [1] [2]

Test suite updates:

Moves the Xml_DerivedIXmlSerializable test from XmlSerializerTests.Internal.cs (where it was skipped due to the missing feature) to the main XmlSerializerTests.cs file, verifying that derived IXmlSerializable types are now correctly serialized and deserialized. [1] [2]

Fixes: #1401

Copilot

Pull request overview

This pull request updates the reflection-based XmlSerializer reader to support deserializing derived IXmlSerializable types by resolving xsi:type against SerializableMapping.DerivedMappings, and enables the previously-skipped derived-IXmlSerializable test in the main test suite.

Changes:

Update ReflectionXmlSerializationReader to choose the correct derived SerializableMapping based on xsi:type (including walking the derived-mapping tree).
Add a helper (ReadDerivedSerializable) to perform the derived-mapping lookup + instantiation/deserialization.
Move/enable the Xml_DerivedIXmlSerializable test in XmlSerializerTests.cs.

Reviewed changes

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

File	Description
src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs	Adds derived `IXmlSerializable` dispatch by walking `SerializableMapping.DerivedMappings` based on `xsi:type`.
src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs	Enables a regression test validating derived `IXmlSerializable` serialization/deserialization scenarios.
src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.Internal.cs	Removes the derived `IXmlSerializable` test from the internal/skip-only suite now that the feature is implemented.

…rializable Mirror XmlSerializationReaderILGen in the reflection-based reader: when an unknown xsi:type is encountered for a serializable member with derived mappings, consume the node and perform choice/specified bookkeeping but skip the member value assignment so a pre-populated member is not overwritten with null. Adds a regression test that runs in both the ILGen and ReflectionOnly readers, and replaces a culture-dependent DateTime.Parse in the existing derived-IXmlSerializable test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…terference The XmlSerializableMemberWrapper type lived in the shared SerializationTypes.cs, which is compiled into SerializableAssembly. The pregen-serializer test (SgenCommandTest) runs SGen over that assembly and compares output length against a checked-in golden file, so the added type changed the generated length. Move the derived IXmlSerializable tests and the wrapper type into XmlSerializerTests.RuntimeOnly.cs, which the pregen test project does not compile, so the tests still run in both the ILGen and ReflectionOnly readers without affecting SerializableAssembly or the golden file. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

…mlSerializer

Copilot

Pull request overview

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

Copilot

Pull request overview

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

…mlSerializer

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

Re-add test that got lost during merge conflict resolution

Copilot

Pull request overview

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

github-actions · Jul 19, 2026

Workflow state for the Holistic Review Orchestrator.

{
  "version": 5,
  "last_dispatched_commit": "25d4774d0ceb95ea08e34f7ed3330c5a44672c27",
  "last_dispatched_base_ref": "main",
  "last_dispatched_base_sha": "a991d279bd4e8f0aadf0a685f60d6bcafaebaca3",
  "last_reviewed_commit": "25d4774d0ceb95ea08e34f7ed3330c5a44672c27",
  "last_reviewed_base_ref": "main",
  "last_reviewed_base_sha": "a991d279bd4e8f0aadf0a685f60d6bcafaebaca3",
  "last_recorded_worker_run_id": "29684004083",
  "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": "25d4774d0ceb95ea08e34f7ed3330c5a44672c27",
      "review_id": 4730639078
    }
  ]
}

github-actions

Holistic Review

Motivation: Justified. The reflection-based XmlSerializer reader previously threw NotImplementedException (or silently clobbered the member with null) when an xsi:type referred to a derived IXmlSerializable type, closing the long-standing gap tracked by #1401. This is real, observable behavior that diverged from the RefEmit (ILGen) reader.

Approach: Sound. The new ReadDerivedSerializable walks the SerializableMapping.DerivedMappings tree and mirrors XmlSerializationReaderILGen.WriteDerivedSerializable closely — same traversal order (each derived mapping, then its children, then the next sibling), the same CreateMissingIXmlSerializableType/CreateBadDerivationException error semantics, and the same "emit UnknownNode(null) and leave the member untouched" behavior on an unrecognized xsi:type. It also corrects a pre-existing bug where the base-type match used defaultNamespace instead of sm.XsiType.Namespace, aligning with the ILGen WriteQNameEqual("tser", sm.XsiType.Name, sm.XsiType.Namespace).

Summary: ✅ LGTM. The reflection reader now matches the RefEmit reference implementation for derived IXmlSerializable types, the namespace-comparison fix is correct, and the choice/specified bookkeeping on the unknown-type path faithfully reproduces the unconditional checkSpecified/choice sets that WriteElement performs in ILGen. Tests exercise both the success and no-clobber regression paths, and because they live in XmlSerializerTests.RuntimeOnly.cs they run under both the RefEmit and Reflection (Mode=1) serializers, giving genuine coverage of the new code.

Detailed Findings

✅ Correctness — Faithful mirror of the ILGen reader

The recursion in ReadDerivedSerializable reproduces WriteDerivedSerializable: it matches on tser == null || QNameEqual(tser, derived.XsiType.Name, derived.XsiType.Namespace), throws CreateMissingIXmlSerializableType when derived.Type == null, throws CreateBadDerivationException when the derived type is not assignable to the head type, and otherwise reads it. Traversal order (self, then children, then next sibling) matches the ILGen else-if chain generation, so overload resolution across a multi-level derivation tree is preserved.

✅ Correctness — Unknown-type path does not clobber the member

On the no-match branch the code calls UnknownNode(null) and invokes member?.ChoiceSource / member?.CheckSpecifiedSource before returning without calling member.Source, so the existing member value is preserved. This reproduces ILGen, where WriteElement sets checkSpecified unconditionally at the top and the choice source at the bottom, while the unmatched xsi:type only emits Reader.UnknownNode(null). The Xml_DerivedIXmlSerializable_UnknownXsiTypeDoesNotClobberMember test verifies the preserved value.

✅ Test quality — Coverage in both serializer modes

Moving Xml_DerivedIXmlSerializable out of XmlSerializerTests.Internal.cs (RefEmit-only) into XmlSerializerTests.RuntimeOnly.cs means it now also compiles into the ReflectionOnly test project, which sets XmlSerializer Mode=1, so the derived-type path is validated against the new reflection code as well as the RefEmit baseline. The added no-clobber test uses a wrapper whose constructor pre-populates the member, then string-swaps a valid xsi:type to an unknown one — a clean, targeted regression for the previous null-clobbering behavior.

💡 Observation — Value-type `IXmlSerializable`

Both the base and derived reads go through (IXmlSerializable)ReflectionCreateObject(...), which boxes value-type IXmlSerializable implementers. The ILGen reader special-cases IsValueType with an explicit ConvertValue, but the reflection reader’s object-based flow already handles this via the cast and member.Source assignment, so no change is needed; noting only for completeness.

Note

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

Generated by Holistic Review · 191.3 AIC · ⌖ 10.9 AIC · ⊞ 10K

…mlSerializer

StephenMolloy · Jul 27, 2026

@mangod9 , this one needed to resolve a merge conflict in our test project. (The result is an empty class, I know. It's test code we can clean up later. I didn't want to figure out how to delete an entire file and modify the test project file in the web merge editor.) Resolving the conflict requires another approval. :/

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

StephenMolloy · Jul 27, 2026

@mangod9 , this one needed to resolve a merge conflict in our test project. (The result is an empty class, I know. It's test code we can clean up later. I didn't want to figure out how to delete an entire file and modify the test project file in the web merge editor.) Resolving the conflict requires another approval. :/

Actually... hang on. There might be a hidden merge conflict going on here as well with choice identifiers.

Update: resolved

Copilot

Pull request overview

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

mconnew

Only comment I had was really one of style, not correctness.

Implement support for derived IXmlSerializable types in XmlSerializer

589f49c

StephenMolloy added this to the 11.0.0 milestone Jun 16, 2026

StephenMolloy requested a review from mangod9 June 16, 2026 23:46

StephenMolloy self-assigned this Jun 16, 2026

Copilot AI review requested due to automatic review settings June 16, 2026 23:46

StephenMolloy added the area-Serialization label Jun 16, 2026

Copilot started reviewing on behalf of StephenMolloy June 16, 2026 23:46 View session

Copilot AI reviewed Jun 16, 2026

View reviewed changes

Comment thread ...ibraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs

Comment thread src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs Outdated

build-analysis Bot mentioned this pull request Jun 17, 2026

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

StephenMolloy and others added 2 commits June 29, 2026 18:26

Copilot AI review requested due to automatic review settings June 30, 2026 16:47

Copilot started reviewing on behalf of StephenMolloy June 30, 2026 16:47 View session

Merge branch 'main' into 1401-Handle-Derived-Mappings-in-Reflection-X…

2e2559b

…mlSerializer

Copilot AI reviewed Jun 30, 2026

View reviewed changes

Copilot AI review requested due to automatic review settings June 30, 2026 16:53

Copilot started reviewing on behalf of StephenMolloy June 30, 2026 16:54 View session

Copilot AI reviewed Jun 30, 2026

View reviewed changes

Merge branch 'main' into 1401-Handle-Derived-Mappings-in-Reflection-X…

dad750a

…mlSerializer

Copilot AI review requested due to automatic review settings July 9, 2026 19:00

Copilot started reviewing on behalf of StephenMolloy July 9, 2026 19:00 View session

Copilot AI reviewed Jul 9, 2026

View reviewed changes

Comment thread src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.RuntimeOnly.cs

Re-add test that got lost during merge conflict resolution

25d4774

Re-add test that got lost during merge conflict resolution

Copilot AI review requested due to automatic review settings July 15, 2026 01:11

Copilot started reviewing on behalf of StephenMolloy July 15, 2026 01:11 View session

Copilot AI reviewed Jul 15, 2026

View reviewed changes

github-actions Bot reviewed Jul 19, 2026

View reviewed changes

mangod9 reviewed Jul 24, 2026

View reviewed changes

Comment thread ...ibraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs

mangod9 approved these changes Jul 27, 2026

View reviewed changes

Merge branch 'main' into 1401-Handle-Derived-Mappings-in-Reflection-X…

123d97c

…mlSerializer

Copilot AI review requested due to automatic review settings July 27, 2026 21:23

Copilot started reviewing on behalf of StephenMolloy July 27, 2026 21:23 View session

Copilot AI reviewed Jul 27, 2026

View reviewed changes

Comment thread ...ibraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs Outdated

Merge conflict edits

6ac5270

Copilot AI review requested due to automatic review settings July 28, 2026 06:02

Copilot started reviewing on behalf of StephenMolloy July 28, 2026 06:02 View session

Copilot AI reviewed Jul 28, 2026

View reviewed changes

mconnew reviewed Jul 28, 2026

View reviewed changes

Comment thread ...ibraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs

mconnew approved these changes Jul 28, 2026

View reviewed changes

StephenMolloy merged commit ac3fc49 into dotnet:main Jul 28, 2026
80 of 84 checks passed

dotnet-maestro Bot mentioned this pull request Jul 29, 2026

[main] Source code updates from dotnet/runtime dotnet/dotnet#8022

Open

Search code, repositories, users, issues, pull requests...

Uh oh!

Conversation

StephenMolloy commented Jun 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

github-actions Bot commented Jul 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot left a comment

Choose a reason for hiding this comment

Holistic Review

Detailed Findings

✅ Correctness — Faithful mirror of the ILGen reader

✅ Correctness — Unknown-type path does not clobber the member

✅ Test quality — Coverage in both serializer modes

💡 Observation — Value-type IXmlSerializable

Uh oh!

Uh oh!

StephenMolloy commented Jul 27, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

StephenMolloy commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

mconnew left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

StephenMolloy commented Jun 16, 2026 •

edited

Loading

github-actions Bot commented Jul 19, 2026 •

edited

Loading

💡 Observation — Value-type `IXmlSerializable`

StephenMolloy commented Jul 27, 2026 •

edited

Loading