Implemented Jan's suggestion of FieldCollection, replacing FieldAccessorCollection.

I think Jan was actually suggesting keeping both, but that feels redundant to me. The test diff is misleading here IMO, because I wouldn't expect real code using reflection to use several accessors one after another like this, unless it was within a loop. Evidence to the contrary would be welcome :)

This change also incidentally goes part way to fixing the issue of the JSON formatter not writing out the fields in field number order - with this change, it does except for oneofs, which we can fix in a follow-up change.

I haven't actually added a test with a message with fields deliberately out of order - I'm happy to do so though. It feels like it would make sense to be in google/src/protobuf, but it's not entirely clear what the rules of engagement are for adding new messages there. (unittest_proto3.proto?)

6 files changed, 128 insertions, 119 deletions

diff --git a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs b/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs
index f4128e08..5b6dc0fb 100644
--- a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs
+++ b/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs
@@ -103,15 +103,16 @@ namespace Google.Protobuf.Reflection
             Assert.AreEqual(UnittestProto3.Descriptor, nestedType.File);
             Assert.AreEqual(messageType, nestedType.ContainingType);
 
-            FieldDescriptor field = messageType.Fields[0];
+            FieldDescriptor field = messageType.Fields.InDeclarationOrder()[0];
             Assert.AreEqual("single_int32", field.Name);
             Assert.AreEqual(field, messageType.FindDescriptor<FieldDescriptor>("single_int32"));
             Assert.Null(messageType.FindDescriptor<FieldDescriptor>("no_such_field"));
             Assert.AreEqual(field, messageType.FindFieldByNumber(1));
             Assert.Null(messageType.FindFieldByNumber(571283));
-            for (int i = 0; i < messageType.Fields.Count; i++)
+            var fieldsInDeclarationOrder = messageType.Fields.InDeclarationOrder();
+            for (int i = 0; i < fieldsInDeclarationOrder.Count; i++)
             {
-                Assert.AreEqual(i, messageType.Fields[i].Index);
+                Assert.AreEqual(i, fieldsInDeclarationOrder[i].Index);
             }
 
             Assert.AreEqual(nestedType, messageType.NestedTypes[0]);
