Raise "Proto Message has an Unknown Field" in certain situations.

The motivation for this change is to flag accidental loss of protobuf extensions when `use_fast_cpp_protos` is in use, to turn silent but potentially critical failures into noisy failures, for example:

```
Proto Message of type pybind11.test.NestRepeated has an Unknown Field with
parent of type pybind11.test.BaseMessage: base_msgs.1003
(pybind11_protobuf/tests/extension_nest_repeated.proto,
pybind11_protobuf/tests/extension.proto).
Please add the required `cc_proto_library` `deps`.
Only if there is no alternative to suppressing this error, use
`pybind11_protobuf::AllowUnknownFieldsFor("pybind11.test.NestRepeated", "base_msgs");`
(Warning: suppressions may mask critical bugs.)
```

See the updated "Protobuf Extensions" section in README.md for background and details.

check_unknown_fields.cc is mostly the work of @laramiel, with an initial implementation only by @rwgk.

`MessageMayContainExtensionsMemoized()` is for speed-up, but note that it also limits the scope of the Unknown Field detection (as explained in the updated README.md section).

PiperOrigin-RevId: 468553207

Author: rwgk

README.md

@@ -70,18 +70,58 @@ C++ native protobuf object with C++ when passed by `const &` or `const *`.
 
 ### Protobuf Extensions
 
-With `use_fast_cpp_protos`, any `py_proto_library` becomes implicitly dependent
-on the corresponding `cc_proto_library`, but this is currently not handled
-automatically, nor clearly diagnosed or formally enforced. In the absence of
+When `use_fast_cpp_protos` is in use, and
 protobuf extensions
-usually there is no problem: Python code tends to depend organically on
-a given `py_proto_library`, and pybind11-wrapped C++ code tends to depend
-organically on the corresponding `cc_proto_library`. However, when extensions
-are involved, a well-known pitfall is to accidentally omit the corresponding
-`cc_proto_library`. Currently this needs to be kept in mind as a pitfall
-(sorry), but the usual best-practice unit testing is likely to catch such
-situations. Once discovered, the fix is easy: add the `cc_proto_library`
-to the `deps` of the relevant `pybind_library` or `pybind_extension`.
+are involved, a well-known pitfall is that extensions are silently moved
+to the `proto2::UnknownFieldSet` when a message is deserialized in C++,
+but the `cc_proto_library` for the extensions is not linked in. The root
+cause is an asymmetry in the handling of Python protos vs C++ protos: when
+a Python proto is deserialized, both the Python descriptor pool and the C++
+descriptor pool are inspected, but when a C++ proto is deserialized, only
+the C++ descriptor pool is inspected. Until this asymmetry is resolved, the
+`cc_proto_library` for all extensions involved must be added to the `deps` of
+the relevant `pybind_library` or `pybind_extension`, but this is sufficiently
+unobvious to be a setup for regular accidents, potentially with critical
+consequences.
+
+To guard against the most common type of accident, native_proto_caster.h
+includes a safety mechanism that raises "Proto Message has an Unknown Field"
+in certain situations:
+
+* When `use_fast_cpp_protos` is in use,
+* a protobuf message is returned from C++ to Python,
+* the message involves protobuf extensions (recursively),
+* and the `proto2::UnknownFieldSet` for the message or any of its submessages
+  is not empty.
+
+`pybind11_protobuf::AllowUnknownFieldsFor` is an escape hatch for situations in
+which
+
+* unknown fields existed before the safety mechanism was
+  introduced.
+* unknown fields are needed in the future.
+
+An example of a full error message (with lines breaks here for readability):
+
+```
+Proto Message of type pybind11.test.NestRepeated has an Unknown Field with
+parent of type pybind11.test.BaseMessage: base_msgs.1003
+(pybind11_protobuf/tests/extension_nest_repeated.proto,
+pybind11_protobuf/tests/extension.proto).
+Please add the required `cc_proto_library` `deps`.
+Only if there is no alternative to suppressing this error, use
+`pybind11_protobuf::AllowUnknownFieldsFor("pybind11.test.NestRepeated", "base_msgs");`
+(Warning: suppressions may mask critical bugs.)
+```
+
+The current implementation is a compromise solution, trading off simplicity
+of implementation, runtime performance, and precision. Generally, the runtime
+overhead is expected to be very small, but fields flagged as unknown may not
+necessarily be in extensions.
+Alerting developers of new code to unknown fields is assumed to be generally
+helpful, but the unknown fields detection is limited to messages with
+extensions, to avoid the runtime overhead for the presumably much more common
+case that no extensions are involved.
 
 ### Enumerations
 

