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?)

1 files changed, 42 insertions, 34 deletions

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;
                 }
             }
         }