Skip to content

Commit

Permalink
Improve semantic analysis
Browse files Browse the repository at this point in the history 
Summary:
Semantic analysis is a collection of mutation and validation steps that should be run in a specific sequence. Exposing individual components is error-prone and unnecessarily increases public API surface. Improve the current situation by providing a single interface (facade) to semantic analysis (`sema`) and removing standard mutators from the public API.

Remove useless `ast_validator_test` - all of it is covered by functional (compiler) tests.

Remove deprecated `required` from LSP tests since now that validation is fixed we'll get diagnostics for those.

Reviewed By: thedavekwon

Differential Revision: D63469949

fbshipit-source-id: 4f96693e663a668420b2d26ff7c4272218810a70
  • Loading branch information
@vitaut @facebook-github-bot
vitaut authored and facebook-github-bot committed Sep 30, 2024
1 parent 9cb6b67 commit b26e26d
Show file tree
Hide file tree
Showing 10 changed files with 140 additions and 223 deletions.
2 changes: 1 addition & 1 deletion thrift/compiler/CMakeLists.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ add_library(
sema/check_initializer.cc
sema/explicit_include_validator.cc
sema/scope_validator.cc
sema/standard_mutator.cc
sema/sema.cc
sema/standard_validator.cc
)
target_link_libraries(
Expand Down
27 changes: 16 additions & 11 deletions thrift/compiler/compiler.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,10 @@
#include <thrift/compiler/diagnostic.h>
#include <thrift/compiler/generate/t_generator.h>
#include <thrift/compiler/parse/parse_ast.h>
#include <thrift/compiler/sema/ast_mutator.h>
#include <thrift/compiler/sema/ast_validator.h>
#include <thrift/compiler/sema/sema.h>
#include <thrift/compiler/sema/sema_context.h>
#include <thrift/compiler/sema/standard_mutator.h>
#include <thrift/compiler/sema/standard_validator.h>

namespace apache {
Expand Down Expand Up @@ -733,18 +734,20 @@ std::unique_ptr<t_program_bundle> parse_and_mutate(
ctx,
input_filename,
pparams,
true /* return_nullptr_on_failure */);
true /* return_nullptr_on_failure */,
nullptr,
&ctx.sema_parameters());
if (program_bundle == nullptr) {
return nullptr;
}

// Load standard library if available.
static const std::string kSchemaPath = "thrift/lib/thrift/schema.thrift";
static const std::string schema_path = "thrift/lib/thrift/schema.thrift";
auto found_or_error =
source_mgr.find_include_file(kSchemaPath, "", pparams.incl_searchpath);
source_mgr.find_include_file(schema_path, "", pparams.incl_searchpath);
if (found_or_error.index() == 0) {
// Found
if (!program_bundle->find_program(kSchemaPath)) {
if (!program_bundle->find_program(schema_path)) {
sema_context stdlib_ctx(
source_mgr,
[&](diagnostic&& d) {
Expand All @@ -758,7 +761,7 @@ std::unique_ptr<t_program_bundle> parse_and_mutate(
std::unique_ptr<t_program_bundle> inc = parse_and_mutate_program(
source_mgr,
stdlib_ctx,
kSchemaPath,
schema_path,
pparams,
true /* return_nullptr_on_failure */,
program_bundle.get());
Expand All @@ -785,7 +788,6 @@ std::unique_ptr<t_program_bundle> parse_and_mutate(
program_bundle->root_program()->set_include_prefix(
get_include_path(gparams.targets, input_filename));

standard_validator()(ctx, *program_bundle->root_program());
return ctx.has_errors() ? nullptr : std::move(program_bundle);
}

Expand All @@ -797,7 +799,8 @@ std::unique_ptr<t_program_bundle> parse_and_mutate_program(
const std::string& filename,
parsing_params params,
bool return_nullptr_on_failure,
t_program_bundle* already_parsed) {
t_program_bundle* already_parsed,
const sema_params* sparams) {
bool use_legacy_type_ref_resolution = params.use_legacy_type_ref_resolution;
auto programs =
parse_ast(sm, diags, filename, std::move(params), already_parsed);
Expand All @@ -806,9 +809,11 @@ std::unique_ptr<t_program_bundle> parse_and_mutate_program(
return !return_nullptr_on_failure ? std::move(programs) : nullptr;
}
auto ctx = sema_context(diags);
auto result =
standard_mutators(use_legacy_type_ref_resolution)(ctx, *programs);
if (result.unresolvable_typeref && return_nullptr_on_failure) {
if (sparams) {
ctx.sema_parameters() = *sparams;
}
auto result = sema(use_legacy_type_ref_resolution).run(ctx, *programs);
if (result.unresolved_types && return_nullptr_on_failure) {
// Stop processing if there is unresolvable typeref.
programs = nullptr;
}
Expand Down
5 changes: 4 additions & 1 deletion thrift/compiler/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ namespace apache {
namespace thrift {
namespace compiler {

struct sema_params;
class sema_context;
class t_program_bundle;

enum class compile_retcode {
Expand Down Expand Up @@ -68,7 +70,8 @@ std::unique_ptr<t_program_bundle> parse_and_mutate_program(
const std::string& filename,
parsing_params params,
bool return_nullptr_on_failure = false,
t_program_bundle* already_parsed = nullptr);
t_program_bundle* already_parsed = nullptr,
const sema_params* sparams = nullptr);

/**
* Runs the Thrift parser with the specified (command-line) arguments and
Expand Down
61 changes: 0 additions & 61 deletions thrift/compiler/sema/ast_mutator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,6 @@

#pragma once

#include <vector>

#include <thrift/compiler/ast/ast_visitor.h>
#include <thrift/compiler/ast/t_program_bundle.h>
#include <thrift/compiler/sema/sema_context.h>
Expand Down Expand Up @@ -141,65 +139,6 @@ struct type_ref_resolver {
bool unresolved_{false};
};

// A thin wrapper around a vector of mutators that
// knows how to apply those mutators in order.
struct ast_mutators {
private:
struct mutation_result {
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) {}

ast_mutator& add_stage() {
stages_.push_back({});
return stages_.back();
}

mutation_result operator()(sema_context& ctx, t_program_bundle& bundle) {
for (auto& stage : stages_) {
stage.mutate(ctx, bundle);
}
// We have no more mutators, so all type references **must** resolve.
mutation_result ret;
ret.unresolvable_typeref = !resolve_all_types(ctx, bundle);
return ret;
}

private:
// Tries to resolve any unresolved type references, returning true if
// successful.
//
// Any errors are reported via diags.
bool resolve_all_types(sema_context& diags, t_program_bundle& bundle) {
bool success = true;
if (!use_legacy_type_ref_resolution_) {
success = type_ref_resolver{}(diags, bundle);
}
for (auto& td : bundle.root_program()->scope()->placeholder_typedefs()) {
if (!td.type()) {
if (use_legacy_type_ref_resolution_) {
if (td.resolve()) {
continue;
}
success = false;
}

diags.error(td, "Type `{}` not defined.", td.name());
assert(!td.resolve());
assert(!success);
success = false;
}
}
return success;
}
};

} // namespace compiler
} // namespace thrift
} // namespace apache
85 changes: 60 additions & 25 deletions thrift/compiler/sema/standard_mutator.cc → thrift/compiler/sema/sema.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
* limitations under the License.
*/

#include <thrift/compiler/sema/standard_mutator.h>
#include <thrift/compiler/sema/sema.h>

#include <algorithm>
#include <functional>
Expand All @@ -25,11 +25,11 @@
#include <thrift/compiler/lib/cpp2/util.h>
#include <thrift/compiler/lib/schematizer.h>
#include <thrift/compiler/lib/uri.h>
#include <thrift/compiler/sema/ast_mutator.h>
#include <thrift/compiler/sema/sema_context.h>
#include <thrift/compiler/sema/standard_validator.h>

namespace apache {
namespace thrift {
namespace compiler {

namespace apache::thrift::compiler {
namespace {

void match_type_with_const_value(
Expand Down Expand Up @@ -185,32 +185,32 @@ void match_type_with_const_value(

void maybe_match_type_with_const_value(
sema_context& ctx,
mutator_context& mCtx,
mutator_context& mctx,
const t_type* type,
t_const_value* value) {
if (type == nullptr || value == nullptr) {
return;
}

match_type_with_const_value(ctx, mCtx.program(), type, value);
match_type_with_const_value(ctx, mctx.program(), type, value);
}

void match_const_type_with_value(
sema_context& ctx, mutator_context& mCtx, t_const& const_node) {
sema_context& ctx, mutator_context& mctx, t_const& const_node) {
maybe_match_type_with_const_value(
ctx, mCtx, const_node.type(), const_node.value());
ctx, mctx, const_node.type(), const_node.value());
}

void match_field_type_with_default_value(
sema_context& ctx, mutator_context& mCtx, t_field& field_node) {
sema_context& ctx, mutator_context& mctx, t_field& field_node) {
maybe_match_type_with_const_value(
ctx, mCtx, field_node.get_type(), field_node.get_default_value());
ctx, mctx, field_node.get_type(), field_node.get_default_value());
}

static void match_annotation_types_with_const_values(
sema_context& ctx, mutator_context& mCtx, t_named& node) {
sema_context& ctx, mutator_context& mctx, t_named& node) {
for (t_const& tconst : node.structured_annotations()) {
maybe_match_type_with_const_value(ctx, mCtx, tconst.type(), tconst.value());
maybe_match_type_with_const_value(ctx, mctx, tconst.type(), tconst.value());
}
}

Expand Down Expand Up @@ -347,7 +347,7 @@ void normalize_return_type(
}

template <typename Node>
void lower_type_annotations(sema_context&, mutator_context& mCtx, Node& node) {
void lower_type_annotations(sema_context&, mutator_context& mctx, Node& node) {
std::map<std::string, std::string> unstructured;

if (const t_const* annot =
Expand Down Expand Up @@ -378,7 +378,7 @@ void lower_type_annotations(sema_context&, mutator_context& mCtx, Node& node) {
}
} else if (node_type->is_primitive_type()) {
// Copy type as we don't handle unnamed typedefs to base types :(
auto& program = mCtx.program();
auto& program = mctx.program();
auto unnamed = std::make_unique<t_primitive_type>(
*static_cast<const t_primitive_type*>(node_type));
for (auto& pair : unstructured) {
Expand All @@ -388,7 +388,7 @@ void lower_type_annotations(sema_context&, mutator_context& mCtx, Node& node) {
program.add_unnamed_type(std::move(unnamed));
} else {
// Wrap in an unnamed typedef :(
auto& program = mCtx.program();
auto& program = mctx.program();
auto unnamed = t_typedef::make_unnamed(
const_cast<t_program*>(node_type->get_program()),
node_type->get_name(),
Expand Down Expand Up @@ -449,36 +449,71 @@ void deduplicate_thrift_includes(
includes.erase(it, includes.end());
}

} // namespace

ast_mutators standard_mutators(bool use_legacy_type_ref_resolution) {
ast_mutators mutators(use_legacy_type_ref_resolution);
std::vector<ast_mutator> standard_mutators() {
std::vector<ast_mutator> mutators;

auto& initial = mutators.add_stage();
ast_mutator 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);
mutators.push_back(std::move(initial));

auto& main = mutators.add_stage();
ast_mutator 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);
main.add_program_visitor(&deduplicate_thrift_includes);
mutators.push_back(std::move(main));

return mutators;
}

} // namespace

ast_mutator schema_mutator() {
ast_mutator mutator;
mutator.add_program_visitor(&inject_schema_const);
return mutator;
}

} // namespace compiler
} // namespace thrift
} // namespace apache
bool sema::resolve_all_types(sema_context& diags, t_program_bundle& bundle) {
bool success = true;
if (!use_legacy_type_ref_resolution_) {
success = type_ref_resolver()(diags, bundle);
}
for (auto& td : bundle.root_program()->scope()->placeholder_typedefs()) {
if (td.type()) {
continue;
}

if (use_legacy_type_ref_resolution_ && !td.resolve()) {
success = false;
}

diags.error(td, "Type `{}` not defined.", td.name());
assert(!td.resolve());
assert(!success);
success = false;
}
return success;
}

sema::result sema::run(sema_context& ctx, t_program_bundle& bundle) {
for (auto& mutator : standard_mutators()) {
mutator.mutate(ctx, bundle);
}
// We have no more mutators, so all type references **must** resolve.
result ret;
ret.unresolved_types = !resolve_all_types(ctx, bundle);
if (!ret.unresolved_types) {
standard_validator()(ctx, *bundle.root_program());
}
return ret;
}

} // namespace apache::thrift::compiler
Loading

0 comments on commit b26e26d

Please sign in to comment.