diff --git a/clang-tools-extra/clangd/CMakeLists.txt b/clang-tools-extra/clangd/CMakeLists.txt --- a/clang-tools-extra/clangd/CMakeLists.txt +++ b/clang-tools-extra/clangd/CMakeLists.txt @@ -81,6 +81,7 @@ SemanticSelection.cpp SourceCode.cpp QueryDriverDatabase.cpp + TidyProvider.cpp TUScheduler.cpp URI.cpp XRefs.cpp diff --git a/clang-tools-extra/clangd/ClangdServer.h b/clang-tools-extra/clangd/ClangdServer.h --- a/clang-tools-extra/clangd/ClangdServer.h +++ b/clang-tools-extra/clangd/ClangdServer.h @@ -41,13 +41,6 @@ namespace clang { namespace clangd { - -/// When set, used by ClangdServer to get clang-tidy options for each particular -/// file. Must be thread-safe. We use this instead of ClangTidyOptionsProvider -/// to allow reading tidy configs from the VFS used for parsing. -using ClangTidyOptionsBuilder = std::function; - /// Manages a collection of source files and derived data (ASTs, indexes), /// and provides language-aware features such as code completion. /// @@ -121,12 +114,9 @@ /// If set, queried to obtain the configuration to handle each request. config::Provider *ConfigProvider = nullptr; - /// If set, enable clang-tidy in clangd and use to it get clang-tidy - /// configurations for a particular file. - /// Clangd supports only a small subset of ClangTidyOptions, these options - /// (Checks, CheckOptions) are about which clang-tidy checks will be - /// enabled. - ClangTidyOptionsBuilder GetClangTidyOptions; + /// The Options provider to use when running clang-tidy. If null, clang-tidy + /// checks will be disabled. + TidyProviderRef ClangTidyProvider; /// If true, force -frecovery-ast flag. /// If false, respect the value in clang. @@ -377,7 +367,7 @@ std::vector> MergedIdx; // When set, provides clang-tidy options for a specific file. - ClangTidyOptionsBuilder GetClangTidyOptions; + TidyProviderRef ClangTidyProvider; // If this is true, suggest include insertion fixes for diagnostic errors that // can be caused by missing includes (e.g. member access in incomplete type). diff --git a/clang-tools-extra/clangd/ClangdServer.cpp b/clang-tools-extra/clangd/ClangdServer.cpp --- a/clang-tools-extra/clangd/ClangdServer.cpp +++ b/clang-tools-extra/clangd/ClangdServer.cpp @@ -114,40 +114,6 @@ bool TheiaSemanticHighlighting; }; -// Set of clang-tidy checks that don't work well in clangd, either due to -// crashes or false positives. -// Returns a clang-tidy filter string: -check1,-check2. -llvm::StringRef getUnusableTidyChecks() { - static const std::string FalsePositives = - llvm::join_items(", ", - // Check relies on seeing ifndef/define/endif directives, - // clangd doesn't replay those when using a preamble. - "-llvm-header-guard"); - static const std::string CrashingChecks = - llvm::join_items(", ", - // Check can choke on invalid (intermediate) c++ code, - // which is often the case when clangd tries to build an - // AST. - "-bugprone-use-after-move"); - static const std::string UnusableTidyChecks = - llvm::join_items(", ", FalsePositives, CrashingChecks); - return UnusableTidyChecks; -} - -// Returns a clang-tidy options string: check1,check2. -llvm::StringRef getDefaultTidyChecks() { - // These default checks are chosen for: - // - low false-positive rate - // - providing a lot of value - // - being reasonably efficient - static const std::string DefaultChecks = 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"); - return DefaultChecks; -} } // namespace ClangdServer::Options ClangdServer::optsForTest() { @@ -178,7 +144,7 @@ ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex, Opts.CollectMainFileRefs) : nullptr), - GetClangTidyOptions(Opts.GetClangTidyOptions), + ClangTidyProvider(Opts.ClangTidyProvider), SuggestMissingIncludes(Opts.SuggestMissingIncludes), BuildRecoveryAST(Opts.BuildRecoveryAST), PreserveRecoveryASTType(Opts.PreserveRecoveryASTType), @@ -236,20 +202,6 @@ llvm::StringRef Version, WantDiagnostics WantDiags, bool ForceRebuild) { ParseOptions Opts; - Opts.ClangTidyOpts = tidy::ClangTidyOptions::getDefaults(); - // FIXME: call tidy options builder on the worker thread, it can do IO. - if (GetClangTidyOptions) - Opts.ClangTidyOpts = - GetClangTidyOptions(*TFS.view(/*CWD=*/llvm::None), File); - if (Opts.ClangTidyOpts.Checks.hasValue()) { - // If the set of checks was configured, make sure clangd incompatible ones - // are disabled. - Opts.ClangTidyOpts.Checks = llvm::join_items( - ", ", *Opts.ClangTidyOpts.Checks, getUnusableTidyChecks()); - } else { - // Otherwise provide a nice set of defaults. - Opts.ClangTidyOpts.Checks = getDefaultTidyChecks().str(); - } Opts.SuggestMissingIncludes = SuggestMissingIncludes; // Compile command is set asynchronously during update, as it can be slow. @@ -260,6 +212,7 @@ Inputs.ForceRebuild = ForceRebuild; Inputs.Opts = std::move(Opts); Inputs.Index = Index; + Inputs.ClangTidyProvider = ClangTidyProvider; Inputs.Opts.BuildRecoveryAST = BuildRecoveryAST; Inputs.Opts.PreserveRecoveryASTType = PreserveRecoveryASTType; bool NewFile = WorkScheduler.update(File, Inputs, WantDiags); diff --git a/clang-tools-extra/clangd/Compiler.h b/clang-tools-extra/clangd/Compiler.h --- a/clang-tools-extra/clangd/Compiler.h +++ b/clang-tools-extra/clangd/Compiler.h @@ -15,8 +15,8 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H #define LLVM_CLANG_TOOLS_EXTRA_CLANGD_COMPILER_H -#include "../clang-tidy/ClangTidyOptions.h" #include "GlobalCompilationDatabase.h" +#include "TidyProvider.h" #include "index/Index.h" #include "support/ThreadsafeFS.h" #include "clang/Frontend/CompilerInstance.h" @@ -37,7 +37,6 @@ // Options to run clang e.g. when parsing AST. struct ParseOptions { - tidy::ClangTidyOptions ClangTidyOpts; bool SuggestMissingIncludes = false; bool BuildRecoveryAST = false; bool PreserveRecoveryASTType = false; @@ -56,6 +55,7 @@ // Used to recover from diagnostics (e.g. find missing includes for symbol). const SymbolIndex *Index = nullptr; ParseOptions Opts = ParseOptions(); + TidyProviderRef ClangTidyProvider = {}; }; /// Builds compiler invocation that could be used to build AST or preamble. diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h --- a/clang-tools-extra/clangd/Config.h +++ b/clang-tools-extra/clangd/Config.h @@ -26,6 +26,7 @@ #include "support/Context.h" #include "llvm/ADT/FunctionExtras.h" +#include "llvm/ADT/StringMap.h" #include #include @@ -70,6 +71,14 @@ // ::). All nested namespaces are affected as well. std::vector FullyQualifiedNamespaces; } Style; + + /// Configures what clang-tidy checks to run and options to use with them. + struct { + // A comma-seperated list of globs to specify which clang-tidy checks to + // run. + std::string Checks; + llvm::StringMap CheckOptions; + } ClangTidy; }; } // namespace clangd diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp --- a/clang-tools-extra/clangd/ConfigCompile.cpp +++ b/clang-tools-extra/clangd/ConfigCompile.cpp @@ -157,6 +157,7 @@ compile(std::move(F.If)); compile(std::move(F.CompileFlags)); compile(std::move(F.Index)); + compile(std::move(F.ClangTidy)); } void compile(Fragment::IfBlock &&F) { @@ -264,6 +265,49 @@ } } + void appendTidyCheckSpec(std::string &CurSpec, + const Located &Arg, bool IsPositive) { + StringRef Str = *Arg; + // Don't support negating here, its handled if the item is in the Add or + // Remove list. + if (Str.startswith("-") || Str.contains(',')) { + diag(Error, "Invalid clang-tidy check name", Arg.Range); + return; + } + CurSpec += ','; + if (!IsPositive) + CurSpec += '-'; + CurSpec += Str; + } + + void compile(Fragment::ClangTidyBlock &&F) { + std::string Checks; + for (auto &CheckGlob : F.Add) + appendTidyCheckSpec(Checks, CheckGlob, true); + + for (auto &CheckGlob : F.Remove) + appendTidyCheckSpec(Checks, CheckGlob, false); + + if (!Checks.empty()) + Out.Apply.push_back( + [Checks = std::move(Checks)](const Params &, Config &C) { + C.ClangTidy.Checks.append( + Checks, C.ClangTidy.Checks.empty() ? /*skip comma*/ 1 : 0); + }); + if (!F.CheckOptions.empty()) { + std::vector> CheckOptions; + for (auto &Opt : F.CheckOptions) + CheckOptions.emplace_back(std::move(*Opt.first), + std::move(*Opt.second)); + Out.Apply.push_back( + [CheckOptions = std::move(CheckOptions)](const Params &, Config &C) { + for (auto &StringPair : CheckOptions) + C.ClangTidy.CheckOptions.insert_or_assign(StringPair.first, + StringPair.second); + }); + } + } + constexpr static llvm::SourceMgr::DiagKind Error = llvm::SourceMgr::DK_Error; constexpr static llvm::SourceMgr::DiagKind Warning = llvm::SourceMgr::DK_Warning; diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h --- a/clang-tools-extra/clangd/ConfigFragment.h +++ b/clang-tools-extra/clangd/ConfigFragment.h @@ -174,6 +174,29 @@ std::vector> FullyQualifiedNamespaces; }; StyleBlock Style; + + /// Controls how clang-tidy will run over the code base. + /// + /// The settings are merged with any settings found in .clang-tidy + /// configiration files with these ones taking precedence. + struct ClangTidyBlock { + std::vector> Add; + /// List of checks to disable. + /// Takes precedence over Add. To enable all llvm checks except include + /// order: + /// Add: llvm-* + /// Remove: llvm-include-onder + std::vector> Remove; + + /// A Key-Value pair list of options to pass to clang-tidy checks + /// These take precedence over options specified in clang-tidy configuration + /// files. Example: + /// CheckOptions: + /// readability-braces-around-statements.ShortStatementLines: 2 + std::vector, Located>> + CheckOptions; + }; + ClangTidyBlock ClangTidy; }; } // namespace config diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp --- a/clang-tools-extra/clangd/ConfigYAML.cpp +++ b/clang-tools-extra/clangd/ConfigYAML.cpp @@ -40,6 +40,7 @@ Dict.handle("CompileFlags", [&](Node &N) { parse(F.CompileFlags, N); }); Dict.handle("Index", [&](Node &N) { parse(F.Index, N); }); Dict.handle("Style", [&](Node &N) { parse(F.Style, N); }); + Dict.handle("ClangTidy", [&](Node &N) { parse(F.ClangTidy, N); }); Dict.parse(N); return !(N.failed() || HadError); } @@ -47,8 +48,10 @@ private: void parse(Fragment::IfBlock &F, Node &N) { DictParser Dict("If", this); - Dict.unrecognized( - [&](llvm::StringRef) { F.HasUnrecognizedCondition = true; }); + Dict.unrecognized([&](Located, Node &) { + F.HasUnrecognizedCondition = true; + return true; // Emit a warning for the unrecognized key. + }); Dict.handle("PathMatch", [&](Node &N) { if (auto Values = scalarValues(N)) F.PathMatch = std::move(*Values); @@ -82,6 +85,28 @@ Dict.parse(N); } + void parse(Fragment::ClangTidyBlock &F, Node &N) { + DictParser Dict("ClangTidy", this); + Dict.handle("Add", [&](Node &N) { + if (auto Values = scalarValues(N)) + F.Add = std::move(*Values); + }); + Dict.handle("Remove", [&](Node &N) { + if (auto Values = scalarValues(N)) + F.Remove = std::move(*Values); + }); + Dict.handle("CheckOptions", [&](Node &N) { + DictParser CheckOptDict("CheckOptions", this); + CheckOptDict.unrecognized([&](Located &&Key, Node &Val) { + if (auto Value = scalarValue(Val, *Key)) + F.CheckOptions.emplace_back(std::move(Key), std::move(*Value)); + return false; // Don't emit a warning + }); + CheckOptDict.parse(N); + }); + Dict.parse(N); + } + void parse(Fragment::IndexBlock &F, Node &N) { DictParser Dict("Index", this); Dict.handle("Background", @@ -94,7 +119,7 @@ class DictParser { llvm::StringRef Description; std::vector>> Keys; - std::function Unknown; + std::function, Node &)> UnknownHandler; Parser *Outer; public: @@ -112,10 +137,12 @@ Keys.emplace_back(Key, std::move(Parse)); } - // Fallback is called when a Key is not matched by any handle(). - // A warning is also automatically emitted. - void unrecognized(std::function Fallback) { - Unknown = std::move(Fallback); + // Handler is called when a Key is not matched by any handle(). + // If this is unset or the Handler returns true, a warning is emitted for + // the unknown key. + void + unrecognized(std::function, Node &)> Handler) { + UnknownHandler = std::move(Handler); } // Process a mapping node and call handlers for each key/value pair. @@ -135,6 +162,8 @@ continue; if (!Seen.insert(**Key).second) { Outer->warning("Duplicate key " + **Key + " is ignored", *K); + if (auto *Value = KV.getValue()) + Value->skip(); continue; } auto *Value = KV.getValue(); @@ -149,9 +178,12 @@ } } if (!Matched) { - Outer->warning("Unknown " + Description + " key " + **Key, *K); - if (Unknown) - Unknown(**Key); + bool Warn = !UnknownHandler; + if (UnknownHandler) + Warn = UnknownHandler( + Located(**Key, K->getSourceRange()), *Value); + if (Warn) + Outer->warning("Unknown " + Description + " key " + **Key, *K); } } } diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -17,6 +17,7 @@ #include "IncludeFixer.h" #include "Preamble.h" #include "SourceCode.h" +#include "TidyProvider.h" #include "index/CanonicalIncludes.h" #include "index/Index.h" #include "support/Logger.h" @@ -236,6 +237,10 @@ } // namespace +/// Empty clang tidy provider, using this as a provider will disable clang-tidy. +static void emptyTidyProvider(tidy::ClangTidyOptions &, llvm::StringRef, + unsigned &) {} + llvm::Optional ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, std::unique_ptr CI, @@ -292,16 +297,17 @@ llvm::Optional CTContext; { trace::Span Tracer("ClangTidyInit"); - dlog("ClangTidy configuration for file {0}: {1}", Filename, - tidy::configurationAsText(Inputs.Opts.ClangTidyOpts)); tidy::ClangTidyCheckFactories CTFactories; for (const auto &E : tidy::ClangTidyModuleRegistry::entries()) E.instantiate()->addCheckFactories(CTFactories); - CTContext.emplace(std::make_unique( - tidy::ClangTidyGlobalOptions(), Inputs.Opts.ClangTidyOpts)); + CTContext.emplace(asClangTidyOptionsProvider(Inputs.ClangTidyProvider + ? Inputs.ClangTidyProvider + : emptyTidyProvider)); CTContext->setDiagnosticsEngine(&Clang->getDiagnostics()); CTContext->setASTContext(&Clang->getASTContext()); CTContext->setCurrentFile(Filename); + dlog("ClangTidy configuration for file {0}: {1}", Filename, + tidy::configurationAsText(CTContext->getOptions())); CTChecks = CTFactories.createChecks(CTContext.getPointer()); llvm::erase_if(CTChecks, [&](const auto &Check) { return !Check->isLanguageVersionSupported(CTContext->getLangOpts()); diff --git a/clang-tools-extra/clangd/TidyProvider.h b/clang-tools-extra/clangd/TidyProvider.h new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clangd/TidyProvider.h @@ -0,0 +1,59 @@ +//===--- TidyProvider.h - create options for running clang-tidy------------===// +// +// 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_TIDYPROVIDER_H +#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_TIDYPROVIDER_H + +#include "../clang-tidy/ClangTidyOptions.h" +#include "support/ThreadsafeFS.h" +#include "llvm/ADT/FunctionExtras.h" +#include "llvm/ADT/STLExtras.h" +#include "llvm/ADT/StringRef.h" + +namespace clang { +namespace clangd { + +namespace detail { +using TidyProviderInterface = void(tidy::ClangTidyOptions &, + /*Filename=*/llvm::StringRef, + /*Priority=*/unsigned &); +} // namespace detail + +using TidyProvider = llvm::unique_function; +using TidyProviderRef = llvm::function_ref; + +TidyProvider combine(std::vector Providers); +std::unique_ptr + asClangTidyOptionsProvider(TidyProviderRef); + +/// Provider that just sets the defaults. +TidyProvider provideEnvironment(); + +/// Provider that will enable a nice set of default checks if none are +/// specified. +TidyProvider provideDefaultChecks(); + +/// Provider the enables a specific set of checks and warnings as errors. +TidyProvider fixedTidyProvider(llvm::StringRef Checks, + llvm::StringRef WarningsAsErrors = {}); + +/// Provider that will disable checks known to not work with clangd. +TidyProvider +disableUnusableChecks(std::vector ChecksToDisable = {}); + +/// Provider that searches for .clang-tidy configuration files in the directory +/// tree. +TidyProvider provideClangTidyFiles(ThreadsafeFS &); + +// Provider that uses clangd configuration files. +TidyProvider provideClangdConfig(); + +} // namespace clangd +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TIDYPROVIDER_H diff --git a/clang-tools-extra/clangd/TidyProvider.cpp b/clang-tools-extra/clangd/TidyProvider.cpp new file mode 100644 --- /dev/null +++ b/clang-tools-extra/clangd/TidyProvider.cpp @@ -0,0 +1,215 @@ +//===--- TidyProvider.cpp - create options for running clang-tidy----------===// +// +// 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 "TidyProvider.h" +#include "Config.h" +#include "llvm/ADT/SmallString.h" +#include "llvm/ADT/StringExtras.h" +#include "llvm/Support/Process.h" +#include "llvm/Support/VirtualFileSystem.h" +#include + +namespace clang { +namespace clangd { + +static void mergeCheckList(llvm::Optional &Checks, + llvm::StringRef List) { + if (List.empty()) + return; + if (!Checks || Checks->empty()) { + Checks.emplace(List); + return; + } + *Checks = llvm::join_items(",", *Checks, List); +} + +static llvm::Optional +tryReadConfigFile(llvm::vfs::FileSystem *FS, llvm::StringRef Directory) { + assert(!Directory.empty()); + + llvm::ErrorOr DirectoryStatus = FS->status(Directory); + + if (!DirectoryStatus || !DirectoryStatus->isDirectory()) { + llvm::errs() << "Error reading configuration from " << Directory + << ": directory doesn't exist.\n"; + return llvm::None; + } + + llvm::SmallString<128> ConfigFile(Directory); + llvm::sys::path::append(ConfigFile, ".clang-tidy"); + + llvm::ErrorOr FileStatus = FS->status(ConfigFile); + + if (!FileStatus || !FileStatus->isRegularFile()) + return llvm::None; + + llvm::ErrorOr> Text = + FS->getBufferForFile(ConfigFile); + if (std::error_code EC = Text.getError()) { + llvm::errs() << "Can't read " << ConfigFile << ": " << EC.message() << "\n"; + return llvm::None; + } + + // Skip empty files, e.g. files opened for writing via shell output + // redirection. + if ((*Text)->getBuffer().empty()) + return llvm::None; + llvm::ErrorOr ParsedOptions = + tidy::parseConfiguration((*Text)->getBuffer()); + if (!ParsedOptions) { + if (ParsedOptions.getError()) + llvm::errs() << "Error parsing " << ConfigFile << ": " + << ParsedOptions.getError().message() << "\n"; + return llvm::None; + } + return std::move(*ParsedOptions); +} + +TidyProvider provideEnvironment() { + return [](tidy::ClangTidyOptions &Opts, llvm::StringRef, unsigned &Order) { + auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults(); + EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set. + EmptyDefaults.User = llvm::sys::Process::GetEnv("USER"); +#ifdef _WIN32 + if (!EmptyDefaults.User) + EmptyDefaults.User = llvm::sys::Process::GetEnv("USERNAME"); +#endif + Opts.mergeWith(EmptyDefaults, Order); + }; +} + +TidyProvider provideDefaultChecks() { + return [](tidy::ClangTidyOptions &Opts, llvm::StringRef, unsigned &Order) { + if (!Opts.Checks || Opts.Checks->empty()) { + // These default checks are chosen for: + // - low false-positive rate + // - providing a lot of value + // - being reasonably efficient + static const std::string DefaultChecks = 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"); + Opts.Checks->assign(DefaultChecks); + } + }; +} + +TidyProvider fixedTidyProvider(llvm::StringRef Checks, + llvm::StringRef WarningsAsErrors) { + return [Checks = std::string(Checks), + WarningsAsErrors = std::string(WarningsAsErrors)]( + tidy::ClangTidyOptions &Opts, llvm::StringRef, unsigned &Order) { + mergeCheckList(Opts.Checks, Checks); + mergeCheckList(Opts.WarningsAsErrors, WarningsAsErrors); + }; +} + +TidyProvider disableUnusableChecks(std::vector ChecksToDisable) { + return [ChecksToDisable = std::move(ChecksToDisable)]( + tidy::ClangTidyOptions &Opts, llvm::StringRef, unsigned &Order) { + if (Opts.Checks && !Opts.Checks->empty()) { + static const std::string FalsePositives = llvm::join_items( + ", ", + // Check relies on seeing ifndef/define/endif directives, + // clangd doesn't replay those when using a preamble. + "-llvm-header-guard"); + static const std::string CrashingChecks = + llvm::join_items(", ", + // Check can choke on invalid (intermediate) c++ + // code, which is often the case when clangd tries to + // build an AST. + "-bugprone-use-after-move"); + Opts.Checks = + llvm::join_items(",", *Opts.Checks, FalsePositives, CrashingChecks, + llvm::join(ChecksToDisable, ",")); + } + }; +} + +TidyProvider provideClangdConfig() { + return [](tidy::ClangTidyOptions &Opts, llvm::StringRef, unsigned &Order) { + const auto &CurTidyConfig = Config::current().ClangTidy; + if (!CurTidyConfig.Checks.empty()) + mergeCheckList(Opts.Checks, CurTidyConfig.Checks); + + for (const auto &CheckOption : CurTidyConfig.CheckOptions) + Opts.CheckOptions.insert_or_assign(CheckOption.getKey(), + tidy::ClangTidyOptions::ClangTidyValue( + CheckOption.getValue(), Order)); + }; +} + +TidyProvider provideClangTidyFiles(ThreadsafeFS &TFS) { + return [&](tidy::ClangTidyOptions &Opts, llvm::StringRef Filename, + unsigned &Order) { + llvm::SmallVector OptionStack; + auto FS(TFS.view(llvm::None)); + llvm::SmallString<128> AbsolutePath(Filename); + + if (FS->makeAbsolute(AbsolutePath)) + return; + + llvm::StringRef Path = llvm::sys::path::parent_path(AbsolutePath); + for (llvm::StringRef CurrentPath = Path; !CurrentPath.empty(); + CurrentPath = llvm::sys::path::parent_path(CurrentPath)) { + auto ConfigFile = tryReadConfigFile(FS.get(), CurrentPath); + if (!ConfigFile) + continue; + OptionStack.push_back(std::move(*ConfigFile)); + if (!OptionStack.back().InheritParentConfig.getValueOr(false)) + break; + } + for (auto &Option : llvm::reverse(OptionStack)) + Opts.mergeWith(Option, ++Order); + }; +} + +TidyProvider combine(std::vector Providers) { + return [Providers = std::move(Providers)](tidy::ClangTidyOptions &Opts, + llvm::StringRef Filename, + unsigned &Order) mutable { + for (auto &Provider : Providers) { + Provider(Opts, Filename, Order); + ++Order; + } + }; +} + +namespace { + +class TidyProviderInstance : public tidy::ClangTidyOptionsProvider { +public: + TidyProviderInstance(TidyProviderRef Provider) : Provider(Provider) {} + + const tidy::ClangTidyGlobalOptions &getGlobalOptions() override { + return Defaults; + } + std::vector getRawOptions(llvm::StringRef FileName) override { + std::vector Opts; + Opts.emplace_back(); + unsigned Priority = 0; + Provider(Opts.back().first, FileName, Priority); + return Opts; + } + +private: + TidyProviderRef Provider; + static const tidy::ClangTidyGlobalOptions Defaults; +}; +const tidy::ClangTidyGlobalOptions TidyProviderInstance::Defaults; +} // namespace + +std::unique_ptr +asClangTidyOptionsProvider(TidyProviderRef Provider) { + return std::make_unique(Provider); +} +} // namespace clangd +} // namespace clang diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp --- a/clang-tools-extra/clangd/tool/Check.cpp +++ b/clang-tools-extra/clangd/tool/Check.cpp @@ -133,8 +133,6 @@ return false; } } - Inputs.Opts.ClangTidyOpts = - Opts.GetClangTidyOptions(*TFS.view(llvm::None), File); log("Parsing command..."); Invocation = buildCompilerInvocation(Inputs, CaptureInvocationDiags, &CC1Args); diff --git a/clang-tools-extra/clangd/tool/ClangdMain.cpp b/clang-tools-extra/clangd/tool/ClangdMain.cpp --- a/clang-tools-extra/clangd/tool/ClangdMain.cpp +++ b/clang-tools-extra/clangd/tool/ClangdMain.cpp @@ -11,6 +11,7 @@ #include "Features.inc" #include "PathMapping.h" #include "Protocol.h" +#include "TidyProvider.h" #include "Transport.h" #include "index/Background.h" #include "index/Serialization.h" @@ -158,6 +159,10 @@ init(""), }; +list DisabledClangTidyChecks{ + "disable-clang-tidy-checks", cat(Features), + desc("List of clang-tidy checks to disable"), Hidden, CommaSeparated}; + opt CodeCompletionParse{ "completion-parse", cat(Features), @@ -803,35 +808,21 @@ } // Create an empty clang-tidy option. - std::mutex ClangTidyOptMu; - std::unique_ptr - ClangTidyOptProvider; /*GUARDED_BY(ClangTidyOptMu)*/ + TidyProvider ClangTidyOptProvider; if (EnableClangTidy) { - auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults(); - EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set. - EmptyDefaults.User = llvm::sys::Process::GetEnv("USER"); -#ifdef _WIN32 - if (!EmptyDefaults.User) - EmptyDefaults.User = llvm::sys::Process::GetEnv("USERNAME"); -#endif - tidy::ClangTidyOptions OverrideClangTidyOptions; - if (!ClangTidyChecks.empty()) - OverrideClangTidyOptions.Checks = ClangTidyChecks; - ClangTidyOptProvider = std::make_unique( - tidy::ClangTidyGlobalOptions(), - /* Default */ EmptyDefaults, - /* Override */ OverrideClangTidyOptions, TFS.view(/*CWD=*/llvm::None)); - Opts.GetClangTidyOptions = [&](llvm::vfs::FileSystem &, - llvm::StringRef File) { - // This function must be thread-safe and tidy option providers are not. - tidy::ClangTidyOptions Opts; - { - std::lock_guard Lock(ClangTidyOptMu); - // FIXME: use the FS provided to the function. - Opts = ClangTidyOptProvider->getOptions(File); - } - return Opts; - }; + std::vector Providers; + Providers.reserve(4 + EnableConfig); + Providers.push_back(provideEnvironment()); + Providers.push_back(provideClangTidyFiles(TFS)); + if (EnableConfig) + Providers.push_back(provideClangdConfig()); + if (ClangTidyChecks.empty()) + Providers.push_back(fixedTidyProvider(ClangTidyChecks)); + else + Providers.push_back(provideDefaultChecks()); + Providers.push_back(disableUnusableChecks(DisabledClangTidyChecks)); + ClangTidyOptProvider = combine(std::move(Providers)); + Opts.ClangTidyProvider = ClangTidyOptProvider; } Opts.AsyncPreambleBuilds = AsyncPreamble; Opts.SuggestMissingIncludes = SuggestMissingIncludes; diff --git a/clang-tools-extra/clangd/unittests/ClangdTests.cpp b/clang-tools-extra/clangd/unittests/ClangdTests.cpp --- a/clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ b/clang-tools-extra/clangd/unittests/ClangdTests.cpp @@ -16,6 +16,7 @@ #include "SyncAPI.h" #include "TestFS.h" #include "TestTU.h" +#include "TidyProvider.h" #include "URI.h" #include "support/MemoryTree.h" #include "support/Path.h" @@ -1214,17 +1215,25 @@ bool HadDiagsInLastCallback = false; } DiagConsumer; + // We need to use the actual options provider here as we're interested in + // making sure those providers correctly disable problematic checks. + // As an added bonus it demonstrates the provider is capable of handling the + // ThreadsafeFS. + MockFS FS; + FS.Files[testPath(".clang-tidy")] = R"( + Checks: -*,bugprone-use-after-move,llvm-header-guard + )"; MockCompilationDatabase CDB; + std::vector Stack; + Stack.push_back(provideEnvironment()); + Stack.push_back(provideClangTidyFiles(FS)); + Stack.push_back(provideDefaultChecks()); + Stack.push_back(disableUnusableChecks()); + TidyProvider Provider = combine(std::move(Stack)); CDB.ExtraClangFlags = {"-xc++"}; auto Opts = ClangdServer::optsForTest(); - Opts.GetClangTidyOptions = [](llvm::vfs::FileSystem &, llvm::StringRef) { - auto Opts = tidy::ClangTidyOptions::getDefaults(); - // These checks don't work well in clangd, even if configured they shouldn't - // run. - Opts.Checks = "bugprone-use-after-move,llvm-header-guard"; - return Opts; - }; + Opts.ClangTidyProvider = Provider; 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/ConfigCompileTests.cpp b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp --- a/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigCompileTests.cpp @@ -176,6 +176,26 @@ ASSERT_THAT(Diags.Diagnostics, IsEmpty()); } } + +TEST_F(ConfigCompileTests, Tidy) { + Frag.ClangTidy.Add.emplace_back("bugprone-use-after-move"); + Frag.ClangTidy.Add.emplace_back("llvm-*"); + Frag.ClangTidy.Remove.emplace_back("llvm-include-order"); + Frag.ClangTidy.Remove.emplace_back("readability-*"); + Frag.ClangTidy.CheckOptions.emplace_back( + std::make_pair(std::string("StrictMode"), std::string("true"))); + Frag.ClangTidy.CheckOptions.emplace_back(std::make_pair( + std::string("example-check.ExampleOption"), std::string("0"))); + EXPECT_TRUE(compileAndApply()); + EXPECT_EQ( + Conf.ClangTidy.Checks, + "bugprone-use-after-move,llvm-*,-llvm-include-order,-readability-*"); + EXPECT_EQ(Conf.ClangTidy.CheckOptions.size(), 2U); + EXPECT_EQ(Conf.ClangTidy.CheckOptions.lookup("StrictMode"), "true"); + EXPECT_EQ(Conf.ClangTidy.CheckOptions.lookup("example-check.ExampleOption"), + "0"); +} + } // namespace } // namespace config } // namespace clangd diff --git a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp --- a/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp +++ b/clang-tools-extra/clangd/unittests/ConfigYAMLTests.cpp @@ -35,6 +35,14 @@ return false; } +MATCHER_P2(PairVal, Value1, Value2, "") { + if (*arg.first == Value1 && *arg.second == Value2) + return true; + *result_listener << "values are [" << *arg.first << ", " << *arg.second + << "]"; + return false; +} + TEST(ParseYAML, SyntacticForms) { CapturedDiags Diags; const char *YAML = R"yaml( @@ -50,10 +58,15 @@ --- Index: Background: Skip +--- +ClangTidy: + CheckOptions: + IgnoreMacros: true + example-check.ExampleOption: 0 )yaml"; auto Results = Fragment::parseYAML(YAML, "config.yaml", Diags.callback()); EXPECT_THAT(Diags.Diagnostics, IsEmpty()); - ASSERT_EQ(Results.size(), 3u); + ASSERT_EQ(Results.size(), 4u); EXPECT_FALSE(Results[0].If.HasUnrecognizedCondition); EXPECT_THAT(Results[0].If.PathMatch, ElementsAre(Val("abc"))); EXPECT_THAT(Results[0].CompileFlags.Add, ElementsAre(Val("foo"), Val("bar"))); @@ -62,6 +75,9 @@ ASSERT_TRUE(Results[2].Index.Background); EXPECT_EQ("Skip", *Results[2].Index.Background.getValue()); + EXPECT_THAT(Results[3].ClangTidy.CheckOptions, + ElementsAre(PairVal("IgnoreMacros", "true"), + PairVal("example-check.ExampleOption", "0"))); } TEST(ParseYAML, Locations) { @@ -84,11 +100,11 @@ CapturedDiags Diags; Annotations YAML(R"yaml( If: - [[UnknownCondition]]: "foo" + $unknown[[UnknownCondition]]: "foo" CompileFlags: Add: 'first' --- -CompileFlags: {^ +CompileFlags: {$unexpected^ )yaml"); auto Results = Fragment::parseYAML(YAML.code(), "config.yaml", Diags.callback()); @@ -97,11 +113,13 @@ Diags.Diagnostics, ElementsAre(AllOf(DiagMessage("Unknown If key UnknownCondition"), DiagKind(llvm::SourceMgr::DK_Warning), - DiagPos(YAML.range().start), DiagRange(YAML.range())), + DiagPos(YAML.range("unknown").start), + DiagRange(YAML.range("unknown"))), AllOf(DiagMessage("Unexpected token. Expected Key, Flow " "Entry, or Flow Mapping End."), DiagKind(llvm::SourceMgr::DK_Error), - DiagPos(YAML.point()), DiagRange(llvm::None)))); + DiagPos(YAML.point("unexpected")), + DiagRange(llvm::None)))); ASSERT_EQ(Results.size(), 1u); // invalid fragment discarded. EXPECT_THAT(Results.front().CompileFlags.Add, ElementsAre(Val("first"))); diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp --- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -14,6 +14,7 @@ #include "TestFS.h" #include "TestIndex.h" #include "TestTU.h" +#include "TidyProvider.h" #include "index/MemIndex.h" #include "support/Path.h" #include "clang/Basic/Diagnostic.h" @@ -129,7 +130,7 @@ } )cpp"); auto TU = TestTU::withCode(Test.code()); - TU.ClangTidyChecks = "-*,google-explicit-constructor"; + TU.ClangTidyProvider = fixedTidyProvider("-*,google-explicit-constructor"); EXPECT_THAT( TU.build().getDiagnostics(), ElementsAre( @@ -201,8 +202,9 @@ auto TU = TestTU::withCode(Test.code()); // Enable alias clang-tidy checks, these check emits the same diagnostics // (except the check name). - TU.ClangTidyChecks = "-*, readability-uppercase-literal-suffix, " - "hicpp-uppercase-literal-suffix"; + TU.ClangTidyProvider = + fixedTidyProvider("-*, readability-uppercase-literal-suffix, " + "hicpp-uppercase-literal-suffix"); // Verify that we filter out the duplicated diagnostic message. EXPECT_THAT( TU.build().getDiagnostics(), @@ -245,9 +247,9 @@ )cpp"); auto TU = TestTU::withCode(Test.code()); TU.HeaderFilename = "assert.h"; // Suppress "not found" error. - TU.ClangTidyChecks = + TU.ClangTidyProvider = fixedTidyProvider( "-*, bugprone-sizeof-expression, bugprone-macro-repeated-side-effects, " - "modernize-deprecated-headers, modernize-use-trailing-return-type"; + "modernize-deprecated-headers, modernize-use-trailing-return-type"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre( @@ -289,7 +291,7 @@ auto TU = TestTU::withCode(Test.code()); TU.ExtraArgs = {"-isystem."}; TU.AdditionalFiles["a.h"] = TU.AdditionalFiles["b.h"] = ""; - TU.ClangTidyChecks = "-*, llvm-include-order"; + TU.ClangTidyProvider = fixedTidyProvider("-*, llvm-include-order"); EXPECT_THAT( TU.build().getDiagnostics(), Contains(AllOf(Diag(Test.range(), "#includes are not sorted properly"), @@ -361,7 +363,7 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "modernize-loop-convert"; + TU.ClangTidyProvider = fixedTidyProvider("modernize-loop-convert"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre(::testing::AllOf( @@ -384,7 +386,7 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "bugprone-integer-division"; + TU.ClangTidyProvider = fixedTidyProvider("bugprone-integer-division"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre(::testing::AllOf( @@ -401,8 +403,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "bugprone-integer-division"; - TU.ClangTidyWarningsAsErrors = "bugprone-integer-division"; + TU.ClangTidyProvider = fixedTidyProvider("bugprone-integer-division", + "bugprone-integer-division"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre(::testing::AllOf( @@ -452,8 +454,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "bugprone-integer-division"; - TU.ClangTidyWarningsAsErrors = "bugprone-integer-division"; + TU.ClangTidyProvider = fixedTidyProvider("bugprone-integer-division", + "bugprone-integer-division"); EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre()); } @@ -468,7 +470,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "-*,bugprone-bad-signal-to-kill-thread"; + TU.ClangTidyProvider = + fixedTidyProvider("-*,bugprone-bad-signal-to-kill-thread"); EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre()); // no-crash } diff --git a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp --- a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp +++ b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp @@ -24,6 +24,7 @@ #include "SourceCode.h" #include "TestFS.h" #include "TestTU.h" +#include "TidyProvider.h" #include "clang/AST/DeclTemplate.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" @@ -250,7 +251,8 @@ TestTU TU; // this check runs the preprocessor, we need to make sure it does not break // our recording logic. - TU.ClangTidyChecks = "modernize-use-trailing-return-type"; + TU.ClangTidyProvider = + fixedTidyProvider("modernize-use-trailing-return-type"); TU.Code = "inline int foo() {}"; auto AST = TU.build(); @@ -406,7 +408,7 @@ "replay-preamble-module", ""); TestTU TU; // This check records inclusion directives replayed by clangd. - TU.ClangTidyChecks = "replay-preamble-check"; + TU.ClangTidyProvider = fixedTidyProvider("replay-preamble-check"); llvm::Annotations Test(R"cpp( $hash^#$include[[import]] $filebegin^"$filerange[[bar.h]]" $hash^#$include[[include_next]] $filebegin^"$filerange[[baz.h]]" diff --git a/clang-tools-extra/clangd/unittests/TestTU.h b/clang-tools-extra/clangd/unittests/TestTU.h --- a/clang-tools-extra/clangd/unittests/TestTU.h +++ b/clang-tools-extra/clangd/unittests/TestTU.h @@ -17,6 +17,7 @@ #ifndef LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H #define LLVM_CLANG_TOOLS_EXTRA_UNITTESTS_CLANGD_TESTTU_H +#include "../TidyProvider.h" #include "Compiler.h" #include "ParsedAST.h" #include "TestFS.h" @@ -58,8 +59,7 @@ // Extra arguments for the compiler invocation. std::vector ExtraArgs; - llvm::Optional ClangTidyChecks; - llvm::Optional ClangTidyWarningsAsErrors; + mutable TidyProvider ClangTidyProvider = {}; // Index to use when building AST. const SymbolIndex *ExternalIndex = nullptr; diff --git a/clang-tools-extra/clangd/unittests/TestTU.cpp b/clang-tools-extra/clangd/unittests/TestTU.cpp --- a/clang-tools-extra/clangd/unittests/TestTU.cpp +++ b/clang-tools-extra/clangd/unittests/TestTU.cpp @@ -59,8 +59,8 @@ FS.OverlayRealFileSystemForModules = true; Inputs.TFS = &FS; Inputs.Opts = ParseOptions(); - Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks; - Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors; + if (ClangTidyProvider) + Inputs.ClangTidyProvider = ClangTidyProvider; Inputs.Index = ExternalIndex; if (Inputs.Index) Inputs.Opts.SuggestMissingIncludes = true;