Validate enum names matching enclosing type name

Summary: This adds validation for enum values, simiar to the existing logic for field names. It also adds extra-validation flags to eventually allow checks (for both conflicting field names and enum value names) to become errors.

Reviewed By: Mizuchi

Differential Revision: D94785494

fbshipit-source-id: 1460c41ab8b9ea2ed5ce5707b4272d78223ef49c

Author: aristidisp

third-party/thrift/src/thrift/compiler/compiler.cc

@@ -217,6 +217,16 @@ void printUsageTo(FILE* stream) {
                   Ignore @thrift.AllowLegacyMissingUris when checking for
                   missing URIs. Requires missing_uris=error.
 
+                field_name_matches_enclosing_type=none|warn|error
+                  Action to take when a field in a structured type has the
+                  same name as the enclosing type.
+                  Default: warn
+
+                enum_value_name_matches_enclosing_type=none|warn|error
+                  Action to take when an enum value has the same name as the
+                  enclosing enum type.
+                  Default: none
+
                 warn_on_redundant_custom_default_values
                   DEPRECATED, prefer: redundant_custom_default_values=warn
 
@@ -1049,6 +1059,19 @@ std::string parse_args(
             continue;
           }
 
+          if (maybe_parse_validation_level_flag(
+                  /*flag=*/validator,
+                  /*prefix=*/"field_name_matches_enclosing_type",
+                  &sparams.field_name_matches_enclosing_type)) {
+            continue;
+          }
+
+          if (maybe_parse_validation_level_flag(
+                  /*flag=*/validator,
+                  /*prefix=*/"enum_value_name_matches_enclosing_type",
+                  &sparams.enum_value_name_matches_enclosing_type)) {
+            continue;
+          }
         } catch (const std::exception& e) {
           fmt::print(
               stderr,
@@ -1243,6 +1266,14 @@ void record_invocation_params(
   sema_params_metric.add(
       fmt::format(
           "ignore_missing_uri_opt_out={}", sparams.ignore_missing_uri_opt_out));
+  sema_params_metric.add(
+      fmt::format(
+          "field_name_matches_enclosing_type={}",
+          fmt::underlying(sparams.field_name_matches_enclosing_type)));
+  sema_params_metric.add(
+      fmt::format(
+          "enum_value_name_matches_enclosing_type={}",
+          fmt::underlying(sparams.enum_value_name_matches_enclosing_type)));
   sema_params_metric.add(
       "warn_on_redundant_custom_default_values=" +
       std::to_string(

third-party/thrift/src/thrift/compiler/sema/sema_context.h

@@ -198,6 +198,15 @@ struct sema_params {
   // If true, ignore @thrift.AllowLegacyMissingUris opt-out when checking for
   // missing URIs. Requires missing_uris to be set to error.
   bool ignore_missing_uri_opt_out = false;
+
+  // Action to take when a field in a structured type (struct, union, exception)
+  // has the same name as the enclosing type.
+  validation_level field_name_matches_enclosing_type = validation_level::warn;
+
+  // Action to take when an enum value has the same name as the enclosing enum
+  // type.
+  validation_level enum_value_name_matches_enclosing_type =
+      validation_level::none;
 };
 
 // An AST visitor context for semantic analysis. It combines diagnostics

third-party/thrift/src/thrift/compiler/sema/standard_validator.cc

@@ -808,6 +808,22 @@ void validate_enum_value_uniqueness(sema_context& ctx, const t_enum& node) {
   }
 }
 
+void validate_enum_value_name_not_enclosing_type(
+    sema_context& ctx, const t_enum& node) {
+  for (const t_enum_value& value : node.values()) {
+    if (value.name() == node.name()) {
+      ctx.report(
+          value,
+          validation_to_diagnostic_level(
+              ctx.sema_parameters().enum_value_name_matches_enclosing_type),
+          "Enum value `{}` has the same name as the enclosing type `{}`.",
+          value.name(),
+          node.name());
+      return;
+    }
+  }
+}
+
 void validate_enum_value(sema_context& ctx, const t_enum_value& node) {
   if (!node.has_value()) {
     ctx.error(
@@ -942,7 +958,10 @@ void validate_field_name(sema_context& ctx, const t_field& field) {
     } else {
       parent_structure = "struct";
     }
-    ctx.warning(
+    ctx.report(
+        field,
+        validation_to_diagnostic_level(
+            ctx.sema_parameters().field_name_matches_enclosing_type),
         "Field '{}' has the same name as the containing {}.",
         field.name(),
         parent_structure);
@@ -2292,6 +2311,7 @@ ast_validator standard_validator() {
 
   validator.add_enum_visitor(&validate_enum_value_name_uniqueness);
   validator.add_enum_visitor(&validate_enum_value_uniqueness);
+  validator.add_enum_visitor(&validate_enum_value_name_not_enclosing_type);
   validator.add_enum_visitor(&validate_reserved_ids_enum);
   validator.add_enum_value_visitor(&validate_enum_value);
 

third-party/thrift/src/thrift/compiler/test/compiler_test.cc

@@ -2624,22 +2624,37 @@ TEST(CompilerTest, alias_enum_in_external_const) {
   check_compile(name_contents_map, "baz.thrift");
 }
 
-TEST(CompilerTest, same_named_field) {
-  check_compile(R"(
+TEST(CompilerTest, same_named_field_error) {
+  check_compile(
+      R"(
     package "facebook.com/thrift/test"
     struct S {
       1: i32 S;
-      # expected-warning@-1: Field 'S' has the same name as the containing struct.
+      # expected-error@-1: Field 'S' has the same name as the containing struct.
     }
     union U {
       1: i32 U;
-      # expected-warning@-1: Field 'U' has the same name as the containing union.
+      # expected-error@-1: Field 'U' has the same name as the containing union.
     }
     exception E {
       1: i32 E;
-      # expected-warning@-1: Field 'E' has the same name as the containing exception.
+      # expected-error@-1: Field 'E' has the same name as the containing exception.
     }
-  )");
+  )",
+      {"--extra-validation", "field_name_matches_enclosing_type=error"});
+}
+
+TEST(CompilerTest, enum_value_name_matches_enclosing_type) {
+  check_compile(
+      R"(
+    package "facebook.com/thrift/test"
+    enum E {
+      E = 0,
+      # expected-warning@-1: Enum value `E` has the same name as the enclosing type `E`.
+      Other = 1,
+    }
+  )",
+      {"--extra-validation", "enum_value_name_matches_enclosing_type=warn"});
 }
 
 TEST(CompilerTest, cursor_serialization_adapter) {