Add no_package extra-validation flag and rename missing_package to empty_or_no_package

Summary:
Add a new `no_package` extra-validation flag (default: `none`) that fires when a
.thrift file has no `package` directive at all — not even an explicitly empty
`package;`. This is distinct from the existing check (renamed from
`missing_package` to `empty_or_no_package`) which fires when the package has no
URI content.

Reviewed By: hchokshi

Differential Revision: D95601653

fbshipit-source-id: b4cdca23537f05e0aab8a1316a0430805bbd6140

Author: aristidisp

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

@@ -160,9 +160,22 @@ void printUsageTo(FILE* stream) {
                   Action to take on `required` (struct and exception) fields.
                   Default: warn
 
-                missing_package=none|warn|error
-                  Action to take on files that do not have a package.
+                no_package=none|warn|error
+                  Action to take on files that do not have any `package`
+                  directive (not even an empty one).
                   Default: warn
+                  NOTE: This will become an `error` soon, following the full
+                  deployment of the empty `package;` (with
+                  `@thrift.AllowLegacyMissingUris`).
+
+                empty_or_no_package=none|warn|error
+                  Action to take on files that either do not have a package
+                  (like no_package above) OR have an empty package (`package;`).
+                  Default: warn
+
+                  NOTE: if both empty_or_no_package and no_package validations
+                  are enabled, the higher level of validation will be applied
+                  to the (overlapping) case when there is no `package` at all.
 
                 missing_uris=none|warn|error
                   Action to take on types (struct, union, exception, enum)
@@ -970,8 +983,15 @@ std::string parse_args(
 
           if (maybe_parse_validation_level_flag(
                   /*flag=*/validator,
-                  /*prefix=*/"missing_package",
-                  &sparams.missing_package)) {
+                  /*prefix=*/"no_package",
+                  &sparams.no_package)) {
+            continue;
+          }
+
+          if (maybe_parse_validation_level_flag(
+                  /*flag=*/validator,
+                  /*prefix=*/"empty_or_no_package",
+                  &sparams.empty_or_no_package)) {
             continue;
           }
 
@@ -1191,9 +1211,12 @@ void record_invocation_params(
       fmt::format(
           "required_field_qualifier={}",
           fmt::underlying(sparams.required_field_qualifier)));
+  sema_params_metric.add(
+      fmt::format("no_package={}", fmt::underlying(sparams.no_package)));
   sema_params_metric.add(
       fmt::format(
-          "missing_package={}", fmt::underlying(sparams.missing_package)));
+          "empty_or_no_package={}",
+          fmt::underlying(sparams.empty_or_no_package)));
   sema_params_metric.add(
       fmt::format("missing_uris={}", fmt::underlying(sparams.missing_uris)));
   sema_params_metric.add(

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

@@ -157,8 +157,13 @@ struct sema_params {
   // Action to take on `required` (struct and exception) fields.
   validation_level required_field_qualifier = validation_level::warn;
 
-  // Action to take on files without a package.
-  validation_level missing_package = validation_level::warn;
+  // Action to take on files without any `package` directive (not even an empty
+  // one).
+  validation_level no_package = validation_level::warn;
+
+  // Action to take on files that either do not have a package (like no_package
+  // above) or have an empty package (`package;`).
+  validation_level empty_or_no_package = validation_level::warn;
 
   // Action to take on types (struct, union, exception, enum) missing Thrift
   // URIs (without the annotation that explicitly allows it, i.e.

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

@@ -455,15 +455,33 @@ void validate_python_namespaces(sema_context& ctx, const t_program& program) {
 
 void validate_program_package(sema_context& ctx, const t_program& program) {
   const t_package& package = program.package();
+  if (!package.is_explicit()) {
+    // No package directive at all. Both no_package and empty_or_no_package
+    // cover this case — report once at the higher level.
+    const sema_params::validation_level level = std::max(
+        ctx.sema_parameters().no_package,
+        ctx.sema_parameters().empty_or_no_package);
+    ctx.report(
+        program,
+        validation_to_diagnostic_level(level),
+        "Thrift file is missing a `package` directive. This will soon become "
+        "an error (see https://fburl.com/thrift-uri-add-package).");
+    return;
+  }
+
+  // There is a (potentially empty) `package` directive ...
   if (package.empty()) {
+    // Explicitly empty package (package;). Only empty_or_no_package applies.
     ctx.report(
         program,
-        validation_to_diagnostic_level(ctx.sema_parameters().missing_package),
-        "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");
+        validation_to_diagnostic_level(
+            ctx.sema_parameters().empty_or_no_package),
+        "Thrift file should have a (non-empty) package. This will soon become "
+        "an error (see https://fburl.com/thrift-uri-add-package).");
     return;
   }
+
+  // There is a non-empty `package ...` directive ...
   try {
     thrift::detail::check_univeral_name_domain(package.domain());
   } catch (const std::exception& e) {

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

@@ -1641,7 +1641,7 @@ TEST(CompilerTest, missing_uris_annotated_types) {
     include "thrift/annotation/thrift.thrift"
 
     package;
-    # 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
+    # expected-warning@-1: Thrift file should have a (non-empty) package. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
 
     // Valid: struct with URI
     @thrift.Uri{value = "facebook.com/thrift/test/ValidStruct"}
@@ -1702,7 +1702,7 @@ TEST(CompilerTest, missing_uris_annotated_package) {
 
     @thrift.AllowLegacyMissingUris
     package;
-    # 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
+    # expected-warning@-2: Thrift file should have a (non-empty) package. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
 
     // Valid: struct with explicit URI
     @thrift.Uri{value = "facebook.com/thrift/test/ValidStruct"}
@@ -1747,7 +1747,7 @@ TEST(CompilerTest, thrift_missing_uris_unnecessary_package_annotation) {
     @thrift.AllowLegacyMissingUris
     package;
     # expected-error@-2: Unnecessary use of @thrift.AllowLegacyMissingUris at the package level: there are no types who are missing URIs in the file.
-    # expected-warning@-3: 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
+    # expected-warning@-3: Thrift file should have a (non-empty) package. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
 
     // Struct has a URI, making the package annotation redundant.
     @thrift.Uri{value = "facebook.com/thrift/test/ValidStruct"}
@@ -1766,7 +1766,7 @@ TEST(CompilerTest, thrift_missing_uris_unnecessary_package_annotation_none) {
 
     @thrift.AllowLegacyMissingUris
     package;
-    # 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
+    # expected-warning@-2: Thrift file should have a (non-empty) package. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
 
     // Struct has a URI, making the package annotation redundant.
     @thrift.Uri{value = "facebook.com/thrift/test/ValidStruct"}
@@ -1785,7 +1785,7 @@ TEST(CompilerTest, thrift_missing_uris_redundant_package_and_type_annotation) {
 
     @thrift.AllowLegacyMissingUris
     package;
-    # 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
+    # expected-warning@-2: Thrift file should have a (non-empty) package. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
 
     // Annotation is redundant: package already has the annotation
     @thrift.AllowLegacyMissingUris
@@ -1807,7 +1807,7 @@ TEST(
 
     @thrift.AllowLegacyMissingUris
     package;
-    # 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
+    # expected-warning@-2: Thrift file should have a (non-empty) package. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
 
     // Annotation is redundant: package already has the annotation
     @thrift.AllowLegacyMissingUris
@@ -3045,7 +3045,7 @@ TEST(CompilerTest, base_service_defined_after_use) {
 TEST(CompilerTest, cpp_orderable) {
   check_compile(
       R"(
-# 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
+# expected-warning@-1: Thrift file is missing a `package` directive. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
 include "thrift/annotation/cpp.thrift"
 include "thrift/annotation/thrift.thrift"
 
@@ -3512,26 +3512,99 @@ TEST(CompilerTest, required_field_qualifier) {
       {"--extra-validation", "required_field_qualifier=warn"});
 }
 
-TEST(CompilerTest, missing_package) {
-  static const char* kMissingPackage =
-      "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";
+/**
+ * Tests the various validation behavior for missing package directives,
+ * depending on the `no_package` and `empty_or_no_package` validation flags.
+ */
+TEST(CompilerTest, no_package) {
+  // Missing directive, but no validation
+  check_compile(
+      R"(
+    struct TestStruct { }
+  )",
+      {"--extra-validation",
+       "empty_or_no_package=none,no_package=none,missing_uris=none"});
+
+  // Now with some validation. Note that the higher of `empty_or_no_package` and
+  // `no_package` takes effect.
   check_compile(
-      "struct TestStruct { }",
-      {"--extra-validation", "missing_package=none,missing_uris=none"});
+      R"(
+    # expected-warning@-1: Thrift file is missing a `package` directive. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
+    struct TestStruct { }
+  )",
+      {"--extra-validation",
+       "empty_or_no_package=warn,no_package=none,missing_uris=none"});
+
+  check_compile(
+      R"(
+    # expected-error@-1: Thrift file is missing a `package` directive. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
+    struct TestStruct { }
+  )",
+      {"--extra-validation",
+       "empty_or_no_package=warn,no_package=error,missing_uris=none"});
+}
+
+/**
+ * Tests the various validation behavior for explicitly empty package
+ * directives, depending on the `no_package` and `empty_or_no_package`
+ * validation flags.
+ */
+TEST(CompilerTest, empty_package) {
+  // Empty package directive, but no validation
   check_compile(
-      fmt::format(
-          "# expected-warning@1: {}\n"
-          "struct TestStruct {{ }}",
-          kMissingPackage),
-      {"--extra-validation", "missing_package=warn,missing_uris=none"});
+      R"(
+    package;
+
+    struct TestStruct { }
+  )",
+      {"--extra-validation",
+       "empty_or_no_package=none,no_package=none,missing_uris=none"});
+
+  // The no_package validation does not apply:
   check_compile(
-      fmt::format(
-          "# expected-error@1: {}\n"
-          "struct TestStruct {{ }}",
-          kMissingPackage),
-      {"--extra-validation", "missing_package=error,missing_uris=none"});
+      R"(
+    package;
+
+    struct TestStruct { }
+  )",
+      {"--extra-validation",
+       "empty_or_no_package=none,no_package=error,missing_uris=none"});
+
+  // The empty_or_no_package validation DOES apply
+  check_compile(
+      R"(
+    package;
+    # expected-warning@-1: Thrift file should have a (non-empty) package. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
+    struct TestStruct { }
+  )",
+      {"--extra-validation",
+       "empty_or_no_package=warn,no_package=error,missing_uris=none"});
+}
+
+/**
+ * Tests the various validation behavior for non-empty package directives,
+ * depending on the `no_package` and `empty_or_no_package` validation flags.
+ */
+TEST(CompilerTest, non_empty_package) {
+  // Non-empty package directive, but no validation
+  check_compile(
+      R"(
+    package "facebook.com/test"
+
+    struct TestStruct { }
+  )",
+      {"--extra-validation",
+       "empty_or_no_package=none,no_package=none,missing_uris=none"});
+
+  // Even with validation, no warnings/errors:
+  check_compile(
+      R"(
+    package "facebook.com/test"
+
+    struct TestStruct { }
+  )",
+      {"--extra-validation",
+       "empty_or_no_package=error,no_package=error,missing_uris=none"});
 }
 
 TEST(CompilerTest, bidirectional_streaming) {
@@ -4118,7 +4191,7 @@ TEST(CompilerTest, ignore_missing_uri_opt_out_basic) {
     include "thrift/annotation/thrift.thrift"
 
     package;
-    # 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
+    # expected-warning@-1: Thrift file should have a (non-empty) package. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
 
     // Valid: struct with URI
     @thrift.Uri{value = "facebook.com/thrift/test/ValidStruct"}
@@ -4151,7 +4224,7 @@ TEST(CompilerTest, ignore_missing_uri_opt_out_with_allow_legacy) {
 
     @thrift.AllowLegacyMissingUris
     package;
-    # 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
+    # expected-warning@-2: Thrift file should have a (non-empty) package. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
 
     // Error: package-level AllowLegacyMissingUris does not suppress
     struct PkgAnnotatedStruct { }

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

@@ -272,7 +272,7 @@ TEST(StandardValidatorTest, WarnProgramMissingPackage) {
   )");
 
   check_compile(R"(
-    # 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
+    # expected-warning@-1: Thrift file is missing a `package` directive. This will soon become an error (see https://fburl.com/thrift-uri-add-package).
   )");
 }