From fac786f6eaf2987ce807e69fc628d61b0496874d Mon Sep 17 00:00:00 2001
From: Aristidis Papaioannou <aristidis@meta.com>
Date: Wed, 11 Dec 2024 09:10:46 -0800
Subject: [PATCH] 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
---
 .../thrift/src/thrift/compiler/compiler.cc    |  5 +++--
 .../src/thrift/compiler/sema/sema_context.h   |  1 -
 .../compiler/sema/standard_validator.cc       | 17 +++++-----------
 .../src/thrift/compiler/test/compiler_test.cc | 20 ++-----------------
 4 files changed, 10 insertions(+), 33 deletions(-)

diff --git a/third-party/thrift/src/thrift/compiler/compiler.cc b/third-party/thrift/src/thrift/compiler/compiler.cc
index 60dd0c196c3a6d..679dafa8c5ec3f 100644
--- 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
index dd22499e6316e4..a5db15994d09d3 100644
--- 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
index 4d80577a459e2d..19b7a28701db46 100644
--- 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
index c9102906e96c10..f41c93a7ba8367 100644
--- 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.