Lots more tests for FieldCodec, MapField, RepeatedField

... and some implementation changes to go with them.

csharp/src/ProtocolBuffers.Test/Collections/MapFieldTest.cs

@@ -34,6 +34,8 @@ using System;
 using System.Collections.Generic;
 using Google.Protobuf.TestProtos;
 using NUnit.Framework;
+using System.Collections;
+using System.Linq;
 
 namespace Google.Protobuf.Collections
 {
@@ -53,6 +55,18 @@ namespace Google.Protobuf.Collections
             Assert.IsTrue(message.IsFrozen);
         }
 
+        [Test]
+        public void Freeze_Idempotent()
+        {
+            var message = new ForeignMessage { C = 20 };
+            var map = new MapField<string, ForeignMessage> { { "x", message } };
+            Assert.IsFalse(map.IsFrozen);
+            map.Freeze();
+            Assert.IsTrue(message.IsFrozen);
+            map.Freeze();
+            Assert.IsTrue(message.IsFrozen);
+        }
+
         [Test]
         public void Freeze_PreventsMutation()
         {
@@ -187,6 +201,15 @@ namespace Google.Protobuf.Collections
             EqualityTester.AssertInequality(map1, map2);
         }
 
+        [Test]
+        public void Equality_Simple()
+        {
+            var map = new MapField<string, string>();
+            EqualityTester.AssertEquality(map, map);
+            EqualityTester.AssertInequality(map, null);
+            Assert.IsFalse(map.Equals(new object()));
+        }
+
         [Test]
         public void EqualityIsValueSensitive()
         {
@@ -287,7 +310,8 @@ namespace Google.Protobuf.Collections
             Assert.IsFalse(map.Remove("missing"));
             Assert.AreEqual(1, map.Count);
             Assert.IsTrue(map.Remove("foo"));
-            Assert.AreEqual(0, map.Count);           
+            Assert.AreEqual(0, map.Count);
+            Assert.Throws<ArgumentNullException>(() => map.Remove(null));
         }
 
         [Test]
@@ -346,6 +370,164 @@ namespace Google.Protobuf.Collections
             Assert.AreEqual("z", map["x"]);
         }
 
