Browse Source

Implement reflection properly for fields.

- FieldAccessorTable is now non-generic
- We don't have a static field per message type in the umbrella class. (Message descriptors are accessed via the file descriptor.)
- Removed the "descriptor assigner" complication from the descriptor fixup; without extensions, we don't need it
- MapField implements IDictionary (more tests would be good...)
- RepeatedField implements IList (more tests would be good)
- Use expression trees to build accessors. (Will need to test this on various platforms... probably need a fallback strategy just using reflection directly.)
- Added FieldDescriptor.IsMap
- Added tests for reflection with generated messages

Changes to generated code coming in next commit.
Jon Skeet 10 years ago
parent
commit
78ea98f56f

+ 140 - 1
csharp/src/ProtocolBuffers.Test/GeneratedMessageTest.cs

@@ -29,11 +29,13 @@
 // (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.IO;
 using Google.Protobuf.TestProtos;
 using NUnit.Framework;
+using System.Collections;
+using System.Collections.Generic;
 
 namespace Google.Protobuf
 {
@@ -595,5 +597,142 @@ namespace Google.Protobuf
             Assert.AreEqual(message, message2);
             Assert.AreEqual(TestAllTypes.OneofFieldOneofCase.OneofUint32, message2.OneofFieldCase);
         }
+
+        // TODO: Consider moving these tests to a separate reflection test - although they do require generated messages.
+
+        [Test]
+        public void Reflection_GetValue()
+        {
+            var message = GetSampleMessage();
+            var fields = message.Fields;
+            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));
+
+            // Just one example for repeated fields - they're all just returning the list
+            var list = (IList)fields[TestAllTypes.RepeatedInt32FieldNumber].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" } } };
+            var dictionary = (IDictionary)mapMessage.Fields[TestMap.MapStringStringFieldNumber].GetValue(mapMessage);
+            Assert.AreEqual(mapMessage.MapStringString, dictionary);
+            Assert.AreEqual("value1", dictionary["key1"]);
+        }
+
+        [Test]
+        public void Reflection_Clear()
+        {
+            var message = GetSampleMessage();
+            var fields = message.Fields;
+            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 expected = new TestAllTypes(GetSampleMessage())
+            {
+                SingleBool = false,
+                SingleInt32 = 0,
+                SingleString = "",
+                SingleBytes = ByteString.Empty,
+                SingleForeignEnum = 0,
+                SingleForeignMessage = null,
+            };
+            expected.RepeatedDouble.Clear();
+
+            Assert.AreEqual(expected, message);
+
+            // Separately, maps.
+            var mapMessage = new TestMap { MapStringString = { { "key1", "value1" }, { "key2", "value2" } } };
+            mapMessage.Fields[TestMap.MapStringStringFieldNumber].Clear(mapMessage);
+            Assert.AreEqual(0, mapMessage.MapStringString.Count);
+        }
+
+        [Test]
+        public void Reflection_SetValue_SingleFields()
+        {
+            // Just a sample (primitives, messages, enums, strings, byte strings)
+            var message = GetSampleMessage();
+            var fields = message.Fields;
+            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 expected = new TestAllTypes(GetSampleMessage())
+            {
+                SingleBool = false,
+                SingleInt32 = 500,
+                SingleString = "It's a string",
+                SingleBytes = ByteString.CopyFrom(99, 98, 97),
+                SingleForeignEnum = ForeignEnum.FOREIGN_FOO,
+                SingleForeignMessage = new ForeignMessage { C = 12345 },
+                SingleDouble = 20150701.5
+            };
+
+            Assert.AreEqual(expected, message);
+        }
+
+        [Test]
+        public void Reflection_SetValue_SingleFields_WrongType()
+        {
+            var message = GetSampleMessage();
+            var fields = message.Fields;
+            Assert.Throws<InvalidCastException>(() => fields[TestAllTypes.SingleBoolFieldNumber].SetValue(message, "This isn't a bool"));
+        }
+
+        [Test]
+        public void Reflection_SetValue_MapFields()
+        {
+            var message = new TestMap();
+            var fields = message.Fields;
+            Assert.Throws<InvalidOperationException>(() => fields[TestMap.MapStringStringFieldNumber].SetValue(message, new Dictionary<string, string>()));
+        }
+
+        [Test]
+        public void Reflection_SetValue_RepeatedFields()
+        {
+            var message = GetSampleMessage();
+            var fields = message.Fields;
+            Assert.Throws<InvalidOperationException>(() => fields[TestAllTypes.RepeatedDoubleFieldNumber].SetValue(message, new double[10]));
+        }
+
+        [Test]
+        public void Reflection_GetValue_IncorrectType()
+        {
+            var message = GetSampleMessage();
+            Assert.Throws<InvalidCastException>(() => message.Fields[TestAllTypes.SingleBoolFieldNumber].GetValue(new TestMap()));
+        }
     }
 }

+ 65 - 4
csharp/src/ProtocolBuffers/Collections/MapField.cs

@@ -48,7 +48,7 @@ namespace Google.Protobuf.Collections
     /// </remarks>
     /// <typeparam name="TKey">Key type in the map. Must be a type supported by Protocol Buffer map keys.</typeparam>
     /// <typeparam name="TValue">Value type in the map. Must be a type supported by Protocol Buffers.</typeparam>
