Generalizing and implementing ValidateFeatureSupport for both Options and Features during proto parsing

karenwuz · copybara-github · commit ed3c57114d8e · 2025-12-16T12:28:48.000-08:00
PiperOrigin-RevId: 845387685
diff --git a/cmake/abseil-cpp.cmake b/cmake/abseil-cpp.cmake
@@ -92,5 +92,6 @@ else()
   )
   set(protobuf_ABSL_USED_TEST_TARGETS
     absl::scoped_mock_log
+    absl::status_matchers
   )
 endif ()
diff --git a/src/google/protobuf/BUILD.bazel b/src/google/protobuf/BUILD.bazel
@@ -1893,6 +1893,7 @@ cc_test(
         "@abseil-cpp//absl/log:die_if_null",
         "@abseil-cpp//absl/memory",
         "@abseil-cpp//absl/status",
+        "@abseil-cpp//absl/status:status_matchers",
         "@abseil-cpp//absl/status:statusor",
         "@abseil-cpp//absl/strings",
         "@googletest//:gtest",
diff --git a/src/google/protobuf/compiler/command_line_interface.cc b/src/google/protobuf/compiler/command_line_interface.cc
@@ -1367,6 +1367,7 @@ int CommandLineInterface::Run(int argc, const char* const argv[]) {
   descriptor_pool->EnforceWeakDependencies(true);
   descriptor_pool->EnforceSymbolVisibility(true);
   descriptor_pool->EnforceNamingStyle(true);
+  descriptor_pool->EnforceFeatureSupportValidation(true);
 
   if (!SetupFeatureResolution(*descriptor_pool)) {
     return EXIT_FAILURE;
diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc
@@ -1617,6 +1617,41 @@ TEST_F(CommandLineInterfaceTest, ImportOptions_MissingImport) {
   ExpectErrorSubstring("options.proto: File not found");
 }
 
+TEST_F(CommandLineInterfaceTest, ValidateFeatureSupportError) {
+  CreateTempFile("foo.proto",
+                 R"schema(
+    edition = "2023";
+    option features.field_presence = IMPLICIT;
+    message Foo {
+      int32 bar = 1 [
+        feature_support = {
+          edition_removed: EDITION_2023
+        }
+      ];
+    })schema");
+  Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir foo.proto");
+  ExpectErrorSubstring(
+      "foo.proto: Foo.bar has been removed but does not specify a removal "
+      "error.");
+}
+
+TEST_F(CommandLineInterfaceTest, ValidateFeatureSupportValid) {
+  CreateTempFile("foo.proto",
+                 R"schema(
+    edition = "2023";
+    option features.field_presence = IMPLICIT;
+    message Foo {
+      int32 bar = 1 [
+        feature_support = {
+          edition_removed: EDITION_2023
+          removal_error: "Custom removal error"
+        }
+      ];
+    })schema");
+  Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir foo.proto");
+  ExpectNoErrors();
+}
+
 TEST_F(CommandLineInterfaceTest, FeatureValidationError) {
   CreateTempFile("foo.proto",
                  R"schema(
diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc
@@ -1638,7 +1638,9 @@ class DescriptorPool::DeferredValidation {
   }
 
   bool Validate() {
-    if (lifetimes_info_map_.empty()) return true;
+    if (lifetimes_info_map_.empty()) {
+      return true;
+    }
 
     static absl::string_view feature_set_name = "google.protobuf.FeatureSet";
     bool has_errors = false;
@@ -5013,6 +5015,9 @@ class DescriptorBuilder {
                        const DescriptorProto::ExtensionRange& proto) {}
   void ValidateExtensionRangeOptions(const DescriptorProto& proto,
                                      const Descriptor& message);
+  void MaybeAddFeatureSupportError(const absl::Status& feature_support_status,
+                                   const Message& proto,
+                                   absl::string_view full_name);
   void ValidateExtensionDeclaration(
       absl::string_view full_name,
       const RepeatedPtrField<ExtensionRangeOptions_Declaration>& declarations,
@@ -8503,11 +8508,28 @@ void DescriptorBuilder::ValidateOptions(const OneofDescriptor* /*oneof*/,
 }
 
 
+void DescriptorBuilder::MaybeAddFeatureSupportError(
+    const absl::Status& feature_support_status, const Message& proto,
+    absl::string_view full_name) {
+  if (feature_support_status.ok()) {
+    return;
+  }
+  std::string feature_support_error(feature_support_status.message());
+  AddError(full_name, proto, DescriptorPool::ErrorCollector::OPTION_NAME,
+           feature_support_error.c_str());
+}
+
 void DescriptorBuilder::ValidateOptions(const FieldDescriptor* field,
                                         const FieldDescriptorProto& proto) {
   if (pool_->lazily_build_dependencies_ && (!field || !field->message_type())) {
     return;
   }
+  if (pool_->enforce_feature_support_validation_) {
+    absl::Status feature_support_status =
+        FeatureResolver::ValidateFieldFeatureSupport(*field);
+    MaybeAddFeatureSupportError(feature_support_status, proto,
+                                field->full_name());
+  }
 
   ValidateFieldFeatures(field, proto);
 
@@ -8838,10 +8860,15 @@ void DescriptorBuilder::ValidateOptions(const EnumDescriptor* enm,
   }
 }
 
-void DescriptorBuilder::ValidateOptions(
-    const EnumValueDescriptor* /* enum_value */,
-    const EnumValueDescriptorProto& /* proto */) {
-  // Nothing to do so far.
+void DescriptorBuilder::ValidateOptions(const EnumValueDescriptor* enum_value,
+                                        const EnumValueDescriptorProto& proto) {
+  if (pool_->enforce_feature_support_validation_) {
+    absl::Status feature_support_result =
+        FeatureResolver::ValidateFeatureSupport(
+            enum_value->options().feature_support(), enum_value->full_name());
+    MaybeAddFeatureSupportError(feature_support_result, proto,
+                                enum_value->full_name());
+  }
 }
 
 namespace {
diff --git a/src/google/protobuf/descriptor.h b/src/google/protobuf/descriptor.h
@@ -1594,9 +1594,9 @@ class PROTOBUF_EXPORT EnumValueDescriptor : private internal::SymbolBaseN<0>,
   EnumValueDescriptor& operator=(const EnumValueDescriptor&) = delete;
 #endif
 
-  absl::string_view name() const;   // Name of this enum constant.
-  int index() const;                // Index within the enums's Descriptor.
-  int number() const;               // Numeric value of this enum constant.
+  absl::string_view name() const;  // Name of this enum constant.
+  int index() const;               // Index within the enums's Descriptor.
+  int number() const;              // Numeric value of this enum constant.
 
   // The full_name of an enum value is a sibling symbol of the enum type.
   // e.g. the full name of FieldDescriptorProto::TYPE_INT32 is actually
@@ -2365,6 +2365,14 @@ class PROTOBUF_EXPORT DescriptorPool {
   // of this enforcement.
   void EnforceNamingStyle(bool enforce) { enforce_naming_style_ = enforce; }
 
+  // Enforce validation of feature support.
+  //
+  // This is used to guard feature support validation for the lifetimes of
+  // options and features.
+  void EnforceFeatureSupportValidation(bool enforce) {
+    enforce_feature_support_validation_ = enforce;
+  }
+
   // Enforce symbol visibility rules.  This will enable enforcement of the
   // `export` and `local` keywords added in edition 2024, honoring the behavior
   // of the `default_symbol_visibility` feature.
@@ -2670,6 +2678,7 @@ class PROTOBUF_EXPORT DescriptorPool {
   bool disallow_enforce_utf8_;
   bool deprecated_legacy_json_field_conflicts_;
   bool enforce_naming_style_;
+  bool enforce_feature_support_validation_ = false;
   bool enforce_symbol_visibility_ = false;
   mutable bool build_started_ = false;
 
@@ -3221,8 +3230,8 @@ PROTOBUF_EXPORT inline bool IsTrackingEnabled() {
 }
 
 template <typename F>
-auto VisitDescriptorsInFileOrder(const Descriptor* desc,
-                                 F& f) -> decltype(f(desc)) {
+auto VisitDescriptorsInFileOrder(const Descriptor* desc, F& f)
+    -> decltype(f(desc)) {
   for (int i = 0; i < desc->nested_type_count(); i++) {
     if (auto res = VisitDescriptorsInFileOrder(desc->nested_type(i), f)) {
       return res;
@@ -3238,8 +3247,8 @@ auto VisitDescriptorsInFileOrder(const Descriptor* desc,
 // If any call returns a "truthy" value, it stops visitation and returns that
 // value right away. Otherwise returns `{}` after visiting all types.
 template <typename F>
-auto VisitDescriptorsInFileOrder(const FileDescriptor* file,
-                                 F f) -> decltype(f(file->message_type(0))) {
+auto VisitDescriptorsInFileOrder(const FileDescriptor* file, F f)
+    -> decltype(f(file->message_type(0))) {
   for (int i = 0; i < file->message_type_count(); i++) {
     if (auto res = VisitDescriptorsInFileOrder(file->message_type(i), f)) {
       return res;
diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc
@@ -4904,6 +4904,154 @@ TEST(CustomOptions, DebugString) {
       descriptor->DebugString());
 }
 
+TEST(CustomOptions, FeatureSupportInvalidDeprecatedAfterRemoved) {
+  DescriptorPool pool;
+  pool.EnforceFeatureSupportValidation(true);
+
+  FileDescriptorProto file_proto;
+  FileDescriptorProto::descriptor()->file()->CopyTo(&file_proto);
+  ASSERT_TRUE(pool.BuildFile(file_proto) != nullptr);
+
+  ASSERT_TRUE(TextFormat::ParseFromString(
+      R"pb(
+        name: "foo.proto"
+        edition: EDITION_2024
+        package: "proto2_unittest"
+        dependency: "google/protobuf/descriptor.proto"
+        extension {
+          name: "file_opt1"
+          number: 7739974
+          label: LABEL_OPTIONAL
+          type: TYPE_UINT64
+          extendee: ".google.protobuf.FieldOptions"
+          options {
+            feature_support {
+              edition_introduced: EDITION_2023
+              edition_deprecated: EDITION_2024
+              deprecation_warning: "warning"
+              edition_removed: EDITION_2024
+              removal_error: "Custom feature removal error"
+            }
+          }
+        })pb",
+      &file_proto));
+
+  MockErrorCollector error_collector;
+  EXPECT_FALSE(pool.BuildFileCollectingErrors(file_proto, &error_collector));
+  EXPECT_EQ(error_collector.text_,
+            "foo.proto: proto2_unittest.file_opt1: OPTION_NAME: proto"
+            "2_unittest.file_opt1 was deprecated after it was removed.\n");
+}
+
+TEST(CustomOptions, FeatureSupportInvalidValueDeprecatedAfterOption) {
+  DescriptorPool pool;
+  pool.EnforceFeatureSupportValidation(true);
+
+  FileDescriptorProto file_proto;
+  FileDescriptorProto::descriptor()->file()->CopyTo(&file_proto);
+  ASSERT_TRUE(pool.BuildFile(file_proto) != nullptr);
+
+  ASSERT_TRUE(TextFormat::ParseFromString(
+      R"pb(
+        name: "foo.proto"
+        edition: EDITION_2024
+        package: "proto2_unittest"
+        dependency: "google/protobuf/descriptor.proto"
+        enum_type {
+          name: "Foo"
+          value { name: "UNKNOWN" number: 0 }
+          value {
+            name: "VALUE"
+            number: 1
+            options {
+              feature_support {
+                edition_deprecated: EDITION_99997_TEST_ONLY
+                deprecation_warning: "warning"
+              }
+            }
+          }
+        }
+        message_type {
+          name: "Bar"
+          extension {
+            name: "bool_field"
+            number: 7739973
+            label: LABEL_OPTIONAL
+            type: TYPE_ENUM
+            type_name: "Foo"
+            extendee: ".google.protobuf.FieldOptions"
+            options {
+              feature_support {
+                edition_introduced: EDITION_2023
+                edition_deprecated: EDITION_2024
+                deprecation_warning: "warning"
+              }
+            }
+          }
+        })pb",
+      &file_proto));
+
+  MockErrorCollector error_collector;
+  EXPECT_FALSE(pool.BuildFileCollectingErrors(file_proto, &error_collector));
+  EXPECT_THAT(error_collector.text_,
+              testing::HasSubstr(
+                  "foo.proto: proto2_unittest.Bar.bool_field: "
+                  "OPTION_NAME: value proto2_unittest.VALUE was "
+                  "deprecated after proto2_unittest.Bar.bool_field was.\n"));
+}
+
+TEST(CustomOptions, FeatureSupportValid) {
+  DescriptorPool pool;
+  pool.EnforceFeatureSupportValidation(true);
+
+  FileDescriptorProto file_proto;
+  FileDescriptorProto::descriptor()->file()->CopyTo(&file_proto);
+  ASSERT_TRUE(pool.BuildFile(file_proto) != nullptr);
+
+  ASSERT_TRUE(TextFormat::ParseFromString(
+      R"pb(
+        name: "foo.proto"
+        edition: EDITION_2024
+        package: "proto2_unittest"
+        dependency: "google/protobuf/descriptor.proto"
+        enum_type {
+          name: "Foo"
+          value { name: "UNKNOWN" number: 0 }
+          value {
+            name: "VALUE"
+            number: 1
+            options {
+              feature_support {
+                edition_introduced: EDITION_2024
+                edition_deprecated: EDITION_99997_TEST_ONLY
+                deprecation_warning: "warning"
+              }
+            }
+          }
+        }
+        message_type {
+          name: "Bar"
+          extension {
+            name: "bool_field"
+            number: 7739971
+            label: LABEL_OPTIONAL
+            type: TYPE_ENUM
+            type_name: "Foo"
+            extendee: ".google.protobuf.FieldOptions"
+            options {
+              feature_support {
+                edition_introduced: EDITION_2023
+                edition_removed: EDITION_99998_TEST_ONLY
+                removal_error: "removed"
+              }
+            }
+          }
+        })pb",
+      &file_proto));
+
+  EXPECT_NE(pool.BuildFile(file_proto), nullptr);
+}
+
 // ===================================================================
 
 TEST_F(ValidationErrorTest, AlreadyDefined) {
diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc
diff --git a/src/google/protobuf/feature_resolver.h b/src/google/protobuf/feature_resolver.h
diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc

Original file line number	Diff line number	Diff line change
`@@ -92,5 +92,6 @@ else()`
`92`	`92`	`)`
`93`	`93`	`set(protobuf_ABSL_USED_TEST_TARGETS`
`94`	`94`	`absl::scoped_mock_log`
	`95`	`+ absl::status_matchers`
`95`	`96`	`)`
`96`	`97`	`endif ()`