Skip to content

Commit

Permalink
Add extra_validation: implicit_field_id
Browse files Browse the repository at this point in the history
Summary: Turns missing field ids into an error (instead of warning).

Reviewed By: thedavekwon

Differential Revision: D63488400

fbshipit-source-id: f5595ecc12a59d62f0ca5598673be366b3ab53e9
  • Loading branch information
Aristidis Papaioannou authored and facebook-github-bot committed Sep 27, 2024
1 parent 9147a5b commit 4788293
Show file tree
Hide file tree
Showing 4 changed files with 28 additions and 5 deletions.
2 changes: 2 additions & 0 deletions thrift/compiler/compiler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -389,6 +389,8 @@ std::string parse_args(
for (const auto& validator : validators) {
if (validator == "unstructured_annotations_on_field_type") {
sparams.forbid_unstructured_annotations_on_field_types = true;
} else if (validator == "implicit_field_ids") {
sparams.forbid_implicit_field_ids = true;
} else {
fprintf(
stderr, "!!! Unrecognized validator: %s\n\n", validator.c_str());
Expand Down
1 change: 1 addition & 0 deletions thrift/compiler/sema/sema_context.h
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ class node_metadata_cache {

struct sema_params {
bool forbid_unstructured_annotations_on_field_types = false;
bool forbid_implicit_field_ids = false;
};

// An AST visitor context for semantic analysis. It combines diagnostics
Expand Down
14 changes: 9 additions & 5 deletions thrift/compiler/sema/standard_validator.cc
Original file line number Diff line number Diff line change
Expand Up @@ -662,11 +662,15 @@ void limit_terse_write_on_experimental_mode(

void validate_field_id(sema_context& ctx, const t_field& node) {
if (node.explicit_id() != node.id()) {
ctx.warning(
node,
"No field id specified for `{}`, resulting protocol may have conflicts "
"or not be backwards compatible!",
node.name());
if (ctx.sema_parameters().forbid_implicit_field_ids) {
ctx.error(node, "No field id specified for `{}`", node.name());
} else {
ctx.warning(
node,
"No field id specified for `{}`, resulting protocol may have conflicts "
"or not be backwards compatible!",
node.name());
}
}

ctx.check(
Expand Down
16 changes: 16 additions & 0 deletions thrift/compiler/test/compiler_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -103,6 +103,22 @@ TEST(CompilerTest, no_field_id) {
)");
}

TEST(CompilerTest, no_field_id_with_validation) {
check_compile(
R"(
struct Experimental {} (thrift.uri = "facebook.com/thrift/annotation/Experimental") # expected-warning: The annotation thrift.uri is deprecated. Please use @thrift.Uri instead.
struct Foo {
@Experimental
i32 field2; # expected-error@-1: No field id specified for `field2`
}
struct Bar {
i32 field4; # expected-error: No field id specified for `field4`
}
)",
{"--extra-validation", "implicit_field_ids"});
}

TEST(CompilerTest, zero_as_field_id_annotation) {
check_compile(R"(
struct Foo {
Expand Down

0 comments on commit 4788293

Please sign in to comment.