Add reflection support for proto3 optional fields

This is more involved than might be expected because the synthetic oneofs don't generate the properties we would usually expect to see.

csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs

@@ -32,6 +32,7 @@
 
 using Google.Protobuf.TestProtos;
 using NUnit.Framework;
+using ProtobufUnittest;
 using System;
 using System.Collections.Generic;
 using System.Linq;
@@ -247,6 +248,7 @@ namespace Google.Protobuf.Reflection
             FieldDescriptor enumField = testAllTypesDescriptor.FindDescriptor<FieldDescriptor>("single_nested_enum");
             FieldDescriptor foreignMessageField = testAllTypesDescriptor.FindDescriptor<FieldDescriptor>("single_foreign_message");
             FieldDescriptor importMessageField = testAllTypesDescriptor.FindDescriptor<FieldDescriptor>("single_import_message");
+            FieldDescriptor fieldInOneof = testAllTypesDescriptor.FindDescriptor<FieldDescriptor>("oneof_string");
 
             Assert.AreEqual("single_int32", primitiveField.Name);
             Assert.AreEqual("protobuf_unittest3.TestAllTypes.single_int32",
@@ -268,6 +270,10 @@ namespace Google.Protobuf.Reflection
             Assert.AreEqual("single_import_message", importMessageField.Name);
             Assert.AreEqual(FieldType.Message, importMessageField.FieldType);
             Assert.AreEqual(importMessageDescriptor, importMessageField.MessageType);
+
+            // For a field in a regular onoef, ContainingOneof and RealContainingOneof should be the same.
+            Assert.AreEqual("oneof_field", fieldInOneof.ContainingOneof.Name);
+            Assert.AreSame(fieldInOneof.ContainingOneof, fieldInOneof.RealContainingOneof);
         }
 
         [Test]
@@ -318,6 +324,7 @@ namespace Google.Protobuf.Reflection
         public void OneofDescriptor()
         {
             OneofDescriptor descriptor = TestAllTypes.Descriptor.FindDescriptor<OneofDescriptor>("oneof_field");
+            Assert.IsFalse(descriptor.IsSynthetic);
             Assert.AreEqual("oneof_field", descriptor.Name);
             Assert.AreEqual("protobuf_unittest3.TestAllTypes.oneof_field", descriptor.FullName);
 
@@ -383,5 +390,48 @@ namespace Google.Protobuf.Reflection
             var importingDescriptor = TestProtos.OldGenerator.OldExtensions1Reflection.Descriptor;
             Assert.NotNull(importingDescriptor);
         }
+
+        [Test]
+        public void Proto3OptionalDescriptors()
+        {
+            var descriptor = TestProto3Optional.Descriptor;
+            var field = descriptor.Fields[TestProto3Optional.OptionalInt32FieldNumber];
+            Assert.NotNull(field.ContainingOneof);
+            Assert.IsTrue(field.ContainingOneof.IsSynthetic);
+            Assert.Null(field.RealContainingOneof);
+        }
+
+
+        [Test]
+        public void SyntheticOneofReflection()
+        {
+            // Expect every oneof in TestProto3Optional to be synthetic
+            var proto3OptionalDescriptor = TestProto3Optional.Descriptor;
+            Assert.AreEqual(0, proto3OptionalDescriptor.RealOneofCount);
+            foreach (var oneof in proto3OptionalDescriptor.Oneofs)
+            {
+                Assert.True(oneof.IsSynthetic);
+            }
+
+            // Expect no oneof in the original proto3 unit test file to be synthetic.
+            foreach (var descriptor in ProtobufTestMessages.Proto3.TestMessagesProto3Reflection.Descriptor.MessageTypes)
+            {
+                Assert.AreEqual(descriptor.Oneofs.Count, descriptor.RealOneofCount);
+                foreach (var oneof in descriptor.Oneofs)
+                {
+                    Assert.False(oneof.IsSynthetic);
+                }
+            }
+
+            // Expect no oneof in the original proto2 unit test file to be synthetic.
+            foreach (var descriptor in ProtobufTestMessages.Proto2.TestMessagesProto2Reflection.Descriptor.MessageTypes)
+            {
+                Assert.AreEqual(descriptor.Oneofs.Count, descriptor.RealOneofCount);
+                foreach (var oneof in descriptor.Oneofs)
+                {
+                    Assert.False(oneof.IsSynthetic);
+                }
+            }
+        }
     }
 }

csharp/src/Google.Protobuf.Test/Reflection/FieldAccessTest.cs

@@ -38,6 +38,7 @@ using System.Collections;
 using System.Collections.Generic;
 
 using static Google.Protobuf.TestProtos.Proto2.UnittestExtensions;
