diff --git a/CMakeLists.txt b/CMakeLists.txt index fafb1a55..887f1a41 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -69,6 +69,21 @@ add_custom_target( DEPENDS ${GENERATED_HEADER} ) +set(CONFIG_SOURCE_FILE "${CMAKE_CURRENT_SOURCE_DIR}/config/clang-tidy-config.h") +set(CONFIG_GENERATED_FILE "${CMAKE_CURRENT_BINARY_DIR}/generated/clang-tidy-config.h") + +add_custom_command( + OUTPUT ${CONFIG_GENERATED_FILE} + COMMAND ${CMAKE_COMMAND} -E copy_if_different ${CONFIG_SOURCE_FILE} ${CONFIG_GENERATED_FILE} + DEPENDS ${CONFIG_SOURCE_FILE} + COMMENT "Generating C++ header from ${CONFIG_SOURCE_FILE}" +) + +add_custom_target( + generate_config + DEPENDS ${CONFIG_GENERATED_FILE} +) + file(GLOB_RECURSE CLICE_SOURCES CONFIGURE_DEPENDS "${PROJECT_SOURCE_DIR}/src/AST/*.cpp" "${PROJECT_SOURCE_DIR}/src/Async/*.cpp" @@ -80,7 +95,7 @@ file(GLOB_RECURSE CLICE_SOURCES CONFIGURE_DEPENDS "${PROJECT_SOURCE_DIR}/src/Support/*.cpp" ) add_library(clice-core STATIC "${CLICE_SOURCES}") -add_dependencies(clice-core generate_flatbuffers_schema) +add_dependencies(clice-core generate_flatbuffers_schema generate_config) target_include_directories(clice-core PUBLIC "${PROJECT_SOURCE_DIR}/include" diff --git a/config/clang-tidy-config.h b/config/clang-tidy-config.h new file mode 100644 index 00000000..c3136fc5 --- /dev/null +++ b/config/clang-tidy-config.h @@ -0,0 +1,11 @@ +/* This generated file is for internal use. Do not include it from headers. */ + +#ifdef CLANG_TIDY_CONFIG_H +#error clang-tidy-config.h can only be included once +#else +#define CLANG_TIDY_CONFIG_H + +// Clice currently doesn't support this configuration, and we use the same default value as clangd. +#define CLANG_TIDY_ENABLE_STATIC_ANALYZER 0 + +#endif diff --git a/include/AST/Utility.h b/include/AST/Utility.h index e7d29c0b..9ac0f68d 100644 --- a/include/AST/Utility.h +++ b/include/AST/Utility.h @@ -15,6 +15,12 @@ bool is_templated(const clang::Decl* decl); /// Check whether the decl is anonymous. bool is_anonymous(const clang::NamedDecl* decl); +/// Checks whether the location is inside the main file. +bool is_inside_main_file(clang::SourceLocation loc, const clang::SourceManager& sm); + +/// Checks whether the decl is an implicit template instantiation. +bool is_implicit_template_instantiation(const clang::NamedDecl* decl); + /// Return the decl where it is instantiated from. If could be a template decl /// or a member of a class template. If the decl is a full specialization, return /// itself. diff --git a/include/Compiler/Compilation.h b/include/Compiler/Compilation.h index ed95d99f..e25406cc 100644 --- a/include/Compiler/Compilation.h +++ b/include/Compiler/Compilation.h @@ -15,6 +15,9 @@ struct CompilationParams { /// The kind of this compilation. CompilationUnit::Kind kind; + /// Whether to run clang-tidy. + bool clang_tidy = false; + /// Output file path. llvm::SmallString<128> output_file; diff --git a/include/Compiler/Diagnostic.h b/include/Compiler/Diagnostic.h index 2bea73dc..48d38121 100644 --- a/include/Compiler/Diagnostic.h +++ b/include/Compiler/Diagnostic.h @@ -2,7 +2,11 @@ #include #include + #include "AST/SourceCode.h" +#include "Compiler/Tidy.h" + +#include "clang/Basic/Diagnostic.h" namespace clang { class DiagnosticConsumer; @@ -52,6 +56,11 @@ struct DiagnosticID { bool is_unused() const; }; +class DiagnosticCollector : public clang::DiagnosticConsumer { +public: + tidy::ClangTidyChecker* checker = nullptr; +}; + struct Diagnostic { /// The diagnostic id. DiagnosticID id; @@ -66,7 +75,7 @@ struct Diagnostic { /// The error message of this diagnostic. std::string message; - static clang::DiagnosticConsumer* create(std::shared_ptr> diagnostics); + static DiagnosticCollector* create(std::shared_ptr> diagnostics); }; } // namespace clice diff --git a/include/Compiler/Tidy.h b/include/Compiler/Tidy.h index 849cb612..64acc7c4 100644 --- a/include/Compiler/Tidy.h +++ b/include/Compiler/Tidy.h @@ -2,9 +2,23 @@ #include "llvm/ADT/StringRef.h" +#include + +namespace clang { +class CompilerInstance; +} + namespace clice::tidy { bool is_registered_tidy_check(llvm::StringRef check); std::optional is_fast_tidy_check(llvm::StringRef check); +struct TidyParams {}; + +class ClangTidyChecker; + +/// Configure to run clang-tidy on the given file. +std::unique_ptr configure(clang::CompilerInstance& instance, + const TidyParams& params); + } // namespace clice::tidy diff --git a/src/AST/Utility.cpp b/src/AST/Utility.cpp index 5ffaa1fb..532140b1 100644 --- a/src/AST/Utility.cpp +++ b/src/AST/Utility.cpp @@ -16,6 +16,13 @@ namespace clice::ast { +bool is_inside_main_file(clang::SourceLocation loc, const clang::SourceManager& sm) { + if(!loc.isValid()) + return false; + clang::FileID fid = sm.getFileID(sm.getExpansionLoc(loc)); + return fid == sm.getMainFileID() || fid == sm.getPreambleFileID(); +}; + bool is_definition(const clang::Decl* decl) { if(auto VD = llvm::dyn_cast(decl)) { return VD->isThisDeclarationADefinition(); @@ -58,6 +65,25 @@ bool is_anonymous(const clang::NamedDecl* decl) { return name.isIdentifier() && !name.getAsIdentifierInfo(); } +template +bool is_template_specialization_kind(const clang::NamedDecl* decl, + clang::TemplateSpecializationKind kind) { + if(const auto* td = dyn_cast(decl)) + return td->getTemplateSpecializationKind() == kind; + return false; +} + +inline bool is_template_specialization_kind(const clang::NamedDecl* decl, + clang::TemplateSpecializationKind kind) { + return is_template_specialization_kind(decl, kind) || + is_template_specialization_kind(decl, kind) || + is_template_specialization_kind(decl, kind); +} + +bool is_implicit_template_instantiation(const clang::NamedDecl* decl) { + return is_template_specialization_kind(decl, clang::TSK_ImplicitInstantiation); +} + const static clang::CXXRecordDecl* getDeclContextForTemplateInstationPattern(const clang::Decl* D) { if(const auto* CTSD = dyn_cast(D->getDeclContext())) { return CTSD->getTemplateInstantiationPattern(); diff --git a/src/Compiler/Compilation.cpp b/src/Compiler/Compilation.cpp index 5c17bbea..04ea3b35 100644 --- a/src/Compiler/Compilation.cpp +++ b/src/Compiler/Compilation.cpp @@ -1,7 +1,12 @@ + +#include "TidyImpl.h" + +#include "AST/Utility.h" #include "CompilationUnitImpl.h" #include "Compiler/Command.h" #include "Compiler/Compilation.h" #include "Compiler/Diagnostic.h" +#include "Compiler/Tidy.h" #include "clang/Lex/PreprocessorOptions.h" #include "clang/Frontend/TextDiagnosticPrinter.h" #include "clang/Frontend/MultiplexConsumer.h" @@ -21,16 +26,17 @@ public: src_mgr(instance.getSourceManager()), top_level_decls(top_level_decls), stop(stop) {} void collect_decl(clang::Decl* decl) { - auto location = decl->getLocation(); - if(location.isInvalid()) { + if(!(ast::is_inside_main_file(decl->getLocation(), src_mgr))) { return; } - location = src_mgr.getExpansionLoc(location); - auto fid = src_mgr.getFileID(location); - if(fid == src_mgr.getPreambleFileID() || fid == src_mgr.getMainFileID()) { - top_level_decls->push_back(decl); + if(const clang::NamedDecl* named_decl = dyn_cast(decl)) { + if(ast::is_implicit_template_instantiation(named_decl)) { + return; + } } + + top_level_decls->push_back(decl); } auto HandleTopLevelDecl(clang::DeclGroupRef group) -> bool final { @@ -158,10 +164,11 @@ CompilationResult run_clang(CompilationParams& params, auto diagnostics = params.diagnostics ? params.diagnostics : std::make_shared>(); + auto diagnostic_collector = Diagnostic::create(diagnostics); auto diagnostic_engine = clang::CompilerInstance::createDiagnostics(*params.vfs, new clang::DiagnosticOptions(), - Diagnostic::create(diagnostics)); + diagnostic_collector); auto invocation = create_invocation(params, diagnostic_engine); if(!invocation) { @@ -193,7 +200,7 @@ CompilationResult run_clang(CompilationParams& params, auto action = std::make_unique( std::make_unique(), /// We only collect top level declarations for parse main file. - params.kind == CompilationUnit::Content ? &top_level_decls : nullptr, + (params.clang_tidy || params.kind == CompilationUnit::Content) ? &top_level_decls : nullptr, params.stop); if(!action->BeginSourceFile(*instance, instance->getFrontendOpts().Inputs[0])) { @@ -201,7 +208,15 @@ CompilationResult run_clang(CompilationParams& params, } auto& pp = instance->getPreprocessor(); - /// FIXME: clang-tidy, include-fixer, etc? + /// FIXME: include-fixer, etc? + + /// Setup clang-tidy + std::unique_ptr checker; + if(params.clang_tidy) { + tidy::TidyParams tidy_params; + checker = tidy::configure(*instance, tidy_params); + diagnostic_collector->checker = checker.get(); + } /// `BeginSourceFile` may create new preprocessor, so all operations related to preprocessor /// should be done after `BeginSourceFile`. @@ -239,6 +254,19 @@ CompilationResult run_clang(CompilationParams& params, token_buffer = std::move(*token_collector).consume(); } + // Must be called before EndSourceFile because the ast context can be destroyed later. + if(checker) { + // AST traversals should exclude the preamble, to avoid performance cliffs. + // TODO: is it okay to affect the unit-level traversal scope here? + instance->getASTContext().setTraversalScope(top_level_decls); + checker->finder.matchAST(instance->getASTContext()); + } + + /// XXX: This is messy: clang-tidy checks flush some diagnostics at EOF. + /// However Action->EndSourceFile() would destroy the ASTContext! + /// So just inform the preprocessor of EOF, while keeping everything alive. + pp.EndSourceFile(); + /// FIXME: getDependencies currently return ArrayRef, which actually results in /// extra copy. It would be great to avoid this copy. @@ -247,10 +275,15 @@ CompilationResult run_clang(CompilationParams& params, resolver.emplace(instance->getSema()); } + if(checker) { + /// Avoid dangling pointer. + diagnostic_collector->checker = nullptr; + } + auto build_end = chrono::steady_clock::now().time_since_epoch(); auto impl = new CompilationUnit::Impl{ - .interested = pp.getSourceManager().getMainFileID(), + .interested = instance->getSourceManager().getMainFileID(), .src_mgr = instance->getSourceManager(), .action = std::move(action), .instance = std::move(instance), diff --git a/src/Compiler/CompilationUnit.cpp b/src/Compiler/CompilationUnit.cpp index 6a01e75a..25a315c1 100644 --- a/src/Compiler/CompilationUnit.cpp +++ b/src/Compiler/CompilationUnit.cpp @@ -6,6 +6,11 @@ namespace clice { CompilationUnit::~CompilationUnit() { if(impl && impl->action) { + auto instance = impl->instance.get(); + // We already notified the pp of end-of-file earlier, so detach it first. + // We must keep it alive until after EndSourceFile(), Sema relies on this. + std::shared_ptr pp = instance->getPreprocessorPtr(); + instance->setPreprocessor(nullptr); // Detach so we don't send EOF again impl->action->EndSourceFile(); } diff --git a/src/Compiler/Diagnostic.cpp b/src/Compiler/Diagnostic.cpp index 56abba50..9dd39f62 100644 --- a/src/Compiler/Diagnostic.cpp +++ b/src/Compiler/Diagnostic.cpp @@ -1,4 +1,7 @@ #include "Compiler/Diagnostic.h" +#include "Support/Format.h" +#include "TidyImpl.h" + #include "clang/AST/Type.h" #include "clang/AST/Decl.h" #include "clang/AST/DeclCXX.h" @@ -7,7 +10,6 @@ #include "clang/Basic/AllDiagnostics.h" #include "clang/Basic/SourceManager.h" #include "clang/Lex/Preprocessor.h" -#include "Support/Format.h" namespace clice { @@ -131,6 +133,10 @@ bool DiagnosticID::is_unused() const { return source == DiagnosticSource::Clang && unused_diags.contains(value); } +bool is_note(clang::DiagnosticsEngine::Level level) { + return level == clang::DiagnosticsEngine::Note || level == clang::DiagnosticsEngine::Remark; +} + static DiagnosticLevel diagnostic_level(clang::DiagnosticsEngine::Level level) { switch(level) { case clang::DiagnosticsEngine::Ignored: return DiagnosticLevel::Ignored; @@ -194,9 +200,9 @@ auto diagnostic_range(const clang::Diagnostic& diagnostic, const clang::LangOpti }; } -class DiagnosticCollector : public clang::DiagnosticConsumer { +class DiagnosticCollectorImpl : public DiagnosticCollector { public: - DiagnosticCollector(std::shared_ptr> diagnostics) : + DiagnosticCollectorImpl(std::shared_ptr> diagnostics) : diagnostics(diagnostics) {} void BeginSourceFile(const clang::LangOptions& Opts, const clang::Preprocessor* PP) override { @@ -209,6 +215,12 @@ public: auto& diagnostic = diagnostics->emplace_back(); diagnostic.id.value = raw_diagnostic.getID(); + + if(!is_note(level)) { + if(checker) { + level = checker->adjust_level(level, raw_diagnostic); + } + } diagnostic.id.level = diagnostic_level(level); /// TODO: @@ -229,6 +241,10 @@ public: diagnostic.range = range; } + if(checker) { + checker->adjust_diag(diagnostic); + } + /// TODO: handle FixIts /// raw_diagnostic.getFixItHints(); } @@ -241,9 +257,8 @@ private: clang::SourceManager* src_mgr; }; -clang::DiagnosticConsumer* - Diagnostic::create(std::shared_ptr> diagnostics) { - return new DiagnosticCollector(diagnostics); +DiagnosticCollector* Diagnostic::create(std::shared_ptr> diagnostics) { + return new DiagnosticCollectorImpl(diagnostics); } } // namespace clice diff --git a/src/Compiler/Tidy.cpp b/src/Compiler/Tidy.cpp index b4282247..4b1b2a4f 100644 --- a/src/Compiler/Tidy.cpp +++ b/src/Compiler/Tidy.cpp @@ -10,13 +10,30 @@ /// https://github.com/llvm/llvm-project//blob/0865ecc5150b9a55ba1f9e30b6d463a66ac362a6/clang-tools-extra/clangd/ParsedAST.cpp#L547 /// https://github.com/llvm/llvm-project//blob/0865ecc5150b9a55ba1f9e30b6d463a66ac362a6/clang-tools-extra/clangd/TidyProvider.cpp +#include "TidyImpl.h" + +#include "AST/Utility.h" +#include "Compiler/Diagnostic.h" +#include "Compiler/Tidy.h" +#include "Support/Logging.h" + #include "clang-tidy/ClangTidyModuleRegistry.h" #include "clang-tidy/ClangTidyOptions.h" +#include "clang-tidy/ClangTidyCheck.h" +#include "clang-tidy/ClangTidyDiagnosticConsumer.h" + +#include "clang/Frontend/CompilerInstance.h" #include "llvm/ADT/StringSet.h" +#include "llvm/ADT/StringExtras.h" #include "llvm/Support/Allocator.h" +#include "llvm/Support/Process.h" +#include "llvm/Support/StringSaver.h" -#include "Compiler/Tidy.h" +// Force the linker to link in Clang-tidy modules. +// clangd doesn't support the static analyzer. +#define CLANG_TIDY_DISABLE_STATIC_ANALYZER_CHECKS +#include "clang-tidy/ClangTidyForceLinker.h" namespace clice::tidy { @@ -25,7 +42,7 @@ using namespace clang::tidy; bool is_registered_tidy_check(llvm::StringRef check) { assert(!check.empty()); assert(!check.contains('*') && !check.contains(',') && - "isRegisteredCheck doesn't support globs"); + "is_registered_tidy_check doesn't support globs"); assert(check.ltrim().front() != '-'); const static llvm::StringSet all_checks = [] { @@ -53,4 +70,338 @@ std::optional is_fast_tidy_check(llvm::StringRef check) { return std::nullopt; } +tidy::ClangTidyCheckFactories get_fast_checks(const tidy::ClangTidyCheckFactories& all) { + tidy::ClangTidyCheckFactories fast; + for(const auto& factory: all) { + if(is_fast_tidy_check(factory.getKey()).value_or(false)) { + fast.registerCheckFactory(factory.first(), factory.second); + } + } + return fast; +} + +tidy::ClangTidyOptions create_options() { + // getDefaults instantiates all check factories, which are registered at link + // time. So cache the results once. + const static auto default_opts = [] { + auto opts = tidy::ClangTidyOptions::getDefaults(); + opts.Checks->clear(); + return opts; + }(); + // These default checks are chosen for: + // - low false-positive rate + // - providing a lot of value + // - being reasonably efficient + const static std::string default_checks = llvm::join_items(",", + "readability-misleading-indentation", + "readability-deleted-default", + "bugprone-integer-division", + "bugprone-sizeof-expression", + "bugprone-suspicious-missing-comma", + "bugprone-unused-raii", + "bugprone-unused-return-value", + "misc-unused-using-decls", + "misc-unused-alias-decls", + "misc-definitions-in-headers"); + const static std::string bad_checks = + llvm::join_items(",", + // We want this list to start with a separator to + // simplify appending in the lambda. So including an + // empty string here will force that. + "", + // include-cleaner is directly integrated in IncludeCleaner.cpp + "-misc-include-cleaner", + + // ----- False Positives ----- + + // Check relies on seeing ifndef/define/endif directives, + // clangd doesn't replay those when using a preamble. + "-llvm-header-guard", + "-modernize-macro-to-enum", + + // ----- Crashing Checks ----- + + // Check can choke on invalid (intermediate) c++ + // code, which is often the case when clangd + // tries to build an AST. + "-bugprone-use-after-move", + // Alias for bugprone-use-after-move. + "-hicpp-invalid-access-moved", + // Check uses dataflow analysis, which might hang/crash unexpectedly on + // incomplete code. + "-bugprone-unchecked-optional-access"); + + tidy::ClangTidyOptions opts = default_opts; + + // clang::clangd::provideEnvironment + if(std::optional user = llvm::sys::Process::GetEnv("USER")) { + opts.User = user; + } + // TODO: Providers.push_back(provideClangTidyFiles(TFS)); Filename + // TODO: if(EnableConfig) Providers.push_back(provideClangdConfig()); + // clang::clangd::provideDefaultChecks + if(!opts.Checks || opts.Checks->empty()) { + opts.Checks = default_checks; + } + // clang::clangd::disableUnusableChecks + if(opts.Checks && !opts.Checks->empty()) { + opts.Checks->append(bad_checks); + } + return opts; +} + +// Filter for clang diagnostics groups enabled by CTOptions.Checks. +// +// These are check names like clang-diagnostics-unused. +// Note that unlike -Wunused, clang-diagnostics-unused does not imply +// subcategories like clang-diagnostics-unused-function. +// +// This is used to determine which diagnostics can be enabled by ExtraArgs in +// the clang-tidy configuration. +class TidyDiagnosticGroups { + // Whether all diagnostic groups are enabled by default. + // True if we've seen clang-diagnostic-*. + bool default_enable = false; + // Set of diag::Group whose enablement != default_enable. + // If default_enable is false, this is foo where we've seen clang-diagnostic-foo. + llvm::DenseSet exceptions; + +public: + TidyDiagnosticGroups(llvm::StringRef checks) { + constexpr llvm::StringLiteral CDPrefix = "clang-diagnostic-"; + + llvm::StringRef check; + while(!checks.empty()) { + std::tie(check, checks) = checks.split(','); + check = check.trim(); + + if(check.empty()) { + continue; + } + + bool enable = !check.consume_front("-"); + bool glob = check.consume_back("*"); + if(glob) { + // Is this clang-diagnostic-*, or *, or so? + // (We ignore all other types of globs). + if(CDPrefix.starts_with(check)) { + default_enable = enable; + exceptions.clear(); + } + continue; + } + + // In "*,clang-diagnostic-foo", the latter is a no-op. + if(default_enable == enable) { + continue; + } + // The only non-glob entries we care about are clang-diagnostic-foo. + if(!check.consume_front(CDPrefix)) { + continue; + } + + if(auto group = clang::DiagnosticIDs::getGroupForWarningOption(check)) { + exceptions.insert(static_cast(*group)); + } + } + } + + bool operator() (clang::diag::Group group_id) const { + return exceptions.contains(static_cast(group_id)) ? !default_enable + : default_enable; + } +}; + +// Find -W and -Wno- options in extra_args and apply them to diags. +// +// This is used to handle extra_args in clang-tidy configuration. +// We don't use clang's standard handling of this as we want slightly different +// behavior (e.g. we want to exclude these from -Wno-error). +void apply_warning_options(llvm::ArrayRef extra_args, + llvm::function_ref enable_groups, + clang::DiagnosticsEngine& diags) { + for(llvm::StringRef group: extra_args) { + // Only handle args that are of the form -W[no-]. + // Other flags are possible but rare and deliberately out of scope. + llvm::SmallVector members; + if(!group.consume_front("-W") || group.empty()) { + continue; + } + bool enable = !group.consume_front("no-"); + if(diags.getDiagnosticIDs()->getDiagnosticsInGroup(clang::diag::Flavor::WarningOrError, + group, + members)) { + continue; + } + + // Upgrade (or downgrade) the severity of each diagnostic in the group. + // If -Werror is on, newly added warnings will be treated as errors. + // We don't want this, so keep track of them to fix afterwards. + bool needs_werror_exclusion = false; + for(clang::diag::kind id: members) { + if(enable) { + if(diags.getDiagnosticLevel(id, clang::SourceLocation()) < + clang::DiagnosticsEngine::Warning) { + auto group = diags.getDiagnosticIDs()->getGroupForDiag(id); + if(!group || !enable_groups(*group)) { + continue; + } + diags.setSeverity(id, clang::diag::Severity::Warning, clang::SourceLocation()); + if(diags.getWarningsAsErrors()) { + needs_werror_exclusion = true; + } + } + } else { + diags.setSeverity(id, clang::diag::Severity::Ignored, clang::SourceLocation()); + } + } + if(needs_werror_exclusion) { + // FIXME: there's no API to suppress -Werror for single diagnostics. + // In some cases with sub-groups, we may end up erroneously + // downgrading diagnostics that were -Werror in the compile command. + diags.setDiagnosticGroupWarningAsError(group, false); + } + } +} + +ClangTidyChecker::ClangTidyChecker(std::unique_ptr provider) : + context(std::move(provider)) {} + +clang::DiagnosticsEngine::Level + ClangTidyChecker::adjust_level(clang::DiagnosticsEngine::Level level, + const clang::Diagnostic& diag) { + if(!checks.empty()) { + std::string tidy_diag = context.getCheckName(diag.getID()); + bool is_clang_tidy_diag = !tidy_diag.empty(); + if(is_clang_tidy_diag) { + // Check for suppression comment. Skip the check for diagnostics not + // in the main file, because we don't want that function to query the + // source buffer for preamble files. For the same reason, we ask + // shouldSuppressDiagnostic to avoid I/O. + // We let suppression comments take precedence over warning-as-error + // to match clang-tidy's behaviour. + bool in_main_file = + diag.hasSourceManager() && + ast::is_inside_main_file(diag.getLocation(), diag.getSourceManager()); + llvm::SmallVector tidy_suppressed_errors; + if(in_main_file && context.shouldSuppressDiagnostic(level, + diag, + tidy_suppressed_errors, + /*AllowIO=*/false, + /*EnableNolintBlocks=*/true)) { + // FIXME: should we expose the suppression error (invalid use of + // NOLINT comments)? + return clang::DiagnosticsEngine::Ignored; + } + if(!context.getOptions().SystemHeaders.value_or(false) && diag.hasSourceManager() && + diag.getSourceManager().isInSystemMacro(diag.getLocation())) { + return clang::DiagnosticsEngine::Ignored; + } + + // Check for warning-as-error. + if(level == clang::DiagnosticsEngine::Warning && context.treatAsError(tidy_diag)) { + return clang::DiagnosticsEngine::Error; + } + } + } + return level; +} + +void ClangTidyChecker::adjust_diag(Diagnostic& diag) { + std::string tidy_diag = context.getCheckName(diag.id.value); + if(!tidy_diag.empty()) { + // TODO: using a global string saver. + static llvm::BumpPtrAllocator allocator; + static llvm::StringSaver saver(allocator); + diag.id.name = saver.save(tidy_diag); + diag.id.source = DiagnosticSource::ClangTidy; + // clang-tidy bakes the name into diagnostic messages. Strip it out. + // It would be much nicer to make clang-tidy not do this. + auto clean_message = [&](std::string& msg) { + llvm::StringRef rest(msg); + if(rest.consume_back("]") && rest.consume_back(diag.id.name) && rest.consume_back(" [")) + msg.resize(rest.size()); + }; + clean_message(diag.message); + // todo: where is clice notes and fixes? + // for(auto& note: diag.Notes) + // clean_message(note.Message); + // for(auto& fix: diag.Fixes) + // clean_message(fix.Message); + } +} + +std::unique_ptr configure(clang::CompilerInstance& instance, + const TidyParams& params) { + auto& input = instance.getFrontendOpts().Inputs[0]; + + if(!input.isFile()) { + return nullptr; + } + auto file_name = input.getFile(); + logging::info("Tidy configure file: {}", file_name); + + tidy::ClangTidyOptions opts = create_options(); + if(opts.Checks) { + logging::info("Tidy configure checks: {}", *opts.Checks); + } + + { + // If clang-tidy is configured to emit clang warnings, we should too. + // + // Such clang-tidy configuration consists of two parts: + // - ExtraArgs: ["-Wfoo"] causes clang to produce the warnings + // - Checks: "clang-diagnostic-foo" prevents clang-tidy filtering them out + // + // In clang-tidy, diagnostics are emitted if they pass both checks. + // When groups contain subgroups, -Wparent includes the child, but + // clang-diagnostic-parent does not. + // + // We *don't* want to change the compile command directly. This can have + // too many unexpected effects: breaking the command, interactions with + // -- and -Werror, etc. Besides, we've already parsed the command. + // Instead we parse the -W flags and handle them directly. + // + // Similarly, we don't want to use Checks to filter clang diagnostics after + // they are generated, as this spreads clang-tidy emulation everywhere. + // Instead, we just use these to filter which extra diagnostics we enable. + auto& diags = instance.getDiagnostics(); + TidyDiagnosticGroups groups(opts.Checks ? *opts.Checks : llvm::StringRef()); + if(opts.ExtraArgsBefore) { + apply_warning_options(*opts.ExtraArgsBefore, groups, diags); + } + if(opts.ExtraArgs) { + apply_warning_options(*opts.ExtraArgs, groups, diags); + } + } + + /// No need to run clang-tidy or IncludeFixerif we are not going to surface + /// diagnostics. + const static auto all_factories = [] { + tidy::ClangTidyCheckFactories factories; + for(const auto& e: tidy::ClangTidyModuleRegistry::entries()) { + e.instantiate()->addCheckFactories(factories); + } + return factories; + }(); + tidy::ClangTidyCheckFactories factories = get_fast_checks(all_factories); + std::unique_ptr checker = std::make_unique( + std::make_unique(tidy::ClangTidyGlobalOptions(), opts)); + + checker->context.setDiagnosticsEngine(&instance.getDiagnostics()); + checker->context.setASTContext(&instance.getASTContext()); + // TODO: is `file_name` always the file to check? + checker->context.setCurrentFile(file_name); + checker->context.setSelfContainedDiags(true); + checker->checks = factories.createChecksForLanguage(&checker->context); + logging::info("Tidy configure checks: {}", checker->checks.size()); + clang::Preprocessor* pp = &instance.getPreprocessor(); + for(const auto& check: checker->checks) { + check->registerPPCallbacks(instance.getSourceManager(), pp, pp); + check->registerMatchers(&checker->finder); + } + + return checker; +} + } // namespace clice::tidy diff --git a/src/Compiler/TidyImpl.h b/src/Compiler/TidyImpl.h new file mode 100644 index 00000000..3dffa8d2 --- /dev/null +++ b/src/Compiler/TidyImpl.h @@ -0,0 +1,33 @@ +#pragma once + +#include + +#include "Compiler/Diagnostic.h" +#include "Compiler/Tidy.h" + +#include "clang-tidy/ClangTidyModuleRegistry.h" +#include "clang-tidy/ClangTidyOptions.h" +#include "clang-tidy/ClangTidyCheck.h" + +namespace clice::tidy { + +using namespace clang::tidy; + +class ClangTidyChecker { + +public: + /// The context of the clang-tidy checker. + ClangTidyContext context; + /// The instances of checks that are enabled for the current Language. + std::vector> checks; + /// The match finder to run clang-tidy on ASTs. + clang::ast_matchers::MatchFinder finder; + + ClangTidyChecker(std::unique_ptr provider); + + clang::DiagnosticsEngine::Level adjust_level(clang::DiagnosticsEngine::Level level, + const clang::Diagnostic& diag); + void adjust_diag(Diagnostic& diag); +}; + +} // namespace clice::tidy diff --git a/src/Server/Document.cpp b/src/Server/Document.cpp index 2129f012..be19307a 100644 --- a/src/Server/Document.cpp +++ b/src/Server/Document.cpp @@ -320,6 +320,7 @@ async::Task<> Server::build_ast(std::string path, std::string content) { params.pch = {pch->path, pch->preamble.size()}; file->diagnostics->clear(); params.diagnostics = file->diagnostics; + params.clang_tidy = config.project.clang_tidy; /// Check result auto ast = co_await async::submit([&] { return compile(params); }); @@ -332,12 +333,6 @@ async::Task<> Server::build_ast(std::string path, std::string content) { co_return; } - /// Run Clang-Tidy - if(config.project.clang_tidy) { - logging::warn( - "clang-tidy is not fully supported yet. Tracked in https://github.com/clice-project/clice/issues/90."); - } - /// Send diagnostics auto diagnostics = co_await async::submit( [&, kind = this->kind] { return feature::diagnostics(kind, mapping, *ast); }); diff --git a/tests/data/clang_tidy/main.cpp b/tests/data/clang_tidy/main.cpp index ff682901..950316c3 100644 --- a/tests/data/clang_tidy/main.cpp +++ b/tests/data/clang_tidy/main.cpp @@ -1,6 +1,25 @@ #include +// Tests bugprone-integer-division int main() { - std::cout << "Hello World!" << std::endl; + float floatFunc(float); + int intFunc(int); + double d; + int i = 42; + + // Warn, floating-point values expected. + d = 32 * 8 / (2 + i); + d = 8 * floatFunc(1 + 7 / 2); + d = i / (1 << 4); + + // OK, no integer division. + d = 32 * 8.0 / (2 + i); + d = 8 * floatFunc(1 + 7.0 / 2); + d = (double)i / (1 << 4); + + // OK, there are signs of deliberateness. + d = 1 << (i / 2); + d = 9 + intFunc(6 * i / 32); + d = (int)(i / 32) - 8; return 0; } diff --git a/xmake.lua b/xmake.lua index b3addb4e..e1bd74a6 100644 --- a/xmake.lua +++ b/xmake.lua @@ -52,7 +52,7 @@ target("clice-core") add_files("src/**.cpp|Driver/*.cpp", "include/Index/schema.fbs") add_includedirs("include", {public = true}) - add_rules("flatbuffers.schema.gen") + add_rules("flatbuffers.schema.gen", "clice_clang_tidy_config") add_packages("flatbuffers") add_packages("libuv", "spdlog", "toml++", "croaring", {public = true}) @@ -177,6 +177,23 @@ target("integration_tests") return true end) +rule("clice_clang_tidy_config") + on_load(function (target) + import("core.project.depend") + + local autogendir = path.join(target:autogendir(), "rules/clice_clang_tidy_config") + os.mkdir(autogendir) + target:add("includedirs", autogendir, {public = true}) + + local src = path.join(os.projectdir(), "config/clang-tidy-config.h") + depend.on_changed(function() + os.vcp(src, path.join(autogendir, "clang-tidy-config.h")) + end, { + files = src, + changed = target:is_rebuilt() + }) + end) + rule("clice_build_config") on_load(function (target) target:add("cxflags", "-fno-rtti", {tools = {"clang", "clangxx", "gcc", "gxx"}})