pybind11_protobuf/BUILD

@@ -25,6 +25,7 @@ pybind_library(
         "//visibility:public",
     ],
     deps = [
+        ":check_unknown_fields",
         ":enum_type_caster",
         ":proto_cast_util",
         "@com_google_protobuf//:protobuf",
@@ -58,6 +59,7 @@ pybind_library(
         "//conditions:default": [],
     }),
     deps = [
+        ":check_unknown_fields",
         "@com_google_absl//absl/container:flat_hash_map",
         "@com_google_absl//absl/strings",
         "@com_google_absl//absl/types:optional",
@@ -79,3 +81,20 @@ pybind_library(
         "@com_google_protobuf//:protobuf",
     ],
 )
+
+cc_library(
+    name = "check_unknown_fields",
+    srcs = ["check_unknown_fields.cc"],
+    hdrs = ["check_unknown_fields.h"],
+    visibility = [
+        "//visibility:private",
+    ],
+    deps = [
+        "@com_google_absl//absl/container:flat_hash_map",
+        "@com_google_absl//absl/container:flat_hash_set",
+        "@com_google_absl//absl/meta:type_traits",
+        "@com_google_absl//absl/strings",
+        "@com_google_absl//absl/synchronization",
+        "@com_google_protobuf//:protobuf",
+    ],
+)

pybind11_protobuf/check_unknown_fields.cc

