[build] Add misc-include-cleaner clang-tidy check for headers

.clang-tidy

@@ -29,6 +29,7 @@ Checks: >
   google-readability-casting,
   misc-confusable-identifiers,
   misc-header-include-cycle,
+  misc-include-cleaner,
   misc-redundant-expression,
   misc-throw-by-value-catch-by-reference,
   misc-unused-alias-decls,
@@ -96,6 +97,11 @@ CheckOptions:
     value: "jsg/jsg.h|jsg/dom-exception.h"
   - key: cppcoreguidelines-missing-std-forward.ForwardFunction
     value: "kj::fwd"
+  - key: misc-include-cleaner.MissingIncludes
+    value: False
+  # Ignore KJ headers (too many to clean up for now) and some JSG headers where misc-include-cleaner reports false positives
+  - key: misc-include-cleaner.IgnoreHeaders
+    value: "jsg.h|function.h|kj/.*"
 
 ####
 # Custom checks.

build/tools/clang_tidy/check_path_filters.bzl

@@ -23,3 +23,9 @@ CHECK_PATH_FILTERS = {
         # "src/workerd/api",
     ],
 }
+
+HEADER_ONLY_CHECKS = [
+    # Enabling misc-include-cleaner on headers means having to clean up much
+    # fewer includes while still getting rid of superfluous transitive includes.
+    "misc-include-cleaner",
+]

build/tools/clang_tidy/clang_tidy.bzl

@@ -7,13 +7,15 @@ load("@rules_cc//cc:action_names.bzl", "ACTION_NAMES")
 load("@rules_cc//cc:find_cc_toolchain.bzl", "find_cc_toolchain")
 load("@rules_cc//cc/common:cc_common.bzl", "cc_common")
 load("@rules_cc//cc/common:cc_info.bzl", "CcInfo")
-load("//build/tools/clang_tidy:check_path_filters.bzl", "CHECK_PATH_FILTERS")
+load("//build/tools/clang_tidy:check_path_filters.bzl", "CHECK_PATH_FILTERS", "HEADER_ONLY_CHECKS")
 
 def _get_disabled_checks_for_file(file_path):
     """Returns checks that should be disabled for this file.
 
     Checks listed in CHECK_PATH_FILTERS are only enabled for files under their
     allowed paths. For files not under any allowed path, the check is disabled.
+    Checks listed in HEADER_ONLY_CHECKS are only enabled for header files and
+    disabled otherwise.
     """
     disabled = []
     for check, allowed_paths in CHECK_PATH_FILTERS.items():
@@ -30,6 +32,11 @@ def _get_disabled_checks_for_file(file_path):
                 break
         if not enabled:
             disabled.append(check)
+
+    for check in HEADER_ONLY_CHECKS:
+        if not file_path.endswith(".h"):
+            disabled.append(check)
+
     return disabled
 
 def _clang_tidy_aspect_impl(target, ctx):
@@ -60,6 +67,12 @@ def _clang_tidy_aspect_impl(target, ctx):
     # we use $location in our copts, expand it
     rule_copts = [ctx.expand_location(opt) for opt in rule_copts]
 
+    # disable clang tidy if no-clang-tidy tag is defined. For no-clang-tidy-
+    # headers, we only disable it on header files.
+    # todo: figure out a better way to control clang tidy on a per-target basis.
+    if "no-clang-tidy" in ctx.rule.attr.tags:
+        return []
+
     srcs = []
     if hasattr(ctx.rule.attr, "srcs"):
         for src in ctx.rule.attr.srcs:
@@ -68,7 +81,7 @@ def _clang_tidy_aspect_impl(target, ctx):
                 for src in src.files.to_list()
                 if src.is_source and src.short_path.endswith((".c++", ".c", ".h"))
             ]