-    public sealed class MapField<TKey, TValue> : IDeepCloneable<MapField<TKey, TValue>>, IFreezable, IDictionary<TKey, TValue>, IEquatable<MapField<TKey, TValue>>
+    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 frozen;
@@ -64,7 +64,7 @@ namespace Google.Protobuf.Collections
             {
                 foreach (var pair in list)
                 {
-                    clone.Add(pair.Key, pair.Value == null ? pair.Value : ((IDeepCloneable<TValue>) pair.Value).Clone());
+                    clone.Add(pair.Key, pair.Value == null ? pair.Value : ((IDeepCloneable<TValue>)pair.Value).Clone());
                 }
             }
             else
@@ -309,7 +309,7 @@ namespace Google.Protobuf.Collections
         /// </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)            
+        public void AddEntriesFrom(CodedInputStream input, Codec codec)
         {
             var adapter = new Codec.MessageAdapter(codec);
             do
@@ -318,7 +318,7 @@ namespace Google.Protobuf.Collections
                 input.ReadMessage(adapter);
                 this[adapter.Key] = adapter.Value;
             } while (input.MaybeConsumeTag(codec.MapTag));
-        }        
+        }
 
         public void WriteTo(CodedOutputStream output, Codec codec)
         {
@@ -350,6 +350,67 @@ namespace Google.Protobuf.Collections
             return size;
         }
 
+        #region IDictionary explicit interface implementation
+        void IDictionary.Add(object key, object value)
+        {
+            Add((TKey)key, (TValue)value);
+        }
+
+        bool IDictionary.Contains(object key)
+        {
+            if (!(key is TKey))
+            {
+                return false;
+            }
+            return ContainsKey((TKey)key);
+        }
+
+        IDictionaryEnumerator IDictionary.GetEnumerator()
+        {
+            throw new NotImplementedException();
+        }
+
+        void IDictionary.Remove(object key)
+        {
+            if (!(key is TKey))
+            {
+                return;
+            }
+            Remove((TKey)key);
+        }
+
+        void ICollection.CopyTo(Array array, int index)
+        {
+            throw new NotImplementedException();
+        }
+
+        bool IDictionary.IsFixedSize { get { return IsFrozen; } }
+
+        ICollection IDictionary.Keys { get { return (ICollection)Keys; } }
+
+        ICollection IDictionary.Values { get { return (ICollection)Values; } }
+
+        bool ICollection.IsSynchronized { get { return false; } }
+
+        object ICollection.SyncRoot { get { return null; } }
+
+        object IDictionary.this[object key]
+        {
+            get
+            {
+                if (!(key is TKey))
+                {
+                    return null;
+                }
+                TValue value;
+                TryGetValue((TKey)key, out value);
+                return value;
+            }
+
+            set { this[(TKey)key] = (TValue)value; }
+        }
+        #endregion
+
         /// <summary>
         /// A codec for a specific map field. This contains all the information required to encoded and
         /// decode the nested messages.

+ 61 - 2
csharp/src/ProtocolBuffers/Collections/RepeatedField.cs

@@ -41,7 +41,7 @@ namespace Google.Protobuf.Collections
     /// restrictions (no null values) and capabilities (deep cloning and freezing).
     /// </summary>
     /// <typeparam name="T">The element type of the repeated field.</typeparam>
-    public sealed class RepeatedField<T> : IList<T>, IDeepCloneable<RepeatedField<T>>, IEquatable<RepeatedField<T>>, IFreezable
+    public sealed class RepeatedField<T> : IList<T>, IList, IDeepCloneable<RepeatedField<T>>, IEquatable<RepeatedField<T>>, IFreezable
     {
         private static readonly T[] EmptyArray = new T[0];
 
@@ -415,7 +415,66 @@ namespace Google.Protobuf.Collections
                 array[index] = value;
             }
         }
-        
+
+        #region Explicit interface implementation for IList and ICollection.
+        bool IList.IsFixedSize { get { return IsFrozen; } }
+
+        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);
+        }
+
+        bool ICollection.IsSynchronized { get { return false; } }
+
+        object ICollection.SyncRoot { get { return null; } }
+
+        object IList.this[int index]
+        {
+            get { return this[index]; }
+            set { this[index] = (T)value; }
+        }
+
+        int IList.Add(object value)
+        {
+            Add((T) value);
+            return count - 1;
+        }
+
+        bool IList.Contains(object value)
+        {
+            return (value is T && Contains((T)value));
+        }
+
+        int IList.IndexOf(object value)
+        {
+            if (!(value is T))
+            {
+                return -1;
+            }
+            return IndexOf((T)value);
+        }
+
+        void IList.Insert(int index, object value)
+        {
+            Insert(index, (T) value);
+        }
+
+        void IList.Remove(object value)
+        {
+            if (!(value is T))
+            {
+                return;
+            }
+            Remove((T)value);
+        }
+        #endregion
+
         public struct Enumerator : IEnumerator<T>
         {
             private int index;

+ 5 - 0
csharp/src/ProtocolBuffers/Descriptors/FieldDescriptor.cs

@@ -131,6 +131,11 @@ namespace Google.Protobuf.Descriptors
             get { return Proto.Label == FieldDescriptorProto.Types.Label.LABEL_REPEATED; }
         }
 
