Feat: clang-tidy for linting of C/C++ code (#287)

* install clang-tidy and test data

* fix: some windows issues

* feat: clang-tidy aspect

* feat: integrate clang-tidy aspect into example

* feat: avoid missing linters on windows

* fix: fix compile error

* fix: code tidy

* fix: extra debugging for --fix mode

* feat: support clang-tidy --fix on linux

* fix: bug

* fix: chmod=+x

* fix: tests

* fix: typo

* fix: remove workaround_windows.bat

* fix: show all reports on windows

* chore: update docs

* fix: address comments

* fix: address comments

* fix: address comment

* fix: send multiple files to clang-tidy

* fix: typo

* fix: make wrapper executable

* chore: green up buildifier

* chore: update docs

* chore: fix integration test

---------

Co-authored-by: Alex Eagle <[email protected]>

docs/BUILD.bazel

@@ -52,4 +52,9 @@ stardoc_with_diff_test(
     bzl_library_target = "//lint:ktlint",
 )
 
+stardoc_with_diff_test(
+    name = "clang-tidy",
+    bzl_library_target = "//lint:clang_tidy",
+)
+
 update_docs(name = "update")

example/.bazelrc

@@ -1,6 +1,22 @@
+# Automatically apply --config=linux, --config=windows etc
+common --enable_platform_specific_config
+
 # Don't depend on a JAVA_HOME pointing at a system JDK
 # see https://github.com/bazelbuild/rules_jvm_external/issues/445
 build --repo_env=JAVA_HOME=../bazel_tools/jdk
 
 common --incompatible_enable_proto_toolchain_resolution
 common --@aspect_rules_ts//ts:skipLibCheck=always
+
+# enable symlinks on windows
+startup --windows_enable_symlinks
+
+# c++ options
+common:linux --action_env=BAZEL_CXXOPTS="-std=c++20"
+common:windows --action_env=BAZEL_CXXOPTS="/std:c++20"
+
+# clang-tidy needs the INCLUDE variable set to the system value. 
+# Would prefer to do this inside the aspect, not sure if it is possible.
+common --action_env=INCLUDE
+# ensure that minimal other envvars are passed by clang-tidy run_shell
+common --incompatible_strict_action_env

example/.clang-tidy

@@ -0,0 +1,19 @@
+# https://clang.llvm.org/extra/clang-tidy/
+# config-file to provoke some checks in example
+---
+Checks: "*,
+  -abseil-*,
+  -altera-*,
+  -android-*,
+  -fuchsia-*,
+  -google-*,
+  -llvm*,
+  -modernize-use-trailing-return-type,
+  -zircon-*,
+  -readability-else-after-return,
+  -readability-static-accessed-through-instance,
+  -readability-avoid-const-params-in-decls,
+  -cppcoreguidelines-non-private-member-variables-in-classes,
+  -misc-non-private-member-variables-in-classes,"
+# promote a random warning to error to test integration. Do not copy this line.
+WarningsAsErrors: "readability-inconsistent-declaration-parameter-name"

example/BUILD.bazel

@@ -22,6 +22,7 @@ exports_files(
         ".vale.ini",
         ".editorconfig",
         "ktlint-baseline.xml",
+        ".clang-tidy",
     ],
     visibility = ["//visibility:public"],
 )

example/MODULE.bazel

@@ -17,6 +17,7 @@ bazel_dep(name = "rules_rust", version = "0.45.1")
 bazel_dep(name = "buildifier_prebuilt", version = "6.3.3")
 bazel_dep(name = "platforms", version = "0.0.8")
 bazel_dep(name = "rules_kotlin", version = "1.9.0")
+bazel_dep(name = "rules_cc", version = "0.0.9")
 
 local_path_override(
     module_name = "aspect_rules_lint",

example/lint.sh

@@ -22,9 +22,26 @@ fix=""
 buildevents=$(mktemp)
 filter='.namedSetOfFiles | values | .files[] | select(.name | endswith($ext)) | ((.pathPrefix | join("/")) + "/" + .name)'
 
+unameOut="$(uname -s)"
+case "${unameOut}" in
+    Linux*)     machine=Linux;;
+    Darwin*)    machine=Mac;;
+    CYGWIN*)    machine=Windows;;
+    MINGW*)     machine=Windows;;
+    MSYS_NT*)   machine=Windows;;
+    *)          machine="UNKNOWN:${unameOut}"
+esac
+
+args=()
+if [ $machine == "Windows" ]; then
+    # avoid missing linters on windows platform
+    args=("--aspects=$(echo //tools/lint:linters.bzl%{flake8,pmd,ruff,vale,clang_tidy} | tr ' ' ',')")
+else
+    args=("--aspects=$(echo //tools/lint:linters.bzl%{buf,eslint,flake8,ktlint,pmd,ruff,shellcheck,vale,clang_tidy} | tr ' ' ',')")
+fi
+
 # NB: perhaps --remote_download_toplevel is needed as well with remote execution?
