[clang-tidy] Add ClangQueryChecks config option #123734
Open
+257
−1
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@AaronBallman could you be a reviewer please? |
|
@llvm/pr-subscribers-clang-tools-extra Author: None (DeNiCoN) ChangesThis addresses the #107680. This pull request adds new config option that allows to define new checks using the matchers syntax by incorporating clang-query parser Example: Checks: -*,custom-*
ClangQueryChecks:
custom-math: |
let expr callExpr(
callee(functionDecl(
hasAnyName("acos", "asin", "atan", "atan2", "cos", "sin", "tan", "cosh", "sinh", "tanh", "ctan"),
anyOf(
hasDeclContext(namespaceDecl(hasName("std"))),
unless(hasParent(namespaceDecl()))
)
))
).bind("Please use different math functions")
match expr
custom-sort: |
match callExpr(
callee(functionDecl(
hasAnyName("sort", "nth_element", "partial_sort", "partition"),
anyOf(
hasDeclContext(namespaceDecl(hasName("std"))),
unless(hasParent(namespaceDecl()))
)
))
).bind("Please use different sort functions")
custom-long: |
match varDecl(
hasType(asString("long")),
hasTypeLoc(typeLoc().bind("Please use int instead of long"))
)Unimplemented ideas
Some thoughts on further possible workIf transformers parser will be implemented in the future(it seems like in the past there were some work on it being done by @ymand) it can be added to clang-query and as a consequence it can be made that custom checks defined by this new option will support automatic fixes being provided Full diff: https://github.com/llvm/llvm-project/pull/123734.diff 10 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/CMakeLists.txt b/clang-tools-extra/clang-tidy/CMakeLists.txt
index 93117cf1d6373a..76585b012d1742 100644
--- a/clang-tools-extra/clang-tidy/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -11,6 +11,7 @@ include_directories(BEFORE ${CMAKE_CURRENT_BINARY_DIR})
add_clang_library(clangTidy STATIC
ClangTidy.cpp
ClangTidyCheck.cpp
+ ClangQueryCheck.cpp
ClangTidyModule.cpp
ClangTidyDiagnosticConsumer.cpp
ClangTidyOptions.cpp
@@ -38,6 +39,7 @@ clang_target_link_libraries(clangTidy
clangSerialization
clangTooling
clangToolingCore
+ clangQuery
)
if(CLANG_TIDY_ENABLE_STATIC_ANALYZER)
diff --git a/clang-tools-extra/clang-tidy/ClangQueryCheck.cpp b/clang-tools-extra/clang-tidy/ClangQueryCheck.cpp
new file mode 100644
index 00000000000000..a9f46116f70899
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/ClangQueryCheck.cpp
@@ -0,0 +1,30 @@
+//===--- ClangQueryCheck.cpp - 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 "ClangQueryCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+void ClangQueryCheck::registerMatchers(MatchFinder *Finder) {
+ for (const auto &Matcher : Matchers) {
+ bool Ok = Finder->addDynamicMatcher(Matcher, this);
+ assert(Ok && "Expected to get top level matcher from query parser");
+ }
+}
+
+void ClangQueryCheck::check(const MatchFinder::MatchResult &Result) {
+ auto Map = Result.Nodes.getMap();
+ for (const auto &[k, v] : Map) {
+ diag(v.getSourceRange().getBegin(), k) << v.getSourceRange();
+ }
+}
+
+} // namespace clang::tidy::misc
diff --git a/clang-tools-extra/clang-tidy/ClangQueryCheck.h b/clang-tools-extra/clang-tidy/ClangQueryCheck.h
new file mode 100644
index 00000000000000..3c3c7029720687
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/ClangQueryCheck.h
@@ -0,0 +1,43 @@
+//===--- ClangQueryCheck.h - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGQUERYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGQUERYCHECK_H
+
+#include "ClangTidyCheck.h"
+#include "clang/ASTMatchers/Dynamic/VariantValue.h"
+#include <vector>
+
+namespace clang::query {
+class QuerySession;
+} // namespace clang::query
+
+namespace clang::tidy::misc {
+
+/// A check that matches a given matchers printing their binds as warnings
+class ClangQueryCheck : public ClangTidyCheck {
+ using MatcherVec = std::vector<ast_matchers::dynamic::DynTypedMatcher>;
+
+public:
+ ClangQueryCheck(StringRef Name, ClangTidyContext *Context,
+ MatcherVec Matchers)
+ : ClangTidyCheck(Name, Context), Matchers(std::move(Matchers)) {}
+
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+
+private:
+ MatcherVec Matchers;
+};
+
+} // namespace clang::tidy::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGQUERYCHECK_H
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 959b11777e88d4..75466b6612b253 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -15,6 +15,7 @@
//===----------------------------------------------------------------------===//
#include "ClangTidy.h"
+#include "ClangQueryCheck.h"
#include "ClangTidyCheck.h"
#include "ClangTidyDiagnosticConsumer.h"
#include "ClangTidyModuleRegistry.h"
@@ -350,6 +351,13 @@ ClangTidyASTConsumerFactory::ClangTidyASTConsumerFactory(
std::unique_ptr<ClangTidyModule> Module = E.instantiate();
Module->addCheckFactories(*CheckFactories);
}
+
+ for (const auto &[k, v] : Context.getOptions().ClangQueryChecks) {
+ CheckFactories->registerCheckFactory(k, [v](StringRef Name,
+ ClangTidyContext *Context) {
+ return std::make_unique<misc::ClangQueryCheck>(Name, Context, v.Matchers);
+ });
+ }
}
#if CLANG_TIDY_ENABLE_STATIC_ANALYZER
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index 8bac6f161fa05b..0d6edb88a1d603 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -7,6 +7,8 @@
//===----------------------------------------------------------------------===//
#include "ClangTidyOptions.h"
+#include "../clang-query/Query.h"
+#include "../clang-query/QueryParser.h"
#include "ClangTidyModuleRegistry.h"
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/SmallString.h"
@@ -126,6 +128,83 @@ void yamlize(IO &IO, ClangTidyOptions::OptionMap &Val, bool,
}
}
+std::vector<clang::ast_matchers::dynamic::DynTypedMatcher>
+processQuerySource(IO &IO, StringRef SourceRef,
+ clang::query::QuerySession &QS) {
+ namespace query = clang::query;
+ std::vector<clang::ast_matchers::dynamic::DynTypedMatcher> Matchers;
+
+ while (!SourceRef.empty()) {
+ query::QueryRef Q = query::QueryParser::parse(SourceRef, QS);
+ switch (Q->Kind) {
+ case query::QK_Match: {
+ const auto &MatchQuerry = llvm::cast<query::MatchQuery>(*Q);
+ Matchers.push_back(MatchQuerry.Matcher);
+ break;
+ }
+ case query::QK_Let: {
+ const auto &LetQuerry = llvm::cast<query::LetQuery>(*Q);
+ LetQuerry.run(llvm::errs(), QS);
+ break;
+ }
+ case query::QK_Invalid: {
+ const auto &InvalidQuerry = llvm::cast<query::InvalidQuery>(*Q);
+ for (const auto &Line : llvm::split(InvalidQuerry.ErrStr, "\n")) {
+ IO.setError(Line);
+ }
+ break;
+ }
+ // FIXME FileQuerry should also be supported, but what to do with relative
+ // paths?
+ case query::QK_File:
+ case query::QK_DisableOutputKind:
+ case query::QK_EnableOutputKind:
+ case query::QK_SetOutputKind:
+ case query::QK_SetTraversalKind:
+ case query::QK_Help:
+ case query::QK_NoOp:
+ case query::QK_Quit:
+ case query::QK_SetBool: {
+ IO.setError("unsupported querry kind");
+ }
+ }
+ SourceRef = Q->RemainingContent;
+ }
+
+ return Matchers;
+}
+
+template <>
+void yamlize(IO &IO, ClangTidyOptions::QueryCheckMap &Val, bool,
+ EmptyContext &Ctx) {
+ IO.beginMapping();
+ if (IO.outputting()) {
+ for (auto &[k, v] : Val) {
+ IO.mapRequired(k.data(), v);
+ }
+ } else {
+ for (StringRef Key : IO.keys()) {
+ IO.mapRequired(Key.data(), Val[Key]);
+ }
+ }
+ IO.endMapping();
+}
+
+template <>
+void yamlize(IO &IO, ClangTidyOptions::QueryCheckValue &Val, bool,
+ EmptyContext &Ctx) {
+ if (IO.outputting()) {
+ StringRef SourceRef = Val.Source;
+ IO.blockScalarString(SourceRef);
+ } else {
+ StringRef SourceRef;
+ IO.blockScalarString(SourceRef);
+ Val.Source = SourceRef;
+ clang::query::QuerySession QS({});
+ Val.Matchers = processQuerySource(IO, SourceRef, QS);
+ }
+}
+
struct ChecksVariant {
std::optional<std::string> AsString;
std::optional<std::vector<std::string>> AsVector;
@@ -181,6 +260,7 @@ template <> struct MappingTraits<ClangTidyOptions> {
IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
IO.mapOptional("UseColor", Options.UseColor);
IO.mapOptional("SystemHeaders", Options.SystemHeaders);
+ IO.mapOptional("ClangQueryChecks", Options.ClangQueryChecks);
}
};
@@ -249,6 +329,10 @@ ClangTidyOptions &ClangTidyOptions::mergeWith(const ClangTidyOptions &Other,
ClangTidyValue(KeyValue.getValue().Value,
KeyValue.getValue().Priority + Order));
}
+
+ for (const auto &KeyValue : Other.ClangQueryChecks) {
+ ClangQueryChecks.insert_or_assign(KeyValue.getKey(), KeyValue.getValue());
+ }
return *this;
}
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.h b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
index dd78c570d25d9b..3a015ed5163cdc 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -9,6 +9,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYOPTIONS_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYOPTIONS_H
+#include "clang/ASTMatchers/Dynamic/VariantValue.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringMap.h"
@@ -126,8 +127,15 @@ struct ClangTidyOptions {
using StringPair = std::pair<std::string, std::string>;
using OptionMap = llvm::StringMap<ClangTidyValue>;
+ struct QueryCheckValue {
+ std::string Source;
+ std::vector<ast_matchers::dynamic::DynTypedMatcher> Matchers;
+ };
+ using QueryCheckMap = llvm::StringMap<QueryCheckValue>;
+
/// Key-value mapping used to store check-specific options.
OptionMap CheckOptions;
+ QueryCheckMap ClangQueryChecks;
using ArgList = std::vector<std::string>;
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index fa8887e4639b4a..c60e6fcb8c5fa7 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -60,6 +60,18 @@ Configuration files:
Checks - Same as '--checks'. Additionally, the list of
globs can be specified as a list instead of a
string.
+ ClangQueryChecks - List of key-value pairs. Key specifies a name
+ of the new check and value specifies a list
+ of matchers in the form of clang-query
+ syntax. Example:
+ ClangQueryChecks:
+ custom-check: |
+ let matcher varDecl(
+ hasTypeLoc(
+ typeLoc().bind("Custom message")
+ )
+ )
+ match matcher
ExcludeHeaderFilterRegex - Same as '--exclude-header-filter'.
ExtraArgs - Same as '--extra-arg'.
ExtraArgsBefore - Same as '--extra-arg-before'.
@@ -483,7 +495,8 @@ static StringRef closest(StringRef Value, const StringSet<> &Allowed) {
return Closest;
}
-static constexpr StringLiteral VerifyConfigWarningEnd = " [-verify-config]\n";
+static constexpr llvm::StringLiteral VerifyConfigWarningEnd =
+ " [-verify-config]\n";
static bool verifyChecks(const StringSet<> &AllChecks, StringRef CheckGlob,
StringRef Source) {
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 33a452f525f763..b845b32569872c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -140,6 +140,9 @@ Improvements to clang-tidy
:doc:`readability-redundant-access-specifiers <clang-tidy/checks/readability/redundant-access-specifiers>`, CheckFirstDeclaration
:doc:`readability-redundant-casting <clang-tidy/checks/readability/redundant-casting>`, IgnoreTypeAliases
+- New :program:`clang-tidy` config property `ClangQueryChecks` that allows adding
+ custom checks based on a :program:`clang-query` syntax
+
New checks
^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/index.rst b/clang-tools-extra/docs/clang-tidy/index.rst
index b7a366e874130e..7ad35aceb094f9 100644
--- a/clang-tools-extra/docs/clang-tidy/index.rst
+++ b/clang-tools-extra/docs/clang-tidy/index.rst
@@ -292,6 +292,18 @@ An overview of all the command-line options:
Checks - Same as '--checks'. Additionally, the list of
globs can be specified as a list instead of a
string.
+ ClangQueryChecks - List of key-value pairs. Key specifies a name
+ of the new check and value specifies a list
+ of matchers in the form of clang-query
+ syntax. Example:
+ ClangQueryChecks:
+ custom-check: |
+ let matcher varDecl(
+ hasTypeLoc(
+ typeLoc().bind("Custom message")
+ )
+ )
+ match matcher
ExcludeHeaderFilterRegex - Same as '--exclude-header-filter'.
ExtraArgs - Same as '--extra-arg'.
ExtraArgsBefore - Same as '--extra-arg-before'.
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/query-checks.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/query-checks.cpp
new file mode 100644
index 00000000000000..e84516a6977b77
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/query-checks.cpp
@@ -0,0 +1,53 @@
+// DEFINE: %{custom-call-yaml} = custom-call: 'm callExpr().bind(\"Custom message\")'
+//
+// DEFINE: %{custom-let-call-yaml} = custom-let-call: \" \
+// DEFINE: let expr varDecl( \
+// DEFINE: hasType(asString(\\\"long long\\\")), \
+// DEFINE: hasTypeLoc(typeLoc().bind(\\\"Let message\\\")) \
+// DEFINE: ) \n \
+// DEFINE: match expr\"
+//
+// DEFINE: %{full-config} = "{ClangQueryChecks: {%{custom-call-yaml},%{custom-let-call-yaml}}}"
+
+//Check single match expression
+// RUN: clang-tidy %s -checks='-*, custom-*' \
+// RUN: -config="{ClangQueryChecks: {%{custom-call-yaml}}}" \
+// RUN: -- | FileCheck %s -check-prefix=CHECK-CUSTOM-CALL
+
+void a() {
+}
+
+// CHECK-CUSTOM-CALL: warning: Custom message [custom-call]
+// CHECK-CUSTOM-CALL-NEXT: a();{{$}}
+void b() {
+ a();
+}
+
+//Check let with match expression
+// RUN: clang-tidy %s -checks='-*, custom-*' \
+// RUN: -config="{ClangQueryChecks: {%{custom-let-call-yaml}}}" \
+// RUN: -- | FileCheck %s -check-prefix=CHECK-CUSTOM-LET
+void c() {
+ // CHECK-CUSTOM-LET: warning: Let message [custom-let-call]
+ // CHECK-CUSTOM-LET-NEXT: long long test_long_long = 0;{{$}}
+ long long test_long_long_nolint = 0; //NOLINT(custom-let-call)
+ long long test_long_long = 0;
+}
+
+//Check multiple checks in one config
+// RUN: clang-tidy %s -checks='-*, custom-*' \
+// RUN: -config=%{full-config} \
+// RUN: -- | FileCheck %s -check-prefixes=CHECK-CUSTOM-CALL,CHECK-CUSTOM-LET
+
+//Check multiple checks in one config but only one enabled
+// RUN: clang-tidy %s -checks='-*, custom-call' \
+// RUN: -config=%{full-config} \
+// RUN: -- | FileCheck %s -check-prefixes=CHECK-CUSTOM-CALL --implicit-check-not warning:
+
+//Check config dump
+// RUN: clang-tidy -dump-config -checks='-*, custom-*' \
+// RUN: -config=%{full-config} \
+// RUN: -- | FileCheck %s -check-prefix=CHECK-CONFIG
+// CHECK-CONFIG: ClangQueryChecks:
+// CHECK-CONFIG-DAG: custom-let-call:
+// CHECK-CONFIG-DAG: custom-call: |{{$[[:space:]]}} m callExpr().bind("Custom message")
|
|
@llvm/pr-subscribers-clang-tidy Author: None (DeNiCoN) ChangesThis addresses the #107680. This pull request adds new config option that allows to define new checks using the matchers syntax by incorporating clang-query parser Example: Checks: -*,custom-*
ClangQueryChecks:
custom-math: |
let expr callExpr(
callee(functionDecl(
hasAnyName("acos", "asin", "atan", "atan2", "cos", "sin", "tan", "cosh", "sinh", "tanh", "ctan"),
anyOf(
hasDeclContext(namespaceDecl(hasName("std"))),
unless(hasParent(namespaceDecl()))
)
))
).bind("Please use different math functions")
match expr
custom-sort: |
match callExpr(
callee(functionDecl(
hasAnyName("sort", "nth_element", "partial_sort", "partition"),
anyOf(
hasDeclContext(namespaceDecl(hasName("std"))),
unless(hasParent(namespaceDecl()))
)
))
).bind("Please use different sort functions")
custom-long: |
match varDecl(
hasType(asString("long")),
hasTypeLoc(typeLoc().bind("Please use int instead of long"))
)Unimplemented ideas
Some thoughts on further possible workIf transformers parser will be implemented in the future(it seems like in the past there were some work on it being done by @ymand) it can be added to clang-query and as a consequence it can be made that custom checks defined by this new option will support automatic fixes being provided Full diff: https://github.com/llvm/llvm-project/pull/123734.diff 10 Files Affected:
diff --git a/clang-tools-extra/clang-tidy/CMakeLists.txt b/clang-tools-extra/clang-tidy/CMakeLists.txt
index 93117cf1d6373a..76585b012d1742 100644
--- a/clang-tools-extra/clang-tidy/CMakeLists.txt
+++ b/clang-tools-extra/clang-tidy/CMakeLists.txt
@@ -11,6 +11,7 @@ include_directories(BEFORE ${CMAKE_CURRENT_BINARY_DIR})
add_clang_library(clangTidy STATIC
ClangTidy.cpp
ClangTidyCheck.cpp
+ ClangQueryCheck.cpp
ClangTidyModule.cpp
ClangTidyDiagnosticConsumer.cpp
ClangTidyOptions.cpp
@@ -38,6 +39,7 @@ clang_target_link_libraries(clangTidy
clangSerialization
clangTooling
clangToolingCore
+ clangQuery
)
if(CLANG_TIDY_ENABLE_STATIC_ANALYZER)
diff --git a/clang-tools-extra/clang-tidy/ClangQueryCheck.cpp b/clang-tools-extra/clang-tidy/ClangQueryCheck.cpp
new file mode 100644
index 00000000000000..a9f46116f70899
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/ClangQueryCheck.cpp
@@ -0,0 +1,30 @@
+//===--- ClangQueryCheck.cpp - 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 "ClangQueryCheck.h"
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+
+using namespace clang::ast_matchers;
+
+namespace clang::tidy::misc {
+
+void ClangQueryCheck::registerMatchers(MatchFinder *Finder) {
+ for (const auto &Matcher : Matchers) {
+ bool Ok = Finder->addDynamicMatcher(Matcher, this);
+ assert(Ok && "Expected to get top level matcher from query parser");
+ }
+}
+
+void ClangQueryCheck::check(const MatchFinder::MatchResult &Result) {
+ auto Map = Result.Nodes.getMap();
+ for (const auto &[k, v] : Map) {
+ diag(v.getSourceRange().getBegin(), k) << v.getSourceRange();
+ }
+}
+
+} // namespace clang::tidy::misc
diff --git a/clang-tools-extra/clang-tidy/ClangQueryCheck.h b/clang-tools-extra/clang-tidy/ClangQueryCheck.h
new file mode 100644
index 00000000000000..3c3c7029720687
--- /dev/null
+++ b/clang-tools-extra/clang-tidy/ClangQueryCheck.h
@@ -0,0 +1,43 @@
+//===--- ClangQueryCheck.h - clang-tidy --------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGQUERYCHECK_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGQUERYCHECK_H
+
+#include "ClangTidyCheck.h"
+#include "clang/ASTMatchers/Dynamic/VariantValue.h"
+#include <vector>
+
+namespace clang::query {
+class QuerySession;
+} // namespace clang::query
+
+namespace clang::tidy::misc {
+
+/// A check that matches a given matchers printing their binds as warnings
+class ClangQueryCheck : public ClangTidyCheck {
+ using MatcherVec = std::vector<ast_matchers::dynamic::DynTypedMatcher>;
+
+public:
+ ClangQueryCheck(StringRef Name, ClangTidyContext *Context,
+ MatcherVec Matchers)
+ : ClangTidyCheck(Name, Context), Matchers(std::move(Matchers)) {}
+
+ void registerMatchers(ast_matchers::MatchFinder *Finder) override;
+ void check(const ast_matchers::MatchFinder::MatchResult &Result) override;
+ bool isLanguageVersionSupported(const LangOptions &LangOpts) const override {
+ return LangOpts.CPlusPlus;
+ }
+
+private:
+ MatcherVec Matchers;
+};
+
+} // namespace clang::tidy::misc
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGQUERYCHECK_H
diff --git a/clang-tools-extra/clang-tidy/ClangTidy.cpp b/clang-tools-extra/clang-tidy/ClangTidy.cpp
index 959b11777e88d4..75466b6612b253 100644
--- a/clang-tools-extra/clang-tidy/ClangTidy.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidy.cpp
@@ -15,6 +15,7 @@
//===----------------------------------------------------------------------===//
#include "ClangTidy.h"
+#include "ClangQueryCheck.h"
#include "ClangTidyCheck.h"
#include "ClangTidyDiagnosticConsumer.h"
#include "ClangTidyModuleRegistry.h"
@@ -350,6 +351,13 @@ ClangTidyASTConsumerFactory::ClangTidyASTConsumerFactory(
std::unique_ptr<ClangTidyModule> Module = E.instantiate();
Module->addCheckFactories(*CheckFactories);
}
+
+ for (const auto &[k, v] : Context.getOptions().ClangQueryChecks) {
+ CheckFactories->registerCheckFactory(k, [v](StringRef Name,
+ ClangTidyContext *Context) {
+ return std::make_unique<misc::ClangQueryCheck>(Name, Context, v.Matchers);
+ });
+ }
}
#if CLANG_TIDY_ENABLE_STATIC_ANALYZER
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
index 8bac6f161fa05b..0d6edb88a1d603 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
@@ -7,6 +7,8 @@
//===----------------------------------------------------------------------===//
#include "ClangTidyOptions.h"
+#include "../clang-query/Query.h"
+#include "../clang-query/QueryParser.h"
#include "ClangTidyModuleRegistry.h"
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/SmallString.h"
@@ -126,6 +128,83 @@ void yamlize(IO &IO, ClangTidyOptions::OptionMap &Val, bool,
}
}
+std::vector<clang::ast_matchers::dynamic::DynTypedMatcher>
+processQuerySource(IO &IO, StringRef SourceRef,
+ clang::query::QuerySession &QS) {
+ namespace query = clang::query;
+ std::vector<clang::ast_matchers::dynamic::DynTypedMatcher> Matchers;
+
+ while (!SourceRef.empty()) {
+ query::QueryRef Q = query::QueryParser::parse(SourceRef, QS);
+ switch (Q->Kind) {
+ case query::QK_Match: {
+ const auto &MatchQuerry = llvm::cast<query::MatchQuery>(*Q);
+ Matchers.push_back(MatchQuerry.Matcher);
+ break;
+ }
+ case query::QK_Let: {
+ const auto &LetQuerry = llvm::cast<query::LetQuery>(*Q);
+ LetQuerry.run(llvm::errs(), QS);
+ break;
+ }
+ case query::QK_Invalid: {
+ const auto &InvalidQuerry = llvm::cast<query::InvalidQuery>(*Q);
+ for (const auto &Line : llvm::split(InvalidQuerry.ErrStr, "\n")) {
+ IO.setError(Line);
+ }
+ break;
+ }
+ // FIXME FileQuerry should also be supported, but what to do with relative
+ // paths?
+ case query::QK_File:
+ case query::QK_DisableOutputKind:
+ case query::QK_EnableOutputKind:
+ case query::QK_SetOutputKind:
+ case query::QK_SetTraversalKind:
+ case query::QK_Help:
+ case query::QK_NoOp:
+ case query::QK_Quit:
+ case query::QK_SetBool: {
+ IO.setError("unsupported querry kind");
+ }
+ }
+ SourceRef = Q->RemainingContent;
+ }
+
+ return Matchers;
+}
+
+template <>
+void yamlize(IO &IO, ClangTidyOptions::QueryCheckMap &Val, bool,
+ EmptyContext &Ctx) {
+ IO.beginMapping();
+ if (IO.outputting()) {
+ for (auto &[k, v] : Val) {
+ IO.mapRequired(k.data(), v);
+ }
+ } else {
+ for (StringRef Key : IO.keys()) {
+ IO.mapRequired(Key.data(), Val[Key]);
+ }
+ }
+ IO.endMapping();
+}
+
+template <>
+void yamlize(IO &IO, ClangTidyOptions::QueryCheckValue &Val, bool,
+ EmptyContext &Ctx) {
+ if (IO.outputting()) {
+ StringRef SourceRef = Val.Source;
+ IO.blockScalarString(SourceRef);
+ } else {
+ StringRef SourceRef;
+ IO.blockScalarString(SourceRef);
+ Val.Source = SourceRef;
+ clang::query::QuerySession QS({});
+ Val.Matchers = processQuerySource(IO, SourceRef, QS);
+ }
+}
+
struct ChecksVariant {
std::optional<std::string> AsString;
std::optional<std::vector<std::string>> AsVector;
@@ -181,6 +260,7 @@ template <> struct MappingTraits<ClangTidyOptions> {
IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
IO.mapOptional("UseColor", Options.UseColor);
IO.mapOptional("SystemHeaders", Options.SystemHeaders);
+ IO.mapOptional("ClangQueryChecks", Options.ClangQueryChecks);
}
};
@@ -249,6 +329,10 @@ ClangTidyOptions &ClangTidyOptions::mergeWith(const ClangTidyOptions &Other,
ClangTidyValue(KeyValue.getValue().Value,
KeyValue.getValue().Priority + Order));
}
+
+ for (const auto &KeyValue : Other.ClangQueryChecks) {
+ ClangQueryChecks.insert_or_assign(KeyValue.getKey(), KeyValue.getValue());
+ }
return *this;
}
diff --git a/clang-tools-extra/clang-tidy/ClangTidyOptions.h b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
index dd78c570d25d9b..3a015ed5163cdc 100644
--- a/clang-tools-extra/clang-tidy/ClangTidyOptions.h
+++ b/clang-tools-extra/clang-tidy/ClangTidyOptions.h
@@ -9,6 +9,7 @@
#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYOPTIONS_H
#define LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYOPTIONS_H
+#include "clang/ASTMatchers/Dynamic/VariantValue.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringMap.h"
@@ -126,8 +127,15 @@ struct ClangTidyOptions {
using StringPair = std::pair<std::string, std::string>;
using OptionMap = llvm::StringMap<ClangTidyValue>;
+ struct QueryCheckValue {
+ std::string Source;
+ std::vector<ast_matchers::dynamic::DynTypedMatcher> Matchers;
+ };
+ using QueryCheckMap = llvm::StringMap<QueryCheckValue>;
+
/// Key-value mapping used to store check-specific options.
OptionMap CheckOptions;
+ QueryCheckMap ClangQueryChecks;
using ArgList = std::vector<std::string>;
diff --git a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
index fa8887e4639b4a..c60e6fcb8c5fa7 100644
--- a/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
+++ b/clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
@@ -60,6 +60,18 @@ Configuration files:
Checks - Same as '--checks'. Additionally, the list of
globs can be specified as a list instead of a
string.
+ ClangQueryChecks - List of key-value pairs. Key specifies a name
+ of the new check and value specifies a list
+ of matchers in the form of clang-query
+ syntax. Example:
+ ClangQueryChecks:
+ custom-check: |
+ let matcher varDecl(
+ hasTypeLoc(
+ typeLoc().bind("Custom message")
+ )
+ )
+ match matcher
ExcludeHeaderFilterRegex - Same as '--exclude-header-filter'.
ExtraArgs - Same as '--extra-arg'.
ExtraArgsBefore - Same as '--extra-arg-before'.
@@ -483,7 +495,8 @@ static StringRef closest(StringRef Value, const StringSet<> &Allowed) {
return Closest;
}
-static constexpr StringLiteral VerifyConfigWarningEnd = " [-verify-config]\n";
+static constexpr llvm::StringLiteral VerifyConfigWarningEnd =
+ " [-verify-config]\n";
static bool verifyChecks(const StringSet<> &AllChecks, StringRef CheckGlob,
StringRef Source) {
diff --git a/clang-tools-extra/docs/ReleaseNotes.rst b/clang-tools-extra/docs/ReleaseNotes.rst
index 33a452f525f763..b845b32569872c 100644
--- a/clang-tools-extra/docs/ReleaseNotes.rst
+++ b/clang-tools-extra/docs/ReleaseNotes.rst
@@ -140,6 +140,9 @@ Improvements to clang-tidy
:doc:`readability-redundant-access-specifiers <clang-tidy/checks/readability/redundant-access-specifiers>`, CheckFirstDeclaration
:doc:`readability-redundant-casting <clang-tidy/checks/readability/redundant-casting>`, IgnoreTypeAliases
+- New :program:`clang-tidy` config property `ClangQueryChecks` that allows adding
+ custom checks based on a :program:`clang-query` syntax
+
New checks
^^^^^^^^^^
diff --git a/clang-tools-extra/docs/clang-tidy/index.rst b/clang-tools-extra/docs/clang-tidy/index.rst
index b7a366e874130e..7ad35aceb094f9 100644
--- a/clang-tools-extra/docs/clang-tidy/index.rst
+++ b/clang-tools-extra/docs/clang-tidy/index.rst
@@ -292,6 +292,18 @@ An overview of all the command-line options:
Checks - Same as '--checks'. Additionally, the list of
globs can be specified as a list instead of a
string.
+ ClangQueryChecks - List of key-value pairs. Key specifies a name
+ of the new check and value specifies a list
+ of matchers in the form of clang-query
+ syntax. Example:
+ ClangQueryChecks:
+ custom-check: |
+ let matcher varDecl(
+ hasTypeLoc(
+ typeLoc().bind("Custom message")
+ )
+ )
+ match matcher
ExcludeHeaderFilterRegex - Same as '--exclude-header-filter'.
ExtraArgs - Same as '--extra-arg'.
ExtraArgsBefore - Same as '--extra-arg-before'.
diff --git a/clang-tools-extra/test/clang-tidy/infrastructure/query-checks.cpp b/clang-tools-extra/test/clang-tidy/infrastructure/query-checks.cpp
new file mode 100644
index 00000000000000..e84516a6977b77
--- /dev/null
+++ b/clang-tools-extra/test/clang-tidy/infrastructure/query-checks.cpp
@@ -0,0 +1,53 @@
+// DEFINE: %{custom-call-yaml} = custom-call: 'm callExpr().bind(\"Custom message\")'
+//
+// DEFINE: %{custom-let-call-yaml} = custom-let-call: \" \
+// DEFINE: let expr varDecl( \
+// DEFINE: hasType(asString(\\\"long long\\\")), \
+// DEFINE: hasTypeLoc(typeLoc().bind(\\\"Let message\\\")) \
+// DEFINE: ) \n \
+// DEFINE: match expr\"
+//
+// DEFINE: %{full-config} = "{ClangQueryChecks: {%{custom-call-yaml},%{custom-let-call-yaml}}}"
+
+//Check single match expression
+// RUN: clang-tidy %s -checks='-*, custom-*' \
+// RUN: -config="{ClangQueryChecks: {%{custom-call-yaml}}}" \
+// RUN: -- | FileCheck %s -check-prefix=CHECK-CUSTOM-CALL
+
+void a() {
+}
+
+// CHECK-CUSTOM-CALL: warning: Custom message [custom-call]
+// CHECK-CUSTOM-CALL-NEXT: a();{{$}}
+void b() {
+ a();
+}
+
+//Check let with match expression
+// RUN: clang-tidy %s -checks='-*, custom-*' \
+// RUN: -config="{ClangQueryChecks: {%{custom-let-call-yaml}}}" \
+// RUN: -- | FileCheck %s -check-prefix=CHECK-CUSTOM-LET
+void c() {
+ // CHECK-CUSTOM-LET: warning: Let message [custom-let-call]
+ // CHECK-CUSTOM-LET-NEXT: long long test_long_long = 0;{{$}}
+ long long test_long_long_nolint = 0; //NOLINT(custom-let-call)
+ long long test_long_long = 0;
+}
+
+//Check multiple checks in one config
+// RUN: clang-tidy %s -checks='-*, custom-*' \
+// RUN: -config=%{full-config} \
+// RUN: -- | FileCheck %s -check-prefixes=CHECK-CUSTOM-CALL,CHECK-CUSTOM-LET
+
+//Check multiple checks in one config but only one enabled
+// RUN: clang-tidy %s -checks='-*, custom-call' \
+// RUN: -config=%{full-config} \
+// RUN: -- | FileCheck %s -check-prefixes=CHECK-CUSTOM-CALL --implicit-check-not warning:
+
+//Check config dump
+// RUN: clang-tidy -dump-config -checks='-*, custom-*' \
+// RUN: -config=%{full-config} \
+// RUN: -- | FileCheck %s -check-prefix=CHECK-CONFIG
+// CHECK-CONFIG: ClangQueryChecks:
+// CHECK-CONFIG-DAG: custom-let-call:
+// CHECK-CONFIG-DAG: custom-call: |{{$[[:space:]]}} m callExpr().bind("Custom message")
|
HerrCai0907
reviewed
Jan 21, 2025
It could be. But this will only be a single check. Having the ability to define multiple checks allows to have more fine control over them using NOLINT. If multiple checks are defined in a single ClangTidyCheck then it should have the ability to report diagnostics under different names, but ClangTidyCheck base class allows to report diagnostics only with a single name passed in the constructor so some changes would have to be made in the base class in this caseAnd I think the approach with a single ClangTidyCheck and multiple checks defined will require some changes in config parsing too |
PiotrZSL
requested changes
Jan 21, 2025
There was a problem hiding this comment.
Additionally:
- more detailed documentation needed (entire section in documentation)
- i would name it CustomChecks instead
- need protection against check name conflicts, and maybe some protection for adding checks for current modules ? Maybe enforce "custom-" as prefix ?
Idea is fine, require some work.
| @@ -140,6 +140,9 @@ Improvements to clang-tidy | |||
| :doc:`readability-redundant-access-specifiers <clang-tidy/checks/readability/redundant-access-specifiers>`, CheckFirstDeclaration | |||
| :doc:`readability-redundant-casting <clang-tidy/checks/readability/redundant-casting>`, IgnoreTypeAliases | |||
|
|
|||
| - New :program:`clang-tidy` config property `ClangQueryChecks` that allows adding | |||
There was a problem hiding this comment.
merge with release notes in line 120
| @@ -292,6 +292,18 @@ An overview of all the command-line options: | |||
| Checks - Same as '--checks'. Additionally, the list of | |||
| globs can be specified as a list instead of a | |||
| string. | |||
| ClangQueryChecks - List of key-value pairs. Key specifies a name | |||
There was a problem hiding this comment.
this should be documented in separate section, thats not a good place for example
| @@ -38,6 +39,7 @@ clang_target_link_libraries(clangTidy | |||
| clangSerialization | |||
| clangTooling | |||
| clangToolingCore | |||
| clangQuery | |||
There was a problem hiding this comment.
this introduces new dependency,
how it will impact for example clangd ?
will those checks work there ?
There was a problem hiding this comment.
it is good to compare clangtidy binary size change before and after, since clangquery is not a small binary.
There was a problem hiding this comment.
how it will impact for example clangd ?
will those checks work there ?
I will need to check that. It seems it might not see those checks
clangtidy binary size change before and after
With the following cmake flags
-DCMAKE_EXPORT_COMPILE_COMMANDS=ON -DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_PROJECTS="clang;clang-tools-extra;lldb" -DLLVM_OPTIMIZED_TABLEGEN=ON -DLLVM_USE_SPLIT_DWARF=ON -DLLVM_USE_LINKER=lld
clang-query 52M -> 52M
clang-tidy 133M -> 135M
clangd 182M -> 184M
| void registerMatchers(ast_matchers::MatchFinder *Finder) override; | ||
| void check(const ast_matchers::MatchFinder::MatchResult &Result) override; | ||
| bool isLanguageVersionSupported(const LangOptions &LangOpts) const override { | ||
| return LangOpts.CPlusPlus; |
There was a problem hiding this comment.
unnecessary limitation, maybe this should be an "config option"
| // RUN: -- | FileCheck %s -check-prefix=CHECK-CONFIG | ||
| // CHECK-CONFIG: ClangQueryChecks: | ||
| // CHECK-CONFIG-DAG: custom-let-call: | ||
| // CHECK-CONFIG-DAG: custom-call: |{{$[[:space:]]}} m callExpr().bind("Custom message") |
There was a problem hiding this comment.
missing test for --verify-config and --list-checks
| custom-check: | | ||
| let matcher varDecl( | ||
| hasTypeLoc( | ||
| typeLoc().bind("Custom message") |
There was a problem hiding this comment.
what if i want to get "name of fuction" in output message ?
or write check with multiple "bind" by simply using equalsBoundNode ?
Or generate "note"
There was a problem hiding this comment.
"name of fuction" in output message
This is possible to get with transformers. There are some examples that construct fixes and messages out of binds in documentation
llvm::StringRef s = "str";
makeRule(
cxxMemberCallExpr(
on(expr(hasType(namedDecl(hasName("std::string"))))
.bind(s)),
callee(cxxMethodDecl(hasName("size")))),
changeTo(cat("Size(", node(s), ")")),
cat("Method ``size`` is deprecated in favor of free function ``Size``"));But a parser for them is not implemented
using equalsBoundNode
I didn't thought about that. Currently it should work but will print all binds.
Maybe some rule can be introduced so that binds that start with underscore are not printed
generate "note"
It will be possible with transformers too
makeRule(returnStmt(),
note(node(RootID), cat("some note")),
cat("message")),I think that currently matchers can be left as a basic system that will only generate warnings from the strings inside binds. And later this feature can be expanded with transformers
|
Additionally, consider implementing this as an "module" (like bugprone), simply implement factory that would look into config options that start with "custom-" and then parse options like: Source, Message, Language, and based on that construct checks, gain is that dependency on clang-query could be turn on/off in cmake file, and it could only result in registering or not that module. Also extending that functionality would be easier with things like TraverseMode and so on... |
HerrCai0907
reviewed
Jan 21, 2025
There was a problem hiding this comment.
It is good enough as a mvp for this feature.
But consider some features mentioned by @PiotrZSL. Could this yaml format change to array and each array element contains check name and check matches. then is easier to add new optional field.
|
|
||
| void ClangQueryCheck::check(const MatchFinder::MatchResult &Result) { | ||
| auto Map = Result.Nodes.getMap(); | ||
| for (const auto &[k, v] : Map) { |
There was a problem hiding this comment.
avoid to use k, v and map. it is hard to understand what the meaning of this variable.
|
Hello @DeNiCoN, Are you still working on this patch? If not, I hope I can take over this patch to finally finish it. |
Hello @HerrCai0907. I'm not actively working on it. You can take over Some notes: I've also found that --list-checks would only list custom checks for the first file specified And --verify-config does not respect custom checks clangd seems to not see custom checks because it uses ClangTidyModuleRegistry to get all checks
|
|
Just FYI: Very useful feature, I hope it gets into clang-tidy at some point. At the moment we use backported patch for clang-tidy-16 because of our internal requirements, and we have made some custom matchers for our CI to prohibit merges with bad things like "long" types. Let us know if you have any questions. |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This addresses the #107680.
This pull request adds new config option that allows to define new checks using the matchers syntax by incorporating clang-query parser
Example:
Unimplemented ideas
Some thoughts on further possible work
If transformers parser will be implemented in the future(it seems like in the past there were some work on it being done by @ymand) it can be added to clang-query and as a consequence it can be made that custom checks defined by this new option will support automatic fixes being provided