Skip to content

[clang-tidy] Add ClangQueryChecks config option #123734

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang-tools-extra/clang-tidy/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -38,6 +39,7 @@ clang_target_link_libraries(clangTidy
clangSerialization
clangTooling
clangToolingCore
clangQuery
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this introduces new dependency,
how it will impact for example clangd ?
will those checks work there ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is good to compare clangtidy binary size change before and after, since clangquery is not a small binary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

)

if(CLANG_TIDY_ENABLE_STATIC_ANALYZER)
Expand Down
30 changes: 30 additions & 0 deletions clang-tools-extra/clang-tidy/ClangQueryCheck.cpp
Original file line number Diff line number Diff line change
@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid to use k, v and map. it is hard to understand what the meaning of this variable.

diag(v.getSourceRange().getBegin(), k) << v.getSourceRange();
}
}

} // namespace clang::tidy::misc
43 changes: 43 additions & 0 deletions clang-tools-extra/clang-tidy/ClangQueryCheck.h
Original file line number Diff line number Diff line change
@@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unnecessary limitation, maybe this should be an "config option"

}

private:
MatcherVec Matchers;
};

} // namespace clang::tidy::misc

#endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGQUERYCHECK_H
8 changes: 8 additions & 0 deletions clang-tools-extra/clang-tidy/ClangTidy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
//===----------------------------------------------------------------------===//

#include "ClangTidy.h"
#include "ClangQueryCheck.h"
#include "ClangTidyCheck.h"
#include "ClangTidyDiagnosticConsumer.h"
#include "ClangTidyModuleRegistry.h"
Expand Down Expand Up @@ -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
Expand Down
84 changes: 84 additions & 0 deletions clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}
};

Expand Down Expand Up @@ -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;
}

Expand Down
8 changes: 8 additions & 0 deletions clang-tools-extra/clang-tidy/ClangTidyOptions.h
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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>;

Expand Down
15 changes: 14 additions & 1 deletion clang-tools-extra/clang-tidy/tool/ClangTidyMain.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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'.
Expand Down Expand Up @@ -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) {
Expand Down
3 changes: 3 additions & 0 deletions clang-tools-extra/docs/ReleaseNotes.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

merge with release notes in line 120

custom checks based on a :program:`clang-query` syntax

New checks
^^^^^^^^^^

Expand Down
12 changes: 12 additions & 0 deletions clang-tools-extra/docs/clang-tidy/index.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be documented in separate section, thats not a good place for example

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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Copy link
Contributor Author

@DeNiCoN DeNiCoN Jan 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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

)
)
match matcher
ExcludeHeaderFilterRegex - Same as '--exclude-header-filter'.
ExtraArgs - Same as '--extra-arg'.
ExtraArgsBefore - Same as '--extra-arg-before'.
Expand Down
53 changes: 53 additions & 0 deletions clang-tools-extra/test/clang-tidy/infrastructure/query-checks.cpp
Original file line number Diff line number Diff line change
@@ -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")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing test for --verify-config and --list-checks

Loading