-args=(
-	"--aspects=$(echo //tools/lint:linters.bzl%{buf,eslint,flake8,ktlint,pmd,ruff,shellcheck,vale} | tr ' ' ',')"
+args+=(
 	# Allow lints of code that fails some validation action
 	# See https://github.com/aspect-build/rules_ts/pull/574#issuecomment-2073632879
 	"--norun_validations"
@@ -59,7 +76,12 @@ fi
 bazel build ${args[@]} $@
 
 # TODO: Maybe this could be hermetic with bazel run @aspect_bazel_lib//tools:jq or sth
-valid_reports=$(jq --arg ext report --raw-output "$filter" "$buildevents")
+if [ $machine == "Windows" ]; then
+    # jq on windows outputs CRLF which breaks this script. https://github.com/jqlang/jq/issues/92
+    valid_reports=$(jq --arg ext report --raw-output "$filter" "$buildevents" | tr -d '\r')
+else
+    valid_reports=$(jq --arg ext report --raw-output "$filter" "$buildevents")
+fi
 
 # Show the results.
 while IFS= read -r report; do
@@ -77,7 +99,7 @@ done <<<"$valid_reports"
 # [*] 1 fixable with the `--fix` option.
 # so that the naive thing of pasting that flag to lint.sh will do what the user expects.
 if [ -n "$fix" ]; then
-	valid_patches=$(jq --arg ext patch --raw-output "$filter" "$buildevents")
+	valid_patches=$valid_reports
 	while IFS= read -r patch; do
 		# Exclude coverage reports, and check if the report is empty.
 		if [[ "$patch" == *coverage.dat ]] || [[ ! -s "$patch" ]]; then

example/src/cpp/lib/BUILD

@@ -0,0 +1,18 @@
+load("@rules_cc//cc:defs.bzl", "cc_library")
+
+exports_files(["get/.clang-tidy"])
+
+cc_library(
+    name = "hello-time",
+    srcs = [
+        "get/get-time.cc",
+        "get/get-time.h",
+        "hello-time.cc",
+    ],
+    hdrs = [
+        "hello-time.h",
+        "xhello-time.h",
+    ],
+    includes = ["."],
+    visibility = ["//src/cpp/main:__pkg__"],
+)

example/src/cpp/lib/get/.clang-tidy

@@ -0,0 +1,7 @@
+# https://clang.llvm.org/extra/clang-tidy/
+# this is an example of a .clang-tidy file in a subdirectory that overrides
+# the configuration for files in this directory and below. It must be made
+# visibile through an export_files declaration in this package and added
+# to the 'configs' attribute in clang_tidy aspect.
+---
+Checks: "-*, misc-const-correctness"

example/src/cpp/lib/get/get-time.cc

@@ -0,0 +1,13 @@
+#include "get-time.h"
+#include <ctime>
+#include <string>
+
+// Some deliberately bad code
+const char* get_localtime_impl() {
+  // warning: variable 'result' of type 'std::time_t' (aka 'long long') can be declared 'const' [misc-const-correctness]
+  std::time_t result = std::time(nullptr);
+  // warning: function 'asctime' is not bounds-checking and non-reentrant; 'strftime' should be used instead [bugprone-unsafe-functions,cert-msc24-c,cert-msc33-c]
+  std::string result_str = std::asctime(std::localtime(&result));
+  // rules_lint author: note the memory leak is not detected
+  return strdup(result_str.c_str());
+}

example/src/cpp/lib/get/get-time.h

@@ -0,0 +1,17 @@
+#ifndef LIB_GET_TIME_H_
+#define LIB_GET_TIME_H_
+
+#include <string.h>
+
+const char* get_localtime_impl();
+
+// Deliberatly bad code
+inline char* get_localtime() {
+    const char* time = get_localtime_impl();
+    char timebuf[20];
+    strcpy(timebuf, time);
+    // warning: Address of stack memory associated with local variable 'timebuf' returned to caller [clang-analyzer-core.StackAddressEscape]
+    return timebuf;
+}
+
+#endif

example/src/cpp/lib/hello-time.cc

@@ -0,0 +1,12 @@
+#include "hello-time.h"
+// warning: included header ctime is not used directly [misc-include-cleaner]
+#include <ctime>
+#include <iostream>
+#include <get/get-time.h>
+
+// Deliberately bad code
+void print_localtime() {
+  char* localtime = get_localtime();
+  // warning: do not call c-style vararg functions [cppcoreguidelines-pro-type-vararg,hicpp-vararg]
+  printf("%s\n", localtime);
+}

example/src/cpp/lib/hello-time.h

@@ -0,0 +1,16 @@
+#ifndef LIB_HELLO_TIME_H_
+#define LIB_HELLO_TIME_H_
+
+#include <string>
+#include <stdio.h>
+#include <xhello-time.h>
+
+void print_localtime();
+
+inline void print_localtime2() {
+    std::string a = "time";
+    for (int i=0; i<a.size(); ++i) {
+        printf("%s", a.c_str()[i]);
+    }
+}
+#endif

example/src/cpp/lib/xhello-time.h

@@ -0,0 +1,13 @@
+#ifndef LIB_HELLO_TIME2_H_
+#define LIB_HELLO_TIME2_H_
+
+#include <string>
+#include <stdio.h>
+
+inline void print_localtime3() {
+    std::string a = "time";
+    for (int i=0; i<a.size(); ++i) {
+        printf("%s", a.c_str()[i]);
+    }
+}
+#endif

example/src/cpp/main/BUILD

@@ -0,0 +1,26 @@
+load("@rules_cc//cc:defs.bzl", "cc_binary", "cc_library")
+
+cc_library(
+    name = "hello-greet",
+    srcs = ["hello-greet.cc"],
+    hdrs = ["hello-greet.h"],
+    includes = ["."],
+)
+
+cc_binary(
+    name = "hello-world",
+    srcs = ["hello-world.cc"],
+    includes = ["."],
+    deps = [
+        ":hello-greet",
+        "//src/cpp/lib:hello-time",
+    ],
+)
+
+cc_library(
+    name = "hello-greet-with-error",
+    srcs = ["hello-greet-with-error.cc"],
+    hdrs = ["hello-greet-with-error.h"],
+    includes = ["."],
+    tags = ["manual"],
+)

example/src/cpp/main/hello-greet-with-error.cc

@@ -0,0 +1,5 @@
+#include "hello-greet-with-error.h"
+
+std::string get_greet(const std::string& who) {
+  return "Hello " + who;
+}