+        public bool IsMap
+        {
+            get { return fieldType == FieldType.Message && messageType.Options != null && messageType.Options.MapEntry; }
+        }
+
         public bool IsPacked
         {
             get { return Proto.Options.Packed; }

+ 2 - 31
csharp/src/ProtocolBuffers/Descriptors/FileDescriptor.cs

@@ -336,33 +336,8 @@ namespace Google.Protobuf.Descriptors
             }
         }
 
-        /// <summary>
-        /// This method is to be called by generated code only.  It is equivalent
-        /// to BuildFrom except that the FileDescriptorProto is encoded in
-        /// protocol buffer wire format. This overload is maintained for backward
-        /// compatibility with source code generated before the custom options were available
-        /// (and working).
-        /// </summary>
-        public static FileDescriptor InternalBuildGeneratedFileFrom(byte[] descriptorData, FileDescriptor[] dependencies)
-        {
-            return InternalBuildGeneratedFileFrom(descriptorData, dependencies, x => { });
-        }
-
-        /// <summary>
-        /// This delegate should be used by generated code only. When calling
-        /// FileDescriptor.InternalBuildGeneratedFileFrom, the caller can provide
-        /// a callback which assigns the global variables defined in the generated code
-        /// which point at parts of the FileDescriptor. The callback returns an
-        /// Extension Registry which contains any extensions which might be used in
-        /// the descriptor - that is, extensions of the various "Options" messages defined
-        /// in descriptor.proto. The callback may also return null to indicate that
-        /// no extensions are used in the descriptor.
-        /// </summary>
-        public delegate void InternalDescriptorAssigner(FileDescriptor descriptor);
-
         public static FileDescriptor InternalBuildGeneratedFileFrom(byte[] descriptorData,
-                                                                    FileDescriptor[] dependencies,
-                                                                    InternalDescriptorAssigner descriptorAssigner)
+                                                                    FileDescriptor[] dependencies)
         {
             FileDescriptorProto proto;
             try
@@ -374,20 +349,16 @@ namespace Google.Protobuf.Descriptors
                 throw new ArgumentException("Failed to parse protocol buffer descriptor for generated code.", e);
             }
 
-            FileDescriptor result;
             try
             {
                 // When building descriptors for generated code, we allow unknown
                 // dependencies by default.
-                result = BuildFrom(proto, dependencies, true);
+                return BuildFrom(proto, dependencies, true);
             }
             catch (DescriptorValidationException e)
             {
                 throw new ArgumentException("Invalid embedded descriptor for \"" + proto.Name + "\".", e);
             }
-
-            descriptorAssigner(result);
-            return result;
         }
         
         public override string ToString()

+ 13 - 10
csharp/src/ProtocolBuffers/FieldAccess/FieldAccessorBase.cs

@@ -32,34 +32,37 @@
 
 using System;
 using System.Reflection;
+using Google.Protobuf.Descriptors;
 
 namespace Google.Protobuf.FieldAccess
 {
     /// <summary>
     /// Base class for field accessors.
     /// </summary>
-    /// <typeparam name="T">Type of message containing the field</typeparam>
-    internal abstract class FieldAccessorBase<T> : IFieldAccessor<T> where T : IMessage<T>
+    internal abstract class FieldAccessorBase : IFieldAccessor
     {
-        private readonly Func<T, object> getValueDelegate;
+        private readonly Func<object, object> getValueDelegate;
+        private readonly FieldDescriptor descriptor;
 
-        internal FieldAccessorBase(string name)
+        internal FieldAccessorBase(Type type, string propertyName, FieldDescriptor descriptor)
         {
-            PropertyInfo property = typeof(T).GetProperty(name);
+            PropertyInfo property = type.GetProperty(propertyName);
             if (property == null || !property.CanRead)
             {
                 throw new ArgumentException("Not all required properties/methods available");
             }
-            getValueDelegate = ReflectionUtil.CreateUpcastDelegate<T>(property.GetGetMethod());
+            this.descriptor = descriptor;
+            getValueDelegate = ReflectionUtil.CreateFuncObjectObject(property.GetGetMethod());
         }
 
-        public object GetValue(T message)
+        public FieldDescriptor Descriptor { get { return descriptor; } }
+
+        public object GetValue(object message)
         {
             return getValueDelegate(message);
         }
 
-        public abstract bool HasValue(T message);
-        public abstract void Clear(T message);
-        public abstract void SetValue(T message, object value);
+        public abstract void Clear(object message);
+        public abstract void SetValue(object message, object value);
     }
 }

+ 21 - 11
csharp/src/ProtocolBuffers/FieldAccess/FieldAccessorTable.cs

@@ -31,6 +31,7 @@
 #endregion
 
 using System;
+using System.Collections.ObjectModel;
 using Google.Protobuf.Descriptors;
 
 namespace Google.Protobuf.FieldAccess
@@ -38,34 +39,43 @@ namespace Google.Protobuf.FieldAccess
     /// <summary>
     /// Provides access to fields in generated messages via reflection.
     /// </summary>
