njames93 updated this revision to Diff 303720. njames93 added a comment. Fix potential race when updating cache in `FileTidyProvider`.
Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D91029/new/ https://reviews.llvm.org/D91029 Files: clang-tools-extra/clangd/CMakeLists.txt clang-tools-extra/clangd/ClangdServer.cpp clang-tools-extra/clangd/ClangdServer.h clang-tools-extra/clangd/Compiler.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/TidyProvider.cpp clang-tools-extra/clangd/TidyProvider.h clang-tools-extra/clangd/tool/Check.cpp clang-tools-extra/clangd/tool/ClangdMain.cpp clang-tools-extra/clangd/unittests/CMakeLists.txt clang-tools-extra/clangd/unittests/ClangdTests.cpp clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp clang-tools-extra/clangd/unittests/ParsedASTTests.cpp clang-tools-extra/clangd/unittests/TestTU.cpp clang-tools-extra/clangd/unittests/TestTU.h clang-tools-extra/clangd/unittests/TestTidyProvider.cpp clang-tools-extra/clangd/unittests/TestTidyProvider.h
Index: clang-tools-extra/clangd/unittests/TestTidyProvider.h =================================================================== --- /dev/null +++ clang-tools-extra/clangd/unittests/TestTidyProvider.h @@ -0,0 +1,34 @@ +//===-- TestTidyProvider.h --------------------------------------*- 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 +// +//===----------------------------------------------------------------------===// + +#include "../TidyProvider.h" +#include "llvm/Support/VirtualFileSystem.h" + +namespace clang { +namespace clangd { + +class TestClangTidyProvider : public ClangdTidyProvider { +public: + /// Convienence method for creating a provider that just uses \p Checks and \p + /// WarningsAsErrors + TestClangTidyProvider(llvm::StringRef Checks, + llvm::StringRef WarningsAsErrors = {}); + TestClangTidyProvider(const tidy::ClangTidyGlobalOptions &GlobalOptions, + const tidy::ClangTidyOptions &Options); + std::vector<OptionsSource> getRawOptions(llvm::vfs::FileSystem * /*unused*/, + llvm::StringRef FileName) override; + const tidy::ClangTidyGlobalOptions & + getGlobalOptions(llvm::vfs::FileSystem * /*unused*/) override; + +private: + tidy::ClangTidyGlobalOptions GlobalOpts; + tidy::ClangTidyOptions Opts; +}; + +} // namespace clangd +} // namespace clang Index: clang-tools-extra/clangd/unittests/TestTidyProvider.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clangd/unittests/TestTidyProvider.cpp @@ -0,0 +1,50 @@ +//===-- TestTidyProvider.cpp ------------------------------------*- 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 +// +//===----------------------------------------------------------------------===// + +#include "TestTidyProvider.h" + +namespace clang { + +using namespace tidy; + +namespace clangd { + +TestClangTidyProvider::TestClangTidyProvider(llvm::StringRef Checks, + llvm::StringRef WarningsAsErrors) { + if (Checks.empty()) + Opts.Checks.emplace(getDefaultTidyChecks()); + else + Opts.Checks.emplace((Checks + "," + getUnusableTidyChecks()).str()); + if (!WarningsAsErrors.empty()) + Opts.WarningsAsErrors.emplace(WarningsAsErrors); +} + +TestClangTidyProvider::TestClangTidyProvider( + const ClangTidyGlobalOptions &GlobalOptions, + const ClangTidyOptions &Options) + : GlobalOpts(GlobalOptions), Opts(Options) { + if (!Opts.Checks.hasValue() || Opts.Checks->empty()) + Opts.Checks = getDefaultTidyChecks().str(); + else + Opts.Checks->append(("," + getUnusableTidyChecks().str())); +} + +std::vector<ClangTidyOptionsProvider::OptionsSource> +TestClangTidyProvider::getRawOptions(llvm::vfs::FileSystem * /*unused*/, + llvm::StringRef FileName) { + std::vector<OptionsSource> RawOpts; + RawOpts.emplace_back(Opts, "mock-options"); + return RawOpts; +} + +const ClangTidyGlobalOptions & +TestClangTidyProvider::getGlobalOptions(llvm::vfs::FileSystem * /*unused*/) { + return GlobalOpts; +} +} // namespace clangd +} // namespace clang Index: clang-tools-extra/clangd/unittests/TestTU.h =================================================================== --- clang-tools-extra/clangd/unittests/TestTU.h +++ 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<std::string> ExtraArgs; - llvm::Optional<std::string> ClangTidyChecks; - llvm::Optional<std::string> ClangTidyWarningsAsErrors; + std::unique_ptr<ClangdTidyProvider> ClangTidyProvider; // Index to use when building AST. const SymbolIndex *ExternalIndex = nullptr; Index: clang-tools-extra/clangd/unittests/TestTU.cpp =================================================================== --- clang-tools-extra/clangd/unittests/TestTU.cpp +++ clang-tools-extra/clangd/unittests/TestTU.cpp @@ -59,8 +59,7 @@ FS.OverlayRealFileSystemForModules = true; Inputs.TFS = &FS; Inputs.Opts = ParseOptions(); - Inputs.Opts.ClangTidyOpts.Checks = ClangTidyChecks; - Inputs.Opts.ClangTidyOpts.WarningsAsErrors = ClangTidyWarningsAsErrors; + Inputs.ClangTidyProvider = ClangTidyProvider.get(); Inputs.Index = ExternalIndex; if (Inputs.Index) Inputs.Opts.SuggestMissingIncludes = true; Index: clang-tools-extra/clangd/unittests/ParsedASTTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ParsedASTTests.cpp +++ clang-tools-extra/clangd/unittests/ParsedASTTests.cpp @@ -24,6 +24,7 @@ #include "SourceCode.h" #include "TestFS.h" #include "TestTU.h" +#include "TestTidyProvider.h" #include "clang/AST/DeclTemplate.h" #include "clang/Basic/SourceLocation.h" #include "clang/Basic/SourceManager.h" @@ -37,6 +38,7 @@ #include "gmock/gmock-matchers.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include <memory> namespace clang { namespace clangd { @@ -250,7 +252,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 = std::make_unique<TestClangTidyProvider>( + "modernize-use-trailing-return-type"); TU.Code = "inline int foo() {}"; auto AST = TU.build(); @@ -406,7 +409,8 @@ "replay-preamble-module", ""); TestTU TU; // This check records inclusion directives replayed by clangd. - TU.ClangTidyChecks = "replay-preamble-check"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("replay-preamble-check"); llvm::Annotations Test(R"cpp( $hash^#$include[[import]] $filebegin^"$filerange[[bar.h]]" $hash^#$include[[include_next]] $filebegin^"$filerange[[baz.h]]" Index: clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp +++ clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp @@ -14,6 +14,7 @@ #include "TestFS.h" #include "TestIndex.h" #include "TestTU.h" +#include "TestTidyProvider.h" #include "index/MemIndex.h" #include "support/Path.h" #include "clang/Basic/Diagnostic.h" @@ -129,7 +130,8 @@ } )cpp"); auto TU = TestTU::withCode(Test.code()); - TU.ClangTidyChecks = "-*,google-explicit-constructor"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("-*,google-explicit-constructor"); EXPECT_THAT( TU.build().getDiagnostics(), ElementsAre( @@ -201,8 +203,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 = std::make_unique<TestClangTidyProvider>( + "-*, readability-uppercase-literal-suffix, " + "hicpp-uppercase-literal-suffix"); // Verify that we filter out the duplicated diagnostic message. EXPECT_THAT( TU.build().getDiagnostics(), @@ -245,9 +248,9 @@ )cpp"); auto TU = TestTU::withCode(Test.code()); TU.HeaderFilename = "assert.h"; // Suppress "not found" error. - TU.ClangTidyChecks = + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( "-*, 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 +292,8 @@ auto TU = TestTU::withCode(Test.code()); TU.ExtraArgs = {"-isystem."}; TU.AdditionalFiles["a.h"] = TU.AdditionalFiles["b.h"] = ""; - TU.ClangTidyChecks = "-*, llvm-include-order"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("-*, llvm-include-order"); EXPECT_THAT( TU.build().getDiagnostics(), Contains(AllOf(Diag(Test.range(), "#includes are not sorted properly"), @@ -361,7 +365,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "modernize-loop-convert"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("modernize-loop-convert"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre(::testing::AllOf( @@ -384,7 +389,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "bugprone-integer-division"; + TU.ClangTidyProvider = + std::make_unique<TestClangTidyProvider>("bugprone-integer-division"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre(::testing::AllOf( @@ -401,8 +407,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "bugprone-integer-division"; - TU.ClangTidyWarningsAsErrors = "bugprone-integer-division"; + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( + "bugprone-integer-division", "bugprone-integer-division"); EXPECT_THAT( TU.build().getDiagnostics(), UnorderedElementsAre(::testing::AllOf( @@ -452,8 +458,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "bugprone-integer-division"; - TU.ClangTidyWarningsAsErrors = "bugprone-integer-division"; + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( + "bugprone-integer-division", "bugprone-integer-division"); EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre()); } @@ -468,7 +474,8 @@ } )cpp"); TestTU TU = TestTU::withCode(Main.code()); - TU.ClangTidyChecks = "-*,bugprone-bad-signal-to-kill-thread"; + TU.ClangTidyProvider = std::make_unique<TestClangTidyProvider>( + "-*,bugprone-bad-signal-to-kill-thread"); EXPECT_THAT(TU.build().getDiagnostics(), UnorderedElementsAre()); // no-crash } Index: clang-tools-extra/clangd/unittests/ClangdTests.cpp =================================================================== --- clang-tools-extra/clangd/unittests/ClangdTests.cpp +++ 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,20 @@ 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; + CheckAdjustingTidyProvider Provider({}, {}, {}); 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&&); }; Index: clang-tools-extra/clangd/unittests/CMakeLists.txt =================================================================== --- clang-tools-extra/clangd/unittests/CMakeLists.txt +++ clang-tools-extra/clangd/unittests/CMakeLists.txt @@ -87,6 +87,7 @@ TUSchedulerTests.cpp TestFS.cpp TestIndex.cpp + TestTidyProvider.cpp TestTU.cpp TestWorkspace.cpp TypeHierarchyTests.cpp Index: clang-tools-extra/clangd/tool/ClangdMain.cpp =================================================================== --- clang-tools-extra/clangd/tool/ClangdMain.cpp +++ 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" @@ -803,9 +804,7 @@ } // Create an empty clang-tidy option. - std::mutex ClangTidyOptMu; - std::unique_ptr<tidy::ClangTidyOptionsProvider> - ClangTidyOptProvider; /*GUARDED_BY(ClangTidyOptMu)*/ + std::unique_ptr<FileSystemTidyProvider> ClangTidyOptProvider; if (EnableClangTidy) { auto EmptyDefaults = tidy::ClangTidyOptions::getDefaults(); EmptyDefaults.Checks.reset(); // So we can tell if checks were ever set. @@ -817,21 +816,15 @@ tidy::ClangTidyOptions OverrideClangTidyOptions; if (!ClangTidyChecks.empty()) OverrideClangTidyOptions.Checks = ClangTidyChecks; - ClangTidyOptProvider = std::make_unique<tidy::FileOptionsProvider>( - 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<std::mutex> Lock(ClangTidyOptMu); - // FIXME: use the FS provided to the function. - Opts = ClangTidyOptProvider->getOptions(File); - } - return Opts; - }; + if (EnableConfig) + ClangTidyOptProvider = std::make_unique<ConfigTidyProvider>( + tidy::ClangTidyGlobalOptions(), std::move(EmptyDefaults), + / std::move(OverrideClangTidyOptions)); + else + ClangTidyOptProvider = std::make_unique<CheckAdjustingTidyProvider>( + tidy::ClangTidyGlobalOptions(), std::move(EmptyDefaults), + std::move(OverrideClangTidyOptions)); + Opts.ClangTidyProvider = ClangTidyOptProvider.get(); } Opts.AsyncPreambleBuilds = AsyncPreamble; Opts.SuggestMissingIncludes = SuggestMissingIncludes; Index: clang-tools-extra/clangd/tool/Check.cpp =================================================================== --- clang-tools-extra/clangd/tool/Check.cpp +++ 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); Index: clang-tools-extra/clangd/TidyProvider.h =================================================================== --- /dev/null +++ clang-tools-extra/clangd/TidyProvider.h @@ -0,0 +1,118 @@ +//===--- 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 "llvm/Support/VirtualFileSystem.h" +#include <shared_mutex> + +namespace clang { +namespace clangd { + +/// The base class of all clang-tidy config providers for clangd. The option +/// getters take a pointer to a Filesystem that should be used for any file IO. +class ClangdTidyProvider { +public: + using OptionsSource = tidy::ClangTidyOptionsProvider::OptionsSource; + + /// Returns a provider than can be passed directly to the constructor of \ref + /// ClangTidyContext which delegates its option sourcing to this class. + virtual std::unique_ptr<tidy::ClangTidyOptionsProvider> + getInstance(llvm::vfs::FileSystem *FS); + + virtual const tidy::ClangTidyGlobalOptions & + getGlobalOptions(llvm::vfs::FileSystem *) = 0; + + virtual std::vector<OptionsSource> + getRawOptions(llvm::vfs::FileSystem *FS, llvm::StringRef FileName) = 0; + + virtual ~ClangdTidyProvider() = default; +}; + +/// A Provider that will mimic the behaviour of tidy::FileOptionsProvider but +/// it caches options retrieved in a thread safe manner. To be completely thread +/// safe it must be passed a thread safe filesystem when reading files. +class FileTidyProvider : public ClangdTidyProvider { +public: + FileTidyProvider(tidy::ClangTidyGlobalOptions GlobalOptions, + tidy::ClangTidyOptions DefaultOptions, + tidy::ClangTidyOptions OverrideOptions) + : GlobalOpts(std::move(GlobalOptions)), + DefaultOpts(std::move(DefaultOptions)), + OverrideOpts(std::move(OverrideOptions)) {} + const tidy::ClangTidyGlobalOptions & + getGlobalOptions(llvm::vfs::FileSystem * /*unused*/) override; + + std::vector<OptionsSource> getRawOptions(llvm::vfs::FileSystem *FS, + llvm::StringRef FileName) override; + +protected: + /// Loads .clang-tidy config files into \p CurOptions from the filesystem \p + /// FS found in parent directories of \p FileName. + void addFileOptions(llvm::vfs::FileSystem *FS, llvm::StringRef FileName, + std::vector<OptionsSource> &CurOptions); + +private: + /// Tries to read and parse a configuration file located in \p Directory. + llvm::Optional<OptionsSource> tryReadConfigFile(llvm::vfs::FileSystem *FS, + llvm::StringRef Directory); + + /// Trys to get a OptionsSource from \p Directory, returns the OptionsSource + /// and a flag indicating if the value was read from the cache. + llvm::Optional<std::pair<OptionsSource, bool>> + tryGetConfig(llvm::vfs::FileSystem *FS, llvm::StringRef Directory); + + // Using a shared mutex lets different threads read the cache at the same + // time, while preventing any thread updating the cache. + std::shared_timed_mutex CacheGuard; + llvm::StringMap<OptionsSource> CachedOptions; + + const tidy::ClangTidyGlobalOptions GlobalOpts; + const tidy::ClangTidyOptions DefaultOpts; + const tidy::ClangTidyOptions OverrideOpts; +}; + +/// A FileTidyProvider that will adjust its checks for clangd. +/// Either by assigning a nice default set of checks if none are specified or +/// removing checks known to cause issues in clang-tidy. +class CheckAdjustingTidyProvider : public FileTidyProvider { +public: + using FileTidyProvider::FileTidyProvider; + std::vector<OptionsSource> getRawOptions(llvm::vfs::FileSystem *FS, + llvm::StringRef FileName) override; + +protected: + /// Appends an OptionsSource that will set default checks if no checks exists, + /// or disable checks known to cause issue when ran in clangd. + void adjustChecks(std::vector<OptionsSource> &RawOptions); +}; + +/// A ClangdTidyProvider that will use clangd::config when building its options. +class ConfigTidyProvider : public CheckAdjustingTidyProvider { +public: + using CheckAdjustingTidyProvider::CheckAdjustingTidyProvider; + + std::vector<OptionsSource> getRawOptions(llvm::vfs::FileSystem *FS, + llvm::StringRef FileName) override; +}; + +// 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(); + +// Set of nice checks to enable by default if no checks are specified. +// Returns a clang-tidy options string: check1,check2. +llvm::StringRef getDefaultTidyChecks(); + +} // namespace clangd +} // namespace clang + +#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_TIDYPROVIDER_H Index: clang-tools-extra/clangd/TidyProvider.cpp =================================================================== --- /dev/null +++ clang-tools-extra/clangd/TidyProvider.cpp @@ -0,0 +1,293 @@ +//===--- 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 "clang/Basic/LLVM.h" +#include "llvm/Support/VirtualFileSystem.h" +#include <shared_mutex> + +namespace clang { +namespace clangd { + +namespace { + +class ProxyFSTidyOptionsProvider : public tidy::ClangTidyOptionsProvider { +public: + ProxyFSTidyOptionsProvider(ClangdTidyProvider &Provider, + llvm::vfs::FileSystem *FS) + : Provider(Provider), TFSView(FS) {} + + const tidy::ClangTidyGlobalOptions &getGlobalOptions() override { + return Provider.getGlobalOptions(TFSView.get()); + } + std::vector<OptionsSource> getRawOptions(llvm::StringRef FileName) override { + return Provider.getRawOptions(TFSView.get(), FileName); + } + +private: + ClangdTidyProvider &Provider; + llvm::IntrusiveRefCntPtr<llvm::vfs::FileSystem> TFSView; +}; + +} // namespace + +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; +} + +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; +} + +std::unique_ptr<tidy::ClangTidyOptionsProvider> +ClangdTidyProvider::getInstance(llvm::vfs::FileSystem *FS) { + return std::make_unique<ProxyFSTidyOptionsProvider>(*this, FS); +} + +const tidy::ClangTidyGlobalOptions & +FileTidyProvider::getGlobalOptions(llvm::vfs::FileSystem * /*unused*/) { + return GlobalOpts; +} + +std::vector<ClangdTidyProvider::OptionsSource> +FileTidyProvider::getRawOptions(llvm::vfs::FileSystem *FS, + llvm::StringRef FileName) { + std::vector<OptionsSource> RawOptions; + RawOptions.emplace_back(this->DefaultOpts, "clangd: default options"); + addFileOptions(FS, FileName, RawOptions); + + RawOptions.emplace_back(OverrideOpts, "command-line: -clang-tidy-checks"); + + return RawOptions; +} + +void FileTidyProvider::addFileOptions(llvm::vfs::FileSystem *FS, + StringRef FileName, + std::vector<OptionsSource> &CurOptions) { + + llvm::SmallString<128> AbsolutePath(FileName); + + if (FS->makeAbsolute(AbsolutePath)) + return; + + auto CurSize = CurOptions.size(); + + // Look for a suitable configuration file in all parent directories of the + // file. Start with the immediate parent directory and move up. + StringRef Path = llvm::sys::path::parent_path(AbsolutePath); + for (StringRef CurrentPath = Path; !CurrentPath.empty(); + CurrentPath = llvm::sys::path::parent_path(CurrentPath)) { + auto SourceAndFromCache = tryGetConfig(FS, CurrentPath); + + if (!SourceAndFromCache) + continue; // No config found for this directory + + OptionsSource &OptionsAndSource = SourceAndFromCache->first; + + if (!SourceAndFromCache->second) { + // Require exclusive access while we update the CachedOptions. + std::unique_lock<std::shared_timed_mutex> ExclusiveGuard(CacheGuard); + CachedOptions.insert(std::make_pair(Path, OptionsAndSource)); + } + + if (Path != CurrentPath) { + // Defer locking until we are sure we need to update the cache. + std::unique_lock<std::shared_timed_mutex> ExclusiveGuard( + CacheGuard, std::defer_lock_t{}); + + // Acquire this lock now as it's needed to read the cache. + // If we ever need to update the cache, this will be released. + std::shared_lock<std::shared_timed_mutex> SharedGuard(CacheGuard); + + // Store cached value for all intermediate directories. + while (Path != CurrentPath) { + if (CachedOptions.count(Path) == 0) { + // Release the shared access and gain exclusive access if we don't + // already have it. + if (!ExclusiveGuard.owns_lock()) { + assert(SharedGuard.owns_lock()); + SharedGuard.unlock(); + ExclusiveGuard.lock(); + } + CachedOptions.insert(std::make_pair(Path, OptionsAndSource)); + } + Path = llvm::sys::path::parent_path(Path); + } + } + + CurOptions.push_back(std::move(OptionsAndSource)); + + if (!CurOptions.back().first.InheritParentConfig.getValueOr(false)) + break; + // We need to inherit from the directory where the options were found. + // dir1/ + // .clang-tidy + // dir2/ + // .clang-tidy + // dir3/ + // dir4/ + // <FileName> + // If search at dir4/FileName and dir2/.clang-tidy is in the cache, the + // cache will also contain an entry for dir4 that is a copy of dir2. + // Likewise so will dir3. If we don't adjust the search path we'll look for + // the parent config in dir3 which will be a copy of whats in dir4. Where as + // if we adjust the search path to start from dir2 parent we'll search in + // dir1 and find the config we actually need. + AbsolutePath.assign(CurOptions.back().second); + if (FS->makeAbsolute(AbsolutePath)) + break; + Path = llvm::sys::path::parent_path(AbsolutePath); + CurrentPath = Path; + } + // Reverse order of file configs because closer configs should have higher + // priority. + std::reverse(CurOptions.begin() + CurSize, CurOptions.end()); +} + +llvm::Optional<std::pair<ClangdTidyProvider::OptionsSource, bool>> +FileTidyProvider::tryGetConfig(llvm::vfs::FileSystem *FS, + llvm::StringRef Directory) { + { + // Only require shared control when reading. This lets multiple threads + // query the cache at the same time so long as no one is updating it. + std::shared_lock<std::shared_timed_mutex> Guard(CacheGuard); + auto Iter = CachedOptions.find(Directory); + if (Iter != CachedOptions.end()) + return std::make_pair(Iter->getValue(), true); + } + + if (auto Res = tryReadConfigFile(FS, Directory)) + return std::make_pair(std::move(*Res), false); + return llvm::None; +} + +llvm::Optional<ClangdTidyProvider::OptionsSource> +FileTidyProvider::tryReadConfigFile(llvm::vfs::FileSystem *FS, + llvm::StringRef Directory) { + assert(!Directory.empty()); + + llvm::ErrorOr<llvm::vfs::Status> DirectoryStatus = FS->status(Directory); + + if (!DirectoryStatus || !DirectoryStatus->isDirectory()) { + llvm::errs() << "Error reading configuration from " << Directory + << ": directory doesn't exist.\n"; + return llvm::None; + } + + SmallString<128> ConfigFile(Directory); + llvm::sys::path::append(ConfigFile, ".clang-tidy"); + + llvm::ErrorOr<llvm::vfs::Status> FileStatus = FS->status(ConfigFile); + + if (!FileStatus || !FileStatus->isRegularFile()) + return llvm::None; + + llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> 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<tidy::ClangTidyOptions> ParsedOptions = + tidy::parseConfiguration((*Text)->getBuffer()); + if (!ParsedOptions) { + if (ParsedOptions.getError()) + llvm::errs() << "Error parsing " << ConfigFile << ": " + << ParsedOptions.getError().message() << "\n"; + return llvm::None; + } + return OptionsSource(std::move(*ParsedOptions), ConfigFile); +} + +std::vector<ClangdTidyProvider::OptionsSource> +CheckAdjustingTidyProvider::getRawOptions(llvm::vfs::FileSystem *FS, + llvm::StringRef FileName) { + std::vector<OptionsSource> Opts = + FileTidyProvider::getRawOptions(FS, FileName); + adjustChecks(Opts); + return Opts; +} + +void CheckAdjustingTidyProvider::adjustChecks( + std::vector<ClangdTidyProvider::OptionsSource> &RawOptions) { + if (llvm::all_of(RawOptions, [](OptionsSource &Source) { + return !Source.first.Checks.hasValue() || Source.first.Checks->empty(); + })) { + tidy::ClangTidyOptions DefaultChecks; + DefaultChecks.Checks.emplace(getDefaultTidyChecks()); + RawOptions.emplace_back(DefaultChecks, "clangd: default checks"); + } else { + tidy::ClangTidyOptions RemoveIncompatiableChecks; + RemoveIncompatiableChecks.Checks.emplace(getUnusableTidyChecks()); + RawOptions.emplace_back(RemoveIncompatiableChecks, + "clangd: incompatible checks"); + } +} + +std::vector<ClangdTidyProvider::OptionsSource> +ConfigTidyProvider::getRawOptions(llvm::vfs::FileSystem *FS, + llvm::StringRef FileName) { + + const auto &CurTidyConfig = Config::current().ClangTidy; + + // If the config doesn't contain any clang-tidy options, fall back to the + // CheckAdjustingTidyProvider. + if (CurTidyConfig.Checks.empty() && CurTidyConfig.CheckOptions.empty()) + return CheckAdjustingTidyProvider::getRawOptions(FS, FileName); + + std::vector<OptionsSource> OptionSources = + FileTidyProvider::getRawOptions(FS, FileName); + + tidy::ClangTidyOptions ConfigOpts; + + if (!CurTidyConfig.Checks.empty()) + ConfigOpts.Checks.emplace(CurTidyConfig.Checks); + + for (const auto &CheckOption : CurTidyConfig.CheckOptions) + ConfigOpts.CheckOptions.insert_or_assign( + CheckOption.getKey(), + tidy::ClangTidyOptions::ClangTidyValue(CheckOption.getValue())); + + OptionSources.insert(OptionSources.end() - 1, + OptionsSource(ConfigOpts, "clangd-config")); + + adjustChecks(OptionSources); + return OptionSources; +} + +} // namespace clangd +} // namespace clang Index: clang-tools-extra/clangd/ParsedAST.cpp =================================================================== --- clang-tools-extra/clangd/ParsedAST.cpp +++ 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" @@ -246,7 +247,7 @@ auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory); if (Preamble && Preamble->StatCache) - VFS = Preamble->StatCache->getConsumingFS(std::move(VFS)); + VFS = Preamble->StatCache->getConsumingFS(VFS.get()); assert(CI); // Command-line parsing sets DisableFree to true by default, but we don't want @@ -292,13 +293,20 @@ llvm::Optional<tidy::ClangTidyContext> 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::DefaultOptionsProvider>( - tidy::ClangTidyGlobalOptions(), Inputs.Opts.ClangTidyOpts)); + if (Inputs.ClangTidyProvider) { + // Use a proxy provider so we can re-use the same underlying provider for + // building different ASTs. As long as the underlying provider is thread + // safe there won't be any issue. + CTContext.emplace(Inputs.ClangTidyProvider->getInstance(VFS.get())); + } else { + // This OptionsProvider will have no checks enabled, essentially disabling + // clang-tidy. + CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>( + tidy::ClangTidyGlobalOptions(), tidy::ClangTidyOptions())); + } CTContext->setDiagnosticsEngine(&Clang->getDiagnostics()); CTContext->setASTContext(&Clang->getASTContext()); CTContext->setCurrentFile(Filename); Index: clang-tools-extra/clangd/Compiler.h =================================================================== --- clang-tools-extra/clangd/Compiler.h +++ clang-tools-extra/clangd/Compiler.h @@ -15,7 +15,6 @@ #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 "index/Index.h" #include "support/ThreadsafeFS.h" @@ -26,6 +25,8 @@ namespace clang { namespace clangd { +class ClangdTidyProvider; + class IgnoreDiagnostics : public DiagnosticConsumer { public: static void log(DiagnosticsEngine::Level DiagLevel, @@ -37,7 +38,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 +56,7 @@ // Used to recover from diagnostics (e.g. find missing includes for symbol). const SymbolIndex *Index = nullptr; ParseOptions Opts = ParseOptions(); + ClangdTidyProvider *ClangTidyProvider = nullptr; }; /// Builds compiler invocation that could be used to build AST or preamble. Index: clang-tools-extra/clangd/ClangdServer.h =================================================================== --- clang-tools-extra/clangd/ClangdServer.h +++ clang-tools-extra/clangd/ClangdServer.h @@ -41,13 +41,7 @@ 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<tidy::ClangTidyOptions( - llvm::vfs::FileSystem &, llvm::StringRef /*File*/)>; - +class ClangdTidyProvider; /// Manages a collection of source files and derived data (ASTs, indexes), /// and provides language-aware features such as code completion. /// @@ -121,12 +115,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. + ClangdTidyProvider *ClangTidyProvider = nullptr; /// If true, force -frecovery-ast flag. /// If false, respect the value in clang. @@ -373,8 +364,8 @@ // Storage for merged views of the various indexes. std::vector<std::unique_ptr<SymbolIndex>> MergedIdx; - // When set, provides clang-tidy options for a specific file. - ClangTidyOptionsBuilder GetClangTidyOptions; + // OptionsPrivder for clang-tidy. + ClangdTidyProvider *ClangTidyProvider = nullptr; // 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). Index: clang-tools-extra/clangd/ClangdServer.cpp =================================================================== --- clang-tools-extra/clangd/ClangdServer.cpp +++ clang-tools-extra/clangd/ClangdServer.cpp @@ -113,40 +113,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() { @@ -177,7 +143,7 @@ ? new FileIndex(Opts.HeavyweightDynamicSymbolIndex, Opts.CollectMainFileRefs) : nullptr), - GetClangTidyOptions(Opts.GetClangTidyOptions), + ClangTidyProvider(Opts.ClangTidyProvider), SuggestMissingIncludes(Opts.SuggestMissingIncludes), BuildRecoveryAST(Opts.BuildRecoveryAST), PreserveRecoveryASTType(Opts.PreserveRecoveryASTType), @@ -235,20 +201,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. @@ -259,6 +211,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); Index: clang-tools-extra/clangd/CMakeLists.txt =================================================================== --- clang-tools-extra/clangd/CMakeLists.txt +++ clang-tools-extra/clangd/CMakeLists.txt @@ -77,6 +77,7 @@ SemanticSelection.cpp SourceCode.cpp QueryDriverDatabase.cpp + TidyProvider.cpp TUScheduler.cpp URI.cpp XRefs.cpp
_______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
