always generate fixes in ClangTidy aspect

README.md

@@ -32,6 +32,7 @@ rules_clang_tidy_dependencies()
 # //:.bazelrc
 build:clang-tidy --aspects=@rules_clang_tidy//:aspects.bzl%check
 build:clang-tidy --output_groups=report
+build:clang-tidy --remote_download_outputs=toplevel
 build:clang-tidy --keep_going
 ```
 
@@ -52,13 +53,14 @@ To specify a specific binary (e.g. `clang-tidy` is specified by a hermetic
 toolchain like [this](https://github.com/bazel-contrib/toolchains_llvm)), update
 the build setting in `.bazelrc`.
 
-```Starlark
+```starlark
 # //:.bazelrc
 
 build --@rules_clang_tidy//:clang-tidy=@llvm18//:clang-tidy
 
 build:clang-tidy --aspects=@rules_clang_tidy//:aspects.bzl%check
 build:clang-tidy --output_groups=report
+build:clang-tidy --remote_download_outputs=toplevel
 build:clang-tidy --keep_going
 ```
 
@@ -71,7 +73,7 @@ build:clang-tidy --keep_going
 To override the default `.clang-tidy`, define a `filegroup` containing the
 replacement config and update build setting in `.bazelrc`.
 
-```Starlark
+```starlark
 # //:BUILD.bazel
 
 filegroup(
@@ -81,13 +83,14 @@ filegroup(
 )
 ```
 
-```Starlark
+```starlark
 # //:.bazelrc
 
 build --@rules_clang_tidy//:config=//:clang-tidy-config
 
 build:clang-tidy --aspects=@rules_clang_tidy//:aspects.bzl%check
 build:clang-tidy --output_groups=report
+build:clang-tidy --remote_download_outputs=toplevel
 build:clang-tidy --keep_going
 ```
 
@@ -97,38 +100,27 @@ build:clang-tidy --keep_going
 
 <details><summary></summary>
 
-To apply fixes, generate the exported fixes with the `export_fixes` aspect.
-
-```Starlark
-# //:.bazelrc
-
-build:clang-tidy-export-fixes --aspects=@rules_clang_tidy//:aspects.bzl%export_fixes
-build:clang-tidy-export-fixes --output_groups=report
-build:clang-tidy-export-fixes --remote_download_outputs=toplevel
-```
+To apply fixes, run `@rules_clang_tidy//:apply-fixes`. This uses the exported
+fixes generated by the `check` aspect in the `output_path`.
 
 ```sh
-bazel build //... --config=clang-tidy-export-fixes
+bazel build //... --config=clang-tidy
+bazel run @rules_clang_tidy//:apply-fixes -- $(bazel info output_path)
 ```
 
 If only a subset of checks needs to be run, those can be specified with
 `extra-options`. This flag can be specified multiples times.
 
 ```sh
-bazel build //... --config=clang-tidy-export-fixes \
+bazel build //... --config=clang-tidy \
   --@rules_clang_tidy//:extra-options="--checks=-*,misc-unused-alias-decls"
-```
-
-Then apply the exported fixes with
-
-```sh
 bazel run @rules_clang_tidy//:apply-fixes -- $(bazel info output_path)
 ```
 
 Alternatively, use rule `apply_fixes` and specify the dependencies for the
 target.
 
-```Starlark
+```starlark
 load("@rules_clang_tidy//:defs.bzl", "apply_fixes")
 
 apply_fixes(
@@ -146,6 +138,7 @@ and run the `apply_fixes` target
 ```sh
 bazel run //:apply-fixes
 
+# or generate and apply fixes for only a single check
 bazel run //:apply-fixes \
   --@rules_clang_tidy//:extra-options="--checks=-*,misc-unused-alias-decls*"
 ```
@@ -156,6 +149,12 @@ set, `clang-apply-replacements` must be in `PATH`. Similarly to
 `--@rules_clang_tidy//:clang-tidy`, it's convenient to define the value in
 `.bazelrc`.
 
+```starlark
+# //:.bazelrc
+
+build --@rules_clang_tidy//:clang-apply-replacements=@llvm18//:clang-apply-replacements
+```
+
 </details>
 
 ## Requirements

apply_fixes.bzl

@@ -8,7 +8,7 @@ load("@bazel_skylib//lib:shell.bzl", "shell")
 load("@bazel_skylib//lib:versions.bzl", "versions")
 load("@local_bazel_version//:bazel_version.bzl", "BAZEL_VERSION")
 load("//private:functional.bzl", fn = "functional")
-load(":aspects.bzl", "export_fixes")
+load(":aspects.bzl", "check")
 
 def _do_verify_deps(ctx, out):
     label = str(ctx.label).strip("@").removesuffix(".verify")
@@ -204,7 +204,7 @@ _verify_deps = rule(
 
 def _apply_fixes_impl(ctx):
     apply_bin = ctx.attr._clang_apply_replacements.files_to_run.executable
-    depsets = [dep[OutputGroupInfo].report for dep in ctx.attr.deps]
+    depsets = [dep[OutputGroupInfo].fixes for dep in ctx.attr.deps]
     fixes = [f for dep in depsets for f in dep.to_list()]
 
     runfiles = ctx.runfiles(
@@ -252,7 +252,7 @@ _apply_fixes = rule(
     attrs = {
         "verify": attr.label(),
         "deps": attr.label_list(
-            aspects = [export_fixes],
+            aspects = [check],
             providers = [CcInfo],
             mandatory = True,
         ),

aspects.bzl

@@ -10,11 +10,3 @@ check = make_clang_tidy_aspect(
         "--warnings-as-errors='*'",
     ],
 )
-
-export_fixes = make_clang_tidy_aspect(
-    options = [
-        "--use-color",
-        "--warnings-as-errors='-*'",
-        "--export-fixes=$@",
-    ],
-)

example/dep-cc_test/.bazelrc

@@ -5,7 +5,3 @@ build --symlink_prefix=/
 
 build:clang-tidy --aspects=@rules_clang_tidy//:aspects.bzl%check
 build:clang-tidy --output_groups=report
-
-build:clang-tidy-export-fixes --aspects=@rules_clang_tidy//:aspects.bzl%export_fixes
-build:clang-tidy-export-fixes --output_groups=report
-

example/hermetic-toolchain/.bazelrc

@@ -6,8 +6,5 @@ build --symlink_prefix=/
 build:clang-tidy --aspects=@rules_clang_tidy//:aspects.bzl%check
 build:clang-tidy --output_groups=report
 
-build:clang-tidy-export-fixes --aspects=@rules_clang_tidy//:aspects.bzl%export_fixes
-build:clang-tidy-export-fixes --output_groups=report
-
 build --@rules_clang_tidy//:clang-tidy=@llvm_toolchain//:clang-tidy
 build --@rules_clang_tidy//:clang-apply-replacements=@llvm_toolchain//:clang-apply-replacements

example/misc-unused/.bazelrc

@@ -6,6 +6,3 @@ build --symlink_prefix=/
 build:clang-tidy --aspects=@rules_clang_tidy//:aspects.bzl%check
 build:clang-tidy --output_groups=report
 
-build:clang-tidy-export-fixes --aspects=@rules_clang_tidy//:aspects.bzl%export_fixes
-build:clang-tidy-export-fixes --output_groups=report
-

example/shared-source/.bazelrc

@@ -6,6 +6,3 @@ build --symlink_prefix=/
 build:clang-tidy --aspects=@rules_clang_tidy//:aspects.bzl%check
 build:clang-tidy --output_groups=report
 
-build:clang-tidy-export-fixes --aspects=@rules_clang_tidy//:aspects.bzl%export_fixes
-build:clang-tidy-export-fixes --output_groups=report
-

make_clang_tidy_aspect.bzl

@@ -61,18 +61,18 @@ def _do_tidy(ctx, compilation_ctx, source_file, **kwargs):
         s.elems(),
     )).replace(".bzl", "", 1)
 
+    outbase = ".".join([
+        source_file.short_path,
+        "_".join(fn.map(sanitize, ctx.aspect_ids)),
+    ])
+
     # This output file may be used by clang-apply-replacements which requires a
     # yaml extension.
-    out = ctx.actions.declare_file(
-        ".".join([
-            source_file.short_path,
-            "_".join(fn.map(sanitize, ctx.aspect_ids)),
-            "yaml",
-        ]),
-    )
-
-    sub_outfile = lambda s: s.replace("$@", out.path)
+    fixes = ctx.actions.declare_file(outbase + ".yaml")
+    result = ctx.actions.declare_file(outbase + ".result")
+    phony = ctx.actions.declare_file(outbase + ".phony")
 
+    # This action should succeed so that the fixes file is generated and cached.
     ctx.actions.run_shell(
         inputs = depset(
             direct = [
@@ -82,38 +82,36 @@ def _do_tidy(ctx, compilation_ctx, source_file, **kwargs):
             ],
             transitive = [compilation_ctx.headers],
         ),
-        outputs = [out],
+        outputs = [fixes, result],
         arguments = [str(not kwargs["display_stderr"]).lower()],
         command = """\
 #!/usr/bin/env bash
 set -euo pipefail
 
-{clang_tidy} \
+({clang_tidy} \
     --config-file={config} \
     {tidy_options} \
     {extra_options} \
+    --export-fixes="{fixes}" \
     {infile} \
-      -- {compiler_command} 2> log.stderr \
-  || (cat log.stderr >&2 && false)
+      -- {compiler_command} 2> log.stderr && echo "true" > {result})\
+  || (cat log.stderr >&2 && echo "false" > {result})
 
 $1 || cat log.stderr
 
-touch {outfile}
+touch {fixes}
 
 # replace sandbox path prefix from file paths and
 # hope `+` isn't used anywhere
-sed --in-place --expression "s+$(pwd)+%workspace%+g" {outfile}
+sed --in-place --expression "s+$(pwd)+%workspace%+g" {fixes}
         """.format(
             clang_tidy = clang_tidy.path,
             config = ctx.file._config.path,
-            tidy_options = sub_outfile(
-                " ".join(kwargs["tidy_options"]),
-            ),
-            extra_options = sub_outfile(
-                " ".join(ctx.attr._extra_options[BuildSettingInfo].value),
-            ),
+            tidy_options = " ".join(kwargs["tidy_options"]),
+            extra_options = " ".join(ctx.attr._extra_options[BuildSettingInfo].value),
             infile = source_file.path,
-            outfile = out.path,
+            fixes = fixes.path,
+            result = result.path,
             compiler_command = " ".join(
                 _toolchain_args(ctx) +
                 _compilation_ctx_args(compilation_ctx) +
@@ -126,7 +124,23 @@ sed --in-place --expression "s+$(pwd)+%workspace%+g" {outfile}
         execution_requirements = kwargs["execution_requirements"],
     )
 
-    return out
+    # use result to conditionally fail the action
+    ctx.actions.run_shell(
+        inputs = depset(direct = [result]),
+        outputs = [phony],
+        command = """\
+#!/usr/bin/env bash
+set -euo pipefail
+
+bash {result}
+touch {phony}
+        """.format(
+            result = result.path,
+            phony = phony.path,
+        ),
+    )
+
+    return struct(fixes = fixes, phony = phony)
 
 def _check_attr(ctx):
     config_files = ctx.attr._config.files.to_list()
@@ -148,20 +162,27 @@ def _clang_tidy_aspect_impl(**kwargs):
 
         _check_attr(ctx)
 
-        outputs = [
-            _do_tidy(
+        fixes = []
+        phony = []
+        for source_file in (
+            _source_files_in(ctx, "hdrs") +
+            _source_files_in(ctx, "srcs")
+        ):
+            out = _do_tidy(
                 ctx,
                 target[CcInfo].compilation_context,
                 source_file,
                 **kwargs
             )
-            for source_file in (
-                _source_files_in(ctx, "hdrs") +
-                _source_files_in(ctx, "srcs")
-            )
-        ]
+            fixes.append(out.fixes)
+            phony.append(out.phony)
 
-        return [OutputGroupInfo(report = depset(outputs))]
+        return [
+            OutputGroupInfo(
+                report = depset(fixes + phony),
+                fixes = depset(fixes),
+            ),
+        ]
 
     return impl
 

test/BUILD.bazel

@@ -71,3 +71,8 @@ tidy_test(
     ],
     example_external = "hermetic-toolchain",
 )
+
+tidy_test(
+    name = "fix_uses-check-action-cache_test",
+    example_external = "misc-unused",
+)

test/fix_hermetic-toolchain_test.bash

@@ -14,7 +14,7 @@ bazel \
   run \
   //:apply-fixes | tee "$log"
 
-grep "warning: .*misc-unused-alias-decls" "$log"
+grep "error: .*misc-unused-alias-decls" "$log"
 
 diff \
   --color=always \

test/fix_rule_test.bash

@@ -26,7 +26,7 @@ bazel \
   run \
   //:apply-fixes | tee "$log"
 
-grep "warning: .*misc-unused-alias-decls" "$log"
+grep "error: .*misc-unused-alias-decls" "$log"
 
 diff \
   --color=always \

test/fix_script_test.bash

@@ -13,8 +13,8 @@ unset TEST_TMPDIR
 bazel \
   --bazelrc="$test_bazelrc" \
   build \
-  --config=clang-tidy-export-fixes \
-  //...
+  --config=clang-tidy \
+  //... || true
 
 # apply fixes
 bazel \

test/fix_uses-check-action-cache_test.bash

@@ -0,0 +1,23 @@
+#!/usr/bin/env bash
+set -euxo pipefail
+
+source test/prelude.bash
+
+orig="$(mktemp)"
+
+fix_setup example/misc-unused
+
+# This sets a 15 second idle timeout
+# https://github.com/bazelbuild/bazel/issues/11062
+unset TEST_TMPDIR
+
+cat "$test_bazelrc" >> .bazelrc
+
+# run check, generating fixes in cache
+bazel build --config=clang-tidy //... | tee "$log" || true
+
+grep "error: .*misc-unused-alias-decls" "$log"
+
+bazel run //:apply-fixes 2>&1 | tee "$log"
+
+[ 0 -eq $(grep -c "error: .*misc-unused-alias-decls" "$log") ]