Merge pull request #6824 from jskeet/fix-extensions

Fix reflection access when using old generated code

Makefile.am

@@ -66,6 +66,8 @@ csharp_EXTRA_DIST=                                                           \
   csharp/keys/README.md                                                      \
   csharp/protos/README.md                                                    \
   csharp/protos/map_unittest_proto3.proto                                    \
+  csharp/protos/old_extensions1.proto                                        \
+  csharp/protos/old_extensions2.proto                                        \
   csharp/protos/unittest_custom_options_proto3.proto                         \
   csharp/protos/unittest_import_public_proto3.proto                          \
   csharp/protos/unittest_import_public.proto                                 \

csharp/generate_protos.sh

@@ -41,6 +41,9 @@ $PROTOC -Isrc --csharp_out=csharp/src/Google.Protobuf \
     src/google/protobuf/wrappers.proto
 
 # Test protos
+# Note that this deliberately does *not* include old_extensions1.proto
+# and old_extensions2.proto, which are generated with an older version
+# of protoc.
 $PROTOC -Isrc -Icsharp/protos \
     --csharp_out=csharp/src/Google.Protobuf.Test/TestProtos \
     --descriptor_set_out=csharp/src/Google.Protobuf.Test/testprotos.pb \

csharp/protos/old_extensions1.proto

@@ -0,0 +1,52 @@
+// Protocol Buffers - Google's data interchange format
+// Copyright 2008 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.
+
+// Author: kenton@google.com (Kenton Varda)
+//  Based on original Protocol Buffers design by
+//  Sanjay Ghemawat, Jeff Dean, and others.
+//
+// A proto file we will use for unit testing.
+//
+// LINT: ALLOW_GROUPS, LEGACY_NAMES
+
+// This file is part of the unit test for issue #6822. It is
+// generated with protoc from version 3.10.1.
+
+syntax = "proto3";
+
+// Import the proto file containing the extension. We don't use it,
+// but the import is what caused the issue.
+import "old_extensions2.proto";
+
+option csharp_namespace = "Google.Protobuf.TestProtos.OldGenerator";
+
+// We don't use this message other than to get its descriptor.
+message TestMessage {
+}

csharp/protos/old_extensions2.proto

@@ -0,0 +1,50 @@
+// Protocol Buffers - Google's data interchange format
+// Copyright 2008 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.
+
+// Author: kenton@google.com (Kenton Varda)
+//  Based on original Protocol Buffers design by
+//  Sanjay Ghemawat, Jeff Dean, and others.
+//
+// A proto file we will use for unit testing.
+//
+// LINT: ALLOW_GROUPS, LEGACY_NAMES
+
+// This file is part of the unit test for issue #6822. It is
+// generated with protoc from version 3.10.1.
+
+syntax = "proto3";
+
+import "google/protobuf/descriptor.proto";
+
+option csharp_namespace = "Google.Protobuf.TestProtos.OldGenerator";
+
+extend google.protobuf.MethodOptions {
+  string method_ext = 1234567;
+}

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

@@ -365,5 +365,23 @@ namespace Google.Protobuf.Reflection
             var descriptor = Google.Protobuf.Reflection.FileDescriptor.DescriptorProtoFileDescriptor;
             Assert.AreEqual("google/protobuf/descriptor.proto", descriptor.Name);
         }
+
+        [Test]
+        public void DescriptorImportingExtensionsFromOldCodeGen()
+        {
+            // The extension collection includes a null extension. There's not a lot we can do about that
+            // in itself, as the old generator didn't provide us the extension information.
+            var extensions = TestProtos.OldGenerator.OldExtensions2Reflection.Descriptor.Extensions;
+            Assert.AreEqual(1, extensions.UnorderedExtensions.Count);
+            // Note: this assertion is present so that it will fail if OldExtensions2 is regenerated
+            // with a new generator.
+            Assert.Null(extensions.UnorderedExtensions[0].Extension);
+
+            // ... but we can make sure we at least don't cause a failure when retrieving descriptors.
+            // In particular, old_extensions1.proto imports old_extensions2.proto, and this used to cause
+            // an execution-time failure.
+            var importingDescriptor = TestProtos.OldGenerator.OldExtensions1Reflection.Descriptor;
+            Assert.NotNull(importingDescriptor);
+        }
     }
 }

csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions1.cs

