Fixes from PR review.

3 files changed, 89 insertions, 14 deletions

diff --git a/csharp/src/ProtocolBuffers.Test/WellKnownTypes/WrappersTest.cs b/csharp/src/ProtocolBuffers.Test/WellKnownTypes/WrappersTest.cs
index 6c706014..ad88c4eb 100644
--- a/csharp/src/ProtocolBuffers.Test/WellKnownTypes/WrappersTest.cs
+++ b/csharp/src/ProtocolBuffers.Test/WellKnownTypes/WrappersTest.cs
@@ -34,6 +34,7 @@ using System;
 using Google.Protobuf.TestProtos;
 using NUnit.Framework;
 using System.Collections;
+using System.IO;
 
 namespace Google.Protobuf.WellKnownTypes
 {
@@ -232,9 +233,44 @@ namespace Google.Protobuf.WellKnownTypes
             Assert.IsTrue(dictionary.Contains(3));
         }
 
-        // Merging is odd with wrapper types, due to the way that default values aren't emitted in
-        // the binary stream. In fact we cheat a little bit - a message with an explicitly present default
-        // value will have that default value ignored.
+        [Test]
+        public void Oneof()
+        {
+            var message = new OneofWellKnownTypes { EmptyField = new Empty() };
+            // Start off with a non-wrapper
+            Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.EmptyField, message.OneofFieldCase);
+            AssertOneofRoundTrip(message);
+
+            message.StringField = "foo";
+            Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.StringField, message.OneofFieldCase);
+            AssertOneofRoundTrip(message);
+
+            message.StringField = "foo";
+            Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.StringField, message.OneofFieldCase);
+            AssertOneofRoundTrip(message);
+
+            message.DoubleField = 0.0f;
+            Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.DoubleField, message.OneofFieldCase);
+            AssertOneofRoundTrip(message);
+
+            message.DoubleField = 1.0f;
+            Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.DoubleField, message.OneofFieldCase);
+            AssertOneofRoundTrip(message);
+
+            message.ClearOneofField();
+            Assert.AreEqual(OneofWellKnownTypes.OneofFieldOneofCase.None, message.OneofFieldCase);
+            AssertOneofRoundTrip(message);
+        }
+
+        private void AssertOneofRoundTrip(OneofWellKnownTypes message)
+        {
+            // Normal roundtrip, but explicitly checking the case...
+            var bytes = message.ToByteArray();
+            var parsed = OneofWellKnownTypes.Parser.ParseFrom(bytes);
+            Assert.AreEqual(message, parsed);
+            Assert.AreEqual(message.OneofFieldCase, parsed.OneofFieldCase);
+        }
+
         [Test]
         [TestCase("x", "y", "y")]
         [TestCase("x", "", "x")]
@@ -257,5 +293,34 @@ namespace Google.Protobuf.WellKnownTypes
             originalMessage.MergeFrom(mergingMessage.ToByteArray());
             Assert.AreEqual(expected, originalMessage.StringField);
         }
+
+        // Merging is odd with wrapper types, due to the way that default values aren't emitted in
+        // the binary stream. In fact we cheat a little bit - a message with an explicitly present default
+        // value will have that default value ignored.
+        [Test]
+        public void MergingCornerCase()
+        {
+            var message = new TestWellKnownTypes { Int32Field = 5 };
+
+            // Create a byte array which has the data of an Int32Value explicitly containing a value of 0.
+            // This wouldn't normally happen.
+            byte[] bytes;
+            var wrapperTag = WireFormat.MakeTag(TestWellKnownTypes.Int32FieldFieldNumber, WireFormat.WireType.LengthDelimited);
+            var valueTag = WireFormat.MakeTag(Int32Value.ValueFieldNumber, WireFormat.WireType.Varint);
+            using (var stream = new MemoryStream())
+            {
+                var coded = CodedOutputStream.CreateInstance(stream);
+                coded.WriteTag(wrapperTag);
+                coded.WriteLength(2); // valueTag + a value 0, each one byte
+                coded.WriteTag(valueTag);
+                coded.WriteInt32(0);
+                coded.Flush();
+                bytes = stream.ToArray();
+            }
+
+            message.MergeFrom(bytes);
+            // A normal implementation would have 0 now, as the explicit default would have been overwritten the 5.
+            Assert.AreEqual(5, message.Int32Field);
+        }
     }
 }
