diff --git a/.bazelrc b/.bazelrc index cb3a4f0e108..f276d4922e3 100644 --- a/.bazelrc +++ b/.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 diff --git a/.clang-tidy b/.clang-tidy index fd3208663fb..2f47792a90f 100644 --- a/.clang-tidy +++ b/.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 diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 52d7f9b08d4..2ab7bee8e5e 100644 --- a/.github/workflows/test.yml +++ b/.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: diff --git a/BUILD.bazel b/BUILD.bazel index d42d7004fce..56890a353bb 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -21,6 +21,7 @@ wd_cc_embed( alias( name = "v8_icu", actual = "@v8//:v8_icu", + tags = ["manual"], visibility = ["//visibility:public"], ) @@ -107,3 +108,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"], +) diff --git a/MODULE.bazel b/MODULE.bazel index 975a1a7e48c..073fde63337 100644 --- a/MODULE.bazel +++ b/MODULE.bazel @@ -31,6 +31,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 diff --git a/build/tools/clang_tidy/BUILD b/build/tools/clang_tidy/BUILD new file mode 100644 index 00000000000..1f995c978aa --- /dev/null +++ b/build/tools/clang_tidy/BUILD @@ -0,0 +1 @@ +exports_files(["clang_tidy_wrapper.sh"]) diff --git a/build/tools/clang_tidy/clang_tidy.bazelrc b/build/tools/clang_tidy/clang_tidy.bazelrc new file mode 100644 index 00000000000..90d9b993486 --- /dev/null +++ b/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=-* diff --git a/build/tools/clang_tidy/clang_tidy.bzl b/build/tools/clang_tidy/clang_tidy.bzl new file mode 100644 index 00000000000..1f3310fca74 --- /dev/null +++ b/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"], +) diff --git a/build/tools/clang_tidy/clang_tidy_wrapper.sh b/build/tools/clang_tidy/clang_tidy_wrapper.sh new file mode 100755 index 00000000000..4710c249823 --- /dev/null +++ b/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" diff --git a/src/workerd/api/eventsource.h b/src/workerd/api/eventsource.h index c55982bfd5d..0f54abd6c73 100644 --- a/src/workerd/api/eventsource.h +++ b/src/workerd/api/eventsource.h @@ -4,6 +4,7 @@ #pragma once #include "basics.h" +#include "http.h" #include #include diff --git a/src/workerd/api/pyodide/pyodide.c++ b/src/workerd/api/pyodide/pyodide.c++ index bcf0367a959..1283c3bb3a4 100644 --- a/src/workerd/api/pyodide/pyodide.c++ +++ b/src/workerd/api/pyodide/pyodide.c++ @@ -21,6 +21,8 @@ #include // for std::sort +#include + #include namespace workerd::api::pyodide { @@ -573,11 +575,6 @@ void SetupEmscripten::visitForGc(jsg::GcVisitor& visitor) { } // namespace workerd::api::pyodide -#include "workerd/io/compatibility-date.h" - -#include -#include - namespace workerd { struct PythonSnapshotParsedField { diff --git a/src/workerd/jsg/BUILD.bazel b/src/workerd/jsg/BUILD.bazel index 39892a715f7..d91d932a31a 100644 --- a/src/workerd/jsg/BUILD.bazel +++ b/src/workerd/jsg/BUILD.bazel @@ -112,6 +112,7 @@ wd_cc_library( # Some JSG headers can't be compiled on their own features = ["-parse_headers"], local_defines = ["JSG_IMPLEMENTATION"], + tags = ["no-clang-tidy"], deps = [ ":exception", ":macro-meta", diff --git a/src/workerd/jsg/jsg.h b/src/workerd/jsg/jsg.h index d64d6e32761..5f06e01dea0 100644 --- a/src/workerd/jsg/jsg.h +++ b/src/workerd/jsg/jsg.h @@ -3005,6 +3005,8 @@ inline Value SelfRef::asValue(Lock& js) const { // These includes are needed for the JSG type glue macros to work. #include "modules.h" #include "resource.h" +// JSG has very entrenched include cycles +// NOLINTNEXTLINE(misc-header-include-cycle) #include "jsvalue.h" // clang-format on diff --git a/src/workerd/jsg/resource.h b/src/workerd/jsg/resource.h index 97c89246d87..688aafeed94 100644 --- a/src/workerd/jsg/resource.h +++ b/src/workerd/jsg/resource.h @@ -14,6 +14,8 @@ #include #include #include +// JSG has very entrenched include cycles +// NOLINTNEXTLINE(misc-header-include-cycle) #include #include #include diff --git a/tools/BUILD.bazel b/tools/BUILD.bazel index 82e6a55731d..2ae47ab0687 100644 --- a/tools/BUILD.bazel +++ b/tools/BUILD.bazel @@ -1,5 +1,6 @@ load("@aspect_rules_js//js:defs.bzl", "js_library") load("@aspect_rules_ts//ts:defs.bzl", "ts_config") +load("@bazel_skylib//rules:native_binary.bzl", "native_binary") js_library( name = "base-eslint", @@ -19,3 +20,17 @@ ts_config( src = "base.tsconfig.json", visibility = ["//visibility:public"], ) + +native_binary( + name = "clang-tidy", + src = select( + { + "@bazel_tools//src/conditions:linux_x86_64": "@clang_tidy_linux_amd64//file:downloaded", + "@bazel_tools//src/conditions:linux_aarch64": "@clang_tidy_linux_arm64//file:downloaded", + "@bazel_tools//src/conditions:darwin_arm64": "@clang_tidy_macos_arm64//file:downloaded", + "@bazel_tools//src/conditions:windows_x64": "@clang_tidy_windows_amd64//file:downloaded", + }, + ), + out = "clang_tidy", + visibility = ["//visibility:public"], +)