Clean up feature lifetime enforcement prior to release.

PiperOrigin-RevId: 625524817
protocolbuffers · Apr 17, 2024 · 03a620d · 03a620d
1 parent 7d24d5f
commit 03a620d
Show file tree

Hide file tree

Showing 3 changed files with 29 additions and 40 deletions.
diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc
@@ -95,8 +95,8 @@ absl::Status ValidateDescriptor(const Descriptor& descriptor) {
     }
 
     if (!field.options().has_feature_support()) {
-      // TODO Enforce that feature_support is always set.
-      return absl::OkStatus();
+      return Error("Feature field ", field.full_name(),
+                   " has no feature support specified.");
     }
 
     const FieldOptions::FeatureSupport& support =
@@ -347,6 +347,7 @@ absl::StatusOr<FeatureSetDefaults> FeatureResolver::CompileDefaults(
   }
   // Sanity check validation conditions above.
   ABSL_CHECK(!editions.empty());
+  // TODO Check that this is always EDITION_LEGACY.
   ABSL_CHECK_LE(*editions.begin(), EDITION_PROTO2);
 
   if (*editions.begin() > minimum_edition) {

diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc
@@ -365,35 +365,6 @@ TEST(FeatureResolverTest, CompileDefaultsOverridableNoSupportWindow) {
             pb::VALUE2);
 }
 
-TEST(FeatureResolverTest, CompileDefaultsOverridableEmptySupportWindow) {
-  absl::StatusOr<FeatureSetDefaults> defaults =
-      FeatureResolver::CompileDefaults(FeatureSet::descriptor(),
-                                       {GetExtension(pb::test)}, EDITION_PROTO2,
-                                       EDITION_2023);
-  ASSERT_OK(defaults);
-  ASSERT_EQ(defaults->defaults_size(), 3);
-
-  const auto& edition_defaults = defaults->defaults(2);
-  ASSERT_EQ(edition_defaults.edition(), EDITION_2023);
-
-  EXPECT_TRUE(edition_defaults.features()
-                  .GetExtension(pb::test)
-                  .has_empty_support_window_feature());
-  EXPECT_EQ(edition_defaults.features()
-                .GetExtension(pb::test)
-                .empty_support_window_feature(),
-            pb::VALUE2);
-  EXPECT_FALSE(edition_defaults.fixed_features()
-                   .GetExtension(pb::test)
-                   .has_empty_support_window_feature());
-  EXPECT_TRUE(edition_defaults.overridable_features()
-                  .GetExtension(pb::test)
-                  .has_empty_support_window_feature());
-  EXPECT_EQ(edition_defaults.overridable_features()
-                .GetExtension(pb::test)
-                .empty_support_window_feature(),
-            pb::VALUE2);
-}
 
 TEST(FeatureResolverTest, CompileDefaultsOverridable) {
   absl::StatusOr<FeatureSetDefaults> defaults =
@@ -1250,6 +1221,32 @@ TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidDefaultsAfterRemoved) {
                              HasSubstr("after it was removed"))));
 }
 
+// TODO Enable this once lifetimes are enforced internally.
+TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidWithMissingSupport) {
+  const FileDescriptor* file = ParseSchema(R"schema(
+    syntax = "proto2";
+    package test;
+    import "google/protobuf/descriptor.proto";
+
+    extend google.protobuf.FeatureSet {
+      optional Foo bar = 9999;
+    }
+    message Foo {
+      optional bool bool_field = 1 [
+        targets = TARGET_TYPE_FIELD,
+        edition_defaults = { edition: EDITION_LEGACY, value: "true" }
+      ];
+    }
+  )schema");
+  ASSERT_NE(file, nullptr);
+
+  const FieldDescriptor* ext = file->extension(0);
+  EXPECT_THAT(FeatureResolver::CompileDefaults(feature_set_, {ext},
+                                               EDITION_2023, EDITION_2023),
+              HasError(AllOf(HasSubstr("test.Foo.bool_field"),
+                             HasSubstr("no feature support"))));
+}
+
 TEST_F(FeatureResolverPoolTest,
        CompileDefaultsInvalidDefaultsScalarParsingError) {
   const FileDescriptor* file = ParseSchema(R"schema(

diff --git a/src/google/protobuf/unittest_features.proto b/src/google/protobuf/unittest_features.proto
@@ -200,13 +200,4 @@ message TestFeatures {
     edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" },
     edition_defaults = { edition: EDITION_2023, value: "VALUE2" }
   ];
-
-  optional EnumFeature empty_support_window_feature = 21 [
-    retention = RETENTION_RUNTIME,
-    targets = TARGET_TYPE_FILE,
-    targets = TARGET_TYPE_FIELD,
-    feature_support = {},
-    edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" },
-    edition_defaults = { edition: EDITION_2023, value: "VALUE2" }
-  ];
 }