diff --git a/tools/api_boost/api_boost.py b/tools/api_boost/api_boost.py index 9ecd2650c2e1..2d0fc7d4ad6a 100755 --- a/tools/api_boost/api_boost.py +++ b/tools/api_boost/api_boost.py @@ -82,7 +82,8 @@ def RewriteIncludes(args): def ApiBoostTree(target_paths, generate_compilation_database=False, build_api_booster=False, - debug_log=False): + debug_log=False, + sequential=False): dep_build_targets = ['//%s/...' % PrefixDirectory(prefix) for prefix in target_paths] # Optional setup of state. We need the compilation database and api_booster @@ -139,12 +140,15 @@ def ApiBoostTree(target_paths, file_path = entry['file'] if any(file_path.startswith(prefix) for prefix in target_paths): file_paths.add(file_path) + # Ensure a determinstic ordering if we are going to process sequentially. + if sequential: + file_paths = sorted(file_paths) # The API boosting is file local, so this is trivially parallelizable, use # multiprocessing pool with default worker pool sized to cpu_count(), since # this is CPU bound. try: - with mp.Pool() as p: + with mp.Pool(processes=1 if sequential else None) as p: # We need multiple phases, to ensure that any dependency on files being modified # in one thread on consumed transitive headers on the other thread isn't an # issue. This also ensures that we complete all analysis error free before @@ -173,7 +177,11 @@ def ApiBoostTree(target_paths, parser.add_argument('--generate_compilation_database', action='store_true') parser.add_argument('--build_api_booster', action='store_true') parser.add_argument('--debug_log', action='store_true') + parser.add_argument('--sequential', action='store_true') parser.add_argument('paths', nargs='*', default=['source', 'test', 'include']) args = parser.parse_args() - ApiBoostTree(args.paths, args.generate_compilation_database, args.build_api_booster, - args.debug_log) + ApiBoostTree(args.paths, + generate_compilation_database=args.generate_compilation_database, + build_api_booster=args.build_api_booster, + debug_log=args.debug_log, + sequential=args.sequential) diff --git a/tools/api_boost/api_boost_test.py b/tools/api_boost/api_boost_test.py index ef6154db9096..abb2d6dcd6d4 100755 --- a/tools/api_boost/api_boost_test.py +++ b/tools/api_boost/api_boost_test.py @@ -5,6 +5,8 @@ # tools/clang_tools/api_booster, as well as the type whisperer and API type # database. +import argparse +from collections import namedtuple import logging import os import pathlib @@ -15,10 +17,15 @@ import api_boost +TestCase = namedtuple('TestCase', ['name', 'description']) + # List of test in the form [(file_name, explanation)] -TESTS = [ - ('elaborated_type.cc', 'ElaboratedTypeLoc type upgrades'), -] +TESTS = list( + map(lambda x: TestCase(*x), [ + ('elaborated_type', 'ElaboratedTypeLoc type upgrades'), + ('using_decl', 'UsingDecl upgrades for named types'), + ('decl_ref_expr', 'DeclRefExpr upgrades for named constants'), + ])) TESTDATA_PATH = 'tools/api_boost/testdata' @@ -31,29 +38,45 @@ def Diff(some_path, other_path): if __name__ == '__main__': + parser = argparse.ArgumentParser(description='Golden C++ source tests for api_boost.py') + parser.add_argument('tests', nargs='*') + args = parser.parse_args() + # Accumulated error messages. logging.basicConfig(format='%(message)s') messages = [] + def ShouldRunTest(test_name): + return len(args.tests) == 0 or test_name in args.tests + # Run API booster against test artifacts in a directory relative to workspace. # We use a temporary copy as the API booster does in-place rewriting. with tempfile.TemporaryDirectory(dir=pathlib.Path.cwd()) as path: # Setup temporary tree. shutil.copy(os.path.join(TESTDATA_PATH, 'BUILD'), path) - for filename, _ in TESTS: - shutil.copy(os.path.join(TESTDATA_PATH, filename), path) + for test in TESTS: + if ShouldRunTest(test.name): + shutil.copy(os.path.join(TESTDATA_PATH, test.name + '.cc'), path) + else: + # Place an empty file to make Bazel happy. + pathlib.Path(path, test.name + '.cc').write_text('') # Run API booster. - api_boost.ApiBoostTree([str(pathlib.Path(path).relative_to(pathlib.Path.cwd()))], + relpath_to_testdata = str(pathlib.Path(path).relative_to(pathlib.Path.cwd())) + api_boost.ApiBoostTree([relpath_to_testdata], generate_compilation_database=True, build_api_booster=True, - debug_log=True) + debug_log=True, + sequential=True) # Validate output against golden files. - for filename, description in TESTS: - delta = Diff(os.path.join(TESTDATA_PATH, filename + '.gold'), os.path.join(path, filename)) - if delta is not None: - messages.append('Non-empty diff for %s (%s):\n%s\n' % (filename, description, delta)) + for test in TESTS: + if ShouldRunTest(test.name): + delta = Diff(os.path.join(TESTDATA_PATH, test.name + '.cc.gold'), + os.path.join(path, test.name + '.cc')) + if delta is not None: + messages.append('Non-empty diff for %s (%s):\n%s\n' % + (test.name, test.description, delta)) if len(messages) > 0: logging.error('FAILED:\n{}'.format('\n'.join(messages))) diff --git a/tools/api_boost/testdata/BUILD b/tools/api_boost/testdata/BUILD index aba697c0b06e..78a4479fc9e7 100644 --- a/tools/api_boost/testdata/BUILD +++ b/tools/api_boost/testdata/BUILD @@ -8,8 +8,20 @@ load( envoy_package() +envoy_cc_library( + name = "decl_ref_expr", + srcs = ["decl_ref_expr.cc"], + deps = ["@envoy_api//envoy/config/overload/v2alpha:pkg_cc_proto"], +) + envoy_cc_library( name = "elaborated_type", srcs = ["elaborated_type.cc"], deps = ["@envoy_api//envoy/config/overload/v2alpha:pkg_cc_proto"], ) + +envoy_cc_library( + name = "using_decl", + srcs = ["using_decl.cc"], + deps = ["@envoy_api//envoy/config/overload/v2alpha:pkg_cc_proto"], +) diff --git a/tools/api_boost/testdata/decl_ref_expr.cc b/tools/api_boost/testdata/decl_ref_expr.cc new file mode 100644 index 000000000000..1d5adc092066 --- /dev/null +++ b/tools/api_boost/testdata/decl_ref_expr.cc @@ -0,0 +1,21 @@ +#include "envoy/config/overload/v2alpha/overload.pb.h" + +using envoy::config::overload::v2alpha::Trigger; + +class ThresholdTriggerImpl { +public: + ThresholdTriggerImpl(const envoy::config::overload::v2alpha::Trigger& config) { + switch (config.trigger_oneof_case()) { + case envoy::config::overload::v2alpha::Trigger::kThreshold: + break; + default: + break; + } + switch (config.trigger_oneof_case()) { + case Trigger::kThreshold: + break; + default: + break; + } + } +}; diff --git a/tools/api_boost/testdata/decl_ref_expr.cc.gold b/tools/api_boost/testdata/decl_ref_expr.cc.gold new file mode 100644 index 000000000000..1b774c4fee41 --- /dev/null +++ b/tools/api_boost/testdata/decl_ref_expr.cc.gold @@ -0,0 +1,21 @@ +#include "envoy/config/overload/v3alpha/overload.pb.h" + +using envoy::config::overload::v3alpha::Trigger; + +class ThresholdTriggerImpl { +public: + ThresholdTriggerImpl(const envoy::config::overload::v3alpha::Trigger& config) { + switch (config.trigger_oneof_case()) { + case envoy::config::overload::v3alpha::Trigger::kThreshold: + break; + default: + break; + } + switch (config.trigger_oneof_case()) { + case Trigger::kThreshold: + break; + default: + break; + } + } +}; diff --git a/tools/api_boost/testdata/using_decl.cc b/tools/api_boost/testdata/using_decl.cc new file mode 100644 index 000000000000..97335b2bc2ed --- /dev/null +++ b/tools/api_boost/testdata/using_decl.cc @@ -0,0 +1,10 @@ +#include "envoy/config/overload/v2alpha/overload.pb.h" + +using envoy::config::overload::v2alpha::ThresholdTrigger; +using SomePtrAlias = std::unique_ptr; + +class ThresholdTriggerImpl { +public: + ThresholdTriggerImpl(const ThresholdTrigger& /*config*/) {} + ThresholdTriggerImpl(SomePtrAlias /*config*/) {} +}; diff --git a/tools/api_boost/testdata/using_decl.cc.gold b/tools/api_boost/testdata/using_decl.cc.gold new file mode 100644 index 000000000000..5237e57f1300 --- /dev/null +++ b/tools/api_boost/testdata/using_decl.cc.gold @@ -0,0 +1,10 @@ +#include "envoy/config/overload/v3alpha/overload.pb.h" + +using envoy::config::overload::v3alpha::ThresholdTrigger; +using SomePtrAlias = std::unique_ptr; + +class ThresholdTriggerImpl { +public: + ThresholdTriggerImpl(const ThresholdTrigger& /*config*/) {} + ThresholdTriggerImpl(SomePtrAlias /*config*/) {} +}; diff --git a/tools/clang_tools/api_booster/main.cc b/tools/clang_tools/api_booster/main.cc index 30df4978302f..cfbf63c35114 100644 --- a/tools/clang_tools/api_booster/main.cc +++ b/tools/clang_tools/api_booster/main.cc @@ -49,77 +49,30 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, ApiBooster(std::map& replacements) : replacements_(replacements) {} - // AST match callback. + // AST match callback dispatcher. void run(const clang::ast_matchers::MatchFinder::MatchResult& match_result) override { clang::SourceManager& source_manager = match_result.Context->getSourceManager(); - // If we have a match on type, we should track the corresponding .pb.h and - // attempt to upgrade. - if (const clang::TypeLoc* type_loc = match_result.Nodes.getNodeAs("type")) { - const std::string type_name = - type_loc->getType().getCanonicalType().getUnqualifiedType().getAsString(); - const auto result = getLatestTypeInformationFromCType(type_name); - if (result) { - source_api_proto_paths_.insert(adjustProtoSuffix(result->proto_path_, ".pb.h")); - DEBUG_LOG(absl::StrCat("Matched type ", type_name, " ", type_loc->getTypeLocClass(), " ", - type_loc->getType()->getTypeClassName())); - addTypeLocReplacements(*type_loc, result->type_name_, source_manager); - } + if (const auto* type_loc = match_result.Nodes.getNodeAs("type")) { + onTypeLocMatch(*type_loc, source_manager); return; } - - // If we have a match on a call expression, check to see if it's something - // like loadFromYamlAndValidate; if so, we might need to look at the - // argument type to figure out any corresponding .pb.validate.h we require. - if (const clang::CallExpr* call_expr = - match_result.Nodes.getNodeAs("call_expr")) { - auto* direct_callee = call_expr->getDirectCallee(); - if (direct_callee != nullptr) { - const std::unordered_map ValidateNameToArg = { - {"loadFromYamlAndValidate", 1}, - {"loadFromFileAndValidate", 1}, - {"downcastAndValidate", 0}, - {"validate", 0}, - }; - const std::string& callee_name = direct_callee->getNameInfo().getName().getAsString(); - const auto arg = ValidateNameToArg.find(callee_name); - // Sometimes we hit false positives because we aren't qualifying above. - // TODO(htuch): fix this before merging. - if (arg != ValidateNameToArg.end() && arg->second < call_expr->getNumArgs()) { - const std::string type_name = call_expr->getArg(arg->second) - ->getType() - .getCanonicalType() - .getUnqualifiedType() - .getAsString(); - const auto result = getLatestTypeInformationFromCType(type_name); - if (result) { - source_api_proto_paths_.insert( - adjustProtoSuffix(result->proto_path_, ".pb.validate.h")); - } - } - } + if (const auto* using_decl = match_result.Nodes.getNodeAs("using_decl")) { + onUsingDeclMatch(*using_decl, source_manager); return; } - - // The last place we need to look for .pb.validate.h reference is - // instantiation of FactoryBase. - if (const clang::ClassTemplateSpecializationDecl* tmpl = + if (const auto* decl_ref_expr = + match_result.Nodes.getNodeAs("decl_ref_expr")) { + onDeclRefExprMatch(*decl_ref_expr, source_manager); + return; + } + if (const auto* call_expr = match_result.Nodes.getNodeAs("call_expr")) { + onCallExprMatch(*call_expr, source_manager); + return; + } + if (const auto* tmpl = match_result.Nodes.getNodeAs("tmpl")) { - const std::string tmpl_type_name = tmpl->getSpecializedTemplate() - ->getInjectedClassNameSpecialization() - .getCanonicalType() - .getAsString(); - if (tmpl_type_name == "FactoryBase") { - const std::string type_name = tmpl->getTemplateArgs() - .get(0) - .getAsType() - .getCanonicalType() - .getUnqualifiedType() - .getAsString(); - const auto result = getLatestTypeInformationFromCType(type_name); - if (result) { - source_api_proto_paths_.insert(adjustProtoSuffix(result->proto_path_, ".pb.validate.h")); - } - } + onClassTemplateSpecializationDeclMatch(*tmpl, source_manager); + return; } } @@ -139,31 +92,143 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, } private: - // Attempt to add type replacements as applicable for Envoy API types. - void addTypeLocReplacements(const clang::TypeLoc& type_loc, - const std::string& latest_proto_type_name, - const clang::SourceManager& source_manager) { - // We only support upgrading ElaboratedTypes so far. TODO(htuch): extend - // this to other AST type matches. + // Match callback for TypeLoc. These are explicit mentions of the type in the + // source. If we have a match on type, we should track the corresponding .pb.h + // and attempt to upgrade. + void onTypeLocMatch(const clang::TypeLoc& type_loc, const clang::SourceManager& source_manager) { + const std::string type_name = + type_loc.getType().getCanonicalType().getUnqualifiedType().getAsString(); + // Remove qualifiers, e.g. const. const clang::UnqualTypeLoc unqual_type_loc = type_loc.getUnqualifiedLoc(); - if (unqual_type_loc.getTypeLocClass() == clang::TypeLoc::Elaborated) { - clang::LangOptions lopt; - const clang::SourceLocation start = unqual_type_loc.getSourceRange().getBegin(); - const clang::SourceLocation end = clang::Lexer::getLocForEndOfToken( - unqual_type_loc.getSourceRange().getEnd(), 0, source_manager, lopt); - const size_t length = source_manager.getFileOffset(end) - source_manager.getFileOffset(start); - clang::tooling::Replacement type_replacement( - source_manager, start, length, ProtoCxxUtils::protoToCxxType(latest_proto_type_name)); - llvm::Error error = replacements_[type_replacement.getFilePath()].add(type_replacement); - if (error) { - std::cerr << " Replacement insertion error: " << llvm::toString(std::move(error)) - << std::endl; - } else { - std::cerr << " Replacement added: " << type_replacement.toString() << std::endl; + + // Today we are only smart enought to rewrite ElaborateTypeLoc, which are + // full namespace prefixed types. We probably will need to support more, in + // particular if we want message-level type renaming. TODO(htuch): add more + // supported AST TypeLoc classes as needed. + const bool rewrite = unqual_type_loc.getTypeLocClass() == clang::TypeLoc::Elaborated; + tryBoostType(unqual_type_loc.getType().getCanonicalType().getAsString(), + rewrite ? absl::make_optional(unqual_type_loc.getSourceRange()) + : absl::nullopt, + source_manager, type_loc.getType()->getTypeClassName()); + } + + // Match callback for clang::UsingDecl. These are 'using' aliases for API type + // names. + void onUsingDeclMatch(const clang::UsingDecl& using_decl, + const clang::SourceManager& source_manager) { + // Not all using declaration are types, but we try the rewrite in case there + // is such an API type database match. + const clang::SourceRange source_range = clang::SourceRange( + using_decl.getQualifierLoc().getBeginLoc(), using_decl.getNameInfo().getEndLoc()); + const std::string type_name = clang::Lexer::getSourceText( + clang::CharSourceRange::getTokenRange(source_range), source_manager, lexer_lopt_, 0); + tryBoostType(type_name, source_range, source_manager, "UsingDecl"); + } + + // Match callback for clang::DeclRefExpr. These occur when enums constants, + // e.g. foo::bar::kBaz, appear in the source. + void onDeclRefExprMatch(const clang::DeclRefExpr& decl_ref_expr, + const clang::SourceManager& source_manager) { + // We don't need to consider non-namespace qualified DeclRefExprfor now (no + // renaming support yet). + if (!decl_ref_expr.hasQualifier()) { + return; + } + // Remove trailing : from namespace qualifier. + const clang::SourceRange source_range = + clang::SourceRange(decl_ref_expr.getQualifierLoc().getBeginLoc(), + decl_ref_expr.getQualifierLoc().getEndLoc().getLocWithOffset(-1)); + const std::string type_name = clang::Lexer::getSourceText( + clang::CharSourceRange::getTokenRange(source_range), source_manager, lexer_lopt_, 0); + tryBoostType(type_name, source_range, source_manager, "DeclRefExpr"); + } + + // Match callback clang::CallExpr. We don't need to rewrite, but if it's something like + // loadFromYamlAndValidate, we might need to look at the argument type to + // figure out any corresponding .pb.validate.h we require. + void onCallExprMatch(const clang::CallExpr& call_expr, + const clang::SourceManager& source_manager) { + auto* direct_callee = call_expr.getDirectCallee(); + if (direct_callee != nullptr) { + const std::unordered_map ValidateNameToArg = { + {"loadFromYamlAndValidate", 1}, + {"loadFromFileAndValidate", 1}, + {"downcastAndValidate", 0}, + {"validate", 0}, + }; + const std::string& callee_name = direct_callee->getNameInfo().getName().getAsString(); + const auto arg = ValidateNameToArg.find(callee_name); + // Sometimes we hit false positives because we aren't qualifying above. + // TODO(htuch): fix this. + if (arg != ValidateNameToArg.end() && arg->second < call_expr.getNumArgs()) { + const std::string type_name = call_expr.getArg(arg->second) + ->getType() + .getCanonicalType() + .getUnqualifiedType() + .getAsString(); + tryBoostType(type_name, {}, source_manager, "validation invocation", true); } } } + // Match callback for clang::ClassTemplateSpecializationDecl. An additional + // place we need to look for .pb.validate.h reference is instantiation of + // FactoryBase. + void onClassTemplateSpecializationDeclMatch(const clang::ClassTemplateSpecializationDecl& tmpl, + const clang::SourceManager& source_manager) { + const std::string tmpl_type_name = tmpl.getSpecializedTemplate() + ->getInjectedClassNameSpecialization() + .getCanonicalType() + .getAsString(); + if (tmpl_type_name == "FactoryBase") { + const std::string type_name = tmpl.getTemplateArgs() + .get(0) + .getAsType() + .getCanonicalType() + .getUnqualifiedType() + .getAsString(); + tryBoostType(type_name, {}, source_manager, "FactoryBase template", true); + } + } + + // Attempt to boost a given type and rewrite the given source range. + void tryBoostType(const std::string& type_name, absl::optional source_range, + const clang::SourceManager& source_manager, absl::string_view debug_description, + bool validation_required = false) { + const auto latest_type_info = getLatestTypeInformationFromCType(type_name); + // If this isn't a known API type, our work here is done. + if (!latest_type_info) { + return; + } + DEBUG_LOG(absl::StrCat("Matched type '", type_name, "' (", debug_description, ")")); + // Track corresponding imports. + source_api_proto_paths_.insert(adjustProtoSuffix(latest_type_info->proto_path_, ".pb.h")); + if (validation_required) { + source_api_proto_paths_.insert( + adjustProtoSuffix(latest_type_info->proto_path_, ".pb.validate.h")); + } + // Not all AST matchers know how to do replacements (yet?). + if (!source_range) { + return; + } + // Add corresponding replacement. + const clang::SourceLocation start = source_range->getBegin(); + const clang::SourceLocation end = + clang::Lexer::getLocForEndOfToken(source_range->getEnd(), 0, source_manager, lexer_lopt_); + const size_t length = source_manager.getFileOffset(end) - source_manager.getFileOffset(start); + clang::tooling::Replacement type_replacement( + source_manager, start, length, ProtoCxxUtils::protoToCxxType(latest_type_info->type_name_)); + llvm::Error error = replacements_[type_replacement.getFilePath()].add(type_replacement); + if (error) { + std::cerr << " Replacement insertion error: " << llvm::toString(std::move(error)) + << std::endl; + } else { + std::cerr << " Replacement added: " << type_replacement.toString() << std::endl; + } + } + + void addNamedspaceQualifiedTypeReplacement() {} + // Remove .proto from a path, apply specified suffix instead. std::string adjustProtoSuffix(absl::string_view proto_path, absl::string_view suffix) { return absl::StrCat(proto_path.substr(0, proto_path.size() - 6), suffix); @@ -205,7 +270,9 @@ class ApiBooster : public clang::ast_matchers::MatchFinder::MatchCallback, std::set source_api_proto_paths_; // Map from source file to replacements. std::map& replacements_; -}; + // Language options for interacting with Lexer. Currently empty. + clang::LangOptions lexer_lopt_; +}; // namespace ApiBooster } // namespace ApiBooster @@ -226,6 +293,18 @@ int main(int argc, const char** argv) { clang::ast_matchers::typeLoc(clang::ast_matchers::isExpansionInMainFile()).bind("type"); finder.addMatcher(type_matcher, &api_booster); + // Match on all "using" declarations. + auto using_decl_matcher = + clang::ast_matchers::usingDecl(clang::ast_matchers::isExpansionInMainFile()) + .bind("using_decl"); + finder.addMatcher(using_decl_matcher, &api_booster); + + // Match on references to enum constants. + auto decl_ref_expr_matcher = + clang::ast_matchers::declRefExpr(clang::ast_matchers::isExpansionInMainFile()) + .bind("decl_ref_expr"); + finder.addMatcher(decl_ref_expr_matcher, &api_booster); + // Match on all call expressions. We are interested in particular in calls // where validation on protos is performed. auto call_matcher = clang::ast_matchers::callExpr().bind("call_expr");