Clean up feature lifetime enforcement prior to release.

PiperOrigin-RevId: 625771423
protocolbuffers · Apr 17, 2024 · 71f9906 · 71f9906
1 parent 7d24d5f
commit 71f9906
Show file tree

Hide file tree

Showing 3 changed files with 7 additions and 80 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
@@ -335,65 +335,6 @@ TEST(FeatureResolverTest, CompileDefaultsFixedRemovedFeature) {
                    .has_removed_feature());
 }
 
-TEST(FeatureResolverTest, CompileDefaultsOverridableNoSupportWindow) {
-  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_no_support_window_feature());
-  EXPECT_EQ(edition_defaults.features()
-                .GetExtension(pb::test)
-                .no_support_window_feature(),
-            pb::VALUE2);
-  EXPECT_FALSE(edition_defaults.fixed_features()
-                   .GetExtension(pb::test)
-                   .has_no_support_window_feature());
-  EXPECT_TRUE(edition_defaults.overridable_features()
-                  .GetExtension(pb::test)
-                  .has_no_support_window_feature());
-  EXPECT_EQ(edition_defaults.overridable_features()
-                .GetExtension(pb::test)
-                .no_support_window_feature(),
-            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 =
@@ -977,8 +918,10 @@ TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidWithMissingSupport) {
   ASSERT_NE(file, nullptr);
 
   const FieldDescriptor* ext = file->extension(0);
-  EXPECT_OK(FeatureResolver::CompileDefaults(feature_set_, {ext}, EDITION_2023,
-                                             EDITION_2023));
+  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,

diff --git a/src/google/protobuf/unittest_features.proto b/src/google/protobuf/unittest_features.proto
@@ -192,21 +192,4 @@ message TestFeatures {
     edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" },
     edition_defaults = { edition: EDITION_2023, value: "VALUE2" }
   ];
-
-  optional EnumFeature no_support_window_feature = 20 [
-    retention = RETENTION_RUNTIME,
-    targets = TARGET_TYPE_FILE,
-    targets = TARGET_TYPE_FIELD,
-    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" }
-  ];
 }