+using ProtobufUnittest;
 
 namespace Google.Protobuf.Reflection
 {
@@ -104,6 +105,21 @@ namespace Google.Protobuf.Reflection
             Assert.Throws<InvalidOperationException>(() => fields[TestProtos.TestAllTypes.SingleBoolFieldNumber].Accessor.HasValue(message));
         }
 
+        [Test]
+        public void HasValue_Proto3Optional()
+        {
+            IMessage message = new TestProto3Optional
+            {
+                OptionalInt32 = 0,
+                LazyNestedMessage = new TestProto3Optional.Types.NestedMessage()
+            };
+            var fields = message.Descriptor.Fields;
+            Assert.IsFalse(fields[TestProto3Optional.OptionalInt64FieldNumber].Accessor.HasValue(message));
+            Assert.IsFalse(fields[TestProto3Optional.OptionalNestedMessageFieldNumber].Accessor.HasValue(message));
+            Assert.IsTrue(fields[TestProto3Optional.LazyNestedMessageFieldNumber].Accessor.HasValue(message));
+            Assert.IsTrue(fields[TestProto3Optional.OptionalInt32FieldNumber].Accessor.HasValue(message));
+        }
+
         [Test]
         public void HasValue()
         {
@@ -225,6 +241,27 @@ namespace Google.Protobuf.Reflection
             Assert.AreEqual(0, mapMessage.MapStringString.Count);
         }
 
+        [Test]
+        public void Clear_Proto3Optional()
+        {
+            TestProto3Optional message = new TestProto3Optional
+            {
+                OptionalInt32 = 0,
+                OptionalNestedMessage = new TestProto3Optional.Types.NestedMessage()
+            };
+            var primitiveField = TestProto3Optional.Descriptor.Fields[TestProto3Optional.OptionalInt32FieldNumber];
+            var messageField = TestProto3Optional.Descriptor.Fields[TestProto3Optional.OptionalNestedMessageFieldNumber];
+
+            Assert.True(message.HasOptionalInt32);
+            Assert.NotNull(message.OptionalNestedMessage);
+
+            primitiveField.Accessor.Clear(message);
+            messageField.Accessor.Clear(message);
+
+            Assert.False(message.HasOptionalInt32);
+            Assert.Null(message.OptionalNestedMessage);
+        }
+
         [Test]
         public void FieldDescriptor_ByName()
         {

csharp/src/Google.Protobuf/Reflection/FieldDescriptor.cs

@@ -58,6 +58,12 @@ namespace Google.Protobuf.Reflection
         /// </summary>
         public OneofDescriptor ContainingOneof { get; }
 
+        /// <summary>
+        /// Returns the oneof containing this field if it's a "real" oneof, or <c>null</c> if either this
+        /// field is not part of a oneof, or the oneof is synthetic.
+        /// </summary>
+        public OneofDescriptor RealContainingOneof => ContainingOneof?.IsSynthetic == false ? ContainingOneof : null;
+
         /// <summary>
         /// The effective JSON name for this field. This is usually the lower-camel-cased form of the field name,
         /// but can be overridden using the <c>json_name</c> option in the .proto file.

csharp/src/Google.Protobuf/Reflection/IFieldAccessor.cs

@@ -59,7 +59,8 @@ namespace Google.Protobuf.Reflection
         object GetValue(IMessage message);
 
         /// <summary>
-        /// Indicates whether the field in the specified message is set. For proto3 fields, this throws an <see cref="InvalidOperationException"/>
+        /// Indicates whether the field in the specified message is set.
+        /// For proto3 fields that aren't explicitly optional, this throws an <see cref="InvalidOperationException"/>
         /// </summary>
         bool HasValue(IMessage message);
 

csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs

@@ -80,6 +80,20 @@ namespace Google.Protobuf.Reflection
                 (oneof, index) =>
                 new OneofDescriptor(oneof, file, this, index, generatedCodeInfo?.OneofNames[index]));
 
