Skip to content

Commit 6d7a7ef

Browse files
aristidispfacebook-github-bot
authored andcommitted
standard_validator: warn on custom default value in union
Summary: Defaults on `optional` fields cause jank. Explicit `optional` is banned in `union`, but all union fields are implicitly optional, so we should discourage defaults on union fields. Reviewed By: ahilger Differential Revision: D74677815 fbshipit-source-id: 654f4fd206b0202f5382a2712b0a75a659641d58
1 parent 165ec15 commit 6d7a7ef

File tree

2 files changed

+21
-0
lines changed

2 files changed

+21
-0
lines changed

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

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -667,6 +667,7 @@ std::string_view field_qualifier_to_string(t_field_qualifier qualifier) {
667667

668668
void validate_field_default_value(sema_context& ctx, const t_field& field) {
669669
if (field.default_value() == nullptr) {
670+
// Field does not have a default value => nothing to validate.
670671
return;
671672
}
672673

@@ -680,6 +681,16 @@ void validate_field_default_value(sema_context& ctx, const t_field& field) {
680681
const t_structured& parent_node =
681682
dynamic_cast<const t_structured&>(*ctx.parent());
682683

684+
if (parent_node.is_union()) {
685+
ctx.warning(
686+
field,
687+
"Union field is implicitly optional and should not have custom "
688+
"default value: `{}` (in union `{}`).",
689+
field.name(),
690+
parent_node.name());
691+
return;
692+
}
693+
683694
const t_field_qualifier field_qualifier = field.qualifier();
684695

685696
if (ctx.sema_parameters().warn_on_redundant_custom_default_values &&

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2325,6 +2325,16 @@ TEST(Compilertest, custom_default_values) {
23252325
# expected-warning@-1: Explicit default value is redundant for (optional) field: `j` (in `Widget`).
23262326
# expected-warning@-2: Optional field should not have custom default value: `j` (in `Widget`).
23272327
}
2328+
2329+
union UnionWidget {
2330+
1: i32 a;
2331+
2332+
2: i32 b = 0;
2333+
# expected-warning@-1: Union field is implicitly optional and should not have custom default value: `b` (in union `UnionWidget`).
2334+
2335+
3: bool c = true;
2336+
# expected-warning@-1: Union field is implicitly optional and should not have custom default value: `c` (in union `UnionWidget`).
2337+
}
23282338
)",
23292339
{"--extra-validation", "warn_on_redundant_custom_default_values"});
23302340
}

0 commit comments

Comments
 (0)