diff --git a/third-party/thrift/src/thrift/compiler/compiler.cc b/third-party/thrift/src/thrift/compiler/compiler.cc index 17d8a0dba1484d..ce6659697200f0 100644 --- 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 index cf7f66aaf42a99..17b7ff48b07597 100644 --- 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 index c709fe218f5b62..e4575c1b5ce467 100644 --- 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 index 93817ab7b1bf61..769841fc6f1cbd 100644 --- 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"(