+            int syntheticOneofCount = 0;
+            foreach (var oneof in Oneofs)
+            {
+                if (oneof.IsSynthetic)
+                {
+                    syntheticOneofCount++;
+                }
+                else if (syntheticOneofCount != 0)
+                {
+                    throw new ArgumentException("All synthetic oneofs should come after real oneofs");
+                }
+            }
+            RealOneofCount = Oneofs.Count - syntheticOneofCount;
+
             NestedTypes = DescriptorUtil.ConvertAndMakeReadOnly(
                 proto.NestedType,
                 (type, index) =>
@@ -234,9 +248,19 @@ namespace Google.Protobuf.Reflection
 
         /// <value>
         /// An unmodifiable list of the "oneof" field collections in this message type.
+        /// All "real" oneofs (where <see cref="OneofDescriptor.IsSynthetic"/> returns false)
+        /// come before synthetic ones.
         /// </value>
         public IList<OneofDescriptor> Oneofs { get; }
 
+        /// <summary>
+        /// The number of real "oneof" descriptors in this message type. Every element in <see cref="Oneofs"/>
+        /// with an index less than this will have a <see cref="OneofDescriptor.IsSynthetic"/> property value
+        /// of <c>false</c>; every element with an index greater than or equal to this will have a
+        /// <see cref="OneofDescriptor.IsSynthetic"/> property value of <c>true</c>.
+        /// </summary>
+        public int RealOneofCount { get; }
+
         /// <summary>
         /// Finds a field by field name.
         /// </summary>

csharp/src/Google.Protobuf/Reflection/OneofAccessor.cs

@@ -43,19 +43,31 @@ namespace Google.Protobuf.Reflection
     {
         private readonly Func<IMessage, int> caseDelegate;
         private readonly Action<IMessage> clearDelegate;
-        private OneofDescriptor descriptor;
 
-        internal OneofAccessor(PropertyInfo caseProperty, MethodInfo clearMethod, OneofDescriptor descriptor)
+        private OneofAccessor(OneofDescriptor descriptor, Func<IMessage, int> caseDelegate, Action<IMessage> clearDelegate)
         {
-            if (!caseProperty.CanRead)
-            {
-                throw new ArgumentException("Cannot read from property");
-            }
-            this.descriptor = descriptor;
-            caseDelegate = ReflectionUtil.CreateFuncIMessageInt32(caseProperty.GetGetMethod());
+            Descriptor = descriptor;
+            this.caseDelegate = caseDelegate;
+            this.clearDelegate = clearDelegate;
+        }
+
+        internal static OneofAccessor ForRegularOneof(
+            OneofDescriptor descriptor,
+            PropertyInfo caseProperty,
+            MethodInfo clearMethod) =>
+            new OneofAccessor(
+                descriptor,
+                ReflectionUtil.CreateFuncIMessageInt32(caseProperty.GetGetMethod()),
+                ReflectionUtil.CreateActionIMessage(clearMethod));
 
-            this.descriptor = descriptor;
-            clearDelegate = ReflectionUtil.CreateActionIMessage(clearMethod);
+        internal static OneofAccessor ForSyntheticOneof(OneofDescriptor descriptor)
+        {
+            // Note: descriptor.Fields will be null when this method is called, because we haven't
+            // cross-linked yet. But by the time the delgates are called by user code, all will be
+            // well. (That's why we capture the descriptor itself rather than a field.)
+            return new OneofAccessor(descriptor,
+                message => descriptor.Fields[0].Accessor.HasValue(message) ? descriptor.Fields[0].FieldNumber : 0,
+                message => descriptor.Fields[0].Accessor.Clear(message));
         }
 
         /// <summary>
@@ -64,15 +76,12 @@ namespace Google.Protobuf.Reflection
         /// <value>
         /// The descriptor of the oneof.
         /// </value>
-        public OneofDescriptor Descriptor { get { return descriptor; } }
+        public OneofDescriptor Descriptor { get; }
 
         /// <summary>
         /// Clears the oneof in the specified message.
         /// </summary>
-        public void Clear(IMessage message)
-        {
-            clearDelegate(message);
-        }
+        public void Clear(IMessage message) => clearDelegate(message);
 
         /// <summary>
         /// Indicates which field in the oneof is set for specified message
@@ -80,11 +89,9 @@ namespace Google.Protobuf.Reflection
         public FieldDescriptor GetCaseFieldDescriptor(IMessage message)
         {
             int fieldNumber = caseDelegate(message);
-            if (fieldNumber > 0)
-            {
-                return descriptor.ContainingType.FindFieldByNumber(fieldNumber);
-            }
-            return null;
+            return fieldNumber > 0
+                ? Descriptor.ContainingType.FindFieldByNumber(fieldNumber)
+                : null;
         }
     }
 }

csharp/src/Google.Protobuf/Reflection/OneofDescriptor.cs

@@ -33,6 +33,7 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.ObjectModel;
+using System.Linq;
 using Google.Protobuf.Collections;
 using Google.Protobuf.Compatibility;
 
@@ -54,8 +55,13 @@ namespace Google.Protobuf.Reflection
         {
             this.proto = proto;
             containingType = parent;
-
             file.DescriptorPool.AddSymbol(this);
+
+            // It's useful to determine whether or not this is a synthetic oneof before cross-linking. That means
+            // diving into the proto directly rather than using FieldDescriptor, but that's okay.
+            var firstFieldInOneof = parent.Proto.Field.FirstOrDefault(fieldProto => fieldProto.OneofIndex == index);
+            IsSynthetic = firstFieldInOneof?.Proto3Optional ?? false;
+
             accessor = CreateAccessor(clrName);
         }
 
