Merge pull request #6938 from ObsidianMinor/csharp/fix/6936 (#7188)

Fix latest ArgumentException for C# extensions

Co-authored-by: Jan Tattermusch <jtattermusch@users.noreply.github.com>

Makefile.am

@@ -68,6 +68,9 @@ csharp_EXTRA_DIST=                                                           \
   csharp/protos/map_unittest_proto3.proto                                    \
   csharp/protos/old_extensions1.proto                                        \
   csharp/protos/old_extensions2.proto                                        \
+  csharp/protos/unittest_issue6936_a.proto                                   \
+  csharp/protos/unittest_issue6936_b.proto                                   \
+  csharp/protos/unittest_issue6936_c.proto                                   \
   csharp/protos/unittest_custom_options_proto3.proto                         \
   csharp/protos/unittest_import_public_proto3.proto                          \
   csharp/protos/unittest_import_public.proto                                 \
@@ -128,6 +131,9 @@ csharp_EXTRA_DIST=                                                           \
   csharp/src/Google.Protobuf.Test/SampleNaNs.cs                              \
   csharp/src/Google.Protobuf.Test/TestCornerCases.cs                         \
   csharp/src/Google.Protobuf.Test.TestProtos/Google.Protobuf.Test.TestProtos.csproj \
+  csharp/src/Google.Protobuf.Test.TestProtos/UnittestIssue6936A.cs              \
+  csharp/src/Google.Protobuf.Test.TestProtos/UnittestIssue6936B.cs              \
+  csharp/src/Google.Protobuf.Test.TestProtos/UnittestIssue6936C.cs              \
   csharp/src/Google.Protobuf.Test.TestProtos/ForeignMessagePartial.cs           \
   csharp/src/Google.Protobuf.Test.TestProtos/MapUnittestProto3.cs               \
   csharp/src/Google.Protobuf.Test.TestProtos/OldExtensions1.cs                  \

csharp/generate_protos.sh

@@ -58,6 +58,9 @@ $PROTOC -Isrc -Icsharp/protos \
     csharp/protos/unittest.proto \
     csharp/protos/unittest_import.proto \
     csharp/protos/unittest_import_public.proto \
+    csharp/protos/unittest_issue6936_a.proto \
+    csharp/protos/unittest_issue6936_b.proto \
+    csharp/protos/unittest_issue6936_c.proto \
     src/google/protobuf/unittest_well_known_types.proto \
     src/google/protobuf/test_messages_proto3.proto \
     src/google/protobuf/test_messages_proto2.proto

csharp/protos/unittest_issue6936_a.proto

@@ -0,0 +1,15 @@
+syntax = "proto3";
+
+package unittest_issues;
+
+option csharp_namespace = "UnitTest.Issues.TestProtos";
+
+// This file is used as part of a unit test for issue 6936
+// We don't need to use it, we just have to import it in both 
+// "extensions_issue6936_b.proto" and "extensions_issue6936_c.proto"
+
+import "google/protobuf/descriptor.proto";
+
+extend google.protobuf.MessageOptions {
+    string opt = 50000;
+}

csharp/protos/unittest_issue6936_b.proto

@@ -0,0 +1,14 @@
+syntax = "proto3";
+
+import "unittest_issue6936_a.proto";
+
+package unittest_issues;
+
+option csharp_namespace = "UnitTest.Issues.TestProtos";
+
+// This file is used as part of a unit test for issue 6936
+// We don't need to use it, we just have to import it in "unittest_issue6936_c.proto"
+
+message Foo {
+    option (opt) = "foo";
+}

csharp/protos/unittest_issue6936_c.proto

@@ -0,0 +1,16 @@
+syntax = "proto3";
+
+import "unittest_issue6936_a.proto";
+import "unittest_issue6936_b.proto";
+
+package unittest_issues;
+
+option csharp_namespace = "UnitTest.Issues.TestProtos";
+
+// This file is used as part of a unit test for issue 6936
+// We don't need to use it, we just have to load it at runtime
+
+message Bar {
+    option (opt) = "bar";
+    Foo foo = 1;
+}

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

