Sync from Piper @306496510

PROTOBUF_SYNC_PIPER

csharp/src/Google.Protobuf.Benchmarks/SerializationConfig.cs → csharp/src/Google.Protobuf.Benchmarks/BenchmarkDatasetConfig.cs

@@ -43,20 +43,20 @@ namespace Google.Protobuf.Benchmarks
     /// <summary>
     /// The configuration for a single serialization test, loaded from a dataset.
     /// </summary>
-    public class SerializationConfig
+    public class BenchmarkDatasetConfig
     {
         private static readonly Dictionary<string, MessageParser> parsersByMessageName = 
-            typeof(SerializationBenchmark).Assembly.GetTypes()
+            typeof(GoogleMessageBenchmark).Assembly.GetTypes()
                 .Where(t => typeof(IMessage).IsAssignableFrom(t))
                 .ToDictionary(
                     t => ((MessageDescriptor) t.GetProperty("Descriptor", BindingFlags.Static | BindingFlags.Public).GetValue(null)).FullName,
                     t => ((MessageParser) t.GetProperty("Parser", BindingFlags.Static | BindingFlags.Public).GetValue(null)));
 
         public MessageParser Parser { get; }
-        public IEnumerable<ByteString> Payloads { get; }
+        public List<byte[]> Payloads { get; }
         public string Name { get; }
 
-        public SerializationConfig(string resource)
+        public BenchmarkDatasetConfig(string resource, string shortName = null)
         {
             var data = LoadData(resource);
             var dataset = BenchmarkDataset.Parser.ParseFrom(data);
@@ -66,13 +66,13 @@ namespace Google.Protobuf.Benchmarks
                 throw new ArgumentException($"No parser for message {dataset.MessageName} in this assembly");
             }
             Parser = parser;
-            Payloads = dataset.Payload;
-            Name = dataset.Name;
+            Payloads = new List<byte[]>(dataset.Payload.Select(p => p.ToByteArray()));
+            Name = shortName ?? dataset.Name;
         }
 
         private static byte[] LoadData(string resource)
         {
-            using (var stream = typeof(SerializationBenchmark).Assembly.GetManifestResourceStream($"Google.Protobuf.Benchmarks.{resource}"))
+            using (var stream = typeof(GoogleMessageBenchmark).Assembly.GetManifestResourceStream($"Google.Protobuf.Benchmarks.{resource}"))
             {
                 if (stream == null)
                 {

csharp/src/Google.Protobuf.Benchmarks/SerializationBenchmark.cs → csharp/src/Google.Protobuf.Benchmarks/GoogleMessageBenchmark.cs

@@ -38,23 +38,27 @@ using System.Linq;
 namespace Google.Protobuf.Benchmarks
 {
     /// <summary>
-    /// Benchmark for serializing (to a MemoryStream) and deserializing (from a ByteString).
+    /// Benchmark for serializing and deserializing of standard datasets that are also
+    /// measured by benchmarks in other languages.
     /// Over time we may wish to test the various different approaches to serialization and deserialization separately.
+    /// See https://github.com/protocolbuffers/protobuf/blob/master/benchmarks/README.md
+    /// See https://github.com/protocolbuffers/protobuf/blob/master/docs/performance.md
     /// </summary>
     [MemoryDiagnoser]
-    public class SerializationBenchmark
+    public class GoogleMessageBenchmark
     {
         /// <summary>
-        /// All the configurations to be tested. Add more datasets to the array as they're available.
+        /// All the datasets to be tested. Add more datasets to the array as they're available.
         /// (When C# supports proto2, this will increase significantly.)
         /// </summary>
-        public static SerializationConfig[] Configurations => new[]
+        public static BenchmarkDatasetConfig[] DatasetConfigurations => new[]
         {
-            new SerializationConfig("dataset.google_message1_proto3.pb")
+            // short name is specified to make results table more readable
+            new BenchmarkDatasetConfig("dataset.google_message1_proto3.pb", "goog_msg1_proto3")
         };
 
-        [ParamsSource(nameof(Configurations))]
-        public SerializationConfig Configuration { get; set; }
+        [ParamsSource(nameof(DatasetConfigurations))]
+        public BenchmarkDatasetConfig Dataset { get; set; }
 
         private MessageParser parser;
         /// <summary>
@@ -67,8 +71,8 @@ namespace Google.Protobuf.Benchmarks
         [GlobalSetup]
         public void GlobalSetup()
         {
-            parser = Configuration.Parser;
-            subTests = Configuration.Payloads.Select(p => new SubTest(p, parser.ParseFrom(p))).ToList();
+            parser = Dataset.Parser;
+            subTests = Dataset.Payloads.Select(p => new SubTest(p, parser.ParseFrom(p))).ToList();
         }
 
         [Benchmark]
@@ -78,7 +82,7 @@ namespace Google.Protobuf.Benchmarks
         public void ToByteArray() => subTests.ForEach(item => item.ToByteArray());
 
         [Benchmark]
-        public void ParseFromByteString() => subTests.ForEach(item => item.ParseFromByteString(parser));
+        public void ParseFromByteArray() => subTests.ForEach(item => item.ParseFromByteArray(parser));
 
         [Benchmark]
         public void ParseFromStream() => subTests.ForEach(item => item.ParseFromStream(parser));
@@ -87,13 +91,13 @@ namespace Google.Protobuf.Benchmarks
         {
             private readonly Stream destinationStream;
             private readonly Stream sourceStream;
-            private readonly ByteString data;
+            private readonly byte[] data;
             private readonly IMessage message;
 
-            public SubTest(ByteString data, IMessage message)
+            public SubTest(byte[] data, IMessage message)
             {
                 destinationStream = new MemoryStream(data.Length);
-                sourceStream = new MemoryStream(data.ToByteArray());
+                sourceStream = new MemoryStream(data);
                 this.data = data;
                 this.message = message;
             }
@@ -108,7 +112,7 @@ namespace Google.Protobuf.Benchmarks
 
             public void ToByteArray() => message.ToByteArray();
 
-            public void ParseFromByteString(MessageParser parser) => parser.ParseFrom(data);
+            public void ParseFromByteArray(MessageParser parser) => parser.ParseFrom(data);
 
             public void ParseFromStream(MessageParser parser)
             {

csharp/src/Google.Protobuf.Benchmarks/ParseMessagesBenchmark.cs

@@ -0,0 +1,170 @@
+#region Copyright notice and license
+// Protocol Buffers - Google's data interchange format
+// Copyright 2019 Google Inc.  All rights reserved.
+// https://github.com/protocolbuffers/protobuf
+//
+// 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 BenchmarkDotNet.Attributes;
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Buffers;
+using Google.Protobuf.WellKnownTypes;
+
+namespace Google.Protobuf.Benchmarks
+{
+    /// <summary>
+    /// Benchmark that tests parsing performance for various messages.
+    /// </summary>
+    [MemoryDiagnoser]
+    public class ParseMessagesBenchmark
+    {
+        const int MaxMessages = 100;
+
+        SubTest manyWrapperFieldsTest = new SubTest(CreateManyWrapperFieldsMessage(), ManyWrapperFieldsMessage.Parser, () => new ManyWrapperFieldsMessage(), MaxMessages);
+        SubTest manyPrimitiveFieldsTest = new SubTest(CreateManyPrimitiveFieldsMessage(), ManyPrimitiveFieldsMessage.Parser, () => new ManyPrimitiveFieldsMessage(), MaxMessages);
+        SubTest emptyMessageTest = new SubTest(new Empty(), Empty.Parser, () => new Empty(), MaxMessages);
+
+        public IEnumerable<int> MessageCountValues => new[] { 10, 100 };
+
+        [GlobalSetup]
+        public void GlobalSetup()
+        {
+        }
+
+        [Benchmark]
+        public IMessage ManyWrapperFieldsMessage_ParseFromByteArray()
+        {
+            return manyWrapperFieldsTest.ParseFromByteArray();
+        }
+
+        [Benchmark]
+        public IMessage ManyPrimitiveFieldsMessage_ParseFromByteArray()
+        {
+            return manyPrimitiveFieldsTest.ParseFromByteArray();
+        }
+
+        [Benchmark]
+        public IMessage EmptyMessage_ParseFromByteArray()
+        {
+            return emptyMessageTest.ParseFromByteArray();
+        }
+
+        [Benchmark]
+        [ArgumentsSource(nameof(MessageCountValues))]
+        public void ManyWrapperFieldsMessage_ParseDelimitedMessagesFromByteArray(int messageCount)
+        {
+            manyWrapperFieldsTest.ParseDelimitedMessagesFromByteArray(messageCount);
+        }
+
+        [Benchmark]
+        [ArgumentsSource(nameof(MessageCountValues))]
+        public void ManyPrimitiveFieldsMessage_ParseDelimitedMessagesFromByteArray(int messageCount)
+        {
+            manyPrimitiveFieldsTest.ParseDelimitedMessagesFromByteArray(messageCount);
+        }
+
+        private static ManyWrapperFieldsMessage CreateManyWrapperFieldsMessage()
+        {
+            // Example data match data of an internal benchmarks
+            return new ManyWrapperFieldsMessage()
+            {
+                Int64Field19 = 123,
+                Int64Field37 = 1000032,
+                Int64Field26 = 3453524500,
+                DoubleField79 = 1.2,
+                DoubleField25 = 234,
+                DoubleField9 = 123.3,
+                DoubleField28 = 23,
+                DoubleField7 = 234,
+                DoubleField50 = 2.45
+            };
+        }
+
+        private static ManyPrimitiveFieldsMessage CreateManyPrimitiveFieldsMessage()
+        {
+            // Example data match data of an internal benchmarks
+            return new ManyPrimitiveFieldsMessage()
+            {
+                Int64Field19 = 123,
+                Int64Field37 = 1000032,
+                Int64Field26 = 3453524500,
+                DoubleField79 = 1.2,
+                DoubleField25 = 234,
+                DoubleField9 = 123.3,
+                DoubleField28 = 23,
+                DoubleField7 = 234,
+                DoubleField50 = 2.45
+            };
+        }
+
+        private class SubTest
+        {
+            private readonly IMessage message;
+            private readonly MessageParser parser;
+            private readonly Func<IMessage> factory;
+            private readonly byte[] data;
+            private readonly byte[] multipleMessagesData;
+
+            public SubTest(IMessage message, MessageParser parser, Func<IMessage> factory, int maxMessageCount)
+            {
+                this.message = message;
+                this.parser = parser;
+                this.factory = factory;
+                this.data = message.ToByteArray();
+                this.multipleMessagesData = CreateBufferWithMultipleMessages(message, maxMessageCount);
+            }
+
+            public IMessage ParseFromByteArray() => parser.ParseFrom(data);
+
+            public void ParseDelimitedMessagesFromByteArray(int messageCount)
+            {
+                var input = new CodedInputStream(multipleMessagesData);
+                for (int i = 0; i < messageCount; i++)
+                {
+                    var msg = factory();
+                    input.ReadMessage(msg);
+                }
+            }
+
+            private static byte[] CreateBufferWithMultipleMessages(IMessage msg, int msgCount)
+            {
+                var ms = new MemoryStream();
+                var cos = new CodedOutputStream(ms);
+                for (int i = 0; i < msgCount; i++)
+                {
+                    cos.WriteMessage(msg);
+                }
+                cos.Flush();
+                return ms.ToArray();
+            }
+        }
+    }
+}

csharp/src/Google.Protobuf.Benchmarks/WrapperBenchmark.cs

@@ -1,102 +0,0 @@
-#region Copyright notice and license
-// Protocol Buffers - Google's data interchange format
-// Copyright 2019 Google Inc.  All rights reserved.
-// https://github.com/protocolbuffers/protobuf
-//
-// 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 BenchmarkDotNet.Attributes;
-using System.Collections.Generic;
-using System.IO;
-using System.Linq;
-
-namespace Google.Protobuf.Benchmarks
-{
-    /// <summary>
-    /// Benchmark that tests serialization/deserialization of wrapper fields.
-    /// </summary>
-    [MemoryDiagnoser]
-    public class WrapperBenchmark
-    {
-        byte[] manyWrapperFieldsData;
-        byte[] manyPrimitiveFieldsData;
-
-        [GlobalSetup]
-        public void GlobalSetup()
-        {
-            manyWrapperFieldsData = CreateManyWrapperFieldsMessage().ToByteArray();
-            manyPrimitiveFieldsData = CreateManyPrimitiveFieldsMessage().ToByteArray();
-        }
-
-        [Benchmark]
-        public ManyWrapperFieldsMessage ParseWrapperFields()
-        {
-            return ManyWrapperFieldsMessage.Parser.ParseFrom(manyWrapperFieldsData);
-        }
-
-        [Benchmark]
-        public ManyPrimitiveFieldsMessage ParsePrimitiveFields()
-        {
-            return ManyPrimitiveFieldsMessage.Parser.ParseFrom(manyPrimitiveFieldsData);
-        }
-
-        private static ManyWrapperFieldsMessage CreateManyWrapperFieldsMessage()
-        {
-            // Example data match data of an internal benchmarks
-            return new ManyWrapperFieldsMessage()
-            {
-                Int64Field19 = 123,
-                Int64Field37 = 1000032,
-                Int64Field26 = 3453524500,
-                DoubleField79 = 1.2,
-                DoubleField25 = 234,
-                DoubleField9 = 123.3,
-                DoubleField28 = 23,
-                DoubleField7 = 234,
-                DoubleField50 = 2.45
-            };
-        }
-
-        private static ManyPrimitiveFieldsMessage CreateManyPrimitiveFieldsMessage()
-        {
-            // Example data match data of an internal benchmarks
-            return new ManyPrimitiveFieldsMessage()
-            {
-                Int64Field19 = 123,
-                Int64Field37 = 1000032,
-                Int64Field26 = 3453524500,
-                DoubleField79 = 1.2,
-                DoubleField25 = 234,
-                DoubleField9 = 123.3,
-                DoubleField28 = 23,
-                DoubleField7 = 234,
-                DoubleField50 = 2.45
-            };
-        }
-    }
-}

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

@@ -2074,10 +2074,21 @@ namespace Google.Protobuf.Reflection {
     /// If true, this is a proto3 "optional". When a proto3 field is optional, it
     /// tracks presence regardless of field type.
     ///
-    /// For message fields this doesn't create any semantic change, since
-    /// non-repeated message fields always track presence. However it still
+    /// When proto3_optional is true, this field must be belong to a oneof to
+    /// signal to old proto3 clients that presence is tracked for this field. This
+    /// oneof is known as a "synthetic" oneof, and this field must be its sole
+    /// member (each proto3 optional field gets its own synthetic oneof). Synthetic
+    /// oneofs exist in the descriptor only, and do not generate any API. Synthetic
+    /// oneofs must be ordered after all "real" oneofs.
+    ///
+    /// For message fields, proto3_optional doesn't create any semantic change,
+    /// since non-repeated message fields always track presence. However it still
     /// indicates the semantic detail of whether the user wrote "optional" or not.
-    /// This can be useful for round-tripping the .proto file.
+    /// This can be useful for round-tripping the .proto file. For consistency we
+    /// give message fields a synthetic oneof also, even though it is not required
+    /// to track presence. This is especially important because the parser can't
+    /// tell if a field is a message or an enum, so it must always create a
+    /// synthetic oneof.
     ///
     /// Proto2 optional fields do not set this flag, because they already indicate
     /// optional with `LABEL_OPTIONAL`.

objectivec/DevTools/full_mac_build.sh

@@ -290,12 +290,9 @@ if [[ "${DO_XCODE_IOS_TESTS}" == "yes" ]] ; then
       )
       ;;
     11.*)
+      # Dropped 32bit as Apple doesn't seem support the simulators either.
       XCODEBUILD_TEST_BASE_IOS+=(
-          -destination "platform=iOS Simulator,name=iPhone 4s,OS=8.1" # 32bit
           -destination "platform=iOS Simulator,name=iPhone 8,OS=latest" # 64bit
-          # 10.x also seems to often fail running destinations in parallel (with
-          # 32bit one include atleast)
-          -disable-concurrent-destination-testing
       )
       ;;
     * )

objectivec/Tests/GPBMessageTests+Runtime.m

@@ -252,7 +252,7 @@
     // build the selector, i.e. - repeatedInt32Array_Count
     SEL countSel = NSSelectorFromString(
         [NSString stringWithFormat:@"repeated%@Array_Count", name]);
-    XCTAssertTrue([Message2 instancesRespondToSelector:countSel], @"field: %@",
+    XCTAssertTrue([Message3 instancesRespondToSelector:countSel], @"field: %@",
                   name);
   }
 
@@ -264,9 +264,9 @@
         NSSelectorFromString([NSString stringWithFormat:@"hasOneof%@", name]);
     SEL setHasSel = NSSelectorFromString(
         [NSString stringWithFormat:@"setHasOneof%@:", name]);
-    XCTAssertFalse([Message2 instancesRespondToSelector:hasSel], @"field: %@",
+    XCTAssertFalse([Message3 instancesRespondToSelector:hasSel], @"field: %@",
                    name);
-    XCTAssertFalse([Message2 instancesRespondToSelector:setHasSel],
+    XCTAssertFalse([Message3 instancesRespondToSelector:setHasSel],
                    @"field: %@", name);
   }
 
@@ -302,14 +302,14 @@
         [NSString stringWithFormat:@"hasMap%@", name]);
     SEL setHasSel = NSSelectorFromString(
         [NSString stringWithFormat:@"setHasMap%@:", name]);
-    XCTAssertFalse([Message2 instancesRespondToSelector:hasSel], @"field: %@",
+    XCTAssertFalse([Message3 instancesRespondToSelector:hasSel], @"field: %@",
                    name);
-    XCTAssertFalse([Message2 instancesRespondToSelector:setHasSel],
+    XCTAssertFalse([Message3 instancesRespondToSelector:setHasSel],
                    @"field: %@", name);
     // build the selector, i.e. - mapInt32Int32Count
     SEL countSel = NSSelectorFromString(
         [NSString stringWithFormat:@"map%@_Count", name]);
-    XCTAssertTrue([Message2 instancesRespondToSelector:countSel], @"field: %@",
+    XCTAssertTrue([Message3 instancesRespondToSelector:countSel], @"field: %@",
                    name);
   }
 }

objectivec/Tests/GPBSwiftTests.swift

@@ -355,7 +355,7 @@ class GPBBridgeTests: XCTestCase {
     msg.oneof = nil
     XCTAssertEqual(msg.oneof.optionalInt32, Int32(0))  // Default
     XCTAssertEqual(msg.oOneOfCase, Message2_O_OneOfCase.gpbUnsetOneOfCase)
-}
+  }
 
   func testProto3OneOfSupport() {
     let msg = Message3()

php/src/Google/Protobuf/Internal/FieldDescriptorProto.php

@@ -96,10 +96,20 @@ class FieldDescriptorProto extends \Google\Protobuf\Internal\Message
     /**
      * If true, this is a proto3 "optional". When a proto3 field is optional, it
      * tracks presence regardless of field type.
-     * For message fields this doesn't create any semantic change, since
-     * non-repeated message fields always track presence. However it still
+     * When proto3_optional is true, this field must be belong to a oneof to
+     * signal to old proto3 clients that presence is tracked for this field. This
+     * oneof is known as a "synthetic" oneof, and this field must be its sole
+     * member (each proto3 optional field gets its own synthetic oneof). Synthetic
+     * oneofs exist in the descriptor only, and do not generate any API. Synthetic
+     * oneofs must be ordered after all "real" oneofs.
+     * For message fields, proto3_optional doesn't create any semantic change,
+     * since non-repeated message fields always track presence. However it still
      * indicates the semantic detail of whether the user wrote "optional" or not.
-     * This can be useful for round-tripping the .proto file.
+     * This can be useful for round-tripping the .proto file. For consistency we
+     * give message fields a synthetic oneof also, even though it is not required
+     * to track presence. This is especially important because the parser can't
+     * tell if a field is a message or an enum, so it must always create a
+     * synthetic oneof.
      * Proto2 optional fields do not set this flag, because they already indicate
      * optional with `LABEL_OPTIONAL`.
      *
@@ -147,10 +157,20 @@ class FieldDescriptorProto extends \Google\Protobuf\Internal\Message
      *     @type bool $proto3_optional
      *           If true, this is a proto3 "optional". When a proto3 field is optional, it
      *           tracks presence regardless of field type.
-     *           For message fields this doesn't create any semantic change, since
-     *           non-repeated message fields always track presence. However it still
+     *           When proto3_optional is true, this field must be belong to a oneof to
+     *           signal to old proto3 clients that presence is tracked for this field. This
+     *           oneof is known as a "synthetic" oneof, and this field must be its sole
+     *           member (each proto3 optional field gets its own synthetic oneof). Synthetic
+     *           oneofs exist in the descriptor only, and do not generate any API. Synthetic
+     *           oneofs must be ordered after all "real" oneofs.
+     *           For message fields, proto3_optional doesn't create any semantic change,
+     *           since non-repeated message fields always track presence. However it still
      *           indicates the semantic detail of whether the user wrote "optional" or not.
-     *           This can be useful for round-tripping the .proto file.
+     *           This can be useful for round-tripping the .proto file. For consistency we
+     *           give message fields a synthetic oneof also, even though it is not required
+     *           to track presence. This is especially important because the parser can't
+     *           tell if a field is a message or an enum, so it must always create a
+     *           synthetic oneof.
      *           Proto2 optional fields do not set this flag, because they already indicate
      *           optional with `LABEL_OPTIONAL`.
      * }
@@ -495,10 +515,20 @@ class FieldDescriptorProto extends \Google\Protobuf\Internal\Message
     /**
      * If true, this is a proto3 "optional". When a proto3 field is optional, it
      * tracks presence regardless of field type.
-     * For message fields this doesn't create any semantic change, since
-     * non-repeated message fields always track presence. However it still
+     * When proto3_optional is true, this field must be belong to a oneof to
+     * signal to old proto3 clients that presence is tracked for this field. This
+     * oneof is known as a "synthetic" oneof, and this field must be its sole
+     * member (each proto3 optional field gets its own synthetic oneof). Synthetic
+     * oneofs exist in the descriptor only, and do not generate any API. Synthetic
+     * oneofs must be ordered after all "real" oneofs.
+     * For message fields, proto3_optional doesn't create any semantic change,
+     * since non-repeated message fields always track presence. However it still
      * indicates the semantic detail of whether the user wrote "optional" or not.
-     * This can be useful for round-tripping the .proto file.
+     * This can be useful for round-tripping the .proto file. For consistency we
+     * give message fields a synthetic oneof also, even though it is not required
+     * to track presence. This is especially important because the parser can't
+     * tell if a field is a message or an enum, so it must always create a
+     * synthetic oneof.
      * Proto2 optional fields do not set this flag, because they already indicate
      * optional with `LABEL_OPTIONAL`.
      *
@@ -513,10 +543,20 @@ class FieldDescriptorProto extends \Google\Protobuf\Internal\Message
     /**
      * If true, this is a proto3 "optional". When a proto3 field is optional, it
      * tracks presence regardless of field type.
-     * For message fields this doesn't create any semantic change, since
-     * non-repeated message fields always track presence. However it still
+     * When proto3_optional is true, this field must be belong to a oneof to
+     * signal to old proto3 clients that presence is tracked for this field. This
+     * oneof is known as a "synthetic" oneof, and this field must be its sole
+     * member (each proto3 optional field gets its own synthetic oneof). Synthetic
+     * oneofs exist in the descriptor only, and do not generate any API. Synthetic
+     * oneofs must be ordered after all "real" oneofs.
+     * For message fields, proto3_optional doesn't create any semantic change,
+     * since non-repeated message fields always track presence. However it still
      * indicates the semantic detail of whether the user wrote "optional" or not.
-     * This can be useful for round-tripping the .proto file.
+     * This can be useful for round-tripping the .proto file. For consistency we
+     * give message fields a synthetic oneof also, even though it is not required
+     * to track presence. This is especially important because the parser can't
+     * tell if a field is a message or an enum, so it must always create a
+     * synthetic oneof.
      * Proto2 optional fields do not set this flag, because they already indicate
      * optional with `LABEL_OPTIONAL`.
      *

ruby/ext/google/protobuf_c/encode_decode.c

@@ -434,10 +434,8 @@ static void *startmap_handler(void *closure, const void *hd) {
 }
 
 static bool endmap_handler(void *closure, const void *hd) {
-  MessageHeader* msg = closure;
-  const map_handlerdata_t* mapdata = hd;
-  VALUE map_rb = DEREF(msg, mapdata->ofs, VALUE);
-  Map_set_frame(map_rb, Qnil);
+  map_parse_frame_t* frame = closure;
+  Map_set_frame(frame->map, Qnil);
   return true;
 }
 
@@ -1200,7 +1198,7 @@ static void putsubmsg(VALUE submsg, const upb_fielddef *f, upb_sink sink,
 
   upb_sink_startsubmsg(sink, getsel(f, UPB_HANDLER_STARTSUBMSG), &subsink);
   putmsg(submsg, subdesc, subsink, depth + 1, emit_defaults, is_json, true);
-  upb_sink_endsubmsg(sink, getsel(f, UPB_HANDLER_ENDSUBMSG));
+  upb_sink_endsubmsg(sink, subsink, getsel(f, UPB_HANDLER_ENDSUBMSG));
 }
 
 static void putary(VALUE ary, const upb_fielddef* f, upb_sink sink, int depth,
@@ -1345,7 +1343,7 @@ static void putmap(VALUE map, const upb_fielddef* f, upb_sink sink, int depth,
                    entry_sink, emit_defaults, is_json);
 
     upb_sink_endmsg(entry_sink, &status);
-    upb_sink_endsubmsg(subsink, getsel(f, UPB_HANDLER_ENDSUBMSG));
+    upb_sink_endsubmsg(subsink, entry_sink, getsel(f, UPB_HANDLER_ENDSUBMSG));
   }
 
   upb_sink_endseq(sink, getsel(f, UPB_HANDLER_ENDSEQ));

ruby/ext/google/protobuf_c/map.c

@@ -100,11 +100,11 @@ static VALUE table_key(Map* self, VALUE key,
   return key;
 }
 
-static VALUE table_key_to_ruby(Map* self, const char* buf, size_t length) {
+static VALUE table_key_to_ruby(Map* self, upb_strview key) {
   switch (self->key_type) {
     case UPB_TYPE_BYTES:
     case UPB_TYPE_STRING: {
-      VALUE ret = rb_str_new(buf, length);
+      VALUE ret = rb_str_new(key.data, key.size);
       rb_enc_associate(ret,
                        (self->key_type == UPB_TYPE_BYTES) ?
                        kRubyString8bitEncoding : kRubyStringUtf8Encoding);
@@ -116,7 +116,7 @@ static VALUE table_key_to_ruby(Map* self, const char* buf, size_t length) {
     case UPB_TYPE_INT64:
     case UPB_TYPE_UINT32:
     case UPB_TYPE_UINT64:
-      return native_slot_get(self->key_type, Qnil, buf);
+      return native_slot_get(self->key_type, Qnil, key.data);
 
     default:
       assert(false);
@@ -289,9 +289,7 @@ VALUE Map_each(VALUE _self) {
   for (upb_strtable_begin(&it, &self->table);
        !upb_strtable_done(&it);
        upb_strtable_next(&it)) {
-
-    VALUE key = table_key_to_ruby(
-        self, upb_strtable_iter_key(&it), upb_strtable_iter_keylength(&it));
+    VALUE key = table_key_to_ruby(self, upb_strtable_iter_key(&it));
 
     upb_value v = upb_strtable_iter_value(&it);
     void* mem = value_memory(&v);
@@ -319,9 +317,7 @@ VALUE Map_keys(VALUE _self) {
   for (upb_strtable_begin(&it, &self->table);
        !upb_strtable_done(&it);
        upb_strtable_next(&it)) {
-
-    VALUE key = table_key_to_ruby(
-        self, upb_strtable_iter_key(&it), upb_strtable_iter_keylength(&it));
+    VALUE key = table_key_to_ruby(self, upb_strtable_iter_key(&it));
 
     rb_ary_push(ret, key);
   }
@@ -526,17 +522,14 @@ VALUE Map_dup(VALUE _self) {
   for (upb_strtable_begin(&it, &self->table);
        !upb_strtable_done(&it);
        upb_strtable_next(&it)) {
-
+    upb_strview k = upb_strtable_iter_key(&it);
     upb_value v = upb_strtable_iter_value(&it);
     void* mem = value_memory(&v);
     upb_value dup;
     void* dup_mem = value_memory(&dup);
     native_slot_dup(self->value_type, dup_mem, mem);
 
-    if (!upb_strtable_insert2(&new_self->table,
-                              upb_strtable_iter_key(&it),
-                              upb_strtable_iter_keylength(&it),
-                              dup)) {
+    if (!upb_strtable_insert2(&new_self->table, k.data, k.size, dup)) {
       rb_raise(rb_eRuntimeError, "Error inserting value into new table");
     }
   }
@@ -554,7 +547,7 @@ VALUE Map_deep_copy(VALUE _self) {
   for (upb_strtable_begin(&it, &self->table);
        !upb_strtable_done(&it);
        upb_strtable_next(&it)) {
-
+    upb_strview k = upb_strtable_iter_key(&it);
     upb_value v = upb_strtable_iter_value(&it);
     void* mem = value_memory(&v);
     upb_value dup;
@@ -562,10 +555,7 @@ VALUE Map_deep_copy(VALUE _self) {
     native_slot_deep_copy(self->value_type, self->value_type_class, dup_mem,
                           mem);
 
-    if (!upb_strtable_insert2(&new_self->table,
-                              upb_strtable_iter_key(&it),
-                              upb_strtable_iter_keylength(&it),
-                              dup)) {
+    if (!upb_strtable_insert2(&new_self->table, k.data, k.size, dup)) {
       rb_raise(rb_eRuntimeError, "Error inserting value into new table");
     }
   }
@@ -618,16 +608,13 @@ VALUE Map_eq(VALUE _self, VALUE _other) {
   for (upb_strtable_begin(&it, &self->table);
        !upb_strtable_done(&it);
        upb_strtable_next(&it)) {
-
+    upb_strview k = upb_strtable_iter_key(&it);
     upb_value v = upb_strtable_iter_value(&it);
     void* mem = value_memory(&v);
     upb_value other_v;
     void* other_mem = value_memory(&other_v);
 
-    if (!upb_strtable_lookup2(&other->table,
-                              upb_strtable_iter_key(&it),
-                              upb_strtable_iter_keylength(&it),
-                              &other_v)) {
+    if (!upb_strtable_lookup2(&other->table, k.data, k.size, &other_v)) {
       // Not present in other map.
       return Qfalse;
     }
@@ -655,11 +642,9 @@ VALUE Map_hash(VALUE _self) {
   VALUE hash_sym = rb_intern("hash");
 
   upb_strtable_iter it;
-  for (upb_strtable_begin(&it, &self->table);
-       !upb_strtable_done(&it);
+  for (upb_strtable_begin(&it, &self->table); !upb_strtable_done(&it);
        upb_strtable_next(&it)) {
-    VALUE key = table_key_to_ruby(
-        self, upb_strtable_iter_key(&it), upb_strtable_iter_keylength(&it));
+    VALUE key = table_key_to_ruby(self, upb_strtable_iter_key(&it));
 
     upb_value v = upb_strtable_iter_value(&it);
     void* mem = value_memory(&v);
@@ -687,8 +672,7 @@ VALUE Map_to_h(VALUE _self) {
   for (upb_strtable_begin(&it, &self->table);
        !upb_strtable_done(&it);
        upb_strtable_next(&it)) {
-    VALUE key = table_key_to_ruby(
-        self, upb_strtable_iter_key(&it), upb_strtable_iter_keylength(&it));
+    VALUE key = table_key_to_ruby(self, upb_strtable_iter_key(&it));
     upb_value v = upb_strtable_iter_value(&it);
     void* mem = value_memory(&v);
     VALUE value = native_slot_get(self->value_type,
@@ -720,11 +704,9 @@ VALUE Map_inspect(VALUE _self) {
   VALUE inspect_sym = rb_intern("inspect");
 
   upb_strtable_iter it;
-  for (upb_strtable_begin(&it, &self->table);
-       !upb_strtable_done(&it);
+  for (upb_strtable_begin(&it, &self->table); !upb_strtable_done(&it);
        upb_strtable_next(&it)) {
-    VALUE key = table_key_to_ruby(
-        self, upb_strtable_iter_key(&it), upb_strtable_iter_keylength(&it));
+    VALUE key = table_key_to_ruby(self, upb_strtable_iter_key(&it));
 
     upb_value v = upb_strtable_iter_value(&it);
     void* mem = value_memory(&v);
@@ -785,20 +767,15 @@ VALUE Map_merge_into_self(VALUE _self, VALUE hashmap) {
     for (upb_strtable_begin(&it, &other->table);
          !upb_strtable_done(&it);
          upb_strtable_next(&it)) {
+      upb_strview k = upb_strtable_iter_key(&it);
 
       // Replace any existing value by issuing a 'remove' operation first.
       upb_value v;
       upb_value oldv;
-      upb_strtable_remove2(&self->table,
-                           upb_strtable_iter_key(&it),
-                           upb_strtable_iter_keylength(&it),
-                           &oldv);
+      upb_strtable_remove2(&self->table, k.data, k.size, &oldv);
 
       v = upb_strtable_iter_value(&it);
-      upb_strtable_insert2(&self->table,
-                           upb_strtable_iter_key(&it),
-                           upb_strtable_iter_keylength(&it),
-                           v);
+      upb_strtable_insert2(&self->table, k.data, k.size, v);
     }
   } else {
     rb_raise(rb_eArgError, "Unknown type merging into Map");
@@ -822,10 +799,7 @@ bool Map_done(Map_iter* iter) {
 }
 
 VALUE Map_iter_key(Map_iter* iter) {
-  return table_key_to_ruby(
-      iter->self,
-      upb_strtable_iter_key(&iter->it),
-      upb_strtable_iter_keylength(&iter->it));
+  return table_key_to_ruby(iter->self, upb_strtable_iter_key(&iter->it));
 }
 
 VALUE Map_iter_value(Map_iter* iter) {

ruby/tests/common_tests.rb

@@ -1723,7 +1723,7 @@ module CommonTests
     m.duration = Rational(3, 2)
     assert_equal Google::Protobuf::Duration.new(seconds: 1, nanos: 500_000_000), m.duration
 
-    m.duration = BigDecimal.new("5")
+    m.duration = BigDecimal("5")
     assert_equal Google::Protobuf::Duration.new(seconds: 5, nanos: 0), m.duration
 
     m = proto_module::TimeMessage.new(duration: 1.1)

src/google/protobuf/compiler/cpp/cpp_field.cc

@@ -156,7 +156,7 @@ FieldGenerator* FieldGeneratorMap::MakeGenerator(
       default:
         return new RepeatedPrimitiveFieldGenerator(field, options);
     }
-  } else if (InRealOneof(field)) {
+  } else if (field->real_containing_oneof()) {
     switch (field->cpp_type()) {
       case FieldDescriptor::CPPTYPE_MESSAGE:
         return new MessageOneofFieldGenerator(field, options, scc_analyzer);

src/google/protobuf/compiler/cpp/cpp_helpers.cc

@@ -1151,7 +1151,7 @@ bool IsImplicitWeakField(const FieldDescriptor* field, const Options& options,
   return UsingImplicitWeakFields(field->file(), options) &&
          field->type() == FieldDescriptor::TYPE_MESSAGE &&
          !field->is_required() && !field->is_map() && !field->is_extension() &&
-         !InRealOneof(field) &&
+         !field->real_containing_oneof() &&
          !IsWellKnownMessage(field->message_type()->file()) &&
          field->message_type()->file()->name() !=
              "net/proto2/proto/descriptor.proto" &&
@@ -1474,7 +1474,7 @@ class ParseLoopGenerator {
         GetOptimizeFor(field->file(), options_) != FileOptions::LITE_RUNTIME &&
         // For now only use arena string for strings with empty defaults.
         field->default_value_string().empty() &&
-        !IsStringInlined(field, options_) && !InRealOneof(field) &&
+        !IsStringInlined(field, options_) && !field->real_containing_oneof() &&
         ctype == FieldOptions::STRING) {
       GenerateArenaString(field);
     } else {
@@ -1580,7 +1580,7 @@ class ParseLoopGenerator {
                       FieldName(field));
             }
           } else if (IsLazy(field, options_)) {
-            if (InRealOneof(field)) {
+            if (field->real_containing_oneof()) {
               format_(
                   "if (!_internal_has_$1$()) {\n"
                   "  clear_$2$();\n"
@@ -1684,7 +1684,7 @@ class ParseLoopGenerator {
                field->type() == FieldDescriptor::TYPE_SINT64)) {
             zigzag = "ZigZag";
           }
-          if (field->is_repeated() || InRealOneof(field)) {
+          if (field->is_repeated() || field->real_containing_oneof()) {
             std::string prefix = field->is_repeated() ? "add" : "set";
             format_(
                 "_internal_$1$_$2$($pi_ns$::ReadVarint$3$$4$(&ptr));\n"
@@ -1706,7 +1706,7 @@ class ParseLoopGenerator {
       case WireFormatLite::WIRETYPE_FIXED32:
       case WireFormatLite::WIRETYPE_FIXED64: {
         std::string type = PrimitiveTypeName(options_, field->cpp_type());
-        if (field->is_repeated() || InRealOneof(field)) {
+        if (field->is_repeated() || field->real_containing_oneof()) {
           std::string prefix = field->is_repeated() ? "add" : "set";
           format_(
               "_internal_$1$_$2$($pi_ns$::UnalignedLoad<$3$>(ptr));\n"

src/google/protobuf/compiler/cpp/cpp_helpers.h

@@ -444,23 +444,6 @@ inline bool HasHasbit(const FieldDescriptor* field) {
          !field->options().weak();
 }
 
-inline bool InRealOneof(const FieldDescriptor* field) {
-  return field->containing_oneof() &&
-         !field->containing_oneof()->is_synthetic();
-}
-
-// In practice all synthetic oneofs should be at the end of the list, but we
-// decline to depend on this for correctness of the function.
-inline int RealOneofCount(const Descriptor* descriptor) {
-  int count = 0;
-  for (int i = 0; i < descriptor->oneof_decl_count(); i++) {
-    if (!descriptor->oneof_decl(i)->is_synthetic()) {
-      count++;
-    }
-  }
-  return count;
-}
-
 // Returns true if 'enum' semantics are such that unknown values are preserved
 // in the enum field itself, rather than going to the UnknownFieldSet.
 inline bool HasPreservingUnknownEnumSemantics(const FieldDescriptor* field) {
@@ -886,14 +869,6 @@ struct OneOfRangeImpl {
     using value_type = const OneofDescriptor*;
     using difference_type = int;
 
-    explicit Iterator(const Descriptor* _descriptor)
-        : idx(-1), descriptor(_descriptor) {
-      Next();
-    }
-
-    Iterator(int _idx, const Descriptor* _descriptor)
-        : idx(_idx), descriptor(_descriptor) {}
-
     value_type operator*() { return descriptor->oneof_decl(idx); }
 
     friend bool operator==(const Iterator& a, const Iterator& b) {
@@ -905,23 +880,18 @@ struct OneOfRangeImpl {
     }
 
     Iterator& operator++() {
-      Next();
+      idx++;
       return *this;
     }
 
-    void Next() {
-      do {
-        idx++;
-      } while (idx < descriptor->oneof_decl_count() &&
-               descriptor->oneof_decl(idx)->is_synthetic());
-    }
-
     int idx;
     const Descriptor* descriptor;
   };
 
-  Iterator begin() const { return Iterator(descriptor); }
-  Iterator end() const { return {descriptor->oneof_decl_count(), descriptor}; }
+  Iterator begin() const { return {0, descriptor}; }
+  Iterator end() const {
+    return {descriptor->real_oneof_decl_count(), descriptor};
+  }
 
   const Descriptor* descriptor;
 };

src/google/protobuf/compiler/cpp/cpp_message.cc

@@ -226,7 +226,7 @@ bool EmitFieldNonDefaultCondition(io::Printer* printer,
     }
     format.Indent();
     return true;
-  } else if (InRealOneof(field)) {
+  } else if (field->real_containing_oneof()) {
     format("if (_internal_has_$name$()) {\n");
     format.Indent();
     return true;
@@ -282,7 +282,7 @@ void CollectMapInfo(const Options& options, const Descriptor* descriptor,
 bool HasPrivateHasMethod(const FieldDescriptor* field) {
   // Only for oneofs in message types with no field presence. has_$name$(),
   // based on the oneof case, is still useful internally for generated code.
-  return (!HasFieldPresence(field->file()) && InRealOneof(field));
+  return (!HasFieldPresence(field->file()) && field->real_containing_oneof());
 }
 
 // TODO(ckennelly):  Cull these exclusions if/when these protos do not have
@@ -597,7 +597,7 @@ MessageGenerator::MessageGenerator(
 
     if (IsWeak(field, options_)) {
       num_weak_fields_++;
-    } else if (!InRealOneof(field)) {
+    } else if (!field->real_containing_oneof()) {
       optimized_order_.push_back(field);
     }
   }
@@ -677,7 +677,7 @@ void MessageGenerator::AddGenerators(
 void MessageGenerator::GenerateFieldAccessorDeclarations(io::Printer* printer) {
   Formatter format(printer, variables_);
   // optimized_fields_ does not contain fields where
-  //    InRealOneof(field) == true
+  //    field->real_containing_oneof()
   // so we need to iterate over those as well.
   //
   // We place the non-oneof fields in optimized_order_, as that controls the
@@ -689,7 +689,7 @@ void MessageGenerator::GenerateFieldAccessorDeclarations(io::Printer* printer) {
   ordered_fields.insert(ordered_fields.begin(), optimized_order_.begin(),
                         optimized_order_.end());
   for (auto field : FieldRange(descriptor_)) {
-    if (!InRealOneof(field) && !field->options().weak() &&
+    if (!field->real_containing_oneof() && !field->options().weak() &&
         IsFieldUsed(field, options_)) {
       continue;
     }
@@ -922,7 +922,7 @@ void MessageGenerator::GenerateFieldClear(const FieldDescriptor* field,
 
   format.Indent();
 
-  if (InRealOneof(field)) {
+  if (field->real_containing_oneof()) {
     // Clear this field only if it is the active field in this oneof,
     // otherwise ignore
     format("if (_internal_has_$name$()) {\n");
@@ -983,7 +983,7 @@ void MessageGenerator::GenerateFieldAccessorDefinitions(io::Printer* printer) {
                 ? ".weak"
                 : "");
       }
-    } else if (InRealOneof(field)) {
+    } else if (field->real_containing_oneof()) {
       format.Set("field_name", UnderscoresToCamelCase(field->name(), true));
       format.Set("oneof_name", field->containing_oneof()->name());
       format.Set("oneof_index",
@@ -1485,7 +1485,7 @@ void MessageGenerator::GenerateClassDefinition(io::Printer* printer) {
   for (auto field : FieldRange(descriptor_)) {
     // set_has_***() generated in all oneofs.
     if (!field->is_repeated() && !field->options().weak() &&
-        InRealOneof(field)) {
+        field->real_containing_oneof()) {
       format("void set_has_$1$();\n", FieldName(field));
     }
   }
@@ -1594,12 +1594,11 @@ void MessageGenerator::GenerateClassDefinition(io::Printer* printer) {
   }
 
   // Generate _oneof_case_.
-  int count = RealOneofCount(descriptor_);
-  if (count > 0) {
+  if (descriptor_->real_oneof_decl_count() > 0) {
     format(
         "$uint32$ _oneof_case_[$1$];\n"
         "\n",
-        count);
+        descriptor_->real_oneof_decl_count());
   }
 
   if (num_weak_fields_) {
@@ -1695,7 +1694,7 @@ bool MessageGenerator::GenerateParseTable(io::Printer* printer, size_t offset,
     format("PROTOBUF_FIELD_OFFSET($classtype$, _has_bits_),\n");
   }
 
-  if (RealOneofCount(descriptor_) > 0) {
+  if (descriptor_->real_oneof_decl_count() > 0) {
     format("PROTOBUF_FIELD_OFFSET($classtype$, _oneof_case_),\n");
   } else {
     format("-1,  // no _oneof_case_\n");
@@ -1755,7 +1754,7 @@ uint32 CalcFieldNum(const FieldGenerator& generator,
     }
   }
 
-  if (InRealOneof(field)) {
+  if (field->real_containing_oneof()) {
     return internal::FieldMetadata::CalculateType(
         type, internal::FieldMetadata::kOneOf);
   } else if (field->is_packed()) {
@@ -1764,7 +1763,7 @@ uint32 CalcFieldNum(const FieldGenerator& generator,
   } else if (field->is_repeated()) {
     return internal::FieldMetadata::CalculateType(
         type, internal::FieldMetadata::kRepeated);
-  } else if (HasHasbit(field) || InRealOneof(field) || is_a_map) {
+  } else if (HasHasbit(field) || field->real_containing_oneof() || is_a_map) {
     return internal::FieldMetadata::CalculateType(
         type, internal::FieldMetadata::kPresence);
   } else {
@@ -1859,7 +1858,7 @@ int MessageGenerator::GenerateFieldMetadata(io::Printer* printer) {
     }
 
     std::string classfieldname = FieldName(field);
-    if (InRealOneof(field)) {
+    if (field->real_containing_oneof()) {
       classfieldname = field->containing_oneof()->name();
     }
     format.Set("field_name", classfieldname);
@@ -1895,7 +1894,7 @@ int MessageGenerator::GenerateFieldMetadata(io::Printer* printer) {
       type = internal::FieldMetadata::kSpecial;
       ptr = "reinterpret_cast<const void*>(::" + variables_["proto_ns"] +
             "::internal::LazyFieldSerializer";
-      if (InRealOneof(field)) {
+      if (field->real_containing_oneof()) {
         ptr += "OneOf";
       } else if (!HasHasbit(field)) {
         ptr += "NoPresence";
@@ -1912,7 +1911,7 @@ int MessageGenerator::GenerateFieldMetadata(io::Printer* printer) {
           "reinterpret_cast<const "
           "void*>(::$proto_ns$::internal::WeakFieldSerializer)},\n",
           tag);
-    } else if (InRealOneof(field)) {
+    } else if (field->real_containing_oneof()) {
       format.Set("oneofoffset",
                  sizeof(uint32) * field->containing_oneof()->index());
       format(
@@ -1972,10 +1971,10 @@ void MessageGenerator::GenerateDefaultInstanceInitializer(
 
     if (!field->is_repeated() && !IsLazy(field, options_) &&
         field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE &&
-        (!InRealOneof(field) ||
+        (!field->real_containing_oneof() ||
          HasDescriptorMethods(descriptor_->file(), options_))) {
       std::string name;
-      if (InRealOneof(field) || field->options().weak()) {
+      if (field->real_containing_oneof() || field->options().weak()) {
         name = "_" + classname_ + "_default_instance_.";
       } else {
         name =
@@ -2007,7 +2006,7 @@ void MessageGenerator::GenerateDefaultInstanceInitializer(
             "    $1$::internal_default_instance());\n",
             FieldMessageTypeName(field, options_));
       }
-    } else if (InRealOneof(field) &&
+    } else if (field->real_containing_oneof() &&
                HasDescriptorMethods(descriptor_->file(), options_)) {
       field_generators_.get(field).GenerateConstructorCode(printer);
     }
@@ -2118,7 +2117,7 @@ void MessageGenerator::GenerateClassMethods(io::Printer* printer) {
       Formatter::SaveState saver(&format);
       std::map<std::string, std::string> vars;
       SetCommonFieldVariables(field, &vars, options_);
-      if (InRealOneof(field)) {
+      if (field->real_containing_oneof()) {
         SetCommonOneofFieldVariables(field, &vars);
       }
       format.AddMap(vars);
@@ -2129,7 +2128,7 @@ void MessageGenerator::GenerateClassMethods(io::Printer* printer) {
   GenerateStructors(printer);
   format("\n");
 
-  if (RealOneofCount(descriptor_) > 0) {
+  if (descriptor_->real_oneof_decl_count() > 0) {
     GenerateOneofClear(printer);
     format("\n");
   }
@@ -2258,8 +2257,8 @@ size_t MessageGenerator::GenerateParseOffsets(io::Printer* printer) {
 
     processing_type |= static_cast<unsigned>(
         field->is_repeated() ? internal::kRepeatedMask : 0);
-    processing_type |=
-        static_cast<unsigned>(InRealOneof(field) ? internal::kOneofMask : 0);
+    processing_type |= static_cast<unsigned>(
+        field->real_containing_oneof() ? internal::kOneofMask : 0);
 
     if (field->is_map()) {
       processing_type = internal::TYPE_MAP;
@@ -2269,7 +2268,7 @@ size_t MessageGenerator::GenerateParseOffsets(io::Printer* printer) {
         WireFormat::TagSize(field->number(), field->type());
 
     std::map<std::string, std::string> vars;
-    if (InRealOneof(field)) {
+    if (field->real_containing_oneof()) {
       vars["name"] = field->containing_oneof()->name();
       vars["presence"] = StrCat(field->containing_oneof()->index());
     } else {
@@ -2400,7 +2399,7 @@ std::pair<size_t, size_t> MessageGenerator::GenerateOffsets(
   } else {
     format("~0u,  // no _extensions_\n");
   }
-  if (RealOneofCount(descriptor_) > 0) {
+  if (descriptor_->real_oneof_decl_count() > 0) {
     format("PROTOBUF_FIELD_OFFSET($classtype$, _oneof_case_[0]),\n");
   } else {
     format("~0u,  // no _oneof_case_\n");
@@ -2418,13 +2417,13 @@ std::pair<size_t, size_t> MessageGenerator::GenerateOffsets(
   }
   const int kNumGenericOffsets = 5;  // the number of fixed offsets above
   const size_t offsets = kNumGenericOffsets + descriptor_->field_count() +
-                         RealOneofCount(descriptor_) - num_stripped;
+                         descriptor_->real_oneof_decl_count() - num_stripped;
   size_t entries = offsets;
   for (auto field : FieldRange(descriptor_)) {
     if (!IsFieldUsed(field, options_)) {
       continue;
     }
-    if (InRealOneof(field) || field->options().weak()) {
+    if (field->real_containing_oneof() || field->options().weak()) {
       format("offsetof($classtype$DefaultTypeInternal, $1$_)",
              FieldName(field));
     } else {
@@ -2444,7 +2443,7 @@ std::pair<size_t, size_t> MessageGenerator::GenerateOffsets(
     format("PROTOBUF_FIELD_OFFSET($classtype$, $1$_),\n", oneof->name());
     count++;
   }
-  GOOGLE_CHECK_EQ(count, RealOneofCount(descriptor_));
+  GOOGLE_CHECK_EQ(count, descriptor_->real_oneof_decl_count());
 
   if (IsMapEntryMessage(descriptor_)) {
     entries += 2;
@@ -2656,7 +2655,7 @@ void MessageGenerator::GenerateStructors(io::Printer* printer) {
   for (auto field : optimized_order_) {
     GOOGLE_DCHECK(IsFieldUsed(field, options_));
     bool has_arena_constructor = field->is_repeated();
-    if (!InRealOneof(field) &&
+    if (!field->real_containing_oneof() &&
         (IsLazy(field, options_) || IsStringPiece(field, options_))) {
       has_arena_constructor = true;
     }
@@ -3122,8 +3121,7 @@ void MessageGenerator::GenerateSwap(io::Printer* printer) {
       format("swap($1$_, other->$1$_);\n", oneof->name());
     }
 
-    int count = RealOneofCount(descriptor_);
-    for (int i = 0; i < count; i++) {
+    for (int i = 0; i < descriptor_->real_oneof_decl_count(); i++) {
       format("swap(_oneof_case_[$1$], other->_oneof_case_[$1$]);\n", i);
     }
 
@@ -3572,7 +3570,7 @@ void MessageGenerator::GenerateSerializeWithCachedSizesBody(
       if (eager_ || MustFlush(field)) {
         Flush();
       }
-      if (!InRealOneof(field)) {
+      if (!field->real_containing_oneof()) {
         // TODO(ckennelly): Defer non-oneof fields similarly to oneof fields.
 
         if (!field->options().weak() && !field->is_repeated() && !eager_) {
@@ -4014,7 +4012,7 @@ void MessageGenerator::GenerateIsInitialized(io::Printer* printer) {
       } else if (field->options().weak()) {
         continue;
       } else {
-        GOOGLE_CHECK(!InRealOneof(field));
+        GOOGLE_CHECK(!field->real_containing_oneof());
         format(
             "if (_internal_has_$1$()) {\n"
             "  if (!$1$_->IsInitialized()) return false;\n"
@@ -4054,7 +4052,7 @@ void MessageGenerator::GenerateIsInitialized(io::Printer* printer) {
           field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE &&
           !ShouldIgnoreRequiredFieldCheck(field, options_) &&
           scc_analyzer_->HasRequiredFields(field->message_type())) {
-        GOOGLE_CHECK(!(field->options().weak() || !InRealOneof(field)));
+        GOOGLE_CHECK(!(field->options().weak() || !field->real_containing_oneof()));
         if (field->options().weak()) {
           // Just skip.
         } else {

src/google/protobuf/compiler/cpp/cpp_string_field.cc

@@ -501,7 +501,7 @@ void StringFieldGenerator::GenerateMessageClearingCode(
 
 void StringFieldGenerator::GenerateMergingCode(io::Printer* printer) const {
   Formatter format(printer, variables_);
-  if (SupportsArenas(descriptor_) || InRealOneof(descriptor_)) {
+  if (SupportsArenas(descriptor_) || descriptor_->real_containing_oneof()) {
     // TODO(gpike): improve this
     format("_internal_set_$name$(from._internal_$name$());\n");
   } else {
@@ -545,7 +545,7 @@ void StringFieldGenerator::GenerateCopyConstructorCode(
 
   format.Indent();
 
-  if (SupportsArenas(descriptor_) || InRealOneof(descriptor_)) {
+  if (SupportsArenas(descriptor_) || descriptor_->real_containing_oneof()) {
     // TODO(gpike): improve this
     format(
         "$name$_.Set$lite$($default_variable$, from._internal_$name$(),\n"

src/google/protobuf/compiler/parser.cc

@@ -783,8 +783,7 @@ bool Parser::ParseMessageDefinition(
     }
 
     for (auto& field : *message->mutable_field()) {
-      if (field.proto3_optional() &&
-          field.type() != FieldDescriptorProto::TYPE_MESSAGE) {
+      if (field.proto3_optional()) {
         std::string oneof_name = field.name();
 
         // Prepend 'XXXXX_' until we are no longer conflicting.

src/google/protobuf/descriptor.cc

@@ -5500,6 +5500,42 @@ void DescriptorBuilder::CrossLinkMessage(Descriptor* message,
           message->field(i);
     }
   }
+
+  for (int i = 0; i < message->field_count(); i++) {
+    const FieldDescriptor* field = message->field(i);
+    if (field->proto3_optional_) {
+      if (!field->containing_oneof() ||
+          !field->containing_oneof()->is_synthetic()) {
+        AddError(message->full_name(), proto.field(i),
+                 DescriptorPool::ErrorCollector::OTHER,
+                 "Fields with proto3_optional set must be "
+                 "a member of a one-field oneof");
+      }
+    }
+  }
+
+  // Synthetic oneofs must be last.
+  int first_synthetic = -1;
+  for (int i = 0; i < message->oneof_decl_count(); i++) {
+    const OneofDescriptor* oneof = message->oneof_decl(i);
+    if (oneof->is_synthetic()) {
+      if (first_synthetic == -1) {
+        first_synthetic = i;
+      }
+    } else {
+      if (first_synthetic != -1) {
+        AddError(message->full_name(), proto.oneof_decl(i),
+                 DescriptorPool::ErrorCollector::OTHER,
+                 "Synthetic oneofs must be after all other oneofs");
+      }
+    }
+  }
+
+  if (first_synthetic == -1) {
+    message->real_oneof_decl_count_ = message->oneof_decl_count_;
+  } else {
+    message->real_oneof_decl_count_ = first_synthetic;
+  }
 }
 
 void DescriptorBuilder::CrossLinkExtensionRange(

src/google/protobuf/descriptor.h

@@ -337,6 +337,10 @@ class PROTOBUF_EXPORT Descriptor {
 
   // The number of oneofs in this message type.
   int oneof_decl_count() const;
+  // The number of oneofs in this message type, excluding synthetic oneofs.
+  // Real oneofs always come first, so iterating up to real_oneof_decl_cout()
+  // will yield all real oneofs.
+  int real_oneof_decl_count() const;
   // Get a oneof by index, where 0 <= index < oneof_decl_count().
   // These are returned in the order they were defined in the .proto file.
   const OneofDescriptor* oneof_decl(int index) const;
@@ -526,6 +530,7 @@ class PROTOBUF_EXPORT Descriptor {
 
   int field_count_;
   int oneof_decl_count_;
+  int real_oneof_decl_count_;
   int nested_type_count_;
   int enum_type_count_;
   int extension_range_count_;
@@ -745,6 +750,10 @@ class PROTOBUF_EXPORT FieldDescriptor {
   // nullptr.
   const OneofDescriptor* containing_oneof() const;
 
+  // If the field is a member of a non-synthetic oneof, returns the descriptor
+  // for the oneof, otherwise returns nullptr.
+  const OneofDescriptor* real_containing_oneof() const;
+
   // If the field is a member of a oneof, returns the index in that oneof.
   int index_in_oneof() const;
 
@@ -1972,6 +1981,7 @@ PROTOBUF_DEFINE_ACCESSOR(Descriptor, containing_type, const Descriptor*)
 
 PROTOBUF_DEFINE_ACCESSOR(Descriptor, field_count, int)
 PROTOBUF_DEFINE_ACCESSOR(Descriptor, oneof_decl_count, int)
+PROTOBUF_DEFINE_ACCESSOR(Descriptor, real_oneof_decl_count, int)
 PROTOBUF_DEFINE_ACCESSOR(Descriptor, nested_type_count, int)
 PROTOBUF_DEFINE_ACCESSOR(Descriptor, enum_type_count, int)
 
@@ -2166,9 +2176,15 @@ inline bool FieldDescriptor::has_optional_keyword() const {
           !containing_oneof());
 }
 
+inline const OneofDescriptor* FieldDescriptor::real_containing_oneof() const {
+  return containing_oneof_ && !containing_oneof_->is_synthetic()
+             ? containing_oneof_
+             : nullptr;
+}
+
 inline bool FieldDescriptor::is_singular_with_presence() const {
   if (is_repeated()) return false;
-  if (containing_oneof() && !containing_oneof()->is_synthetic()) return false;
+  if (real_containing_oneof()) return false;
   return cpp_type() == CPPTYPE_MESSAGE || proto3_optional_ ||
          file()->syntax() == FileDescriptor::SYNTAX_PROTO2;
 }

src/google/protobuf/descriptor.proto

@@ -217,10 +217,21 @@ message FieldDescriptorProto {
   // If true, this is a proto3 "optional". When a proto3 field is optional, it
   // tracks presence regardless of field type.
   //
-  // For message fields this doesn't create any semantic change, since
-  // non-repeated message fields always track presence. However it still
+  // When proto3_optional is true, this field must be belong to a oneof to
+  // signal to old proto3 clients that presence is tracked for this field. This
+  // oneof is known as a "synthetic" oneof, and this field must be its sole
+  // member (each proto3 optional field gets its own synthetic oneof). Synthetic
+  // oneofs exist in the descriptor only, and do not generate any API. Synthetic
+  // oneofs must be ordered after all "real" oneofs.
+  //
+  // For message fields, proto3_optional doesn't create any semantic change,
+  // since non-repeated message fields always track presence. However it still
   // indicates the semantic detail of whether the user wrote "optional" or not.
-  // This can be useful for round-tripping the .proto file.
+  // This can be useful for round-tripping the .proto file. For consistency we
+  // give message fields a synthetic oneof also, even though it is not required
+  // to track presence. This is especially important because the parser can't
+  // tell if a field is a message or an enum, so it must always create a
+  // synthetic oneof.
   //
   // Proto2 optional fields do not set this flag, because they already indicate
   // optional with `LABEL_OPTIONAL`.

src/google/protobuf/generated_message_table_driven_lite.h

@@ -280,9 +280,13 @@ static inline bool HandleString(io::CodedInputStream* input, MessageLite* msg,
           }
           utf8_string_data = field->Get();
         } break;
+        default:
+          PROTOBUF_ASSUME(false);
       }
       break;
     }
+    default:
+      PROTOBUF_ASSUME(false);
   }
 
   if (kValidateUtf8) {
@@ -322,6 +326,8 @@ inline bool HandleEnum(const ParseTable& table, io::CodedInputStream* input,
         SetOneofField(msg, presence, presence_index, offset, field_number,
                       value);
         break;
+      default:
+        PROTOBUF_ASSUME(false);
     }
   } else {
     UnknownFieldHandler::Varint(msg, table, tag, value);
@@ -406,9 +412,6 @@ bool MergePartialFromCodedStreamInlined(MessageLite* msg,
     const unsigned char processing_type = data->processing_type;
 
     if (data->normal_wiretype == static_cast<unsigned char>(wire_type)) {
-      // TODO(ckennelly): Use a computed goto on GCC/LLVM or otherwise eliminate
-      // the bounds check on processing_type.
-
       switch (processing_type) {
 #define HANDLE_TYPE(TYPE, CPPTYPE)                                             \
   case (WireFormatLite::TYPE_##TYPE): {                                        \
@@ -739,7 +742,7 @@ bool MergePartialFromCodedStreamInlined(MessageLite* msg,
           return true;
         }
         default:
-          break;
+          PROTOBUF_ASSUME(false);
       }
     } else if (data->packed_wiretype == static_cast<unsigned char>(wire_type)) {
       // Non-packable fields have their packed_wiretype masked with
@@ -751,8 +754,6 @@ bool MergePartialFromCodedStreamInlined(MessageLite* msg,
       GOOGLE_DCHECK_NE(TYPE_BYTES_INLINED | kRepeatedMask, processing_type);
       GOOGLE_DCHECK_NE(TYPE_STRING_INLINED | kRepeatedMask, processing_type);
 
-      // TODO(ckennelly): Use a computed goto on GCC/LLVM.
-      //
       // Mask out kRepeatedMask bit, allowing the jump table to be smaller.
       switch (static_cast<WireFormatLite::FieldType>(processing_type ^
                                                      kRepeatedMask)) {
@@ -825,7 +826,7 @@ bool MergePartialFromCodedStreamInlined(MessageLite* msg,
           GOOGLE_DCHECK(false);
           return false;
         default:
-          break;
+          PROTOBUF_ASSUME(false);
       }
     } else {
       if (wire_type == WireFormatLite::WIRETYPE_END_GROUP) {