diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/CodeGenerator.cs b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/CodeGenerator.cs index 493a5ca37e1136..20e9d52fa5867d 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/CodeGenerator.cs +++ b/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); diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/SourceInfo.cs b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/SourceInfo.cs index 971b9184216f15..3a544fb9dcd954 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/SourceInfo.cs +++ b/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", diff --git a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs b/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs index 7b39193b4a72d2..f7d8514e55a2c6 100644 --- a/src/libraries/System.Private.Xml/src/System/Xml/Serialization/XmlSerializationReaderILGen.cs +++ b/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(); } diff --git a/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs b/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs index 760eb4d2873cc3..faf24cb89369bd 100644 --- a/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs +++ b/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 roc = new ReadOnlyCollection(new string[] { "one", "two" }); + +#if ReflectionOnly + // Expect exception when _using_ the serializer + var serializer = new XmlSerializer(typeof(ReadOnlyCollection)); + var ex = Assert.Throws(() => Serialize(roc, null, () => serializer)); + Assert.Equal("There was an error generating the XML document.", ex.Message); + Assert.NotNull(ex.InnerException); + Assert.IsType(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(() => new XmlSerializer(typeof(ReadOnlyCollection))); + 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 Xml_ImmutableCollections_MemberData() + { + string arrayOfInt = "42"; + string arrayOfAny = ""; + +#if ReflectionOnly + yield return new object[] { typeof(ImmutableArray), ImmutableArray.Create(42), null, typeof(InvalidOperationException), arrayOfInt, "Specified method is not supported." }; + yield return new object[] { typeof(ImmutableArray), ImmutableArray.Create(new object()), null, typeof(InvalidOperationException), arrayOfAny, "Specified method is not supported." }; + yield return new object[] { typeof(ImmutableList), ImmutableList.Create(42), null, typeof(InvalidOperationException), arrayOfInt, "Specified method is not supported." }; + yield return new object[] { typeof(ImmutableStack), 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), 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), new Dictionary() { { "one", 1 } }.ToImmutableDictionary(), typeof(InvalidOperationException), null, null, "is not supported because it implements IDictionary." }; +#else + yield return new object[] { typeof(ImmutableArray), ImmutableArray.Create(42), null, typeof(InvalidOperationException), arrayOfInt, "Parameterless constructor is required for collections and enumerators." }; + yield return new object[] { typeof(ImmutableArray), ImmutableArray.Create(new object()), null, typeof(InvalidOperationException), arrayOfAny, "Parameterless constructor is required for collections and enumerators." }; + yield return new object[] { typeof(ImmutableList), ImmutableList.Create(42), null, null, arrayOfInt }; + yield return new object[] { typeof(ImmutableStack), 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), 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), new Dictionary() { { "one", 1 } }.ToImmutableDictionary(), typeof(NotSupportedException), null, null, "is not supported because it implements IDictionary." }; +#endif + } +#endif // !XMLSERIALIZERGENERATORTESTS + [Fact] public static void Xml_EnumAsRoot() { @@ -2198,4 +2298,55 @@ private static T SerializeAndDeserializeWithWrapper(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 value, string baseline, Func 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; + } }