Cleanup semantic analysis

Summary:
* Remove useless `standard_mutator_stage`.
* Replace hacky mutating `operator[]` with self-descriptive `add_stage` in `ast_mutators`.
* Make `stages_` private in `ast_mutators`.
* Merge validators and mutators into existing `sema` target since they are both are part of semantic analyzer and not very useful individually.
* Reduce public API surface by moving `validate_annotation_scopes` to the detail namespace.
* Apply naming conventions in a few places, fix includes and comments.

Reviewed By: yoney

Differential Revision: D61796181

fbshipit-source-id: e11d5b2c400f3d52636a2abe5b1543e408b066aa

thrift/compiler/sema/ast_mutator.h

@@ -16,7 +16,6 @@
 
 #pragma once
 
-#include <type_traits>
 #include <vector>
 
 #include <thrift/compiler/ast/ast_visitor.h>
@@ -93,17 +92,17 @@ struct type_ref_resolver {
         });
 
     mutator.add_function_visitor(
-        [&](diagnostic_context& ctx, mutator_context& mCtx, t_function& node) {
+        [&](diagnostic_context& ctx, mutator_context& mctx, t_function& node) {
           resolve_in_place(node.return_type());
           resolve_in_place(node.interaction());
           for (auto& field : node.params().fields()) {
-            mutator(ctx, mCtx, field);
+            mutator(ctx, mctx, field);
           }
         });
     mutator.add_throws_visitor(
-        [&](diagnostic_context& ctx, mutator_context& mCtx, t_throws& node) {
+        [&](diagnostic_context& ctx, mutator_context& mctx, t_throws& node) {
           for (auto& field : node.fields()) {
-            mutator(ctx, mCtx, field);
+            mutator(ctx, mctx, field);
           }
         });
     mutator.add_stream_visitor(
@@ -152,28 +151,21 @@ struct ast_mutators {
     bool unresolvable_typeref = false;
   };
 
+  std::vector<ast_mutator> stages_;
   bool use_legacy_type_ref_resolution_;
 
  public:
   explicit ast_mutators(bool use_legacy_type_ref_resolution)
       : use_legacy_type_ref_resolution_(use_legacy_type_ref_resolution) {}
 
-  std::vector<ast_mutator> stages;
-
-  // Access a specific mutator stage, growing the number of stages if needed.
-  template <typename T>
-  ast_mutator& operator[](T&& stage) {
-    static_assert(std::is_enum<T>::value, "");
-    auto index = static_cast<size_t>(stage);
-    if (stages.size() <= index) {
-      stages.resize(index + 1);
-    }
-    return stages[index];
+  ast_mutator& add_stage() {
+    stages_.push_back({});
+    return stages_.back();
   }
 
   mutation_result operator()(
       diagnostic_context& ctx, t_program_bundle& bundle) {
-    for (auto& stage : stages) {
+    for (auto& stage : stages_) {
       stage.mutate(ctx, bundle);
     }
     // We have no more mutators, so all type references **must** resolve.

thrift/compiler/sema/check_initializer.cc

@@ -310,11 +310,11 @@ class checker {
 // validation and inference, except the "runtime" is the code generator
 // runtime. Shit. I've been had.
 void detail::check_initializer(
-    diagnostic_context& ctx,
+    diagnostics_engine& diags,
     const t_named& node,
     const t_type* type,
     const t_const_value* initializer) {
-  checker(ctx, node, node.name()).check(type, initializer);
+  checker(diags, node, node.name()).check(type, initializer);
 }
 
 } // namespace apache::thrift::compiler

thrift/compiler/sema/explicit_include_validator.h

@@ -16,22 +16,20 @@
 
 #pragma once
 
-#include <memory>
-#include <string>
-#include <vector>
-
-#include <thrift/compiler/ast/t_named.h>
-#include <thrift/compiler/ast/t_type.h>
+#include <thrift/compiler/diagnostic.h>
 #include <thrift/compiler/sema/ast_validator.h>
-#include <thrift/compiler/sema/diagnostic_context.h>
 
 namespace apache::thrift::compiler {
 
+class diagnostic_context;
+class t_named;
+class t_type;
+
 /**
- * Transitive includes "work" in thrift C++, but they invite vexing
- * bugs if multiple transitive includes have the same module name. In other
- * langauges without transitive includes, they cause compilation errors (e.g.,
- * Rust). Thrift-python C API doesn't leak field types in headers, so transitive
+ * Transitive includes "work" in Thrift C++, but they invite vexing bugs if
+ * multiple transitive includes have the same module name. In other languages
+ * without transitive includes such as Rust, they cause compilation errors.
+ * Thrift-python C API doesn't leak field types in headers, so transitive
  * includes cause confusing compilation errors re: template specialization not
  * found.
  *

thrift/compiler/sema/scope_validator.cc

@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-#include <thrift/compiler/sema/scope_validator.h>
+#include <thrift/compiler/sema/standard_validator.h>
 
 #include <memory>
 #include <typeindex>
@@ -72,7 +72,8 @@ struct allowed_scopes {
 
 } // namespace
 
-void validate_annotation_scopes(diagnostic_context& ctx, const t_named& node) {
+void detail::validate_annotation_scopes(
+    diagnostic_context& ctx, const t_named& node) {
   // Ignore a transitive annotation definition because it is a collection of
   // annotations that apply at other scopes. For example:
   //

thrift/compiler/sema/standard_mutator.cc

@@ -23,7 +23,6 @@
 #include <thrift/compiler/lib/cpp2/util.h>
 #include <thrift/compiler/lib/schematizer.h>
 #include <thrift/compiler/lib/uri.h>
-#include <thrift/compiler/sema/standard_mutator_stage.h>
 
 namespace apache {
 namespace thrift {
@@ -435,24 +434,21 @@ void inject_schema_const(
 
 ast_mutators standard_mutators(bool use_legacy_type_ref_resolution) {
   ast_mutators mutators(use_legacy_type_ref_resolution);
-  {
-    auto& initial = mutators[standard_mutator_stage::initial];
-    initial.add_field_visitor(&lower_type_annotations<t_field>);
-    initial.add_typedef_visitor(&lower_type_annotations<t_typedef>);
-    initial.add_function_visitor(&normalize_return_type);
-    initial.add_named_visitor(&set_generated);
-    initial.add_named_visitor(&lower_deprecated_annotations);
-  }
 
-  {
-    auto& main = mutators[standard_mutator_stage::main];
-    main.add_struct_visitor(&mutate_terse_write_annotation_structured);
-    main.add_exception_visitor(&mutate_terse_write_annotation_structured);
-    main.add_struct_visitor(&mutate_inject_metadata_fields);
-    main.add_const_visitor(&match_const_type_with_value);
-    main.add_field_visitor(&match_field_type_with_default_value);
-    main.add_named_visitor(&match_annotation_types_with_const_values);
-  }
+  auto& initial = mutators.add_stage();
+  initial.add_field_visitor(&lower_type_annotations<t_field>);
+  initial.add_typedef_visitor(&lower_type_annotations<t_typedef>);
+  initial.add_function_visitor(&normalize_return_type);
+  initial.add_named_visitor(&set_generated);
+  initial.add_named_visitor(&lower_deprecated_annotations);
+
+  auto& main = mutators.add_stage();
+  main.add_struct_visitor(&mutate_terse_write_annotation_structured);
+  main.add_exception_visitor(&mutate_terse_write_annotation_structured);
+  main.add_struct_visitor(&mutate_inject_metadata_fields);
+  main.add_const_visitor(&match_const_type_with_value);
+  main.add_field_visitor(&match_field_type_with_default_value);
+  main.add_named_visitor(&match_annotation_types_with_const_values);
 
   return mutators;
 }

thrift/compiler/sema/standard_validator.cc

@@ -44,7 +44,6 @@
 #include <thrift/compiler/lib/reserved_identifier.h>
 #include <thrift/compiler/lib/uri.h>
 #include <thrift/compiler/sema/explicit_include_validator.h>
-#include <thrift/compiler/sema/scope_validator.h>
 
 namespace apache {
 namespace thrift {
@@ -1403,7 +1402,7 @@ ast_validator standard_validator() {
   validator.add_enum_value_visitor(&validate_enum_value);
 
   validator.add_named_visitor(&validate_structured_annotation);
-  validator.add_named_visitor(&validate_annotation_scopes);
+  validator.add_named_visitor(&detail::validate_annotation_scopes);
   validator.add_named_visitor(&validate_cpp_adapter_annotation);
   validator.add_named_visitor(&validate_hack_adapter_annotation);
   validator.add_named_visitor(&validate_hack_wrapper_annotation);

thrift/compiler/sema/standard_validator.h

@@ -24,11 +24,13 @@ namespace detail {
 // Checks if an initializer is compatible with a const or a field it
 // initializes.
 void check_initializer(
-    diagnostic_context& ctx,
+    diagnostics_engine& diags,
     const t_named& node,
     const t_type* type,
     const t_const_value* initializer);
 
+void validate_annotation_scopes(diagnostic_context& ctx, const t_named& node);
+
 } // namespace detail
 
 // The standard validator for Thrift.