More map tests, and various production code improvements.

Generated code in next commit.

7 files changed, 490 insertions, 39 deletions

diff --git a/csharp/src/ProtocolBuffers.Test/Collections/MapFieldTest.cs b/csharp/src/ProtocolBuffers.Test/Collections/MapFieldTest.cs
new file mode 100644
index 00000000..498e4718
--- /dev/null
+++ b/csharp/src/ProtocolBuffers.Test/Collections/MapFieldTest.cs
@@ -0,0 +1,354 @@
+#region Copyright notice and license
+// Protocol Buffers - Google's data interchange format
+// Copyright 2015 Google Inc.  All rights reserved.
+// https://developers.google.com/protocol-buffers/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#endregion
+
+using System;
+using System.Collections.Generic;
+using Google.Protobuf.TestProtos;
+using NUnit.Framework;
+
+namespace Google.Protobuf.Collections
+{
+    /// <summary>
+    /// Tests for MapField which aren't reliant on the encoded format -
+    /// tests for serialization/deserialization are part of GeneratedMessageTest.
+    /// </summary>
+    public class MapFieldTest
+    {
+        // Protobuf-specific tests
+        [Test]
+        public void Freeze_FreezesMessages()
+        {
+            var message = new ForeignMessage { C = 20 };
+            var map = new MapField<string, ForeignMessage> { { "x", message } };
+            map.Freeze();
+            Assert.IsTrue(message.IsFrozen);
+        }
+
+        [Test]
+        public void Freeze_PreventsMutation()
+        {
+            var map = new MapField<string, string>();
+            map.Freeze();
+            Assert.IsTrue(map.IsFrozen);
+            Assert.IsTrue(map.IsReadOnly);
+            ICollection<KeyValuePair<string, string>> collection = map;
+            Assert.Throws<InvalidOperationException>(() => map["x"] = "y");
+            Assert.Throws<InvalidOperationException>(() => map.Add("x", "y"));
+            Assert.Throws<InvalidOperationException>(() => map.Remove("x"));
+            Assert.Throws<InvalidOperationException>(() => map.Clear());
+            Assert.Throws<InvalidOperationException>(() => collection.Add(NewKeyValuePair("x", "y")));
+            Assert.Throws<InvalidOperationException>(() => collection.Remove(NewKeyValuePair("x", "y")));
+        }
+
+        [Test]
+        public void Clone_ReturnsNonFrozen()
+        {
+            var map = new MapField<string, string>();
+            map.Freeze();
+            var clone = map.Clone();
+            clone.Add("x", "y");
+        }
+
+        [Test]
+        public void Clone_ClonesMessages()
+        {
+            var message = new ForeignMessage { C = 20 };
+            var map = new MapField<string, ForeignMessage> { { "x", message } };
+            var clone = map.Clone();
+            map["x"].C = 30;
+            Assert.AreEqual(20, clone["x"].C);
+        }
+
+        [Test]
+        public void Add_ForbidsNullKeys()
+        {
+            var map = new MapField<string, ForeignMessage>();
+            Assert.Throws<ArgumentNullException>(() => map.Add(null, new ForeignMessage()));
+        }
+
+        [Test]
+        public void Add_AcceptsNullMessageValues()
+        {
+            var map = new MapField<string, ForeignMessage>();
+            map.Add("missing", null);
+            Assert.IsNull(map["missing"]);
+        }
+
+        [Test]
+        public void Add_ForbidsNullStringValues()
+        {
+            var map = new MapField<string, string>();
+            Assert.Throws<ArgumentNullException>(() => map.Add("missing", null));
+        }
+
+        [Test]
+        public void Add_ForbidsNullByteStringValues()
+        {
+            var map = new MapField<string, ByteString>();
+            Assert.Throws<ArgumentNullException>(() => map.Add("missing", null));
+        }
+
+        [Test]
+        public void Indexer_ForbidsNullKeys()
+        {
+            var map = new MapField<string, ForeignMessage>();
+            Assert.Throws<ArgumentNullException>(() => map[null] = new ForeignMessage());
+        }
+
+        [Test]
+        public void Indexer_AcceptsNullMessageValues()
+        {
+            var map = new MapField<string, ForeignMessage>();
+            map["missing"] = null;
+            Assert.IsNull(map["missing"]);
+        }
+
+        [Test]
+        public void Indexer_ForbidsNullStringValues()
+        {
+            var map = new MapField<string, string>();
+            Assert.Throws<ArgumentNullException>(() => map["missing"] = null);
+        }
+
+        [Test]
+        public void Indexer_ForbidsNullByteStringValues()
+        {
+            var map = new MapField<string, ByteString>();
+            Assert.Throws<ArgumentNullException>(() => map["missing"] = null);
+        }
+
+        [Test]
+        public void AddPreservesInsertionOrder()
+        {
+            var map = new MapField<string, string>();
+            map.Add("a", "v1");
+            map.Add("b", "v2");
+            map.Add("c", "v3");
+            map.Remove("b");
+            map.Add("d", "v4");
+            CollectionAssert.AreEqual(new[] { "a", "c", "d" }, map.Keys);
+            CollectionAssert.AreEqual(new[] { "v1", "v3", "v4" }, map.Values);
+        }
+
+        [Test]
+        public void EqualityIsOrderInsensitive()
+        {
+            var map1 = new MapField<string, string>();
+            map1.Add("a", "v1");
+            map1.Add("b", "v2");
+
+            var map2 = new MapField<string, string>();
+            map2.Add("b", "v2");
+            map2.Add("a", "v1");
+
+            EqualityTester.AssertEquality(map1, map2);
+        }
+
+        [Test]
+        public void EqualityIsKeySensitive()
+        {
+            var map1 = new MapField<string, string>();
+            map1.Add("a1", "v1");
+            map1.Add("b1", "v2");
+
+            var map2 = new MapField<string, string>();
+            map2.Add("a2", "v1");
+            map2.Add("b2", "v2");
+
+            EqualityTester.AssertInequality(map1, map2);
+        }
+
+        [Test]
+        public void EqualityIsValueSensitive()
+        {
+            // Note: Without some care, it's a little easier than one might
+            // hope to see hash collisions, but only in some environments...
+            var map1 = new MapField<string, string>();
+            map1.Add("a", "a1");
+            map1.Add("b", "a2");
+
+            var map2 = new MapField<string, string>();
+            map2.Add("a", "b1");
+            map2.Add("b", "b2");
+
+            EqualityTester.AssertInequality(map1, map2);
+        }
+
+        [Test]
+        public void EqualityHandlesNullValues()
+        {
+            var map1 = new MapField<string, ForeignMessage>();
+            map1.Add("a", new ForeignMessage { C = 10 });
+            map1.Add("b", null);
+
+            var map2 = new MapField<string, ForeignMessage>();
+            map2.Add("a", new ForeignMessage { C = 10 });
+            map2.Add("b", null);
+
+            EqualityTester.AssertEquality(map1, map2);
+            // Check the null value isn't ignored entirely...
+            Assert.IsTrue(map1.Remove("b"));
+            EqualityTester.AssertInequality(map1, map2);
+            map1.Add("b", new ForeignMessage());
+            EqualityTester.AssertInequality(map1, map2);
+            map1["b"] = null;
+            EqualityTester.AssertEquality(map1, map2);
+        }
+
+        [Test]
+        public void Add_Dictionary()
+        {
+            var map1 = new MapField<string, string>
+            {
+                { "x", "y" },
+                { "a", "b" }
+            };
+            var map2 = new MapField<string, string>
+            {
+                { "before", "" },
+                map1,
+                { "after", "" }
+            };
+            var expected = new MapField<string, string>
+            {
+                { "before", "" },
+                { "x", "y" },
+                { "a", "b" },
+                { "after", "" }
+            };
+            Assert.AreEqual(expected, map2);
+            CollectionAssert.AreEqual(new[] { "before", "x", "a", "after" }, map2.Keys);
+        }
+
+        // General IDictionary<TKey, TValue> behavior tests
+        [Test]
+        public void Add_KeyAlreadyExists()
+        {
+            var map = new MapField<string, string>();
+            map.Add("foo", "bar");
+            Assert.Throws<ArgumentException>(() => map.Add("foo", "baz"));
+        }
+
+        [Test]
+        public void Add_Pair()
+        {
+            var map = new MapField<string, string>();
+            ICollection<KeyValuePair<string, string>> collection = map;
+            collection.Add(NewKeyValuePair("x", "y"));
+            Assert.AreEqual("y", map["x"]);
+            Assert.Throws<ArgumentException>(() => collection.Add(NewKeyValuePair("x", "z")));
+        }
+
+        [Test]
+        public void Contains_Pair()
+        {
+            var map = new MapField<string, string> { { "x", "y" } };
+            ICollection<KeyValuePair<string, string>> collection = map;
+            Assert.IsTrue(collection.Contains(NewKeyValuePair("x", "y")));
+            Assert.IsFalse(collection.Contains(NewKeyValuePair("x", "z")));
+            Assert.IsFalse(collection.Contains(NewKeyValuePair("z", "y")));
+        }
+
+        [Test]
+        public void Remove_Key()
+        {
+            var map = new MapField<string, string>();
+            map.Add("foo", "bar");
+            Assert.AreEqual(1, map.Count);
+            Assert.IsFalse(map.Remove("missing"));
+            Assert.AreEqual(1, map.Count);
+            Assert.IsTrue(map.Remove("foo"));
+            Assert.AreEqual(0, map.Count);           
+        }
+
+        [Test]
+        public void Remove_Pair()
+        {
+            var map = new MapField<string, string>();
+            map.Add("foo", "bar");
+            ICollection<KeyValuePair<string, string>> collection = map;
+            Assert.AreEqual(1, map.Count);
+            Assert.IsFalse(collection.Remove(NewKeyValuePair("wrong key", "bar")));
+            Assert.AreEqual(1, map.Count);
+            Assert.IsFalse(collection.Remove(NewKeyValuePair("foo", "wrong value")));
+            Assert.AreEqual(1, map.Count);
+            Assert.IsTrue(collection.Remove(NewKeyValuePair("foo", "bar")));
+            Assert.AreEqual(0, map.Count);
+            Assert.Throws<ArgumentException>(() => collection.Remove(new KeyValuePair<string, string>(null, "")));
+        }
+
+        [Test]
+        public void CopyTo_Pair()
+        {
+            var map = new MapField<string, string>();
+            map.Add("foo", "bar");
+            ICollection<KeyValuePair<string, string>> collection = map;
+            KeyValuePair<string, string>[] array = new KeyValuePair<string, string>[3];
+            collection.CopyTo(array, 1);
+            Assert.AreEqual(NewKeyValuePair("foo", "bar"), array[1]);
+        }
+
+        [Test]
+        public void Clear()
+        {
+            var map = new MapField<string, string> { { "x", "y" } };
+            Assert.AreEqual(1, map.Count);
+            map.Clear();
+            Assert.AreEqual(0, map.Count);
+            map.Add("x", "y");
+            Assert.AreEqual(1, map.Count);
+        }
+
+        [Test]
+        public void Indexer_Get()
+        {
+            var map = new MapField<string, string> { { "x", "y" } };
+            Assert.AreEqual("y", map["x"]);
+            Assert.Throws<KeyNotFoundException>(() => { var ignored = map["z"]; });
+        }
+
+        [Test]
+        public void Indexer_Set()
+        {
+            var map = new MapField<string, string>();
+            map["x"] = "y";
+            Assert.AreEqual("y", map["x"]);
+            map["x"] = "z"; // This won't throw, unlike Add.
+            Assert.AreEqual("z", map["x"]);
+        }
+
+        private static KeyValuePair<TKey, TValue> NewKeyValuePair<TKey, TValue>(TKey key, TValue value)
+        {
+            return new KeyValuePair<TKey, TValue>(key, value);
+        }
+    }
+}
diff --git a/csharp/src/ProtocolBuffers.Test/RepeatedFieldTest.cs b/csharp/src/ProtocolBuffers.Test/Collections/RepeatedFieldTest.cs
index c01cd8ff..29945c36 100644
--- a/csharp/src/ProtocolBuffers.Test/RepeatedFieldTest.cs
+++ b/csharp/src/ProtocolBuffers.Test/Collections/RepeatedFieldTest.cs
@@ -1,10 +1,9 @@
 using System;
 using System.Collections.Generic;
