address code review feedback:

- prefer NET over NETCOREAPP when writing #if defines
- add ConditionalTheory to make the SkipTestException actually work
- ObjectNullMultiple must not allow for 0 inputs
- preallocate the List<T> for TFMs other than .NET

Author: adamsitnik

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/ArrayRecord.cs

@@ -67,7 +67,7 @@ private protected ArrayRecord(ArrayInfo arrayInfo)
     [RequiresDynamicCode("The code for an array of the specified type might not be available.")]
     public Array GetArray(Type expectedArrayType, bool allowNulls = true)
     {
-#if NETCOREAPP
+#if NET
         ArgumentNullException.ThrowIfNull(expectedArrayType);
 #else
         if (expectedArrayType is null)
@@ -153,6 +153,7 @@ private protected ArrayRecord(ArrayInfo arrayInfo) : base(arrayInfo)
     ///   <see langword="true" /> to permit <see langword="null" /> values within the array;
     ///   otherwise, <see langword="false" />.
     /// </param>
+    /// <returns>An array filled with the data provided in the serialized records.</returns>
     public abstract T?[] GetArray(bool allowNulls = true);
 
 #pragma warning disable IL3051 // RequiresDynamicCode is not required in this particualar case

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/ArraySinglePrimitiveRecord.cs

@@ -63,39 +63,30 @@ internal static IReadOnlyList<T> DecodePrimitiveTypes(BinaryReader reader, int c
     {
         if (typeof(T) == typeof(byte) && reader.IsDataAvailable(count))
         {
-            T[] result = (T[])(object)reader.ReadBytes(count);
-            // This might be less than the number of bytes requested if the end of the stream is reached.
-            if (result.Length != count)
-            {
-                ThrowHelper.ThrowEndOfStreamException();
-            }
-            return result;
+            return (T[])(object)reader.ReadBytes(count);
         }
+        // the input is UTF8, so the check does not include sizeof(char)
         else if (typeof(T) == typeof(char) && reader.IsDataAvailable(count))
         {
-            T[] result = (T[])(object)reader.ReadChars(count);
-            if (result.Length != count)
-            {
-                ThrowHelper.ThrowEndOfStreamException();
-            }
-            return result;
+            return (T[])(object)reader.ReadChars(count);
         }
 
+        // For decimals, the input is provided as strings, so we can't compute the required size up-front.
+        bool canPreAllocate = typeof(T) != typeof(decimal) && reader.IsDataAvailable(count * Unsafe.SizeOf<T>());
         // Most of the tests use MemoryStream or FileStream and they both allow for executing the fast path.
         // To ensure the slow path is tested as well, the fast path is executed only for optimized builds.
-#if NET
-        if (typeof(T) != typeof(decimal) && typeof(T) != typeof(DateTime) && typeof(T) != typeof(TimeSpan) // not optimized
-            && reader.IsDataAvailable(count * Unsafe.SizeOf<T>()))
+#if NET && RELEASE
+        if (canPreAllocate)
         {
             return DecodePrimitiveTypesToArray(reader, count);
         }
 #endif
-        return DecodePrimitiveTypesToList(reader, count);
+        return DecodePrimitiveTypesToList(reader, count, canPreAllocate);
     }
 
-    private static List<T> DecodePrimitiveTypesToList(BinaryReader reader, int count)
+    private static List<T> DecodePrimitiveTypesToList(BinaryReader reader, int count, bool canPreAllocate)
     {
-        List<T> values = [];
+        List<T> values = new List<T>(canPreAllocate ? count : Math.Min(count, 4));
         for (int i = 0; i < count; i++)
         {
             if (typeof(T) == typeof(byte))
@@ -174,35 +165,37 @@ private static T[] DecodePrimitiveTypesToArray(BinaryReader reader, int count)
 
         if (!BitConverter.IsLittleEndian)
         {
-            if (typeof(T) == typeof(short))
+            if (typeof(T) == typeof(short) || typeof(T) == typeof(ushort))
             {
                 Span<short> span = MemoryMarshal.Cast<T, short>(result);
                 BinaryPrimitives.ReverseEndianness(span, span);
             }
-            else if (typeof(T) == typeof(ushort))
-            {
-                Span<ushort> span = MemoryMarshal.Cast<T, ushort>(result);
-                BinaryPrimitives.ReverseEndianness(span, span);
-            }
-            else if (typeof(T) == typeof(int) || typeof(T) == typeof(float))
+            else if (typeof(T) == typeof(int) || typeof(T) == typeof(uint) || typeof(T) == typeof(float))
             {
                 Span<int> span = MemoryMarshal.Cast<T, int>(result);
                 BinaryPrimitives.ReverseEndianness(span, span);
             }
-            else if (typeof(T) == typeof(uint))
-            {
-                Span<uint> span = MemoryMarshal.Cast<T, uint>(result);
-                BinaryPrimitives.ReverseEndianness(span, span);
-            }
-            else if (typeof(T) == typeof(long) || typeof(T) == typeof(double))
+            else if (typeof(T) == typeof(long) || typeof(T) == typeof(ulong) || typeof(T) == typeof(double)
+                  || typeof(T) == typeof(DateTime) || typeof(T) == typeof(TimeSpan))
             {
                 Span<long> span = MemoryMarshal.Cast<T, long>(result);
                 BinaryPrimitives.ReverseEndianness(span, span);
             }
-            else if (typeof(T) == typeof(ulong))
-            {
-                Span<ulong> span = MemoryMarshal.Cast<T, ulong>(result);
-                BinaryPrimitives.ReverseEndianness(span, span);
+        }
+
+        if (typeof(T) == typeof(DateTime) || typeof(T) == typeof(TimeSpan))
+        {
+            Span<long> longs = MemoryMarshal.Cast<T, long>(result);
+            for (int i = 0; i < longs.Length; i++)
+            {
+                if (typeof(T) == typeof(DateTime))
+                {
+                    result[i] = (T)(object)Utils.BinaryReaderExtensions.CreateDateTimeFromData(longs[i]);
+                }
+                else
+                {
+                    result[i] = (T)(object)new TimeSpan(longs[i]);
+                }
             }
         }
 

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/BinaryArrayRecord.cs

@@ -121,7 +121,10 @@ internal static ArrayRecord Decode(BinaryReader reader, RecordMap recordMap, Pay
 
         bool isRectangular = arrayType is BinaryArrayType.Rectangular or BinaryArrayType.RectangularOffset;
 
-        if (rank < 1 || rank > 32
+        // It is an arbitrary limit in the current CoreCLR type loader.
+        const int MaxSupportedArrayRank = 32;
+
+        if (rank < 1 || rank > MaxSupportedArrayRank
             || (rank != 1 && !isRectangular)
             || (rank == 1 && isRectangular))
         {

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/ClassInfo.cs

@@ -57,7 +57,7 @@ internal static ClassInfo Decode(BinaryReader reader)
             // however it's impossible to get such output with BinaryFormatter,
             // so we prohibit that on purpose.
             string memberName = reader.ReadString();
-#if NETCOREAPP
+#if NET
             if (memberNames.TryAdd(memberName, i))
             {
                 continue;

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/ObjectNullMultiple256Record.cs

@@ -2,6 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using System.IO;
+using System.Runtime.Serialization.BinaryFormat.Utils;
 
 namespace System.Runtime.Serialization.BinaryFormat;
 
@@ -20,5 +21,14 @@ internal sealed class ObjectNullMultiple256Record : NullsRecord
     internal override int NullCount { get; }
 
     internal static ObjectNullMultiple256Record Decode(BinaryReader reader)
-        => new(reader.ReadByte());
+    {
+        // The NRBF spec for 2.5.6 ObjectNullMultiple allows for 0, but we don't.
+        byte count = reader.ReadByte();
+        if (count == 0)
+        {
+            ThrowHelper.ThrowInvalidValue(count);
+        }
+
+        return new ObjectNullMultiple256Record(count);
+    }
 }

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/ObjectNullMultipleRecord.cs

@@ -23,7 +23,7 @@ internal sealed class ObjectNullMultipleRecord : NullsRecord
     internal static ObjectNullMultipleRecord Decode(BinaryReader reader)
     {
         // 2.5.5 ObjectNullMultiple
-        
+
         // NullCount (4 bytes): An INT32 value ... The value MUST be a positive integer.
         int count = reader.ReadInt32();
         if (count <= 0)

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/PayloadReader.cs

@@ -26,7 +26,7 @@ static class PayloadReader
     /// <returns><see langword="true" /> if it starts with NRBF payload header; otherwise, <see langword="false" />.</returns>
     public static bool StartsWithPayloadHeader(byte[] bytes)
     {
-#if NETCOREAPP
+#if NET
         ArgumentNullException.ThrowIfNull(bytes);
 #else
         if (bytes is null)
@@ -37,7 +37,7 @@ public static bool StartsWithPayloadHeader(byte[] bytes)
 
         return bytes.Length >= SerializedStreamHeaderRecord.Size
             && bytes[0] == (byte)RecordType.SerializedStreamHeader
-#if NETCOREAPP
+#if NET
             && BinaryPrimitives.ReadInt32LittleEndian(bytes.AsSpan(9)) == SerializedStreamHeaderRecord.MajorVersion
             && BinaryPrimitives.ReadInt32LittleEndian(bytes.AsSpan(13)) == SerializedStreamHeaderRecord.MinorVersion;
 #else
@@ -58,7 +58,7 @@ public static bool StartsWithPayloadHeader(byte[] bytes)
     /// <remarks><para>When this method returns, <paramref name="stream" /> will be restored to its original position.</para></remarks>
     public static bool StartsWithPayloadHeader(Stream stream)
     {
-#if NETCOREAPP
+#if NET
         ArgumentNullException.ThrowIfNull(stream);
 #else
         if (stream is null)
@@ -81,7 +81,7 @@ public static bool StartsWithPayloadHeader(Stream stream)
 
         try
         {
-#if NETCOREAPP
+#if NET
             stream.ReadExactly(buffer, 0, buffer.Length);
 #else
             int offset = 0;
@@ -134,7 +134,7 @@ public static SerializationRecord Read(Stream payload, PayloadOptions? options =
     /// <inheritdoc cref="Read(Stream, PayloadOptions?, bool)"/>
     public static SerializationRecord Read(Stream payload, out IReadOnlyDictionary<int, SerializationRecord> recordMap, PayloadOptions? options = default, bool leaveOpen = false)
     {
-#if NETCOREAPP
+#if NET
         ArgumentNullException.ThrowIfNull(payload);
 #else
         if (payload is null)

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/PrimitiveTypeRecord.cs

@@ -12,7 +12,7 @@ namespace System.Runtime.Serialization.BinaryFormat;
 /// <remarks>
 /// <para>
 /// The NRBF specification considers the following types to be primitive:
-/// <see cref="String"/>, <see cref="bool"/>, <see cref="byte"/>, <see cref="sbyte"/>
+/// <see cref="string"/>, <see cref="bool"/>, <see cref="byte"/>, <see cref="sbyte"/>
 /// <see cref="char"/>, <see cref="short"/>, <see cref="ushort"/>,
 /// <see cref="int"/>, <see cref="uint"/>, <see cref="long"/>, <see cref="ulong"/>,
 /// <see cref="float"/>, <see cref="double"/>, <see cref="decimal"/>,

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/RecordMap.cs

@@ -44,7 +44,7 @@ internal void Add(SerializationRecord record)
             }
             else
             {
-#if NETCOREAPP
+#if NET
                 if (_map.TryAdd(record.ObjectId, record))
                 {
                     return;
@@ -84,7 +84,7 @@ private CollisionResistantInt32Comparer() { }
 
         public int GetHashCode(int obj)
         {
-#if NETCOREAPP
+#if NET
             Span<int> integers = new(ref obj);
 #else
             Span<int> integers = stackalloc int[1] { obj };

src/libraries/System.Runtime.Serialization.BinaryFormat/src/System/Runtime/Serialization/BinaryFormat/RecordType.cs

@@ -27,9 +27,9 @@ enum RecordType : byte
     /// Class information that references another class record's metadata.
     /// </summary>
     ClassWithId = 1,
-    
+
     // SystemClassWithMembers and ClassWithMembers are not supported by design (require type loading)
-    
+
     /// <summary>
     /// A system class information with type info.
     /// </summary>