+        [Test]
+        public void GetEnumerator_NonGeneric()
+        {
+            IEnumerable map = new MapField<string, string> { { "x", "y" } };
+            CollectionAssert.AreEqual(new[] { new KeyValuePair<string, string>("x", "y") },
+                map.Cast<object>().ToList());
+        }
+
+        // Test for the explicitly-implemented non-generic IDictionary interface
+        [Test]
+        public void IDictionary_GetEnumerator()
+        {
+            IDictionary map = new MapField<string, string> { { "x", "y" } };
+            var enumerator = map.GetEnumerator();
+
+            // Commented assertions show an ideal situation - it looks like
+            // the LinkedList enumerator doesn't throw when you ask for the current entry
+            // at an inappropriate time; fixing this would be more work than it's worth.
+            // Assert.Throws<InvalidOperationException>(() => enumerator.Current.GetHashCode());
+            Assert.IsTrue(enumerator.MoveNext());
+            Assert.AreEqual("x", enumerator.Key);
+            Assert.AreEqual("y", enumerator.Value);
+            Assert.AreEqual(new DictionaryEntry("x", "y"), enumerator.Current);
+            Assert.AreEqual(new DictionaryEntry("x", "y"), enumerator.Entry);
+            Assert.IsFalse(enumerator.MoveNext());
+            // Assert.Throws<InvalidOperationException>(() => enumerator.Current.GetHashCode());
+            enumerator.Reset();
+            // Assert.Throws<InvalidOperationException>(() => enumerator.Current.GetHashCode());
+            Assert.IsTrue(enumerator.MoveNext());
+            Assert.AreEqual("x", enumerator.Key); // Assume the rest are okay
+        }
+
+        [Test]
+        public void IDictionary_Add()
+        {
+            var map = new MapField<string, string> { { "x", "y" } };
+            IDictionary dictionary = map;
+            dictionary.Add("a", "b");
+            Assert.AreEqual("b", map["a"]);
+            Assert.Throws<ArgumentException>(() => dictionary.Add("a", "duplicate"));
+            Assert.Throws<InvalidCastException>(() => dictionary.Add(new object(), "key is bad"));
+            Assert.Throws<InvalidCastException>(() => dictionary.Add("value is bad", new object()));
+        }
+
+        [Test]
+        public void IDictionary_Contains()
+        {
+            var map = new MapField<string, string> { { "x", "y" } };
+            IDictionary dictionary = map;
+
+            Assert.IsFalse(dictionary.Contains("a"));
+            Assert.IsFalse(dictionary.Contains(5));
+            // Surprising, but IDictionary.Contains is only about keys.
+            Assert.IsFalse(dictionary.Contains(new DictionaryEntry("x", "y")));
+            Assert.IsTrue(dictionary.Contains("x"));
+        }
+
+        [Test]
+        public void IDictionary_Remove()
+        {
+            var map = new MapField<string, string> { { "x", "y" } };
+            IDictionary dictionary = map;
+            dictionary.Remove("a");
+            Assert.AreEqual(1, dictionary.Count);
+            dictionary.Remove(5);
+            Assert.AreEqual(1, dictionary.Count);
+            dictionary.Remove(new DictionaryEntry("x", "y"));
+            Assert.AreEqual(1, dictionary.Count);
+            dictionary.Remove("x");
+            Assert.AreEqual(0, dictionary.Count);
+            Assert.Throws<ArgumentNullException>(() => dictionary.Remove(null));
+
+            map.Freeze();
+            // Call should fail even though it clearly doesn't contain 5 as a key.
+            Assert.Throws<InvalidOperationException>(() => dictionary.Remove(5));
+        }
+
+        [Test]
+        public void IDictionary_CopyTo()
+        {
+            var map = new MapField<string, string> { { "x", "y" } };
+            IDictionary dictionary = map;
+            var array = new DictionaryEntry[3];
+            dictionary.CopyTo(array, 1);
+            CollectionAssert.AreEqual(new[] { default(DictionaryEntry), new DictionaryEntry("x", "y"), default(DictionaryEntry) },
+                array);
+            var objectArray = new object[3];
+            dictionary.CopyTo(objectArray, 1);
+            CollectionAssert.AreEqual(new object[] { null, new DictionaryEntry("x", "y"), null },
+                objectArray);
+        }
+
+        [Test]
+        public void IDictionary_IsFixedSize()
+        {
+            var map = new MapField<string, string> { { "x", "y" } };
+            IDictionary dictionary = map;
+            Assert.IsFalse(dictionary.IsFixedSize);
+            map.Freeze();
+            Assert.IsTrue(dictionary.IsFixedSize);
+        }
+
+        [Test]
+        public void IDictionary_Keys()
+        {
+            IDictionary dictionary = new MapField<string, string> { { "x", "y" } };
+            CollectionAssert.AreEqual(new[] { "x" }, dictionary.Keys);
+        }
+
+        [Test]
+        public void IDictionary_Values()
+        {
+            IDictionary dictionary = new MapField<string, string> { { "x", "y" } };
+            CollectionAssert.AreEqual(new[] { "y" }, dictionary.Values);
+        }
+
+        [Test]
+        public void IDictionary_IsSynchronized()
+        {
+            IDictionary dictionary = new MapField<string, string> { { "x", "y" } };
+            Assert.IsFalse(dictionary.IsSynchronized);
+        }
+
+        [Test]
+        public void IDictionary_SyncRoot()
+        {
+            IDictionary dictionary = new MapField<string, string> { { "x", "y" } };
+            Assert.AreSame(dictionary, dictionary.SyncRoot);
+        }
+
+        [Test]
+        public void IDictionary_Indexer_Get()
+        {
+            IDictionary dictionary = new MapField<string, string> { { "x", "y" } };
+            Assert.AreEqual("y", dictionary["x"]);
+            Assert.IsNull(dictionary["a"]);
+            Assert.IsNull(dictionary[5]);
+            Assert.Throws<ArgumentNullException>(() => dictionary[null].GetHashCode());
+        }
+
+        [Test]
+        public void IDictionary_Indexer_Set()
+        {
+            var map = new MapField<string, string> { { "x", "y" } };
+            IDictionary dictionary = map;
+            map["a"] = "b";
+            Assert.AreEqual("b", map["a"]);
+            map["a"] = "c";
+            Assert.AreEqual("c", map["a"]);
+            Assert.Throws<InvalidCastException>(() => dictionary[5] = "x");
+            Assert.Throws<InvalidCastException>(() => dictionary["x"] = 5);
+            Assert.Throws<ArgumentNullException>(() => dictionary[null] = "z");
+            Assert.Throws<ArgumentNullException>(() => dictionary["x"] = null);
+            map.Freeze();
+            // Note: Not InvalidOperationException.
+            Assert.Throws<NotSupportedException>(() => dictionary["a"] = "c");
+        }
+
         private static KeyValuePair<TKey, TValue> NewKeyValuePair<TKey, TValue>(TKey key, TValue value)
         {
             return new KeyValuePair<TKey, TValue>(key, value);

csharp/src/ProtocolBuffers.Test/EqualityTester.cs

@@ -45,15 +45,20 @@ namespace Google.Protobuf
         public static void AssertEquality<T>(T first, T second) where T : IEquatable<T>
         {
             Assert.IsTrue(first.Equals(second));
+            Assert.IsTrue(first.Equals((object) second));
             Assert.AreEqual(first.GetHashCode(), second.GetHashCode());
         }
 
         public static void AssertInequality<T>(T first, T second) where T : IEquatable<T>
         {
             Assert.IsFalse(first.Equals(second));
+            Assert.IsFalse(first.Equals((object) second));
             // While this isn't a requirement, the chances of this test failing due to
             // coincidence rather than a bug are very small.
-            Assert.AreNotEqual(first.GetHashCode(), second.GetHashCode());
+            if (first != null && second != null)
+            {
+                Assert.AreNotEqual(first.GetHashCode(), second.GetHashCode());
+            }
         }
     }
 }