-using Google.Protobuf.Collections;
 using Google.Protobuf.TestProtos;
 using NUnit.Framework;
 
-namespace Google.Protobuf
+namespace Google.Protobuf.Collections
 {
     public class RepeatedFieldTest
     {
diff --git a/csharp/src/ProtocolBuffers.Test/EqualityTester.cs b/csharp/src/ProtocolBuffers.Test/EqualityTester.cs
new file mode 100644
index 00000000..816f2be1
--- /dev/null
+++ b/csharp/src/ProtocolBuffers.Test/EqualityTester.cs
@@ -0,0 +1,63 @@
+#region Copyright notice and license
+// Protocol Buffers - Google's data interchange format
+// Copyright 2015 Google Inc.  All rights reserved.
+// https://developers.google.com/protocol-buffers/
+//
+// Redistribution and use in source and binary forms, with or without
+// modification, are permitted provided that the following conditions are
+// met:
+//
+//     * Redistributions of source code must retain the above copyright
+// notice, this list of conditions and the following disclaimer.
+//     * Redistributions in binary form must reproduce the above
+// copyright notice, this list of conditions and the following disclaimer
+// in the documentation and/or other materials provided with the
+// distribution.
+//     * Neither the name of Google Inc. nor the names of its
+// contributors may be used to endorse or promote products derived from
+// this software without specific prior written permission.
+//
+// THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+// "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+// LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+// A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+// OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+// SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+// LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+// DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+// THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+// (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+// OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+#endregion
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Text;
+using System.Threading.Tasks;
+using NUnit.Framework;
+
+namespace Google.Protobuf
+{
+    /// <summary>
+    /// Helper methods when testing equality. NUnit's Assert.AreEqual and
+    /// Assert.AreNotEqual methods try to be clever with collections, which can
+    /// be annoying...
+    /// </summary>
+    internal static class EqualityTester
+    {
+        public static void AssertEquality<T>(T first, T second) where T : IEquatable<T>
+        {
+            Assert.IsTrue(first.Equals(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));
+            // 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());
+        }
+    }
+}
diff --git a/csharp/src/ProtocolBuffers.Test/GeneratedMessageTest.cs b/csharp/src/ProtocolBuffers.Test/GeneratedMessageTest.cs
index 81e35940..efeb7a97 100644
--- a/csharp/src/ProtocolBuffers.Test/GeneratedMessageTest.cs
+++ b/csharp/src/ProtocolBuffers.Test/GeneratedMessageTest.cs
@@ -12,6 +12,15 @@ namespace Google.Protobuf
     public class GeneratedMessageTest

     {

         [Test]

+        public void EmptyMessageFieldDistinctFromMissingMessageField()

+        {

+            // This demonstrates what we're really interested in...

+            var message1 = new TestAllTypes { SingleForeignMessage = new ForeignMessage() };

+            var message2 = new TestAllTypes(); // SingleForeignMessage is null

+            EqualityTester.AssertInequality(message1, message2);

+        }

+

+        [Test]

         public void DefaultValues()

         {

             // Single fields

diff --git a/csharp/src/ProtocolBuffers.Test/ProtocolBuffers.Test.csproj b/csharp/src/ProtocolBuffers.Test/ProtocolBuffers.Test.csproj
index 31095a5b..d11e2596 100644
--- a/csharp/src/ProtocolBuffers.Test/ProtocolBuffers.Test.csproj
+++ b/csharp/src/ProtocolBuffers.Test/ProtocolBuffers.Test.csproj
@@ -74,8 +74,10 @@
     <Compile Include="ByteStringTest.cs" />

     <Compile Include="CodedInputStreamTest.cs" />

     <Compile Include="CodedOutputStreamTest.cs" />

+    <Compile Include="EqualityTester.cs" />

     <Compile Include="GeneratedMessageTest.cs" />

-    <Compile Include="RepeatedFieldTest.cs" />

+    <Compile Include="Collections\MapFieldTest.cs" />

+    <Compile Include="Collections\RepeatedFieldTest.cs" />

     <Compile Include="TestProtos\UnittestImportProto3.cs" />

     <Compile Include="TestProtos\UnittestImportPublicProto3.cs" />

     <Compile Include="TestProtos\UnittestIssues.cs" />

@@ -99,9 +101,7 @@
   <ItemGroup>

     <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />

   </ItemGroup>

-  <ItemGroup>

-    <Folder Include="Collections\" />

-  </ItemGroup>

+  <ItemGroup />

   <Import Project="$(MSBuildToolsPath)\Microsoft.CSharp.targets" />

   <!-- To modify your build process, add your task inside one of the targets below and uncomment it. 

        Other similar extension points exist, see Microsoft.Common.targets.

diff --git a/csharp/src/ProtocolBuffers/CodedInputStream.cs b/csharp/src/ProtocolBuffers/CodedInputStream.cs
index 905cdb9d..dab3e5e3 100644
--- a/csharp/src/ProtocolBuffers/CodedInputStream.cs
+++ b/csharp/src/ProtocolBuffers/CodedInputStream.cs
@@ -456,14 +456,16 @@ namespace Google.Protobuf
         }

 

         /// <summary>

-        /// Returns true if the next tag is also part of the same unpacked array.

+        /// Peeks at the next tag in the stream. If it matches <paramref name="tag"/>,

+        /// the tag is consumed and the method returns <c>true</c>; otherwise, the

+        /// stream is left in the original position and the method returns <c>false</c>.

         /// </summary>

-        private bool ContinueArray(uint currentTag)

+        public bool MaybeConsumeTag(uint tag)

         {

             uint next;

             if (PeekNextTag(out next))

             {

-                if (next == currentTag)

+                if (next == tag)

                 {

                     hasNextTag = false;

                     return true;

@@ -486,17 +488,7 @@ namespace Google.Protobuf
                 }

                 return true;

             }

-

-            uint next;

-            if (PeekNextTag(out next))

-            {

-                if (next == currentTag)

-                {

-                    hasNextTag = false;

-                    return true;

-                }

-            }

-            return false;

+            return MaybeConsumeTag(currentTag);

         }

 

         /// <summary>

@@ -512,7 +504,7 @@ namespace Google.Protobuf
             do

             {

                 list.Add(ReadString());

-            } while (ContinueArray(fieldTag));

+            } while (MaybeConsumeTag(fieldTag));

         }

 

         public void ReadBytesArray(ICollection<ByteString> list)

@@ -521,7 +513,7 @@ namespace Google.Protobuf
             do

             {

                 list.Add(ReadBytes());

-            } while (ContinueArray(fieldTag));

+            } while (MaybeConsumeTag(fieldTag));

         }

 

         public void ReadBoolArray(ICollection<bool> list)

