From e613ba6980cfe380510f085190949ac46e9e4b15 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Wed, 30 Oct 2019 09:13:04 +0000 Subject: [PATCH 1/3] Add braces around single-statement if/foreach --- csharp/src/Google.Protobuf/ExtensionRegistry.cs | 6 ++++++ .../src/Google.Protobuf/Reflection/ExtensionCollection.cs | 2 ++ 2 files changed, 8 insertions(+) diff --git a/csharp/src/Google.Protobuf/ExtensionRegistry.cs b/csharp/src/Google.Protobuf/ExtensionRegistry.cs index b8eee24176..2318e449e6 100644 --- a/csharp/src/Google.Protobuf/ExtensionRegistry.cs +++ b/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); + } } /// @@ -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++) { diff --git a/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs b/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs index 1b09bdde7f..38e33d7ff2 100644 --- a/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs +++ b/csharp/src/Google.Protobuf/Reflection/ExtensionCollection.cs @@ -109,7 +109,9 @@ namespace Google.Protobuf.Reflection IList _; if (!declarationOrder.TryGetValue(descriptor.ExtendeeType, out _)) + { declarationOrder.Add(descriptor.ExtendeeType, new List()); + } declarationOrder[descriptor.ExtendeeType].Add(descriptor); } From 7581fd5ea62ddbc3777523ffe681b9b1975917cc Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Wed, 30 Oct 2019 09:46:24 +0000 Subject: [PATCH 2/3] Tests for issue 6822 (The implementation to fix this is in the following commit.) --- Makefile.am | 2 + csharp/generate_protos.sh | 3 + csharp/protos/old_extensions1.proto | 52 ++++++ csharp/protos/old_extensions2.proto | 50 ++++++ .../Reflection/DescriptorsTest.cs | 18 +++ .../TestProtos/OldExtensions1.cs | 148 ++++++++++++++++++ .../TestProtos/OldExtensions2.cs | 40 +++++ 7 files changed, 313 insertions(+) create mode 100644 csharp/protos/old_extensions1.proto create mode 100644 csharp/protos/old_extensions2.proto create mode 100644 csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions1.cs create mode 100644 csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions2.cs diff --git a/Makefile.am b/Makefile.am index 62e8a782dd..4550ce8e95 100644 --- a/Makefile.am +++ b/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 \ diff --git a/csharp/generate_protos.sh b/csharp/generate_protos.sh index 06daa4e81c..3829d05e64 100755 --- a/csharp/generate_protos.sh +++ b/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 \ diff --git a/csharp/protos/old_extensions1.proto b/csharp/protos/old_extensions1.proto new file mode 100644 index 0000000000..58f6eb580c --- /dev/null +++ b/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 { +} diff --git a/csharp/protos/old_extensions2.proto b/csharp/protos/old_extensions2.proto new file mode 100644 index 0000000000..78f732a8ad --- /dev/null +++ b/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; +} \ No newline at end of file diff --git a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs b/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs index 264cbbe037..482db535e2 100644 --- a/csharp/src/Google.Protobuf.Test/Reflection/DescriptorsTest.cs +++ b/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); + } } } diff --git a/csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions1.cs b/csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions1.cs new file mode 100644 index 0000000000..3824f4c6ea --- /dev/null +++ b/csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions1.cs @@ -0,0 +1,148 @@ +// +// Generated by the protocol buffer compiler. DO NOT EDIT! +// source: old_extensions1.proto +// +#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 { + + /// Holder for reflection information generated from old_extensions1.proto + public static partial class OldExtensions1Reflection { + + #region Descriptor + /// File descriptor for old_extensions1.proto + 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 + /// + /// We don't use this message other than to get its descriptor. + /// + public sealed partial class TestMessage : pb::IMessage { + private static readonly pb::MessageParser _parser = new pb::MessageParser(() => new TestMessage()); + private pb::UnknownFieldSet _unknownFields; + [global::System.Diagnostics.DebuggerNonUserCodeAttribute] + public static pb::MessageParser 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 diff --git a/csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions2.cs b/csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions2.cs new file mode 100644 index 0000000000..265dd3ba58 --- /dev/null +++ b/csharp/src/Google.Protobuf.Test/TestProtos/OldExtensions2.cs @@ -0,0 +1,40 @@ +// +// Generated by the protocol buffer compiler. DO NOT EDIT! +// source: old_extensions2.proto +// +#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 { + + /// Holder for reflection information generated from old_extensions2.proto + public static partial class OldExtensions2Reflection { + + #region Descriptor + /// File descriptor for old_extensions2.proto + 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 From 9b5fdb09380aa28aae2080aa0f5936e4100ab9d1 Mon Sep 17 00:00:00 2001 From: Jon Skeet Date: Wed, 30 Oct 2019 09:47:45 +0000 Subject: [PATCH 3/3] Ignore incomplete extensions when building a FileDescriptor FileDescriptor construction uses an extension registry including extensions from imports. If these were created using an older version of protoc, the FieldDescriptor.Extension property may be null; we ignore such extensions rather than failing. --- csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs | 5 +++++ csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs | 5 ++++- 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs index 83849a2b25..79ba13b771 100644 --- a/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/FileDescriptor.cs @@ -282,6 +282,9 @@ namespace Google.Protobuf.Reflection /// /// 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#. /// 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)); } diff --git a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs index 944f6e88d2..510c079434 100644 --- a/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs +++ b/csharp/src/Google.Protobuf/Reflection/MessageDescriptor.cs @@ -215,7 +215,10 @@ namespace Google.Protobuf.Reflection public FieldCollection Fields { get; } /// - /// 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#. /// public ExtensionCollection Extensions { get; }