@@ -83,6 +89,12 @@ namespace Google.Protobuf.Reflection
         /// </value>
         public IList<FieldDescriptor> Fields { get { return fields; } }
 
+        /// <summary>
+        /// Returns <c>true</c> if this oneof is a synthetic oneof containing a proto3 optional field;
+        /// <c>false</c> otherwise.
+        /// </summary>
+        public bool IsSynthetic { get; }
+
         /// <summary>
         /// Gets an accessor for reflective access to the values associated with the oneof
         /// in a particular message.
@@ -146,18 +158,28 @@ namespace Google.Protobuf.Reflection
             {
                 return null;
             }
-            var caseProperty = containingType.ClrType.GetProperty(clrName + "Case");
-            if (caseProperty == null)
+            if (IsSynthetic)
             {
-                throw new DescriptorValidationException(this, $"Property {clrName}Case not found in {containingType.ClrType}");
+                return OneofAccessor.ForSyntheticOneof(this);
             }
-            var clearMethod = containingType.ClrType.GetMethod("Clear" + clrName);
-            if (clearMethod == null)
+            else
             {
-                throw new DescriptorValidationException(this, $"Method Clear{clrName} not found in {containingType.ClrType}");
+                var caseProperty = containingType.ClrType.GetProperty(clrName + "Case");
+                if (caseProperty == null)
+                {
+                    throw new DescriptorValidationException(this, $"Property {clrName}Case not found in {containingType.ClrType}");
+                }
+                if (!caseProperty.CanRead)
+                {
+                    throw new ArgumentException($"Cannot read from property {clrName}Case in {containingType.ClrType}");
+                }
+                var clearMethod = containingType.ClrType.GetMethod("Clear" + clrName);
+                if (clearMethod == null)
+                {
+                    throw new DescriptorValidationException(this, $"Method Clear{clrName} not found in {containingType.ClrType}");
+                }
+                return OneofAccessor.ForRegularOneof(this, caseProperty, clearMethod);
             }
-
-            return new OneofAccessor(caseProperty, clearMethod, this);
         }
     }
 }

csharp/src/Google.Protobuf/Reflection/SingleFieldAccessor.cs

@@ -57,10 +57,11 @@ namespace Google.Protobuf.Reflection
                 throw new ArgumentException("Not all required properties/methods available");
             }
             setValueDelegate = ReflectionUtil.CreateActionIMessageObject(property.GetSetMethod());
-            if (descriptor.File.Syntax == Syntax.Proto3)
+            if (descriptor.File.Syntax == Syntax.Proto3 && !descriptor.Proto.Proto3Optional)
             {
-                hasDelegate = message => {
-                  throw new InvalidOperationException("HasValue is not implemented for proto3 fields");
+                hasDelegate = message =>
+                {
+                    throw new InvalidOperationException("HasValue is not implemented for non-optional proto3 fields");
                 };
                 var clrType = property.PropertyType;
 
@@ -74,16 +75,29 @@ namespace Google.Protobuf.Reflection
             }
             else
             {
-                MethodInfo hasMethod = property.DeclaringType.GetRuntimeProperty("Has" + property.Name).GetMethod;
-                if (hasMethod == null) {
-                  throw new ArgumentException("Not all required properties/methods are available");
+                // For message fields, just compare with null and set to null.
+                // For primitive fields, use the Has/Clear methods.
+
+                if (descriptor.FieldType == FieldType.Message)
+                {
+                    hasDelegate = message => GetValue(message) != null;
+                    clearDelegate = message => SetValue(message, null);
                 }
-                hasDelegate = ReflectionUtil.CreateFuncIMessageBool(hasMethod);
-                MethodInfo clearMethod = property.DeclaringType.GetRuntimeMethod("Clear" + property.Name, ReflectionUtil.EmptyTypes);
-                if (clearMethod == null) {
-                  throw new ArgumentException("Not all required properties/methods are available");
+                else
+                {
+                    MethodInfo hasMethod = property.DeclaringType.GetRuntimeProperty("Has" + property.Name).GetMethod;
+                    if (hasMethod == null)
+                    {
+                        throw new ArgumentException("Not all required properties/methods are available");
+                    }
+                    hasDelegate = ReflectionUtil.CreateFuncIMessageBool(hasMethod);
+                    MethodInfo clearMethod = property.DeclaringType.GetRuntimeMethod("Clear" + property.Name, ReflectionUtil.EmptyTypes);
+                    if (clearMethod == null)
+                    {
+                        throw new ArgumentException("Not all required properties/methods are available");
+                    }
+                    clearDelegate = ReflectionUtil.CreateActionIMessage(clearMethod);
                 }
-                clearDelegate = ReflectionUtil.CreateActionIMessage(clearMethod);
             }
         }