Run clang-tidy on headers (#3572)

This patches bazel_clang_tidy handling of headers. I found an equivalent
change at https://github.com/erenon/bazel_clang_tidy/pull/13, but that
was [already
rejected](https://github.com/erenon/bazel_clang_tidy/pull/13#issuecomment-1047007424).
Per the criticism, this will result in redundant processing of headers.

The project instead uses `HeaderFilterRegex: ".*"`, but that results in
two problems:

1. When running with `-k`, errors are repeated when a header is included
more than once, which is common.
2. clang-tidy including errors from headers that are included from other
modules (e.g., abseil-cpp); filtering correctly is difficult.

Given the trade-offs and options (including forking), I thought patching
was preferable so long as it remains narrow.

.pre-commit-config.yaml

@@ -232,6 +232,7 @@ repos:
 exclude: |
   (?x)^(
       MODULE.bazel.lock|
+      bazel/bazel_clang_tidy/.*\.patch|
       bazel/llvm_project/.*\.patch|
       third_party/examples/.*/carbon/.*|
   )$

MODULE.bazel

@@ -67,6 +67,8 @@ clang_tidy_version = "d2aecc583d14c9554febeab185833c1e8cce5384"
 
 http_archive(
     name = "bazel_clang_tidy",
+    patch_args = ["-p1"],
+    patches = ["@carbon//bazel/bazel_clang_tidy:0001_Add_hdrs.patch"],
     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)],

MODULE.bazel.lock

@@ -1,6 +1,6 @@
 {
   "lockFileVersion": 3,
-  "moduleFileHash": "1eff193166261836db74ff270f9bc0ef7d800107c1593383d78cd652d685bb6d",
+  "moduleFileHash": "5704ebd75cf3cb5f4ab56ba2939bb74d98752cd7eed0ea9357a9ed39ca41640e",
   "flags": {
     "cmdRegistries": [
       "https://bcr.bazel.build/"
@@ -63,6 +63,12 @@
             {
               "tagName": "@bazel_tools//tools/build_defs/repo:http.bzl%http_archive",
               "attributeValues": {
+                "patch_args": [
+                  "-p1"
+                ],
+                "patches": [
+                  "@carbon//bazel/bazel_clang_tidy:0001_Add_hdrs.patch"
+                ],
                 "sha256": "89c198a9f544beac119bb41904d16d8870686ccb5fe946442c1576934c9e6869",
                 "strip_prefix": "bazel_clang_tidy-d2aecc583d14c9554febeab185833c1e8cce5384",
                 "urls": [
@@ -99,7 +105,7 @@
               "devDependency": false,
               "location": {
                 "file": "@@//:MODULE.bazel",
-                "line": 106,
+                "line": 108,
                 "column": 13
               }
             }
@@ -113,7 +119,7 @@
           "usingModule": "<root>",
           "location": {
             "file": "@@//:MODULE.bazel",
-            "line": 75,
+            "line": 77,
             "column": 35
           },
           "imports": {
@@ -130,7 +136,7 @@
           "usingModule": "<root>",
           "location": {
             "file": "@@//:MODULE.bazel",
-            "line": 121,
+            "line": 123,
             "column": 29
           },
           "imports": {
@@ -147,7 +153,7 @@
           "usingModule": "<root>",
           "location": {
             "file": "@@//:MODULE.bazel",
-            "line": 133,
+            "line": 135,
             "column": 23
           },
           "imports": {
@@ -163,7 +169,7 @@
               "devDependency": false,
               "location": {
                 "file": "@@//:MODULE.bazel",
-                "line": 134,
+                "line": 136,
                 "column": 17
               }
             }
@@ -177,7 +183,7 @@
           "usingModule": "<root>",
           "location": {
             "file": "@@//:MODULE.bazel",
-            "line": 140,
+            "line": 142,
             "column": 20
           },
           "imports": {
@@ -195,7 +201,7 @@
               "devDependency": false,
               "location": {
                 "file": "@@//:MODULE.bazel",
-                "line": 141,
+                "line": 143,
                 "column": 10
               }
             }
@@ -1673,6 +1679,12 @@
             "bzlFile": "@@bazel_tools//tools/build_defs/repo:http.bzl",
             "ruleClassName": "http_archive",
             "attributes": {
+              "patch_args": [
+                "-p1"
+              ],
+              "patches": [
+                "@@//bazel/bazel_clang_tidy:0001_Add_hdrs.patch"
+              ],
               "sha256": "89c198a9f544beac119bb41904d16d8870686ccb5fe946442c1576934c9e6869",
               "strip_prefix": "bazel_clang_tidy-d2aecc583d14c9554febeab185833c1e8cce5384",
               "urls": [

bazel/bazel_clang_tidy/0001_Add_hdrs.patch

@@ -0,0 +1,25 @@
+From c2b1cfb7ec8ba667218382b97a986a3ef2f7ff56 Mon Sep 17 00:00:00 2001
+From: jonmeow <jperkins@google.com>
+Date: Fri, 5 Jan 2024 10:35:19 -0800
+Subject: [PATCH] Add hdrs so that clang-tidy runs on them directly.
+
+---
+ clang_tidy/clang_tidy.bzl | 3 +++
+ 1 file changed, 3 insertions(+)
+
+diff --git a/clang_tidy/clang_tidy.bzl b/clang_tidy/clang_tidy.bzl
+index 4d515fc..1af7ee4 100644
+--- a/clang_tidy/clang_tidy.bzl
++++ b/clang_tidy/clang_tidy.bzl
+@@ -94,6 +94,9 @@ def _rule_sources(ctx):
+     if hasattr(ctx.rule.attr, "srcs"):
+         for src in ctx.rule.attr.srcs:
+             srcs += [src for src in src.files.to_list() if src.is_source and check_valid_file_type(src)]
++    if hasattr(ctx.rule.attr, "hdrs"):
++        for src in ctx.rule.attr.hdrs:
++            srcs += [src for src in src.files.to_list() if src.is_source and check_valid_file_type(src)]
+     return srcs
+
+ def _toolchain_flags(ctx, action_name = ACTION_NAMES.cpp_compile):
+--
+2.43.0.472.g3155946c3a-goog

bazel/bazel_clang_tidy/BUILD

@@ -0,0 +1,9 @@
+# Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+# Exceptions. See /LICENSE for license information.
+# SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+package(default_visibility = ["//visibility:public"])
+
+exports_files(glob([
+    "*.patch",
+]))

common/all_llvm_targets.cpp

@@ -17,6 +17,7 @@ static auto InitLLVMTargets() -> void {
 
 // On program startup, set `InitLLVM::InitializeTargets` to be our
 // initialization function so that `InitLLVM` can call it at the right moment.
-char InitLLVM::RegisterTargets = (InitializeTargets = &InitLLVMTargets, 0);
+const char InitLLVM::RegisterTargets =
+    (InitializeTargets = &InitLLVMTargets, 0);
 
 }  // namespace Carbon

common/init_llvm.cpp

@@ -34,6 +34,6 @@ InitLLVM::InitLLVM(int& argc, char**& argv)
   }
 }
 
-auto (*InitLLVM::InitializeTargets)() -> void = nullptr;
+InitLLVM::InitializeTargetsFn* InitLLVM::InitializeTargets = nullptr;
 
 }  // namespace Carbon

common/init_llvm.h

@@ -24,17 +24,20 @@ class InitLLVM {
   ~InitLLVM() = default;
 
  private:
+  using InitializeTargetsFn = auto() -> void;
+
   llvm::InitLLVM init_llvm_;
   llvm::SmallVector<char*> args_;
 
   // A pointer to the LLVM target initialization function, if :all_llvm_targets
   // is linked in. Otherwise nullptr.
-  static auto (*InitializeTargets)() -> void;
+  // NOLINTNEXTLINE(readability-identifier-naming): Constant after static init.
+  static InitializeTargetsFn* InitializeTargets;
 
   // The initializer of this static data member populates `InitializeTargets`.
   // Defined only if :all_llvm_targets is linked in. This is a member so that
   // it has access to `InitializeTargets`.
-  static char RegisterTargets;
+  static const char RegisterTargets;
 };
 
 }  // namespace Carbon

explorer/base/BUILD

@@ -9,6 +9,9 @@ package(default_visibility = ["//explorer:__subpackages__"])
 cc_library(
     name = "arena",
     hdrs = ["arena.h"],
+    # Running clang-tidy is slow, and explorer is currently feature frozen, so
+    # don't spend time linting it.
+    tags = ["no-clang-tidy"],
     deps = [
         ":nonnull",
         "@llvm-project//llvm:Support",

toolchain/check/node_stack.h

@@ -150,7 +150,7 @@ class NodeStack {
   template <const Parse::NodeKind& RequiredParseKind>
   auto PopWithParseNode() -> auto {
     constexpr IdKind RequiredIdKind = ParseNodeKindToIdKind(RequiredParseKind);
-    auto NodeIdCast = [&](auto back) {
+    auto node_id_cast = [&](auto back) {
       using NodeIdT = Parse::NodeIdForKind<RequiredParseKind>;
       return std::pair<NodeIdT, decltype(back.second)>(back);
     };
@@ -158,37 +158,37 @@ class NodeStack {
     if constexpr (RequiredIdKind == IdKind::InstId) {
       auto back = PopWithParseNode<SemIR::InstId>();
       RequireParseKind<RequiredParseKind>(back.first);
-      return NodeIdCast(back);
+      return node_id_cast(back);
     }
     if constexpr (RequiredIdKind == IdKind::InstBlockId) {
       auto back = PopWithParseNode<SemIR::InstBlockId>();
       RequireParseKind<RequiredParseKind>(back.first);
-      return NodeIdCast(back);
+      return node_id_cast(back);
     }
     if constexpr (RequiredIdKind == IdKind::FunctionId) {
       auto back = PopWithParseNode<SemIR::FunctionId>();
       RequireParseKind<RequiredParseKind>(back.first);
-      return NodeIdCast(back);
+      return node_id_cast(back);
     }
     if constexpr (RequiredIdKind == IdKind::ClassId) {
       auto back = PopWithParseNode<SemIR::ClassId>();
       RequireParseKind<RequiredParseKind>(back.first);
-      return NodeIdCast(back);
+      return node_id_cast(back);
     }
     if constexpr (RequiredIdKind == IdKind::InterfaceId) {
       auto back = PopWithParseNode<SemIR::InterfaceId>();
       RequireParseKind<RequiredParseKind>(back.first);
-      return NodeIdCast(back);
+      return node_id_cast(back);
     }
     if constexpr (RequiredIdKind == IdKind::NameId) {
       auto back = PopWithParseNode<SemIR::NameId>();
       RequireParseKind<RequiredParseKind>(back.first);
-      return NodeIdCast(back);
+      return node_id_cast(back);
     }
     if constexpr (RequiredIdKind == IdKind::TypeId) {
       auto back = PopWithParseNode<SemIR::TypeId>();
       RequireParseKind<RequiredParseKind>(back.first);
-      return NodeIdCast(back);
+      return node_id_cast(back);
     }
     CARBON_FATAL() << "Unpoppable IdKind for parse kind: " << RequiredParseKind
                    << "; see value in ParseNodeKindToIdKind";

toolchain/parse/node_ids.h

@@ -23,7 +23,8 @@ struct NodeId : public IdBase {
   static constexpr InvalidNodeId Invalid;
 
   using IdBase::IdBase;
-  constexpr NodeId(InvalidNodeId) : IdBase(NodeId::InvalidIndex) {}
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  constexpr NodeId(InvalidNodeId /*invalid*/) : IdBase(NodeId::InvalidIndex) {}
 };
 
 // For looking up the type associated with a given id type.
@@ -34,9 +35,12 @@ struct NodeForId;
 // `<KindName>`:
 template <const NodeKind& K>
 struct NodeIdForKind : public NodeId {
+  // NOLINTNEXTLINE(readability-identifier-naming)
   static const NodeKind& Kind;
   constexpr explicit NodeIdForKind(NodeId node_id) : NodeId(node_id) {}
-  constexpr NodeIdForKind(InvalidNodeId) : NodeId(NodeId::InvalidIndex) {}
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  constexpr NodeIdForKind(InvalidNodeId /*invalid*/)
+      : NodeId(NodeId::InvalidIndex) {}
 };
 template <const NodeKind& K>
 const NodeKind& NodeIdForKind<K>::Kind = K;
@@ -52,7 +56,9 @@ struct NodeIdInCategory : public NodeId {
   // overlaps with `Category`.
 
   constexpr explicit NodeIdInCategory(NodeId node_id) : NodeId(node_id) {}
-  constexpr NodeIdInCategory(InvalidNodeId) : NodeId(NodeId::InvalidIndex) {}
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  constexpr NodeIdInCategory(InvalidNodeId /*invalid*/)
+      : NodeId(NodeId::InvalidIndex) {}
 };
 
 // Aliases for `NodeIdInCategory` to describe particular categories of nodes.
@@ -69,10 +75,13 @@ template <typename T, typename U>
 struct NodeIdOneOf : public NodeId {
   constexpr explicit NodeIdOneOf(NodeId node_id) : NodeId(node_id) {}
   template <const NodeKind& Kind>
+  // NOLINTNEXTLINE(google-explicit-constructor)
   NodeIdOneOf(NodeIdForKind<Kind> node_id) : NodeId(node_id) {
     static_assert(T::Kind == Kind || U::Kind == Kind);
   }
-  constexpr NodeIdOneOf(InvalidNodeId) : NodeId(NodeId::InvalidIndex) {}
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  constexpr NodeIdOneOf(InvalidNodeId /*invalid*/)
+      : NodeId(NodeId::InvalidIndex) {}
 };
 
 using AnyClassDeclId = NodeIdOneOf<ClassDeclId, ClassDefinitionStartId>;
@@ -85,7 +94,9 @@ using AnyInterfaceDeclId =
 template <typename T>
 struct NodeIdNot : public NodeId {
   constexpr explicit NodeIdNot(NodeId node_id) : NodeId(node_id) {}
-  constexpr NodeIdNot(InvalidNodeId) : NodeId(NodeId::InvalidIndex) {}
+  // NOLINTNEXTLINE(google-explicit-constructor)
+  constexpr NodeIdNot(InvalidNodeId /*invalid*/)
+      : NodeId(NodeId::InvalidIndex) {}
 };
 
 // Note that the support for extracting these types using the `Tree::Extract*`

toolchain/parse/tree.h

@@ -439,7 +439,8 @@ auto Tree::ExtractNodeFromChildren(
     // On error try again, this time capturing a trace.
     ErrorBuilder trace;
     TryExtractNodeFromChildren<T>(children, &trace);
-    CARBON_FATAL() << "Malformed parse node:\n" << Error(trace).message();
+    CARBON_FATAL() << "Malformed parse node:\n"
+                   << static_cast<Error>(trace).message();
   }
   return *result;
 }

toolchain/parse/tree_node_location_translator.h

@@ -13,11 +13,13 @@ namespace Carbon::Parse {
 
 class NodeLocation {
  public:
+  // NOLINTNEXTLINE(google-explicit-constructor)
   NodeLocation(NodeId node_id) : NodeLocation(node_id, false) {}
   NodeLocation(NodeId node_id, bool token_only)
       : node_id_(node_id), token_only_(token_only) {}
   // TODO: Have some other way of representing diagnostic that applies to a file
   // as a whole.
+  // NOLINTNEXTLINE(google-explicit-constructor)
   NodeLocation(InvalidNodeId node_id) : NodeLocation(node_id, false) {}
 
   auto node_id() const -> NodeId { return node_id_; }

toolchain/sem_ir/file.h

@@ -178,7 +178,7 @@ class File : public Printable<File> {
                 const File* builtins);
 
   File(const File&) = delete;
-  File& operator=(const File&) = delete;
+  auto operator=(const File&) -> File& = delete;
 
   // Verifies that invariants of the semantics IR hold.
   auto Verify() const -> ErrorOr<Success>;

toolchain/sem_ir/inst.h

@@ -57,12 +57,14 @@ struct InstLikeTypeInfoBase {
 // A particular type of instruction is instruction-like.
 template <typename TypedInst>
 struct InstLikeTypeInfo<
-    TypedInst,
-    (bool)std::is_same_v<const InstKind::Definition, decltype(TypedInst::Kind)>>
+    TypedInst, static_cast<bool>(std::is_same_v<const InstKind::Definition,
+                                                decltype(TypedInst::Kind)>)>
     : InstLikeTypeInfoBase<TypedInst> {
   static_assert(!HasKindMemberAsField<TypedInst>,
                 "Instruction type should not have a kind field");
-  static auto GetKind(TypedInst) -> InstKind { return TypedInst::Kind; }
+  static auto GetKind(TypedInst /*inst*/) -> InstKind {
+    return TypedInst::Kind;
+  }
   static auto IsKind(InstKind kind) -> bool { return kind == TypedInst::Kind; }
   // A name that can be streamed to an llvm::raw_ostream.
   static auto DebugName() -> InstKind { return TypedInst::Kind; }
@@ -71,7 +73,8 @@ struct InstLikeTypeInfo<
 // An instruction category is instruction-like.
 template <typename InstCat>
 struct InstLikeTypeInfo<
-    InstCat, (bool)std::is_same_v<const InstKind&, decltype(InstCat::Kinds[0])>>
+    InstCat, static_cast<bool>(
+                 std::is_same_v<const InstKind&, decltype(InstCat::Kinds[0])>)>
     : InstLikeTypeInfoBase<InstCat> {
   static_assert(HasKindMemberAsField<InstCat>,
                 "Instruction category should have a kind field");