Fix handling of repeated wrappers

Previously we were incorrectly packing wrapper types.
This also refactors FieldCodec a bit as well, using more C# 6-ness.

3 files changed, 94 insertions, 77 deletions

diff --git a/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs b/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
index b72ef982..a2c833fe 100644
--- a/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
+++ b/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
@@ -149,6 +149,30 @@ namespace Google.Protobuf.WellKnownTypes
         }
 
         [Test]
+        public void RepeatedWrappersBinaryFormat()
+        {
+            // At one point we accidentally used a packed format for repeated wrappers, which is wrong (and weird).
+            // This test is just to prove that we use the right format.
+
+            var rawOutput = new MemoryStream();
+            var output = new CodedOutputStream(rawOutput);
+            // Write a value of 5
+            output.WriteTag(RepeatedWellKnownTypes.Int32FieldFieldNumber, WireFormat.WireType.LengthDelimited);
+            output.WriteLength(2);
+            output.WriteTag(WrappersReflection.WrapperValueFieldNumber, WireFormat.WireType.Varint);
+            output.WriteInt32(5);
+            // Write a value of 0 (empty message)
+            output.WriteTag(RepeatedWellKnownTypes.Int32FieldFieldNumber, WireFormat.WireType.LengthDelimited);
+            output.WriteLength(0);
+            output.Flush();
+            var expectedBytes = rawOutput.ToArray();
+
+            var message = new RepeatedWellKnownTypes { Int32Field = { 5, 0 } };
+            var actualBytes = message.ToByteArray();
+            Assert.AreEqual(expectedBytes, actualBytes);
+        }
+
+        [Test]
         public void MapWrappersSerializeDeserialize()
         {
             // Note: no null values here, as they are prohibited in map fields
diff --git a/csharp/src/Google.Protobuf/Collections/RepeatedField.cs b/csharp/src/Google.Protobuf/Collections/RepeatedField.cs
index e3f65afe..1cde03bc 100644
--- a/csharp/src/Google.Protobuf/Collections/RepeatedField.cs
+++ b/csharp/src/Google.Protobuf/Collections/RepeatedField.cs
@@ -34,7 +34,6 @@ using System;
 using System.Collections;
 using System.Collections.Generic;
 using System.Text;
-using Google.Protobuf.Compatibility;
 
 namespace Google.Protobuf.Collections
 {
@@ -96,8 +95,8 @@ namespace Google.Protobuf.Collections
             // iteration.
             uint tag = input.LastTag;
             var reader = codec.ValueReader;
-            // Value types can be packed or not.
-            if (typeof(T).IsValueType() && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited)
+            // Non-nullable value types can be packed or not.
+            if (FieldCodec<T>.IsPackedRepeatedField(tag))
             {
                 int length = input.ReadLength();
                 if (length > 0)
@@ -134,7 +133,7 @@ namespace Google.Protobuf.Collections
                 return 0;
             }
             uint tag = codec.Tag;
-            if (typeof(T).IsValueType() && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited)
+            if (codec.PackedRepeatedField)
             {
                 int dataSize = CalculatePackedDataSize(codec);
                 return CodedOutputStream.ComputeRawVarint32Size(tag) +
@@ -186,7 +185,7 @@ namespace Google.Protobuf.Collections
             }
             var writer = codec.ValueWriter;
             var tag = codec.Tag;
-            if (typeof(T).IsValueType() && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited)
+            if (codec.PackedRepeatedField)
             {
                 // Packed primitive type
                 uint size = (uint)CalculatePackedDataSize(codec);
diff --git a/csharp/src/Google.Protobuf/FieldCodec.cs b/csharp/src/Google.Protobuf/FieldCodec.cs
index c2b8d268..98313088 100644
--- a/csharp/src/Google.Protobuf/FieldCodec.cs
+++ b/csharp/src/Google.Protobuf/FieldCodec.cs
@@ -30,6 +30,7 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #endregion
 
+using Google.Protobuf.Compatibility;
 using Google.Protobuf.WellKnownTypes;
 using System;
 using System.Collections.Generic;
@@ -329,17 +330,24 @@ namespace Google.Protobuf
     }
 
     /// <summary>
+    /// <para>
     /// An encode/decode pair for a single field. This effectively encapsulates
     /// all the information needed to read or write the field value from/to a coded
     /// stream.
+    /// </para>
+    /// <para>
+    /// This class is public and has to be as it is used by generated code, but its public
+    /// API is very limited - just what the generated code needs to call directly.
+    /// </para>
     /// </summary>
     /// <remarks>
-    /// This never writes default values to the stream, and is not currently designed
-    /// to play well with packed arrays.
+    /// This never writes default values to the stream, and does not address "packedness"
+    /// in repeated fields itself, other than to know whether or not the field *should* be packed.
     /// </remarks>
     public sealed class FieldCodec<T>
     {
         private static readonly T DefaultDefault;
+        private static readonly bool TypeSupportsPacking = typeof(T).IsValueType() && Nullable.GetUnderlyingType(typeof(T)) == null;
 
         static FieldCodec()
         {
@@ -354,75 +362,31 @@ namespace Google.Protobuf
             // Otherwise it's the default value of the CLR type
         }
 
-        private readonly Func<CodedInputStream, T> reader;
-        private readonly Action<CodedOutputStream, T> writer;
-        private readonly Func<T, int> sizeCalculator;
-        private readonly uint tag;
-        private readonly int tagSize;
-        private readonly int fixedSize;
-        // Default value for this codec. Usually the same for every instance of the same type, but
-        // for string/ByteString wrapper fields the codec's default value is null, whereas for
-        // other string/ByteString fields it's "" or ByteString.Empty.
-        private readonly T defaultValue;
+        internal static bool IsPackedRepeatedField(uint tag) =>
+            TypeSupportsPacking && WireFormat.GetTagWireType(tag) == WireFormat.WireType.LengthDelimited;
 
-        internal FieldCodec(
-            Func<CodedInputStream, T> reader,
-            Action<CodedOutputStream, T> writer,
-            Func<T, int> sizeCalculator,
-            uint tag) : this(reader, writer, sizeCalculator, tag, DefaultDefault)
-        {
-        }
-
-        internal FieldCodec(
-            Func<CodedInputStream, T> reader,
-            Action<CodedOutputStream, T> writer,
-            Func<T, int> sizeCalculator,
-            uint tag,
-            T defaultValue)
-        {
-            this.reader = reader;
-            this.writer = writer;
-            this.sizeCalculator = sizeCalculator;
-            this.fixedSize = 0;
-            this.tag = tag;
-            this.defaultValue = defaultValue;
-            tagSize = CodedOutputStream.ComputeRawVarint32Size(tag);
-        }
-
-        internal FieldCodec(
-            Func<CodedInputStream, T> reader,
-            Action<CodedOutputStream, T> writer,
-            int fixedSize,
-            uint tag)
-        {
-            this.reader = reader;
-            this.writer = writer;
-            this.sizeCalculator = _ => fixedSize;
-            this.fixedSize = fixedSize;
-            this.tag = tag;
-            tagSize = CodedOutputStream.ComputeRawVarint32Size(tag);
-        }
+        internal bool PackedRepeatedField { get; }
 
         /// <summary>
-        /// Returns the size calculator for just a value.
+        /// Returns a delegate to write a value (unconditionally) to a coded output stream.
         /// </summary>
-        internal Func<T, int> ValueSizeCalculator { get { return sizeCalculator; } }
+        internal Action<CodedOutputStream, T> ValueWriter { get; }
 
         /// <summary>
-        /// Returns a delegate to write a value (unconditionally) to a coded output stream.
+        /// Returns the size calculator for just a value.
         /// </summary>
-        internal Action<CodedOutputStream, T> ValueWriter { get { return writer; } }
+        internal Func<T, int> ValueSizeCalculator { get; }
 
         /// <summary>
         /// Returns a delegate to read a value from a coded input stream. It is assumed that
         /// the stream is already positioned on the appropriate tag.
         /// </summary>
-        internal Func<CodedInputStream, T> ValueReader { get { return reader; } }
+        internal Func<CodedInputStream, T> ValueReader { get; }
 
         /// <summary>
         /// Returns the fixed size for an entry, or 0 if sizes vary.
         /// </summary>
-        internal int FixedSize { get { return fixedSize; } }
+        internal int FixedSize { get; }
 
         /// <summary>
         /// Gets the tag of the codec.
@@ -430,15 +394,54 @@ namespace Google.Protobuf
         /// <value>
         /// The tag of the codec.
         /// </value>
-        public uint Tag { get { return tag; } }
+        internal uint Tag { get; }
 
         /// <summary>
-        /// Gets the default value of the codec's type.
+        /// Default value for this codec. Usually the same for every instance of the same type, but
+        /// for string/ByteString wrapper fields the codec's default value is null, whereas for
+        /// other string/ByteString fields it's "" or ByteString.Empty.
         /// </summary>
         /// <value>
         /// The default value of the codec's type.
         /// </value>
-        public T DefaultValue { get { return defaultValue; } }
+        internal T DefaultValue { get; }
+
+        private readonly int tagSize;
+        
+        internal FieldCodec(
+                Func<CodedInputStream, T> reader,
+                Action<CodedOutputStream, T> writer,
+                int fixedSize,
+                uint tag) : this(reader, writer, _ => fixedSize, tag)
+        {
+            FixedSize = fixedSize;
+        }
+
+        internal FieldCodec(
+            Func<CodedInputStream, T> reader,
+            Action<CodedOutputStream, T> writer,
+            Func<T, int> sizeCalculator,
+            uint tag) : this(reader, writer, sizeCalculator, tag, DefaultDefault)
+        {
+        }
+
+        internal FieldCodec(
+            Func<CodedInputStream, T> reader,
+            Action<CodedOutputStream, T> writer,
+            Func<T, int> sizeCalculator,
+            uint tag,
+            T defaultValue)
+        {
+            ValueReader = reader;
+            ValueWriter = writer;
+            ValueSizeCalculator = sizeCalculator;
+            FixedSize = 0;
+            Tag = tag;
+            DefaultValue = defaultValue;
+            tagSize = CodedOutputStream.ComputeRawVarint32Size(tag);
+            // Detect packed-ness once, so we can check for it within RepeatedField<T>.
+            PackedRepeatedField = IsPackedRepeatedField(tag);
+        }
 
         /// <summary>
         /// Write a tag and the given value, *if* the value is not the default.
@@ -447,8 +450,8 @@ namespace Google.Protobuf
         {
             if (!IsDefault(value))
             {
-                output.WriteTag(tag);
-                writer(output, value);
+                output.WriteTag(Tag);
+                ValueWriter(output, value);
             }
         }
 
@@ -457,23 +460,14 @@ namespace Google.Protobuf
         /// </summary>
         /// <param name="input">The input stream to read from.</param>
         /// <returns>The value read from the stream.</returns>
-        public T Read(CodedInputStream input)
-        {
-            return reader(input);
-        }
+        public T Read(CodedInputStream input) => ValueReader(input);
 
         /// <summary>
         /// Calculates the size required to write the given value, with a tag,
         /// if the value is not the default.
         /// </summary>
-        public int CalculateSizeWithTag(T value)
-        {
-            return IsDefault(value) ? 0 : sizeCalculator(value) + tagSize;
-        }
+        public int CalculateSizeWithTag(T value) => IsDefault(value) ? 0 : ValueSizeCalculator(value) + tagSize;
 
-        private bool IsDefault(T value)
-        {
-            return EqualityComparer<T>.Default.Equals(value, defaultValue);
-        }
+        private bool IsDefault(T value) => EqualityComparer<T>.Default.Equals(value, DefaultValue);
     }
 }