@@ -729,7 +721,7 @@ namespace Google.Protobuf
                 do

                 {

                     list.Add((T)(object) ReadEnum());

-                } while (ContinueArray(fieldTag));

+                } while (MaybeConsumeTag(fieldTag));

             }

         }

 

@@ -742,7 +734,7 @@ namespace Google.Protobuf
                 T message = messageParser.CreateTemplate();

                 ReadMessage(message);

                 list.Add(message);

-            } while (ContinueArray(fieldTag));

+            } while (MaybeConsumeTag(fieldTag));

         }

         #endregion

 

diff --git a/csharp/src/ProtocolBuffers/Collections/MapField.cs b/csharp/src/ProtocolBuffers/Collections/MapField.cs
index 2f5a741d..6834cfbb 100644
--- a/csharp/src/ProtocolBuffers/Collections/MapField.cs
+++ b/csharp/src/ProtocolBuffers/Collections/MapField.cs
@@ -77,8 +77,7 @@ namespace Google.Protobuf.Collections
 
         public void Add(TKey key, TValue value)
         {
-            ThrowHelper.ThrowIfNull(key, "key");
-            this.CheckMutable();
+            // Validation of arguments happens in ContainsKey and the indexer
             if (ContainsKey(key))
             {
                 throw new ArgumentException("Key already exists in map", "key");
@@ -88,12 +87,14 @@ namespace Google.Protobuf.Collections
 
         public bool ContainsKey(TKey key)
         {
+            ThrowHelper.ThrowIfNull(key, "key");
             return map.ContainsKey(key);
         }
 
         public bool Remove(TKey key)
         {
             this.CheckMutable();
+            ThrowHelper.ThrowIfNull(key, "key");
             LinkedListNode<KeyValuePair<TKey, TValue>> node;
             if (map.TryGetValue(key, out node))
             {
@@ -126,6 +127,7 @@ namespace Google.Protobuf.Collections
         {
             get
             {
+                ThrowHelper.ThrowIfNull(key, "key");
                 TValue value;
                 if (TryGetValue(key, out value))
                 {
@@ -135,6 +137,11 @@ namespace Google.Protobuf.Collections
             }
             set
             {
+                ThrowHelper.ThrowIfNull(key, "key");
+                if (value == null && (typeof(TValue) == typeof(ByteString) || typeof(TValue) == typeof(string)))
+                {
+                    ThrowHelper.ThrowIfNull(value, "value");
+                }
                 this.CheckMutable();
                 LinkedListNode<KeyValuePair<TKey, TValue>> node;
                 var pair = new KeyValuePair<TKey, TValue>(key, value);
@@ -156,9 +163,10 @@ namespace Google.Protobuf.Collections
 
         public void Add(IDictionary<TKey, TValue> entries)
         {
+            ThrowHelper.ThrowIfNull(entries, "entries");
             foreach (var pair in entries)
             {
-                Add(pair);
+                Add(pair.Key, pair.Value);
             }
         }
 
@@ -172,9 +180,8 @@ namespace Google.Protobuf.Collections
             return GetEnumerator();
         }
 
-        public void Add(KeyValuePair<TKey, TValue> item)
+        void ICollection<KeyValuePair<TKey, TValue>>.Add(KeyValuePair<TKey, TValue> item)
         {
-            this.CheckMutable();
             Add(item.Key, item.Value);
         }
 
@@ -185,22 +192,37 @@ namespace Google.Protobuf.Collections
             map.Clear();
         }
 
-        public bool Contains(KeyValuePair<TKey, TValue> item)
+        bool ICollection<KeyValuePair<TKey, TValue>>.Contains(KeyValuePair<TKey, TValue> item)
         {
             TValue value;
             return TryGetValue(item.Key, out value)
                 && EqualityComparer<TValue>.Default.Equals(item.Value, value);
         }
 
-        public void CopyTo(KeyValuePair<TKey, TValue>[] array, int arrayIndex)
+        void ICollection<KeyValuePair<TKey, TValue>>.CopyTo(KeyValuePair<TKey, TValue>[] array, int arrayIndex)
         {
             list.CopyTo(array, arrayIndex);
         }
 
-        public bool Remove(KeyValuePair<TKey, TValue> item)
+        bool ICollection<KeyValuePair<TKey, TValue>>.Remove(KeyValuePair<TKey, TValue> item)
         {
             this.CheckMutable();
-            return Remove(item.Key);
+            if (item.Key == null)
+            {
+                throw new ArgumentException("Key is null", "item");
+            }
+            LinkedListNode<KeyValuePair<TKey, TValue>> node;
+            if (map.TryGetValue(item.Key, out node) &&
+                EqualityComparer<TValue>.Default.Equals(item.Value, node.Value.Value))
+            {
+                map.Remove(item.Key);
+                node.List.Remove(node);
+                return true;
+            }
+            else
+            {
+                return false;
+            }
         }
 
         public int Count { get { return list.Count; } }
@@ -239,7 +261,7 @@ namespace Google.Protobuf.Collections
         public override int GetHashCode()
         {
             var valueComparer = EqualityComparer<TValue>.Default;
-            int hash = 0;
+            int hash = 19;
             foreach (var pair in list)
             {
                 hash ^= pair.Key.GetHashCode() * 31 + valueComparer.GetHashCode(pair.Value);
@@ -277,14 +299,26 @@ namespace Google.Protobuf.Collections
             return true;
         }
 
+        /// <summary>
+        /// Adds entries to the map from the given stream.
+        /// </summary>
+        /// <remarks>
+        /// It is assumed that the stream is initially positioned after the tag specified by the codec.
+        /// This method will continue reading entries from the stream until the end is reached, or
+        /// a different tag is encountered.
+        /// </remarks>
+        /// <param name="input">Stream to read from</param>
+        /// <param name="codec">Codec describing how the key/value pairs are encoded</param>
         public void AddEntriesFrom(CodedInputStream input, Codec codec)            
         {
-            // TODO: Peek at the next tag and see if it's the same. If it is, we can reuse the entry object...
             var adapter = new Codec.MessageAdapter(codec);
-            adapter.Reset();
-            input.ReadMessage(adapter);
-            this[adapter.Key] = adapter.Value;
-        }
+            do
+            {
+                adapter.Reset();
+                input.ReadMessage(adapter);
+                this[adapter.Key] = adapter.Value;
+            } while (input.MaybeConsumeTag(codec.MapTag));
+        }        
 
         public void WriteTo(CodedOutputStream output, Codec codec)
         {