[nfc] clang-tidy

Author: mikea

.bazelrc

@@ -30,8 +30,8 @@ build --incompatible_autoload_externally="+py_binary,+py_library,+ProtoInfo,+sh_
 # Import CI-specific configuration. As the amount of custom configuration settings we use grows,
 # consider moving more flags out to separate files.
 import %workspace%/build/ci.bazelrc
-
 import %workspace%/build/rust_lint.bazelrc
+import %workspace%/build/tools/clang_tidy/clang_tidy.bazelrc
 
 # Prevents bazel cache invalidation when switching terminals
 build --incompatible_strict_action_env

.clang-tidy

@@ -4,13 +4,35 @@
 # TODO: these checks are in progress of cleaning up
 Checks: >
   -*,
+  bugprone-argument-comment,
   bugprone-capturing-this-in-member-variable,
+  bugprone-forward-declaration-namespace,
   bugprone-move-forwarding-reference,
-  bugprone-return-const-ref-from-parameter,
   bugprone-use-after-move,
+  cppcoreguidelines-misleading-capture-default-by-value,
+  misc-confusable-identifiers,
   misc-header-include-cycle,
+  misc-throw-by-value-catch-by-reference,
+  misc-unused-alias-decls,
+  modernize-macro-to-enum,
+  modernize-redundant-void-arg,
+  modernize-unary-static-assert,
+  modernize-use-bool-literals,
+  performance-for-range-copy,
+  performance-move-constructor-init,
+  readability-container-contains,
   readability-duplicate-include,
+  readability-redundant-access-specifiers,
+  readability-static-accessed-through-instance
+
+# TODO: Fix and re-enable
+# bugprone-return-const-ref-from-parameter
+# modernize-type-traits
+# modernize-use-override
+# readability-redundant-casting
+
 WarningsAsErrors: '*'
+HeaderFilterRegex: '.*/workerd/.*'
 
 CheckOptions:
   # JSG has very entrenched include cycles

.github/workflows/test.yml

@@ -98,7 +98,7 @@ jobs:
   lint:
     uses: ./.github/workflows/_bazel.yml
     with:
-      extra_bazel_args: '--config=lint --config=ci-test --config=ci-linux'
+      extra_bazel_args: '--config=lint --config=clang-tidy --config=ci-test --config=ci-linux'
       run_tests: false
       parse_headers: true
     secrets:

BUILD.bazel

@@ -22,6 +22,7 @@ wd_cc_embed(
 alias(
     name = "v8_icu",
     actual = "@v8//:v8_icu",
+    tags = ["manual"],
     visibility = ["//visibility:public"],
 )
 
@@ -122,3 +123,10 @@ selects.config_setting_group(
         ":not_dbg_build",
     ],
 )
+
+# Clang-tidy config to use
+label_flag(
+    name = "clang_tidy_config",
+    build_setting_default = ":.clang-tidy",
+    visibility = ["//visibility:public"],
+)

MODULE.bazel

@@ -32,6 +32,33 @@ git_override(
     remote = "https://chromium.googlesource.com/chromium/src/third_party/zlib.git",
 )
 
+# clang-tidy
+http_file = use_repo_rule("@bazel_tools//tools/build_defs/repo:http.bzl", "http_file")
+http_file(
+    name = "clang_tidy_macos_arm64",
+    url = "https://github.com/cloudflare/workerd-tools/releases/download/clang-tidy-21.1.4/llvm-21.1.4-darwin-arm64-clang-tidy",
+    executable = True,
+    integrity = "sha256-vzU7J99wf6a/DMsLJ7/q5f3JckU0i3kbNAtck3vO5oA="
+)
+http_file(
+    name = "clang_tidy_linux_amd64",
+    url = "https://github.com/cloudflare/workerd-tools/releases/download/clang-tidy-21.1.4/llvm-21.1.4-linux-amd64-clang-tidy",
+    executable = True,
+    integrity = "sha256-6B+qIjJhCXJpRPp9nSQxnchEzuHH7UL5mWNrcGU/3z8="
+)
+http_file(
+    name = "clang_tidy_linux_arm64",
+    url = "https://github.com/cloudflare/workerd-tools/releases/download/clang-tidy-21.1.4/llvm-21.1.4-linux-arm64-clang-tidy",
+    executable = True,
+    integrity = "sha256-o4grMwBpnxMv+291b4ZbNHvmgtzUhjJL6KlGENuq/5E="
+)
+http_file(
+    name = "clang_tidy_windows_amd64",
+    url = "https://github.com/cloudflare/workerd-tools/releases/download/clang-tidy-21.1.4/llvm-21.1.4-windows-amd64-clang-tidy.exe",
+    executable = True,
+    integrity = "sha256-9KJLz6bHbwpj5yAkjJzUQV0oL+ZrFsI2Wo5Rpjg69vc="
+)
+
 # BoringSSL may subtly break backwards compatibility and behave differently than the latest FIPS
 # version, often by rejecting key values that it considers invalid/unsafe even though they are still
 # accepted by BoringSSL. Update with caution and only after confirming this is compatible with the