diff --git a/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs b/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs
index 5d6e777f..6e1d804e 100644
--- a/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs
+++ b/csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs
@@ -44,43 +44,43 @@ namespace Google.Protobuf.Reflection
         public void GetValue()
         {
             var message = SampleMessages.CreateFullTestAllTypes();
-            var fields = TestAllTypes.Descriptor.FieldAccessors;
-            Assert.AreEqual(message.SingleBool, fields[TestAllTypes.SingleBoolFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleBytes, fields[TestAllTypes.SingleBytesFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleDouble, fields[TestAllTypes.SingleDoubleFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleFixed32, fields[TestAllTypes.SingleFixed32FieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleFixed64, fields[TestAllTypes.SingleFixed64FieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleFloat, fields[TestAllTypes.SingleFloatFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleForeignEnum, fields[TestAllTypes.SingleForeignEnumFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleForeignMessage, fields[TestAllTypes.SingleForeignMessageFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleImportEnum, fields[TestAllTypes.SingleImportEnumFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleImportMessage, fields[TestAllTypes.SingleImportMessageFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleInt32, fields[TestAllTypes.SingleInt32FieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleInt64, fields[TestAllTypes.SingleInt64FieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleNestedEnum, fields[TestAllTypes.SingleNestedEnumFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleNestedMessage, fields[TestAllTypes.SingleNestedMessageFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SinglePublicImportMessage, fields[TestAllTypes.SinglePublicImportMessageFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleSint32, fields[TestAllTypes.SingleSint32FieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleSint64, fields[TestAllTypes.SingleSint64FieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleString, fields[TestAllTypes.SingleStringFieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleSfixed32, fields[TestAllTypes.SingleSfixed32FieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleSfixed64, fields[TestAllTypes.SingleSfixed64FieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleUint32, fields[TestAllTypes.SingleUint32FieldNumber].GetValue(message));
-            Assert.AreEqual(message.SingleUint64, fields[TestAllTypes.SingleUint64FieldNumber].GetValue(message));
-            Assert.AreEqual(message.OneofBytes, fields[TestAllTypes.OneofBytesFieldNumber].GetValue(message));
-            Assert.AreEqual(message.OneofString, fields[TestAllTypes.OneofStringFieldNumber].GetValue(message));
-            Assert.AreEqual(message.OneofNestedMessage, fields[TestAllTypes.OneofNestedMessageFieldNumber].GetValue(message));
-            Assert.AreEqual(message.OneofUint32, fields[TestAllTypes.OneofUint32FieldNumber].GetValue(message));
+            var fields = TestAllTypes.Descriptor.Fields;
+            Assert.AreEqual(message.SingleBool, fields[TestAllTypes.SingleBoolFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleBytes, fields[TestAllTypes.SingleBytesFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleDouble, fields[TestAllTypes.SingleDoubleFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleFixed32, fields[TestAllTypes.SingleFixed32FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleFixed64, fields[TestAllTypes.SingleFixed64FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleFloat, fields[TestAllTypes.SingleFloatFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleForeignEnum, fields[TestAllTypes.SingleForeignEnumFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleForeignMessage, fields[TestAllTypes.SingleForeignMessageFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleImportEnum, fields[TestAllTypes.SingleImportEnumFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleImportMessage, fields[TestAllTypes.SingleImportMessageFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleInt32, fields[TestAllTypes.SingleInt32FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleInt64, fields[TestAllTypes.SingleInt64FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleNestedEnum, fields[TestAllTypes.SingleNestedEnumFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleNestedMessage, fields[TestAllTypes.SingleNestedMessageFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SinglePublicImportMessage, fields[TestAllTypes.SinglePublicImportMessageFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleSint32, fields[TestAllTypes.SingleSint32FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleSint64, fields[TestAllTypes.SingleSint64FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleString, fields[TestAllTypes.SingleStringFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleSfixed32, fields[TestAllTypes.SingleSfixed32FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleSfixed64, fields[TestAllTypes.SingleSfixed64FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleUint32, fields[TestAllTypes.SingleUint32FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.SingleUint64, fields[TestAllTypes.SingleUint64FieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.OneofBytes, fields[TestAllTypes.OneofBytesFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.OneofString, fields[TestAllTypes.OneofStringFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.OneofNestedMessage, fields[TestAllTypes.OneofNestedMessageFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(message.OneofUint32, fields[TestAllTypes.OneofUint32FieldNumber].Accessor.GetValue(message));
 
             // Just one example for repeated fields - they're all just returning the list
-            var list = (IList) fields[TestAllTypes.RepeatedInt32FieldNumber].GetValue(message);
+            var list = (IList) fields[TestAllTypes.RepeatedInt32FieldNumber].Accessor.GetValue(message);
             Assert.AreEqual(message.RepeatedInt32, list);
             Assert.AreEqual(message.RepeatedInt32[0], list[0]); // Just in case there was any doubt...
 
             // Just a single map field, for the same reason
             var mapMessage = new TestMap { MapStringString = { { "key1", "value1" }, { "key2", "value2" } } };
-            fields = TestMap.Descriptor.FieldAccessors;
-            var dictionary = (IDictionary) fields[TestMap.MapStringStringFieldNumber].GetValue(mapMessage);
+            fields = TestMap.Descriptor.Fields;
+            var dictionary = (IDictionary) fields[TestMap.MapStringStringFieldNumber].Accessor.GetValue(mapMessage);
             Assert.AreEqual(mapMessage.MapStringString, dictionary);
             Assert.AreEqual("value1", dictionary["key1"]);
         }
@@ -89,14 +89,14 @@ namespace Google.Protobuf.Reflection
         public void Clear()
         {
             var message = SampleMessages.CreateFullTestAllTypes();
-            var fields = TestAllTypes.Descriptor.FieldAccessors;
-            fields[TestAllTypes.SingleBoolFieldNumber].Clear(message);
-            fields[TestAllTypes.SingleInt32FieldNumber].Clear(message);
-            fields[TestAllTypes.SingleStringFieldNumber].Clear(message);
-            fields[TestAllTypes.SingleBytesFieldNumber].Clear(message);
-            fields[TestAllTypes.SingleForeignEnumFieldNumber].Clear(message);
-            fields[TestAllTypes.SingleForeignMessageFieldNumber].Clear(message);
-            fields[TestAllTypes.RepeatedDoubleFieldNumber].Clear(message);
+            var fields = TestAllTypes.Descriptor.Fields;
+            fields[TestAllTypes.SingleBoolFieldNumber].Accessor.Clear(message);
+            fields[TestAllTypes.SingleInt32FieldNumber].Accessor.Clear(message);
+            fields[TestAllTypes.SingleStringFieldNumber].Accessor.Clear(message);
+            fields[TestAllTypes.SingleBytesFieldNumber].Accessor.Clear(message);
+            fields[TestAllTypes.SingleForeignEnumFieldNumber].Accessor.Clear(message);
+            fields[TestAllTypes.SingleForeignMessageFieldNumber].Accessor.Clear(message);
+            fields[TestAllTypes.RepeatedDoubleFieldNumber].Accessor.Clear(message);
 
             var expected = new TestAllTypes(SampleMessages.CreateFullTestAllTypes())
             {
@@ -113,8 +113,8 @@ namespace Google.Protobuf.Reflection
 
             // Separately, maps.
             var mapMessage = new TestMap { MapStringString = { { "key1", "value1" }, { "key2", "value2" } } };
-            fields = TestMap.Descriptor.FieldAccessors;
-            fields[TestMap.MapStringStringFieldNumber].Clear(mapMessage);
+            fields = TestMap.Descriptor.Fields;
+            fields[TestMap.MapStringStringFieldNumber].Accessor.Clear(mapMessage);
             Assert.AreEqual(0, mapMessage.MapStringString.Count);
         }
 
@@ -123,14 +123,14 @@ namespace Google.Protobuf.Reflection
         {
             // Just a sample (primitives, messages, enums, strings, byte strings)
             var message = SampleMessages.CreateFullTestAllTypes();
-            var fields = TestAllTypes.Descriptor.FieldAccessors;
-            fields[TestAllTypes.SingleBoolFieldNumber].SetValue(message, false);
-            fields[TestAllTypes.SingleInt32FieldNumber].SetValue(message, 500);
-            fields[TestAllTypes.SingleStringFieldNumber].SetValue(message, "It's a string");
-            fields[TestAllTypes.SingleBytesFieldNumber].SetValue(message, ByteString.CopyFrom(99, 98, 97));
-            fields[TestAllTypes.SingleForeignEnumFieldNumber].SetValue(message, ForeignEnum.FOREIGN_FOO);
-            fields[TestAllTypes.SingleForeignMessageFieldNumber].SetValue(message, new ForeignMessage { C = 12345 });
-            fields[TestAllTypes.SingleDoubleFieldNumber].SetValue(message, 20150701.5);
+            var fields = TestAllTypes.Descriptor.Fields;
+            fields[TestAllTypes.SingleBoolFieldNumber].Accessor.SetValue(message, false);
+            fields[TestAllTypes.SingleInt32FieldNumber].Accessor.SetValue(message, 500);
+            fields[TestAllTypes.SingleStringFieldNumber].Accessor.SetValue(message, "It's a string");
+            fields[TestAllTypes.SingleBytesFieldNumber].Accessor.SetValue(message, ByteString.CopyFrom(99, 98, 97));
+            fields[TestAllTypes.SingleForeignEnumFieldNumber].Accessor.SetValue(message, ForeignEnum.FOREIGN_FOO);
+            fields[TestAllTypes.SingleForeignMessageFieldNumber].Accessor.SetValue(message, new ForeignMessage { C = 12345 });
+            fields[TestAllTypes.SingleDoubleFieldNumber].Accessor.SetValue(message, 20150701.5);
 
             var expected = new TestAllTypes(SampleMessages.CreateFullTestAllTypes())
             {
@@ -150,32 +150,32 @@ namespace Google.Protobuf.Reflection
         public void SetValue_SingleFields_WrongType()
         {
             IMessage message = SampleMessages.CreateFullTestAllTypes();
-            var fields = message.Descriptor.FieldAccessors;
-            Assert.Throws<InvalidCastException>(() => fields[TestAllTypes.SingleBoolFieldNumber].SetValue(message, "This isn't a bool"));
+            var fields = message.Descriptor.Fields;
+            Assert.Throws<InvalidCastException>(() => fields[TestAllTypes.SingleBoolFieldNumber].Accessor.SetValue(message, "This isn't a bool"));
         }
 
         [Test]
         public void SetValue_MapFields()
         {
             IMessage message = new TestMap();
-            var fields = message.Descriptor.FieldAccessors;
-            Assert.Throws<InvalidOperationException>(() => fields[TestMap.MapStringStringFieldNumber].SetValue(message, new Dictionary<string, string>()));
+            var fields = message.Descriptor.Fields;
+            Assert.Throws<InvalidOperationException>(() => fields[TestMap.MapStringStringFieldNumber].Accessor.SetValue(message, new Dictionary<string, string>()));
         }
 
         [Test]
         public void SetValue_RepeatedFields()
         {
             IMessage message = SampleMessages.CreateFullTestAllTypes();
-            var fields = message.Descriptor.FieldAccessors;
-            Assert.Throws<InvalidOperationException>(() => fields[TestAllTypes.RepeatedDoubleFieldNumber].SetValue(message, new double[10]));
+            var fields = message.Descriptor.Fields;
+            Assert.Throws<InvalidOperationException>(() => fields[TestAllTypes.RepeatedDoubleFieldNumber].Accessor.SetValue(message, new double[10]));
         }
 
         [Test]
         public void GetValue_IncorrectType()
         {
             IMessage message = SampleMessages.CreateFullTestAllTypes();
-            var fields = message.Descriptor.FieldAccessors;
-            Assert.Throws<InvalidCastException>(() => fields[TestAllTypes.SingleBoolFieldNumber].GetValue(new TestMap()));
+            var fields = message.Descriptor.Fields;
+            Assert.Throws<InvalidCastException>(() => fields[TestAllTypes.SingleBoolFieldNumber].Accessor.GetValue(new TestMap()));
         }
 
         [Test]
@@ -189,30 +189,30 @@ namespace Google.Protobuf.Reflection
             Assert.IsNull(oneof.Accessor.GetCaseFieldDescriptor(message));
 
             message.OneofString = "foo";
-            Assert.AreSame(descriptor.FieldAccessors[TestAllTypes.OneofStringFieldNumber].Descriptor, oneof.Accessor.GetCaseFieldDescriptor(message));
+            Assert.AreSame(descriptor.Fields[TestAllTypes.OneofStringFieldNumber], oneof.Accessor.GetCaseFieldDescriptor(message));
 
             message.OneofUint32 = 10;
-            Assert.AreSame(descriptor.FieldAccessors[TestAllTypes.OneofUint32FieldNumber].Descriptor, oneof.Accessor.GetCaseFieldDescriptor(message));
+            Assert.AreSame(descriptor.Fields[TestAllTypes.OneofUint32FieldNumber], oneof.Accessor.GetCaseFieldDescriptor(message));
 
             oneof.Accessor.Clear(message);
             Assert.AreEqual(TestAllTypes.OneofFieldOneofCase.None, message.OneofFieldCase);
         }
 
         [Test]
-        public void FieldAccessor_ByName()
+        public void FieldDescriptor_ByName()
         {
             var descriptor = TestAllTypes.Descriptor;
             Assert.AreSame(
-                descriptor.FieldAccessors[TestAllTypes.SingleBoolFieldNumber],
-                descriptor.FieldAccessors["single_bool"]);
+                descriptor.Fields[TestAllTypes.SingleBoolFieldNumber],
+                descriptor.Fields["single_bool"]);
         }
 
         [Test]
-        public void FieldAccessor_NotFound()
+        public void FieldDescriptor_NotFound()
         {
             var descriptor = TestAllTypes.Descriptor;
-            Assert.Throws<KeyNotFoundException>(() => descriptor.FieldAccessors[999999].ToString());
-            Assert.Throws<KeyNotFoundException>(() => descriptor.FieldAccessors["not found"].ToString());
+            Assert.Throws<KeyNotFoundException>(() => descriptor.Fields[999999].ToString());
+            Assert.Throws<KeyNotFoundException>(() => descriptor.Fields["not found"].ToString());
         }
     }
 }
diff --git a/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs b/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
index a8288483..670bc5f8 100644
--- a/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
+++ b/csharp/src/Google.Protobuf.Test/WellKnownTypes/WrappersTest.cs
@@ -192,23 +192,23 @@ namespace Google.Protobuf.WellKnownTypes
                 Uint32Field = 3,
                 Uint64Field = 4
             };
-            var fields = TestWellKnownTypes.Descriptor.FieldAccessors;
+            var fields = TestWellKnownTypes.Descriptor.Fields;
 
-            Assert.AreEqual("x", fields[TestWellKnownTypes.StringFieldFieldNumber].GetValue(message));
-            Assert.AreEqual(ByteString.CopyFrom(1, 2, 3), fields[TestWellKnownTypes.BytesFieldFieldNumber].GetValue(message));
-            Assert.AreEqual(true, fields[TestWellKnownTypes.BoolFieldFieldNumber].GetValue(message));
-            Assert.AreEqual(12.5f, fields[TestWellKnownTypes.FloatFieldFieldNumber].GetValue(message));
-            Assert.AreEqual(12.25d, fields[TestWellKnownTypes.DoubleFieldFieldNumber].GetValue(message));
-            Assert.AreEqual(1, fields[TestWellKnownTypes.Int32FieldFieldNumber].GetValue(message));
-            Assert.AreEqual(2L, fields[TestWellKnownTypes.Int64FieldFieldNumber].GetValue(message));
-            Assert.AreEqual(3U, fields[TestWellKnownTypes.Uint32FieldFieldNumber].GetValue(message));
-            Assert.AreEqual(4UL, fields[TestWellKnownTypes.Uint64FieldFieldNumber].GetValue(message));
+            Assert.AreEqual("x", fields[TestWellKnownTypes.StringFieldFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(ByteString.CopyFrom(1, 2, 3), fields[TestWellKnownTypes.BytesFieldFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(true, fields[TestWellKnownTypes.BoolFieldFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(12.5f, fields[TestWellKnownTypes.FloatFieldFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(12.25d, fields[TestWellKnownTypes.DoubleFieldFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(1, fields[TestWellKnownTypes.Int32FieldFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(2L, fields[TestWellKnownTypes.Int64FieldFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(3U, fields[TestWellKnownTypes.Uint32FieldFieldNumber].Accessor.GetValue(message));
+            Assert.AreEqual(4UL, fields[TestWellKnownTypes.Uint64FieldFieldNumber].Accessor.GetValue(message));
 
             // And a couple of null fields...
             message.StringField = null;
             message.FloatField = null;
-            Assert.IsNull(fields[TestWellKnownTypes.StringFieldFieldNumber].GetValue(message));
-            Assert.IsNull(fields[TestWellKnownTypes.FloatFieldFieldNumber].GetValue(message));
+            Assert.IsNull(fields[TestWellKnownTypes.StringFieldFieldNumber].Accessor.GetValue(message));
+            Assert.IsNull(fields[TestWellKnownTypes.FloatFieldFieldNumber].Accessor.GetValue(message));
         }
 
         [Test]
@@ -216,8 +216,8 @@ namespace Google.Protobuf.WellKnownTypes
         {
             // Just a single example... note that we can't have a null value here
             var message = new RepeatedWellKnownTypes { Int32Field = { 1, 2 } };
-            var fields = RepeatedWellKnownTypes.Descriptor.FieldAccessors;
-            var list = (IList) fields[RepeatedWellKnownTypes.Int32FieldFieldNumber].GetValue(message);
+            var fields = RepeatedWellKnownTypes.Descriptor.Fields;
+            var list = (IList) fields[RepeatedWellKnownTypes.Int32FieldFieldNumber].Accessor.GetValue(message);
             CollectionAssert.AreEqual(new[] { 1, 2 }, list);
         }
 
@@ -226,8 +226,8 @@ namespace Google.Protobuf.WellKnownTypes
         {
             // Just a single example... note that we can't have a null value here
             var message = new MapWellKnownTypes { Int32Field = { { 1, 2 }, { 3, null } } };
-            var fields = MapWellKnownTypes.Descriptor.FieldAccessors;
-            var dictionary = (IDictionary) fields[MapWellKnownTypes.Int32FieldFieldNumber].GetValue(message);
+            var fields = MapWellKnownTypes.Descriptor.Fields;
+            var dictionary = (IDictionary) fields[MapWellKnownTypes.Int32FieldFieldNumber].Accessor.GetValue(message);
             Assert.AreEqual(2, dictionary[1]);
             Assert.IsNull(dictionary[3]);
             Assert.IsTrue(dictionary.Contains(3));
diff --git a/csharp/src/Google.Protobuf/JsonFormatter.cs b/csharp/src/Google.Protobuf/JsonFormatter.cs
index 7f13e33e..f624b090 100644
--- a/csharp/src/Google.Protobuf/JsonFormatter.cs
+++ b/csharp/src/Google.Protobuf/JsonFormatter.cs
@@ -140,7 +140,7 @@ namespace Google.Protobuf
             var fields = message.Descriptor.Fields;
             bool first = true;
             // First non-oneof fields
-            foreach (var field in fields)
+            foreach (var field in fields.InFieldNumberOrder())
             {
                 var accessor = field.Accessor;
                 // Oneofs are written later
diff --git a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs
index aa585c77..1250774d 100644
--- a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs
+++ b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs
@@ -33,6 +33,7 @@
 using Google.Protobuf.Collections;
 using System;
 using System.Collections.Generic;
+using System.Collections.ObjectModel;
 using System.Linq;
 
 namespace Google.Protobuf.Reflection
@@ -60,8 +61,9 @@ namespace Google.Protobuf.Reflection
         private readonly MessageDescriptor containingType;
         private readonly IList<MessageDescriptor> nestedTypes;
         private readonly IList<EnumDescriptor> enumTypes;
-        private readonly IList<FieldDescriptor> fields;
-        private readonly FieldAccessorCollection fieldAccessors;
+        private readonly IList<FieldDescriptor> fieldsInDeclarationOrder;
+        private readonly IList<FieldDescriptor> fieldsInNumberOrder;
+        private readonly FieldCollection fields;
         private readonly IList<OneofDescriptor> oneofs;
         // CLR representation of the type described by this descriptor, if any.
         private readonly Type generatedType;
@@ -89,12 +91,13 @@ namespace Google.Protobuf.Reflection
                 (type, index) =>
                 new EnumDescriptor(type, file, this, index, generatedCodeInfo == null ? null : generatedCodeInfo.NestedEnums[index]));
 
-            fields = DescriptorUtil.ConvertAndMakeReadOnly(
+            fieldsInDeclarationOrder = DescriptorUtil.ConvertAndMakeReadOnly(
                 proto.Field,
                 (field, index) =>
                 new FieldDescriptor(field, file, this, index, generatedCodeInfo == null ? null : generatedCodeInfo.PropertyNames[index]));
+            fieldsInNumberOrder = new ReadOnlyCollection<FieldDescriptor>(fieldsInDeclarationOrder.OrderBy(field => field.FieldNumber).ToArray());
             file.DescriptorPool.AddSymbol(this);
-            fieldAccessors = new FieldAccessorCollection(this);
+            fields = new FieldCollection(this);
         }
                 
         /// <summary>
@@ -136,28 +139,15 @@ namespace Google.Protobuf.Reflection
             get { return containingType; }
         }
 
-        // TODO: It's confusing that FieldAccessors[x] doesn't retrieve the accessor
-        // for Fields[x]. We should think about this further... how often does a user really
-        // want the fields in declaration order?
-
         /// <value>
-        /// An unmodifiable list of this message type's fields, in the declaration order
-        /// within the .proto file.
+        /// A collection of fields, which can be retrieved by name or field number.
         /// </value>
-        public IList<FieldDescriptor> Fields
+        public FieldCollection Fields
         {
             get { return fields; }
         }
 
         /// <value>
-        /// A collection of accessors, which can be retrieved by name or field number.
-        /// </value>
-        public FieldAccessorCollection FieldAccessors
-        {
-            get { return fieldAccessors; }
-        }
-
-        /// <value>
         /// An unmodifiable list of this message type's nested types.
         /// </value>
         public IList<MessageDescriptor> NestedTypes
@@ -220,7 +210,7 @@ namespace Google.Protobuf.Reflection
                 message.CrossLink();
             }
 
-            foreach (FieldDescriptor field in fields)
+            foreach (FieldDescriptor field in fieldsInDeclarationOrder)
             {
                 field.CrossLink();
             }
@@ -234,24 +224,43 @@ namespace Google.Protobuf.Reflection
         /// <summary>
         /// A collection to simplify retrieving the field accessor for a particular field.
         /// </summary>
-        public sealed class FieldAccessorCollection
+        public sealed class FieldCollection
         {
             private readonly MessageDescriptor messageDescriptor;
 
-            internal FieldAccessorCollection(MessageDescriptor messageDescriptor)
+            internal FieldCollection(MessageDescriptor messageDescriptor)
             {
                 this.messageDescriptor = messageDescriptor;
             }
 
+            /// <value>
+            /// Returns the fields in the message as an immutable list, in the order in which they
+            /// are declared in the source .proto file.
+            /// </value>
+            public IList<FieldDescriptor> InDeclarationOrder()
+            {
+                return messageDescriptor.fieldsInDeclarationOrder;
+            }
+
+            /// <value>
+            /// Returns the fields in the message as an immutable list, in ascending field number
+            /// order. Field numbers need not be contiguous, so there is no direct mapping from the
+            /// index in the list to the field number; to retrieve a field by field number, it is better
+            /// to use the <see cref="FieldCollection"/> indexer.
+            /// </value>
+            public IList<FieldDescriptor> InFieldNumberOrder()
+            {
+                return messageDescriptor.fieldsInDeclarationOrder;
+            }
+
             /// <summary>
-            /// Retrieves the accessor for the field with the given number.
+            /// Retrieves the descriptor for the field with the given number.
             /// </summary>
-            /// <param name="number">Number of the field to retrieve the accessor for</param>
-            /// <returns>The accessor for the given field, or null if reflective field access is
-            /// not supported for the field.</returns>
+            /// <param name="number">Number of the field to retrieve the descriptor for</param>
+            /// <returns>The accessor for the given field</returns>
             /// <exception cref="KeyNotFoundException">The message descriptor does not contain a field
             /// with the given number</exception>
-            public IFieldAccessor this[int number]
+            public FieldDescriptor this[int number]
             {
                 get
                 {
@@ -260,19 +269,18 @@ namespace Google.Protobuf.Reflection
                     {
                         throw new KeyNotFoundException("No such field number");
                     }
-                    return fieldDescriptor.Accessor;
+                    return fieldDescriptor;
                 }
             }
 
             /// <summary>
-            /// Retrieves the accessor for the field with the given name.
+            /// Retrieves the descriptor for the field with the given name.
             /// </summary>
-            /// <param name="number">Number of the field to retrieve the accessor for</param>
-            /// <returns>The accessor for the given field, or null if reflective field access is
-            /// not supported for the field.</returns>
+            /// <param name="number">Number of the field to retrieve the descriptor for</param>
+            /// <returns>The descriptor for the given field</returns>
             /// <exception cref="KeyNotFoundException">The message descriptor does not contain a field
             /// with the given name</exception>
-            public IFieldAccessor this[string name]
+            public FieldDescriptor this[string name]
             {
                 get
                 {
@@ -281,7 +289,7 @@ namespace Google.Protobuf.Reflection
                     {
                         throw new KeyNotFoundException("No such field name");
                     }
-                    return fieldDescriptor.Accessor;
+                    return fieldDescriptor;
                 }
             }
         }
diff --git a/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs
index a79d9de4..cd4c5534 100644
--- a/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs
+++ b/csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs
@@ -70,7 +70,7 @@ namespace Google.Protobuf.Reflection
         internal void CrossLink()
         {
             List<FieldDescriptor> fieldCollection = new List<FieldDescriptor>();
-            foreach (var field in ContainingType.Fields)
+            foreach (var field in ContainingType.Fields.InDeclarationOrder())
             {
                 if (field.ContainingOneof == this)
                 {