Reflection-based XmlSerializer - Deserialize empty collections and allow for sub-types in collection items. (#111723)

* Add tests to verify issue and fix.

* Check for type-assignability instead of equivallence. Also fix Choice logic.

* Add tests to verify issue and fix.

* Ensure collections are initialized to empty - even if they should be null according to the xml.

* More tests for bringing reflection-based serializer to quirky parity with ILGen.

* Fix Xml/Type mapping to use proper member info with hidden/inherited type members; Fix List-handling in reflection serializer.

* Cleanup tests and comments based on PR feedback.

* While considering initializing empty lists at object creation instead of post-deserialization - noticed a missed empty-list case.

Author: StephenMolloy

src/libraries/Microsoft.XmlSerializer.Generator/tests/Expected.SerializableAssembly.XmlSerializers.cs

@@ -981,6 +981,31 @@ internal MemberMapping Clone()
         {
             return new MemberMapping(this);
         }
+
+        internal bool Hides(MemberMapping mapping)
+        {
+            // Semantics. But all mappings would hide a null mapping, so indicating that this particular instance of mapping
+            // specifically hides a null mapping is not necessary and could be misleading. So just return false.
+            if (mapping == null)
+                return false;
+
+            var baseMember = mapping.MemberInfo;
+            var derivedMember = this.MemberInfo;
+
+            // Similarly, if either mapping (or both) lacks MemberInfo, then its not appropriate to say that one hides the other.
+            if (baseMember == null || derivedMember == null)
+                return false;
+
+            // If the member names are different, they don't hide each other
+            if (baseMember.Name != derivedMember.Name)
+                return false;
+
+            // Names are the same. So, regardless of field or property or accessibility or return type, if derivedDeclaringType is a
+            // subclass of baseDeclaringType, then the base member is hidden.
+            Type? baseDeclaringType = baseMember.DeclaringType;
+            Type? derivedDeclaringType = derivedMember.DeclaringType;
+            return baseDeclaringType != null && derivedDeclaringType != null && baseDeclaringType.IsAssignableFrom(derivedDeclaringType);
+        }
     }
 
     internal sealed class MembersMapping : TypeMapping

src/libraries/System.Private.Xml/src/System/Xml/Serialization/Mappings.cs

