Fix handling of implicit field presence in mergeFrom to match the beh…

…avior in other places. PiperOrigin-RevId: 696629331
protocolbuffers · Nov 14, 2024 · 3e0f82e · 3e0f82e
1 parent 506dd5f
commit 3e0f82e
Show file tree

Hide file tree

Showing 5 changed files with 82 additions and 23 deletions.
diff --git a/java/core/BUILD.bazel b/java/core/BUILD.bazel
@@ -624,6 +624,7 @@ junit_tests(
             "src/test/java/com/google/protobuf/CodedOutputStreamTest.java",
             "src/test/java/com/google/protobuf/CodedInputStreamTest.java",
             "src/test/java/com/google/protobuf/ProtobufToStringOutputTest.java",
+            "src/test/java/com/google/protobuf/FieldPresenceTest.java",
             # Excluded in core_tests
             "src/test/java/com/google/protobuf/DecodeUtf8Test.java",
             "src/test/java/com/google/protobuf/IsValidUtf8Test.java",
@@ -673,6 +674,7 @@ junit_tests(
             "src/test/java/com/google/protobuf/CodedOutputStreamTest.java",
             "src/test/java/com/google/protobuf/CodedInputStreamTest.java",
             "src/test/java/com/google/protobuf/ProtobufToStringOutputTest.java",
+            "src/test/java/com/google/protobuf/FieldPresenceTest.java",
             # Excluded in core_tests
             "src/test/java/com/google/protobuf/DecodeUtf8Test.java",
             "src/test/java/com/google/protobuf/IsValidUtf8Test.java",

diff --git a/java/core/src/test/java/com/google/protobuf/FieldPresenceTest.java b/java/core/src/test/java/com/google/protobuf/FieldPresenceTest.java
@@ -274,6 +274,28 @@ public void testFieldPresence() {
     assertThat(message.hashCode()).isEqualTo(empty.hashCode());
   }
 
+  @Test
+  public void testFieldPresence_mergeEmptyBytesValue() {
+    TestAllTypes mergeFrom =
+        TestAllTypes.newBuilder().setOptionalBytes(ByteString.copyFrom(new byte[0])).build();
+    TestAllTypes mergeTo =
+        TestAllTypes.newBuilder().setOptionalBytes(ByteString.copyFromUtf8("A")).build();
+
+    // An empty ByteString should be treated as "unset" and not override the value in mergeTo.
+    assertThat(mergeTo.toBuilder().mergeFrom(mergeFrom).build()).isEqualTo(mergeTo);
+  }
+
+  @Test
+  public void testFieldPresence_mergeNegativeZeroValue() {
+    TestAllTypes mergeFrom =
+        TestAllTypes.newBuilder().setOptionalFloat(-0.0F).setOptionalDouble(-0.0).build();
+    TestAllTypes mergeTo =
+        TestAllTypes.newBuilder().setOptionalFloat(42.23F).setOptionalDouble(23.42).build();
+
+    // Negative zero should be treated as "set" and override the value in mergeTo.
+    assertThat(mergeTo.toBuilder().mergeFrom(mergeFrom).build()).isEqualTo(mergeFrom);
+  }
+
   @Test
   public void testFieldPresenceByReflection() {
     Descriptor descriptor = TestAllTypes.getDescriptor();
@@ -356,8 +378,7 @@ public void testFieldPresenceDynamicMessage() {
 
     // Field set to default value is seen as not present.
     message =
-        message
-            .toBuilder()
+        message.toBuilder()
             .setField(optionalInt32Field, 0)
             .setField(optionalStringField, "")
             .setField(optionalBytesField, ByteString.EMPTY)

diff --git a/java/core/src/test/proto/com/google/protobuf/field_presence_test.proto b/java/core/src/test/proto/com/google/protobuf/field_presence_test.proto
@@ -29,6 +29,8 @@ message TestAllTypes {
   int32 optional_int32 = 1;
   string optional_string = 2;
   bytes optional_bytes = 3;
+  float optional_float = 8;
+  double optional_double = 9;
   NestedEnum optional_nested_enum = 4;
   NestedMessage optional_nested_message = 5;
   protobuf_unittest.TestRequired optional_proto2_message = 6;

diff --git a/src/google/protobuf/compiler/java/full/BUILD.bazel b/src/google/protobuf/compiler/java/full/BUILD.bazel
@@ -44,6 +44,7 @@ cc_library(
         "//src/google/protobuf/compiler/java:internal_helpers",
         "//src/google/protobuf/io:printer",
         "@com_google_absl//absl/container:flat_hash_map",
+        "@com_google_absl//absl/functional:function_ref",
         "@com_google_absl//absl/log:absl_check",
         "@com_google_absl//absl/log:absl_log",
         "@com_google_absl//absl/strings",

diff --git a/src/google/protobuf/compiler/java/full/primitive_field.cc b/src/google/protobuf/compiler/java/full/primitive_field.cc
@@ -14,9 +14,12 @@
 #include <cstdint>
 #include <string>
 
+#include "absl/container/flat_hash_map.h"
+#include "absl/functional/function_ref.h"
 #include "absl/log/absl_check.h"
 #include "absl/log/absl_log.h"
 #include "absl/strings/str_cat.h"
+#include "absl/strings/string_view.h"
 #include "google/protobuf/compiler/java/context.h"
 #include "google/protobuf/compiler/java/doc_comment.h"
 #include "google/protobuf/compiler/java/field_common.h"
@@ -30,11 +33,29 @@ namespace google {
 namespace protobuf {
 namespace compiler {
 namespace java {
+namespace {
 
 using internal::WireFormat;
 using Semantic = ::google::protobuf::io::AnnotationCollector::Semantic;
 
-namespace {
+// Adds two variables to (*variables) that operate on a particular field value,
+// both for use locally and on another instance named 'other'. This ensures that
+// we treat these values the same way, whether it's in the current instance or
+// another.
+//
+// `this_variable_name` and `other_variable_name` MUST be string constants.
+//
+// The `create_value` FunctionRef takes the representation of the value and
+// should use it to create and return the code that operates on this value.
+void AddPrimitiveVariableForThisAndOther(
+    absl::string_view this_variable_name, absl::string_view other_variable_name,
+    absl::FunctionRef<std::string(absl::string_view)> create_value,
+    absl::flat_hash_map<absl::string_view, std::string>* variables) {
+  (*variables)[this_variable_name] =
+      create_value(absl::StrCat((*variables)["name"], "_"));
+  (*variables)[other_variable_name] = create_value(
+      absl::StrCat("other.get", (*variables)["capitalized_name"], "()"));
+}
 
 void SetPrimitiveVariables(
     const FieldDescriptor* descriptor, int messageBitIndex, int builderBitIndex,
@@ -112,25 +133,44 @@ void SetPrimitiveVariables(
     (*variables)["set_has_field_bit_to_local"] =
         absl::StrCat(GenerateSetBitToLocal(messageBitIndex), ";");
     (*variables)["is_field_present_message"] = GenerateGetBit(messageBitIndex);
+    (*variables)["is_other_field_present_message"] =
+        absl::StrCat("other.has", (*variables)["capitalized_name"], "()");
   } else {
     (*variables)["set_has_field_bit_to_local"] = "";
     switch (descriptor->type()) {
       case FieldDescriptor::TYPE_BYTES:
-        (*variables)["is_field_present_message"] =
-            absl::StrCat("!", name, "_.isEmpty()");
+        AddPrimitiveVariableForThisAndOther(
+            "is_field_present_message", "is_other_field_present_message",
+            [](absl::string_view value) {
+              return absl::StrCat("!", value, ".isEmpty()");
+            },
+            variables);
         break;
       case FieldDescriptor::TYPE_FLOAT:
-        (*variables)["is_field_present_message"] =
-            absl::StrCat("java.lang.Float.floatToRawIntBits(", name, "_) != 0");
+        AddPrimitiveVariableForThisAndOther(
+            "is_field_present_message", "is_other_field_present_message",
+            [](absl::string_view value) {
+              return absl::StrCat("java.lang.Float.floatToRawIntBits(", value,
+                                  ") != 0");
+            },
+            variables);
         break;
       case FieldDescriptor::TYPE_DOUBLE:
-        (*variables)["is_field_present_message"] = absl::StrCat(
-            "java.lang.Double.doubleToRawLongBits(", name, "_) != 0");
+        AddPrimitiveVariableForThisAndOther(
+            "is_field_present_message", "is_other_field_present_message",
+            [](absl::string_view value) {
+              return absl::StrCat("java.lang.Double.doubleToRawLongBits(",
+                                  value, ") != 0");
+            },
+            variables);
         break;
       default:
-        variables->insert(
-            {"is_field_present_message",
-             absl::StrCat(name, "_ != ", (*variables)["default"])});
+        AddPrimitiveVariableForThisAndOther(
+            "is_field_present_message", "is_other_field_present_message",
+            [variables](absl::string_view value) {
+              return absl::StrCat(value, " != ", (*variables)["default"]);
+            },
+            variables);
         break;
     }
   }
@@ -301,17 +341,10 @@ void ImmutablePrimitiveFieldGenerator::GenerateBuilderClearCode(
 
 void ImmutablePrimitiveFieldGenerator::GenerateMergingCode(
     io::Printer* printer) const {
-  if (descriptor_->has_presence()) {
-    printer->Print(variables_,
-                   "if (other.has$capitalized_name$()) {\n"
-                   "  set$capitalized_name$(other.get$capitalized_name$());\n"
-                   "}\n");
-  } else {
-    printer->Print(variables_,
-                   "if (other.get$capitalized_name$() != $default$) {\n"
-                   "  set$capitalized_name$(other.get$capitalized_name$());\n"
-                   "}\n");
-  }
+  printer->Print(variables_,
+                 "if ($is_other_field_present_message$) {\n"
+                 "  set$capitalized_name$(other.get$capitalized_name$());\n"
+                 "}\n");
 }
 
 void ImmutablePrimitiveFieldGenerator::GenerateBuildingCode(