fix(clippy): Store exit code to respect errors

.bazelrc

@@ -4,6 +4,7 @@ common --config=ruleset
 
 # Don't build protoc from the cc_binary, it's slow and spammy when cache miss
 common --incompatible_enable_proto_toolchain_resolution
+common --@protobuf//bazel/toolchains:prefer_prebuilt_protoc
 common --per_file_copt=external/.*protobuf.*@--PROTOBUF_WAS_NOT_SUPPOSED_TO_BE_BUILT
 common --host_per_file_copt=external/.*protobuf.*@--PROTOBUF_WAS_NOT_SUPPOSED_TO_BE_BUILT
 

.bcr/patches/go_dev_deps.patch

@@ -1,11 +1,5 @@
 --- a/MODULE.bazel
 +++ b/MODULE.bazel
-@@ -34,7 +34,7 @@ use_repo(multitool, "multitool")
-
- # To allow /tools to be built from source
- # NOTE: when publishing to BCR, we patch this to be True, as we publish pre-built binaries with our releases.
+@@ -9,1 +9,1 @@
 -IS_RELEASE = False
 +IS_RELEASE = True
-
- bazel_dep(
-     name = "toolchains_protoc",

MODULE.bazel

@@ -6,6 +6,9 @@ module(
     compatibility_level = 1,
 )
 
+# NOTE: when publishing to BCR, we patch this to be True, as we publish pre-built binaries with our releases.
+IS_RELEASE = False
+
 # We don't actually depend directly on aspect_bazel_lib, but we do require this
 # version be used transitively so that it doesn't produce conflicting actions
 # with the bazel_lib module we use.
@@ -58,22 +61,11 @@ use_repo(rules_lint_toolchains, "sarif_parser_toolchains")
 register_toolchains("@sarif_parser_toolchains//:all")
 
 ####### Dev dependencies ########
-
-bazel_dep(name = "bazelrc-preset.bzl", version = "1.1.0", dev_dependency = True)
-
 # To allow /tools to be built from source
-# NOTE: when publishing to BCR, we patch this to be True, as we publish pre-built binaries with our releases.
-IS_RELEASE = False
-
-bazel_dep(
-    name = "toolchains_protoc",
-    version = "0.6.0",
-    dev_dependency = IS_RELEASE,
-)
 
-protoc = use_extension("@toolchains_protoc//protoc:extensions.bzl", "protoc", dev_dependency = IS_RELEASE)
-protoc.toolchain(version = "v32.0")
+bazel_dep(name = "bazelrc-preset.bzl", version = "1.1.0", dev_dependency = True)
 
+bazel_dep(name = "protobuf", version = "33.4", dev_dependency = IS_RELEASE)
 bazel_dep(
     name = "gazelle",
     version = "0.43.0",

examples/rust/src/BUILD

@@ -76,3 +76,14 @@ rust_binary(
     ],
     deps = [":bad_lib_with_noclippy"],
 )
+
+rust_binary(
+    name = "binary_with_warning_and_error",
+    srcs = ["warning_and_error.rs"],
+    edition = "2021",
+    visibility = [
+        "//src/rust:__pkg__",
+        "//test:__pkg__",
+        "//test/machine_outputs:__pkg__",
+    ],
+)

examples/rust/src/bad_binary.rs

@@ -1,5 +1,8 @@
 fn main() {
-    // Will fail clippy because we could just write println!("Hello World").
-    println!("{}", "Hello World");
+    // Will fail clippy because of the missing comma
+    let _missing_comma = &[
+        -1, -2, -3 // <= no comma here
+        -4, -5, -6
+    ];
 }
 

examples/rust/src/bad_lib.rs

@@ -1,5 +1,9 @@
 fn bad() {
-    // Will fail clippy because we could just write println!("Hello World").
-    println!("{}", "Hello World");
+    // Will fail clippy because of the missing comma
+    let _missing_comma = &[
+        -1, -2, -3 // <= no comma here
+        -4, -5, -6
+    ];
+
 }
 

examples/rust/src/warning_and_error.rs

@@ -0,0 +1,11 @@
+fn main() {
+    // Will emit a clippy warning because we could just write println!("Hello World").
+    println!("{}", "Hello World");
+
+    // Will fail clippy because of the missing comma
+    let _missing_comma = &[
+        -1, -2, -3 // <= no comma here
+        -4, -5, -6
+    ];
+
+}

examples/rust/test/BUILD

@@ -43,6 +43,14 @@ clippy_test(
     tags = ["manual"],
 )
 
+clippy_test(
+    name = "clippy_binary_with_warning_and_error",
+    srcs = ["//src:binary_with_warning_and_error"],
+    # Expected to fail based on current content of the file.
+    # Normally you'd fix the file instead of tagging this test.
+    tags = ["manual"],
+)
+
 machine_clippy_report(
     name = "machine_clippy_report",
     src = "//src:bad_lib",

lint/clippy.bzl

@@ -40,11 +40,6 @@ clippy = lint_clippy_aspect(
 Now your targets will be linted with clippy.
 If you wish a target to be excluded from linting, you can give them the `noclippy` tag.
 
-Please note that, for now all clippy warnings are considered failures.
-This is because rules_rust will fail the entire execution if there's even one error,
-so we need to limit the reports to just warnings so that we can continue the target execution and generate useful output files.
-Because we limit all errors to warnings, we must consider every warning as an error.
-
 Please watch issue https://github.com/aspect-build/rules_lint/issues/385 for updates on this behavior.
 """
 
@@ -53,34 +48,26 @@ load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "OPTIONAL_SARIF_PARSER
 
 _MNEMONIC = "AspectRulesLintClippy"
 
-def _marker_to_exit_code(ctx, marker, output, exit_code):
-    """Write 0 to exit_code if marker exists and the output is empty, fail otherwise.
-
-    rules_rust won't write the exit code to the success_marker, so we assert that it exists and write the exit code ourselves.
-    If there is a success marker but the output is not empty, we mark it as a failure.
-    If there is no success marker, the action has failed anyway.
-
-    Please note that all clippy warnings are considered failures.
-
-    Args:
-        ctx (ctx): The rule or aspect context. Must have access to `ctx.actions.run_shell`
-        marker (File): A file that will only exist if the action has succeeded
-        exit_code (File): A file that will be written with the exit code 0 if marker exists
-    """
-    if not exit_code:
-        # fail_on_violation is enabled, we don't have an exit_code file.
-        return
+def _parse_wrapper_output_into_files(ctx, output_file, exit_code_file, raw_process_wrapper_wrapper_output):
     ctx.actions.run_shell(
-        outputs = [exit_code],
-        inputs = [marker, output],
-        arguments = [exit_code.path, output.path],
         command = """
-            if [ -s $2 ]; then
-                echo '1' > $1
-            else
-                echo '0' > $1
-            fi
-        """,
+exit_code=$(head -n 1 $1)
+output=$(tail -n +2 $1)
+echo "${output}" > $2
+echo "${exit_code}" > $3
+""",
+        arguments = [
+            raw_process_wrapper_wrapper_output.path,
+            output_file.path,
+            exit_code_file.path,
+        ],
+        inputs = [
+            raw_process_wrapper_wrapper_output,
+        ],
+        outputs = [
+            output_file,
+            exit_code_file,
+        ],
     )
 
 # buildifier: disable=function-docstring
@@ -105,40 +92,48 @@ def _clippy_aspect_impl(target, ctx):
         noop_lint_action(ctx, outputs)
         return [info]
 
-    extra_options = []
+    extra_options = [
+        # If we don't pass any clippy options, rules_rust will (rightly) default to -Dwarnings, which turns all warnings into errors.
+        # They do this to force Bazel to re-run targets on failures.
+        # However, we don't need to do that because we keep track of output files and exit codes separately.
+        "-Wwarnings",
+    ]
     # FIXME: Implement support for --fix mode. Clippy has a --fix flag, but our patcher doesn't currently support running an action through a macro.
     #        We have to either
     #           (1) modify the patcher so that it can run an action through a macro, or
     #           (2) modify rules_rust so that it gives us a struct with a command line we can run it with the patcher.
 
-    human_success_indicator = ctx.actions.declare_file(OUTFILE_FORMAT.format(label = target.label.name, mnemonic = _MNEMONIC, suffix = "human_success_indicator"))
+    raw_outputs = struct(
+        human = ctx.actions.declare_file(OUTFILE_FORMAT.format(label = target.label.name, mnemonic = _MNEMONIC, suffix = "raw_process_wrapper_wrapper_output_human")),
+        machine = ctx.actions.declare_file(OUTFILE_FORMAT.format(label = target.label.name, mnemonic = _MNEMONIC, suffix = "raw_process_wrapper_wrapper_output_machine")),
+    )
+
     rust_clippy_action.action(
         ctx,
         clippy_executable = clippy_bin,
-        process_wrapper = ctx.executable._process_wrapper,
+        process_wrapper = ctx.executable._process_wrapper_wrapper,
         crate_info = crate_info,
         config = ctx.file._config_file,
-        output = outputs.human.out,
-        success_marker = human_success_indicator,
-        cap_at_warnings = True,  # We don't want to crash the process if there are clippy errors, we just want to report them.
+        output = raw_outputs.human,
+        cap_at_warnings = False,
         extra_clippy_flags = extra_options,
     )
-    _marker_to_exit_code(ctx, human_success_indicator, outputs.human.out, outputs.human.exit_code)
 
-    machine_success_indicator = ctx.actions.declare_file(OUTFILE_FORMAT.format(label = target.label.name, mnemonic = _MNEMONIC, suffix = "machine_success_indicator"))
+    _parse_wrapper_output_into_files(ctx, outputs.human.out, outputs.human.exit_code, raw_outputs.human)
+
     rust_clippy_action.action(
         ctx,
         clippy_executable = clippy_bin,
-        process_wrapper = ctx.executable._process_wrapper,
+        process_wrapper = ctx.executable._process_wrapper_wrapper,
         crate_info = crate_info,
         config = ctx.file._config_file,
-        output = outputs.machine.out,
-        success_marker = machine_success_indicator,
-        cap_at_warnings = True,
+        output = raw_outputs.machine,
+        cap_at_warnings = False,
         extra_clippy_flags = extra_options,
         error_format = "json",
     )
-    _marker_to_exit_code(ctx, machine_success_indicator, outputs.machine.out, outputs.machine.exit_code)
+
+    _parse_wrapper_output_into_files(ctx, outputs.machine.out, outputs.machine.exit_code, raw_outputs.machine)
 
     # FIXME: Rustc only gives us JSON output, which we can't turn into SARIF yet.
     # clippy uses rustc's IO format, which doesn't have a SARIF output mode built in,
@@ -175,9 +170,9 @@ def lint_clippy_aspect(config, rule_kinds = DEFAULT_RULE_KINDS):
         "_rule_kinds": attr.string_list(
             default = rule_kinds,
         ),
-        "_process_wrapper": attr.label(
-            doc = "A process wrapper for running clippy on all platforms",
-            default = Label("@rules_rust//util/process_wrapper"),
+        "_process_wrapper_wrapper": attr.label(
+            doc = "A wrapper around the rules_rust process wrapper. See @aspect_rules_lint//lint/rust:process_wrapper_wrapper.sh for motivation and documetnation.",
+            default = Label("//lint/rust:process_wrapper_wrapper"),
             executable = True,
             cfg = "exec",
         ),

lint/rust/BUILD.bazel

@@ -1,4 +1,5 @@
 load("@aspect_rules_js//js:defs.bzl", "js_binary", "js_library", "js_run_binary", "js_test")
+load("@rules_shell//shell:sh_binary.bzl", "sh_binary")
 
 js_library(
     name = "diagnostic-formatter",
@@ -38,3 +39,14 @@ js_test(
         "GOT": "$(rootpath :got.patch)",
     },
 )
+
+sh_binary(
+    name = "process_wrapper_wrapper",
+    srcs = ["process_wrapper_wrapper.bash"],
+    data = [
+        "@rules_rust//util/process_wrapper",
+    ],
+    # Aspect visibility rules are not on by default.
+    visibility = ["//visibility:public"],
+    deps = ["@rules_shell//shell/runfiles"],
+)

lint/rust/process_wrapper_wrapper.bash

@@ -0,0 +1,64 @@
+#!/bin/bash
+set -euo pipefail
+
+# Run the rustc process wrapper, but:
+# - Instead of forwarding the exit code of the child process, capture it.
+# - Instead of passing --stderr-file to the process wrapper, capture the output and write to that file after everything has finished, with the following format:
+#   ```
+#   <exit code of process wrapper>
+#   <stderr of process wrapper>
+#   ```
+
+# --- begin runfiles.bash initialization v3 ---
+# Copy-pasted from the Bazel Bash runfiles library v3.
+set -uo pipefail; set +e; f=bazel_tools/tools/bash/runfiles/runfiles.bash
+# shellcheck disable=SC1090
+source "${RUNFILES_DIR:-/dev/null}/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "${RUNFILES_MANIFEST_FILE:-/dev/null}" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$0.runfiles/$f" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  source "$(grep -sm1 "^$f " "$0.exe.runfiles_manifest" | cut -f2- -d' ')" 2>/dev/null || \
+  { echo>&2 "ERROR: cannot find $f"; exit 1; }; f=; set -e
+# --- end runfiles.bash initialization v3 ---
+
+process_wrapper_path=$(rlocation "rules_rust/util/process_wrapper/process_wrapper")
+
+stderr_file=""
+cmd=("${process_wrapper_path}")
+
+# Parse arguments
+while [[ $# -gt 0 ]]; do
+  case "$1" in
+    --stderr-file)
+      if [[ $# -lt 2 ]]; then
+        echo "Error: --stderr-file requires a value" >&2
+        exit 1
+      fi
+      stderr_file="$2"
+      shift 2
+      ;;
+    *)
+      cmd+=("$1")
+      shift
+      ;;
+  esac
+done
+
+# Ensure that _some_ command is left
+if [[ ${#cmd[@]} -eq 0 ]]; then
+  echo "Error: no command to execute" >&2
+  exit 1
+fi
+
+set +e # We allow failing for this command as we want to capture the exit code
+
+out=$(${cmd[@]} 2>&1)
+exit_code="$?"
+
+set -e
+
+cat <<EOF > "${stderr_file}"
+${exit_code}
+${out}
+EOF
+