Skip to content

[clang-tidy] support query based custom check #131804

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 23 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 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 @@ -58,6 +58,7 @@ add_subdirectory(bugprone)
add_subdirectory(cert)
add_subdirectory(concurrency)
add_subdirectory(cppcoreguidelines)
add_subdirectory(custom)
add_subdirectory(darwin)
add_subdirectory(fuchsia)
add_subdirectory(google)
Expand Down Expand Up @@ -85,6 +86,7 @@ set(ALL_CLANG_TIDY_CHECKS
clangTidyCERTModule
clangTidyConcurrencyModule
clangTidyCppCoreGuidelinesModule
clangTidyCustomModule
clangTidyDarwinModule
clangTidyFuchsiaModule
clangTidyGoogleModule
Expand Down
9 changes: 8 additions & 1 deletion clang-tools-extra/clang-tidy/ClangTidy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,11 @@ LLVM_INSTANTIATE_REGISTRY(clang::tidy::ClangTidyModuleRegistry)

namespace clang::tidy {

namespace custom {
extern void registerCustomChecks(ClangTidyOptions const &O,
ClangTidyCheckFactories &Factories);
} // namespace custom

namespace {
#if CLANG_TIDY_ENABLE_STATIC_ANALYZER
static const char *AnalyzerCheckNamePrefix = "clang-analyzer-";
Expand Down Expand Up @@ -346,6 +351,7 @@ ClangTidyASTConsumerFactory::ClangTidyASTConsumerFactory(
IntrusiveRefCntPtr<llvm::vfs::OverlayFileSystem> OverlayFS)
: Context(Context), OverlayFS(std::move(OverlayFS)),
CheckFactories(new ClangTidyCheckFactories) {
custom::registerCustomChecks(Context.getOptions(), *CheckFactories);
for (ClangTidyModuleRegistry::entry E : ClangTidyModuleRegistry::entries()) {
std::unique_ptr<ClangTidyModule> Module = E.instantiate();
Module->addCheckFactories(*CheckFactories);
Expand Down Expand Up @@ -416,7 +422,7 @@ ClangTidyASTConsumerFactory::createASTConsumer(
.getCurrentWorkingDirectory();
if (WorkingDir)
Context.setCurrentBuildDirectory(WorkingDir.get());

custom::registerCustomChecks(Context.getOptions(), *CheckFactories);
std::vector<std::unique_ptr<ClangTidyCheck>> Checks =
CheckFactories->createChecksForLanguage(&Context);

Expand Down Expand Up @@ -659,6 +665,7 @@ getAllChecksAndOptions(bool AllowEnablingAnalyzerAlphaCheckers) {
std::make_unique<DefaultOptionsProvider>(ClangTidyGlobalOptions(), Opts),
AllowEnablingAnalyzerAlphaCheckers);
ClangTidyCheckFactories Factories;
custom::registerCustomChecks(Context.getOptions(), Factories);
for (const ClangTidyModuleRegistry::entry &Module :
ClangTidyModuleRegistry::entries()) {
Module.instantiate()->addCheckFactories(Factories);
Expand Down
5 changes: 5 additions & 0 deletions clang-tools-extra/clang-tidy/ClangTidyForceLinker.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ extern volatile int CppCoreGuidelinesModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED CppCoreGuidelinesModuleAnchorDestination =
CppCoreGuidelinesModuleAnchorSource;

// This anchor is used to force the linker to link the CustomModule.
extern volatile int CustomModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED CustomModuleAnchorDestination =
CustomModuleAnchorSource;

// This anchor is used to force the linker to link the DarwinModule.
extern volatile int DarwinModuleAnchorSource;
static int LLVM_ATTRIBUTE_UNUSED DarwinModuleAnchorDestination =
Expand Down
2 changes: 2 additions & 0 deletions clang-tools-extra/clang-tidy/ClangTidyModule.h
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,8 @@ class ClangTidyCheckFactories {
});
}

void erase(llvm::StringRef CheckName) { Factories.erase(CheckName); }

/// Create instances of checks that are enabled.
std::vector<std::unique_ptr<ClangTidyCheck>>
createChecks(ClangTidyContext *Context) const;
Expand Down
57 changes: 53 additions & 4 deletions clang-tools-extra/clang-tidy/ClangTidyOptions.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -8,12 +8,12 @@

#include "ClangTidyOptions.h"
#include "ClangTidyModuleRegistry.h"
#include "clang/Basic/DiagnosticIDs.h"
#include "clang/Basic/LLVM.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringExtras.h"
#include "llvm/Support/Debug.h"
#include "llvm/Support/Errc.h"
#include "llvm/Support/ErrorOr.h"
#include "llvm/Support/FileSystem.h"
#include "llvm/Support/MemoryBufferRef.h"
#include "llvm/Support/Path.h"
#include "llvm/Support/YAMLTraits.h"
Expand Down Expand Up @@ -72,7 +72,8 @@ struct NOptionMap {
NOptionMap(IO &, const ClangTidyOptions::OptionMap &OptionMap) {
Options.reserve(OptionMap.size());
for (const auto &KeyValue : OptionMap)
Options.emplace_back(std::string(KeyValue.getKey()), KeyValue.getValue().Value);
Options.emplace_back(std::string(KeyValue.getKey()),
KeyValue.getValue().Value);
}
ClangTidyOptions::OptionMap denormalize(IO &) {
ClangTidyOptions::OptionMap Map;
Expand Down Expand Up @@ -126,6 +127,52 @@ void yamlize(IO &IO, ClangTidyOptions::OptionMap &Val, bool,
}
}

namespace {
struct MultiLineString {
std::string &S;
};
} // namespace

template <> struct BlockScalarTraits<MultiLineString> {
static void output(const MultiLineString &S, void *Ctxt, raw_ostream &OS) {
OS << S.S;
}
static StringRef input(StringRef Str, void *Ctxt, MultiLineString &S) {
S.S = Str;
return "";
}
};

template <> struct ScalarEnumerationTraits<clang::DiagnosticIDs::Level> {
static void enumeration(IO &IO, clang::DiagnosticIDs::Level &Level) {
IO.enumCase(Level, "Error", clang::DiagnosticIDs::Level::Error);
IO.enumCase(Level, "Warning", clang::DiagnosticIDs::Level::Warning);
IO.enumCase(Level, "Note", clang::DiagnosticIDs::Level::Note);
}
};
template <> struct SequenceElementTraits<ClangTidyOptions::CustomCheckDiag> {
static const bool flow = false;
};
template <> struct MappingTraits<ClangTidyOptions::CustomCheckDiag> {
static void mapping(IO &IO, ClangTidyOptions::CustomCheckDiag &D) {
IO.mapRequired("BindName", D.BindName);
MultiLineString MLS{D.Message};
IO.mapRequired("Message", MLS);
IO.mapOptional("Level", D.Level);
}
};
template <> struct SequenceElementTraits<ClangTidyOptions::CustomCheckValue> {
static const bool flow = false;
};
template <> struct MappingTraits<ClangTidyOptions::CustomCheckValue> {
static void mapping(IO &IO, ClangTidyOptions::CustomCheckValue &V) {
IO.mapRequired("Name", V.Name);
MultiLineString MLS{V.Query};
IO.mapRequired("Query", MLS);
IO.mapRequired("Diagnostic", V.Diags);
}
};

struct ChecksVariant {
std::optional<std::string> AsString;
std::optional<std::vector<std::string>> AsVector;
Expand Down Expand Up @@ -181,6 +228,7 @@ template <> struct MappingTraits<ClangTidyOptions> {
IO.mapOptional("InheritParentConfig", Options.InheritParentConfig);
IO.mapOptional("UseColor", Options.UseColor);
IO.mapOptional("SystemHeaders", Options.SystemHeaders);
IO.mapOptional("CustomChecks", Options.CustomChecks);
}
};

Expand Down Expand Up @@ -242,7 +290,8 @@ ClangTidyOptions &ClangTidyOptions::mergeWith(const ClangTidyOptions &Other,
overrideValue(UseColor, Other.UseColor);
mergeVectors(ExtraArgs, Other.ExtraArgs);
mergeVectors(ExtraArgsBefore, Other.ExtraArgsBefore);

// FIXME: how to handle duplicate names check?
mergeVectors(CustomChecks, Other.CustomChecks);
for (const auto &KeyValue : Other.CheckOptions) {
CheckOptions.insert_or_assign(
KeyValue.getKey(),
Expand Down
15 changes: 15 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/Basic/DiagnosticIDs.h"
#include "llvm/ADT/IntrusiveRefCntPtr.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringMap.h"
Expand All @@ -17,6 +18,7 @@
#include "llvm/Support/MemoryBufferRef.h"
#include "llvm/Support/VirtualFileSystem.h"
#include <functional>
#include <map>
#include <optional>
#include <string>
#include <system_error>
Expand Down Expand Up @@ -129,6 +131,19 @@ struct ClangTidyOptions {
/// Key-value mapping used to store check-specific options.
OptionMap CheckOptions;

struct CustomCheckDiag {
std::string BindName;
std::string Message;
std::optional<DiagnosticIDs::Level> Level;
};
struct CustomCheckValue {
std::string Name;
std::string Query;
llvm::SmallVector<CustomCheckDiag> Diags;
};
using CustomCheckValueList = llvm::SmallVector<CustomCheckValue>;
std::optional<CustomCheckValueList> CustomChecks;

using ArgList = std::vector<std::string>;

/// Add extra compilation arguments to the end of the list.
Expand Down
20 changes: 20 additions & 0 deletions clang-tools-extra/clang-tidy/custom/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
set(LLVM_LINK_COMPONENTS
support
)

add_clang_library(clangTidyCustomModule STATIC
CustomTidyModule.cpp
QueryCheck.cpp

LINK_LIBS
clangTidy
clangTidyUtils

DEPENDS
ClangDriverOptions
)

clang_target_link_libraries(clangTidyCustomModule
PRIVATE
clangQuery
Copy link
Contributor

Choose a reason for hiding this comment

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

Now clang-tidy is made to be dependent on clangQuery, which is a major change. Is this OK? How are build times affected?

It would be great to have @AaronBallman's opinion on this one :)

Copy link
Member

Choose a reason for hiding this comment

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

Probably would be fine, would be good to have cmake options, to enable custom checks, and then if not enabled then support for them woudn't be included in binary

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice, something similar to what's done with the static-analyzer checks

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the suggestion from @PiotrZSL is a reasonable compromise.

I'm presuming it's not an issue that the clangQuery library links against a bunch of clang libraries that clang-tidy already links against, but it would be good to double-check that we're not accidentally getting two copies of those libraries in a static link build.

)
50 changes: 50 additions & 0 deletions clang-tools-extra/clang-tidy/custom/CustomTidyModule.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
#include "../ClangTidy.h"
#include "../ClangTidyModule.h"
#include "../ClangTidyModuleRegistry.h"
#include "../ClangTidyOptions.h"
#include "QueryCheck.h"
#include "llvm/ADT/SmallSet.h"
#include "llvm/ADT/SmallString.h"
#include "llvm/ADT/StringRef.h"
#include <memory>

namespace clang::tidy {
namespace custom {

class CustomModule : public ClangTidyModule {
public:
void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override {}
};

// We need to register the checks more flexibly than builtin modules. The checks
// will changed dynamically when switching to different source file.
extern void registerCustomChecks(ClangTidyOptions const &Options,
ClangTidyCheckFactories &Factories) {
static llvm::SmallSet<llvm::SmallString<32>, 8> CustomCheckNames{};
if (!Options.CustomChecks.has_value() || Options.CustomChecks->empty())
return;
for (llvm::SmallString<32> const &Name : CustomCheckNames)
Factories.erase(Name);
for (const ClangTidyOptions::CustomCheckValue &V :
Options.CustomChecks.value()) {
llvm::SmallString<32> Name = llvm::StringRef{"custom-" + V.Name};
Factories.registerCheckFactory(
// add custom- prefix to avoid conflicts with builtin checks
Name, [&V](llvm::StringRef Name, ClangTidyContext *Context) {
return std::make_unique<custom::QueryCheck>(Name, V, Context);
});
CustomCheckNames.insert(std::move(Name));
}
}

} // namespace custom

// Register the CustomTidyModule using this statically initialized variable.
static ClangTidyModuleRegistry::Add<custom::CustomModule>
X("custom-module", "Adds custom query lint checks.");

// This anchor is used to force the linker to link in the generated object file
// and thus register the AlteraModule.
volatile int CustomModuleAnchorSource = 0;

} // namespace clang::tidy
102 changes: 102 additions & 0 deletions clang-tools-extra/clang-tidy/custom/QueryCheck.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,102 @@
//===--- QueryCheck.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 "QueryCheck.h"
#include "../../clang-query/Query.h"
#include "../../clang-query/QueryParser.h"
#include "clang/ASTMatchers/ASTMatchers.h"
#include "clang/ASTMatchers/Dynamic/VariantValue.h"
#include "clang/Basic/DiagnosticIDs.h"
#include "llvm/ADT/SmallVector.h"
#include "llvm/ADT/StringRef.h"
#include <string>

using namespace clang::ast_matchers;

namespace clang::tidy::custom {

QueryCheck::QueryCheck(llvm::StringRef Name,
const ClangTidyOptions::CustomCheckValue &V,
ClangTidyContext *Context)
: ClangTidyCheck(Name, Context) {
for (const ClangTidyOptions::CustomCheckDiag &D : V.Diags) {
auto DiagnosticIdIt =
Diags
.try_emplace(D.Level.value_or(DiagnosticIDs::Warning),
llvm::StringMap<llvm::SmallVector<std::string>>{})
.first;
auto DiagMessageIt =
DiagnosticIdIt->getSecond()
.try_emplace(D.BindName, llvm::SmallVector<std::string>{})
.first;
DiagMessageIt->second.emplace_back(D.Message);
}

clang::query::QuerySession QS({});
llvm::StringRef QueryStringRef{V.Query};
while (!QueryStringRef.empty()) {
query::QueryRef Q = query::QueryParser::parse(QueryStringRef, 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);
Context->configurationDiag(InvalidQuerry.ErrStr);
break;
}
// FIXME: TODO
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: {
Context->configurationDiag("unsupported querry kind");
}
}
QueryStringRef = Q->RemainingContent;
}
}

void QueryCheck::registerMatchers(MatchFinder *Finder) {
for (const ast_matchers::dynamic::DynTypedMatcher &M : Matchers)
Finder->addDynamicMatcher(M, this);
}

void QueryCheck::check(const MatchFinder::MatchResult &Result) {
auto Emit = [this](DiagMaps const &DiagMaps, std::string const &BindName,
DynTypedNode const &Node, DiagnosticIDs::Level Level) {
if (!DiagMaps.contains(Level))
return;
auto &DiagMap = DiagMaps.at(Level);
if (!DiagMap.contains(BindName))
return;
for (const std::string &Message : DiagMap.at(BindName)) {
diag(Node.getSourceRange().getBegin(), Message, Level);
}
};
for (const auto &[Name, Node] : Result.Nodes.getMap())
Emit(Diags, Name, Node, DiagnosticIDs::Error);
for (const auto &[Name, Node] : Result.Nodes.getMap())
Emit(Diags, Name, Node, DiagnosticIDs::Warning);
// place Note last, otherwise it will not be emitted
for (const auto &[Name, Node] : Result.Nodes.getMap())
Emit(Diags, Name, Node, DiagnosticIDs::Note);
}
} // namespace clang::tidy::custom
Loading
Loading