From c0cb1b31c386d70cf07671cd4d2763fda2e20b83 Mon Sep 17 00:00:00 2001 From: Steve Molloy Date: Wed, 10 Aug 2022 11:17:53 -0700 Subject: [PATCH 1/3] Fiddle the RefEmit IL for value-type arrays like ImmutableArray. Add tests for expected and non-fatal read-only failures. --- .../System/Xml/Serialization/CodeGenerator.cs | 5 + .../System/Xml/Serialization/SourceInfo.cs | 2 +- .../XmlSerializationReaderILGen.cs | 2 +- .../tests/XmlSerializer/XmlSerializerTests.cs | 99 +++++++++++++++++++ 4 files changed, 106 insertions(+), 2 deletions(-) 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..06cfc508e3e4ea 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)); diff --git a/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs b/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs index 760eb4d2873cc3..e3d5a2121650b0 100644 --- a/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs +++ b/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs @@ -4,6 +4,8 @@ using SerializationTypes; using System; using System.Collections.Generic; +using System.Collections.Immutable; +using System.Collections.ObjectModel; using System.Globalization; using System.IO; using System.Linq; @@ -194,6 +196,52 @@ public static void Xml_ListRoot() Assert.Equal((string)x[1], (string)y[1]); } + [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 + } + + [Fact] + public static void Xml_ImmutableArray() + { + // ImmutableArray does implement Add(T)... it just throws unconditionally. But it means we will be allowed to create a serializer + // and even use it to write out. It will fail on deserialization though, with a less-than-helpful exception message. + var arr = ImmutableArray.Create(42); + string expectedXml = "42"; + + var serializer = new XmlSerializer(typeof(ImmutableArray)); + + string serializedValue = Serialize(arr, expectedXml, () => serializer); + + var ex = Assert.Throws(() => Deserialize>(serializer, serializedValue)); + Assert.StartsWith("There is an error in XML document", ex.Message); + Assert.NotNull(ex.InnerException); +#if ReflectionOnly + // In the reflection case, we see IList.Add()... we call IList.Add()... and IList.Add() throws this exception. + // Of note, if we called ImmutableArray.Add() or IImmutableList.Add(), there would be no exception - just a new array returned. + Assert.IsType(ex.InnerException); +#else + // In the RefEmit case, we stumble before the Add() step because we don't see a default constructor + Assert.IsType(ex.InnerException); + Assert.StartsWith("Could not deserialize global::System.Collections.Immutable.ImmutableArray. Parameterless constructor is required for collections and enumerators.", ex.InnerException.Message); +#endif + } + [Fact] public static void Xml_EnumAsRoot() { @@ -2198,4 +2246,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 T? Deserialize(XmlSerializer serializer, string xmlInput) + { + using (Stream stream = StringToStream(xmlInput)) + { + return (T?)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; + } } From 50736cf44763098151308801dafaa8dab06bb66c Mon Sep 17 00:00:00 2001 From: Steve Molloy Date: Wed, 10 Aug 2022 16:22:13 -0700 Subject: [PATCH 2/3] Extend fixup to cover other 'Immutable' collection types. --- .../XmlSerializationReaderILGen.cs | 8 +- .../tests/XmlSerializer/XmlSerializerTests.cs | 84 ++++++++++++++----- 2 files changed, 71 insertions(+), 21 deletions(-) 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 06cfc508e3e4ea..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 @@ -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 e3d5a2121650b0..b08246c538ef64 100644 --- a/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs +++ b/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs @@ -3,6 +3,7 @@ using SerializationTypes; using System; +using System.Collections; using System.Collections.Generic; using System.Collections.Immutable; using System.Collections.ObjectModel; @@ -216,29 +217,74 @@ public static void Xml_ReadOnlyCollection() #endif } - [Fact] - public static void Xml_ImmutableArray() + [Theory] + [MemberData(nameof(Xml_ImmutableCollections_MemberData))] + public static void Xml_ImmutableCollections(Type type, object collection, Type createException, Type addException, string expectedXml, string exMsg = null) { - // ImmutableArray does implement Add(T)... it just throws unconditionally. But it means we will be allowed to create a serializer - // and even use it to write out. It will fail on deserialization though, with a less-than-helpful exception message. - var arr = ImmutableArray.Create(42); - string expectedXml = "42"; + 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 - var serializer = new XmlSerializer(typeof(ImmutableArray)); + // 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); - string serializedValue = Serialize(arr, 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 = ""; - var ex = Assert.Throws(() => Deserialize>(serializer, serializedValue)); - Assert.StartsWith("There is an error in XML document", ex.Message); - Assert.NotNull(ex.InnerException); #if ReflectionOnly - // In the reflection case, we see IList.Add()... we call IList.Add()... and IList.Add() throws this exception. - // Of note, if we called ImmutableArray.Add() or IImmutableList.Add(), there would be no exception - just a new array returned. - Assert.IsType(ex.InnerException); + 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 - // In the RefEmit case, we stumble before the Add() step because we don't see a default constructor - Assert.IsType(ex.InnerException); - Assert.StartsWith("Could not deserialize global::System.Collections.Immutable.ImmutableArray. Parameterless constructor is required for collections and enumerators.", ex.InnerException.Message); + 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 } @@ -2278,11 +2324,11 @@ private static string Serialize(T value, string baseline, Func } } - private static T? Deserialize(XmlSerializer serializer, string xmlInput) + private static object? Deserialize(XmlSerializer serializer, string xmlInput) { using (Stream stream = StringToStream(xmlInput)) { - return (T?)serializer.Deserialize(stream); + return serializer.Deserialize(stream); } } From b83723a478870406f315c271c0e0afe07cf87bfa Mon Sep 17 00:00:00 2001 From: Steve Molloy Date: Wed, 10 Aug 2022 23:49:54 -0700 Subject: [PATCH 3/3] Skip ROC and Immutable tests in pregenerated test suite. Those types aren't in the pregen dll. --- .../tests/XmlSerializer/XmlSerializerTests.cs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs b/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs index b08246c538ef64..faf24cb89369bd 100644 --- a/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs +++ b/src/libraries/System.Private.Xml/tests/XmlSerializer/XmlSerializerTests.cs @@ -197,6 +197,11 @@ 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() { @@ -287,6 +292,7 @@ public static IEnumerable Xml_ImmutableCollections_MemberData() 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()