csharp/src/ProtocolBuffers.Test/FieldCodecTest.cs

@@ -86,12 +86,22 @@ namespace Google.Protobuf
             codec.TestDefaultValue();
         }
 
+        [Test, TestCaseSource("Codecs")]
+        public void FixedSize(ICodecTestData codec)
+        {
+            codec.TestFixedSize();
+        }
+
+        // This is ugly, but it means we can have a non-generic interface.
+        // It feels like NUnit should support this better, but I don't know
+        // of any better ways right now.
         public interface ICodecTestData
         {
             void TestRoundTripRaw();
             void TestRoundTripWithTag();
             void TestCalculateSizeWithTag();
             void TestDefaultValue();
+            void TestFixedSize();
         }
 
         public class FieldCodecTestData<T> : ICodecTestData
@@ -169,6 +179,11 @@ namespace Google.Protobuf
                 }
             }
 
+            public void TestFixedSize()
+            {
+                Assert.AreEqual(name.Contains("Fixed"), codec.FixedSize != 0);
+            }
+
             public override string ToString()
             {
                 return name;

csharp/src/ProtocolBuffers/Collections/MapField.cs

@@ -367,11 +367,13 @@ namespace Google.Protobuf.Collections
 
         IDictionaryEnumerator IDictionary.GetEnumerator()
         {
-            throw new NotImplementedException();
+            return new DictionaryEnumerator(GetEnumerator());
         }
 
         void IDictionary.Remove(object key)
         {
+            ThrowHelper.ThrowIfNull(key, "key");
+            this.CheckMutable();
             if (!(key is TKey))
             {
                 return;
@@ -381,7 +383,9 @@ namespace Google.Protobuf.Collections
 
         void ICollection.CopyTo(Array array, int index)
         {
-            throw new NotImplementedException();
+            // This is ugly and slow as heck, but with any luck it will never be used anyway.
+            ICollection temp = this.Select(pair => new DictionaryEntry(pair.Key, pair.Value)).ToList();
+            temp.CopyTo(array, index);
         }
 
         bool IDictionary.IsFixedSize { get { return IsFrozen; } }
@@ -392,12 +396,13 @@ namespace Google.Protobuf.Collections
 
         bool ICollection.IsSynchronized { get { return false; } }
 
-        object ICollection.SyncRoot { get { return null; } }
+        object ICollection.SyncRoot { get { return this; } }
 
         object IDictionary.this[object key]
         {
             get
             {
+                ThrowHelper.ThrowIfNull(key, "key");
                 if (!(key is TKey))
                 {
                     return null;
@@ -407,10 +412,42 @@ namespace Google.Protobuf.Collections
                 return value;
             }
 
-            set { this[(TKey)key] = (TValue)value; }
+            set
+            {
+                if (frozen)
+                {
+                    throw new NotSupportedException("Dictionary is frozen");
+                }
+                this[(TKey)key] = (TValue)value;
+            }
         }
         #endregion
 
+        private class DictionaryEnumerator : IDictionaryEnumerator
+        {
+            private readonly IEnumerator<KeyValuePair<TKey, TValue>> enumerator;
+
+            internal DictionaryEnumerator(IEnumerator<KeyValuePair<TKey, TValue>> enumerator)
+            {
+                this.enumerator = enumerator;
+            }
+
+            public bool MoveNext()
+            {
+                return enumerator.MoveNext();
+            }
+
+            public void Reset()
+            {
+                enumerator.Reset();
+            }
+
+            public object Current { get { return Entry; } }
+            public DictionaryEntry Entry { get { return new DictionaryEntry(Key, Value); } }
+            public object Key { get { return enumerator.Current.Key; } }
+            public object Value { get { return enumerator.Current.Value; } }
+        }
+
         /// <summary>
         /// A codec for a specific map field. This contains all the information required to encoded and
         /// decode the nested messages.

csharp/src/ProtocolBuffers/Collections/RepeatedField.cs

@@ -376,6 +376,7 @@ namespace Google.Protobuf.Collections
             this.CheckMutable();
             EnsureSize(count + 1);
             Array.Copy(array, index, array, index + 1, count - index);
+            array[index] = item;
             count++;
         }
 
@@ -421,18 +422,12 @@ namespace Google.Protobuf.Collections
 
         void ICollection.CopyTo(Array array, int index)
         {
-            ThrowHelper.ThrowIfNull(array, "array");
-            T[] strongArray = array as T[];
-            if (strongArray == null)
-            {
-                throw new ArgumentException("Array is of incorrect type", "array");
-            }
-            CopyTo(strongArray, index);
+            Array.Copy(this.array, 0, array, index, count);
         }
 
         bool ICollection.IsSynchronized { get { return false; } }
 
-        object ICollection.SyncRoot { get { return null; } }
+        object ICollection.SyncRoot { get { return this; } }
 
         object IList.this[int index]
         {
@@ -490,6 +485,7 @@ namespace Google.Protobuf.Collections
             {
                 if (index + 1 >= field.Count)
                 {
+                    index = field.Count;
                     return false;
                 }
                 index++;

csharp/src/ProtocolBuffers/FieldCodec.cs

@@ -68,12 +68,12 @@ namespace Google.Protobuf
 
         public static FieldCodec<uint> ForFixed32(uint tag)
         {
-            return new FieldCodec<uint>(input => input.ReadFixed32(), (output, value) => output.WriteFixed32(value), CodedOutputStream.ComputeFixed32Size, tag);
+            return new FieldCodec<uint>(input => input.ReadFixed32(), (output, value) => output.WriteFixed32(value), 4, tag);
         }
 
         public static FieldCodec<int> ForSFixed32(uint tag)
         {
-            return new FieldCodec<int>(input => input.ReadSFixed32(), (output, value) => output.WriteSFixed32(value), CodedOutputStream.ComputeSFixed32Size, tag);
+            return new FieldCodec<int>(input => input.ReadSFixed32(), (output, value) => output.WriteSFixed32(value), 4, tag);
         }
 
         public static FieldCodec<uint> ForUInt32(uint tag)
@@ -93,12 +93,12 @@ namespace Google.Protobuf
 
         public static FieldCodec<ulong> ForFixed64(uint tag)
         {
-            return new FieldCodec<ulong>(input => input.ReadFixed64(), (output, value) => output.WriteFixed64(value), CodedOutputStream.ComputeFixed64Size, tag);
+            return new FieldCodec<ulong>(input => input.ReadFixed64(), (output, value) => output.WriteFixed64(value), 8, tag);
         }
 
         public static FieldCodec<long> ForSFixed64(uint tag)
         {
-            return new FieldCodec<long>(input => input.ReadSFixed64(), (output, value) => output.WriteSFixed64(value), CodedOutputStream.ComputeSFixed64Size, tag);
+            return new FieldCodec<long>(input => input.ReadSFixed64(), (output, value) => output.WriteSFixed64(value), 8, tag);
         }
 
         public static FieldCodec<ulong> ForUInt64(uint tag)