From a1ba8d238a356491272c928b76a78a9b1142c98b Mon Sep 17 00:00:00 2001 From: Marcel Hlopko Date: Fri, 14 Apr 2023 02:21:11 -0700 Subject: [PATCH] Generate bindings under the correct package. Before this CL all messages were generated in the top-level crate module. With this change we generate messages under the module specified by the package declaration in the .proto file. Dots are interpreted as submodule separator in consistency with how C++ namespaces are handled. Note that name of the proto_library target still remains to be used as the crate name. This CL only adds crate submodules dependeing on the specified package. PiperOrigin-RevId: 524235162 --- rust/test/BUILD | 64 +++++++++++++++++++ rust/test/child.proto | 2 +- rust/test/cpp/accessors_test.rs | 7 +- rust/test/cpp/interop/main.rs | 7 +- rust/test/dots_in_package.proto | 35 ++++++++++ rust/test/no_package.proto | 35 ++++++++++ rust/test/parent.proto | 2 +- rust/test/shared/BUILD | 36 +++++++++++ rust/test/shared/child_parent_test.rs | 11 ++-- rust/test/shared/package_test.rs | 46 +++++++++++++ rust/test/shared/unittest_proto_test.rs | 4 +- rust/test/upb/accessors_test.rs | 6 +- .../protobuf/compiler/rust/generator.cc | 43 ++++++++++++- 13 files changed, 280 insertions(+), 18 deletions(-) create mode 100644 rust/test/dots_in_package.proto create mode 100644 rust/test/no_package.proto create mode 100644 rust/test/shared/package_test.rs diff --git a/rust/test/BUILD b/rust/test/BUILD index 37e02695c5492..9fca9bc2cc580 100644 --- a/rust/test/BUILD +++ b/rust/test/BUILD @@ -111,3 +111,67 @@ rust_cc_proto_library( ], deps = [":child_cc_proto"], ) + +proto_library( + name = "dots_in_package_proto", + testonly = True, + srcs = ["dots_in_package.proto"], +) + +cc_proto_library( + name = "dots_in_package_cc_proto", + testonly = True, + deps = [":dots_in_package_proto"], +) + +rust_cc_proto_library( + name = "dots_in_package_cc_rust_proto", + testonly = True, + visibility = [ + "//rust/test/cpp:__subpackages__", + "//rust/test/shared:__subpackages__", + ], + deps = [":dots_in_package_cc_proto"], +) + +rust_upb_proto_library( + name = "dots_in_package_upb_rust_proto", + testonly = True, + visibility = [ + "//rust/test/shared:__subpackages__", + "//rust/test/upb:__subpackages__", + ], + deps = [":dots_in_package_proto"], +) + +proto_library( + name = "no_package_proto", + testonly = True, + srcs = ["no_package.proto"], +) + +cc_proto_library( + name = "no_package_cc_proto", + testonly = True, + deps = [":no_package_proto"], +) + +rust_cc_proto_library( + name = "no_package_cc_rust_proto", + testonly = True, + visibility = [ + "//rust/test/cpp:__subpackages__", + "//rust/test/shared:__subpackages__", + ], + deps = [":no_package_cc_proto"], +) + +rust_upb_proto_library( + name = "no_package_upb_rust_proto", + testonly = True, + visibility = [ + "//rust/test/shared:__subpackages__", + "//rust/test/upb:__subpackages__", + ], + deps = [":no_package_proto"], +) diff --git a/rust/test/child.proto b/rust/test/child.proto index 5825818df50b2..dfb698ab94a12 100644 --- a/rust/test/child.proto +++ b/rust/test/child.proto @@ -30,7 +30,7 @@ syntax = "proto2"; -package third_party_protobuf_rust_test; +package child_package; import public "rust/test/parent.proto"; diff --git a/rust/test/cpp/accessors_test.rs b/rust/test/cpp/accessors_test.rs index 91384e3391af2..6d7dcf5362395 100644 --- a/rust/test/cpp/accessors_test.rs +++ b/rust/test/cpp/accessors_test.rs @@ -29,10 +29,11 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. /// Tests covering accessors for singular bool and int64 fields. +use unittest_proto::proto2_unittest::TestAllTypes; #[test] fn test_optional_int64_accessors() { - let mut msg = unittest_proto::TestAllTypes::new(); + let mut msg = TestAllTypes::new(); assert_eq!(msg.optional_int64(), None); msg.optional_int64_set(Some(42)); @@ -44,7 +45,7 @@ fn test_optional_int64_accessors() { #[test] fn test_optional_bool_accessors() { - let mut msg = unittest_proto::TestAllTypes::new(); + let mut msg = TestAllTypes::new(); assert_eq!(msg.optional_bool(), None); msg.optional_bool_set(Some(true)); @@ -56,7 +57,7 @@ fn test_optional_bool_accessors() { #[test] fn test_optional_bytes_accessors() { - let mut msg = unittest_proto::TestAllTypes::new(); + let mut msg = TestAllTypes::new(); assert_eq!(msg.optional_bytes(), None); msg.optional_bytes_set(Some(b"accessors_test")); diff --git a/rust/test/cpp/interop/main.rs b/rust/test/cpp/interop/main.rs index 92174ff5b3009..c32f4cae1e17d 100644 --- a/rust/test/cpp/interop/main.rs +++ b/rust/test/cpp/interop/main.rs @@ -29,6 +29,7 @@ // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. use std::ptr::NonNull; +use unittest_proto::proto2_unittest::TestAllTypes; macro_rules! assert_serializes_equally { ($msg:ident) => {{ @@ -43,14 +44,14 @@ macro_rules! assert_serializes_equally { #[test] fn mutate_message_in_cpp() { - let mut msg = unittest_proto::TestAllTypes::new(); + let mut msg = TestAllTypes::new(); unsafe { MutateInt64Field(msg.__unstable_cpp_repr_grant_permission_to_break()) }; assert_serializes_equally!(msg); } #[test] fn mutate_message_in_rust() { - let mut msg = unittest_proto::TestAllTypes::new(); + let mut msg = TestAllTypes::new(); msg.optional_int64_set(Some(43)); assert_serializes_equally!(msg); } @@ -58,7 +59,7 @@ fn mutate_message_in_rust() { #[test] fn deserialize_message_in_rust() { let serialized = unsafe { SerializeMutatedInstance() }; - let mut msg = unittest_proto::TestAllTypes::new(); + let mut msg = TestAllTypes::new(); msg.deserialize(&serialized).unwrap(); assert_serializes_equally!(msg); } diff --git a/rust/test/dots_in_package.proto b/rust/test/dots_in_package.proto new file mode 100644 index 0000000000000..e5d9fedf500e4 --- /dev/null +++ b/rust/test/dots_in_package.proto @@ -0,0 +1,35 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// 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. + +syntax = "proto2"; + +package package.uses.dots.submodule.separator; + +message Msg {} diff --git a/rust/test/no_package.proto b/rust/test/no_package.proto new file mode 100644 index 0000000000000..b42509810c61e --- /dev/null +++ b/rust/test/no_package.proto @@ -0,0 +1,35 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// 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. + +syntax = "proto2"; + +// package intentionally left unspecified. + +message MsgWithoutPackage {} diff --git a/rust/test/parent.proto b/rust/test/parent.proto index b30ca84fee958..55d67c0dfe471 100644 --- a/rust/test/parent.proto +++ b/rust/test/parent.proto @@ -30,6 +30,6 @@ syntax = "proto2"; -package third_party_protobuf_rust_test; +package parent_package; message Parent {} diff --git a/rust/test/shared/BUILD b/rust/test/shared/BUILD index 5e9591f2854a7..d924bb2223711 100644 --- a/rust/test/shared/BUILD +++ b/rust/test/shared/BUILD @@ -77,3 +77,39 @@ rust_test( "//rust/test:parent_cc_rust_proto", ], ) + +rust_test( + name = "package_cpp_test", + srcs = ["package_test.rs"], + tags = [ + # TODO(b/270274576): Enable testing on arm once we have a Rust Arm toolchain. + "not_build:arm", + # TODO(b/243126140): Enable tsan once we support sanitizers with Rust. + "notsan", + # TODO(b/243126140): Enable msan once we support sanitizers with Rust. + "nomsan", + ], + deps = [ + "//rust/test:dots_in_package_cc_rust_proto", + "//rust/test:no_package_cc_rust_proto", + "//rust/test:unittest_cc_rust_proto", + ], +) + +rust_test( + name = "package_upb_test", + srcs = ["package_test.rs"], + tags = [ + # TODO(b/270274576): Enable testing on arm once we have a Rust Arm toolchain. + "not_build:arm", + # TODO(b/243126140): Enable tsan once we support sanitizers with Rust. + "notsan", + # TODO(b/243126140): Enable msan once we support sanitizers with Rust. + "nomsan", + ], + deps = [ + "//rust/test:dots_in_package_upb_rust_proto", + "//rust/test:no_package_upb_rust_proto", + "//rust/test:unittest_upb_rust_proto", + ], +) diff --git a/rust/test/shared/child_parent_test.rs b/rust/test/shared/child_parent_test.rs index d55e52b9ded42..08446b1661b13 100644 --- a/rust/test/shared/child_parent_test.rs +++ b/rust/test/shared/child_parent_test.rs @@ -30,19 +30,20 @@ #[test] fn test_canonical_types() { - let _child = child_proto::Child::new(); - let _parent = parent_proto::Parent::new(); + let _child = child_proto::child_package::Child::new(); + let _parent = parent_proto::parent_package::Parent::new(); // Parent from child_proto crate should be the same type as Parent from // parent_proto crate. - let _parent_from_child: child_proto::Parent = parent_proto::Parent::new(); + let _parent_from_child: child_proto::child_package::Parent = + parent_proto::parent_package::Parent::new(); } #[test] fn test_parent_serialization() { - assert_eq!(*parent_proto::Parent::new().serialize(), []); + assert_eq!(*parent_proto::parent_package::Parent::new().serialize(), []); } #[test] fn test_child_serialization() { - assert_eq!(*child_proto::Child::new().serialize(), []); + assert_eq!(*child_proto::child_package::Child::new().serialize(), []); } diff --git a/rust/test/shared/package_test.rs b/rust/test/shared/package_test.rs new file mode 100644 index 0000000000000..d9e09778fa4cd --- /dev/null +++ b/rust/test/shared/package_test.rs @@ -0,0 +1,46 @@ +// Protocol Buffers - Google's data interchange format +// Copyright 2023 Google Inc. All rights reserved. +// https://developers.google.com/protocol-buffers/ +// +// 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. + +/// Tests covering proto packages. + +#[test] +fn test_package_specified() { + let _foo: unittest_proto::proto2_unittest::TestAllTypes; +} + +#[test] +fn test_empty_package() { + let _foo: no_package_proto::MsgWithoutPackage; +} + +#[test] +fn test_dots_in_package() { + let _foo: dots_in_package_proto::package::uses::dots::submodule::separator::Msg; +} diff --git a/rust/test/shared/unittest_proto_test.rs b/rust/test/shared/unittest_proto_test.rs index 20d8dbec39681..76d05f8fe7ab8 100644 --- a/rust/test/shared/unittest_proto_test.rs +++ b/rust/test/shared/unittest_proto_test.rs @@ -28,8 +28,10 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +use unittest_proto::proto2_unittest::TestAllTypes; + #[test] fn test_serialization() { - let test_all_types: unittest_proto::TestAllTypes = unittest_proto::TestAllTypes::new(); + let test_all_types = TestAllTypes::new(); assert_eq!(*test_all_types.serialize(), []); } diff --git a/rust/test/upb/accessors_test.rs b/rust/test/upb/accessors_test.rs index 5e8cc717a23ab..670b4e8a57389 100644 --- a/rust/test/upb/accessors_test.rs +++ b/rust/test/upb/accessors_test.rs @@ -28,9 +28,11 @@ // (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE // OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +use unittest_proto::proto2_unittest::TestAllTypes; + #[test] fn test_optional_bool() { - let mut test_all_types: unittest_proto::TestAllTypes = unittest_proto::TestAllTypes::new(); + let mut test_all_types: TestAllTypes = TestAllTypes::new(); test_all_types.optional_bool_set(Some(true)); assert_eq!(test_all_types.optional_bool(), Some(true)); @@ -43,7 +45,7 @@ fn test_optional_bool() { #[test] fn test_optional_int64() { - let mut test_all_types: unittest_proto::TestAllTypes = unittest_proto::TestAllTypes::new(); + let mut test_all_types: TestAllTypes = TestAllTypes::new(); test_all_types.optional_int64_set(Some(10)); assert_eq!(test_all_types.optional_int64(), Some(10)); diff --git a/src/google/protobuf/compiler/rust/generator.cc b/src/google/protobuf/compiler/rust/generator.cc index 6e8733a3fc21c..67e687044c934 100644 --- a/src/google/protobuf/compiler/rust/generator.cc +++ b/src/google/protobuf/compiler/rust/generator.cc @@ -37,7 +37,9 @@ #include "absl/algorithm/container.h" #include "absl/strings/str_cat.h" +#include "absl/strings/str_join.h" #include "absl/strings/str_replace.h" +#include "absl/strings/str_split.h" #include "absl/strings/string_view.h" #include "absl/strings/substitute.h" #include "absl/types/optional.h" @@ -105,6 +107,40 @@ std::string GetFileExtensionForKernel(Kernel kernel) { return ""; } +std::string RustModule(absl::string_view package) { + if (package.empty()) return ""; + return absl::StrCat("", absl::StrReplaceAll(package, {{".", "::"}})); +} + +std::string GetCrateRelativeQualifiedPath(const Descriptor* msg) { + std::string mod = RustModule(msg->file()->package()); + return absl::StrJoin({mod, msg->name()}, "::"); +} + +void EmitOpeningOfPackageModules(absl::string_view package, + google::protobuf::io::Printer& p) { + if (package.empty()) return; + for (absl::string_view segment : absl::StrSplit(package, '.')) { + p.Emit({{"segment", segment}}, + R"rs( + pub mod $segment$ { + )rs"); + } +} + +void EmitClosingOfPackageModules(absl::string_view package, + google::protobuf::io::Printer& p) { + if (package.empty()) return; + std::vector segments = absl::StrSplit(package, '.'); + absl::c_reverse(segments); + for (absl::string_view segment : segments) { + p.Emit({{"segment", segment}}, + R"rs( + } // mod $segment$ + )rs"); + } +} + std::string GetUnderscoreDelimitedFullName(const Descriptor* msg) { std::string result = msg->full_name(); absl::StrReplaceAll({{".", "_"}}, &result); @@ -496,6 +532,7 @@ bool RustGenerator::Generate(const FileDescriptor* file, extern crate std as __std; )rs"); + EmitOpeningOfPackageModules(file->package(), p); // TODO(b/270124215): Delete the following "placeholder impl" of `import // public`. Also make sure to figure out how to map FileDescriptor#name to @@ -506,9 +543,10 @@ bool RustGenerator::Generate(const FileDescriptor* file, for (int j = 0; j < dep->message_type_count(); ++j) { // TODO(b/272728844): Implement real logic p.Emit( - {{"crate", crate_name}, {"type_name", dep->message_type(j)->name()}}, + {{"crate", crate_name}, + {"pkg::Msg", GetCrateRelativeQualifiedPath(dep->message_type(j))}}, R"rs( - pub use $crate$::$type_name$; + pub use $crate$::$pkg::Msg$; )rs"); } } @@ -532,6 +570,7 @@ bool RustGenerator::Generate(const FileDescriptor* file, GenerateThunksForCpp(file, thunks); break; } + EmitClosingOfPackageModules(file->package(), p); return true; }