Sync from Piper @mkruskal/footmitten

PROTOBUF_SYNC_PIPER

Author: mkruskal-google

CHANGES.txt

@@ -40,8 +40,15 @@
   * Optimized Java proto serialization gencode for protos having many extension ranges with few fields in between.
   * More thoroughly annotate public generated code in Java lite protocol buffers.
   * Fixed Bug in proto3 java lite repeated enum fields. Failed to call copyOnWrite before modifying previously built message. Causes modification to already "built" messages that should be immutable.
-  * Refactoring java full runtime to reuse sub-message builders and prepare to migrate parsing logic from parse constructor to builder.
   * Fix Java reflection serialization of empty packed fields.
+  * Refactoring java full runtime to reuse sub-message builders and prepare to
+    migrate parsing logic from parse constructor to builder.
+  * Move proto wireformat parsing functionality from the private "parsing
+    constructor" to the Builder class.
+  * Change the Lite runtime to prefer merging from the wireformat into mutable
+    messages rather than building up a new immutable object before merging. This
+    way results in fewer allocations and copy operations.
+  * Make message-type extensions merge from wire-format instead of building up instances and merging afterwards. This has much better performance.
 
   Python
   * Changes ordering of printed fields in .pyi files from lexicographic to the same ordering found in the proto descriptor.

java/core/src/main/java/com/google/protobuf/MessageReflection.java

