thrift-compiler: Forbid implicit field IDs

Summary: This concludes the elimination of implicit field ID support in fbthrift. Reviewed By: yoney Differential Revision: D67014032 fbshipit-source-id: ef238308e0edd42c9c664552dc12c1dc48f48ac0
facebook · Dec 11, 2024 · fac786f · fac786f
1 parent 61184d0
commit fac786f
Show file tree

Hide file tree

Showing 4 changed files with 10 additions and 33 deletions.
diff --git a/third-party/thrift/src/thrift/compiler/compiler.cc b/third-party/thrift/src/thrift/compiler/compiler.cc
@@ -129,7 +129,8 @@ void usage() {
   --extra-validation
               Comma-separated list of opt-in validators to run. Recognized
               values include:
-                implicit_field_ids
+                implicit_field_ids (IGNORED: always present, i.e. implicit field
+                  IDs are always forbidden).
                 unstructured_annotations_on_field_type
 
 Available generators (and options):
@@ -384,7 +385,7 @@ std::string parse_args(
         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;
+          // no-op
         } else {
           fprintf(
               stderr, "!!! Unrecognized validator: %s\n\n", validator.c_str());

diff --git a/third-party/thrift/src/thrift/compiler/sema/sema_context.h b/third-party/thrift/src/thrift/compiler/sema/sema_context.h
@@ -90,7 +90,6 @@ class node_metadata_cache {
 
 struct sema_params {
   bool forbid_unstructured_annotations_on_field_types = false;
-  bool forbid_implicit_field_ids = false;
   bool skip_lowering_type_annotations = 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
@@ -659,23 +659,16 @@ void limit_terse_write_on_experimental_mode(
 }
 
 void validate_field_id(sema_context& ctx, const t_field& node) {
-  if (node.explicit_id() != node.id()) {
-    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(
+      node.explicit_id() == node.id(),
+      "No field id specified for `{}`",
+      node.name());
 
   ctx.check(
       node.id() != 0 ||
           node.has_annotation("cpp.deprecated_allow_zero_as_field_id"),
       "Zero value (0) not allowed as a field id for `{}`",
-      node.get_name());
+      node.name());
 
   ctx.check(
       node.id() >= t_field::min_id || node.is_injected(),

diff --git a/third-party/thrift/src/thrift/compiler/test/compiler_test.cc b/third-party/thrift/src/thrift/compiler/test/compiler_test.cc
@@ -105,20 +105,6 @@ TEST(CompilerTest, zero_as_field_id_neg_keys) {
 }
 
 TEST(CompilerTest, no_field_id) {
-  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-warning@-1: No field id specified for `field2`, resulting protocol may have conflicts or not be backwards compatible!
-    }
-
-    struct Bar {
-      i32 field4; # expected-warning: No field id specified for `field4`, resulting protocol may have conflicts or not be backwards compatible!
-    }
-  )");
-}
-
-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.
@@ -129,9 +115,7 @@ TEST(CompilerTest, no_field_id_with_validation) {
 
     struct Bar {
       i32 field4; # expected-error: No field id specified for `field4`
-    }
-  )",
-      {"--extra-validation", "implicit_field_ids"});
+    })");
 }
 
 TEST(CompilerTest, zero_as_field_id_annotation) {
@@ -150,7 +134,7 @@ TEST(CompilerTest, neg_field_ids) {
       R"(
       struct Foo {
         i32 f1;  // auto id = -1
-          # expected-warning@-1: No field id specified for `f1`, resulting protocol may have conflicts or not be backwards compatible!
+          # expected-error@-1: No field id specified for `f1`
 
         -2: i32 f2; // auto and manual id = -2
         -32: i32 f3; // min value.