@@ -0,0 +1,148 @@
+// <auto-generated>
+//     Generated by the protocol buffer compiler.  DO NOT EDIT!
+//     source: old_extensions1.proto
+// </auto-generated>
+#pragma warning disable 1591, 0612, 3021
+#region Designer generated code
+
+using pb = global::Google.Protobuf;
+using pbc = global::Google.Protobuf.Collections;
+using pbr = global::Google.Protobuf.Reflection;
+using scg = global::System.Collections.Generic;
+namespace Google.Protobuf.TestProtos.OldGenerator {
+
+  /// <summary>Holder for reflection information generated from old_extensions1.proto</summary>
+  public static partial class OldExtensions1Reflection {
+
+    #region Descriptor
+    /// <summary>File descriptor for old_extensions1.proto</summary>
+    public static pbr::FileDescriptor Descriptor {
+      get { return descriptor; }
+    }
+    private static pbr::FileDescriptor descriptor;
+
+    static OldExtensions1Reflection() {
+      byte[] descriptorData = global::System.Convert.FromBase64String(
+          string.Concat(
+            "ChVvbGRfZXh0ZW5zaW9uczEucHJvdG8aFW9sZF9leHRlbnNpb25zMi5wcm90",
+            "byINCgtUZXN0TWVzc2FnZUIqqgInR29vZ2xlLlByb3RvYnVmLlRlc3RQcm90",
+            "b3MuT2xkR2VuZXJhdG9yYgZwcm90bzM="));
+      descriptor = pbr::FileDescriptor.FromGeneratedCode(descriptorData,
+          new pbr::FileDescriptor[] { global::Google.Protobuf.TestProtos.OldGenerator.OldExtensions2Reflection.Descriptor, },
+          new pbr::GeneratedClrTypeInfo(null, new pbr::GeneratedClrTypeInfo[] {
+            new pbr::GeneratedClrTypeInfo(typeof(global::Google.Protobuf.TestProtos.OldGenerator.TestMessage), global::Google.Protobuf.TestProtos.OldGenerator.TestMessage.Parser, null, null, null, null)
+          }));
+    }
+    #endregion
+
+  }
+  #region Messages
+  /// <summary>
+  /// We don't use this message other than to get its descriptor.
+  /// </summary>
+  public sealed partial class TestMessage : pb::IMessage<TestMessage> {
+    private static readonly pb::MessageParser<TestMessage> _parser = new pb::MessageParser<TestMessage>(() => new TestMessage());
+    private pb::UnknownFieldSet _unknownFields;
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public static pb::MessageParser<TestMessage> Parser { get { return _parser; } }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public static pbr::MessageDescriptor Descriptor {
+      get { return global::Google.Protobuf.TestProtos.OldGenerator.OldExtensions1Reflection.Descriptor.MessageTypes[0]; }
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    pbr::MessageDescriptor pb::IMessage.Descriptor {
+      get { return Descriptor; }
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public TestMessage() {
+      OnConstruction();
+    }
+
+    partial void OnConstruction();
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public TestMessage(TestMessage other) : this() {
+      _unknownFields = pb::UnknownFieldSet.Clone(other._unknownFields);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public TestMessage Clone() {
+      return new TestMessage(this);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public override bool Equals(object other) {
+      return Equals(other as TestMessage);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public bool Equals(TestMessage other) {
+      if (ReferenceEquals(other, null)) {
+        return false;
+      }
+      if (ReferenceEquals(other, this)) {
+        return true;
+      }
+      return Equals(_unknownFields, other._unknownFields);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public override int GetHashCode() {
+      int hash = 1;
+      if (_unknownFields != null) {
+        hash ^= _unknownFields.GetHashCode();
+      }
+      return hash;
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public override string ToString() {
+      return pb::JsonFormatter.ToDiagnosticString(this);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public void WriteTo(pb::CodedOutputStream output) {
+      if (_unknownFields != null) {
+        _unknownFields.WriteTo(output);
+      }
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public int CalculateSize() {
+      int size = 0;
+      if (_unknownFields != null) {
+        size += _unknownFields.CalculateSize();
+      }
+      return size;
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public void MergeFrom(TestMessage other) {
+      if (other == null) {
+        return;
+      }
+      _unknownFields = pb::UnknownFieldSet.MergeFrom(_unknownFields, other._unknownFields);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public void MergeFrom(pb::CodedInputStream input) {
+      uint tag;
+      while ((tag = input.ReadTag()) != 0) {
+        switch(tag) {
+          default:
+            _unknownFields = pb::UnknownFieldSet.MergeFieldFrom(_unknownFields, input);
+            break;
+        }
+      }
+    }
+
+  }
+
+  #endregion
+
+}
+
+#endregion Designer generated code

csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions2.cs

@@ -0,0 +1,40 @@
+// <auto-generated>
+//     Generated by the protocol buffer compiler.  DO NOT EDIT!
+//     source: old_extensions2.proto
+// </auto-generated>
+#pragma warning disable 1591, 0612, 3021
+#region Designer generated code
+
+using pb = global::Google.Protobuf;
+using pbc = global::Google.Protobuf.Collections;
+using pbr = global::Google.Protobuf.Reflection;
+using scg = global::System.Collections.Generic;
+namespace Google.Protobuf.TestProtos.OldGenerator {
+
+  /// <summary>Holder for reflection information generated from old_extensions2.proto</summary>
+  public static partial class OldExtensions2Reflection {
+
+    #region Descriptor
+    /// <summary>File descriptor for old_extensions2.proto</summary>
+    public static pbr::FileDescriptor Descriptor {
+      get { return descriptor; }
+    }
+    private static pbr::FileDescriptor descriptor;
+
+    static OldExtensions2Reflection() {
+      byte[] descriptorData = global::System.Convert.FromBase64String(
+          string.Concat(
+            "ChVvbGRfZXh0ZW5zaW9uczIucHJvdG8aIGdvb2dsZS9wcm90b2J1Zi9kZXNj",
+            "cmlwdG9yLnByb3RvOjQKCm1ldGhvZF9leHQSHi5nb29nbGUucHJvdG9idWYu",
+            "TWV0aG9kT3B0aW9ucxiHrUsgASgJQiqqAidHb29nbGUuUHJvdG9idWYuVGVz",
+            "dFByb3Rvcy5PbGRHZW5lcmF0b3JiBnByb3RvMw=="));
+      descriptor = pbr::FileDescriptor.FromGeneratedCode(descriptorData,
+          new pbr::FileDescriptor[] { pbr::FileDescriptor.DescriptorProtoFileDescriptor, },
+          new pbr::GeneratedClrTypeInfo(null, null));
+    }
+    #endregion
+
+  }
+}
+
+#endregion Designer generated code

csharp/src/Google.Protobuf/ExtensionRegistry.cs

@@ -90,7 +90,9 @@ namespace Google.Protobuf
             ProtoPreconditions.CheckNotNull(extensions, nameof(extensions));
 
             foreach (var extension in extensions)
+            {
                 Add(extension);
+            }
         }
 
         /// <summary>
@@ -120,9 +122,13 @@ namespace Google.Protobuf
         {
             ProtoPreconditions.CheckNotNull(array, nameof(array));
             if (arrayIndex < 0 || arrayIndex >= array.Length)
+            {
                 throw new ArgumentOutOfRangeException(nameof(arrayIndex));
+            }
             if (array.Length - arrayIndex < Count)
+            {
                 throw new ArgumentException("The provided array is shorter than the number of elements in the registry");
+            }
 
             for (int i = 0; i < array.Length; i++)
             {

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

@@ -109,7 +109,9 @@ namespace Google.Protobuf.Reflection
 
                 IList<FieldDescriptor> _;
                 if (!declarationOrder.TryGetValue(descriptor.ExtendeeType, out _))
+                {
                     declarationOrder.Add(descriptor.ExtendeeType, new List<FieldDescriptor>());
+                }
 
                 declarationOrder[descriptor.ExtendeeType].Add(descriptor);
             }

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

@@ -282,6 +282,9 @@ namespace Google.Protobuf.Reflection
 
         /// <summary>
         /// Unmodifiable list of top-level extensions declared in this file.
+        /// Note that some extensions may be incomplete (FieldDescriptor.Extension may be null)
+        /// if this descriptor was generated using a version of protoc that did not fully
+        /// support extensions in C#.
         /// </summary>
         public ExtensionCollection Extensions { get; }
 
@@ -456,6 +459,7 @@ namespace Google.Protobuf.Reflection
         {
             return descriptor.Extensions.UnorderedExtensions
                 .Select(s => s.Extension)
+                .Where(e => e != null)
                 .Concat(descriptor.Dependencies.Concat(descriptor.PublicDependencies).SelectMany(GetAllDependedExtensions))
                 .Concat(descriptor.MessageTypes.SelectMany(GetAllDependedExtensionsFromMessage));
         }
@@ -464,6 +468,7 @@ namespace Google.Protobuf.Reflection
         {
             return descriptor.Extensions.UnorderedExtensions
                 .Select(s => s.Extension)
+                .Where(e => e != null)
                 .Concat(descriptor.NestedTypes.SelectMany(GetAllDependedExtensionsFromMessage));
         }
 

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

@@ -215,7 +215,10 @@ namespace Google.Protobuf.Reflection
         public FieldCollection Fields { get; }
 
         /// <summary>
-        /// An unmodifiable list of extensions defined in this message's scrope
+        /// An unmodifiable list of extensions defined in this message's scope.
+        /// Note that some extensions may be incomplete (FieldDescriptor.Extension may be null)
+        /// if they are declared in a file generated using a version of protoc that did not fully
+        /// support extensions in C#.
         /// </summary>
         public ExtensionCollection Extensions { get; }