Fix (also replace - with _) and rename PythonPackageForDescriptor().

Add tests covering the situations in which the problem was observed.

PiperOrigin-RevId: 623374809

Author: rwgk

pybind11_protobuf/proto_cast_util.cc

@@ -18,8 +18,11 @@
 #include "absl/log/check.h"
 #include "absl/log/log.h"
 #include "absl/memory/memory.h"
+#include "absl/strings/match.h"
+#include "absl/strings/str_cat.h"
 #include "absl/strings/str_replace.h"
 #include "absl/strings/string_view.h"
+#include "absl/strings/strip.h"
 #include "absl/types/optional.h"
 #include "google/protobuf/descriptor.h"
 #include "google/protobuf/descriptor_database.h"
@@ -46,16 +49,24 @@ using ::google::protobuf::python::PyProto_API;
 using ::google::protobuf::python::PyProtoAPICapsuleName;
 
 namespace pybind11_protobuf {
-namespace {
 
-std::string PythonPackageForDescriptor(const FileDescriptor* file) {
-  std::vector<std::pair<const absl::string_view, std::string>> replacements;
-  replacements.emplace_back("/", ".");
-  replacements.emplace_back(".proto", "_pb2");
-  std::string name = file->name();
-  return absl::StrReplaceAll(name, replacements);
+std::string StripProtoSuffixFromDescriptorFileName(absl::string_view filename) {
+  if (absl::EndsWith(filename, ".protodevel")) {
+    return std::string(absl::StripSuffix(filename, ".protodevel"));
+  } else {
+    return std::string(absl::StripSuffix(filename, ".proto"));
+  }
+}
+
+std::string InferPythonModuleNameFromDescriptorFileName(
+    absl::string_view filename) {
+  std::string basename = StripProtoSuffixFromDescriptorFileName(filename);
+  absl::StrReplaceAll({{"-", "_"}, {"/", "."}}, &basename);
+  return absl::StrCat(basename, "_pb2");
 }
 
+namespace {
+
 // Resolves the class name of a descriptor via d->containing_type()
 py::object ResolveDescriptor(py::object p, const Descriptor* d) {
   return d->containing_type() ? ResolveDescriptor(p, d->containing_type())
@@ -299,7 +310,8 @@ py::module_ GlobalState::ImportCached(const std::string& module_name) {
 }
 
 py::object GlobalState::PyMessageInstance(const Descriptor* descriptor) {
-  auto module_name = PythonPackageForDescriptor(descriptor->file());
+  auto module_name =
+      InferPythonModuleNameFromDescriptorFileName(descriptor->file()->name());
   if (!module_name.empty()) {
     auto cached = import_cache_.find(module_name);
     if (cached != import_cache_.end()) {
@@ -567,7 +579,8 @@ void InitializePybindProtoCastUtil() {
 void ImportProtoDescriptorModule(const Descriptor* descriptor) {
   assert(PyGILState_Check());
   if (!descriptor) return;
-  auto module_name = PythonPackageForDescriptor(descriptor->file());
+  auto module_name =
+      InferPythonModuleNameFromDescriptorFileName(descriptor->file()->name());
   if (module_name.empty()) return;
   try {
     GlobalState::instance()->ImportCached(module_name);

pybind11_protobuf/proto_cast_util.h

@@ -29,6 +29,21 @@
 
 namespace pybind11_protobuf {
 
+// Strips ".proto" or ".protodevel" from the end of a filename.
+// Similar to
+// https://github.com/protocolbuffers/protobuf/blob/b375d010bf57a6d673125330ec47f6e6a7e03f5c/src/google/protobuf/compiler/code_generator.cc#L129-L136
+// which is not public, unfortunately. Providing a public function here until
+// that situation changes.
+std::string StripProtoSuffixFromDescriptorFileName(absl::string_view filename);
+
+// Returns the Python module name expected for a given .proto filename.
+// Similar to
+// https://github.com/protocolbuffers/protobuf/blob/b375d010bf57a6d673125330ec47f6e6a7e03f5c/src/google/protobuf/compiler/python/helpers.cc#L31-L35
+// which is not public, unfortunately. Providing a public function here until
+// that situation changes.
+std::string InferPythonModuleNameFromDescriptorFileName(
+    absl::string_view filename);
+
 // Simple helper. Caller has to ensure that the py_bytes argument outlives the
 // returned string_view.
 absl::string_view PyBytesAsStringView(pybind11::bytes py_bytes);

pybind11_protobuf/tests/BUILD

@@ -98,6 +98,21 @@ py_proto_library(
     deps = [":extension_in_other_file_proto"],
 )
 
+proto_library(
+    name = "we-love-dashes_proto",
+    srcs = ["we-love-dashes.proto"],
+)
+
+cc_proto_library(
+    name = "we_love_dashes_cc_proto",
+    deps = [":we-love-dashes_proto"],
+)
+
+py_proto_library(
+    name = "we-love-dashes_py_pb2",
+    deps = [":we-love-dashes_proto"],
+)
+
 # Tests for enum_type_caster
 
 pybind_extension(
@@ -342,3 +357,47 @@ py_test(
         requirement("absl_py"),
     ],
 )
+
+pybind_extension(
+    name = "we_love_dashes_module",
+    srcs = ["we_love_dashes_module.cc"],
+    deps = [
+        ":we_love_dashes_cc_proto",
+        "//pybind11_protobuf:native_proto_caster",
+    ],
+)
+
+py_test(
+    name = "we_love_dashes_cc_only_test",
+    srcs = ["we_love_dashes_cc_only_test.py"],
+    deps = [
+        ":we_love_dashes_module",
+        "@com_google_absl_py//absl/testing:absltest",
+        "@com_google_protobuf//:protobuf_python",
+        requirement("absl_py"),
+    ],
+)
+
+py_test(
+    name = "we_love_dashes_cc_and_py_in_deps_test",
+    srcs = ["we_love_dashes_cc_and_py_in_deps_test.py"],
+    deps = [
+        ":we_love_dashes_module",
+        ":we-love-dashes_py_pb2",  # fixdeps: keep
+        "@com_google_absl_py//absl/testing:absltest",
+        "@com_google_protobuf//:protobuf_python",
+        requirement("absl_py"),
+    ],
+)
+
+py_test(
+    name = "we_love_dashes_py_only_test",
+    srcs = ["we_love_dashes_py_only_test.py"],
+    deps = [
+        ":very_large_proto_module",
+        ":we-love-dashes_py_pb2",
+        "@com_google_absl_py//absl/testing:absltest",
+        "@com_google_protobuf//:protobuf_python",
+        requirement("absl_py"),
+    ],
+)

pybind11_protobuf/tests/we-love-dashes.proto

@@ -0,0 +1,7 @@
+syntax = "proto2";
+
+package pybind11.test;
+
+message TokenEffort {
+  optional int32 score = 1;
+}

pybind11_protobuf/tests/we_love_dashes_cc_and_py_in_deps_test.py

@@ -0,0 +1,21 @@
+# Copyright (c) 2024 The Pybind Development Team. All rights reserved.
+#
+# All rights reserved. Use of this source code is governed by a
+# BSD-style license that can be found in the LICENSE file.
+
+from absl.testing import absltest
+from pybind11_protobuf.tests import we_love_dashes_module
+
+# NOTE: ":we-love-dashes_py_pb2" is in deps but intentionally not imported here.
+
+
+class MessageTest(absltest.TestCase):
+
+  def test_return_then_pass(self):
+    msg = we_love_dashes_module.return_token_effort(234)
+    score = we_love_dashes_module.pass_token_effort(msg)
+    self.assertEqual(score, 234)
+
+
+if __name__ == '__main__':
+  absltest.main()

pybind11_protobuf/tests/we_love_dashes_cc_only_test.py

@@ -0,0 +1,29 @@
+# Copyright (c) 2024 The Pybind Development Team. All rights reserved.
+#
+# All rights reserved. Use of this source code is governed by a
+# BSD-style license that can be found in the LICENSE file.
+
+from absl.testing import absltest
+from google.protobuf.internal import api_implementation
+from pybind11_protobuf.tests import we_love_dashes_module
+
+
+class MessageTest(absltest.TestCase):
+
+  def test_return_then_pass(self):
+    if api_implementation.Type() == 'cpp':
+      msg = we_love_dashes_module.return_token_effort(234)
+      score = we_love_dashes_module.pass_token_effort(msg)
+      self.assertEqual(score, 234)
+    else:
+      with self.assertRaisesRegex(
+          TypeError,
+          r'^Cannot construct a protocol buffer message type'
+          r' pybind11\.test\.TokenEffort in python\.'
+          r' .*pybind11_protobuf\.tests\.we_love_dashes_pb2\?$',
+      ):
+        we_love_dashes_module.return_token_effort(0)
+
+
+if __name__ == '__main__':
+  absltest.main()

pybind11_protobuf/tests/we_love_dashes_module.cc

@@ -0,0 +1,26 @@
+// Copyright (c) 2024 The Pybind Development Team. All rights reserved.
+//
+// All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+#include <pybind11/pybind11.h>
+
+#include "pybind11_protobuf/native_proto_caster.h"
+#include "pybind11_protobuf/tests/we-love-dashes.pb.h"
+
+namespace {
+
+PYBIND11_MODULE(we_love_dashes_module, m) {
+  pybind11_protobuf::ImportNativeProtoCasters();
+
+  m.def("return_token_effort", [](int score) {
+    pybind11::test::TokenEffort msg;
+    msg.set_score(score);
+    return msg;
+  });
+
+  m.def("pass_token_effort",
+        [](const pybind11::test::TokenEffort& msg) { return msg.score(); });
+}
+
+}  // namespace

pybind11_protobuf/tests/we_love_dashes_py_only_test.py

@@ -0,0 +1,21 @@
+# Copyright (c) 2024 The Pybind Development Team. All rights reserved.
+#
+# All rights reserved. Use of this source code is governed by a
+# BSD-style license that can be found in the LICENSE file.
+
+from absl.testing import absltest
+
+from pybind11_protobuf.tests import very_large_proto_module
+from pybind11_protobuf.tests import we_love_dashes_pb2
+
+
+class MessageTest(absltest.TestCase):
+
+  def test_pass_proto2_message(self):
+    msg = we_love_dashes_pb2.TokenEffort(score=345)
+    space_used_estimate = very_large_proto_module.get_space_used_estimate(msg)
+    self.assertGreater(space_used_estimate, 0)
+
+
+if __name__ == '__main__':
+  absltest.main()