-    if hasattr(ctx.rule.attr, "hdrs"):
+    if hasattr(ctx.rule.attr, "hdrs") and not "no-clang-tidy-headers" in ctx.rule.attr.tags:
         for src in ctx.rule.attr.hdrs:
             srcs += [
                 src
@@ -84,11 +97,6 @@ def _clang_tidy_aspect_impl(target, ctx):
     external_includes = compilation_context.external_includes.to_list()
     headers = compilation_context.headers
 
-    # disable clang tidy if no-clang-tidy tag is defined.
-    # todo: figure out a better way to control clang tidy on a per-target basis.
-    if "no-clang-tidy" in ctx.rule.attr.tags:
-        return []
-
     # bazel doesn't expose implementation deps through compilation context
     # https://github.com/bazelbuild/bazel/issues/19663
     if hasattr(ctx.rule.attr, "implementation_deps"):

src/rust/cxx/include/cxx.h

@@ -8,12 +8,10 @@
 #include <initializer_list>
 #include <iosfwd>
 #include <iterator>
-#include <new>
 #include <stdexcept>
 #include <string>
 #include <type_traits>
 #include <utility>
-#include <vector>
 #ifdef _WIN32
 #include <basetsd.h>
 #else

src/rust/cxx/kj-rs/future.h

@@ -5,7 +5,6 @@
 
 #include <kj/debug.h>
 
-#include <concepts>
 #include <cstdint>
 
 namespace kj_rs {

src/rust/cxx/kj-rs/kj-rs.h

@@ -1,8 +1,11 @@
 #pragma once
 
+// This file is intentionally used as a catchall kj-rs header
+// NOLINTBEGIN(misc-include-cleaner)
 // KJ-C++ conversion utilities
 #include "kj-rs/convert.h"
 // Rust futures support
 #include "kj-rs/future.h"
 // KJ promises support
 #include "kj-rs/promise.h"
+// NOLINTEND(misc-include-cleaner)

src/rust/cxx/src/cxx.cc

@@ -5,6 +5,7 @@
 #include <cstring>
 #include <iostream>
 #include <memory>
+#include <vector>
 
 extern "C" {
 void cxxbridge1$cxx_string$init(std::string *s, const std::uint8_t *ptr,

src/rust/cxx/tests/ffi/tests.h

@@ -7,6 +7,7 @@
 #include <atomic>
 #include <memory>
 #include <string>
+#include <vector>
 
 namespace A {
 struct AShared;

src/rust/jsg-test/ffi.h

@@ -8,9 +8,8 @@
 #include <workerd/rust/jsg/ffi.h>
 #include <workerd/rust/jsg/v8.rs.h>
 
-#include <kj-rs/kj-rs.h>
 #include <rust/cxx.h>
-#include <v8.h>
+#include <v8-isolate.h>
 
 #include <kj/function.h>
 #include <kj/memory.h>

src/rust/jsg/BUILD.bazel

@@ -26,7 +26,6 @@ wd_cc_library(
     srcs = [],
     hdrs = ["jsg.h"],
     local_defines = ["JSG_IMPLEMENTATION"],
-    tags = ["no-clang-tidy"],
     visibility = ["//visibility:public"],
     deps = [":jsg"],
 )
@@ -38,7 +37,6 @@ wd_cc_library(
         "ffi.h",
     ],
     local_defines = ["JSG_IMPLEMENTATION"],
-    tags = ["no-clang-tidy"],
     textual_hdrs = [
         "ffi-inl.h",
     ],

src/rust/jsg/ffi.c++

@@ -78,7 +78,7 @@ static v8::Local<v8::String> makeInternedStr(v8::Isolate* isolate, const Name& n
 }
 
 // Wrappable implementation - calls into Rust via CXX bridge
-Wrappable::~Wrappable() {
+Wrappable::~Wrappable() noexcept(false) {
   wrappable_invoke_drop(*this);
 }
 

src/rust/jsg/ffi.h

@@ -4,12 +4,10 @@
 
 #pragma once
 
-#include <workerd/jsg/modules.capnp.h>
 #include <workerd/jsg/wrappable.h>
 
-#include <kj-rs/kj-rs.h>
 #include <rust/cxx.h>
-#include <v8.h>
+#include <v8-isolate.h>
 
 #include <kj/function.h>
 #include <kj/memory.h>
@@ -56,7 +54,7 @@ struct TraitObjectPtr {
 // and destruction without knowing the concrete type at compile time.
 class Wrappable: public ::workerd::jsg::Wrappable {
  public:
-  ~Wrappable();
+  ~Wrappable() noexcept(false);
   void jsgVisitForGc(::workerd::jsg::GcVisitor& visitor) override;
   kj::StringPtr jsgGetMemoryName() const override;
   size_t jsgGetMemorySelfSize() const override;

src/rust/jsg/jsg.h

@@ -11,7 +11,6 @@
 #include <workerd/rust/jsg/ffi.h>
 #include <workerd/rust/jsg/v8.rs.h>
 
-#include <kj-rs/kj-rs.h>
 #include <rust/cxx.h>
 
 #include <kj/function.h>

src/rust/worker/bridge.h

@@ -6,8 +6,6 @@
 
 #include <workerd/rust/worker/ffi.rs.h>
 
-#include <kj-rs/kj-rs.h>
-
 namespace workerd::rust::worker {
 
 inline workerd::EventOutcome fromImpl(kj_rs::Rust*, workerd::rust::worker::EventOutcome outcome) {

src/workerd/api/container.h

@@ -8,7 +8,7 @@
 #include <workerd/api/basics.h>
 #include <workerd/api/js-readable-stream.h>
 #include <workerd/api/js-writable-stream.h>
-#include <workerd/io/compatibility-date.h>
+#include <workerd/io/compatibility-date.capnp.h>
 #include <workerd/io/container.capnp.h>
 #include <workerd/io/io-own.h>
 #include <workerd/jsg/jsg.h>

src/workerd/api/crypto/impl.h

@@ -9,6 +9,8 @@
 
 #include "crypto.h"
 
+// For fastEncodeBase64Url, widely used in crypto implementation
+// NOLINTNEXTLINE(misc-include-cleaner)
 #include <workerd/api/util.h>
 #include <workerd/jsg/jsvalue.h>
 

src/workerd/api/data-url.c++

@@ -4,6 +4,7 @@
 
 #include <workerd/util/strings.h>
 
+#include <kj/debug.h>
 #include <kj/vector.h>
 
 namespace workerd::api {

src/workerd/api/eventsource.c++

@@ -4,8 +4,8 @@
 
 #include "eventsource.h"
 
+#include "events.h"
 #include "http.h"
-#include "messagechannel.h"
 #include "streams/common.h"
 
 #include <workerd/io/features.h>

src/workerd/api/eventsource.h

@@ -4,7 +4,6 @@
 
 #pragma once
 #include "basics.h"
-#include "events.h"
 #include "http.h"
 
 #include <workerd/jsg/jsg.h>

src/workerd/api/fuzzilli.c++

@@ -6,7 +6,12 @@
 #include <workerd/util/immediate-crash.h>
 
 #include <errno.h>
+#include <fcntl.h>
 #include <string.h>
+#include <sys/mman.h>
+#include <sys/stat.h>
+#include <sys/wait.h>
+#include <unistd.h>
 
 #include <kj/common.h>
 #include <kj/debug.h>

src/workerd/api/fuzzilli.h

@@ -4,13 +4,8 @@
 #include <workerd/jsg/jsg.h>
 
 #include <assert.h>
-#include <fcntl.h>
 #include <stdio.h>
 #include <stdlib.h>
-#include <sys/mman.h>
-#include <sys/stat.h>
-#include <sys/wait.h>
-#include <unistd.h>
 
 #include <cstdint>
 

src/workerd/api/global-scope.h

@@ -5,6 +5,7 @@
 #pragma once
 
 #include "basics.h"
+#include "events.h"
 #include "filesystem.h"
 #include "http.h"
 #include "messagechannel.h"

src/workerd/api/html-rewriter.h

@@ -7,7 +7,7 @@
 #include <workerd/api/http.h>
 #include <workerd/jsg/jsg.h>
 
-#include <v8.h>
+#include <v8-template.h>
 
 struct lol_html_HtmlRewriterBuilder;
 struct lol_html_HtmlRewriter;

src/workerd/api/messagechannel.h

@@ -1,10 +1,8 @@
 #pragma once
 
 #include <workerd/api/basics.h>
-#include <workerd/io/io-context.h>
 #include <workerd/jsg/jsg.h>
 #include <workerd/jsg/modules-new.h>
-#include <workerd/jsg/ser.h>
 #include <workerd/jsg/url.h>
 
 namespace workerd::api {

src/workerd/api/modules.h

@@ -8,7 +8,6 @@
 #include <workerd/api/filesystem.h>
 #include <workerd/api/messagechannel.h>
 #include <workerd/api/node/node.h>
-#include <workerd/api/pyodide/pyodide.h>
 #include <workerd/api/rtti.h>
 #include <workerd/api/sockets.h>
 #include <workerd/api/tracing.h>

src/workerd/api/node/node.h

@@ -13,7 +13,6 @@
 #include <workerd/api/node/timers.h>
 #include <workerd/api/node/url.h>
 #include <workerd/api/node/util.h>
-#include <workerd/io/compatibility-date.capnp.h>
 #include <workerd/jsg/jsg.h>
 #include <workerd/jsg/modules-new.h>
 #include <workerd/jsg/url.h>
@@ -23,8 +22,6 @@
 
 #include <node/node.capnp.h>
 
-#include <capnp/dynamic.h>
-
 namespace workerd::api::node {
 
 #define NODEJS_MODULES(V)                                                                          \

src/workerd/api/node/process.c++

@@ -3,6 +3,8 @@
 //     https://opensource.org/licenses/Apache-2.0
 #include "process.h"
 
+#include "node-version.h"
+
 #include <workerd/api/filesystem.h>
 #include <workerd/api/node/exceptions.h>
 #include <workerd/io/features.h>

src/workerd/api/node/process.h

@@ -3,9 +3,6 @@
 //     https://opensource.org/licenses/Apache-2.0
 #pragma once
 
-#include <workerd/api/node/i18n.h>
-#include <workerd/api/node/node-version.h>
-#include <workerd/io/worker-fs.h>
 #include <workerd/jsg/jsg.h>
 
 namespace workerd::api::node {

src/workerd/api/pyodide/pyodide.h

@@ -5,11 +5,9 @@
 
 #include <workerd/io/compatibility-date.capnp.h>
 #include <workerd/jsg/jsg.h>
-#include <workerd/jsg/modules-new.h>
 #include <workerd/util/strong-bool.h>
 
 #include <pyodide/generated/pyodide_extra.capnp.h>
-#include <pyodide/pyodide_static.capnp.h>
 #include <pyodide/python_packages.capnp.h>
 
 #include <capnp/serialize.h>

src/workerd/api/r2-bucket.h

@@ -6,7 +6,7 @@
 
 #include "r2-rpc.h"
 
-#include <workerd/api/streams/readable.h>
+#include <workerd/api/js-readable-stream.h>
 #include <workerd/jsg/jsg.h>
 
 namespace workerd::api {

src/workerd/api/r2.h

@@ -4,7 +4,9 @@
 
 #pragma once
 
+// NOLINTNEXTLINE(misc-include-cleaner)
 #include "r2-bucket.h"
+// NOLINTNEXTLINE(misc-include-cleaner)
 #include "r2-multipart.h"
 
 namespace workerd::api::public_beta {