Implement [NullWrappedCollection] (#1044)

* initial test

* use manually implented equivalent to nail expected bytes

* implement (watch for CS0618) serializer core

* null-wrapped check should preceed other lookups

* tests and fixes for wrapped sub-items (turns out it didn't work correctly for the writer API)

* reset in WriteEmptyWrappedItem

* keep new interface internal for now

* release notes

* add map support for [ProtoWrappedCollection] (not not [ProtoWrappedValue])

* fix RepeatedSerializer<,> - dropped wrong interface by mistake

* fix broken default creation path in map path

* - more map tests and byte analysis
- implemented [NullWrappedValue] properly on maps
- detect usage in schema-gen
- include tests for nulls in the tests
- fix PEVerify dll naming issue in coll/map tests

* - only compile the model to disk for the zero case (avoid IO conflict)
- include more vanilla model tests

Author: mgravell

docs/nullwrappers.md

@@ -1,6 +1,6 @@
 # Support for `null` values and empty collections
 
-For many commaon scenarios, protobuf-net *just works* with null values; for example:
+For many common scenarios, protobuf-net *just works* with null values; for example:
 
 ``` c#
 [ProtoMember(5)]
@@ -22,7 +22,7 @@ needs to consider multiple things:
 - the later addition of "field presence" to `proto3`
 - the existence of `wrappers.proto`
 
-I'm not going to attempt to cover all of these things in detail! However, we can give a flavor. If we just consider `proto3`, and a
+I'm not going to attempt to cover all of these things in detail! However, we can give a flavour. If we just consider `proto3`, and a
 simple integer:
 
 ``` proto
@@ -163,7 +163,7 @@ class SomeMessage
 
 In the above, we can see that `[NullWrappedValue]` applies to the *values* in a collection; sometimes - much more rarely - we
 wish to apply the same logic to the *collection itself*, to distinguish *null* collections from *empty* collections. Protobuf has
-no way of expressing an empty collection, but we can artifically invent an additional message layer that *has* a collection
+no way of expressing an empty collection, but we can artificially invent an additional message layer that *has* a collection
 
 - for null collections, nothing is written
 - for empty collections, an empty message wrapper is written
@@ -181,7 +181,7 @@ class SomeMessage
 ```
 
 As before, a runtime fault is thrown if `[NullWrappedCollection]` is encountered on an unexpected type; `[NullWrappedCollection]` *also*
-supports the same `AsGroup` concept (although it is not expected to be used in many scenarios). Convenienty, `[NullWrappedCollection]`
+supports the same `AsGroup` concept (although it is not expected to be used in many scenarios). Conveniently, `[NullWrappedCollection]`
 can be combined with `[NullWrappedValue]` without difficulty, as they apply at different scopes.
 
 With these features, most common null scenarios can be conveniently and robustly handled.

docs/releasenotes.md

@@ -14,6 +14,7 @@ Packages are available on NuGet: [protobuf-net](https://www.nuget.org/packages/p
 
 ## unreleased
 
+- implement `[NullWrappedCollection]`, usage [as here](https://protobuf-net.github.io/protobuf-net/nullwrappers#null-collections) (#1044)
 - support `nint` (`IntPtr`) and `nuint` (`UIntPtr`) with layout per `long`/`ulong` (#1043; fixes #1042, fixes grpc 282)
 
 ## 3.2.12

src/protobuf-net.Core/Internal/KeyValuePairSerializer.cs

@@ -1,5 +1,6 @@
 using ProtoBuf.Meta;
 using ProtoBuf.Serializers;
+using System;
 using System.Collections.Generic;
 using System.Runtime.InteropServices;
 
@@ -45,12 +46,36 @@ public KeyValuePair<TKey, TValue> Read(ref ProtoReader.State state, KeyValuePair
                 }
             }
             if (TypeHelper<TKey>.IsReferenceType && TypeHelper<TKey>.ValueChecker.IsNull(key))
-                key = TypeModel.CreateInstance<TKey>(state.Context, _keySerializer);
+                key = CreateDefault<TKey>(state.Context, _keySerializer, _keyFeatures);
             if (TypeHelper<TValue>.IsReferenceType && TypeHelper<TValue>.ValueChecker.IsNull(value))
-                value = TypeModel.CreateInstance<TValue>(state.Context, _valueSerializer);
+                value = CreateDefault<TValue>(state.Context, _valueSerializer, _valueFeatures);
 
             return new KeyValuePair<TKey, TValue>(key, value);
         }
+        static T CreateDefault<T>(ISerializationContext context, ISerializer<T> serializer, SerializerFeatures features)
+        {
+            if (features.HasAny(SerializerFeatures.OptionWrappedValue))
+            {   // no field? treat as null
+                return default;
+            }
+            if (serializer is not null && serializer.Features.GetCategory() == SerializerFeatures.CategoryMessage)
+            {
+                // get the serializer to do the work, by reading an empty payload
+                // (zero bytes is a default object in protobuf)
+                // this is useful in case the type is using a non-trivial constructor
+                // or factory API
+                var state = ProtoReader.State.Create(default(ReadOnlyMemory<byte>), context?.Model, context?.UserState);
+                try
+                {
+                    return serializer.Read(ref state, default);
+                }
+                finally
+                {
+                    state.Dispose();
+                }
+            }
+            return TypeModel.CreateInstance<T>(context, serializer);
+        }
 
         public void Write(ref ProtoWriter.State state, KeyValuePair<TKey, TValue> value)
         {

src/protobuf-net.Core/Meta/TypeModel.cs

@@ -1407,6 +1407,7 @@ internal static T CreateInstance<T>(ISerializationContext context, ISerializer<T
                 serializer ??= TypeModel.TryGetSerializer<T>(context?.Model);
                 T obj = default;
                 if (serializer is IFactory<T> factory) obj = factory.Create(context);
+
                 // note we already know this is a ref-type
                 if (obj is null) obj = ActivatorCreate<T>();
                 return obj;

src/protobuf-net.Core/NullWrappedValueAttribute.cs

@@ -11,7 +11,7 @@ namespace ProtoBuf
     [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property, AllowMultiple = false)]
     public sealed class NullWrappedValueAttribute : Attribute
     {
-        /// <summary>Indicates that the collection message wrapper shold use group encoding; this is more
+        /// <summary>Indicates that the collection message wrapper should use group encoding; this is more
         /// efficient to write, but may be hard to consume in cross-platform scenarios; this feature is
         /// usually used for compatibility with protobuf-net v2 <c>SupportNull</c> usage</summary>
         public bool AsGroup { get; set; }
@@ -21,9 +21,9 @@ public sealed class NullWrappedValueAttribute : Attribute
     /// see https://protobuf-net.github.io/protobuf-net/nullwrappers for more information.
     /// </summary>
     [AttributeUsage(AttributeTargets.Field | AttributeTargets.Property, AllowMultiple = false)]
-    internal sealed class NullWrappedCollectionAttribute : Attribute
+    public sealed class NullWrappedCollectionAttribute : Attribute
     {
-        /// <summary>Indicates that the collection message wrapper shold use group encoding; this is more efficient to write, but may be hard to consume in cross-platform scenarios.</summary>
+        /// <summary>Indicates that the collection message wrapper should use group encoding; this is more efficient to write, but may be hard to consume in cross-platform scenarios.</summary>
         public bool AsGroup { get; set; }
     }
 }

src/protobuf-net.Core/ProtoWriter.BufferWriter.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Buffers;
 using System.Collections.Generic;
+using System.Diagnostics;
 using System.IO;
 using System.Runtime.CompilerServices;
 
@@ -14,7 +15,7 @@ public partial class ProtoWriter
         partial struct State
         {
             /// <summary>
-            /// Create a new ProtoWriter that tagets a buffer writer
+            /// Create a new ProtoWriter that targets a buffer writer
             /// </summary>
             public static State Create(IBufferWriter<byte> writer, TypeModel model, object userState = null)
                 => BufferWriterProtoWriter.CreateBufferWriterProtoWriter(writer, model, userState);
@@ -214,6 +215,113 @@ protected internal override void WriteMessage<T>(ref State state, T value, ISeri
                 }
             }
 
+            internal override void WriteWrappedItem<T>(ref State state, SerializerFeatures features, T value, ISerializer<T> serializer)
+            {
+                switch (WireType)
+                {
+                    case WireType.String:
+                        serializer ??= TypeModel.GetSerializer<T>(Model);
+                        long calculatedLength = MeasureAny<T>(_nullWriter, TypeModel.ListItemTag, features, value, serializer);
+
+                        // write length-prefix as varint
+                        AdvanceAndReset(ImplWriteVarint64(ref state, (ulong)calculatedLength));
+
+                        if (calculatedLength != 0)
+                        {
+                            var oldPos = GetPosition(ref state);
+                            state.WriteAny(TypeModel.ListItemTag, features, value, serializer);
+                            var newPos = GetPosition(ref state);
+
+                            var actualLength = (newPos - oldPos);
+                            if (actualLength != calculatedLength)
+                            {
+                                ThrowHelper.ThrowInvalidOperationException($"Length mismatch; calculated '{calculatedLength}', actual '{actualLength}'");
+                            }
+                        }
+
+                        return;
+                    case WireType.StartGroup:
+                        // forwards-only; can use default implementation
+                        base.WriteWrappedItem<T>(ref state, features, value, serializer);
+                        return;
+                    default:
+                        // if we aren't using length-prefix or group... what are we even?
+                        ThrowHelper.ThrowArgumentOutOfRangeException(nameof(WireType));
+                        return;
+                }
+            }
+
+            internal override void WriteWrappedCollection<TCollection, TItem>(ref State state, SerializerFeatures features, TCollection values, RepeatedSerializer<TCollection, TItem> serializer, ISerializer<TItem> valueSerializer)
+            {
+                switch (WireType)
+                {
+                    case WireType.String:
+                        valueSerializer ??= TypeModel.GetSerializer<TItem>(Model);
+                        long calculatedLength = MeasureRepeated<TCollection, TItem>(_nullWriter, TypeModel.ListItemTag, features, values, serializer, valueSerializer);
+
+                        // write length-prefix as varint
+                        AdvanceAndReset(ImplWriteVarint64(ref state, (ulong)calculatedLength));
+
+                        if (calculatedLength != 0)
+                        {
+                            var oldPos = GetPosition(ref state);
+                            serializer.WriteRepeated(ref state, TypeModel.ListItemTag, features, values, valueSerializer);
+                            var newPos = GetPosition(ref state);
+
+                            var actualLength = (newPos - oldPos);
+                            if (actualLength != calculatedLength)
+                            {
+                                ThrowHelper.ThrowInvalidOperationException($"Length mismatch; calculated '{calculatedLength}', actual '{actualLength}'");
+                            }
+                        }
+
+                        return;
+                    case WireType.StartGroup:
+                        // forwards-only; can use default implementation
+                        base.WriteWrappedCollection<TCollection, TItem>(ref state, features, values, serializer, valueSerializer);
+                        return;
+                    default:
+                        // if we aren't using length-prefix or group... what are we even?
+                        ThrowHelper.ThrowArgumentOutOfRangeException(nameof(WireType));
+                        return;
+                }
+            }
+
+            internal override void WriteWrappedMap<TCollection, TKey, TValue>(ref State state, SerializerFeatures features, TCollection values, MapSerializer<TCollection, TKey, TValue> serializer, SerializerFeatures keyFeatures, SerializerFeatures valueFeatures, ISerializer<TKey> keySerializer, ISerializer<TValue> valueSerializer)
+            {
+                switch (WireType)
+                {
+                    case WireType.String:
+                        long calculatedLength = MeasureMap<TCollection, TKey, TValue>(_nullWriter, TypeModel.ListItemTag, features, values, serializer, keyFeatures, valueFeatures, keySerializer, valueSerializer);
+
+                        // write length-prefix as varint
+                        AdvanceAndReset(ImplWriteVarint64(ref state, (ulong)calculatedLength));
+
+                        if (calculatedLength != 0)
+                        {
+                            var oldPos = GetPosition(ref state);
+                            serializer.WriteMap(ref state, TypeModel.ListItemTag, features, values, keyFeatures, valueFeatures, keySerializer, valueSerializer);
+                            var newPos = GetPosition(ref state);
+
+                            var actualLength = (newPos - oldPos);
+                            if (actualLength != calculatedLength)
+                            {
+                                ThrowHelper.ThrowInvalidOperationException($"Length mismatch; calculated '{calculatedLength}', actual '{actualLength}'");
+                            }
+                        }
+
+                        return;
+                    case WireType.StartGroup:
+                        // forwards-only; can use default implementation
+                        base.WriteWrappedMap(ref state, features, values, serializer, keyFeatures, valueFeatures, keySerializer, valueSerializer);
+                        return;
+                    default:
+                        // if we aren't using length-prefix or group... what are we even?
+                        ThrowHelper.ThrowArgumentOutOfRangeException(nameof(WireType));
+                        return;
+                }
+            }
+
             protected internal override void WriteSubType<T>(ref State state, T value, ISubTypeSerializer<T> serializer)
             {
                 switch (WireType)

src/protobuf-net.Core/ProtoWriter.Null.cs

@@ -78,10 +78,27 @@ private void CheckOversized(ref State state)
 
             protected internal override void WriteMessage<T>(ref State state, T value, ISerializer<T> serializer, PrefixStyle style, bool recursionCheck)
             {
-                if (serializer is null) serializer = TypeModel.GetSerializer<T>(Model);
-                var len = Measure<T>(this, value, serializer);
+                var len = Measure<T>(this, value, serializer ?? TypeModel.GetSerializer<T>(Model));
                 AdvanceSubMessage(ref state, len, style);
             }
+
+            internal override void WriteWrappedItem<T>(ref State state, SerializerFeatures features, T value, ISerializer<T> serializer)
+            {
+                var len = MeasureAny<T>(this, TypeModel.ListItemTag, features, value, serializer ?? TypeModel.GetSerializer<T>(Model));
+                AdvanceSubMessage(ref state, len, PrefixStyle.Base128); // only supported styles are group+varint
+            }
+            internal override void WriteWrappedCollection<TCollection, TItem>(ref State state, SerializerFeatures features, TCollection values, RepeatedSerializer<TCollection, TItem> serializer, ISerializer<TItem> valueSerializer)
+            {
+                var len = MeasureRepeated<TCollection, TItem>(this, TypeModel.ListItemTag, features, values, serializer, valueSerializer ?? TypeModel.GetSerializer<TItem>(Model));
+                AdvanceSubMessage(ref state, len, PrefixStyle.Base128); // only supported styles are group+varint
+            }
+
+            internal override void WriteWrappedMap<TCollection, TKey, TValue>(ref State state, SerializerFeatures features, TCollection values, MapSerializer<TCollection, TKey, TValue> serializer, SerializerFeatures keyFeatures, SerializerFeatures valueFeatures, ISerializer<TKey> keySerializer, ISerializer<TValue> valueSerializer)
+            {
+                var len = MeasureMap<TCollection, TKey, TValue>(this, TypeModel.ListItemTag, features, values, serializer, keyFeatures, valueFeatures, keySerializer, valueSerializer);
+                AdvanceSubMessage(ref state, len, PrefixStyle.Base128); // only supported styles are group+varint
+            }
+
             private void AdvanceSubMessage(ref State state, long length, PrefixStyle style)
             {
                 long preamble;

src/protobuf-net.Core/ProtoWriter.State.WriteMethods.cs

@@ -2,6 +2,7 @@
 using ProtoBuf.Meta;
 using ProtoBuf.Serializers;
 using System;
+using System.Diagnostics;
 using System.Diagnostics.CodeAnalysis;
 using System.IO;
 using System.Runtime.CompilerServices;
@@ -340,6 +341,7 @@ public void WriteUInt64(ulong value)
             [MethodImpl(ProtoReader.HotPath)]
             public void WriteMessage<[DynamicallyAccessedMembers(DynamicAccess.ContractType)] T>(int fieldNumber, SerializerFeatures features, T value, ISerializer<T> serializer = null)
             {
+                Debug.Assert(!features.HasAny(SerializerFeatures.OptionWrappedValue), "wrapped value handling has not been processed correctly");
                 if (!(TypeHelper<T>.CanBeNull && TypeHelper<T>.ValueChecker.IsNull(value)))
                 {
                     WriteFieldHeader(fieldNumber, features.IsGroup() ? WireType.StartGroup : WireType.String);
@@ -408,18 +410,19 @@ static WireType AssertWrappedAndGetWireType(ref SerializerFeatures features,
                     return;
                 }
                 WriteFieldHeader(fieldNumber, wrapperFormat);
-#pragma warning disable CS0618 // we don't want to use WriteMessage here; this is a pseudo message layer
-                var tok = StartSubItem(TypeHelper<T>.IsReferenceType & features.ApplyRecursionCheck() ? (object)value : null, PrefixStyle.Base128);
-#pragma warning restore CS0618
+
                 if (!isNull && (fieldPresence || TypeHelper<T>.ValueChecker.HasNonTrivialValue(value)))
                 {   // only write the field if it has a meaningful value (note: we already remove the relevant wrap options,
                     // so: not recursive); if we're using field-presence, we write any non-null value; otherwise,
                     // (think 'wrappers.proto') we only write non-zero values
-                    WriteAny<T>(1, features, value, serializer);
+
+                    Debug.Assert(!features.HasAny(SerializerFeatures.OptionWrappedValue), "should not be wrapped");
+                    GetWriter().WriteWrappedItem<T>(ref this, features, value, serializer);
+                }
+                else
+                {
+                    GetWriter().WriteEmptyWrappedItem(ref this);
                 }
-#pragma warning disable CS0618 // we don't want to use WriteMessage here; this is a pseudo message layer
-                EndSubItem(tok);
-#pragma warning restore CS0618
             }
 
             /// <summary>
@@ -430,7 +433,7 @@ static WireType AssertWrappedAndGetWireType(ref SerializerFeatures features,
                 serializer ??= TypeModel.GetSerializer<T>(Model);
                 features.InheritFrom(serializer.Features);
 
-                if (features.HasAny(SerializerFeatures.OptionWrappedValue | SerializerFeatures.OptionWrappedCollection))
+                if (features.HasAny(SerializerFeatures.OptionWrappedValue))
                 {
                     WriteWrapped<T>(fieldNumber, features, value, serializer);
                     return;