@@ -1633,6 +1633,11 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
                     {
                         if (member.Source == null && mapping.TypeDesc.IsArrayLike && !(mapping.Elements!.Length == 1 && mapping.Elements[0].Mapping is ArrayMapping))
                         {
+                            // Always create a collection for (non-array) collection-like types, even if the XML data says the collection should be null.
+                            if (!mapping.TypeDesc.IsArray)
+                            {
+                                member.Collection ??= new CollectionMember();
+                            }
                             member.Source = (item) =>
                             {
                                 member.Collection ??= new CollectionMember();
@@ -1645,26 +1650,51 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
 
                     if (member.Source == null)
                     {
+                        var isList = mapping.TypeDesc.IsArrayLike && !mapping.TypeDesc.IsArray;
                         var pi = member.Mapping.MemberInfo as PropertyInfo;
-                        if (pi != null && typeof(IList).IsAssignableFrom(pi.PropertyType)
-                            && (pi.SetMethod == null || !pi.SetMethod.IsPublic))
+
+                        // Here we have to deal with some special cases for property members. The old serializers would trip over
+                        // private property setters generally - except in the case of list-like properties. Because lists get special
+                        // treatment, a private setter for a list property would only be a problem if the default constructor didn't
+                        // already create a list instance for the property. If it does create a list, then the serializer can still
+                        // populate it. Try to emulate the old serializer behavior here.
+
+                        // First, for non-list properties, private setters are always a problem.
+                        if (!isList && pi != null && pi.SetMethod != null && !pi.SetMethod.IsPublic)
                         {
-                            member.Source = (value) =>
+                            member.Source = (value) => throw new InvalidOperationException(SR.Format(SR.XmlReadOnlyPropertyError, pi.Name, pi.DeclaringType!.FullName));
+                        }
+
+                        // Next, for list properties, we need to handle not only the private setter case, but also the case where
+                        // there is no setter at all. Because we need to give the default constructor a chance to create the list
+                        // first before we make noise about not being able to set a list property.
+                        else if (isList && pi != null && (pi.SetMethod == null || !pi.SetMethod.IsPublic))
+                        {
+                            var addMethod = mapping.TypeDesc.Type!.GetMethod("Add");
+
+                            if (addMethod != null)
                             {
-                                var getOnlyList = (IList)pi.GetValue(o)!;
-                                if (value is IList valueList)
+                                member.Source = (value) =>
                                 {
-                                    foreach (var v in valueList)
+                                    var getOnlyList = pi.GetValue(o)!;
+                                    if (getOnlyList == null)
                                     {
-                                        getOnlyList.Add(v);
+                                        // No-setter lists should just be ignored if they weren't created by constructor. Private-setter lists are the noisy exception.
+                                        if (pi.SetMethod != null && !pi.SetMethod.IsPublic)
+                                            throw new InvalidOperationException(SR.Format(SR.XmlReadOnlyPropertyError, pi.Name, pi.DeclaringType!.FullName));
                                     }
-                                }
-                                else
-                                {
-                                    getOnlyList.Add(value);
-                                }
-                            };
+                                    else if (value is IEnumerable valueList)
+                                    {
+                                        foreach (var v in valueList)
+                                        {
+                                            addMethod.Invoke(getOnlyList, new object[] { v });
+                                        }
+                                    }
+                                };
+                            }
                         }
+
+                        // For all other members (fields, public setter properties, etc), just carry on as normal
                         else
                         {
                             if (member.Mapping.Xmlns != null)
@@ -1680,6 +1710,21 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
                                 member.Source = (value) => setterDelegate(o, value);
                             }
                         }
+
+                        // Finally, special list handling again. ANY list that we can assign/populate should be initialized with
+                        // an empty list if it hasn't been initialized already. Even if the XML data says the list should be null.
+                        // This is an odd legacy behavior, but it's what the old serializers did.
+                        if (isList && member.Source != null)
+                        {
+                            member.EnsureCollection = (obj) =>
+                            {
+                                if (GetMemberValue(obj, mapping.MemberInfo!) == null)
+                                {
+                                    var empty = ReflectionCreateObject(mapping.TypeDesc.Type!);
+                                    member.Source(empty);
+                                }
+                            };
+                        }
                     }
 
                     if (member.Mapping.CheckSpecified == SpecifiedAccessor.ReadWrite)
@@ -1729,23 +1774,29 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
                 WriteAttributes(allMembers, anyAttribute, unknownNodeAction, ref o);
 
                 Reader.MoveToElement();
+
                 if (Reader.IsEmptyElement)
                 {
                     Reader.Skip();
-                    return o;
                 }
-
-                Reader.ReadStartElement();
-                bool IsSequenceAllMembers = IsSequence();
-                if (IsSequenceAllMembers)
+                else
                 {
-                    // https://github.com/dotnet/runtime/issues/1402:
-                    // Currently the reflection based method treat this kind of type as normal types.
-                    // But potentially we can do some optimization for types that have ordered properties.
-                }
+                    Reader.ReadStartElement();
+                    bool IsSequenceAllMembers = IsSequence();
+                    if (IsSequenceAllMembers)
+                    {
+                        // https://github.com/dotnet/runtime/issues/1402:
+                        // Currently the reflection based method treat this kind of type as normal types.
+                        // But potentially we can do some optimization for types that have ordered properties.
+                    }
 
-                WriteMembers(allMembers, unknownNodeAction, unknownNodeAction, anyElementMember, anyTextMember);
+                    WriteMembers(allMembers, unknownNodeAction, unknownNodeAction, anyElementMember, anyTextMember);
 
+                    ReadEndElement();
+                }
+
+                // Empty element or not, we need to ensure all our array-like members have been initialized in the same
+                // way as the IL / CodeGen - based serializers.
                 foreach (Member member in allMembers)
                 {
                     if (member.Collection != null)
@@ -1757,9 +1808,10 @@ private static XmlSerializationCollectionFixupCallback GetCreateCollectionOfObje
                         var setMemberValue = GetSetMemberValueDelegate(o, memberInfo.Name);
                         setMemberValue(o, collection);
                     }