diff --git a/csharp/src/ProtocolBuffers/Collections/MapField.cs b/csharp/src/ProtocolBuffers/Collections/MapField.cs
index ead45737..f5e4fd3a 100644
--- a/csharp/src/ProtocolBuffers/Collections/MapField.cs
+++ b/csharp/src/ProtocolBuffers/Collections/MapField.cs
@@ -51,7 +51,7 @@ namespace Google.Protobuf.Collections
     public sealed class MapField<TKey, TValue> : IDeepCloneable<MapField<TKey, TValue>>, IFreezable, IDictionary<TKey, TValue>, IEquatable<MapField<TKey, TValue>>, IDictionary
     {
         // TODO: Don't create the map/list until we have an entry. (Assume many maps will be empty.)
-        private bool allowNullValues;
+        private readonly bool allowNullValues;
         private bool frozen;
         private readonly Dictionary<TKey, LinkedListNode<KeyValuePair<TKey, TValue>>> map =
             new Dictionary<TKey, LinkedListNode<KeyValuePair<TKey, TValue>>>();
diff --git a/csharp/src/ProtocolBuffers/FieldCodec.cs b/csharp/src/ProtocolBuffers/FieldCodec.cs
index caf03286..85462787 100644
--- a/csharp/src/ProtocolBuffers/FieldCodec.cs
+++ b/csharp/src/ProtocolBuffers/FieldCodec.cs
@@ -161,20 +161,30 @@ namespace Google.Protobuf
                 null); // Default value for the wrapper
         }
 
-        // Helper code to create codecs for wrapper types. Somewhat ugly with all the 
+        /// <summary>
+        /// Helper code to create codecs for wrapper types.
+        /// </summary>
+        /// <remarks>
+        /// Somewhat ugly with all the static methods, but the conversions involved to/from nullable types make it
+        /// slightly tricky to improve. So long as we keep the public API (ForClassWrapper, ForStructWrapper) in place,
+        /// we can refactor later if we come up with something cleaner.
+        /// </remarks>
         private static class WrapperCodecs
         {
+            // All the field numbers are the same (1).
+            private const int WrapperValueFieldNumber = Google.Protobuf.WellKnownTypes.Int32Value.ValueFieldNumber;
+
             private static readonly Dictionary<Type, object> Codecs = new Dictionary<Type, object>
             {
-                { typeof(bool), ForBool(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) },
-                { typeof(int), ForInt32(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) },
-                { typeof(long), ForInt64(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) },
-                { typeof(uint), ForUInt32(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) },
-                { typeof(ulong), ForUInt64(WireFormat.MakeTag(1, WireFormat.WireType.Varint)) },
-                { typeof(float), ForFloat(WireFormat.MakeTag(1, WireFormat.WireType.Fixed32)) },
-                { typeof(double), ForDouble(WireFormat.MakeTag(1, WireFormat.WireType.Fixed64)) },
-                { typeof(string), ForString(WireFormat.MakeTag(1, WireFormat.WireType.LengthDelimited)) },
-                { typeof(ByteString), ForBytes(WireFormat.MakeTag(1, WireFormat.WireType.LengthDelimited)) }
+                { typeof(bool), ForBool(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) },
+                { typeof(int), ForInt32(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) },
+                { typeof(long), ForInt64(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) },
+                { typeof(uint), ForUInt32(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) },
+                { typeof(ulong), ForUInt64(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Varint)) },
+                { typeof(float), ForFloat(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Fixed32)) },
+                { typeof(double), ForDouble(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.Fixed64)) },
+                { typeof(string), ForString(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.LengthDelimited)) },
+                { typeof(ByteString), ForBytes(WireFormat.MakeTag(WrapperValueFieldNumber, WireFormat.WireType.LengthDelimited)) }
             };
 
             /// <summary>