warn on duplicate keys in field initializer

Summary: Expand validator coverage to include `t_const_value*` from field initializers.

Reviewed By: createdbysk

Differential Revision: D69219405

fbshipit-source-id: c1a865ee9f28e764bd7810f0a5b424af3d6c057d

Author: ahilger

thrift/compiler/sema/check_map_keys.cc

@@ -180,27 +180,28 @@ std::vector<const t_const_value*> find_duplicate_keys(
   return duplicates;
 }
 
-// If owner is null, it's a nested "anonymous" constant value.
-// If owner is non-null and doesn't match enclosing t_const, it's
-// defined elsewhere.
-bool is_named_const_value(const t_const_value* value, const t_const& const_) {
+// If node is a field, only have to check if owner is non-null.
+// If node is a const, also have to check if owner is not the encolsing const.
+bool is_named_const_value(const t_const_value* value, const t_node& node) {
   auto owner = value->get_owner();
-  return owner != nullptr && owner != &const_;
+  if (owner == nullptr) {
+    return false;
+  }
+  // if node is a field, returns true b/c owner is non-null
+  return owner != dynamic_cast<const t_const*>(&node);
 }
 
 void check_key_value(
-    diagnostics_engine& diags,
-    const t_const& const_,
-    const t_const_value* value) {
+    diagnostics_engine& diags, const t_node& node, const t_const_value* value) {
   // recurse on elements
   if (value->kind() == t_const_value::CV_LIST) {
     for (const t_const_value* elem : value->get_list()) {
-      check_key_value(diags, const_, elem);
+      check_key_value(diags, node, elem);
     }
   }
   if (value->kind() == t_const_value::CV_MAP) {
     // Don't recurse or check constant defined elsewhere.
-    if (is_named_const_value(value, const_)) {
+    if (is_named_const_value(value, node)) {
       return;
     }
     auto duplicates = find_duplicate_keys(value);
@@ -209,16 +210,16 @@ void check_key_value(
       // fallback to the source range of the enclosing const.
       const source_range& src_range = duplicate->src_range()
           ? *duplicate->src_range()
-          : (value->src_range() ? *value->src_range() : const_.src_range());
+          : (value->src_range() ? *value->src_range() : node.src_range());
       // TODO(T213710219): Enable this with error severity
       diags.warning(
           src_range.begin,
           "Duplicate key in map literal: `{}`",
           to_string(duplicate));
     }
     for (const auto& kv : value->get_map()) {
-      check_key_value(diags, const_, kv.first);
-      check_key_value(diags, const_, kv.second);
+      check_key_value(diags, node, kv.first);
+      check_key_value(diags, node, kv.second);
     }
   }
 }
@@ -230,5 +231,12 @@ void check_map_keys(diagnostics_engine& diags, const t_const& const_) {
   check_key_value(diags, const_, const_.value());
 }
 
+void check_map_keys(diagnostics_engine& diags, const t_field& field_) {
+  if (field_.default_value() == nullptr) {
+    return;
+  }
+  check_key_value(diags, field_, field_.default_value());
+}
+
 } // namespace detail
 } // namespace apache::thrift::compiler

thrift/compiler/sema/standard_validator.cc

@@ -680,6 +680,7 @@ void validate_field_default_value(sema_context& ctx, const t_field& field) {
     // If initializer is not valid to begin with, stop checks and return error.
     return;
   }
+  detail::check_map_keys(ctx, field);
 
   const t_structured& parent_node =
       dynamic_cast<const t_structured&>(*ctx.parent());

thrift/compiler/sema/standard_validator.h

@@ -52,6 +52,7 @@ bool is_initializer_default_value(
  * const.
  */
 void check_map_keys(diagnostics_engine& diags, const t_const& const_);
+void check_map_keys(diagnostics_engine& diags, const t_field& field);
 
 void validate_annotation_scopes(sema_context& ctx, const t_named& node);
 

thrift/compiler/test/standard_validator_test.cc

@@ -122,7 +122,7 @@ TEST(StandardValidatorTest, ValidatePy3EnableCppAdapter) {
   )");
 }
 
-TEST(StandardValidatorTest, ConstMapKeyCollision) {
+TEST(StandardValidatorTest, ConstKeyCollision) {
   check_compile(R"(
     enum FooBar {
       Foo = 1,
@@ -199,3 +199,32 @@ TEST(StandardValidatorTest, ConstMapKeyCollision) {
 
   )");
 }
+
+TEST(StandardValidatorTest, FieldDefaultKeyCollision) {
+  check_compile(R"(
+    enum FooBar {
+      Foo = 1,
+      Bar = 2,
+    }
+
+    const map<i64, string> INT_DUPE = {
+      2: "Foo",
+      4: "Bar",
+      2: "Bar"
+    # expected-warning@-1: Duplicate key in map literal: `2`
+    }
+
+    struct S {
+      1: map<FooBar, string> ok_init = {};
+      2: map<i64, string> bad_init_no_err = INT_DUPE;
+      3: map<FooBar, string> bad_init_should_err = {
+        FooBar.Foo: "Foo", FooBar.Bar: "Bar", FooBar.Bar: "Bar"
+    # expected-warning@-1: Duplicate key in map literal: `Bar`
+      };
+      4: map<list<i64>, i64> bad_init_list_key = {
+        [1, 1, 2]: 1, [1, 1, 2]: 2
+    # expected-warning@-1: Duplicate key in map literal: `[1, ..., 2]`
+      };
+    }
+  )");
+}