+
+                    member.EnsureCollection?.Invoke(o!);
                 }
 
-                ReadEndElement();
                 return o;
             }
         }
@@ -2031,6 +2083,7 @@ internal sealed class Member
             public Action<object?>? CheckSpecifiedSource;
             public Action<object>? ChoiceSource;
             public Action<string, string>? XmlnsSource;
+            public Action<object>? EnsureCollection;
 
             public Member(MemberMapping mapping)
             {

src/libraries/System.Private.Xml/src/System/Xml/Serialization/ReflectionXmlSerializationReader.cs

@@ -5,6 +5,7 @@
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
+using System.Globalization;
 using System.Reflection;
 using System.Text;
 using System.Xml.Schema;
@@ -122,7 +123,7 @@ private void WriteMember(object? o, object? choiceSource, ElementAccessor[] elem
             }
             else
             {
-                WriteElements(o, elements, text, choice, writeAccessors, memberTypeDesc.IsNullable);
+                WriteElements(o, choiceSource, elements, text, choice, writeAccessors, memberTypeDesc.IsNullable);
             }
         }
 
@@ -146,10 +147,10 @@ private void WriteArray(object o, object? choiceSource, ElementAccessor[] elemen
                 }
             }
 
-            WriteArrayItems(elements, text, choice, o);
+            WriteArrayItems(elements, text, choice, o, choiceSource);
         }
 
-        private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, object o)
+        private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, object o, object? choiceSources)
         {
             var arr = o as IList;
 
@@ -158,7 +159,8 @@ private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, Cho
                 for (int i = 0; i < arr.Count; i++)
                 {
                     object? ai = arr[i];
-                    WriteElements(ai, elements, text, choice, true, true);
+                    var choiceSource = ((Array?)choiceSources)?.GetValue(i);
+                    WriteElements(ai, choiceSource, elements, text, choice, true, true);
                 }
             }
             else
@@ -169,16 +171,18 @@ private void WriteArrayItems(ElementAccessor[] elements, TextAccessor? text, Cho
                 IEnumerator e = a.GetEnumerator();
                 if (e != null)
                 {
+                    int c = 0;
                     while (e.MoveNext())
                     {
                         object ai = e.Current;
-                        WriteElements(ai, elements, text, choice, true, true);
+                        var choiceSource = ((Array?)choiceSources)?.GetValue(c++);
+                        WriteElements(ai, choiceSource, elements, text, choice, true, true);
                     }
                 }
             }
         }
 
