From 3e0f82e57f815a2b3d169d380dd30c40667984fd Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Thu, 14 Nov 2024 13:19:34 -0800 Subject: [PATCH] Fix handling of implicit field presence in mergeFrom to match the behavior in other places. PiperOrigin-RevId: 696629331 --- java/core/BUILD.bazel | 2 + .../google/protobuf/FieldPresenceTest.java | 25 ++++++- .../google/protobuf/field_presence_test.proto | 2 + .../protobuf/compiler/java/full/BUILD.bazel | 1 + .../compiler/java/full/primitive_field.cc | 75 +++++++++++++------ 5 files changed, 82 insertions(+), 23 deletions(-) diff --git a/java/core/BUILD.bazel b/java/core/BUILD.bazel index 2bc691e4eb78d..939d353b16804 100644 --- 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 index 8981ab8af2625..9455df0dc08ac 100644 --- 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 index d73822260192e..c726db91842d1 100644 --- 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 index 32447de054b12..8a437b1724362 100644 --- 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 index 0aa274c018013..80c7e5c74dd39 100644 --- 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 #include +#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 create_value, + absl::flat_hash_map* 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(