@@ -0,0 +1,190 @@
+#include "pybind11_protobuf/check_unknown_fields.h"
+
+#include <cassert>
+#include <cstdint>
+#include <optional>
+#include <string>
+#include <vector>
+
+#include "google/protobuf/descriptor.h"
+#include "google/protobuf/message.h"
+#include "google/protobuf/unknown_field_set.h"
+#include "absl/container/flat_hash_map.h"
+#include "absl/container/flat_hash_set.h"
+#include "absl/strings/str_cat.h"
+#include "absl/strings/str_join.h"
+#include "absl/strings/string_view.h"
+#include "absl/synchronization/mutex.h"
+
+namespace pybind11_protobuf::check_unknown_fields {
+namespace {
+
+using AllowListSet = absl::flat_hash_set<std::string>;
+using MayContainExtensionsMap =
+    absl::flat_hash_map<const ::google::protobuf::Descriptor*, bool>;
+
+AllowListSet* GetAllowList() {
+  static auto* allow_list = new AllowListSet();
+  return allow_list;
+}
+
+std::string MakeAllowListKey(
+    absl::string_view top_message_descriptor_full_name,
+    absl::string_view unknown_field_parent_message_fqn) {
+  return absl::StrCat(top_message_descriptor_full_name, ":",
+                      unknown_field_parent_message_fqn);
+}
+
+/// Recurses through the message Descriptor class looking for valid extensions.
+/// Stores the result to `memoized`.
+bool MessageMayContainExtensionsRecursive(const ::google::protobuf::Descriptor* descriptor,
+                                          MayContainExtensionsMap* memoized) {
+  if (descriptor->extension_range_count() > 0) return true;
+
+  auto [it, inserted] = memoized->try_emplace(descriptor, false);
+  if (!inserted) {
+    return it->second;
+  }
+
+  for (int i = 0; i < descriptor->field_count(); i++) {
+    auto* fd = descriptor->field(i);
+    if (fd->cpp_type() != ::google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE) continue;
+    if (MessageMayContainExtensionsRecursive(fd->message_type(), memoized)) {
+      (*memoized)[descriptor] = true;
+      return true;
+    }
+  }
+
+  return false;
+}
+
+bool MessageMayContainExtensionsMemoized(const ::google::protobuf::Descriptor* descriptor) {
+  static auto* memoized = new MayContainExtensionsMap();
+  static absl::Mutex lock;
+  absl::MutexLock l(&lock);
+  return MessageMayContainExtensionsRecursive(descriptor, memoized);
+}
+
+struct HasUnknownFields {
+  HasUnknownFields(const ::google::protobuf::Descriptor* root_descriptor)
+      : root_descriptor(root_descriptor) {}
+
+  std::string FieldFQN() const { return absl::StrJoin(field_fqn_parts, "."); }
+  std::string FieldFQNWithFieldNumber() const {
+    return field_fqn_parts.empty()
+               ? absl::StrCat(unknown_field_number)
+               : absl::StrCat(FieldFQN(), ".", unknown_field_number);
+  }
+
+  bool FindUnknownFieldsRecursive(const ::google::protobuf::Message* sub_message,
+                                  uint32_t depth);
+
+  std::string BuildErrorMessage() const;
+
+  const ::google::protobuf::Descriptor* root_descriptor = nullptr;
+  const ::google::protobuf::Descriptor* unknown_field_parent_descriptor = nullptr;
+  std::vector<std::string> field_fqn_parts;
+  int unknown_field_number;
+};
+
+/// Recurses through the message fields class looking for UnknownFields.
+bool HasUnknownFields::FindUnknownFieldsRecursive(
+    const ::google::protobuf::Message* sub_message, uint32_t depth) {
+  const ::google::protobuf::Reflection& reflection = *sub_message->GetReflection();
+
+  // If there are unknown fields, stop searching.
+  const ::google::protobuf::UnknownFieldSet& unknown_field_set =
+      reflection.GetUnknownFields(*sub_message);
+  if (!unknown_field_set.empty()) {
+    unknown_field_parent_descriptor = sub_message->GetDescriptor();
+    field_fqn_parts.resize(depth);
+    unknown_field_number = unknown_field_set.field(0).number();
+    return true;
+  }
+
+  // If this message does not include submessages which allow extensions,
+  // then it cannot include unknown fields.
+  if (!MessageMayContainExtensionsMemoized(sub_message->GetDescriptor())) {
+    return false;
+  }
+
+  // Otherwise the method has to check all present fields, including
+  // extensions to determine if they include unknown fields.
+  std::vector<const ::google::protobuf::FieldDescriptor*> present_fields;
+  reflection.ListFields(*sub_message, &present_fields);
+
+  for (const auto* field : present_fields) {
+    if (field->cpp_type() != ::google::protobuf::FieldDescriptor::CPPTYPE_MESSAGE) {
+      continue;
+    }
+    if (field->is_repeated()) {
+      int field_size = reflection.FieldSize(*sub_message, field);
+      for (int i = 0; i != field_size; ++i) {
+        if (FindUnknownFieldsRecursive(
+                &reflection.GetRepeatedMessage(*sub_message, field, i),
+                depth + 1U)) {
+          field_fqn_parts[depth] = field->name();
+          return true;
+        }
+      }
+    } else if (FindUnknownFieldsRecursive(
+                   &reflection.GetMessage(*sub_message, field), depth + 1U)) {
+      field_fqn_parts[depth] = field->name();
+      return true;
+    }
+  }
+
+  return false;
+}
+
+std::string HasUnknownFields::BuildErrorMessage() const {
+  assert(unknown_field_parent_descriptor != nullptr);
+  assert(root_descriptor != nullptr);
+
+  std::string emsg = absl::StrCat(  //
+      "Proto Message of type ", root_descriptor->full_name(),
+      " has an Unknown Field");
+  if (root_descriptor != unknown_field_parent_descriptor) {
+    absl::StrAppend(&emsg, " with parent of type ",
+                    unknown_field_parent_descriptor->full_name());
+  }
+  absl::StrAppend(&emsg, ": ", FieldFQNWithFieldNumber(), " (",
+                  root_descriptor->file()->name());
+  if (root_descriptor->file() != unknown_field_parent_descriptor->file()) {
+    absl::StrAppend(&emsg, ", ",
+                    unknown_field_parent_descriptor->file()->name());
+  }
+  absl::StrAppend(
+      &emsg,
+      "). Please add the required `cc_proto_library` `deps`. "
+      "Only if there is no alternative to suppressing this error, use "
+      "`pybind11_protobuf::AllowUnknownFieldsFor(\"",
+      root_descriptor->full_name(), "\", \"", FieldFQN(),
+      "\");` (Warning: suppressions may mask critical bugs.)");
+
+  return emsg;
+}
+
+}  // namespace
+
+void AllowUnknownFieldsFor(absl::string_view top_message_descriptor_full_name,
+                           absl::string_view unknown_field_parent_message_fqn) {
+  GetAllowList()->insert(MakeAllowListKey(top_message_descriptor_full_name,
+                                          unknown_field_parent_message_fqn));
+}
+
+std::optional<std::string> CheckAndBuildErrorMessageIfAny(
+    const ::google::protobuf::Message* message) {
+  const auto* root_descriptor = message->GetDescriptor();
+  HasUnknownFields search{root_descriptor};
+  if (!search.FindUnknownFieldsRecursive(message, 0u)) {
+    return std::nullopt;
+  }
+  if (GetAllowList()->count(MakeAllowListKey(root_descriptor->full_name(),
+                                             search.FieldFQN())) != 0) {
+    return std::nullopt;
+  }
+  return search.BuildErrorMessage();
+}
+
+}  // namespace pybind11_protobuf::check_unknown_fields

