diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt index 3aa1b7bddb99..1f839a571c46 100644 --- a/clang-tools-extra/clangd/CMakeLists.txt +++ b/clang-tools-extra/clangd/CMakeLists.txt @@ -66,6 +66,7 @@ add_clang_library(clangDaemon STATIC ASTSignals.cpp ClangdLSPServer.cpp ClangdServer.cpp + ClangTidyFeatureModule.cpp CodeComplete.cpp CodeCompletionStrings.cpp CollectMacros.cpp diff --git a/clang-tools-extra/clangd/ClangTidyFeatureModule.cpp b/clang-tools-extra/clangd/ClangTidyFeatureModule.cpp new file mode 100644 index 000000000000..413d89f3777c --- /dev/null +++ b/clang-tools-extra/clangd/ClangTidyFeatureModule.cpp @@ -0,0 +1,412 @@ +//===--- ClangTidyFeatureModule.cpp - clang-tidy integration -------------===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#include "ClangTidyFeatureModule.h" +#include "../clang-tidy/ClangTidyCheck.h" +#include "../clang-tidy/ClangTidyDiagnosticConsumer.h" +#include "../clang-tidy/ClangTidyModule.h" +#include "../clang-tidy/ClangTidyOptions.h" +#include "Config.h" +#include "Diagnostics.h" +#include "Feature.h" +#include "SourceCode.h" +#include "support/Logger.h" +#include "support/Trace.h" +#include "clang/ASTMatchers/ASTMatchFinder.h" +#include "clang/Basic/Diagnostic.h" +#include "clang/Basic/DiagnosticIDs.h" +#include "clang/Basic/FileManager.h" +#include "clang/Frontend/CompilerInstance.h" +#include "clang/Lex/Preprocessor.h" +#include "clang/Lex/PreprocessorOptions.h" +#include "clang/Tooling/Core/Diagnostic.h" +#include "llvm/ADT/DenseSet.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/SmallString.h" +#include "llvm/Support/MemoryBuffer.h" +#include "llvm/Support/Path.h" +#include +#include +#include +#include +#include +#include + +// Force the linker to link in Clang-tidy modules. +// clangd doesn't support the static analyzer. +#if CLANGD_TIDY_CHECKS +#define CLANG_TIDY_DISABLE_STATIC_ANALYZER_CHECKS +#include "../clang-tidy/ClangTidyForceLinker.h" +#endif + +#if CLANG_TIDY_ENABLE_QUERY_BASED_CUSTOM_CHECKS +#include "../clang-tidy/ClangTidy.h" +#include +#endif + +namespace clang { +namespace clangd { +namespace { + +// 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 = false; + // Set of diag::Group whose enablement != Default. + // If Default is false, this is foo where we've seen clang-diagnostic-foo. + llvm::DenseSet Exceptions; + +public: + TidyDiagnosticGroups(llvm::StringRef Checks) { + constexpr llvm::StringLiteral Prefix = "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 (Prefix.starts_with(Check)) { + Default = Enable; + Exceptions.clear(); + } + continue; + } + // In "*,clang-diagnostic-foo", the latter is a no-op. + // The only non-glob entries we care about are clang-diagnostic-foo. + if (Default == Enable || !Check.consume_front(Prefix)) + continue; + if (auto Group = DiagnosticIDs::getGroupForWarningOption(Check)) + Exceptions.insert(static_cast(*Group)); + } + } + + bool operator()(diag::Group Group) const { + return Exceptions.contains(static_cast(Group)) ? !Default + : Default; + } +}; + +// Find -W and -Wno- options in ExtraArgs and apply them to Diags. +// +// This is used to handle ExtraArgs 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 applyWarningOptions(llvm::ArrayRef ExtraArgs, + llvm::function_ref EnabledGroups, + DiagnosticsEngine &Diags) { + for (llvm::StringRef Group : ExtraArgs) { + // 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( + 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 NeedsWerrorExclusion = false; + for (diag::kind ID : Members) { + if (Enable) { + if (Diags.getDiagnosticLevel(ID, SourceLocation()) >= + DiagnosticsEngine::Warning) + continue; + auto DiagGroup = Diags.getDiagnosticIDs()->getGroupForDiag(ID); + if (!DiagGroup || !EnabledGroups(*DiagGroup)) + continue; + Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation()); + NeedsWerrorExclusion |= Diags.getWarningsAsErrors(); + } else { + Diags.setSeverity(ID, diag::Severity::Ignored, SourceLocation()); + } + } + if (NeedsWerrorExclusion) { + // 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); + } + } +} + +tidy::ClangTidyCheckFactories +filterFastChecks(const tidy::ClangTidyCheckFactories &All, + Config::FastCheckPolicy Policy) { + if (Policy == Config::FastCheckPolicy::None) + return All; + bool AllowUnknown = Policy == Config::FastCheckPolicy::Loose; + tidy::ClangTidyCheckFactories Fast; + for (const auto &Factory : All) + if (isFastTidyCheck(Factory.getKey()).value_or(AllowUnknown)) + Fast.registerCheckFactory(Factory.first(), Factory.second); + return Fast; +} + +class TidyASTListener final : public FeatureModule::ASTListener { +public: + explicit TidyASTListener(std::shared_ptr OptionsProvider) + : Provider(std::move(OptionsProvider)) {} + + void beforeBeginSourceFile(CompilerInstance &CI) override { + // Warning options must precede frontend initialization: module loading in + // BeginSourceFile() can consult their severity to decide whether to fail. + const auto &Inputs = CI.getFrontendOpts().Inputs; + if (Inputs.empty() || !Inputs.front().isFile()) { + elog("Cannot configure clang-tidy without a main-file input"); + return; + } + llvm::SmallString<256> Path(Inputs.front().getFile()); + // The remapped buffer carries clangd's original main-file spelling. + for (const auto &Remap : + llvm::reverse(CI.getPreprocessorOpts().RemappedFileBuffers)) { + if (Remap.first == Inputs.front().getFile()) { + Path = Remap.second->getBufferIdentifier(); + break; + } + } + // Providers search parent directories for .clang-tidy and require an + // absolute path. Respect the compiler's working directory, not clangd's. + if (!llvm::sys::path::is_absolute(Path)) + CI.getFileManager().makeAbsolutePath(Path, /*Canonicalize=*/true); + if (!llvm::sys::path::is_absolute(Path)) { + elog("Cannot resolve clang-tidy input path: {0}", Path); + return; + } + Filename = Path.str().str(); + trace::Span Tracer("ClangTidyOpts"); + Options = getTidyOptionsForFile(*Provider, Filename); + dlog("ClangTidy configuration for file {0}: {1}", Filename, + tidy::configurationAsText(*Options)); + + // 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. + TidyDiagnosticGroups EnabledGroups(Options->Checks ? *Options->Checks + : llvm::StringRef()); + if (Options->ExtraArgsBefore) + applyWarningOptions(*Options->ExtraArgsBefore, EnabledGroups, + CI.getDiagnostics()); + if (Options->ExtraArgs) + applyWarningOptions(*Options->ExtraArgs, EnabledGroups, + CI.getDiagnostics()); + } + + void beforePPCallbacks(CompilerInstance &CI) override { + // Preamble builds don't call beforeBeginSourceFile(). Running tidy there + // would duplicate diagnostics and interfere with PCH generation. + if (!Options) + return; + + // Set up ClangTidy. Must happen after BeginSourceFile() so ASTContext + // exists. Clang-tidy has some limitations to ensure reasonable performance: + // - checks don't see all preprocessor events in the preamble + // - matchers run only over the main-file top-level decls (and can't see + // ancestors outside this scope). + // In practice almost all checks work well without modifications. + trace::Span Tracer("ClangTidyInit"); + static const auto *AllFactories = [] { + auto *Factories = new tidy::ClangTidyCheckFactories; + for (const auto &Entry : tidy::ClangTidyModuleRegistry::entries()) + Entry.instantiate()->addCheckFactories(*Factories); + return Factories; + }(); + Context.emplace( + std::make_unique( + tidy::ClangTidyGlobalOptions(), std::move(*Options)), + /*AllowEnablingAnalyzerAlphaCheckers=*/false, + /*EnableModuleHeadersParsing=*/false, + Config::current().Diagnostics.ClangTidy.ExperimentalCustomChecks); + Options.reset(); + // The lifetime of DiagnosticOptions is managed by \c Clang. + Context->setDiagnosticsEngine(nullptr, &CI.getDiagnostics()); + Context->setASTContext(&CI.getASTContext()); + Context->setCurrentFile(Filename); + Context->setSelfContainedDiags(true); + tidy::ClangTidyCheckFactories Factories = *AllFactories; +#if CLANG_TIDY_ENABLE_QUERY_BASED_CUSTOM_CHECKS + if (Context->canExperimentalCustomChecks() && + tidy::custom::RegisterCustomChecks) { + // RegisterCustomChecks tracks names in process-wide mutable state. + // Serializing its use keeps concurrent AST builds independent. + static std::mutex CustomChecksMu; + std::lock_guard Lock(CustomChecksMu); + tidy::custom::RegisterCustomChecks(Context->getOptions(), Factories); + } +#endif + auto FastFactories = filterFastChecks( + Factories, Config::current().Diagnostics.ClangTidy.FastCheckFilter); + Checks = FastFactories.createChecksForLanguage(&*Context); + + Preprocessor *PP = &CI.getPreprocessor(); + for (const auto &Check : Checks) { + Check->registerPPCallbacks(CI.getSourceManager(), PP, PP); + Check->registerMatchers(&Finder); + } + } + + void afterExecute(CompilerInstance &CI) override { + if (Checks.empty()) + return; + // Run the AST-dependent part of the clang-tidy checks. + // (The preprocessor part ran already, via PPCallbacks). + trace::Span Tracer("ClangTidyMatch"); + Finder.matchAST(CI.getASTContext()); + } + + void sawDiagnostic(const clang::Diagnostic &Info, clangd::Diag &D) override { + // Preserve the previous suppression policy when no checks are active. + if (Checks.empty()) + return; + std::string CheckName = Context->getCheckName(Info.getID()); + if (CheckName.empty()) + return; + + if (Config::current().Diagnostics.Suppress.contains(CheckName)) { + D.Severity = DiagnosticsEngine::Ignored; + return; + } + + // 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 InsideMainFile = + Info.hasSourceManager() && + isInsideMainFile(Info.getLocation(), Info.getSourceManager()); + llvm::SmallVector SuppressionErrors; + if (InsideMainFile && + Context->shouldSuppressDiagnostic(D.Severity, Info, SuppressionErrors, + /*AllowIO=*/false, + /*EnableNolintBlocks=*/true)) { + // FIXME: should we expose the suppression error (invalid use of + // NOLINT comments)? + D.Severity = DiagnosticsEngine::Ignored; + return; + } + if (!Context->getOptions().SystemHeaders.value_or(false) && + Info.hasSourceManager() && + Info.getSourceManager().isInSystemMacro(Info.getLocation())) { + D.Severity = DiagnosticsEngine::Ignored; + return; + } + // Check for warning-as-error. + if (D.Severity == DiagnosticsEngine::Warning && + Context->treatAsError(CheckName)) + D.Severity = DiagnosticsEngine::Error; + + // Compiler warnings keep their clang source and -W name, even when their + // severity or suppression is controlled by clang-tidy options. + if (!Context->isCompilerDiagnostic(Info.getID())) { + D.Name = std::move(CheckName); + D.Source = Diag::ClangTidy; + } + } + + void finalizeDiagnostic(clangd::Diag &D) override { + // Constructor diagnostics precede check initialization. + if (D.Source == Diag::Unknown && Context && + !Context->isCompilerDiagnostic(D.ID)) { + auto Name = Context->getCheckName(D.ID); + if (!Name.empty()) { + D.Name = std::move(Name); + D.Source = Diag::ClangTidy; + } + } + if (D.Source != Diag::ClangTidy) + return; + // clang-tidy bakes the name into diagnostic messages. Strip it out. + // It would be much nicer to make clang-tidy not do this. + auto CleanMessage = [&](std::string &Message) { + llvm::StringRef Rest(Message); + if (Rest.consume_back("]") && Rest.consume_back(D.Name) && + Rest.consume_back(" [")) + Message.resize(Rest.size()); + }; + CleanMessage(D.Message); + for (auto &Note : D.Notes) + CleanMessage(Note.Message); + for (auto &Fix : D.Fixes) + CleanMessage(Fix.Message); + if (llvm::StringRef(D.Name).starts_with("misc-unused-") && + !llvm::is_contained(D.Tags, DiagnosticTag::Unnecessary)) + D.Tags.push_back(DiagnosticTag::Unnecessary); + if (llvm::StringRef(D.Name).starts_with("modernize-") && + !llvm::is_contained(D.Tags, DiagnosticTag::Deprecated)) + D.Tags.push_back(DiagnosticTag::Deprecated); + } + +private: + std::shared_ptr Provider; + std::string Filename; + std::optional Options; + // Destruction order matters: Finder and checks refer to Context. + std::optional Context; + std::vector> Checks; + ast_matchers::MatchFinder Finder; +}; + +} // namespace + +std::unique_ptr +ClangTidyFeatureModule::astListeners() { + std::lock_guard Lock(ProviderMu); + if (!Provider) + return nullptr; + return std::make_unique(Provider); +} + +void ClangTidyFeatureModule::setProvider(TidyProvider OptionsProvider) { + std::shared_ptr Replacement; + if (OptionsProvider) + Replacement = + std::make_shared(std::move(OptionsProvider)); + swapProvider(Replacement); +} + +void ClangTidyFeatureModule::swapProvider( + std::shared_ptr &OptionsProvider) { + std::lock_guard Lock(ProviderMu); + Provider.swap(OptionsProvider); +} + +} // namespace clangd +} // namespace clang diff --git a/clang-tools-extra/clangd/ClangTidyFeatureModule.h b/clang-tools-extra/clangd/ClangTidyFeatureModule.h new file mode 100644 index 000000000000..9ad5a87f3f8d --- /dev/null +++ b/clang-tools-extra/clangd/ClangTidyFeatureModule.h @@ -0,0 +1,47 @@ +//===--- ClangTidyFeatureModule.h - clang-tidy integration -----*- C++ -*-===// +// +// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. +// See https://llvm.org/LICENSE.txt for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +// +//===----------------------------------------------------------------------===// + +#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGTIDYFEATUREMODULE_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGTIDYFEATUREMODULE_H + +#include "FeatureModule.h" +#include "TidyProvider.h" +#include +#include +#include + +namespace clang { +namespace clangd { + +/// Runs clang-tidy checks as part of clangd's AST build. +class ClangTidyFeatureModule final : public FeatureModule { +public: + /// Construct explicitly with the application's clang-tidy options provider. + /// Takes ownership of the provider; an empty provider disables checks. + explicit ClangTidyFeatureModule(TidyProvider OptionsProvider) { + setProvider(std::move(OptionsProvider)); + } + + /// Swap provider snapshots, e.g. to temporarily override the configuration + /// for benchmarking and restore it afterwards. Existing listeners retain + /// their snapshot. + void swapProvider(std::shared_ptr &OptionsProvider); + + std::unique_ptr astListeners() override; + +private: + void setProvider(TidyProvider OptionsProvider); + + std::mutex ProviderMu; + std::shared_ptr Provider; +}; + +} // namespace clangd +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_CLANGTIDYFEATUREMODULE_H diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp index 8535823abc52..8e5b0938242d 100644 --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -219,7 +219,6 @@ ClangdServer::ClangdServer(const GlobalCompilationDatabase &CDB, ? new FileIndex(Opts.EnableOutgoingCalls) : nullptr), ModulesManager(Opts.ModulesManager), - ClangTidyProvider(Opts.ClangTidyProvider), UseDirtyHeaders(Opts.UseDirtyHeaders), LineFoldingOnly(Opts.LineFoldingOnly), PreambleParseForwardingFunctions(Opts.PreambleParseForwardingFunctions), @@ -314,7 +313,6 @@ void ClangdServer::addDocument(PathRef File, llvm::StringRef Contents, Inputs.ForceRebuild = ForceRebuild; Inputs.Opts = std::move(Opts); Inputs.Index = Index; - Inputs.ClangTidyProvider = ClangTidyProvider; Inputs.FeatureModules = FeatureModules; Inputs.ModulesManager = ModulesManager; adjustParseInputs(Inputs, File); diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h index ce3e66e4eb28..e5eb660d4684 100644 --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -145,10 +145,6 @@ public: /// If there is no particular file (e.g. project loading) then it is empty. std::function ContextProvider; - /// The Options provider to use when running clang-tidy. If null, clang-tidy - /// checks will be disabled. - TidyProviderRef ClangTidyProvider; - /// Clangd's workspace root. Relevant for "workspace" operations not bound /// to a particular file. /// FIXME: If not set, should use the current working directory. @@ -507,9 +503,6 @@ private: // Manage module files. ModulesBuilder *ModulesManager = nullptr; - // When set, provides clang-tidy options for a specific file. - TidyProviderRef ClangTidyProvider; - bool UseDirtyHeaders = false; // Whether the client supports folding only complete lines. diff --git a/clang-tools-extra/clangd/Compiler.h b/clang-tools-extra/clangd/Compiler.h index 5e5e23d5b968..95dd6726bd1a 100644 --- a/clang-tools-extra/clangd/Compiler.h +++ b/clang-tools-extra/clangd/Compiler.h @@ -17,7 +17,6 @@ #include "FeatureModule.h" #include "ModulesBuilder.h" -#include "TidyProvider.h" #include "index/Index.h" #include "support/ThreadsafeFS.h" #include "clang/Frontend/CompilerInstance.h" @@ -60,7 +59,6 @@ struct ParseInputs { // Used to recover from diagnostics (e.g. find missing includes for symbol). const SymbolIndex *Index = nullptr; ParseOptions Opts = ParseOptions(); - TidyProviderRef ClangTidyProvider = {}; // Used to acquire ASTListeners when parsing files. FeatureModuleSet *FeatureModules = nullptr; // Used to build and manage (C++) modules. diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index 28351c7cc224..66e2d4c55cfa 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -7,7 +7,6 @@ //===----------------------------------------------------------------------===// #include "Diagnostics.h" -#include "../clang-tidy/ClangTidyDiagnosticConsumer.h" #include "Compiler.h" #include "Config.h" #include "Protocol.h" @@ -389,12 +388,6 @@ void setTags(clangd::Diag &D) { } else if (UnusedDiags->contains(D.ID)) { D.Tags.push_back(DiagnosticTag::Unnecessary); } - if (D.Source == Diag::ClangTidy) { - if (llvm::StringRef(D.Name).starts_with("misc-unused-")) - D.Tags.push_back(DiagnosticTag::Unnecessary); - if (llvm::StringRef(D.Name).starts_with("modernize-")) - D.Tags.push_back(DiagnosticTag::Deprecated); - } } } // namespace @@ -571,53 +564,38 @@ int getSeverity(DiagnosticsEngine::Level L) { llvm_unreachable("Unknown diagnostic level!"); } -std::vector StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) { +std::vector StoreDiags::take() { // Do not forget to emit a pending diagnostic if there is one. flushLastDiag(); // Fill in name/source now that we have all the context needed to map them. for (auto &Diag : Output) { if (const char *ClangDiag = getDiagnosticCode(Diag.ID)) { - // Warnings controlled by -Wfoo are better recognized by that name. - StringRef Warning = [&] { - if (OrigSrcMgr) { - return OrigSrcMgr->getDiagnostics() - .getDiagnosticIDs() - ->getWarningOptionForDiag(Diag.ID); + // Feature modules may already have supplied diagnostic metadata. + if (Diag.Source == Diag::Unknown) + Diag.Source = Diag::Clang; + if (Diag.Name.empty()) { + // Warnings controlled by -Wfoo are better recognized by that name. + StringRef Warning = [&] { + if (OrigSrcMgr) { + return OrigSrcMgr->getDiagnostics() + .getDiagnosticIDs() + ->getWarningOptionForDiag(Diag.ID); + } + if (!DiagnosticIDs::IsCustomDiag(Diag.ID)) + return DiagnosticIDs{}.getWarningOptionForDiag(Diag.ID); + return StringRef{}; + }(); + + if (!Warning.empty()) { + Diag.Name = ("-W" + Warning).str(); + } else { + StringRef Name(ClangDiag); + // Almost always an error, with a name like err_enum_class_reference. + // Drop the err_ prefix for brevity. + Name.consume_front("err_"); + Diag.Name = std::string(Name); } - if (!DiagnosticIDs::IsCustomDiag(Diag.ID)) - return DiagnosticIDs{}.getWarningOptionForDiag(Diag.ID); - return StringRef{}; - }(); - - if (!Warning.empty()) { - Diag.Name = ("-W" + Warning).str(); - } else { - StringRef Name(ClangDiag); - // Almost always an error, with a name like err_enum_class_reference. - // Drop the err_ prefix for brevity. - Name.consume_front("err_"); - Diag.Name = std::string(Name); - } - Diag.Source = Diag::Clang; - } else if (Tidy != nullptr) { - std::string TidyDiag = Tidy->getCheckName(Diag.ID); - if (!TidyDiag.empty()) { - Diag.Name = std::move(TidyDiag); - Diag.Source = Diag::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 CleanMessage = [&](std::string &Msg) { - StringRef Rest(Msg); - if (Rest.consume_back("]") && Rest.consume_back(Diag.Name) && - Rest.consume_back(" [")) - Msg.resize(Rest.size()); - }; - CleanMessage(Diag.Message); - for (auto &Note : Diag.Notes) - CleanMessage(Note.Message); - for (auto &Fix : Diag.Fixes) - CleanMessage(Fix.Message); } } setTags(Diag); diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h index ae710f351dbf..992709813fba 100644 --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -28,9 +28,6 @@ #include namespace clang { -namespace tidy { -class ClangTidyContext; -} // namespace tidy namespace clangd { struct ClangdDiagnosticOptions { @@ -137,8 +134,7 @@ std::optional getDiagnosticDocURI(Diag::DiagSource, unsigned ID, /// the diag itself nor its notes are in the main file). class StoreDiags : public DiagnosticConsumer { public: - // The ClangTidyContext populates Source and Name for clang-tidy diagnostics. - std::vector take(const clang::tidy::ClangTidyContext *Tidy = nullptr); + std::vector take(); void BeginSourceFile(const LangOptions &Opts, const Preprocessor *PP) override; diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 434138bcdfa5..52db921f660a 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -7,23 +7,17 @@ //===----------------------------------------------------------------------===// #include "ParsedAST.h" -#include "../clang-tidy/ClangTidyCheck.h" -#include "../clang-tidy/ClangTidyDiagnosticConsumer.h" -#include "../clang-tidy/ClangTidyModule.h" -#include "../clang-tidy/ClangTidyOptions.h" #include "AST.h" #include "CollectMacros.h" #include "Compiler.h" #include "Config.h" #include "Diagnostics.h" -#include "Feature.h" #include "FeatureModule.h" #include "Headers.h" #include "IncludeCleaner.h" #include "IncludeFixer.h" #include "Preamble.h" #include "SourceCode.h" -#include "TidyProvider.h" #include "clang-include-cleaner/Record.h" #include "index/Symbol.h" #include "support/Logger.h" @@ -33,7 +27,6 @@ #include "clang/AST/Decl.h" #include "clang/AST/DeclGroup.h" #include "clang/AST/ExternalASTSource.h" -#include "clang/ASTMatchers/ASTMatchFinder.h" #include "clang/Basic/Diagnostic.h" #include "clang/Basic/DiagnosticIDs.h" #include "clang/Basic/DiagnosticSema.h" @@ -55,11 +48,9 @@ #include "clang/Sema/HeuristicResolver.h" #include "clang/Serialization/ASTWriter.h" #include "clang/Tooling/CompilationDatabase.h" -#include "clang/Tooling/Core/Diagnostic.h" #include "clang/Tooling/Syntax/Tokens.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/DenseMap.h" -#include "llvm/ADT/DenseSet.h" #include "llvm/ADT/STLExtras.h" #include "llvm/ADT/STLFunctionalExtras.h" #include "llvm/ADT/SmallVector.h" @@ -72,22 +63,9 @@ #include #include #include -#include #include #include -// Force the linker to link in Clang-tidy modules. -// clangd doesn't support the static analyzer. -#if CLANGD_TIDY_CHECKS -#define CLANG_TIDY_DISABLE_STATIC_ANALYZER_CHECKS -#include "../clang-tidy/ClangTidyForceLinker.h" -#endif - -#if CLANG_TIDY_ENABLE_QUERY_BASED_CUSTOM_CHECKS -#include "../clang-tidy/ClangTidy.h" -#include -#endif - namespace clang { namespace clangd { namespace { @@ -284,111 +262,6 @@ private: std::vector MainFileTokens; }; -// 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 = false; - // Set of diag::Group whose enablement != Default. - // If Default 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; - Exceptions.clear(); - } - continue; - } - - // In "*,clang-diagnostic-foo", the latter is a no-op. - if (Default == Enable) - continue; - // The only non-glob entries we care about are clang-diagnostic-foo. - if (!Check.consume_front(CDPrefix)) - continue; - - if (auto Group = DiagnosticIDs::getGroupForWarningOption(Check)) - Exceptions.insert(static_cast(*Group)); - } - } - - bool operator()(diag::Group GroupID) const { - return Exceptions.contains(static_cast(GroupID)) ? !Default - : Default; - } -}; - -// Find -W and -Wno- options in ExtraArgs and apply them to Diags. -// -// This is used to handle ExtraArgs 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 applyWarningOptions(llvm::ArrayRef ExtraArgs, - llvm::function_ref EnabledGroups, - DiagnosticsEngine &Diags) { - for (llvm::StringRef Group : ExtraArgs) { - // 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( - 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 NeedsWerrorExclusion = false; - for (diag::kind ID : Members) { - if (Enable) { - if (Diags.getDiagnosticLevel(ID, SourceLocation()) < - DiagnosticsEngine::Warning) { - auto Group = Diags.getDiagnosticIDs()->getGroupForDiag(ID); - if (!Group || !EnabledGroups(*Group)) - continue; - Diags.setSeverity(ID, diag::Severity::Warning, SourceLocation()); - if (Diags.getWarningsAsErrors()) - NeedsWerrorExclusion = true; - } - } else { - Diags.setSeverity(ID, diag::Severity::Ignored, SourceLocation()); - } - } - if (NeedsWerrorExclusion) { - // 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); - } - } -} - std::vector getIncludeCleanerDiags(ParsedAST &AST, llvm::StringRef Code, const ThreadsafeFS &TFS) { auto &Cfg = Config::current(); @@ -413,20 +286,6 @@ std::vector getIncludeCleanerDiags(ParsedAST &AST, llvm::StringRef Code, Cfg.Style.AngledHeaders, Cfg.Style.QuotedHeaders); } -tidy::ClangTidyCheckFactories -filterFastTidyChecks(const tidy::ClangTidyCheckFactories &All, - Config::FastCheckPolicy Policy) { - if (Policy == Config::FastCheckPolicy::None) - return All; - bool AllowUnknown = Policy == Config::FastCheckPolicy::Loose; - tidy::ClangTidyCheckFactories Fast; - for (const auto &Factory : All) { - if (isFastTidyCheck(Factory.getKey()).value_or(AllowUnknown)) - Fast.registerCheckFactory(Factory.first(), Factory.second); - } - return Fast; -} - } // namespace std::optional @@ -509,40 +368,6 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, : "unknown error"); return std::nullopt; } - tidy::ClangTidyOptions ClangTidyOpts; - { - trace::Span Tracer("ClangTidyOpts"); - ClangTidyOpts = getTidyOptionsForFile(Inputs.ClangTidyProvider, Filename); - dlog("ClangTidy configuration for file {0}: {1}", Filename, - tidy::configurationAsText(ClangTidyOpts)); - - // 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 = Clang->getDiagnostics(); - TidyDiagnosticGroups TidyGroups(ClangTidyOpts.Checks ? *ClangTidyOpts.Checks - : llvm::StringRef()); - if (ClangTidyOpts.ExtraArgsBefore) - applyWarningOptions(*ClangTidyOpts.ExtraArgsBefore, TidyGroups, Diags); - if (ClangTidyOpts.ExtraArgs) - applyWarningOptions(*ClangTidyOpts.ExtraArgs, TidyGroups, Diags); - } - auto Action = std::make_unique(); for (const auto &L : ASTListeners) L->beforeBeginSourceFile(*Clang); @@ -563,150 +388,59 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, Clang->getPreprocessor().getHeaderSearchInfo().MarkFileIncludeOnce(*MainFE); } - // Set up ClangTidy. Must happen after BeginSourceFile() so ASTContext exists. - // Clang-tidy has some limitations to ensure reasonable performance: - // - checks don't see all preprocessor events in the preamble - // - matchers run only over the main-file top-level decls (and can't see - // ancestors outside this scope). - // In practice almost all checks work well without modifications. - std::vector> CTChecks; - ast_matchers::MatchFinder CTFinder; - std::optional CTContext; // Must outlive FixIncludes. auto BuildDir = VFS->getCurrentWorkingDirectory(); std::optional FixIncludes; llvm::DenseMap OverriddenSeverity; - // No need to run clang-tidy or IncludeFixerif we are not going to surface - // diagnostics. - { - trace::Span Tracer("ClangTidyInit"); - static const auto *AllCTFactories = [] { - auto *CTFactories = new tidy::ClangTidyCheckFactories; - for (const auto &E : tidy::ClangTidyModuleRegistry::entries()) - E.instantiate()->addCheckFactories(*CTFactories); - return CTFactories; - }(); - CTContext.emplace(std::make_unique( - tidy::ClangTidyGlobalOptions(), ClangTidyOpts), - /*AllowEnablingAnalyzerAlphaCheckers=*/false, - /*EnableModuleHeadersParsing=*/false, - Cfg.Diagnostics.ClangTidy.ExperimentalCustomChecks); - // The lifetime of DiagnosticOptions is managed by \c Clang. - CTContext->setDiagnosticsEngine(nullptr, &Clang->getDiagnostics()); - CTContext->setASTContext(&Clang->getASTContext()); - CTContext->setCurrentFile(Filename); - CTContext->setSelfContainedDiags(true); - tidy::ClangTidyCheckFactories CTCheckFactories = *AllCTFactories; -#if CLANG_TIDY_ENABLE_QUERY_BASED_CUSTOM_CHECKS - if (CTContext->canExperimentalCustomChecks() && - tidy::custom::RegisterCustomChecks) { - // RegisterCustomChecks tracks names in process-wide mutable state. - // Serializing its use keeps concurrent AST builds independent. - static std::mutex CustomChecksMu; - std::lock_guard Lock(CustomChecksMu); - tidy::custom::RegisterCustomChecks(CTContext->getOptions(), - CTCheckFactories); - } -#endif - tidy::ClangTidyCheckFactories FastFactories = filterFastTidyChecks( - CTCheckFactories, Cfg.Diagnostics.ClangTidy.FastCheckFilter); - CTChecks = FastFactories.createChecksForLanguage(&*CTContext); - Preprocessor *PP = &Clang->getPreprocessor(); - for (const auto &Check : CTChecks) { - Check->registerPPCallbacks(Clang->getSourceManager(), PP, PP); - Check->registerMatchers(&CTFinder); - } + // Clang only corrects typos for use of undeclared functions in C if that + // use is an error. Include fixer relies on typo correction, so pretend + // this is an error. (The actual typo correction is nice too). + // We restore the original severity in the level adjuster. + // FIXME: It would be better to have a real API for this, but what? + for (auto ID : {diag::ext_implicit_function_decl_c99, + diag::ext_implicit_lib_function_decl, + diag::ext_implicit_lib_function_decl_c99, + diag::warn_implicit_function_decl}) { + OverriddenSeverity.try_emplace( + ID, Clang->getDiagnostics().getDiagnosticLevel(ID, SourceLocation())); + Clang->getDiagnostics().setSeverity(ID, diag::Severity::Error, + SourceLocation()); + } + ASTDiags.setLevelAdjuster( + [&](DiagnosticsEngine::Level Level, const clang::Diagnostic &Info) { + auto It = OverriddenSeverity.find(Info.getID()); + return It == OverriddenSeverity.end() ? Level : It->second; + }); - // Clang only corrects typos for use of undeclared functions in C if that - // use is an error. Include fixer relies on typo correction, so pretend - // this is an error. (The actual typo correction is nice too). - // We restore the original severity in the level adjuster. - // FIXME: It would be better to have a real API for this, but what? - for (auto ID : {diag::ext_implicit_function_decl_c99, - diag::ext_implicit_lib_function_decl, - diag::ext_implicit_lib_function_decl_c99, - diag::warn_implicit_function_decl}) { - OverriddenSeverity.try_emplace( - ID, Clang->getDiagnostics().getDiagnosticLevel(ID, SourceLocation())); - Clang->getDiagnostics().setSeverity(ID, diag::Severity::Error, - SourceLocation()); + // Add IncludeFixer which can recover diagnostics caused by missing includes + // (e.g. incomplete type) and attach include insertion fixes to diagnostics. + if (Inputs.Index && !BuildDir.getError()) { + auto Style = + getFormatStyleForFile(Filename, Inputs.Contents, *Inputs.TFS, false); + auto Inserter = std::make_shared( + Filename, Inputs.Contents, Style, BuildDir.get(), + &Clang->getPreprocessor().getHeaderSearchInfo(), + Cfg.Style.QuotedHeaders, Cfg.Style.AngledHeaders); + ArrayRef MainFileIncludes; + if (Preamble) { + MainFileIncludes = Preamble->Includes.MainFileIncludes; + for (const auto &Inc : Preamble->Includes.MainFileIncludes) + Inserter->addExisting(Inc); } - - ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel, - const clang::Diagnostic &Info) { - auto It = OverriddenSeverity.find(Info.getID()); - if (It != OverriddenSeverity.end()) - DiagLevel = It->second; - - if (!CTChecks.empty()) { - std::string CheckName = CTContext->getCheckName(Info.getID()); - bool IsClangTidyDiag = !CheckName.empty(); - if (IsClangTidyDiag) { - if (Cfg.Diagnostics.Suppress.contains(CheckName)) - return DiagnosticsEngine::Ignored; - // 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 IsInsideMainFile = - Info.hasSourceManager() && - isInsideMainFile(Info.getLocation(), Info.getSourceManager()); - SmallVector TidySuppressedErrors; - if (IsInsideMainFile && CTContext->shouldSuppressDiagnostic( - DiagLevel, Info, TidySuppressedErrors, - /*AllowIO=*/false, - /*EnableNolintBlocks=*/true)) { - // FIXME: should we expose the suppression error (invalid use of - // NOLINT comments)? - return DiagnosticsEngine::Ignored; - } - if (!CTContext->getOptions().SystemHeaders.value_or(false) && - Info.hasSourceManager() && - Info.getSourceManager().isInSystemMacro(Info.getLocation())) - return DiagnosticsEngine::Ignored; - - // Check for warning-as-error. - if (DiagLevel == DiagnosticsEngine::Warning && - CTContext->treatAsError(CheckName)) { - return DiagnosticsEngine::Error; - } - } - } - return DiagLevel; + // FIXME: Consider piping through ASTSignals to fetch this to handle the + // case where a header file contains ObjC decls but no #imports. + Symbol::IncludeDirective Directive = + Inputs.Opts.ImportInsertions + ? preferredIncludeDirective(Filename, Clang->getLangOpts(), + MainFileIncludes, {}) + : Symbol::Include; + FixIncludes.emplace(Filename, Inserter, *Inputs.Index, + /*IndexRequestLimit=*/5, Directive); + ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level Level, + const clang::Diagnostic &Info) { + return FixIncludes->fix(Level, Info); }); - - // Add IncludeFixer which can recover diagnostics caused by missing includes - // (e.g. incomplete type) and attach include insertion fixes to diagnostics. - if (Inputs.Index && !BuildDir.getError()) { - auto Style = - getFormatStyleForFile(Filename, Inputs.Contents, *Inputs.TFS, false); - auto Inserter = std::make_shared( - Filename, Inputs.Contents, Style, BuildDir.get(), - &Clang->getPreprocessor().getHeaderSearchInfo(), - Cfg.Style.QuotedHeaders, Cfg.Style.AngledHeaders); - ArrayRef MainFileIncludes; - if (Preamble) { - MainFileIncludes = Preamble->Includes.MainFileIncludes; - for (const auto &Inc : Preamble->Includes.MainFileIncludes) - Inserter->addExisting(Inc); - } - // FIXME: Consider piping through ASTSignals to fetch this to handle the - // case where a header file contains ObjC decls but no #imports. - Symbol::IncludeDirective Directive = - Inputs.Opts.ImportInsertions - ? preferredIncludeDirective(Filename, Clang->getLangOpts(), - MainFileIncludes, {}) - : Symbol::Include; - FixIncludes.emplace(Filename, Inserter, *Inputs.Index, - /*IndexRequestLimit=*/5, Directive); - ASTDiags.contributeFixes([&FixIncludes](DiagnosticsEngine::Level DiagLevl, - const clang::Diagnostic &Info) { - return FixIncludes->fix(DiagLevl, Info); - }); - Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder()); - } + Clang->setExternalSemaSource(FixIncludes->unresolvedNameRecorder()); } // ReplayPreamble must capture callbacks installed by feature modules. @@ -776,12 +510,6 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, std::vector ParsedDecls = Action->takeTopLevelDecls(); // AST traversals should exclude the preamble, to avoid performance cliffs. Clang->getASTContext().setTraversalScope(ParsedDecls); - if (!CTChecks.empty()) { - // Run the AST-dependent part of the clang-tidy checks. - // (The preprocessor part ran already, via PPCallbacks). - trace::Span Tracer("ClangTidyMatch"); - CTFinder.matchAST(Clang->getASTContext()); - } for (const auto &L : ASTListeners) L->afterExecute(*Clang); @@ -803,7 +531,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, llvm::append_range(Diags, Patch->patchedDiags()); // Finally, add diagnostics coming from the AST. { - std::vector D = ASTDiags.take(&*CTContext); + std::vector D = ASTDiags.take(); Diags.insert(Diags.end(), D.begin(), D.end()); } ParsedAST Result(Filename, Inputs.Version, std::move(Preamble), diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp index 8284454c6cb5..a22f36ca3a10 100644 --- a/clang-tools-extra/clangd/tool/Check.cpp +++ b/clang-tools-extra/clangd/tool/Check.cpp @@ -27,6 +27,7 @@ #include "../clang-tidy/ClangTidyModule.h" #include "../clang-tidy/ClangTidyOptions.h" #include "../clang-tidy/GlobList.h" +#include "ClangTidyFeatureModule.h" #include "ClangdLSPServer.h" #include "ClangdServer.h" #include "CodeComplete.h" @@ -63,6 +64,7 @@ #include "clang/Tooling/CompilationDatabase.h" #include "llvm/ADT/ArrayRef.h" #include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/ScopeExit.h" #include "llvm/ADT/SmallString.h" #include "llvm/Support/Chrono.h" #include "llvm/Support/CommandLine.h" @@ -205,7 +207,7 @@ public: std::vector CC1Args; Inputs.CompileCommand = Cmd; Inputs.TFS = &TFS; - Inputs.ClangTidyProvider = Opts.ClangTidyProvider; + Inputs.FeatureModules = Opts.FeatureModules; Inputs.Opts.PreambleParseForwardingFunctions = Opts.PreambleParseForwardingFunctions; if (Contents) { @@ -284,7 +286,8 @@ public: #ifndef NDEBUG elog("Timing clang-tidy checks in asserts-mode is not representative!"); #endif - checkTidyTimes(); + if (!checkTidyTimes()) + return false; } return true; @@ -297,7 +300,16 @@ public: // step-function slowdowns due to CPU scaling. // We take the median of 5 measurements, and after every check discard the // measurement if the baseline changed by >3%. - void checkTidyTimes() { + bool checkTidyTimes() { + // Keep all other modules active while varying only tidy's configuration. + auto *TidyModule = + Inputs.FeatureModules + ? Inputs.FeatureModules->get() + : nullptr; + if (!TidyModule) { + elog("Cannot time clang-tidy checks without a clang-tidy feature module"); + return false; + } double Stability = 0.03; log("Timing AST build with individual clang-tidy checks (target accuracy " "{0:P0})", @@ -322,7 +334,12 @@ public: llvm::StringRef) { Opts.Checks = Checks.str(); }; - Inputs.ClangTidyProvider = CTProvider; + auto Provider = + std::make_shared(std::move(CTProvider)); + TidyModule->swapProvider(Provider); + // Provider now holds the original configuration; restore it on exit. + llvm::scope_exit RestoreProvider( + [&] { TidyModule->swapProvider(Provider); }); // Sigh, can't reuse the CompilerInvocation. IgnoringDiagConsumer IgnoreDiags; auto Invocation = buildCompilerInvocation(Inputs, IgnoreDiags); @@ -367,9 +384,7 @@ public: log(" {0} = {1:P0}", Check, Fraction); } log("Finished individual clang-tidy checks"); - - // Restore old options. - Inputs.ClangTidyProvider = Opts.ClangTidyProvider; + return true; } // Build Inlay Hints for the entire AST or the specified range diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp index 451495e1dbf1..9bd7f0ef4cf6 100644 --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "ClangdMain.h" +#include "ClangTidyFeatureModule.h" #include "ClangdLSPServer.h" #include "CodeComplete.h" #include "Compiler.h" @@ -1025,7 +1026,6 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var Providers.push_back(provideDefaultChecks()); Providers.push_back(disableUnusableChecks()); ClangTidyOptProvider = combine(std::move(Providers)); - Opts.ClangTidyProvider = ClangTidyOptProvider; } Opts.UseDirtyHeaders = UseDirtyHeaders; Opts.PreambleParseForwardingFunctions = PreambleParseForwardingFunctions; @@ -1043,6 +1043,9 @@ clangd accepts flags on the commandline, and in the CLANGD_FLAGS environment var Opts.Encoding = ForceOffsetEncoding; FeatureModuleSet ModuleSet = FeatureModuleSet::fromRegistry(); + if (EnableClangTidy) + ModuleSet.add(std::make_unique( + std::move(ClangTidyOptProvider))); if (ModuleSet.begin() != ModuleSet.end()) Opts.FeatureModules = &ModuleSet; diff --git a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp index 460d6201d5a5..28eb48f4bca2 100644 --- a/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdLSPServerTests.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "Annotations.h" +#include "ClangTidyFeatureModule.h" #include "ClangdLSPServer.h" #include "ClangdServer.h" #include "ConfigProvider.h" @@ -214,7 +215,8 @@ TEST_F(LSPTest, ClangTidyRename) { ClangTidyOpts.CheckOptions["readability-identifier-naming.FunctionCase"] = "CamelCase"; }; - Opts.ClangTidyProvider = ClangTidyProvider; + FeatureModules.add( + std::make_unique(ClangTidyProvider)); auto &Client = start(); Client.didOpen("foo.hpp", Header.code()); Client.didOpen("foo.cpp", Source.code()); @@ -272,7 +274,8 @@ TEST_F(LSPTest, ClangTidyCrash_Issue109367) { llvm::StringRef) { ClangTidyOpts.Checks = {"-*,boost-use-ranges"}; }; - Opts.ClangTidyProvider = ClangTidyProvider; + FeatureModules.add( + std::make_unique(ClangTidyProvider)); // Check that registering the boost-use-ranges checker's matchers // on two different threads does not cause a crash. auto &Client = start(); diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp index 9ea7c3e02411..0b3b3485f2a7 100644 --- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "Annotations.h" +#include "ClangTidyFeatureModule.h" #include "ClangdServer.h" #include "CodeComplete.h" #include "CompileCommands.h" @@ -1228,7 +1229,9 @@ TEST(ClangdServer, TidyOverrideTest) { TidyProvider Provider = combine(std::move(Stack)); CDB.ExtraClangFlags = {"-xc++"}; auto Opts = ClangdServer::optsForTest(); - Opts.ClangTidyProvider = Provider; + FeatureModuleSet Modules; + Modules.add(std::make_unique(std::move(Provider))); + Opts.FeatureModules = &Modules; ClangdServer Server(CDB, FS, Opts, &DiagConsumer); const char *SourceContents = R"cpp( struct Foo { Foo(); Foo(Foo&); Foo(Foo&&); }; diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp index 381ba05ccfea..982854c1e5c1 100644 --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -8,6 +8,7 @@ #include "../clang-tidy/ClangTidyOptions.h" #include "Annotations.h" +#include "ClangTidyFeatureModule.h" #include "Compiler.h" #include "Config.h" #include "Diagnostics.h" @@ -847,13 +848,17 @@ TEST(DiagnosticTest, ClangTidyCompilerWarningsWithoutChecks) { TEST(DiagnosticTest, ClangTidyWarningOptionsBeforeBeginSourceFile) { auto Build = [](bool Suppress, bool ExtraArgsBefore) { auto TU = TestTU::withCode("int value;"); - TU.ClangTidyProvider = [=](tidy::ClangTidyOptions &Opts, llvm::StringRef) { - Opts.Checks = "-*"; - if (Suppress) { - auto &Args = ExtraArgsBefore ? Opts.ExtraArgsBefore : Opts.ExtraArgs; - Args = {"-Wno-incomplete-umbrella"}; - } - }; + FeatureModuleSet Modules; + Modules.add(std::make_unique( + [=](tidy::ClangTidyOptions &Opts, llvm::StringRef) { + Opts.Checks = "-*"; + if (Suppress) { + auto &Args = + ExtraArgsBefore ? Opts.ExtraArgsBefore : Opts.ExtraArgs; + Args = {"-Wno-incomplete-umbrella"}; + } + })); + TU.FeatureModules = &Modules; TU.ExtraArgs = {"-fmodules", "-fmodule-map-file=" + testPath("module.modulemap")}; TU.AdditionalFiles["module.modulemap"] = @@ -885,11 +890,13 @@ TEST(DiagnosticTest, ClangTidyRelativeInput) { TU.AdditionalFiles["project/src/.clang-tidy"] = "Checks: '-*,modernize-use-nullptr'"; auto Provider = provideClangTidyFiles(FS); - TU.ClangTidyProvider = [&](tidy::ClangTidyOptions &Opts, - llvm::StringRef Filename) { - EXPECT_EQ(Filename, testPath(TU.Filename)); - Provider(Opts, Filename); - }; + FeatureModuleSet Modules; + Modules.add(std::make_unique( + [&](tidy::ClangTidyOptions &Opts, llvm::StringRef Filename) { + EXPECT_EQ(Filename, testPath(TU.Filename)); + Provider(Opts, Filename); + })); + TU.FeatureModules = &Modules; auto Inputs = TU.inputs(FS); Inputs.CompileCommand.Directory = testPath("project"); // Configuration lookup must use the requested file, not the frontend input. diff --git a/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp b/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp index 46571cfda4e4..826c926b37de 100644 --- a/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp +++ b/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "Annotations.h" +#include "ClangTidyFeatureModule.h" #include "Feature.h" #include "FeatureModule.h" #include "Selection.h" @@ -306,6 +307,26 @@ TEST(FeatureModulesTest, FinalizeDiagnostic) { EXPECT_EQ(Fixes, 1u); } +TEST(FeatureModulesTest, ClangTidyTemporaryProvider) { + if (!CLANGD_TIDY_CHECKS) + GTEST_SKIP() << "Requires clang-tidy checks"; + FeatureModuleSet Modules; + Modules.add(std::make_unique( + addTidyChecks("modernize-use-nullptr"))); + auto TU = TestTU::withCode("int *p = 0;"); + TU.FeatureModules = &Modules; + auto *Tidy = Modules.get(); + auto Provider = std::make_shared(addTidyChecks("-*")); + // Check-mode timing temporarily overrides tidy's configuration, then + // restores the original provider for subsequent builds. + Tidy->swapProvider(Provider); + EXPECT_THAT(TU.build().getDiagnostics(), testing::IsEmpty()); + Tidy->swapProvider(Provider); + auto AST = TU.build(); + ASSERT_THAT(AST.getDiagnostics(), testing::SizeIs(1)); + EXPECT_EQ(AST.getDiagnostics().front().Name, "modernize-use-nullptr"); +} + TEST(FeatureModulesTest, ClangTidyDoesNotReinitializeConsumer) { if (!CLANGD_TIDY_CHECKS) GTEST_SKIP() << "Requires clang-tidy checks"; @@ -331,7 +352,8 @@ TEST(FeatureModulesTest, ClangTidyDoesNotReinitializeConsumer) { Modules.add(std::move(Module)); auto TU = TestTU::withCode("int *p = 0;"); TU.FeatureModules = &Modules; - TU.ClangTidyProvider = addTidyChecks("modernize-use-nullptr"); + Modules.add(std::make_unique( + addTidyChecks("modernize-use-nullptr"))); // Running tidy must not reinitialize the existing AST consumer. EXPECT_THAT(TU.build().getDiagnostics(), testing::SizeIs(1)); } diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp index e2c2e3f000b6..fb959c7c8dcb 100644 --- a/clang-tools-extra/clangd/unittests/TestTU.cpp +++ b/clang-tools-extra/clangd/unittests/TestTU.cpp @@ -7,6 +7,7 @@ //===----------------------------------------------------------------------===// #include "TestTU.h" +#include "ClangTidyFeatureModule.h" #include "CompileCommands.h" #include "Compiler.h" #include "Diagnostics.h" @@ -73,8 +74,6 @@ ParseInputs TestTU::inputs(MockFS &FS) const { FS.OverlayRealFileSystemForModules = true; Inputs.TFS = &FS; Inputs.Opts = ParseOptions(); - if (ClangTidyProvider) - Inputs.ClangTidyProvider = ClangTidyProvider; Inputs.Index = ExternalIndex; return Inputs; } @@ -113,9 +112,21 @@ TestTU::preamble(PreambleParsedCallback PreambleCallback) const { } ParsedAST TestTU::build() const { + if (ClangTidyProvider && FeatureModules) { + llvm::errs() << "Use either ClangTidyProvider or FeatureModules\n"; + std::abort(); + } MockFS FS; auto Inputs = inputs(FS); Inputs.Opts = ParseOpts; + FeatureModuleSet TidyModules; + if (ClangTidyProvider) { + TidyModules.add(std::make_unique( + [this](tidy::ClangTidyOptions &Opts, llvm::StringRef Filename) { + ClangTidyProvider(Opts, Filename); + })); + Inputs.FeatureModules = &TidyModules; + } StoreDiags Diags; auto CI = buildCompilerInvocation(Inputs, Diags); assert(CI && "Failed to build compilation invocation."); diff --git a/llvm/utils/gn/secondary/clang-tools-extra/clangd/BUILD.gn b/llvm/utils/gn/secondary/clang-tools-extra/clangd/BUILD.gn index f30aac6add17..430b86c61941 100644 --- a/llvm/utils/gn/secondary/clang-tools-extra/clangd/BUILD.gn +++ b/llvm/utils/gn/secondary/clang-tools-extra/clangd/BUILD.gn @@ -82,6 +82,7 @@ static_library("clangd") { "ASTSignals.cpp", "ClangdLSPServer.cpp", "ClangdServer.cpp", + "ClangTidyFeatureModule.cpp", "CodeComplete.cpp", "CodeCompletionStrings.cpp", "CollectMacros.cpp",