From de7af0c6d6e034c64de6930da3dba9855fd7607c Mon Sep 17 00:00:00 2001 From: Bo Yang Date: Thu, 23 May 2019 05:33:16 +0000 Subject: [PATCH] Append field number to accessors if there is conflict Previously, foo_value and foo (wrapper field) will both have get/setFooValue --- .../proto/test_wrapper_type_setters.proto | 11 +++ php/tests/wrapper_type_setters_test.php | 38 +++++++++++ .../protobuf/compiler/php/php_generator.cc | 68 +++++++++++++++---- 3 files changed, 104 insertions(+), 13 deletions(-) diff --git a/php/tests/proto/test_wrapper_type_setters.proto b/php/tests/proto/test_wrapper_type_setters.proto index 41ca7f3f311e..119bd2547ac1 100644 --- a/php/tests/proto/test_wrapper_type_setters.proto +++ b/php/tests/proto/test_wrapper_type_setters.proto @@ -24,3 +24,14 @@ message TestWrapperSetters { map map_string_value = 13; } + +message TestWrapperAccessorConflicts { + int32 normal_vs_wrapper_value = 1; + google.protobuf.Int32Value normal_vs_wrapper = 2; + + int32 normal_vs_normal_value = 3; + int32 normal_vs_normal = 4; + + google.protobuf.Int32Value wrapper_vs_wrapper_value = 5; + google.protobuf.Int32Value wrapper_vs_wrapper = 6; +} diff --git a/php/tests/wrapper_type_setters_test.php b/php/tests/wrapper_type_setters_test.php index 5509a175a1bd..35e3a7dec151 100644 --- a/php/tests/wrapper_type_setters_test.php +++ b/php/tests/wrapper_type_setters_test.php @@ -16,6 +16,44 @@ class WrapperTypeSettersTest extends TestBase { + public function testConflictNormalVsWrapper() + { + $m = new Foo\TestWrapperAccessorConflicts(); + + $m->setNormalVsWrapperValue1(1); + $this->assertSame(1, $m->getNormalVsWrapperValue1()); + + $m->setNormalVsWrapperValue2(1); + $this->assertSame(1, $m->getNormalVsWrapperValue2()); + + $wrapper = new Int32Value(["value" => 1]); + $m->setNormalVsWrapper($wrapper); + $this->assertSame(1, $m->getNormalVsWrapper()->getValue()); + } + + public function testConflictNormalVsNormal() + { + $m = new Foo\TestWrapperAccessorConflicts(); + + $m->setNormalVsNormalValue(1); + $this->assertSame(1, $m->getNormalVsNormalValue()); + + $m->setNormalVsNormal(1); + $this->assertSame(1, $m->getNormalVsNormal()); + } + + public function testConflictWrapperVsWrapper() + { + $m = new Foo\TestWrapperAccessorConflicts(); + + $m->setWrapperVsWrapperValueValue(1); + $this->assertSame(1, $m->getWrapperVsWrapperValueValue()); + + $wrapper = new Int32Value(["value" => 1]); + $m->setWrapperVsWrapperValue5($wrapper); + $this->assertSame(1, $m->getWrapperVsWrapperValue5()->getValue()); + } + /** * @dataProvider gettersAndSettersDataProvider */ diff --git a/src/google/protobuf/compiler/php/php_generator.cc b/src/google/protobuf/compiler/php/php_generator.cc index d7e33b2c06b8..8e405608a853 100644 --- a/src/google/protobuf/compiler/php/php_generator.cc +++ b/src/google/protobuf/compiler/php/php_generator.cc @@ -655,27 +655,58 @@ void GenerateOneofField(const OneofDescriptor* oneof, io::Printer* printer) { void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor, io::Printer* printer) { + bool need_other_name_for_accessor = false; + bool need_other_name_for_wrapper_accessor = false; + const Descriptor* desc = field->containing_type(); + + if (!field->is_repeated() && + field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE && + IsWrapperType(field)) { + // Check if there is any field called xxx_value + const FieldDescriptor* other = + desc->FindFieldByName(StrCat(field->name(), "_value")); + if (other != NULL) { + need_other_name_for_wrapper_accessor = true; + } + } + + if (strings::EndsWith(field->name(), "_value")) { + std::size_t pos = (field->name()).find("_value"); + string name = (field->name()).substr(0, pos); + const FieldDescriptor* other = desc->FindFieldByName(name); + if (other != NULL && + !other->is_repeated() && + other->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE && + IsWrapperType(other)) { + need_other_name_for_accessor = true; + } + } + const OneofDescriptor* oneof = field->containing_oneof(); // Generate getter. if (oneof != NULL) { GenerateFieldDocComment(printer, field, is_descriptor, kFieldGetter); printer->Print( - "public function get^camel_name^()\n" + "public function get^camel_name^^field_number^()\n" "{\n" " return $this->readOneof(^number^);\n" "}\n\n", "camel_name", UnderscoresToCamelCase(field->name(), true), - "number", IntToString(field->number())); + "number", IntToString(field->number()), + "field_number", need_other_name_for_accessor ? + StrCat(field->number()) : ""); } else { GenerateFieldDocComment(printer, field, is_descriptor, kFieldGetter); printer->Print( - "public function get^camel_name^()\n" + "public function get^camel_name^^field_number^()\n" "{\n" " return $this->^name^;\n" "}\n\n", - "camel_name", UnderscoresToCamelCase(field->name(), true), "name", - field->name()); + "camel_name", UnderscoresToCamelCase(field->name(), true), + "name", field->name(), + "field_number", need_other_name_for_accessor ? + StrCat(field->number()) : ""); } // For wrapper types, generate an additional getXXXValue getter @@ -684,21 +715,28 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor, field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE && IsWrapperType(field)) { GenerateWrapperFieldGetterDocComment(printer, field); + printer->Print( - "public function get^camel_name^Value()\n" + "public function get^camel_name^Value^field_number1^()\n" "{\n" - " $wrapper = $this->get^camel_name^();\n" + " $wrapper = $this->get^camel_name^^field_number2^();\n" " return is_null($wrapper) ? null : $wrapper->getValue();\n" "}\n\n", - "camel_name", UnderscoresToCamelCase(field->name(), true)); + "camel_name", UnderscoresToCamelCase(field->name(), true), + "field_number1", need_other_name_for_wrapper_accessor ? + StrCat(field->number()) : "", + "field_number2", need_other_name_for_accessor ? + StrCat(field->number()) : ""); } // Generate setter. GenerateFieldDocComment(printer, field, is_descriptor, kFieldSetter); printer->Print( - "public function set^camel_name^($var)\n" + "public function set^camel_name^^field_number^($var)\n" "{\n", - "camel_name", UnderscoresToCamelCase(field->name(), true)); + "camel_name", UnderscoresToCamelCase(field->name(), true), + "field_number", need_other_name_for_accessor ? + StrCat(field->number()) : ""); Indent(printer); @@ -798,13 +836,17 @@ void GenerateFieldAccessor(const FieldDescriptor* field, bool is_descriptor, IsWrapperType(field)) { GenerateWrapperFieldSetterDocComment(printer, field); printer->Print( - "public function set^camel_name^Value($var)\n" + "public function set^camel_name^Value^field_number1^($var)\n" "{\n" " $wrappedVar = is_null($var) ? null : new \\^wrapper_type^(['value' => $var]);\n" - " return $this->set^camel_name^($wrappedVar);\n" + " return $this->set^camel_name^^field_number2^($wrappedVar);\n" "}\n\n", "camel_name", UnderscoresToCamelCase(field->name(), true), - "wrapper_type", LegacyFullClassName(field->message_type(), is_descriptor)); + "wrapper_type", LegacyFullClassName(field->message_type(), is_descriptor), + "field_number1", need_other_name_for_wrapper_accessor ? + StrCat(field->number()) : "", + "field_number2", need_other_name_for_accessor ? + StrCat(field->number()) : ""); } // Generate has method for proto2 only.