-        private void WriteElements(object? o, ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, bool writeAccessors, bool isNullable)
+        private void WriteElements(object? o, object? choiceSource, ElementAccessor[] elements, TextAccessor? text, ChoiceIdentifierAccessor? choice, bool writeAccessors, bool isNullable)
         {
             if (elements.Length == 0 && text == null)
                 return;
@@ -216,16 +220,35 @@ private void WriteElements(object? o, ElementAccessor[] elements, TextAccessor?
                     }
                     else if (choice != null)
                     {
-                        if (o != null && o.GetType() == element.Mapping!.TypeDesc!.Type)
+                        // This looks heavy - getting names of enums in string form for comparison rather than just comparing values.
+                        // But this faithfully mimics NetFx, and is necessary to prevent confusion between different enum types.
+                        // ie EnumType.ValueX could == 1, but TotallyDifferentEnumType.ValueY could also == 1.
+                        TypeDesc td = element.Mapping!.TypeDesc!;
+                        bool enumUseReflection = choice.Mapping!.TypeDesc!.UseReflection;
+                        string enumTypeName = choice.Mapping!.TypeDesc!.FullName;
+                        string enumFullName = (enumUseReflection ? "" : enumTypeName + ".@") + FindChoiceEnumValue(element, (EnumMapping)choice.Mapping, enumUseReflection);
+                        string choiceFullName = (enumUseReflection ? "" : choiceSource!.GetType().FullName + ".@") + choiceSource!.ToString();
+
+                        if (choiceFullName == enumFullName)
                         {
-                            WriteElement(o, element, writeAccessors);
-                            return;
+                            // Object is either non-null, or it is allowed to be null
+                            if (o != null || (!isNullable || element.IsNullable))
+                            {
+                                // But if Object is non-null, it's got to match types
+                                if (o != null && !td.Type!.IsAssignableFrom(o!.GetType()))
+                                {
+                                    throw CreateMismatchChoiceException(td.FullName, choice.MemberName!, enumFullName);
+                                }
+
+                                WriteElement(o, element, writeAccessors);
+                                return;
+                            }
                         }
                     }
                     else
                     {
                         TypeDesc td = element.IsUnbounded ? element.Mapping!.TypeDesc!.CreateArrayTypeDesc() : element.Mapping!.TypeDesc!;
-                        if (o!.GetType() == td.Type)
+                        if (td.Type!.IsAssignableFrom(o!.GetType()))
                         {
                             WriteElement(o, element, writeAccessors);
                             return;
@@ -274,6 +297,58 @@ private void WriteElements(object? o, ElementAccessor[] elements, TextAccessor?
             }
         }
 
+        private static string FindChoiceEnumValue(ElementAccessor element, EnumMapping choiceMapping, bool useReflection)
+        {
+            string? enumValue = null;
+
+            for (int i = 0; i < choiceMapping.Constants!.Length; i++)
+            {
+                string xmlName = choiceMapping.Constants[i].XmlName;
+
+                if (element.Any && element.Name.Length == 0)
+                {
+                    if (xmlName == "##any:")
+                    {
+                        if (useReflection)
+                            enumValue = choiceMapping.Constants[i].Value.ToString(CultureInfo.InvariantCulture);
+                        else
+                            enumValue = choiceMapping.Constants[i].Name;
+                        break;
+                    }
+                    continue;
+                }
+                int colon = xmlName.LastIndexOf(':');
+                string? choiceNs = colon < 0 ? choiceMapping.Namespace : xmlName.Substring(0, colon);
+                string choiceName = colon < 0 ? xmlName : xmlName.Substring(colon + 1);
+
+                if (element.Name == choiceName)
+                {
+                    if ((element.Form == XmlSchemaForm.Unqualified && string.IsNullOrEmpty(choiceNs)) || element.Namespace == choiceNs)
+                    {
+                        if (useReflection)
+                            enumValue = choiceMapping.Constants[i].Value.ToString(CultureInfo.InvariantCulture);
+                        else
+                            enumValue = choiceMapping.Constants[i].Name;
+                        break;
+                    }
+                }
+            }
+
+            if (string.IsNullOrEmpty(enumValue))
+            {
+                if (element.Any && element.Name.Length == 0)
+                {
+                    // Type {0} is missing enumeration value '##any' for XmlAnyElementAttribute.
+                    throw new InvalidOperationException(SR.Format(SR.XmlChoiceMissingAnyValue, choiceMapping.TypeDesc!.FullName));
+                }
+                // Type {0} is missing value for '{1}'.
+                throw new InvalidOperationException(SR.Format(SR.XmlChoiceMissingValue, choiceMapping.TypeDesc!.FullName, element.Namespace + ":" + element.Name, element.Name, element.Namespace));
+            }
+            if (!useReflection)
+                CodeIdentifier.CheckValidIdentifier(enumValue);
+            return enumValue;
+        }
+
         private void WriteText(object o, TextAccessor text)
         {
             if (text.Mapping is PrimitiveMapping primitiveMapping)
@@ -369,7 +444,7 @@ private void WriteElement(object? o, ElementAccessor element, bool writeAccessor
                     if (o != null)
                     {
                         WriteStartElement(name, ns, false);
-                        WriteArrayItems(mapping.ElementsSortedByDerivation!, null, null, o);
+                        WriteArrayItems(mapping.ElementsSortedByDerivation!, null, null, o, null);
                         WriteEndElement();
                     }
                 }