From 9a8c0967ef05ffc34403371bbd55cea2bff5fc0a Mon Sep 17 00:00:00 2001 From: Andrew Hilger Date: Thu, 6 Feb 2025 12:02:25 -0800 Subject: [PATCH] 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 --- thrift/compiler/sema/check_map_keys.cc | 34 ++++++++++++------- thrift/compiler/sema/standard_validator.cc | 1 + thrift/compiler/sema/standard_validator.h | 1 + .../compiler/test/standard_validator_test.cc | 31 ++++++++++++++++- 4 files changed, 53 insertions(+), 14 deletions(-) diff --git a/thrift/compiler/sema/check_map_keys.cc b/thrift/compiler/sema/check_map_keys.cc index 96259711a99..60bd5f1416c 100644 --- a/thrift/compiler/sema/check_map_keys.cc +++ b/thrift/compiler/sema/check_map_keys.cc @@ -180,27 +180,28 @@ std::vector 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(&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,7 +210,7 @@ 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, @@ -217,8 +218,8 @@ void check_key_value( 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 diff --git a/thrift/compiler/sema/standard_validator.cc b/thrift/compiler/sema/standard_validator.cc index 08873f67009..19405c7a9eb 100644 --- a/thrift/compiler/sema/standard_validator.cc +++ b/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(*ctx.parent()); diff --git a/thrift/compiler/sema/standard_validator.h b/thrift/compiler/sema/standard_validator.h index 00d89392f6a..68c3b08f98c 100644 --- a/thrift/compiler/sema/standard_validator.h +++ b/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); diff --git a/thrift/compiler/test/standard_validator_test.cc b/thrift/compiler/test/standard_validator_test.cc index 54b8bc1a8a3..b5548dd4663 100644 --- a/thrift/compiler/test/standard_validator_test.cc +++ b/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 INT_DUPE = { + 2: "Foo", + 4: "Bar", + 2: "Bar" + # expected-warning@-1: Duplicate key in map literal: `2` + } + + struct S { + 1: map ok_init = {}; + 2: map bad_init_no_err = INT_DUPE; + 3: map bad_init_should_err = { + FooBar.Foo: "Foo", FooBar.Bar: "Bar", FooBar.Bar: "Bar" + # expected-warning@-1: Duplicate key in map literal: `Bar` + }; + 4: map, i64> bad_init_list_key = { + [1, 1, 2]: 1, [1, 1, 2]: 2 + # expected-warning@-1: Duplicate key in map literal: `[1, ..., 2]` + }; + } + )"); +}