From 379d776084e04f0634dc8d5366869edcded059f3 Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Thu, 4 Jan 2024 10:09:52 -0800 Subject: [PATCH] Add support for '--config=clang-tidy' (#3559) This sets things up to use `bazel` to run `clang-tidy` using https://github.com/erenon/bazel_clang_tidy. I'm fixing issues outside of explorer, and disabling clang-tidy for targets in explorer that have legacy issues. I was going to disable clang-tidy for targets in explorer such as interpreter anyways, because they're slow to parse, and just extended that to the currently failing targets. --- .bazelrc | 7 +++ .clang-tidy | 6 ++- BUILD | 6 ++- MODULE.bazel | 9 ++++ MODULE.bazel.lock | 46 ++++++++++++++++---- common/init_llvm.cpp | 2 +- explorer/BUILD | 3 ++ explorer/ast/BUILD | 9 ++++ explorer/base/BUILD | 3 ++ explorer/fuzzing/BUILD | 7 ++- explorer/interpreter/BUILD | 12 +++++ language_server/language_server.cpp | 2 +- language_server/language_server.h | 4 +- toolchain/check/check.cpp | 1 + toolchain/check/handle_name.cpp | 2 +- toolchain/lex/lex.cpp | 2 +- toolchain/lex/tokenized_buffer_benchmark.cpp | 1 + toolchain/parse/extract.cpp | 4 +- utils/treesitter/test_runner.cpp | 2 +- 19 files changed, 108 insertions(+), 20 deletions(-) diff --git a/.bazelrc b/.bazelrc index f71c1126bfd5..6d6c7453b338 100644 --- a/.bazelrc +++ b/.bazelrc @@ -2,6 +2,13 @@ # Exceptions. See /LICENSE for license information. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception +# Support running clang-tidy with: +# bazel build --config=clang-tidy -k //... +# See: https://github.com/erenon/bazel_clang_tidy +build:clang-tidy --aspects @bazel_clang_tidy//clang_tidy:clang_tidy.bzl%clang_tidy_aspect +build:clang-tidy --output_groups=report +build:clang-tidy --@bazel_clang_tidy//:clang_tidy_config=//:clang_tidy_config + # Default to using a disk cache to minimize re-building LLVM and Clang which we # try to avoid updating too frequently to minimize rebuild cost. The location # here can be overridden in the user configuration where needed. diff --git a/.clang-tidy b/.clang-tidy index 3e78e16875b6..d90c8d1cb557 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -3,6 +3,11 @@ # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception --- +# Get colors when outputting through `bazel build --config=clang-tidy`. +UseColor: true +# This is necessary for `--config=clang-tidy` to catch errors. +WarningsAsErrors: '*' + # - bugprone-exception-escape finds issues like out-of-memory in main(). We # don't use exceptions, so it's unlikely to find real issues. # - bugprone-unchecked-optional-access in clang-tidy 16 has false positives on @@ -33,7 +38,6 @@ Checks: -readability-magic-numbers, -readability-make-member-function-const, -readability-static-definition-in-anonymous-namespace, -readability-suspicious-call-argument, -readability-use-anyofallof -WarningsAsErrors: true CheckOptions: - { key: readability-identifier-naming.ClassCase, value: CamelCase } - { key: readability-identifier-naming.ClassConstantCase, value: CamelCase } diff --git a/BUILD b/BUILD index a368f2e3e8b6..ac4594c4b738 100644 --- a/BUILD +++ b/BUILD @@ -2,4 +2,8 @@ # Exceptions. See /LICENSE for license information. # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception -# Empty stub. +filegroup( + name = "clang_tidy_config", + srcs = [".clang-tidy"], + visibility = ["//visibility:public"], +) diff --git a/MODULE.bazel b/MODULE.bazel index 885b8fa4397a..adabcae3a8c3 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -63,6 +63,15 @@ http_archive( urls = ["https://github.com/google/libprotobuf-mutator/archive/v{0}.tar.gz".format(libprotobuf_mutator_version)], ) +clang_tidy_version = "d2aecc583d14c9554febeab185833c1e8cce5384" + +http_archive( + name = "bazel_clang_tidy", + sha256 = "89c198a9f544beac119bb41904d16d8870686ccb5fe946442c1576934c9e6869", + strip_prefix = "bazel_clang_tidy-{0}".format(clang_tidy_version), + urls = ["https://github.com/erenon/bazel_clang_tidy/archive/{0}.tar.gz".format(clang_tidy_version)], +) + bazel_cc_toolchain = use_extension( "//bazel/cc_toolchains:clang_configuration.bzl", "clang_toolchain_extension", diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 7b1d055e2852..e06d8f20a195 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -1,6 +1,6 @@ { "lockFileVersion": 3, - "moduleFileHash": "bd9d40db15930ff642c7b8a3449b04e7cb32b8ff7038b0d6c91cc44e9ad68cf5", + "moduleFileHash": "5f71efedda279a9c13889001d008f8d4e66e61a9a386c9f4b5b13d35275a0da3", "flags": { "cmdRegistries": [ "https://bcr.bazel.build/" @@ -37,6 +37,7 @@ }, "imports": { "com_google_libprotobuf_mutator": "com_google_libprotobuf_mutator", + "bazel_clang_tidy": "bazel_clang_tidy", "llvm-raw": "llvm-raw" }, "devImports": [], @@ -59,6 +60,23 @@ "column": 13 } }, + { + "tagName": "@bazel_tools//tools/build_defs/repo:http.bzl%http_archive", + "attributeValues": { + "sha256": "89c198a9f544beac119bb41904d16d8870686ccb5fe946442c1576934c9e6869", + "strip_prefix": "bazel_clang_tidy-d2aecc583d14c9554febeab185833c1e8cce5384", + "urls": [ + "https://github.com/erenon/bazel_clang_tidy/archive/d2aecc583d14c9554febeab185833c1e8cce5384.tar.gz" + ], + "name": "bazel_clang_tidy" + }, + "devDependency": false, + "location": { + "file": "@@//:MODULE.bazel", + "line": 68, + "column": 13 + } + }, { "tagName": "@bazel_tools//tools/build_defs/repo:http.bzl%http_archive", "attributeValues": { @@ -81,7 +99,7 @@ "devDependency": false, "location": { "file": "@@//:MODULE.bazel", - "line": 98, + "line": 107, "column": 13 } } @@ -95,7 +113,7 @@ "usingModule": "", "location": { "file": "@@//:MODULE.bazel", - "line": 66, + "line": 75, "column": 35 }, "imports": { @@ -112,7 +130,7 @@ "usingModule": "", "location": { "file": "@@//:MODULE.bazel", - "line": 113, + "line": 122, "column": 29 }, "imports": { @@ -129,7 +147,7 @@ "usingModule": "", "location": { "file": "@@//:MODULE.bazel", - "line": 125, + "line": 134, "column": 23 }, "imports": { @@ -145,7 +163,7 @@ "devDependency": false, "location": { "file": "@@//:MODULE.bazel", - "line": 126, + "line": 135, "column": 17 } } @@ -159,7 +177,7 @@ "usingModule": "", "location": { "file": "@@//:MODULE.bazel", - "line": 132, + "line": 141, "column": 20 }, "imports": { @@ -177,7 +195,7 @@ "devDependency": false, "location": { "file": "@@//:MODULE.bazel", - "line": 133, + "line": 142, "column": 10 } } @@ -1651,6 +1669,18 @@ "name": "_main~_repo_rules~com_google_libprotobuf_mutator" } }, + "bazel_clang_tidy": { + "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", + "ruleClassName": "http_archive", + "attributes": { + "sha256": "89c198a9f544beac119bb41904d16d8870686ccb5fe946442c1576934c9e6869", + "strip_prefix": "bazel_clang_tidy-d2aecc583d14c9554febeab185833c1e8cce5384", + "urls": [ + "https://github.com/erenon/bazel_clang_tidy/archive/d2aecc583d14c9554febeab185833c1e8cce5384.tar.gz" + ], + "name": "_main~_repo_rules~bazel_clang_tidy" + } + }, "llvm-raw": { "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl", "ruleClassName": "http_archive", diff --git a/common/init_llvm.cpp b/common/init_llvm.cpp index e98dc68a4c24..36cbde464414 100644 --- a/common/init_llvm.cpp +++ b/common/init_llvm.cpp @@ -21,7 +21,7 @@ InitLLVM::InitLLVM(int& argc, char**& argv) argv = args_.data(); // `argv[argc]` is expected to be a null pointer. - args_.push_back(0); + args_.push_back(nullptr); llvm::setBugReportMsg( "Please report issues to " diff --git a/explorer/BUILD b/explorer/BUILD index 04572f80faa8..b68e5747a84d 100644 --- a/explorer/BUILD +++ b/explorer/BUILD @@ -46,6 +46,9 @@ cc_binary( name = "file_test", testonly = 1, srcs = ["file_test.cpp"], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], deps = [ ":main", "//common:check", diff --git a/explorer/ast/BUILD b/explorer/ast/BUILD index 4c67725051c9..d05cee176903 100644 --- a/explorer/ast/BUILD +++ b/explorer/ast/BUILD @@ -41,6 +41,9 @@ cc_library( "value_node.h", "value_transform.h", ], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], textual_hdrs = [ "value_kinds.def", ], @@ -95,6 +98,9 @@ cc_test( name = "expression_test", size = "small", srcs = ["expression_test.cpp"], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], deps = [ ":ast", ":paren_contents", @@ -137,6 +143,9 @@ cc_test( name = "pattern_test", size = "small", srcs = ["pattern_test.cpp"], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], deps = [ ":ast", ":paren_contents", diff --git a/explorer/base/BUILD b/explorer/base/BUILD index a11c316d8ad5..85886dcaf4a1 100644 --- a/explorer/base/BUILD +++ b/explorer/base/BUILD @@ -19,6 +19,9 @@ cc_test( name = "arena_test", size = "small", srcs = ["arena_test.cpp"], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], deps = [ ":arena", "//testing/base:gtest_main", diff --git a/explorer/fuzzing/BUILD b/explorer/fuzzing/BUILD index f8865c417a92..db4ff78b38d2 100644 --- a/explorer/fuzzing/BUILD +++ b/explorer/fuzzing/BUILD @@ -97,7 +97,12 @@ cc_fuzz_test( srcs = ["explorer_fuzzer.cpp"], corpus = glob(["fuzzer_corpus/*"]), shard_count = 8, - tags = ["proto-fuzzer"], + tags = [ + "proto-fuzzer", + # Running clang-tidy is slow, and explorer is currently feature frozen, + # so don't spend time linting it. + "no-clang-tidy", + ], deps = [ ":fuzzer_util", "//common:error", diff --git a/explorer/interpreter/BUILD b/explorer/interpreter/BUILD index 86dee96224d9..42f4a8d3134b 100644 --- a/explorer/interpreter/BUILD +++ b/explorer/interpreter/BUILD @@ -14,6 +14,9 @@ cc_library( hdrs = [ "action.h", ], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], deps = [ ":dictionary", ":heap_allocation_interface", @@ -116,6 +119,9 @@ cc_library( hdrs = [ "interpreter.h", ], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], deps = [ ":action", ":action_stack", @@ -205,6 +211,9 @@ cc_library( "matching_impl_set.h", "type_checker.h", ], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], textual_hdrs = [ "builtins.def", ], @@ -272,6 +281,9 @@ cc_library( hdrs = [ "type_structure.h", ], + # Running clang-tidy is slow, and explorer is currently feature frozen, so + # don't spend time linting it. + tags = ["no-clang-tidy"], deps = [ "//common:ostream", "//explorer/ast", diff --git a/language_server/language_server.cpp b/language_server/language_server.cpp index 227ec507089a..c8be5740b1a4 100644 --- a/language_server/language_server.cpp +++ b/language_server/language_server.cpp @@ -28,7 +28,7 @@ void LanguageServer::OnDidChangeTextDocument( } void LanguageServer::OnInitialize( - clang::clangd::NoParams const& client_capabilities, + clang::clangd::NoParams const& /*client_capabilities*/, clang::clangd::Callback cb) { llvm::json::Object capabilities{{"documentSymbolProvider", true}, {"textDocumentSync", /*Full=*/1}}; diff --git a/language_server/language_server.h b/language_server/language_server.h index 5b88bbfafdfc..cb0457c412b4 100644 --- a/language_server/language_server.h +++ b/language_server/language_server.h @@ -38,8 +38,8 @@ class LanguageServer : public clang::clangd::Transport::MessageHandler, // LSPBinder::RawOutgoing // Send method call to client - void callMethod(llvm::StringRef Method, llvm::json::Value Params, - clang::clangd::Callback Reply) override { + void callMethod(llvm::StringRef method, llvm::json::Value params, + clang::clangd::Callback reply) override { // TODO: implement when needed } diff --git a/toolchain/check/check.cpp b/toolchain/check/check.cpp index 4d3c6e308768..cc0ee1e8703b 100644 --- a/toolchain/check/check.cpp +++ b/toolchain/check/check.cpp @@ -185,6 +185,7 @@ static auto InitPackageScopeAndImports(Context& context, UnitInfo& unit_info) // Loops over all nodes in the tree. On some errors, this may return early, // for example if an unrecoverable state is encountered. +// NOLINTNEXTLINE(readability-function-size) static auto ProcessParseNodes(Context& context, ErrorTrackingDiagnosticConsumer& err_tracker) -> bool { diff --git a/toolchain/check/handle_name.cpp b/toolchain/check/handle_name.cpp index 69c50f2476d0..a37bdd238ce3 100644 --- a/toolchain/check/handle_name.cpp +++ b/toolchain/check/handle_name.cpp @@ -76,7 +76,7 @@ static auto GetClassElementIndex(Context& context, SemIR::InstId element_id) // an implicit `self` parameter. static auto IsInstanceMethod(const SemIR::File& sem_ir, SemIR::FunctionId function_id) -> bool { - auto& function = sem_ir.functions().Get(function_id); + const auto& function = sem_ir.functions().Get(function_id); for (auto param_id : sem_ir.inst_blocks().Get(function.implicit_param_refs_id)) { auto param = diff --git a/toolchain/lex/lex.cpp b/toolchain/lex/lex.cpp index f111db2934ed..533ba0ff0e66 100644 --- a/toolchain/lex/lex.cpp +++ b/toolchain/lex/lex.cpp @@ -1251,7 +1251,7 @@ auto Lexer::LexFileEnd(llvm::StringRef source_text, ssize_t position) -> void { // recovery. These are buffered so that we can perform them in linear time. class Lexer::ErrorRecoveryBuffer { public: - ErrorRecoveryBuffer(TokenizedBuffer& buffer) : buffer_(buffer) {} + explicit ErrorRecoveryBuffer(TokenizedBuffer& buffer) : buffer_(buffer) {} auto empty() const -> bool { return new_tokens_.empty() && !any_error_tokens_; diff --git a/toolchain/lex/tokenized_buffer_benchmark.cpp b/toolchain/lex/tokenized_buffer_benchmark.cpp index b9698a3ec4fe..d1867f4450fd 100644 --- a/toolchain/lex/tokenized_buffer_benchmark.cpp +++ b/toolchain/lex/tokenized_buffer_benchmark.cpp @@ -854,6 +854,7 @@ constexpr DispatchTableT DispatchTable = []() { for (int i = 0; i < 256; ++i) { tmp_table[i] = &BasicDispatch>; } + // NOLINTNEXTLINE(readability-braces-around-statements): False positive. if constexpr (NumDispatchTargets > 1) { // Add additional dispatch targets from our specializable array. SpecializeDispatchTable>( diff --git a/toolchain/parse/extract.cpp b/toolchain/parse/extract.cpp index c30cef83cb07..c4a450d7bef3 100644 --- a/toolchain/parse/extract.cpp +++ b/toolchain/parse/extract.cpp @@ -51,7 +51,7 @@ struct Extractable { if (trace) { *trace << "NodeId: " << tree->node_kind(*it) << " consumed\n"; } - return NodeId(*it++); + return *it++; } }; @@ -288,7 +288,7 @@ struct Extractable { return ExtractTupleLikeType( tree, it, end, trace, std::make_index_sequence>(), - (TupleType*)nullptr); + static_cast(nullptr)); } static auto Extract(const Tree* tree, Tree::SiblingIterator& it, diff --git a/utils/treesitter/test_runner.cpp b/utils/treesitter/test_runner.cpp index eb3751dd26ec..031f3e1476fb 100644 --- a/utils/treesitter/test_runner.cpp +++ b/utils/treesitter/test_runner.cpp @@ -14,7 +14,7 @@ #include extern "C" { -TSLanguage* tree_sitter_carbon(); +auto tree_sitter_carbon() -> TSLanguage*; } // Reads a file to string.