-    public sealed class FieldAccessorTable<T> where T : IMessage<T>
+    public sealed class FieldAccessorTable
     {
-        private readonly IFieldAccessor<T>[] accessors;
+        private readonly ReadOnlyCollection<IFieldAccessor> accessors;
         private readonly MessageDescriptor descriptor;
 
         /// <summary>
         /// Constructs a FieldAccessorTable for a particular message class.
         /// Only one FieldAccessorTable should be constructed per class.
         /// </summary>
+        /// <param name="type">The CLR type for the message.</param>
         /// <param name="descriptor">The type's descriptor</param>
         /// <param name="propertyNames">The Pascal-case names of all the field-based properties in the message.</param>
-        public FieldAccessorTable(MessageDescriptor descriptor, string[] propertyNames)
+        public FieldAccessorTable(Type type, MessageDescriptor descriptor, string[] propertyNames)
         {
             this.descriptor = descriptor;
-            accessors = new IFieldAccessor<T>[descriptor.Fields.Count];
-            bool supportFieldPresence = descriptor.File.Syntax == FileDescriptor.ProtoSyntax.Proto2;
-            for (int i = 0; i < accessors.Length; i++)
+            var accessorsArray = new IFieldAccessor[descriptor.Fields.Count];
+            for (int i = 0; i < accessorsArray.Length; i++)
             {
                 var field = descriptor.Fields[i];
                 var name = propertyNames[i];
-                accessors[i] = field.IsRepeated
-                    ? (IFieldAccessor<T>) new RepeatedFieldAccessor<T>(propertyNames[i])
-                    : new SingleFieldAccessor<T>(field, name, supportFieldPresence);
+                accessorsArray[i] =
+                    field.IsMap ? new MapFieldAccessor(type, name, field)
+                    : field.IsRepeated ? new RepeatedFieldAccessor(type, name, field)
+                    : (IFieldAccessor) new SingleFieldAccessor(type, name, field);
             }
+            accessors = new ReadOnlyCollection<IFieldAccessor>(accessorsArray);
             // TODO(jonskeet): Oneof support
         }
 
-        internal IFieldAccessor<T> this[int fieldNumber]
+        // TODO: Validate the name here... should possibly make this type a more "general reflection access" type,
+        // bearing in mind the oneof parts to come as well.
+        /// <summary>
+        /// Returns all of the field accessors for the message type.
+        /// </summary>
+        public ReadOnlyCollection<IFieldAccessor> Accessors { get { return accessors; } }
+
+        public IFieldAccessor this[int fieldNumber]
         {
             get
             {
@@ -74,7 +84,7 @@ namespace Google.Protobuf.FieldAccess
             }
         }
 
-        internal IFieldAccessor<T> this[FieldDescriptor field]
+        internal IFieldAccessor this[FieldDescriptor field]
         {
             get
             {

+ 17 - 15
csharp/src/ProtocolBuffers/FieldAccess/IFieldAccessor.cs

@@ -30,39 +30,41 @@
 // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
 #endregion
 
+using Google.Protobuf.Descriptors;
+
 namespace Google.Protobuf.FieldAccess
 {
     /// <summary>
-    /// Allows fields to be reflectively accessed in a smart manner.
-    /// The property descriptors for each field are created once and then cached.
-    /// In addition, this interface holds knowledge of repeated fields, builders etc.
+    /// Allows fields to be reflectively accessed.
     /// </summary>
-    internal interface IFieldAccessor<T> where T : IMessage<T>
+    public interface IFieldAccessor
     {
         /// <summary>
-        /// Indicates whether the specified message contains the field. For primitive fields
-        /// declared in proto3-syntax messages, this simply checks whether the value is the default one.
+        /// Returns the descriptor associated with this field.
         /// </summary>
-        /// <exception cref="InvalidOperationException">The field is a repeated field, or a single primitive field.</exception>
-        bool HasValue(T message);
+        FieldDescriptor Descriptor { get; }
 
         /// <summary>
         /// Clears the field in the specified message. (For repeated fields,
         /// this clears the list.)
         /// </summary>
-        void Clear(T message);
+        void Clear(object message);
 
         /// <summary>
         /// Fetches the field value. For repeated values, this will be an
-        /// <see cref="IList"/> implementation.
+        /// <see cref="IList"/> implementation. For map values, this will be an
+        /// <see cref="IDictionary"/> implementation.
         /// </summary>
-        object GetValue(T message);
+        object GetValue(object message);
 
         /// <summary>
-        /// Mutator for single fields only. (Repeated fields must be mutated
-        /// by fetching the list, then mutating that.)
+        /// Mutator for single "simple" fields only.
         /// </summary>
-        /// <exception cref="InvalidOperationException">The field is a repeated field.</exception>
-        void SetValue(T message, object value);
+        /// <remarks>
+        /// Repeated fields are mutated by fetching the value and manipulating it as a list.
+        /// Map fields are mutated by fetching the value and manipulating it as a dictionary.
+        /// </remarks>
+        /// <exception cref="InvalidOperationException">The field is not a "simple" field, or the message is frozen.</exception>
+        void SetValue(object message, object value);
     }
 }

+ 59 - 0
csharp/src/ProtocolBuffers/FieldAccess/MapFieldAccessor.cs

@@ -0,0 +1,59 @@
+#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;
+using Google.Protobuf.Descriptors;
+
+namespace Google.Protobuf.FieldAccess
+{
+    /// <summary>
+    /// Accessor for map fields.
+    /// </summary>
+    internal sealed class MapFieldAccessor : FieldAccessorBase
+    {
+        internal MapFieldAccessor(Type type, string propertyName, FieldDescriptor descriptor) : base(type, propertyName, descriptor)
+        {
+        }
+
+        public override void Clear(object message)
+        {
+            IDictionary list = (IDictionary) GetValue(message);
+            list.Clear();
+        }
+
+        public override void SetValue(object message, object value)
+        {
+            throw new InvalidOperationException("SetValue is not implemented for map fields");
+        }
+    }
+}

+ 26 - 84
csharp/src/ProtocolBuffers/FieldAccess/ReflectionUtil.cs

@@ -31,6 +31,7 @@
 #endregion
 
 using System;
+using System.Linq.Expressions;
 using System.Reflection;
 
 namespace Google.Protobuf.FieldAccess
@@ -51,101 +52,42 @@ namespace Google.Protobuf.FieldAccess
         internal static readonly Type[] EmptyTypes = new Type[0];
 
         /// <summary>
-        /// Creates a delegate which will execute the given method and then return
-        /// the result as an object.
+        /// Creates a delegate which will cast the argument to the appropriate method target type,
+        /// call the method on it, then convert the result to object.
         /// </summary>
-        public static Func<T, object> CreateUpcastDelegate<T>(MethodInfo method)
+        internal static Func<object, object> CreateFuncObjectObject(MethodInfo method)
         {
-            // The tricky bit is invoking CreateCreateUpcastDelegateImpl with the right type parameters
-            MethodInfo openImpl = typeof(ReflectionUtil).GetMethod("CreateUpcastDelegateImpl");
-            MethodInfo closedImpl = openImpl.MakeGenericMethod(typeof(T), method.ReturnType);
-            return (Func<T, object>) closedImpl.Invoke(null, new object[] {method});
+            ParameterExpression parameter = Expression.Parameter(typeof(object), "p");
+            Expression downcast = Expression.Convert(parameter, method.DeclaringType);
+            Expression call = Expression.Call(downcast, method);
+            Expression upcast = Expression.Convert(call, typeof(object));
+            return Expression.Lambda<Func<object, object>>(upcast, parameter).Compile();
         }
-
+        
         /// <summary>
-        /// Method used solely for implementing CreateUpcastDelegate. Public to avoid trust issues
-        /// in low-trust scenarios.
+        /// Creates a delegate which will execute the given method after casting the first argument to
+        /// the target type of the method, and the second argument to the first parameter type of the method.
         /// </summary>
-        public static Func<TSource, object> CreateUpcastDelegateImpl<TSource, TResult>(MethodInfo method)
+        internal static Action<object, object> CreateActionObjectObject(MethodInfo method)
         {
-            // Convert the reflection call into an open delegate, i.e. instead of calling x.Method()
-            // we'll call getter(x).
-            Func<TSource, TResult> getter = ReflectionUtil.CreateDelegateFunc<TSource, TResult>(method);
-
-            // Implicit upcast to object (within the delegate)
-            return source => getter(source);
+            ParameterExpression targetParameter = Expression.Parameter(typeof(object), "target");
+            ParameterExpression argParameter = Expression.Parameter(typeof(object), "arg");
+            Expression castTarget = Expression.Convert(targetParameter, method.DeclaringType);
+            Expression castArgument = Expression.Convert(argParameter, method.GetParameters()[0].ParameterType);
+            Expression call = Expression.Call(castTarget, method, castArgument);
+            return Expression.Lambda<Action<object, object>>(call, targetParameter, argParameter).Compile();
         }
 
         /// <summary>
-        /// Creates a delegate which will execute the given method after casting the parameter
-        /// down from object to the required parameter type.
+        /// Creates a delegate which will execute the given method after casting the first argument to
+        /// the target type of the method.
         /// </summary>
-        public static Action<T, object> CreateDowncastDelegate<T>(MethodInfo method)
-        {
-            MethodInfo openImpl = typeof(ReflectionUtil).GetMethod("CreateDowncastDelegateImpl");
-            MethodInfo closedImpl = openImpl.MakeGenericMethod(typeof(T), method.GetParameters()[0].ParameterType);
-            return (Action<T, object>) closedImpl.Invoke(null, new object[] {method});
-        }
-
-        public static Action<TSource, object> CreateDowncastDelegateImpl<TSource, TParam>(MethodInfo method)
-        {
-            // Convert the reflection call into an open delegate, i.e. instead of calling x.Method(y) we'll
-            // call Method(x, y)
-            Action<TSource, TParam> call = ReflectionUtil.CreateDelegateAction<TSource, TParam>(method);
-
-            return (source, parameter) => call(source, (TParam) parameter);
-        }
-
-        /// <summary>
-        /// Creates a delegate which will execute the given method after casting the parameter
-        /// down from object to the required parameter type.
-        /// </summary>
-        public static Action<T, object> CreateDowncastDelegateIgnoringReturn<T>(MethodInfo method)
-        {
-            MethodInfo openImpl = typeof(ReflectionUtil).GetMethod("CreateDowncastDelegateIgnoringReturnImpl");
-            MethodInfo closedImpl = openImpl.MakeGenericMethod(typeof(T), method.GetParameters()[0].ParameterType,
-                                                               method.ReturnType);
-            return (Action<T, object>) closedImpl.Invoke(null, new object[] {method});
-        }
-
-        public static Action<TSource, object> CreateDowncastDelegateIgnoringReturnImpl<TSource, TParam, TReturn>(
-            MethodInfo method)
-        {
-            // Convert the reflection call into an open delegate, i.e. instead of calling x.Method(y) we'll
-            // call Method(x, y)
-            Func<TSource, TParam, TReturn> call = ReflectionUtil.CreateDelegateFunc<TSource, TParam, TReturn>(method);
-
-            return delegate(TSource source, object parameter) { call(source, (TParam) parameter); };
-        }
-
-        internal static Func<TResult> CreateDelegateFunc<TResult>(MethodInfo method)
-        {
-            object tdelegate = Delegate.CreateDelegate(typeof(Func<TResult>), null, method);
-            return (Func<TResult>)tdelegate;
-        }
-
-        internal static Func<T, TResult> CreateDelegateFunc<T, TResult>(MethodInfo method)
-        {
-            object tdelegate = Delegate.CreateDelegate(typeof(Func<T, TResult>), null, method);
-            return (Func<T, TResult>)tdelegate;
-        }
-
-        internal static Func<T1, T2, TResult> CreateDelegateFunc<T1, T2, TResult>(MethodInfo method)
-        {
-            object tdelegate = Delegate.CreateDelegate(typeof(Func<T1, T2, TResult>), null, method);
-            return (Func<T1, T2, TResult>)tdelegate;
-        }
-
-        internal static Action<T> CreateDelegateAction<T>(MethodInfo method)
-        {
-            object tdelegate = Delegate.CreateDelegate(typeof(Action<T>), null, method);
-            return (Action<T>)tdelegate;
-        }
-
-        internal static Action<T1, T2> CreateDelegateAction<T1, T2>(MethodInfo method)
+        internal static Action<object> CreateActionObject(MethodInfo method)
         {
-            object tdelegate = Delegate.CreateDelegate(typeof(Action<T1, T2>), null, method);
-            return (Action<T1, T2>)tdelegate;
+            ParameterExpression targetParameter = Expression.Parameter(typeof(object), "target");
+            Expression castTarget = Expression.Convert(targetParameter, method.DeclaringType);
+            Expression call = Expression.Call(castTarget, method);
+            return Expression.Lambda<Action<object>>(call, targetParameter).Compile();
         }
     }
 }

+ 6 - 11
csharp/src/ProtocolBuffers/FieldAccess/RepeatedFieldAccessor.cs

@@ -32,33 +32,28 @@
 
 using System;
 using System.Collections;
+using Google.Protobuf.Descriptors;
 
 namespace Google.Protobuf.FieldAccess
 {
     /// <summary>
     /// Accessor for repeated fields.
     /// </summary>
-    /// <typeparam name="T">The type of message containing the field.</typeparam>
-    internal sealed class RepeatedFieldAccessor<T> : FieldAccessorBase<T> where T : IMessage<T>
+    internal sealed class RepeatedFieldAccessor : FieldAccessorBase
     {
-        internal RepeatedFieldAccessor(string name) : base(name)
+        internal RepeatedFieldAccessor(Type type, string propertyName, FieldDescriptor descriptor) : base(type, propertyName, descriptor)
         {
         }
 
-        public override void Clear(T message)
+        public override void Clear(object message)
         {
             IList list = (IList) GetValue(message);
             list.Clear();
         }
 
-        public override bool HasValue(T message)
+        public override void SetValue(object message, object value)
         {
-            throw new NotImplementedException("HasValue is not implemented for repeated fields");
-        }
-
-        public override void SetValue(T message, object value)
-        {
-            throw new NotImplementedException("SetValue is not implemented for repeated fields");
+            throw new InvalidOperationException("SetValue is not implemented for repeated fields");
         }
 
     }

+ 17 - 47
csharp/src/ProtocolBuffers/FieldAccess/SingleFieldAccessor.cs

@@ -39,76 +39,46 @@ namespace Google.Protobuf.FieldAccess
     /// <summary>
     /// Accessor for single fields.
     /// </summary>
-    /// <typeparam name="T">The type of message containing the field.</typeparam>
-    internal sealed class SingleFieldAccessor<T> : FieldAccessorBase<T> where T : IMessage<T>
+    internal sealed class SingleFieldAccessor : FieldAccessorBase
     {
         // All the work here is actually done in the constructor - it creates the appropriate delegates.
         // There are various cases to consider, based on the property type (message, string/bytes, or "genuine" primitive)
         // and proto2 vs proto3 for non-message types, as proto3 doesn't support "full" presence detection or default
         // values.
 
-        private readonly Action<T, object> setValueDelegate;
-        private readonly Action<T> clearDelegate;
-        private readonly Func<T, bool> hasValueDelegate;
+        private readonly Action<object, object> setValueDelegate;
+        private readonly Action<object> clearDelegate;
 
-        internal SingleFieldAccessor(FieldDescriptor descriptor, string name, bool supportsFieldPresence) : base(name)
+        internal SingleFieldAccessor(Type type, string propertyName, FieldDescriptor descriptor) : base(type, propertyName, descriptor)
         {
-            PropertyInfo property = typeof(T).GetProperty(name);
+            PropertyInfo property = type.GetProperty(propertyName);
             // We know there *is* such a property, or the base class constructor would have thrown. We should be able to write
             // to it though.
             if (!property.CanWrite)
             {
                 throw new ArgumentException("Not all required properties/methods available");
             }
-            setValueDelegate = ReflectionUtil.CreateDowncastDelegate<T>(property.GetSetMethod());
+            setValueDelegate = ReflectionUtil.CreateActionObjectObject(property.GetSetMethod());
 
             var clrType = property.PropertyType;
+            
+            // TODO: What should clear on a oneof member do? Clear the oneof?
 
-            if (typeof(IMessage).IsAssignableFrom(clrType))
-            {
-                // Message types are simple - we only need to detect nullity.
-                clearDelegate = message => SetValue(message, null);
-                hasValueDelegate = message => GetValue(message) == null;
-            }
-
-            if (supportsFieldPresence)
-            {
-                // Proto2: we expect a HasFoo property and a ClearFoo method.
-                // For strings and byte arrays, setting the property to null would have the equivalent effect,
-                // but we generate the method for consistency, which makes this simpler.
-                PropertyInfo hasProperty = typeof(T).GetProperty("Has" + name);
-                MethodInfo clearMethod = typeof(T).GetMethod("Clear" + name);
-                if (hasProperty == null || clearMethod == null || !hasProperty.CanRead)
-                {
-                    throw new ArgumentException("Not all required properties/methods available");
-                }
-                hasValueDelegate = ReflectionUtil.CreateDelegateFunc<T, bool>(hasProperty.GetGetMethod());
-                clearDelegate = ReflectionUtil.CreateDelegateAction<T>(clearMethod);
-            }
-            else
-            {
-                /*
-                // TODO(jonskeet): Reimplement. We need a better way of working out default values.
-                // Proto3: for field detection, we just need the default value of the field (0, "", byte[0] etc)
-                // To clear a field, we set the value to that default.
-                object defaultValue = descriptor.DefaultValue;
-                hasValueDelegate = message => GetValue(message).Equals(defaultValue);
-                clearDelegate = message => SetValue(message, defaultValue);
-                */
-            }
-        }
-
-        public override bool HasValue(T message)
-        {
-            return hasValueDelegate(message);
+            // TODO: Validate that this is a reasonable single field? (Should be a value type, a message type, or string/ByteString.)
+            object defaultValue =
+                typeof(IMessage).IsAssignableFrom(clrType) ? null
+                : clrType == typeof(string) ? ""
+                : clrType == typeof(ByteString) ? ByteString.Empty
+                : Activator.CreateInstance(clrType);
+            clearDelegate = message => SetValue(message, defaultValue);
         }
 
-        public override void Clear(T message)
+        public override void Clear(object message)
         {
             clearDelegate(message);
         }
 
-        public override void SetValue(T message, object value)
+        public override void SetValue(object message, object value)
         {
             setValueDelegate(message, value);
         }

+ 3 - 4
csharp/src/ProtocolBuffers/IMessage.cs

@@ -40,12 +40,11 @@ namespace Google.Protobuf
     // TODO(jonskeet): Split these interfaces into separate files when we're happy with them.
 
     /// <summary>
-    /// Reflection support for a specific message type. message
+    /// Reflection support for a specific message type.
     /// </summary>
-    /// <typeparam name="T">The message type being reflected.</typeparam>
-    public interface IReflectedMessage<T> where T : IMessage<T>
+    public interface IReflectedMessage
     {
-        FieldAccessorTable<T> Fields { get; }
+        FieldAccessorTable Fields { get; }
         // TODO(jonskeet): Descriptor? Or a single property which has "all you need for reflection"?
     }
 

+ 1 - 0
csharp/src/ProtocolBuffers/ProtocolBuffers.csproj

@@ -79,6 +79,7 @@
     <Compile Include="Descriptors\MethodDescriptor.cs" />
     <Compile Include="Descriptors\PackageDescriptor.cs" />
     <Compile Include="Descriptors\ServiceDescriptor.cs" />
+    <Compile Include="FieldAccess\MapFieldAccessor.cs" />
     <Compile Include="FieldCodec.cs" />
     <Compile Include="FrameworkPortability.cs" />
     <Compile Include="Freezable.cs" />

+ 22 - 15
src/google/protobuf/compiler/csharp/csharp_message.cc

@@ -116,8 +116,7 @@ void MessageGenerator::GenerateStaticVariables(io::Printer* printer) {
 
   // The descriptor for this type.
   printer->Print(
-      "internal static pbd::MessageDescriptor internal__$identifier$__Descriptor;\n"
-      "internal static pb::FieldAccess.FieldAccessorTable<$full_class_name$> internal__$identifier$__FieldAccessorTable;\n",
+      "internal static pb::FieldAccess.FieldAccessorTable internal__$identifier$__FieldAccessorTable;\n",
       "identifier", GetUniqueFileScopeIdentifier(descriptor_),
       "full_class_name", full_class_name());
 
@@ -130,24 +129,23 @@ void MessageGenerator::GenerateStaticVariables(io::Printer* printer) {
 void MessageGenerator::GenerateStaticVariableInitializers(io::Printer* printer) {
   map<string, string> vars;
   vars["identifier"] = GetUniqueFileScopeIdentifier(descriptor_);
-  vars["index"] = SimpleItoa(descriptor_->index());
   vars["full_class_name"] = full_class_name();
-  if (descriptor_->containing_type() != NULL) {
-    vars["parent"] = GetUniqueFileScopeIdentifier(
-	descriptor_->containing_type());
-  }
-  printer->Print(vars, "internal__$identifier$__Descriptor = ");
 
-  if (!descriptor_->containing_type()) {
-    printer->Print(vars, "Descriptor.MessageTypes[$index$];\n");
-  } else {
-    printer->Print(vars, "internal__$parent$__Descriptor.NestedTypes[$index$];\n");
+  // Work out how to get to the message descriptor (which may be multiply nested) from the file
+  // descriptor.
+  string descriptor_chain;
+  const Descriptor* current_descriptor = descriptor_;
+  while (current_descriptor->containing_type()) {
+    descriptor_chain = ".NestedTypes[" + SimpleItoa(current_descriptor->index()) + "]" + descriptor_chain;
+    current_descriptor = current_descriptor->containing_type();
   }
+  descriptor_chain = "descriptor.MessageTypes[" + SimpleItoa(current_descriptor->index()) + "]" + descriptor_chain;
+  vars["descriptor_chain"] = descriptor_chain;
 
   printer->Print(
     vars,
     "internal__$identifier$__FieldAccessorTable = \n"
-    "    new pb::FieldAccess.FieldAccessorTable<$full_class_name$>(internal__$identifier$__Descriptor,\n");
+    "    new pb::FieldAccess.FieldAccessorTable(typeof($full_class_name$), $descriptor_chain$,\n");
   printer->Print("        new string[] { ");
   for (int i = 0; i < descriptor_->field_count(); i++) {
     printer->Print("\"$property_name$\", ",
@@ -201,13 +199,22 @@ void MessageGenerator::Generate(io::Printer* printer) {
     "private static readonly uint[] _fieldTags = new uint[] { $tags$ };\n",
     "tags", JoinStrings(tags, ", "));
 
+  // Access the message descriptor via the relevant file descriptor or containing message descriptor.
+  if (!descriptor_->containing_type()) {
+    vars["descriptor_accessor"] = GetFullUmbrellaClassName(descriptor_->file())
+        + ".Descriptor.MessageTypes[" + SimpleItoa(descriptor_->index()) + "]";
+  } else {
+    vars["descriptor_accessor"] = GetClassName(descriptor_->containing_type())
+        + ".Descriptor.NestedTypes[" + SimpleItoa(descriptor_->index()) + "]";
+  }
+
   printer->Print(
     vars,
     "public static pbd::MessageDescriptor Descriptor {\n"
-    "  get { return $umbrella_class_name$.internal__$identifier$__Descriptor; }\n"
+    "  get { return $descriptor_accessor$; }\n"
     "}\n"
     "\n"
-    "public pb::FieldAccess.FieldAccessorTable<$class_name$> Fields {\n"
+    "public pb::FieldAccess.FieldAccessorTable Fields {\n"
     "  get { return $umbrella_class_name$.internal__$identifier$__FieldAccessorTable; }\n"
     "}\n"
     "\n"

+ 8 - 14
src/google/protobuf/compiler/csharp/csharp_umbrella_class.cc

@@ -176,22 +176,11 @@ void UmbrellaClassGenerator::WriteDescriptor(io::Printer* printer) {
   printer->Print("\"$base64$\"));\n", "base64", base64);
   printer->Outdent();
   printer->Outdent();
-  printer->Print(
-    "pbd::FileDescriptor.InternalDescriptorAssigner assigner = delegate(pbd::FileDescriptor root) {\n");
-  printer->Indent();
-  printer->Print("descriptor = root;\n");
-  for (int i = 0; i < file_->message_type_count(); i++) {
-    MessageGenerator messageGenerator(file_->message_type(i));
-    messageGenerator.GenerateStaticVariableInitializers(printer);
-  }
-
-  printer->Outdent();
-  printer->Print("};\n");
 
   // -----------------------------------------------------------------
-  // Invoke internalBuildGeneratedFileFrom() to build the file.
+  // Invoke InternalBuildGeneratedFileFrom() to build the file.
   printer->Print(
-      "pbd::FileDescriptor.InternalBuildGeneratedFileFrom(descriptorData,\n");
+      "descriptor = pbd::FileDescriptor.InternalBuildGeneratedFileFrom(descriptorData,\n");
   printer->Print("    new pbd::FileDescriptor[] {\n");
   for (int i = 0; i < file_->dependency_count(); i++) {
     printer->Print(
@@ -199,7 +188,12 @@ void UmbrellaClassGenerator::WriteDescriptor(io::Printer* printer) {
       "full_umbrella_class_name",
       GetFullUmbrellaClassName(file_->dependency(i)));
   }
-  printer->Print("    }, assigner);\n");
+  printer->Print("    });\n");
+  // Then invoke any other static variable initializers, e.g. field accessors.
+  for (int i = 0; i < file_->message_type_count(); i++) {
+      MessageGenerator messageGenerator(file_->message_type(i));
+      messageGenerator.GenerateStaticVariableInitializers(printer);
+  }
   printer->Outdent();
   printer->Print("}\n");
   printer->Print("#endregion\n\n");