@@ -0,0 +1,46 @@
+// <auto-generated>
+//     Generated by the protocol buffer compiler.  DO NOT EDIT!
+//     source: unittest_issue6936_a.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 UnitTest.Issues.TestProtos {
+
+  /// <summary>Holder for reflection information generated from unittest_issue6936_a.proto</summary>
+  public static partial class UnittestIssue6936AReflection {
+
+    #region Descriptor
+    /// <summary>File descriptor for unittest_issue6936_a.proto</summary>
+    public static pbr::FileDescriptor Descriptor {
+      get { return descriptor; }
+    }
+    private static pbr::FileDescriptor descriptor;
+
+    static UnittestIssue6936AReflection() {
+      byte[] descriptorData = global::System.Convert.FromBase64String(
+          string.Concat(
+            "Chp1bml0dGVzdF9pc3N1ZTY5MzZfYS5wcm90bxIPdW5pdHRlc3RfaXNzdWVz",
+            "GiBnb29nbGUvcHJvdG9idWYvZGVzY3JpcHRvci5wcm90bzouCgNvcHQSHy5n",
+            "b29nbGUucHJvdG9idWYuTWVzc2FnZU9wdGlvbnMY0IYDIAEoCUIdqgIaVW5p",
+            "dFRlc3QuSXNzdWVzLlRlc3RQcm90b3NiBnByb3RvMw=="));
+      descriptor = pbr::FileDescriptor.FromGeneratedCode(descriptorData,
+          new pbr::FileDescriptor[] { global::Google.Protobuf.Reflection.DescriptorReflection.Descriptor, },
+          new pbr::GeneratedClrTypeInfo(null, new pb::Extension[] { UnittestIssue6936AExtensions.Opt }, null));
+    }
+    #endregion
+
+  }
+  /// <summary>Holder for extension identifiers generated from the top level of unittest_issue6936_a.proto</summary>
+  public static partial class UnittestIssue6936AExtensions {
+    public static readonly pb::Extension<global::Google.Protobuf.Reflection.MessageOptions, string> Opt =
+      new pb::Extension<global::Google.Protobuf.Reflection.MessageOptions, string>(50000, pb::FieldCodec.ForString(400002, ""));
+  }
+
+}
+
+#endregion Designer generated code

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