@@ -378,6 +378,7 @@ MergeTarget newEmptyTargetForField(
   static class BuilderAdapter implements MergeTarget {
 
     private final Message.Builder builder;
+    private boolean hasNestedBuilders = true;
 
     @Override
     public Descriptors.Descriptor getDescriptorForType() {
@@ -393,13 +394,30 @@ public Object getField(Descriptors.FieldDescriptor field) {
       return builder.getField(field);
     }
 
+    private Message.Builder getFieldBuilder(Descriptors.FieldDescriptor field) {
+      if (hasNestedBuilders) {
+        try {
+          return builder.getFieldBuilder(field);
+        } catch (UnsupportedOperationException e) {
+          hasNestedBuilders = false;
+        }
+      }
+      return null;
+    }
+
     @Override
     public boolean hasField(Descriptors.FieldDescriptor field) {
       return builder.hasField(field);
     }
 
     @Override
     public MergeTarget setField(Descriptors.FieldDescriptor field, Object value) {
+      if (!field.isRepeated() && value instanceof MessageLite.Builder) {
+        if (value != getFieldBuilder(field)) {
+          builder.setField(field, ((MessageLite.Builder) value).buildPartial());
+        }
+        return this;
+      }
       builder.setField(field, value);
       return this;
     }
@@ -413,12 +431,18 @@ public MergeTarget clearField(Descriptors.FieldDescriptor field) {
     @Override
     public MergeTarget setRepeatedField(
         Descriptors.FieldDescriptor field, int index, Object value) {
+      if (value instanceof MessageLite.Builder) {
+        value = ((MessageLite.Builder) value).buildPartial();
+      }
       builder.setRepeatedField(field, index, value);
       return this;
     }
 
     @Override
     public MergeTarget addRepeatedField(Descriptors.FieldDescriptor field, Object value) {
+      if (value instanceof MessageLite.Builder) {
+        value = ((MessageLite.Builder) value).buildPartial();
+      }
       builder.addRepeatedField(field, value);
       return this;
     }
@@ -536,11 +560,19 @@ public void mergeGroup(
         Message defaultInstance)
         throws IOException {
       if (!field.isRepeated()) {
+        Message.Builder subBuilder;
         if (hasField(field)) {
-          input.readGroup(field.getNumber(), builder.getFieldBuilder(field), extensionRegistry);
-          return;
+          subBuilder = getFieldBuilder(field);
+          if (subBuilder != null) {
+            input.readGroup(field.getNumber(), subBuilder, extensionRegistry);
+            return;
+          } else {
+            subBuilder = newMessageFieldInstance(field, defaultInstance);
+            subBuilder.mergeFrom((Message) getField(field));
+          }
+        } else {
+          subBuilder = newMessageFieldInstance(field, defaultInstance);
         }
-        Message.Builder subBuilder = newMessageFieldInstance(field, defaultInstance);
         input.readGroup(field.getNumber(), subBuilder, extensionRegistry);
         Object unused = setField(field, subBuilder.buildPartial());
       } else {
@@ -558,11 +590,19 @@ public void mergeMessage(
         Message defaultInstance)
         throws IOException {
       if (!field.isRepeated()) {
+        Message.Builder subBuilder;
         if (hasField(field)) {
-          input.readMessage(builder.getFieldBuilder(field), extensionRegistry);
-          return;
+          subBuilder = getFieldBuilder(field);
+          if (subBuilder != null) {
+            input.readMessage(subBuilder, extensionRegistry);
+            return;
+          } else {
+            subBuilder = newMessageFieldInstance(field, defaultInstance);
+            subBuilder.mergeFrom((Message) getField(field));
+          }
+        } else {
+          subBuilder = newMessageFieldInstance(field, defaultInstance);
         }
-        Message.Builder subBuilder = newMessageFieldInstance(field, defaultInstance);
         input.readMessage(subBuilder, extensionRegistry);
         Object unused = setField(field, subBuilder.buildPartial());
       } else {
@@ -586,11 +626,14 @@ private Message.Builder newMessageFieldInstance(
     public MergeTarget newMergeTargetForField(
         Descriptors.FieldDescriptor field, Message defaultInstance) {
       Message.Builder subBuilder;
-      if (defaultInstance != null) {
-        subBuilder = defaultInstance.newBuilderForType();
-      } else {
-        subBuilder = builder.newBuilderForField(field);
+      if (!field.isRepeated() && hasField(field)) {
+        subBuilder = getFieldBuilder(field);
+        if (subBuilder != null) {
+          return new BuilderAdapter(subBuilder);
+        }
       }
+
+      subBuilder = newMessageFieldInstance(field, defaultInstance);
       if (!field.isRepeated()) {
         Message originalMessage = (Message) getField(field);
         if (originalMessage != null) {
@@ -626,7 +669,7 @@ public WireFormat.Utf8Validation getUtf8Validation(Descriptors.FieldDescriptor d
 
     @Override
     public Object finish() {
-      return builder.buildPartial();
+      return builder;
     }
   }
 

ruby/compatibility_tests/v3.0.0/tests/basic.rb

@@ -667,8 +667,8 @@ def test_map_msg_enum_valuetypes
       assert m["z"] == :C
       m["z"] = 2
       assert m["z"] == :B
-      m["z"] = 4
-      assert m["z"] == 4
+      m["z"] = 5
+      assert m["z"] == 5
       assert_raise RangeError do
         m["z"] = :Z
       end

ruby/ext/google/protobuf_c/message.c

@@ -1263,15 +1263,20 @@ VALUE build_module_from_enumdesc(VALUE _enumdesc) {
   int n = upb_EnumDef_ValueCount(e);
   for (int i = 0; i < n; i++) {
     const upb_EnumValueDef* ev = upb_EnumDef_Value(e, i);
-    const char* name = upb_EnumValueDef_Name(ev);
+    char* name = strdup(upb_EnumValueDef_Name(ev));
     int32_t value = upb_EnumValueDef_Number(ev);
     if (name[0] < 'A' || name[0] > 'Z') {
-      rb_warn(
+      if (name[0] >= 'a' && name[0] <= 'z') {
+        name[0] -= 32; // auto capitalize
+      } else {
+        rb_warn(
           "Enum value '%s' does not start with an uppercase letter "
           "as is required for Ruby constants.",
           name);
+      }
     }
     rb_define_const(mod, name, INT2NUM(value));
+    free(name);
   }
 
   rb_define_singleton_method(mod, "lookup", enum_lookup, 1);

ruby/src/main/java/com/google/protobuf/jruby/RubyEnumDescriptor.java

@@ -162,9 +162,10 @@ private RubyModule buildModuleFromDescriptor(ThreadContext context) {
     boolean defaultValueRequiredButNotFound =
         descriptor.getFile().getSyntax() == FileDescriptor.Syntax.PROTO3;
     for (EnumValueDescriptor value : descriptor.getValues()) {
-      String name = value.getName();
-      // Make sure its a valid constant name before trying to create it
-      if (Character.isUpperCase(name.codePointAt(0))) {
+      String name = fixEnumConstantName(value.getName());
+      // Make sure it's a valid constant name before trying to create it
+      int ch = name.codePointAt(0);
+      if (Character.isUpperCase(ch)) {
         enumModule.defineConstant(name, runtime.newFixnum(value.getNumber()));
       } else {
         runtime
@@ -189,6 +190,22 @@ private RubyModule buildModuleFromDescriptor(ThreadContext context) {
     return enumModule;
   }
 
+  private static String fixEnumConstantName(String name) {
+    if (name != null && name.length() > 0) {
+      int ch = name.codePointAt(0);
+      if (ch >= 'a' && ch <= 'z') {
+        // Protobuf enums can start with lowercase letters, while Ruby's constant should
+        // always start with uppercase letters. We tolerate this case by capitalizing
+        // the first character if possible.
+        return new StringBuilder()
+                .appendCodePoint(Character.toUpperCase(ch))
+                .append(name.substring(1))
+                .toString();
+      }
+    }
+    return name;
+  }
+
   private EnumDescriptor descriptor;
   private EnumDescriptorProto.Builder builder;
   private IRubyObject name;

ruby/tests/basic_test.proto

@@ -73,6 +73,7 @@ enum TestEnum {
   A = 1;
   B = 2;
   C = 3;
+  v0 = 4;
 }
 
 message TestEmbeddedMessageParent {

ruby/tests/basic_test_proto2.proto

@@ -69,6 +69,7 @@ enum TestEnum {
   A = 1;
   B = 2;
   C = 3;
+  v0 = 4;
 }
 
 enum TestNonZeroEnum {

ruby/tests/common_tests.rb

@@ -331,14 +331,16 @@ def test_rptfield_enum
     l.push :A
     l.push :B
     l.push :C
-    assert l.count == 3
+    l.push :v0
+    assert l.count == 4
     assert_raise RangeError do
       l.push :D
     end
     assert l[0] == :A
+    assert l[3] == :v0
 
-    l.push 4
-    assert l[3] == 4
+    l.push 5
+    assert l[4] == 5
   end
 
   def test_rptfield_initialize
@@ -542,8 +544,8 @@ def test_map_msg_enum_valuetypes
     assert m["z"] == :C
     m["z"] = 2
     assert m["z"] == :B
-    m["z"] = 4
-    assert m["z"] == 4
+    m["z"] = 5
+    assert m["z"] == 5
     assert_raise RangeError do
       m["z"] = :Z
     end
@@ -712,14 +714,17 @@ def test_enum_lookup
     assert proto_module::TestEnum::A == 1
     assert proto_module::TestEnum::B == 2
     assert proto_module::TestEnum::C == 3
+    assert proto_module::TestEnum::V0 == 4
 
     assert proto_module::TestEnum::lookup(1) == :A
     assert proto_module::TestEnum::lookup(2) == :B
     assert proto_module::TestEnum::lookup(3) == :C
+    assert proto_module::TestEnum::lookup(4) == :v0
 
     assert proto_module::TestEnum::resolve(:A) == 1
     assert proto_module::TestEnum::resolve(:B) == 2
     assert proto_module::TestEnum::resolve(:C) == 3
+    assert proto_module::TestEnum::resolve(:v0) == 4
   end
 
   def test_enum_const_get_helpers
@@ -788,7 +793,7 @@ def test_enum_getter_only_enums
     assert_raise(NoMethodError) { m.a }
     assert_raise(NoMethodError) { m.a_const_const }
   end
-  
+
   def test_repeated_push
     m = proto_module::TestMessage.new
 
@@ -1762,7 +1767,7 @@ def test_freeze
     assert_raise(FrozenErrorType) { m.repeated_msg = proto_module::TestMessage2.new }
     assert_raise(FrozenErrorType) { m.repeated_enum = :A }
   end
-  
+
   def test_eq
     m1 = proto_module::TestMessage.new(:optional_string => 'foo', :repeated_string => ['bar1', 'bar2'])
     m2 = proto_module::TestMessage.new(:optional_string => 'foo', :repeated_string => ['bar1', 'bar2'])

ruby/tests/generated_code.proto

@@ -67,6 +67,8 @@ enum TestEnum {
   A = 1;
   B = 2;
   C = 3;
+
+  v0 = 4;
 }
 
 message testLowercaseNested {

ruby/tests/generated_code_proto2.proto

@@ -68,6 +68,8 @@ enum TestEnum {
   A = 1;
   B = 2;
   C = 3;
+
+  v0 = 4;
 }
 
 message TestUnknown {