build/tools/clang_tidy/BUILD

@@ -0,0 +1 @@
+exports_files(["clang_tidy_wrapper.sh"])

build/tools/clang_tidy/clang_tidy.bazelrc

@@ -0,0 +1,5 @@
+# enable clang tidy checks with default configuration
+build:clang-tidy --aspects //build/tools/clang_tidy:clang_tidy.bzl%clang_tidy_aspect --output_groups=+clang_tidy_checks
+
+# enable clang tidy check with all issues reported as warnings
+build:clang-tidy-warnings --config=clang-tidy --aspects_parameters=clang_tidy_args=--warnings-as-errors=-*

build/tools/clang_tidy/clang_tidy.bzl

@@ -0,0 +1,195 @@
+"""Clang tidy aspect.
+
+The aspect, when enabled runs clang_tidy on every compiled c++ file.
+"""
+
+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")
+
+def _clang_tidy_aspect_impl(target, ctx):
+    # not a c++ target
+    if not CcInfo in target:
+        return []
+
+    cc_toolchain = find_cc_toolchain(ctx)
+    feature_configuration = cc_common.configure_features(
+        ctx = ctx,
+        cc_toolchain = cc_toolchain,
+    )
+    compile_variables = cc_common.create_compile_variables(
+        feature_configuration = feature_configuration,
+        cc_toolchain = cc_toolchain,
+        user_compile_flags = ctx.fragments.cpp.cxxopts + ctx.fragments.cpp.copts,
+    )
+    toolchain_flags = cc_common.get_memory_inefficient_command_line(
+        feature_configuration = feature_configuration,
+        action_name = ACTION_NAMES.cpp_compile,
+        variables = compile_variables,
+    )
+
+    compilation_context = target[CcInfo].compilation_context
+
+    rule_copts = getattr(ctx.rule.attr, "copts", [])
+
+    # we use $location in our copts, expand it
+    rule_copts = [ctx.expand_location(opt) for opt in rule_copts]
+
+    srcs = []
+    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 src.short_path.endswith((".c++", ".c", ".h"))
+            ]
+    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 src.short_path.endswith((".c++", ".c", ".h"))
+            ]
+
+    defines = compilation_context.defines.to_list()
+    local_defines = compilation_context.local_defines.to_list()
+    includes = compilation_context.includes.to_list()
+    quote_includes = compilation_context.quote_includes.to_list()
+    system_includes = compilation_context.system_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"):
+        deps = [dep[CcInfo].compilation_context for dep in ctx.rule.attr.implementation_deps if CcInfo in dep]
+        defines = depset(
+            defines,
+            transitive = [dep.defines for dep in deps],
+        )
+        includes = depset(
+            includes,
+            transitive = [dep.includes for dep in deps],
+        )
+        system_includes = depset(
+            system_includes,
+            transitive = [dep.system_includes for dep in deps],
+        )
+        quote_includes = depset(
+            quote_includes,
+            transitive = [dep.quote_includes for dep in deps],
+        )
+        headers = depset(
+            headers.to_list(),
+            transitive = [dep.headers for dep in deps],
+        )
+
+    tools = [
+        ctx.attr._clang_tidy_executable.files,
+        ctx.attr._clang_tidy_wrapper.files,
+        ctx.attr._clang_tidy_config.files,
+    ]
+
+    outs = []
+    for src in srcs:
+        # run actions need to produce something, declare a dummy file
+        # multiple labels can use the same path, so disambiguate.
+        out = ctx.actions.declare_file(src.path + "." + ctx.label.name + ".clang_tidy")
+        outs.append(out)
+
+        args = ctx.actions.args()
+
+        # these are consumed by clang_tidy_wrapper,sh
+        args.add(ctx.attr._clang_tidy_executable.files_to_run.executable)
+        args.add(out)
+
+        # clang-tidy arguments
+        # do not print statistics
+        args.add("--quiet")
+        args.add("--config-file=" + ctx.attr._clang_tidy_config.files.to_list()[0].short_path)
+
+        if ctx.attr.clang_tidy_args:
+            args.add_all(ctx.attr.clang_tidy_args.split(" "))
+
+        args.add(src.path)
+
+        # compiler arguments
+        args.add("--")
+
+        args.add("-xc++")
+
+        args.add_all(ctx.attr._clang_tidy_compiler_flags)
+        args.add_all(rule_copts)
+        args.add_all(defines, before_each = "-D")
+        args.add_all(local_defines, before_each = "-D")
+        args.add_all(includes, before_each = "-I")
+        args.add_all(quote_includes, before_each = "-iquote")
+        args.add_all(system_includes, before_each = "-isystem")
+
+        args.add_all(toolchain_flags)
+
+        # Silence warnings about unused functions or #pragma once being present in header files. For
+        # source files, we already cover these warnings in regular compilation
+        args.add("-Wno-pragma-once-outside-header")
+        args.add("-Wno-unused")
+
+        # TODO(cleanup): These paths provide required includes, but if the toolchain was working
+        # properly we wouldn't need them in the first place...
+        # Linux includes
+        args.add("-isystem/usr/lib/llvm-19/include/c++/v1")
+        args.add("-isystem/usr/lib/llvm-19/lib/clang/19/include")
+        args.add("-isystem/usr/include")
+        args.add("-isystem/usr/include/x86_64-linux-gnu")
+
+        # macOS includes
+        args.add("-isystem/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1")
+        args.add("-isystem/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/lib/clang/17/include")
+        args.add("-isystem/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include")
+
+        inputs = depset(
+            direct = [src],
+            transitive = [headers],
+        )
+
+        ctx.actions.run(
+            outputs = [out],
+            arguments = [args],
+            executable = ctx.attr._clang_tidy_wrapper.files_to_run.executable,
+            progress_message = "Run clang-tidy on {}".format(src.short_path),
+            tools = tools,
+            mnemonic = "ClangTidy",
+            inputs = inputs,
+        )
+
+    return [
+        OutputGroupInfo(clang_tidy_checks = depset(direct = outs)),
+    ]
+
+clang_tidy_aspect = aspect(
+    implementation = _clang_tidy_aspect_impl,
+    fragments = ["cpp"],
+    attrs = {
+        "_clang_tidy_wrapper": attr.label(
+            default = Label("@//build/tools/clang_tidy:clang_tidy_wrapper.sh"),
+            allow_single_file = True,
+        ),
+        "_clang_tidy_executable": attr.label(
+            default = Label("//tools:clang-tidy"),
+            allow_single_file = True,
+        ),
+        "_clang_tidy_config": attr.label(
+            default = Label("//:clang_tidy_config"),
+            allow_single_file = True,
+        ),
+        "_clang_tidy_compiler_flags": attr.string_list(
+            default = [],
+        ),
+        "clang_tidy_args": attr.string(default = ""),
+    },
+    toolchains = ["@bazel_tools//tools/cpp:toolchain_type"],
+)