@@ -0,0 +1,145 @@
+// <auto-generated>
+//     Generated by the protocol buffer compiler.  DO NOT EDIT!
+//     source: unittest_issue6936_b.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 UnitTest.Issues.TestProtos {
+
+  /// <summary>Holder for reflection information generated from unittest_issue6936_b.proto</summary>
+  public static partial class UnittestIssue6936BReflection {
+
+    #region Descriptor
+    /// <summary>File descriptor for unittest_issue6936_b.proto</summary>
+    public static pbr::FileDescriptor Descriptor {
+      get { return descriptor; }
+    }
+    private static pbr::FileDescriptor descriptor;
+
+    static UnittestIssue6936BReflection() {
+      byte[] descriptorData = global::System.Convert.FromBase64String(
+          string.Concat(
+            "Chp1bml0dGVzdF9pc3N1ZTY5MzZfYi5wcm90bxIPdW5pdHRlc3RfaXNzdWVz",
+            "Ghp1bml0dGVzdF9pc3N1ZTY5MzZfYS5wcm90byIOCgNGb286B4K1GANmb29C",
+            "HaoCGlVuaXRUZXN0Lklzc3Vlcy5UZXN0UHJvdG9zYgZwcm90bzM="));
+      descriptor = pbr::FileDescriptor.FromGeneratedCode(descriptorData,
+          new pbr::FileDescriptor[] { global::UnitTest.Issues.TestProtos.UnittestIssue6936AReflection.Descriptor, },
+          new pbr::GeneratedClrTypeInfo(null, null, new pbr::GeneratedClrTypeInfo[] {
+            new pbr::GeneratedClrTypeInfo(typeof(global::UnitTest.Issues.TestProtos.Foo), global::UnitTest.Issues.TestProtos.Foo.Parser, null, null, null, null, null)
+          }));
+    }
+    #endregion
+
+  }
+  #region Messages
+  public sealed partial class Foo : pb::IMessage<Foo> {
+    private static readonly pb::MessageParser<Foo> _parser = new pb::MessageParser<Foo>(() => new Foo());
+    private pb::UnknownFieldSet _unknownFields;
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public static pb::MessageParser<Foo> Parser { get { return _parser; } }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public static pbr::MessageDescriptor Descriptor {
+      get { return global::UnitTest.Issues.TestProtos.UnittestIssue6936BReflection.Descriptor.MessageTypes[0]; }
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    pbr::MessageDescriptor pb::IMessage.Descriptor {
+      get { return Descriptor; }
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public Foo() {
+      OnConstruction();
+    }
+
+    partial void OnConstruction();
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public Foo(Foo other) : this() {
+      _unknownFields = pb::UnknownFieldSet.Clone(other._unknownFields);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public Foo Clone() {
+      return new Foo(this);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public override bool Equals(object other) {
+      return Equals(other as Foo);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public bool Equals(Foo 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(Foo 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/UnittestIssue6936C.cs

@@ -0,0 +1,181 @@
+// <auto-generated>
+//     Generated by the protocol buffer compiler.  DO NOT EDIT!
+//     source: unittest_issue6936_c.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 UnitTest.Issues.TestProtos {
+
+  /// <summary>Holder for reflection information generated from unittest_issue6936_c.proto</summary>
+  public static partial class UnittestIssue6936CReflection {
+
+    #region Descriptor
+    /// <summary>File descriptor for unittest_issue6936_c.proto</summary>
+    public static pbr::FileDescriptor Descriptor {
+      get { return descriptor; }
+    }
+    private static pbr::FileDescriptor descriptor;
+
+    static UnittestIssue6936CReflection() {
+      byte[] descriptorData = global::System.Convert.FromBase64String(
+          string.Concat(
+            "Chp1bml0dGVzdF9pc3N1ZTY5MzZfYy5wcm90bxIPdW5pdHRlc3RfaXNzdWVz",
+            "Ghp1bml0dGVzdF9pc3N1ZTY5MzZfYS5wcm90bxoadW5pdHRlc3RfaXNzdWU2",
+            "OTM2X2IucHJvdG8iMQoDQmFyEiEKA2ZvbxgBIAEoCzIULnVuaXR0ZXN0X2lz",
+            "c3Vlcy5Gb286B4K1GANiYXJCHaoCGlVuaXRUZXN0Lklzc3Vlcy5UZXN0UHJv",
+            "dG9zYgZwcm90bzM="));
+      descriptor = pbr::FileDescriptor.FromGeneratedCode(descriptorData,
+          new pbr::FileDescriptor[] { global::UnitTest.Issues.TestProtos.UnittestIssue6936AReflection.Descriptor, global::UnitTest.Issues.TestProtos.UnittestIssue6936BReflection.Descriptor, },
+          new pbr::GeneratedClrTypeInfo(null, null, new pbr::GeneratedClrTypeInfo[] {
+            new pbr::GeneratedClrTypeInfo(typeof(global::UnitTest.Issues.TestProtos.Bar), global::UnitTest.Issues.TestProtos.Bar.Parser, new[]{ "Foo" }, null, null, null, null)
+          }));
+    }
+    #endregion
+
+  }
+  #region Messages
+  public sealed partial class Bar : pb::IMessage<Bar> {
+    private static readonly pb::MessageParser<Bar> _parser = new pb::MessageParser<Bar>(() => new Bar());
+    private pb::UnknownFieldSet _unknownFields;
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public static pb::MessageParser<Bar> Parser { get { return _parser; } }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public static pbr::MessageDescriptor Descriptor {
+      get { return global::UnitTest.Issues.TestProtos.UnittestIssue6936CReflection.Descriptor.MessageTypes[0]; }
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    pbr::MessageDescriptor pb::IMessage.Descriptor {
+      get { return Descriptor; }
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public Bar() {
+      OnConstruction();
+    }
+
+    partial void OnConstruction();
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public Bar(Bar other) : this() {
+      foo_ = other.foo_ != null ? other.foo_.Clone() : null;
+      _unknownFields = pb::UnknownFieldSet.Clone(other._unknownFields);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public Bar Clone() {
+      return new Bar(this);
+    }
+
+    /// <summary>Field number for the "foo" field.</summary>
+    public const int FooFieldNumber = 1;
+    private global::UnitTest.Issues.TestProtos.Foo foo_;
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public global::UnitTest.Issues.TestProtos.Foo Foo {
+      get { return foo_; }
+      set {
+        foo_ = value;
+      }
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public override bool Equals(object other) {
+      return Equals(other as Bar);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public bool Equals(Bar other) {
+      if (ReferenceEquals(other, null)) {
+        return false;
+      }
+      if (ReferenceEquals(other, this)) {
+        return true;
+      }
+      if (!object.Equals(Foo, other.Foo)) return false;
+      return Equals(_unknownFields, other._unknownFields);
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public override int GetHashCode() {
+      int hash = 1;
+      if (foo_ != null) hash ^= Foo.GetHashCode();
+      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 (foo_ != null) {
+        output.WriteRawTag(10);
+        output.WriteMessage(Foo);
+      }
+      if (_unknownFields != null) {
+        _unknownFields.WriteTo(output);
+      }
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public int CalculateSize() {
+      int size = 0;
+      if (foo_ != null) {
+        size += 1 + pb::CodedOutputStream.ComputeMessageSize(Foo);
+      }
+      if (_unknownFields != null) {
+        size += _unknownFields.CalculateSize();
+      }
+      return size;
+    }
+
+    [global::System.Diagnostics.DebuggerNonUserCodeAttribute]
+    public void MergeFrom(Bar other) {
+      if (other == null) {
+        return;
+      }
+      if (other.foo_ != null) {
+        if (foo_ == null) {
+          Foo = new global::UnitTest.Issues.TestProtos.Foo();
+        }
+        Foo.MergeFrom(other.Foo);
+      }
+      _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;
+          case 10: {
+            if (foo_ == null) {
+              Foo = new global::UnitTest.Issues.TestProtos.Foo();
+            }
+            input.ReadMessage(Foo);
+            break;
+          }
+        }
+      }
+    }
+
+  }
+
+  #endregion
+
+}
+
+#endregion Designer generated code

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

@@ -193,6 +193,16 @@ namespace Google.Protobuf.Test.Reflection
             Assert.NotNull(TestAllTypes.Descriptor.Oneofs[0].CustomOptions);
         }
 
+        [Test]
+        public void MultipleImportOfSameFileWithExtension()
+        {
+            var descriptor = UnittestIssue6936CReflection.Descriptor;
+            var foo = Foo.Descriptor;
+            var bar = Bar.Descriptor;
+            AssertOption("foo", foo.CustomOptions.TryGetString, UnittestIssue6936AExtensions.Opt, foo.GetOption);
+            AssertOption("bar", bar.CustomOptions.TryGetString, UnittestIssue6936AExtensions.Opt, bar.GetOption);
+        }
+
         private void AssertOption<T, D>(T expected, OptionFetcher<T> fetcher, Extension<D, T> extension, Func<Extension<D, T>, T> descriptorOptionFetcher) where D : IExtendableMessage<D>
         {
             T customOptionsValue;

csharp/src/Google.Protobuf/ExtensionRegistry.cs

@@ -42,6 +42,19 @@ namespace Google.Protobuf
     /// </summary>
     public sealed class ExtensionRegistry : ICollection<Extension>, IDeepCloneable<ExtensionRegistry>
     {
+        internal sealed class ExtensionComparer : IEqualityComparer<Extension>
+        {
+            public bool Equals(Extension a, Extension b)
+            {
+                return new ObjectIntPair<Type>(a.TargetType, a.FieldNumber).Equals(new ObjectIntPair<Type>(b.TargetType, b.FieldNumber));
+            }
+            public int GetHashCode(Extension a)
+            {
+                return new ObjectIntPair<Type>(a.TargetType, a.FieldNumber).GetHashCode();
+            }
+
+            internal static ExtensionComparer Instance = new ExtensionComparer();
+        }
         private IDictionary<ObjectIntPair<Type>, Extension> extensions;
 
         /// <summary>

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

@@ -422,7 +422,8 @@ namespace Google.Protobuf.Reflection
             GeneratedClrTypeInfo generatedCodeInfo)
         {
             ExtensionRegistry registry = new ExtensionRegistry();
-            AddAllExtensions(dependencies, generatedCodeInfo, registry);
+            registry.AddRange(GetAllExtensions(dependencies, generatedCodeInfo));
+    
             FileDescriptorProto proto;
             try
             {
@@ -445,9 +446,9 @@ namespace Google.Protobuf.Reflection
             }
         }
 
-        private static void AddAllExtensions(FileDescriptor[] dependencies, GeneratedClrTypeInfo generatedInfo, ExtensionRegistry registry)
+        private static IEnumerable<Extension> GetAllExtensions(FileDescriptor[] dependencies, GeneratedClrTypeInfo generatedInfo)
         {
-            registry.AddRange(dependencies.SelectMany(GetAllDependedExtensions).Concat(GetAllGeneratedExtensions(generatedInfo)).ToArray());
+            return dependencies.SelectMany(GetAllDependedExtensions).Distinct(ExtensionRegistry.ExtensionComparer.Instance).Concat(GetAllGeneratedExtensions(generatedInfo));
         }
 
         private static IEnumerable<Extension> GetAllGeneratedExtensions(GeneratedClrTypeInfo generated)