diff options
author | Jade Lovelace <lix@jade.fyi> | 2024-07-31 20:37:27 -0700 |
---|---|---|
committer | Jade Lovelace <lix@jade.fyi> | 2024-08-04 20:41:19 -0700 |
commit | 3daeeaefb115f47bf52ae96fa2b3f01842ec0f0f (patch) | |
tree | 9e1f7be5c068055e9d1a815e6f47090252bb61f5 /subprojects/lix-clang-tidy | |
parent | 32ca194ebfa3610c8099e5713ee7d5cbb8fb125c (diff) |
build: implement clang-tidy using our plugin
The principle of this is that you can either externally build it with
Nix (actual implementation will be in a future commit), or it can be
built with meson if the Nix one is not passed in.
The idea I have is that dev shells don't receive the one from Nix to
avoid having to build it, but CI can use the one from Nix and save some
gratuitous rebuilds.
The design of this is that you can run `ninja -C build clang-tidy` and
it will simply correctly clang-tidy the codebase in spite of PCH
bullshit caused by the cc-wrapper.
This is a truly horrendous number of hacks in a ball, caused by bugs in
several pieces of software, and I am not even getting started.
I don't consider this to fix the clang-tidy issue filing, since we still
have a fair number of issues to fix even on the existing minimal
configuration, and I have not yet implemented it in CI. Realistically we
will need to do something like https://github.com/Ericsson/codechecker
to be able to silence warnings without physically touching the code, or
at least *diff* reports between versions.
Also, the run-clang-tidy output design is rather atrocious and must
not be inflicted upon anyone I have respect for, since it buries the
diagnostics in a pile of invocation logs. We would do really well to
integrate with the Gerrit SARIF stuff so we can dump the reports on
people in a user-friendly manner.
Related: https://git.lix.systems/lix-project/lix/issues/147
Change-Id: Ifefe533f3b56874795de231667046b2da6ff2461
Diffstat (limited to 'subprojects/lix-clang-tidy')
-rw-r--r-- | subprojects/lix-clang-tidy/.clang-format | 1 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/.editorconfig | 4 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/FixIncludes.cc | 90 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/FixIncludes.hh | 21 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/HasPrefixSuffix.cc | 80 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/HasPrefixSuffix.hh | 25 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/LixClangTidyChecks.cc | 19 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/README.md | 56 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/default.nix | 44 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/meson.build | 18 | ||||
-rw-r--r-- | subprojects/lix-clang-tidy/meson.options | 3 |
11 files changed, 361 insertions, 0 deletions
diff --git a/subprojects/lix-clang-tidy/.clang-format b/subprojects/lix-clang-tidy/.clang-format new file mode 100644 index 000000000..cd8995543 --- /dev/null +++ b/subprojects/lix-clang-tidy/.clang-format @@ -0,0 +1 @@ +BasedOnStyle: llvm diff --git a/subprojects/lix-clang-tidy/.editorconfig b/subprojects/lix-clang-tidy/.editorconfig new file mode 100644 index 000000000..19ee09eec --- /dev/null +++ b/subprojects/lix-clang-tidy/.editorconfig @@ -0,0 +1,4 @@ +# LLVM style code is 2-space indented +[*.{cc,hh}] +indent_style = space +indent_size = 2 diff --git a/subprojects/lix-clang-tidy/FixIncludes.cc b/subprojects/lix-clang-tidy/FixIncludes.cc new file mode 100644 index 000000000..602d3d355 --- /dev/null +++ b/subprojects/lix-clang-tidy/FixIncludes.cc @@ -0,0 +1,90 @@ +#include "FixIncludes.hh" +#include <clang-tidy/ClangTidyCheck.h> +#include <clang/Basic/Diagnostic.h> +#include <clang/Basic/SourceManager.h> +#include <clang/Lex/PPCallbacks.h> +#include <clang/Lex/Preprocessor.h> +#include <llvm/ADT/StringRef.h> +#include <llvm/Support/Debug.h> +#include <memory> +#include <set> +#include <string> + +namespace nix::clang_tidy { + +using namespace clang; +using namespace clang::tidy; + +class FixIncludesCallbacks : public PPCallbacks { +public: + ClangTidyCheck &Check; + Preprocessor &PP; + FixIncludesCallbacks(ClangTidyCheck &Check, Preprocessor &PP) + : Check(Check), PP(PP) {} + +private: + bool Ignore = false; + virtual void LexedFileChanged(FileID FID, LexedFileChangeReason Reason, + SrcMgr::CharacteristicKind FileType, + FileID PrevFID, SourceLocation Loc) override; + + virtual void InclusionDirective(SourceLocation HashLoc, + const Token &IncludeTok, StringRef FileName, + bool IsAngled, CharSourceRange FilenameRange, + OptionalFileEntryRef File, + StringRef SearchPath, StringRef RelativePath, + const Module *Imported, + SrcMgr::CharacteristicKind FileType) override; +}; + +void FixIncludesCallbacks::LexedFileChanged(FileID, LexedFileChangeReason, + SrcMgr::CharacteristicKind FileType, + FileID, SourceLocation) { + Ignore = FileType != SrcMgr::C_User; +} + +void FixIncludesCallbacks::InclusionDirective( + SourceLocation, const Token &, StringRef FileName, bool IsAngled, + CharSourceRange FilenameRange, OptionalFileEntryRef File, StringRef, + StringRef, const Module *, SrcMgr::CharacteristicKind) { + if (Ignore) + return; + + // FIXME: this is kinda evil, but this is a one-time fixup + const std::vector<std::string> SourceDirs = {"src/", "include/lix/"}; + + const auto Bracketize = [IsAngled](StringRef s) { + return IsAngled ? ("<" + s + ">").str() : ("\"" + s + "\"").str(); + }; + + for (const auto &SourceDir : SourceDirs) { + const bool IsAlreadyFixed = FileName.starts_with("lix/lib"); + if (File && File->getNameAsRequested().contains(SourceDir) && + !IsAlreadyFixed) { + StringRef Name = File->getNameAsRequested(); + auto Idx = Name.find(SourceDir); + assert(Idx != std::string::npos); + std::string Suffix = Name.drop_front(Idx + SourceDir.length()).str(); + + if (!Suffix.starts_with("lib")) { + llvm::dbgs() << "ignored: " << Suffix << "\n"; + return; + } + + Suffix = "lix/" + Suffix; + + auto Diag = Check.diag(FilenameRange.getBegin(), + "include needs to specify the source subdir"); + + Diag << FilenameRange + << FixItHint::CreateReplacement(FilenameRange, Bracketize(Suffix)); + } + } +} + +void FixIncludesCheck::registerPPCallbacks(const SourceManager &, + Preprocessor *PP, Preprocessor *) { + PP->addPPCallbacks(std::make_unique<FixIncludesCallbacks>(*this, *PP)); +} + +}; // namespace nix::clang_tidy diff --git a/subprojects/lix-clang-tidy/FixIncludes.hh b/subprojects/lix-clang-tidy/FixIncludes.hh new file mode 100644 index 000000000..ea890cd39 --- /dev/null +++ b/subprojects/lix-clang-tidy/FixIncludes.hh @@ -0,0 +1,21 @@ +#pragma once +///@file + +#include <clang-tidy/ClangTidyCheck.h> +#include <clang/ASTMatchers/ASTMatchFinder.h> +#include <llvm/ADT/StringRef.h> + +namespace nix::clang_tidy { + +using namespace clang; +using namespace clang::tidy; + +class FixIncludesCheck : public ClangTidyCheck { + public: + FixIncludesCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + + void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP, Preprocessor *ModuleExpanderPP) override; +}; + +}; diff --git a/subprojects/lix-clang-tidy/HasPrefixSuffix.cc b/subprojects/lix-clang-tidy/HasPrefixSuffix.cc new file mode 100644 index 000000000..e29b67e7c --- /dev/null +++ b/subprojects/lix-clang-tidy/HasPrefixSuffix.cc @@ -0,0 +1,80 @@ +#include "HasPrefixSuffix.hh" +#include <clang/AST/ASTTypeTraits.h> +#include <clang/AST/Expr.h> +#include <clang/AST/PrettyPrinter.h> +#include <clang/AST/Type.h> +#include <clang/ASTMatchers/ASTMatchers.h> +#include <clang/Basic/Diagnostic.h> +#include <clang/Frontend/FrontendAction.h> +#include <clang/Frontend/FrontendPluginRegistry.h> +#include <clang/Tooling/Transformer/SourceCode.h> +#include <clang/Tooling/Transformer/SourceCodeBuilders.h> +#include <iostream> + +namespace nix::clang_tidy { +using namespace clang::ast_matchers; +using namespace clang; + +void HasPrefixSuffixCheck::registerMatchers(ast_matchers::MatchFinder *Finder) { + Finder->addMatcher( + traverse(clang::TK_AsIs, + callExpr(callee(functionDecl(anyOf(hasName("hasPrefix"), + hasName("hasSuffix"))) + .bind("callee-decl")), + optionally(hasArgument( + 0, cxxConstructExpr( + hasDeclaration(functionDecl(hasParameter( + 0, parmVarDecl(hasType( + asString("const char *"))))))) + .bind("implicit-cast")))) + .bind("call")), + this); +} + +void HasPrefixSuffixCheck::check( + const ast_matchers::MatchFinder::MatchResult &Result) { + + const auto *CalleeDecl = Result.Nodes.getNodeAs<FunctionDecl>("callee-decl"); + auto FuncName = std::string(CalleeDecl->getName()); + std::string NewName; + if (FuncName == "hasPrefix") { + NewName = "starts_with"; + } else if (FuncName == "hasSuffix") { + NewName = "ends_with"; + } else { + llvm_unreachable("nix-has-prefix: invalid callee"); + } + + const auto *MatchedDecl = Result.Nodes.getNodeAs<CallExpr>("call"); + const auto *ImplicitConvertArg = + Result.Nodes.getNodeAs<CXXConstructExpr>("implicit-cast"); + + const auto *Lhs = MatchedDecl->getArg(0); + const auto *Rhs = MatchedDecl->getArg(1); + auto Diag = diag(MatchedDecl->getExprLoc(), FuncName + " is deprecated"); + + std::string Text = ""; + + // Form possible cast to string_view, or nothing. + if (ImplicitConvertArg) { + Text = "std::string_view("; + Text.append(tooling::getText(*Lhs, *Result.Context)); + Text.append(")."); + } else { + Text.append(*tooling::buildAccess(*Lhs, *Result.Context)); + } + + // Call .starts_with. + Text.append(NewName); + Text.push_back('('); + Text.append(tooling::getText(*Rhs, *Result.Context)); + Text.push_back(')'); + + Diag << FixItHint::CreateReplacement(MatchedDecl->getSourceRange(), Text); + + // for (const auto *arg : MatchedDecl->arguments()) { + // arg->dumpColor(); + // arg->getType().dump(); + // } +} +}; // namespace nix::clang_tidy diff --git a/subprojects/lix-clang-tidy/HasPrefixSuffix.hh b/subprojects/lix-clang-tidy/HasPrefixSuffix.hh new file mode 100644 index 000000000..8693b6767 --- /dev/null +++ b/subprojects/lix-clang-tidy/HasPrefixSuffix.hh @@ -0,0 +1,25 @@ +#pragma once +///@file +/// This is an example of a clang-tidy automated refactoring against the Nix +/// codebase. The refactoring has been completed in +/// https://gerrit.lix.systems/c/lix/+/565 so this code is around as +/// an example. + +#include <clang-tidy/ClangTidyCheck.h> +#include <clang/ASTMatchers/ASTMatchFinder.h> +#include <llvm/ADT/StringRef.h> + +namespace nix::clang_tidy { + +using namespace clang; +using namespace clang::tidy; +using namespace llvm; + +class HasPrefixSuffixCheck : public ClangTidyCheck { +public: + HasPrefixSuffixCheck(StringRef Name, ClangTidyContext *Context) + : ClangTidyCheck(Name, Context) {} + void registerMatchers(ast_matchers::MatchFinder *Finder) override; + void check(const ast_matchers::MatchFinder::MatchResult &Result) override; +}; +}; // namespace nix::clang_tidy diff --git a/subprojects/lix-clang-tidy/LixClangTidyChecks.cc b/subprojects/lix-clang-tidy/LixClangTidyChecks.cc new file mode 100644 index 000000000..b3503dd3a --- /dev/null +++ b/subprojects/lix-clang-tidy/LixClangTidyChecks.cc @@ -0,0 +1,19 @@ +#include <clang-tidy/ClangTidyModule.h> +#include <clang-tidy/ClangTidyModuleRegistry.h> +#include "FixIncludes.hh" +#include "HasPrefixSuffix.hh" + +namespace nix::clang_tidy { +using namespace clang; +using namespace clang::tidy; + +class NixClangTidyChecks : public ClangTidyModule { + public: + void addCheckFactories(ClangTidyCheckFactories &CheckFactories) override { + CheckFactories.registerCheck<HasPrefixSuffixCheck>("lix-hasprefixsuffix"); + CheckFactories.registerCheck<FixIncludesCheck>("lix-fixincludes"); + } +}; + +static ClangTidyModuleRegistry::Add<NixClangTidyChecks> X("lix-module", "Adds lix specific checks"); +}; diff --git a/subprojects/lix-clang-tidy/README.md b/subprojects/lix-clang-tidy/README.md new file mode 100644 index 000000000..c2d1cb258 --- /dev/null +++ b/subprojects/lix-clang-tidy/README.md @@ -0,0 +1,56 @@ +# Clang tidy lints for Lix + +This is a skeleton of a clang-tidy lints library for Lix. + +Currently there is one check (which is already obsolete as it has served its +goal and is there as an example), `HasPrefixSuffixCheck`. + +## Running fixes/checks + +One file: + +``` +ninja -C build && clang-tidy --checks='-*,lix-*' --load=build/liblix-clang-tidy.so -p ../compile_commands.json -header-filter '\.\./src/.*\.h' --fix ../src/libcmd/installables.cc +``` + +Several files, in parallel: + +``` +ninja -C build && run-clang-tidy -checks='-*,lix-*' -load=build/liblix-clang-tidy.so -p .. -header-filter '\.\./src/.*\.h' -fix ../src | tee -a clang-tidy-result +``` + +## Resources + +* https://firefox-source-docs.mozilla.org/code-quality/static-analysis/writing-new/clang-query.html +* https://clang.llvm.org/docs/LibASTMatchersReference.html +* https://devblogs.microsoft.com/cppblog/exploring-clang-tooling-part-3-rewriting-code-with-clang-tidy/ + +## Developing new checks + +Put something like so in `myquery.txt`: + +``` +set traversal IgnoreUnlessSpelledInSource +# ^ Ignore implicit AST nodes. May need to use AsIs depending on how you are +# working. +set bind-root true +# ^ true unless you use any .bind("foo") commands +set print-matcher true +enable output dump +match callExpr(callee(functionDecl(hasName("hasPrefix"))), optionally(hasArgument( 0, cxxConstructExpr(hasDeclaration(functionDecl(hasParameter(0, parmVarDecl(hasType(asString("const char *"))).bind("meow2")))))))) +``` + +Then run, e.g. `clang-query --preload hasprefix.query -p compile_commands.json src/libcmd/installables.cc`. + +With this you can iterate a query before writing it in C++ and suffering from +C++. + +### Tips and tricks for the C++ + +There is a function `dump()` on many things that will dump to stderr. Also +`llvm::errs()` lets you print to stderr. + +When I wrote `HasPrefixSuffixCheck`, I was not really able to figure out how +the structured replacement system was supposed to work. In principle you can +describe the replacement with a nice DSL. Look up the Stencil system in Clang +for details. diff --git a/subprojects/lix-clang-tidy/default.nix b/subprojects/lix-clang-tidy/default.nix new file mode 100644 index 000000000..1bfc2d9a4 --- /dev/null +++ b/subprojects/lix-clang-tidy/default.nix @@ -0,0 +1,44 @@ +{ + lib, + stdenv, + cmake, + meson, + ninja, + llvmPackages, +}: +let + inherit (lib) fileset; +in +stdenv.mkDerivation { + pname = "lix-clang-tidy-checks"; + # Setting the version to the Lix version is just going to cause pointless + # rebuilds due to versionSuffix and similar, and I cannot conceive of a usage + # where we actually care about its version since this is internal-only. + version = "0.1"; + + src = fileset.toSource { + root = ./.; + fileset = fileset.unions [ + ./meson.build + ./meson.options + (fileset.fileFilter ( + { hasExt, ... }: + builtins.any hasExt [ + "cc" + "hh" + ] + ) ./.) + ]; + }; + + nativeBuildInputs = [ + meson + cmake + ninja + ]; + + buildInputs = [ + llvmPackages.llvm + llvmPackages.clang-unwrapped.dev + ]; +} diff --git a/subprojects/lix-clang-tidy/meson.build b/subprojects/lix-clang-tidy/meson.build new file mode 100644 index 000000000..ef0226420 --- /dev/null +++ b/subprojects/lix-clang-tidy/meson.build @@ -0,0 +1,18 @@ +project('lix-clang-tidy', ['cpp', 'c'], + version : '0.1', + default_options : ['warning_level=3', 'cpp_std=c++20'] +) + +llvm = dependency('Clang', version: '>= 17', modules: ['libclang']) +sources = files( + 'HasPrefixSuffix.cc', + 'LixClangTidyChecks.cc', + 'FixIncludes.cc', +) + +lix_clang_tidy = shared_module('lix-clang-tidy', sources, + dependencies: llvm, + # overrides build_by_default, see https://github.com/mesonbuild/meson/issues/13498 + install : get_option('build-by-default'), + build_by_default : get_option('build-by-default') +) diff --git a/subprojects/lix-clang-tidy/meson.options b/subprojects/lix-clang-tidy/meson.options new file mode 100644 index 000000000..57f0f713b --- /dev/null +++ b/subprojects/lix-clang-tidy/meson.options @@ -0,0 +1,3 @@ +option('build-by-default', type : 'boolean', value : true, + description : 'set to false to not actually build targets by default. This is a hack to deal with meson lacking a build_by_default default option and building subprojects by default' +) |