From 16c1526c6ac730a18b31ec7a78b03d83562106d9 Mon Sep 17 00:00:00 2001 From: Andrew Hilger Date: Tue, 17 Dec 2024 10:49:10 -0800 Subject: [PATCH] enforce no .value or .type fields in union MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: If a union has a field name `type` or `value`, it conflicts with the property methods inherited from `Union` base, causing ambiguous behavior. Therefore, validate against this. Note this validator wasn’t previously running against unions because it was applied with `struct_visitor`. Reviewed By: Filip-F Differential Revision: D67258434 fbshipit-source-id: 864b82f8117a7ca124b458aef2463ff37e333257 --- .../generate/t_mstch_python_generator.cc | 31 ++++++++++++------- .../thrift/test/thrift-python/union_test.py | 10 +++--- .../test/thrift-python/union_test.thrift | 2 ++ 3 files changed, 27 insertions(+), 16 deletions(-) diff --git a/third-party/thrift/src/thrift/compiler/generate/t_mstch_python_generator.cc b/third-party/thrift/src/thrift/compiler/generate/t_mstch_python_generator.cc index d82e0766dd0d72..78cd4146147e54 100644 --- a/third-party/thrift/src/thrift/compiler/generate/t_mstch_python_generator.cc +++ b/third-party/thrift/src/thrift/compiler/generate/t_mstch_python_generator.cc @@ -980,7 +980,12 @@ class python_mstch_enum_value : public mstch_enum_value { // Generator-specific validator that enforces "name" and "value" are not used // as enum member or union field names (thrift-py3). namespace enum_member_union_field_names_validator { -void validate(const t_named* node, const std::string& name, sema_context& ctx) { +template +void validate( + const t_named* node, + const std::string& name, + sema_context& ctx, + Pred&& field_name_predicate) { auto pyname = node->get_annotation("py3.name", &name); if (const t_const* annot = node->find_structured_annotation_or_null(kPythonNameUri)) { @@ -989,31 +994,33 @@ void validate(const t_named* node, const std::string& name, sema_context& ctx) { pyname = annotation_name->get_string(); } } - if (pyname == "name" || pyname == "value") { + if (field_name_predicate(pyname)) { ctx.report( *node, "enum-member-union-field-names-rule", diagnostic_level::error, "'{}' should not be used as an enum/union field name in thrift-py3. " "Use a different name or annotate the field with " - "`(py3.name=\"\")`", + "`@python.Name{{name=\"\"}}`", pyname); } } bool validate_enum(sema_context& ctx, const t_enum& enm) { + auto predicate = [](const auto& pyname) { + return pyname == "name" || pyname == "value"; + }; for (const t_enum_value* ev : enm.get_enum_values()) { - validate(ev, ev->get_name(), ctx); + validate(ev, ev->get_name(), ctx, predicate); } return true; } -// TODO(T176314881): this never does anything? -bool validate_structured(sema_context& ctx, const t_struct& s) { - if (!s.is_union()) { - return false; - } +bool validate_union(sema_context& ctx, const t_struct& s) { + auto predicate = [](const auto& pyname) { + return pyname == "type" || pyname == "value"; + }; for (const t_field& f : s.fields()) { - validate(&f, f.name(), ctx); + validate(&f, f.name(), ctx, predicate); } return true; } @@ -1096,8 +1103,8 @@ class t_mstch_python_generator : public t_mstch_generator { validator.add_program_visitor(validate_no_reserved_key_in_namespace); validator.add_enum_visitor( enum_member_union_field_names_validator::validate_enum); - validator.add_struct_visitor( - enum_member_union_field_names_validator::validate_structured); + validator.add_union_visitor( + enum_member_union_field_names_validator::validate_union); if (get_py3_namespace(program_).empty()) { validator.add_structured_definition_visitor( module_name_collision_validator::validate_named); diff --git a/third-party/thrift/src/thrift/test/thrift-python/union_test.py b/third-party/thrift/src/thrift/test/thrift-python/union_test.py index 563e3061e68abc..3c78e0282fd1db 100644 --- a/third-party/thrift/src/thrift/test/thrift-python/union_test.py +++ b/third-party/thrift/src/thrift/test/thrift-python/union_test.py @@ -357,15 +357,17 @@ def test_field_name_conflict(self) -> None: type_union.Type = 1 _assert_serialization_round_trip(self, immutable_serializer, type_union) - u = TestUnionAmbiguousValueFieldNameImmutable(value=42) + u = TestUnionAmbiguousValueFieldNameImmutable(value_=42) + self.assertEqual(u.value_, 42) + # this is now the catch-all accessor self.assertEqual(u.value, 42) with self.assertRaises(AttributeError): - u.type + u.type_ _assert_serialization_round_trip(self, immutable_serializer, u) - u2 = TestUnionAmbiguousValueFieldNameImmutable(type=123) + u2 = TestUnionAmbiguousValueFieldNameImmutable(type_=123) with self.assertRaises(AttributeError): - u2.value + u2.value_ _assert_serialization_round_trip(self, immutable_serializer, u2) def test_hash(self) -> None: diff --git a/third-party/thrift/src/thrift/test/thrift-python/union_test.thrift b/third-party/thrift/src/thrift/test/thrift-python/union_test.thrift index b900ad51ea0475..3631c1c5aa38f7 100644 --- a/third-party/thrift/src/thrift/test/thrift-python/union_test.thrift +++ b/third-party/thrift/src/thrift/test/thrift-python/union_test.thrift @@ -61,7 +61,9 @@ union TestUnionAmbiguousTypeFieldName { # (immutable) thrift-python union class implementation. See union_test.py for # the resulting behavior. union TestUnionAmbiguousValueFieldName { + @python.Name{name = "type_"} 1: i32 type; + @python.Name{name = "value_"} 2: i32 value; } # NOTE: Error on import (due to fake "EMPTY" enum value):