pybind11_protobuf/check_unknown_fields.h

@@ -0,0 +1,19 @@
+#ifndef PYBIND11_PROTOBUF_CHECK_UNKNOWN_FIELDS_H_
+#define PYBIND11_PROTOBUF_CHECK_UNKNOWN_FIELDS_H_
+
+#include <optional>
+
+#include "google/protobuf/message.h"
+#include "absl/strings/string_view.h"
+
+namespace pybind11_protobuf::check_unknown_fields {
+
+void AllowUnknownFieldsFor(absl::string_view top_message_descriptor_full_name,
+                           absl::string_view unknown_field_parent_message_fqn);
+
+std::optional<std::string> CheckAndBuildErrorMessageIfAny(
+    const ::google::protobuf::Message* top_message);
+
+}  // namespace pybind11_protobuf::check_unknown_fields
+
+#endif  // PYBIND11_PROTOBUF_CHECK_UNKNOWN_FIELDS_H_

pybind11_protobuf/native_proto_caster.h

@@ -18,6 +18,7 @@
 
 #include "google/protobuf/message.h"
 #include "absl/strings/string_view.h"
+#include "pybind11_protobuf/check_unknown_fields.h"
 #include "pybind11_protobuf/enum_type_caster.h"
 #include "pybind11_protobuf/proto_caster_impl.h"
 
@@ -61,8 +62,8 @@ inline void ImportNativeProtoCasters() { InitializePybindProtoCastUtil(); }
 inline void AllowUnknownFieldsFor(
     absl::string_view top_message_descriptor_full_name,
     absl::string_view unknown_field_parent_message_fqn) {
-  // Preparation for cl/467099971.
-  // TODO(240452999): Submit cl/464678844.
+  check_unknown_fields::AllowUnknownFieldsFor(top_message_descriptor_full_name,
+                                              unknown_field_parent_message_fqn);
 }
 
 }  // namespace pybind11_protobuf

pybind11_protobuf/proto_cast_util.cc

@@ -22,6 +22,7 @@
 #include "absl/strings/str_replace.h"
 #include "absl/strings/str_split.h"
 #include "absl/types/optional.h"
+#include "pybind11_protobuf/check_unknown_fields.h"
 
 namespace py = pybind11;
 
@@ -801,6 +802,12 @@ py::handle GenericProtoCast(Message* src, py::return_value_policy policy,
     return GenericPyProtoCast(src, policy, parent, is_const);
   }
 
+  std::optional<std::string> emsg =
+      check_unknown_fields::CheckAndBuildErrorMessageIfAny(src);
+  if (emsg) {
+    throw py::value_error(*emsg);
+  }
+
   // If this is a dynamically generated proto, then we're going to need to
   // construct a mapping between C++ pool() and python pool(), and then
   // use the PyProto_API to make it work.

pybind11_protobuf/tests/BUILD

@@ -45,6 +45,22 @@ py_proto_library(
     deps = [":extension_proto"],
 )
 
+proto_library(
+    name = "extension_nest_repeated_proto",
+    srcs = ["extension_nest_repeated.proto"],
+    deps = [":extension_proto"],
+)
+
+cc_proto_library(
+    name = "extension_nest_repeated_cc_proto",
+    deps = [":extension_nest_repeated_proto"],
+)
+
+py_proto_library(
+    name = "extension_nest_repeated_py_pb2",
+    deps = [":extension_nest_repeated_proto"],
+)
+
 proto_library(
     name = "extension_in_other_file_in_deps_proto",
     srcs = ["extension_in_other_file_in_deps.proto"],
@@ -135,6 +151,7 @@ pybind_extension(
         ":extension_cc_proto",
         # Intentionally omitted: ":extension_in_other_file_cc_proto",
         ":extension_in_other_file_in_deps_cc_proto",
+        ":extension_nest_repeated_cc_proto",
         ":test_cc_proto",
         "@com_google_protobuf//:protobuf",
         "//pybind11_protobuf:native_proto_caster",
@@ -153,6 +170,7 @@ py_test(
     deps = [
         ":extension_in_other_file_in_deps_py_pb2",
         ":extension_in_other_file_py_pb2",
+        ":extension_nest_repeated_py_pb2",
         ":extension_py_pb2",
         ":test_py_pb2",  # fixdeps: keep - Direct dependency needed in open-source version, see https://github.com/grpc/grpc/issues/22811
         "@com_google_absl_py//absl/testing:absltest",