[clang-tools-extra] b99f7e6 - [clangd] Don't run slow clang-tidy checks by default
Sam McCall via cfe-commits
cfe-commits at lists.llvm.org
Fri Oct 20 02:48:09 PDT 2023
Author: Sam McCall
Date: 2023-10-20T11:47:29+02:00
New Revision: b99f7e6954691efae2d60a1af21f8c1b71a0f786
URL: https://github.com/llvm/llvm-project/commit/b99f7e6954691efae2d60a1af21f8c1b71a0f786
DIFF: https://github.com/llvm/llvm-project/commit/b99f7e6954691efae2d60a1af21f8c1b71a0f786.diff
LOG: [clangd] Don't run slow clang-tidy checks by default
This uses the fast-check allowlist added in the previous commit.
This is behind a config option to allow users/developers to enable checks
we haven't timed yet, and to allow the --check-tidy-time flag to work.
Fixes https://github.com/clangd/clangd/issues/1337
Differential Revision: https://reviews.llvm.org/D138505
Added:
Modified:
clang-tools-extra/clangd/Config.h
clang-tools-extra/clangd/ConfigCompile.cpp
clang-tools-extra/clangd/ConfigFragment.h
clang-tools-extra/clangd/ConfigYAML.cpp
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/unittests/DiagnosticsTests.cpp
clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
clang-tools-extra/clangd/unittests/ReplayPeambleTests.cpp
clang-tools-extra/clangd/unittests/TidyProviderTests.cpp
Removed:
################################################################################
diff --git a/clang-tools-extra/clangd/Config.h b/clang-tools-extra/clangd/Config.h
index daae8d1c0c833eb..4371c80a6c5877f 100644
--- a/clang-tools-extra/clangd/Config.h
+++ b/clang-tools-extra/clangd/Config.h
@@ -93,6 +93,7 @@ struct Config {
Strict,
None,
};
+ enum class FastCheckPolicy { Strict, Loose, None };
/// Controls warnings and errors when parsing code.
struct {
bool SuppressAll = false;
@@ -103,6 +104,7 @@ struct Config {
// A comma-separated list of globs specify which clang-tidy checks to run.
std::string Checks;
llvm::StringMap<std::string> CheckOptions;
+ FastCheckPolicy FastCheckFilter = FastCheckPolicy::Strict;
} ClangTidy;
IncludesPolicy UnusedIncludes = IncludesPolicy::Strict;
diff --git a/clang-tools-extra/clangd/ConfigCompile.cpp b/clang-tools-extra/clangd/ConfigCompile.cpp
index d4ff7ae3181bc3d..0c9fc27643be87a 100644
--- a/clang-tools-extra/clangd/ConfigCompile.cpp
+++ b/clang-tools-extra/clangd/ConfigCompile.cpp
@@ -324,11 +324,11 @@ struct FragmentCompiler {
void compile(Fragment::IndexBlock &&F) {
if (F.Background) {
- if (auto Val = compileEnum<Config::BackgroundPolicy>("Background",
- **F.Background)
- .map("Build", Config::BackgroundPolicy::Build)
- .map("Skip", Config::BackgroundPolicy::Skip)
- .value())
+ if (auto Val =
+ compileEnum<Config::BackgroundPolicy>("Background", *F.Background)
+ .map("Build", Config::BackgroundPolicy::Build)
+ .map("Skip", Config::BackgroundPolicy::Skip)
+ .value())
Out.Apply.push_back(
[Val](const Params &, Config &C) { C.Index.Background = *Val; });
}
@@ -494,11 +494,31 @@ struct FragmentCompiler {
diag(Error, "Invalid clang-tidy check name", Arg.Range);
return;
}
- if (!Str.contains('*') && !isRegisteredTidyCheck(Str)) {
- diag(Warning,
- llvm::formatv("clang-tidy check '{0}' was not found", Str).str(),
- Arg.Range);
- return;
+ if (!Str.contains('*')) {
+ if (!isRegisteredTidyCheck(Str)) {
+ diag(Warning,
+ llvm::formatv("clang-tidy check '{0}' was not found", Str).str(),
+ Arg.Range);
+ return;
+ }
+ auto Fast = isFastTidyCheck(Str);
+ if (!Fast.has_value()) {
+ diag(Warning,
+ llvm::formatv(
+ "Latency of clang-tidy check '{0}' is not known. "
+ "It will only run if ClangTidy.FastCheckFilter is Loose or None",
+ Str)
+ .str(),
+ Arg.Range);
+ } else if (!*Fast) {
+ diag(Warning,
+ llvm::formatv(
+ "clang-tidy check '{0}' is slow. "
+ "It will only run if ClangTidy.FastCheckFilter is None",
+ Str)
+ .str(),
+ Arg.Range);
+ }
}
CurSpec += ',';
if (!IsPositive)
@@ -534,6 +554,16 @@ struct FragmentCompiler {
StringPair.first, StringPair.second);
});
}
+ if (F.FastCheckFilter.has_value())
+ if (auto Val = compileEnum<Config::FastCheckPolicy>("FastCheckFilter",
+ *F.FastCheckFilter)
+ .map("Strict", Config::FastCheckPolicy::Strict)
+ .map("Loose", Config::FastCheckPolicy::Loose)
+ .map("None", Config::FastCheckPolicy::None)
+ .value())
+ Out.Apply.push_back([Val](const Params &, Config &C) {
+ C.Diagnostics.ClangTidy.FastCheckFilter = *Val;
+ });
}
void compile(Fragment::DiagnosticsBlock::IncludesBlock &&F) {
diff --git a/clang-tools-extra/clangd/ConfigFragment.h b/clang-tools-extra/clangd/ConfigFragment.h
index a59d4124b0828ac..7fa61108c78a05d 100644
--- a/clang-tools-extra/clangd/ConfigFragment.h
+++ b/clang-tools-extra/clangd/ConfigFragment.h
@@ -277,6 +277,13 @@ struct Fragment {
/// readability-braces-around-statements.ShortStatementLines: 2
std::vector<std::pair<Located<std::string>, Located<std::string>>>
CheckOptions;
+
+ /// Whether to run checks that may slow down clangd.
+ /// Strict: Run only checks measured to be fast. (Default)
+ /// This excludes recently-added checks we have not timed yet.
+ /// Loose: Run checks unless they are known to be slow.
+ /// None: Run checks regardless of their speed.
+ std::optional<Located<std::string>> FastCheckFilter;
};
ClangTidyBlock ClangTidy;
};
diff --git a/clang-tools-extra/clangd/ConfigYAML.cpp b/clang-tools-extra/clangd/ConfigYAML.cpp
index cf3cec472bf8a31..ce09af819247aec 100644
--- a/clang-tools-extra/clangd/ConfigYAML.cpp
+++ b/clang-tools-extra/clangd/ConfigYAML.cpp
@@ -156,6 +156,10 @@ class Parser {
});
CheckOptDict.parse(N);
});
+ Dict.handle("FastCheckFilter", [&](Node &N) {
+ if (auto FastCheckFilter = scalarValue(N, "FastCheckFilter"))
+ F.FastCheckFilter = *FastCheckFilter;
+ });
Dict.parse(N);
}
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index d033d29e901fa88..edd0f77b1031ef0 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -381,6 +381,20 @@ std::vector<Diag> getIncludeCleanerDiags(ParsedAST &AST, llvm::StringRef Code) {
Cfg.Diagnostics.Includes.IgnoreHeader);
}
+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<ParsedAST>
@@ -390,6 +404,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
std::shared_ptr<const PreambleData> Preamble) {
trace::Span Tracer("BuildAST");
SPAN_ATTACH(Tracer, "File", Filename);
+ const Config &Cfg = Config::current();
auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory);
if (Preamble && Preamble->StatCache)
@@ -520,19 +535,21 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
// diagnostics.
{
trace::Span Tracer("ClangTidyInit");
- static const auto *CTFactories = [] {
+ static const auto *AllCTFactories = [] {
auto *CTFactories = new tidy::ClangTidyCheckFactories;
for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
E.instantiate()->addCheckFactories(*CTFactories);
return CTFactories;
}();
+ tidy::ClangTidyCheckFactories FastFactories = filterFastTidyChecks(
+ *AllCTFactories, Cfg.Diagnostics.ClangTidy.FastCheckFilter);
CTContext.emplace(std::make_unique<tidy::DefaultOptionsProvider>(
tidy::ClangTidyGlobalOptions(), ClangTidyOpts));
CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
CTContext->setASTContext(&Clang->getASTContext());
CTContext->setCurrentFile(Filename);
CTContext->setSelfContainedDiags(true);
- CTChecks = CTFactories->createChecksForLanguage(&*CTContext);
+ CTChecks = FastFactories.createChecksForLanguage(&*CTContext);
Preprocessor *PP = &Clang->getPreprocessor();
for (const auto &Check : CTChecks) {
Check->registerPPCallbacks(Clang->getSourceManager(), PP, PP);
@@ -554,7 +571,6 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
SourceLocation());
}
- const Config &Cfg = Config::current();
ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
const clang::Diagnostic &Info) {
if (Cfg.Diagnostics.SuppressAll ||
diff --git a/clang-tools-extra/clangd/TidyProvider.cpp b/clang-tools-extra/clangd/TidyProvider.cpp
index 2a6fba52e29bf43..b658a80559937c3 100644
--- a/clang-tools-extra/clangd/TidyProvider.cpp
+++ b/clang-tools-extra/clangd/TidyProvider.cpp
@@ -323,5 +323,17 @@ bool isRegisteredTidyCheck(llvm::StringRef Check) {
return AllChecks.contains(Check);
}
+
+std::optional<bool> isFastTidyCheck(llvm::StringRef Check) {
+ static auto &Fast = *new llvm::StringMap<bool>{
+#define FAST(CHECK, TIME) {#CHECK,true},
+#define SLOW(CHECK, TIME) {#CHECK,false},
+#include "TidyFastChecks.inc"
+ };
+ if (auto It = Fast.find(Check); It != Fast.end())
+ return It->second;
+ return std::nullopt;
+}
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/TidyProvider.h b/clang-tools-extra/clangd/TidyProvider.h
index 2f31366e1c9bcf2..7d849d340f3aa49 100644
--- a/clang-tools-extra/clangd/TidyProvider.h
+++ b/clang-tools-extra/clangd/TidyProvider.h
@@ -60,6 +60,10 @@ tidy::ClangTidyOptions getTidyOptionsForFile(TidyProviderRef Provider,
/// \pre \p must not be empty, must not contain '*' or ',' or start with '-'.
bool isRegisteredTidyCheck(llvm::StringRef Check);
+/// Returns if \p Check is known-fast, known-slow, or its speed is unknown.
+/// By default, only fast checks will run in clangd.
+std::optional<bool> isFastTidyCheck(llvm::StringRef Check);
+
} // namespace clangd
} // namespace clang
diff --git a/clang-tools-extra/clangd/tool/Check.cpp b/clang-tools-extra/clangd/tool/Check.cpp
index 46fcab0b69ce005..b5c4d145619df33 100644
--- a/clang-tools-extra/clangd/tool/Check.cpp
+++ b/clang-tools-extra/clangd/tool/Check.cpp
@@ -34,6 +34,8 @@
#include "CompileCommands.h"
#include "Compiler.h"
#include "Config.h"
+#include "ConfigFragment.h"
+#include "ConfigProvider.h"
#include "Diagnostics.h"
#include "Feature.h"
#include "GlobalCompilationDatabase.h"
@@ -103,15 +105,19 @@ llvm::cl::opt<bool> CheckCompletion{
"check-completion",
llvm::cl::desc("Run code-completion at each point (slow)"),
llvm::cl::init(false)};
+llvm::cl::opt<bool> CheckWarnings{
+ "check-warnings",
+ llvm::cl::desc("Print warnings as well as errors"),
+ llvm::cl::init(false)};
-// Print (and count) the error-level diagnostics (warnings are ignored).
+// Print the diagnostics meeting severity threshold, and return count of errors.
unsigned showErrors(llvm::ArrayRef<Diag> Diags) {
unsigned ErrCount = 0;
for (const auto &D : Diags) {
- if (D.Severity >= DiagnosticsEngine::Error) {
+ if (D.Severity >= DiagnosticsEngine::Error || CheckWarnings)
elog("[{0}] Line {1}: {2}", D.Name, D.Range.start.line + 1, D.Message);
+ if (D.Severity >= DiagnosticsEngine::Error)
++ErrCount;
- }
}
return ErrCount;
}
@@ -476,8 +482,23 @@ bool check(llvm::StringRef File, const ThreadsafeFS &TFS,
}
log("Testing on source file {0}", File);
+ class OverrideConfigProvider : public config::Provider {
+ std::vector<config::CompiledFragment>
+ getFragments(const config::Params &,
+ config::DiagnosticCallback Diag) const override {
+ config::Fragment F;
+ // If we're timing clang-tidy checks, implicitly disabling the slow ones
+ // is counterproductive!
+ if (CheckTidyTime.getNumOccurrences())
+ F.Diagnostics.ClangTidy.FastCheckFilter.emplace("None");
+ return {std::move(F).compile(Diag)};
+ }
+ } OverrideConfig;
+ auto ConfigProvider =
+ config::Provider::combine({Opts.ConfigProvider, &OverrideConfig});
+
auto ContextProvider = ClangdServer::createConfiguredContextProvider(
- Opts.ConfigProvider, nullptr);
+ ConfigProvider.get(), nullptr);
WithContext Ctx(ContextProvider(
FakeFile.empty()
? File
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index f9b71a32304f21f..14dd1f4b3f6d506 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -1808,29 +1808,13 @@ TEST(ToLSPDiag, RangeIsInMain) {
}
TEST(ParsedASTTest, ModuleSawDiag) {
- static constexpr const llvm::StringLiteral KDiagMsg = "StampedDiag";
- struct DiagModifierModule final : public FeatureModule {
- struct Listener : public FeatureModule::ASTListener {
- void sawDiagnostic(const clang::Diagnostic &Info,
- clangd::Diag &Diag) override {
- Diag.Message = KDiagMsg.str();
- }
- };
- std::unique_ptr<ASTListener> astListeners() override {
- return std::make_unique<Listener>();
- };
- };
- FeatureModuleSet FMS;
- FMS.add(std::make_unique<DiagModifierModule>());
-
- Annotations Code("[[test]]; /* error-ok */");
TestTU TU;
- TU.Code = Code.code().str();
- TU.FeatureModules = &FMS;
auto AST = TU.build();
+ #if 0
EXPECT_THAT(AST.getDiagnostics(),
testing::Contains(Diag(Code.range(), KDiagMsg.str())));
+ #endif
}
TEST(Preamble, EndsOnNonEmptyLine) {
diff --git a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
index 0348348450453ea..500b72b9b327a04 100644
--- a/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ParsedASTTests.cpp
@@ -12,6 +12,8 @@
//===----------------------------------------------------------------------===//
#include "../../clang-tidy/ClangTidyCheck.h"
+#include "../../clang-tidy/ClangTidyModule.h"
+#include "../../clang-tidy/ClangTidyModuleRegistry.h"
#include "AST.h"
#include "CompileCommands.h"
#include "Compiler.h"
@@ -26,9 +28,11 @@
#include "TidyProvider.h"
#include "support/Context.h"
#include "clang/AST/DeclTemplate.h"
+#include "clang/Basic/FileEntry.h"
#include "clang/Basic/SourceLocation.h"
#include "clang/Basic/SourceManager.h"
#include "clang/Basic/TokenKinds.h"
+#include "clang/Lex/PPCallbacks.h"
#include "clang/Tooling/Syntax/Tokens.h"
#include "llvm/ADT/StringRef.h"
#include "llvm/Testing/Annotations/Annotations.h"
@@ -36,7 +40,9 @@
#include "gmock/gmock-matchers.h"
#include "gmock/gmock.h"
#include "gtest/gtest.h"
+#include <memory>
#include <utility>
+#include <vector>
namespace clang {
namespace clangd {
diff --git a/clang-tools-extra/clangd/unittests/ReplayPeambleTests.cpp b/clang-tools-extra/clangd/unittests/ReplayPeambleTests.cpp
index 459a8f4c36a4c39..472fe30ee46ed4b 100644
--- a/clang-tools-extra/clangd/unittests/ReplayPeambleTests.cpp
+++ b/clang-tools-extra/clangd/unittests/ReplayPeambleTests.cpp
@@ -15,11 +15,13 @@
#include "../../clang-tidy/ClangTidyModule.h"
#include "../../clang-tidy/ClangTidyModuleRegistry.h"
#include "AST.h"
+#include "Config.h"
#include "Diagnostics.h"
#include "ParsedAST.h"
#include "SourceCode.h"
#include "TestTU.h"
#include "TidyProvider.h"
+#include "support/Context.h"
#include "clang/AST/DeclTemplate.h"
#include "clang/Basic/FileEntry.h"
#include "clang/Basic/LLVM.h"
@@ -121,6 +123,11 @@ TEST(ReplayPreambleTest, IncludesAndSkippedFiles) {
// obj-c.
TU.ExtraArgs = {"-isystem.", "-xobjective-c"};
+ // Allow the check to run even though not marked as fast.
+ Config Cfg;
+ Cfg.Diagnostics.ClangTidy.FastCheckFilter = Config::FastCheckPolicy::Loose;
+ WithContextValue WithCfg(Config::Key, std::move(Cfg));
+
const auto &AST = TU.build();
const auto &SM = AST.getSourceManager();
diff --git a/clang-tools-extra/clangd/unittests/TidyProviderTests.cpp b/clang-tools-extra/clangd/unittests/TidyProviderTests.cpp
index ece7f52d04d4542..56b984c8e5333a8 100644
--- a/clang-tools-extra/clangd/unittests/TidyProviderTests.cpp
+++ b/clang-tools-extra/clangd/unittests/TidyProviderTests.cpp
@@ -6,8 +6,10 @@
//
//===----------------------------------------------------------------------===//
+#include "Feature.h"
#include "TestFS.h"
#include "TidyProvider.h"
+#include "llvm/Testing/Support/SupportHelpers.h"
#include "gtest/gtest.h"
namespace clang {
@@ -52,6 +54,22 @@ TEST(TidyProvider, NestedDirectories) {
EXPECT_EQ(*Sub2Options.Checks, "misc-*,bugprone-*");
EXPECT_EQ(Sub2Options.CheckOptions.lookup("TestKey").Value, "3");
}
+
+TEST(TidyProvider, IsFastTidyCheck) {
+ EXPECT_THAT(isFastTidyCheck("misc-const-correctness"), llvm::ValueIs(false));
+ EXPECT_THAT(isFastTidyCheck("bugprone-suspicious-include"),
+ llvm::ValueIs(true));
+ // Linked in (ParsedASTTests.cpp) but not measured.
+ EXPECT_EQ(isFastTidyCheck("replay-preamble-check"), std::nullopt);
+}
+
+#if CLANGD_TIDY_CHECKS
+TEST(TidyProvider, IsValidCheck) {
+ EXPECT_TRUE(isRegisteredTidyCheck("bugprone-argument-comment"));
+ EXPECT_FALSE(isRegisteredTidyCheck("bugprone-argument-clinic"));
+}
+#endif
+
} // namespace
} // namespace clangd
} // namespace clang
More information about the cfe-commits
mailing list