66163 fatal error xml serializer immutable array (#73729)

* Fiddle the RefEmit IL for value-type arrays like ImmutableArray. Add tests for expected and non-fatal read-only failures.

* Extend fixup to cover other 'Immutable' collection types.

* Skip ROC and Immutable tests in pregenerated test suite. Those types aren't in the pregen dll.

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

@@ -303,6 +303,11 @@ internal void EndFor()
                           CodeGenerator.InstanceBindingFlags,
                           Type.EmptyTypes
                           )!;
+                    // ICollection is not a value type, and ICollection::get_Count is a virtual method. So Call() here
+                    // will do a 'callvirt'. If we are working with a value type, box it before calling.
+                    Debug.Assert(ICollection_get_Count.IsVirtual && !ICollection_get_Count.DeclaringType!.IsValueType);
+                    if (varType.IsValueType)
+                        Box(varType);
                     Call(ICollection_get_Count);
                 }
                 Blt(forState.BeginLabel);

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

@@ -103,7 +103,7 @@ private void InternalLoad(Type? elementType, bool asAddress = false)
                 }
                 else
                 {
-                    ILG.Load(varA);
+                    ILG.LoadAddress(varA);
                     ILG.Load(varIA);
                     MethodInfo get_Item = varType.GetMethod(
                         "get_Item",

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

@@ -2176,7 +2176,7 @@ private void WriteMemberBegin(Member[] members)
                         }
                         else
                         {
-                            if (member.IsList && !member.Mapping.ReadOnly && member.Mapping.TypeDesc.IsNullable)
+                            if (member.IsList && !member.Mapping.ReadOnly) //&& member.Mapping.TypeDesc.IsNullable) // nullable or not, we are likely to assign null in the next step if we don't do this initialization. So just do this.
                             {
                                 // we need to new the Collections and ArrayLists
                                 ILGenLoad(member.Source, typeof(object));
@@ -2881,7 +2881,8 @@ private void WriteArray(string source, string? arrayName, ArrayMapping arrayMapp
                 )!;
             ilg.Ldarg(0);
             ilg.Call(XmlSerializationReader_ReadNull);
-            ilg.IfNot();
+            ilg.IfNot();    // if (!ReadNull()) { // EnterScope
+            ilg.EnterScope();
 
             MemberMapping memberMapping = new MemberMapping();
             memberMapping.Elements = arrayMapping.Elements;
@@ -2976,11 +2977,14 @@ private void WriteArray(string source, string? arrayName, ArrayMapping arrayMapp
 
             if (isNullable)
             {
-                ilg.Else();
+                ilg.ExitScope();    // if(!ReadNull()) { ExitScope
+                ilg.Else();         // } else { EnterScope
+                ilg.EnterScope();
                 member.IsNullable = true;
                 WriteMemberBegin(members);
                 WriteMemberEnd(members);
             }
+            ilg.ExitScope();    // if(!ReadNull())/else ExitScope
             ilg.EndIf();
         }
 

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

@@ -3,7 +3,10 @@
 
 using SerializationTypes;
 using System;
+using System.Collections;
 using System.Collections.Generic;
+using System.Collections.Immutable;
+using System.Collections.ObjectModel;
 using System.Globalization;
 using System.IO;
 using System.Linq;
@@ -194,6 +197,103 @@ public static void Xml_ListRoot()
         Assert.Equal((string)x[1], (string)y[1]);
     }
 
+// ROC and Immutable types are not types from 'SerializableAssembly.dll', so they were not included in the
+// pregenerated serializers for the sgen tests. We could wrap them in a type that does exist there...
+// but I think the RO/Immutable story is wonky enough and RefEmit vs Reflection is near enough on the
+// horizon that it's not worth the trouble.
+#if !XMLSERIALIZERGENERATORTESTS
+    [Fact]
+    public static void Xml_ReadOnlyCollection()
+    {
+        ReadOnlyCollection<string> roc = new ReadOnlyCollection<string>(new string[] { "one", "two" });
+
+#if ReflectionOnly
+        // Expect exception when _using_ the serializer
+        var serializer = new XmlSerializer(typeof(ReadOnlyCollection<string>));
+        var ex = Assert.Throws<InvalidOperationException>(() => Serialize(roc, null, () => serializer));
+        Assert.Equal("There was an error generating the XML document.", ex.Message);
+        Assert.NotNull(ex.InnerException);
+        Assert.IsType<InvalidOperationException>(ex.InnerException);
+        Assert.StartsWith("To be XML serializable, types which inherit from ICollection must have an implementation of Add(System.String) at all levels of their inheritance hierarchy.", ex.InnerException.Message);
+#else
+        // Expect exception when _creating_ the serializer
+        var ex = Assert.Throws<InvalidOperationException>(() => new XmlSerializer(typeof(ReadOnlyCollection<string>)));
+        Assert.StartsWith("To be XML serializable, types which inherit from ICollection must have an implementation of Add(System.String) at all levels of their inheritance hierarchy.", ex.Message);
+#endif
+    }
+
+    [Theory]
+    [MemberData(nameof(Xml_ImmutableCollections_MemberData))]
+    public static void Xml_ImmutableCollections(Type type, object collection, Type createException, Type addException, string expectedXml, string exMsg = null)
+    {
+        XmlSerializer serializer;
+
+        // Some collections implement the required enumerator/Add combo (ImmutableList, ImmutableArray) and some don't (ImmutableStack,
+        // ImmutableQueue). If they do not, they will throw upon serializer construction in RefEmit mode. They should throw when
+        // first using the serializer in Reflection mode.
+#if ReflectionOnly
+        serializer = new XmlSerializer(type);
+        if (createException != null)
+        {
+            var ex = Assert.Throws(createException, () => Serialize(collection, expectedXml, () => serializer));
+            if (exMsg != null)
+                Assert.Contains(exMsg, $"{ex.Message} : {ex.InnerException?.Message}");
+            return;
+        }
+#else
+        if (createException != null)
+        {
+            var ex = Assert.Throws(createException, () => serializer = new XmlSerializer(type));
+            if (exMsg != null)
+                Assert.Contains(exMsg, $"{ex.Message} : {ex.InnerException?.Message}");
+            return;
+        }
+        serializer = new XmlSerializer(type);
+#endif
+
+        // If they do meet the signature requirement, they may succeed or fail depending on whether their Add/Indexer explicitly throw
+        // or not. (ImmutableArray throws. ImmutableList does not - it returns a new copy instead... which gets ignored and is thus
+        // essentially a silent failure.) Serializing out to a string first should work though.
+        string serializedValue = Serialize(collection, expectedXml, () => serializer);
+
+        if (addException != null)
+        {
+            var ex = Assert.Throws(addException, () => Deserialize(serializer, serializedValue));
+            if (exMsg != null)
+                Assert.Contains(exMsg, $"{ex.Message} : {ex.InnerException?.Message}");
+            return;
+        }
+
+        // In this case, we can execute everything without exception. But since our calls to '.Add()' do nothing, we end up
+        // with an empty collection
+        var rttCollection = Deserialize(serializer, serializedValue);
+        Assert.NotNull(rttCollection);
+        Assert.Empty((IEnumerable)rttCollection);
+    }
+    public static IEnumerable<object[]> Xml_ImmutableCollections_MemberData()
+    {
+        string arrayOfInt = "<?xml version=\"1.0\" encoding=\"utf-8\"?><ArrayOfInt xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xmlns:xsd=\"http://www.w3.org/2001/XMLSchema\"><int>42</int></ArrayOfInt>";
+        string arrayOfAny = "<?xml version=\"1.0\" encoding=\"utf-8\"?><ArrayOfAnyType xmlns:xsi=\"http://www.w3.org/2001/XMLSchema-instance\" xmlns:xsd=\"http://www.w3.org/2001/XMLSchema\"><anyType /></ArrayOfAnyType>";
+
+#if ReflectionOnly
+        yield return new object[] { typeof(ImmutableArray<int>), ImmutableArray.Create(42), null, typeof(InvalidOperationException), arrayOfInt, "Specified method is not supported." };
+        yield return new object[] { typeof(ImmutableArray<object>), ImmutableArray.Create(new object()), null, typeof(InvalidOperationException), arrayOfAny, "Specified method is not supported." };
+        yield return new object[] { typeof(ImmutableList<int>), ImmutableList.Create(42), null, typeof(InvalidOperationException), arrayOfInt, "Specified method is not supported." };
+        yield return new object[] { typeof(ImmutableStack<int>), ImmutableStack.Create(42), typeof(InvalidOperationException), null, arrayOfInt, "To be XML serializable, types which inherit from IEnumerable must have an implementation of Add" };
+        yield return new object[] { typeof(ImmutableQueue<int>), ImmutableQueue.Create(42), typeof(InvalidOperationException), null, arrayOfInt, "To be XML serializable, types which inherit from IEnumerable must have an implementation of Add" };
+        yield return new object[] { typeof(ImmutableDictionary<string, int>), new Dictionary<string, int>() { { "one", 1 } }.ToImmutableDictionary(), typeof(InvalidOperationException), null, null, "is not supported because it implements IDictionary." };
+#else
+        yield return new object[] { typeof(ImmutableArray<int>), ImmutableArray.Create(42), null, typeof(InvalidOperationException), arrayOfInt, "Parameterless constructor is required for collections and enumerators." };
+        yield return new object[] { typeof(ImmutableArray<object>), ImmutableArray.Create(new object()), null, typeof(InvalidOperationException), arrayOfAny, "Parameterless constructor is required for collections and enumerators." };
+        yield return new object[] { typeof(ImmutableList<int>), ImmutableList.Create(42), null, null, arrayOfInt };
+        yield return new object[] { typeof(ImmutableStack<int>), ImmutableStack.Create(42), typeof(InvalidOperationException), null, arrayOfInt, "To be XML serializable, types which inherit from IEnumerable must have an implementation of Add" };
+        yield return new object[] { typeof(ImmutableQueue<int>), ImmutableQueue.Create(42), typeof(InvalidOperationException), null, arrayOfInt, "To be XML serializable, types which inherit from IEnumerable must have an implementation of Add" };
+        // IDictionary types are denied right from the start with a NotSupportedExcpetion
+        yield return new object[] { typeof(ImmutableDictionary<string, int>), new Dictionary<string, int>() { { "one", 1 } }.ToImmutableDictionary(), typeof(NotSupportedException), null, null, "is not supported because it implements IDictionary." };
+#endif
+    }
+#endif  // !XMLSERIALIZERGENERATORTESTS
+
     [Fact]
     public static void Xml_EnumAsRoot()
     {
@@ -2204,4 +2304,55 @@ private static T SerializeAndDeserializeWithWrapper<T>(T value, XmlSerializer se
             Assert.True(e is ExceptionType, $"Assert.True failed for {typeof(T)}. Expected: {typeof(ExceptionType)}; Actual: {e.GetType()}");
         }
     }
+
+    private static string Serialize<T>(T value, string baseline, Func<XmlSerializer> serializerFactory = null,
+    bool skipStringCompare = false, XmlSerializerNamespaces xns = null)
+    {
+        XmlSerializer serializer = (serializerFactory != null) ? serializerFactory() : new XmlSerializer(typeof(T));
+
+        using (MemoryStream ms = new MemoryStream())
+        {
+            if (xns == null)
+            {
+                serializer.Serialize(ms, value);
+            }
+            else
+            {
+                serializer.Serialize(ms, value, xns);
+            }
+
+            ms.Position = 0;
+
+            string actualOutput = new StreamReader(ms).ReadToEnd();
+
+            if (!skipStringCompare)
+            {
+                Utils.CompareResult result = Utils.Compare(baseline, actualOutput);
+                Assert.True(result.Equal, string.Format("{1}{0}Test failed for input: {2}{0}Expected: {3}{0}Actual: {4}",
+                    Environment.NewLine, result.ErrorMessage, value, baseline, actualOutput));
+            }
+
+            return actualOutput;
+        }
+    }
+
+    private static object? Deserialize(XmlSerializer serializer, string xmlInput)
+    {
+        using (Stream stream = StringToStream(xmlInput))
+        {
+            return serializer.Deserialize(stream);
+        }
+    }
+
+    private static Stream StringToStream(string input)
+    {
+        MemoryStream ms = new MemoryStream();
+        StreamWriter sw = new StreamWriter(ms);
+
+        sw.Write(input);
+        sw.Flush();
+        ms.Position = 0;
+
+        return ms;
+    }
 }