Skip to content

Commit 2dc30c9

Browse files
iahsmeta-codesync[bot]
authored andcommitted
Add option to thrift_library to require URIs
Summary: Opt-in enforcement for package names / URIs. When `require_uris_transitively` is enabled, the extra validation ignores the AllowLegacyMissingUris annotation and requires that the root file and all transitive deps have URIs for all definitions that require them. This enables platforms using Dynamic Thrift features that error at runtime when URIs are missing to catch the errors at diff-time instead. Reviewed By: praihan Differential Revision: D95271039 fbshipit-source-id: a43954da1342e1b6ea1bc016bc67c46beb154f6f
1 parent a9e0825 commit 2dc30c9

4 files changed

Lines changed: 76 additions & 1 deletion

File tree

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,10 @@ void printUsageTo(FILE* stream) {
200200
the first namespace wins.
201201
Default: none
202202
203+
ignore_missing_uri_opt_out
204+
Ignore @thrift.AllowLegacyMissingUris when checking for
205+
missing URIs. Requires missing_uris=error.
206+
203207
warn_on_redundant_custom_default_values
204208
DEPRECATED, prefer: redundant_custom_default_values=warn
205209
@@ -1020,6 +1024,11 @@ std::string parse_args(
10201024
continue;
10211025
}
10221026

1027+
if (validator == "ignore_missing_uri_opt_out") {
1028+
sparams.ignore_missing_uri_opt_out = true;
1029+
continue;
1030+
}
1031+
10231032
} catch (const std::exception& e) {
10241033
fmt::print(
10251034
stderr,
@@ -1208,6 +1217,9 @@ void record_invocation_params(
12081217
fmt::format(
12091218
"duplicate_namespace={}",
12101219
fmt::underlying(sparams.duplicate_namespace)));
1220+
sema_params_metric.add(
1221+
fmt::format(
1222+
"ignore_missing_uri_opt_out={}", sparams.ignore_missing_uri_opt_out));
12111223
sema_params_metric.add(
12121224
"warn_on_redundant_custom_default_values=" +
12131225
std::to_string(

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -189,6 +189,10 @@ struct sema_params {
189189
// Action to take when a namespace is declared more than once for the same
190190
// language within a single file. If not `error`, the first namespace wins.
191191
validation_level duplicate_namespace = validation_level::none;
192+
193+
// If true, ignore @thrift.AllowLegacyMissingUris opt-out when checking for
194+
// missing URIs. Requires missing_uris to be set to error.
195+
bool ignore_missing_uri_opt_out = false;
192196
};
193197

194198
// An AST visitor context for semantic analysis. It combines diagnostics

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1040,8 +1040,9 @@ void validate_missing_uris(sema_context& ctx, const t_program& program) {
10401040
}
10411041
}
10421042

1043+
const bool ignoreOptOut = ctx.sema_parameters().ignore_missing_uri_opt_out;
10431044
const bool shouldReportMissingUri =
1044-
!packageHasAnnotation && !nodeHasAnnotation;
1045+
ignoreOptOut || (!packageHasAnnotation && !nodeHasAnnotation);
10451046
if (shouldReportMissingUri) {
10461047
ctx.report(
10471048
node,

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4107,3 +4107,61 @@ TEST(CompilerTest, duplicate_namespaces) {
41074107
)",
41084108
{"--extra-validation", "duplicate_namespace=error"});
41094109
}
4110+
4111+
/**
4112+
* Tests ignore_missing_uri_opt_out on a single file with types missing URIs.
4113+
* AllowLegacyMissingUris does not suppress errors.
4114+
*/
4115+
TEST(CompilerTest, ignore_missing_uri_opt_out_basic) {
4116+
check_compile(
4117+
R"(
4118+
include "thrift/annotation/thrift.thrift"
4119+
4120+
package;
4121+
# expected-warning@-1: Thrift file should have a (non-empty) package. Packages will soon be required, at which point missing packages will trigger a Thrift compiler error. For more details, see https://fburl.com/thrift-uri-add-package
4122+
4123+
// Valid: struct with URI
4124+
@thrift.Uri{value = "facebook.com/thrift/test/ValidStruct"}
4125+
struct ValidStruct { }
4126+
4127+
// Error: struct without URI
4128+
struct MissingUriStruct { }
4129+
# expected-error@-1: Definition `MissingUriStruct` requires a URI: add a non-empty package to the file, or annotate the type with @thrift.Uri. For more details, see https://fburl.com/thrift-uri-add-package
4130+
4131+
// Error: AllowLegacyMissingUris does NOT suppress when opt-out is ignored
4132+
@thrift.AllowLegacyMissingUris
4133+
struct LegacyAnnotatedStruct { }
4134+
# expected-error@-2: Definition `LegacyAnnotatedStruct` requires a URI: add a non-empty package to the file, or annotate the type with @thrift.Uri. For more details, see https://fburl.com/thrift-uri-add-package
4135+
4136+
// Error: enum without URI
4137+
enum MissingUriEnum { }
4138+
# expected-error@-1: Definition `MissingUriEnum` requires a URI: add a non-empty package to the file, or annotate the type with @thrift.Uri. For more details, see https://fburl.com/thrift-uri-add-package
4139+
)",
4140+
{"--extra-validation", "ignore_missing_uri_opt_out,missing_uris=error"});
4141+
}
4142+
4143+
/**
4144+
* Tests that @thrift.AllowLegacyMissingUris at both package and node level
4145+
* is ignored when ignore_missing_uri_opt_out is set.
4146+
*/
4147+
TEST(CompilerTest, ignore_missing_uri_opt_out_with_allow_legacy) {
4148+
check_compile(
4149+
R"(
4150+
include "thrift/annotation/thrift.thrift"
4151+
4152+
@thrift.AllowLegacyMissingUris
4153+
package;
4154+
# expected-warning@-2: Thrift file should have a (non-empty) package. Packages will soon be required, at which point missing packages will trigger a Thrift compiler error. For more details, see https://fburl.com/thrift-uri-add-package
4155+
4156+
// Error: package-level AllowLegacyMissingUris does not suppress
4157+
struct PkgAnnotatedStruct { }
4158+
# expected-error@-1: Definition `PkgAnnotatedStruct` requires a URI: add a non-empty package to the file, or annotate the type with @thrift.Uri. For more details, see https://fburl.com/thrift-uri-add-package
4159+
4160+
// Error: both package and node level AllowLegacyMissingUris do not suppress
4161+
@thrift.AllowLegacyMissingUris
4162+
struct BothAnnotatedStruct { }
4163+
# expected-error@-2: Unnecessary use of @thrift.AllowLegacyMissingUris on `BothAnnotatedStruct`: the annotation is already applied at the package (i.e., file) level.
4164+
# expected-error@-3: Definition `BothAnnotatedStruct` requires a URI: add a non-empty package to the file, or annotate the type with @thrift.Uri. For more details, see https://fburl.com/thrift-uri-add-package
4165+
)",
4166+
{"--extra-validation", "ignore_missing_uri_opt_out,missing_uris=error"});
4167+
}

0 commit comments

Comments
 (0)