From 9091819afa4352531478d8772a9d36b7febb9286 Mon Sep 17 00:00:00 2001 From: Shai Szulanski Date: Mon, 23 Sep 2024 11:30:18 -0700 Subject: [PATCH] Add option to forbid annotations on field type Summary: Introduces a mechanism for gating validators to allow gradual rollout of new restrictions. Uses that mechanism to gate field type annotations. Reviewed By: aristidisp Differential Revision: D51334853 fbshipit-source-id: de9095340e382e127c8cb2f85702806dc85639f6 --- .../thrift/src/thrift/compiler/compiler.cc | 34 +++++++++++++++++-- .../src/thrift/compiler/sema/sema_context.h | 8 +++++ .../compiler/sema/standard_validator.cc | 13 +++++++ .../src/thrift/compiler/test/compiler_test.cc | 17 ++++++++++ 4 files changed, 69 insertions(+), 3 deletions(-) diff --git a/third-party/thrift/src/thrift/compiler/compiler.cc b/third-party/thrift/src/thrift/compiler/compiler.cc index dcab1ce549d873..a3e8605602a348 100644 --- a/third-party/thrift/src/thrift/compiler/compiler.cc +++ b/third-party/thrift/src/thrift/compiler/compiler.cc @@ -135,6 +135,9 @@ void usage() { fprintf( stderr, " --inject-schema-const Inject generated schema constant (must use thrift2ast)\n"); + fprintf( + stderr, + " --extra-validation Comma-separated list of opt-in validators to run\n"); fprintf(stderr, "\n"); fprintf(stderr, "Available generators (and options):\n"); @@ -277,7 +280,8 @@ std::string parse_args( const std::vector& arguments, parsing_params& pparams, gen_params& gparams, - diagnostic_params& dparams) { + diagnostic_params& dparams, + sema_params& sparams) { // Check for necessary arguments, you gotta have at least a filename and // an output language flag. if (arguments.size() < 3 && gparams.targets.empty()) { @@ -374,6 +378,24 @@ std::string parse_args( gparams.add_gen_dir = (flag == "o"); } else if (flag == "inject-schema-const") { gparams.inject_schema_const = true; + } else if (flag == "extra-validation") { + const std::string* arg = consume_arg("extra validation"); + if (arg == nullptr) { + return {}; + } + std::vector validators; + boost::algorithm::split( + validators, *arg, [](char c) { return c == ','; }); + for (const auto& validator : validators) { + if (validator == "unstructured_annotations_on_field_type") { + sparams.forbid_unstructured_annotations_on_field_types = true; + } else { + fprintf( + stderr, "!!! Unrecognized validator: %s\n\n", validator.c_str()); + usage(); + return {}; + } + } } else { fprintf( stderr, "!!! Unrecognized option: %s\n\n", arguments[arg_i].c_str()); @@ -825,8 +847,10 @@ std::unique_ptr parse_and_get_program( pparams.allow_missing_includes = true; gen_params gparams; diagnostic_params dparams; + sema_params sparams; gparams.targets.push_back(""); // Avoid needing to pass --gen - std::string filename = parse_args(arguments, pparams, gparams, dparams); + std::string filename = + parse_args(arguments, pparams, gparams, dparams, sparams); if (filename.empty()) { return {}; } @@ -835,6 +859,7 @@ std::unique_ptr parse_and_get_program( sm, [](const diagnostic& d) { fmt::print(stderr, "{}\n", d); }, diagnostic_params::only_errors()); + ctx.sema_parameters() = std::move(sparams); return parse_ast(sm, ctx, filename, std::move(pparams)); } @@ -846,11 +871,14 @@ compile_result compile( parsing_params pparams; gen_params gparams; diagnostic_params dparams; - std::string input_filename = parse_args(arguments, pparams, gparams, dparams); + sema_params sparams; + std::string input_filename = + parse_args(arguments, pparams, gparams, dparams, sparams); if (input_filename.empty()) { return result; } sema_context ctx(source_mgr, result.detail, std::move(dparams)); + ctx.sema_parameters() = std::move(sparams); std::unique_ptr program_bundle = parse_and_mutate(source_mgr, ctx, input_filename, pparams, gparams); 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 1ae3f533bef781..28ac4128c370c5 100644 --- a/third-party/thrift/src/thrift/compiler/sema/sema_context.h +++ b/third-party/thrift/src/thrift/compiler/sema/sema_context.h @@ -90,6 +90,10 @@ class node_metadata_cache { } }; +struct sema_params { + bool forbid_unstructured_annotations_on_field_types = false; +}; + // An AST visitor context for semantic analysis. It combines diagnostics // reporting and node metadata cache. class sema_context : public diagnostics_engine, public const_visitor_context { @@ -125,8 +129,12 @@ class sema_context : public diagnostics_engine, public const_visitor_context { using diagnostics_engine::report; + const sema_params& sema_parameters() const { return params_; } + sema_params& sema_parameters() { return params_; } + private: node_metadata_cache cache_; + sema_params params_; }; } // namespace compiler 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 e9379a232abead..64667f9768fb39 100644 --- a/third-party/thrift/src/thrift/compiler/sema/standard_validator.cc +++ b/third-party/thrift/src/thrift/compiler/sema/standard_validator.cc @@ -1189,6 +1189,18 @@ struct ValidateAnnotationPositions { assert(false && "Unknown container type"); } } + void operator()(sema_context& ctx, const t_field& node) { + if (ctx.sema_parameters().forbid_unstructured_annotations_on_field_types && + owns_annotations(node.type()) && + std::any_of( + node.type()->annotations().begin(), + node.type()->annotations().end(), + [](const auto& pair) { + return pair.second.src_range.begin != source_location{}; + })) { + err(ctx); + } + } private: static void err(sema_context& ctx) { @@ -1375,6 +1387,7 @@ ast_validator standard_validator() { validator.add_field_visitor(&validate_required_field); validator.add_field_visitor(&validate_cpp_type_annotation); validator.add_field_visitor(&validate_field_name); + validator.add_field_visitor(ValidateAnnotationPositions{}); validator.add_enum_visitor(&validate_enum_value_name_uniqueness); validator.add_enum_visitor(&validate_enum_value_uniqueness); 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 789cb04ca44c59..ec52d4834934dc 100644 --- a/third-party/thrift/src/thrift/compiler/test/compiler_test.cc +++ b/third-party/thrift/src/thrift/compiler/test/compiler_test.cc @@ -693,15 +693,32 @@ TEST(CompilerTest, mixin_field_name_uniqueness) { TEST(CompilerTest, annotation_positions) { check_compile(R"( + struct Type {1: string name} (thrift.uri = "facebook.com/thrift/annotation/Type") # expected-warning: The annotation thrift.uri is deprecated. Please use @thrift.Uri instead. typedef set (annot)> T # expected-error: Annotations are not allowed in this position. Extract the type into a named typedef instead. const i32 (annot) C = 42 # expected-error: Annotations are not allowed in this position. Extract the type into a named typedef instead. service S { i32 (annot) foo() # expected-error: Annotations are not allowed in this position. Extract the type into a named typedef instead. void bar(1: i32 (annot) p) # expected-error: Annotations are not allowed in this position. Extract the type into a named typedef instead. } + struct Foo { + 1: i32 (annot) f + @Type{name="foo"} + 2: i32 g + } )"); } +TEST(CompilerTest, annotation_positions_field) { + check_compile( + R"( + struct Foo { + 1: i32 (annot) f # expected-error: Annotations are not allowed in this position. Extract the type into a named typedef instead. + 2: i32 g + } + )", + {"--extra-validation", "unstructured_annotations_on_field_type"}); +} + TEST(CompilerTest, performs_in_interaction) { check_compile(R"( interaction J {}