Remove unused extra_validation parameters

Summary: The corresponding logic has been enabled for weeks. Reviewed By: vitaut Differential Revision: D68611106 fbshipit-source-id: 4dd7d292338c1713e92bed70db9d8c6f23522acd
facebook · Jan 24, 2025 · 41747d8 · 41747d8
1 parent 9876ebc
commit 41747d8
Show file tree

Hide file tree

Showing 4 changed files with 7 additions and 88 deletions.
diff --git a/third-party/thrift/src/thrift/compiler/compiler.cc b/third-party/thrift/src/thrift/compiler/compiler.cc
@@ -136,28 +136,19 @@ void printUsageTo(FILE* stream) {
               values include:
                 unstructured_annotations_on_field_type
 
+                warn_on_redundant_custom_default_values
+                  Issues warnings for struct fields that have custom default
+                  values equal to the intrinsic default for that type (eg. 0
+                  for integers, false for boolean, etc.)
+
                 forbid_non_optional_cpp_ref_fields (IGNORED: set by default).
                   Struct (and exception) fields with a @cpp.Ref (or
                   cpp[2].ref[_type]) annotation must be optional, unless
                   annotated with @cpp.AllowLegacyNonOptionalRef.
 
-                  TEMPORARY NOTE: As of Jan 2025, this is enabled by default.
-                  As a short-term escape hatch, in case we missed any existing
-                  use case, users can specify this validator prefixed with a
-                  "-", to explicitly disable this enforcement, i.e.:
-                  "-forbid_non_optional_cpp_ref_fields"
-                  Note however that this is only a temporary, short-term option:
-                  support for "-forbid_non_optional_cpp_ref_fields" will be
-                  removed soon (and will fail if provided in the future).
-
                 implicit_field_ids (IGNORED: always present, i.e. implicit field
                   IDs are always forbidden).
 
-                warn_on_redundant_custom_default_values
-                  Issues warnings for struct fields that have custom default
-                  values equal to the intrinsic default for that type (eg. 0
-                  for integers, false for boolean, etc.)
-
 Available generators (and options):
 )");
   for (const auto& gen : generator_registry::get_generators()) {
@@ -433,10 +424,6 @@ std::string parse_args(
           sparams.warn_on_redundant_custom_default_values = true;
         } else if (validator == "forbid_non_optional_cpp_ref_fields") {
           // no-op
-        } else if (validator == "-forbid_non_optional_cpp_ref_fields") {
-          // DO_BEFORE(aristidis,20250201): Remove support for escape hatch:
-          // -forbid_non_optional_cpp_ref_fields
-          sparams.forbid_non_optional_cpp_ref_fields = false;
         } else if (validator == "implicit_field_ids") {
           // no-op
         } else {

diff --git a/third-party/thrift/src/thrift/compiler/sema/sema_context.h b/third-party/thrift/src/thrift/compiler/sema/sema_context.h
@@ -92,16 +92,6 @@ struct sema_params {
   bool forbid_unstructured_annotations_on_field_types = true;
   bool skip_lowering_annotations = false;
 
-  // DO_BEFORE(aristidis,20250201): Remove forbid_non_optional_cpp_ref_fields
-  // option when always true.
-  /**
-   * If true, standard_validator will report an error on struct (or exception)
-   * fields that have a C++ reference annotation (@cpp.Ref, cpp[2].ref[_type])
-   * and are NOT optional (unless they are annotated with
-   * @cpp.AllowLegacyNonOptionalRef).
-   */
-  bool forbid_non_optional_cpp_ref_fields = true;
-
   // If true, will issue a warning if a default value is explicitly specified
   // for a field, but that value is equal to the intrinsic default value.
   bool warn_on_redundant_custom_default_values = false;

diff --git a/third-party/thrift/src/thrift/compiler/sema/standard_validator.cc b/third-party/thrift/src/thrift/compiler/sema/standard_validator.cc
@@ -547,13 +547,11 @@ void validate_boxed_field_attributes(sema_context& ctx, const t_field& node) {
           "Make sure `{}` is optional.",
           node.name());
     } else if (ref) {
-      // For @cpp.Ref (and cpp[2].ref[_type]), optional fields result in either
-      // a warning on an error, depending on the validation parameters (see
-      // `forbid_non_optional_cpp_ref_fields`) and whether the field is
+      // For @cpp.Ref (and cpp[2].ref[_type]), non-optional fields result in
+      // either a warning or an error, depending on whether the field is
       // annotated with `@cpp.AllowLegacyNonOptionalRef`.
 
       const bool report_error =
-          ctx.sema_parameters().forbid_non_optional_cpp_ref_fields &&
           node.find_structured_annotation_or_null(
               kCppAllowLegacyNonOptionalRefUri) == nullptr;
 

diff --git a/third-party/thrift/src/thrift/compiler/test/compiler_test.cc b/third-party/thrift/src/thrift/compiler/test/compiler_test.cc
@@ -1287,62 +1287,6 @@ TEST(CompilerTest, unique_ref) {
   )");
 }
 
-TEST(CompilerTest, non_optional_ref_and_box) {
-  check_compile(
-      R"(
-    include "thrift/annotation/cpp.thrift"
-    include "thrift/annotation/thrift.thrift"
-
-    struct Bar {
-      1: Foo field1 (cpp.ref);
-        # expected-warning@-1: Field with @cpp.Ref (or similar) annotation should be optional: `field1` (in `Bar`).
-
-      2: Foo field2 (cpp2.ref);
-        # expected-warning@-1: Field with @cpp.Ref (or similar) annotation should be optional: `field2` (in `Bar`).
-
-      @cpp.Ref{type = cpp.RefType.Unique}
-      3: Foo field3;
-        # expected-warning@-2: Field with @cpp.Ref (or similar) annotation should be optional: `field3` (in `Bar`).
-
-      @cpp.Ref{type = cpp.RefType.Shared}
-      4: Foo field4;
-        # expected-warning@-2: Field with @cpp.Ref (or similar) annotation should be optional: `field4` (in `Bar`).
-
-      @cpp.Ref{type = cpp.RefType.SharedMutable}
-      5: Foo field5;
-        # expected-warning@-2: Field with @cpp.Ref (or similar) annotation should be optional: `field5` (in `Bar`).
-
-      6: Foo field6 (cpp.ref_type = "unique");
-        # expected-warning@-1: Field with @cpp.Ref (or similar) annotation should be optional: `field6` (in `Bar`).
-
-      7: Foo field7 (cpp2.ref_type = "unique");
-        # expected-warning@-1: Field with @cpp.Ref (or similar) annotation should be optional: `field7` (in `Bar`).
-
-      8: Foo field8 (cpp.ref_type = "shared");
-        # expected-warning@-1: Field with @cpp.Ref (or similar) annotation should be optional: `field8` (in `Bar`).
-
-      9: Foo field9 (cpp2.ref_type = "shared");
-        # expected-warning@-1: Field with @cpp.Ref (or similar) annotation should be optional: `field9` (in `Bar`).
-
-      10: Foo field10 (cpp.ref = "true");
-        # expected-warning@-1: Field with @cpp.Ref (or similar) annotation should be optional: `field10` (in `Bar`).
-
-      11: Foo field11 (cpp2.ref = "true");
-        # expected-warning@-1: Field with @cpp.Ref (or similar) annotation should be optional: `field11` (in `Bar`).
-
-      12: Foo field12;
-
-      @thrift.Box
-      13: Foo field13;
-        # expected-error@-2: The `thrift.box` annotation can only be used with optional fields. Make sure `field13` is optional.
-    }
-
-    struct Foo {}
-  )",
-
-      {"--extra-validation", "-forbid_non_optional_cpp_ref_fields"});
-}
-
 TEST(CompilerTest, non_optional_ref_and_box_forbidden) {
   check_compile(
       R"(