build/tools/clang_tidy/clang_tidy_wrapper.sh

@@ -0,0 +1,36 @@
+#! /bin/bash
+# simple wrapper script to execute clang-tidy
+
+set -euo pipefail
+
+CLANG_TIDY_BIN=$1
+shift
+
+OUTPUT=$1
+shift
+
+PWD=$(pwd)/
+ESCAPED_PWD=$(sed 's/[\*\.&/]/\\&/g' <<< "$PWD")
+
+# Interestingly clang-tidy prints real errors to stdout, but system message like
+# `4 warnings generated` when they are filtered out, to stderr.
+# Save stderr and print only on errors to reduce the clutter.
+CLANG_TIDY_STDERR=$(mktemp)
+
+set +e
+"${CLANG_TIDY_BIN}" "$@" 2>"$CLANG_TIDY_STDERR" | \
+  # clang-tidy insists on printing absolute file paths, chop current dir off
+  sed "s/$ESCAPED_PWD//g"
+CLANG_TIDY_EXIT_CODE=$?
+set -e
+
+if [[ $CLANG_TIDY_EXIT_CODE -ne 0 ]]; then
+  cat "$CLANG_TIDY_STDERR" >&2
+  rm -f "$CLANG_TIDY_STDERR"
+  exit $CLANG_TIDY_EXIT_CODE
+fi
+
+rm -f "$CLANG_TIDY_STDERR"
+
+# bazel needs run action to produce some output, touch the file
+touch "$OUTPUT"

src/workerd/api/eventsource.h

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