feat: produce new human-readable output group (#331)

* feat: produce new rules_lint_output group for human-readable

Configure linters to be colored and produce prettier output for this new output.
Change lint.sh to present this output rather than the 'report' output which is machine-readable

Fixes #324

* fix: lint_test against .txt output

* chore: renames

* chore: remove color support for now

It makes our tests more complex

* chore: more rename

* chore: missing file

* chore: minify delta

* chore: fix test

* chore: fix tests

* chore: no need for conditional, report output always declared

* refactor: named utility for cases where we must have a meaningless output for exit code

* refactor: code review feedback

* refactor: noop lint action understands output struct

Allows simplification of short-circuit logic for linting zero inputs

* chore: fix noop action

* chore: fix broken test

* refactor: rename stdout to out since buf uses stderr

example/lint.sh

@@ -46,7 +46,7 @@ args+=(
 	# See https://github.com/aspect-build/rules_ts/pull/574#issuecomment-2073632879
 	"--norun_validations"
 	"--build_event_json_file=$buildevents"
-	"--output_groups=rules_lint_report"
+	"--output_groups=rules_lint_human"
 	"--remote_download_regex='.*AspectRulesLint.*'"
 )
 
@@ -58,6 +58,11 @@ if [ $1 == "--fail-on-violation" ]; then
 	)
 	shift
 fi
+
+# Allow a `--fix` option on the command-line.
+# This happens to make output of the linter such as ruff's
+# [*] 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 [ $1 == "--fix" ]; then
 	fix="patch"
 	args+=(
@@ -78,14 +83,14 @@ bazel build ${args[@]} $@
 # TODO: Maybe this could be hermetic with bazel run @aspect_bazel_lib//tools:jq or sth
 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')
+    valid_reports=$(jq --arg ext .out --raw-output "$filter" "$buildevents" | tr -d '\r')
 else
-    valid_reports=$(jq --arg ext report --raw-output "$filter" "$buildevents")
+    valid_reports=$(jq --arg ext .out --raw-output "$filter" "$buildevents")
 fi
 
 # Show the results.
 while IFS= read -r report; do
-	# Exclude coverage reports, and check if the report is empty.
+	# Exclude coverage reports, and check if the output is empty.
 	if [[ "$report" == *coverage.dat ]] || [[ ! -s "$report" ]]; then
 		# Report is empty. No linting errors.
 		continue
@@ -95,15 +100,12 @@ while IFS= read -r report; do
 	echo
 done <<<"$valid_reports"
 
-# This happens to make output of the linter such as ruff's
-# [*] 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=$valid_reports
 	while IFS= read -r patch; do
-		# Exclude coverage reports, and check if the report is empty.
+		# Exclude coverage, and check if the patch is empty.
 		if [[ "$patch" == *coverage.dat ]] || [[ ! -s "$patch" ]]; then
-			# Report is empty. No linting errors.
+			# Patch is empty. No linting errors.
 			continue
 		fi
 

example/test/lint_test.bats

@@ -26,13 +26,19 @@ EOF
 	assert_output --partial "src/hello.kt:1:1: File name 'hello.kt' should conform PascalCase (standard:filename)"
 
 	# ESLint
-	assert_output --partial 'src/file.ts: line 2, col 7, Error - Type string trivially inferred from a string literal, remove type annotation. (@typescript-eslint/no-inferrable-types)'
+	echo <<"EOF" | assert_output --partial
+src/file.ts
+  2:7  error  Type string trivially inferred from a string literal, remove type annotation  @typescript-eslint/no-inferrable-types
+EOF
 
 	# Buf
 	assert_output --partial 'src/file.proto:1:1:Import "src/unused.proto" is unused.'
 
 	# Vale
-	assert_output --partial "src/README.md:3:47:Google.We:Try to avoid using first-person plural like 'We'."
+	echo <<"EOF" | assert_output --partial
+3:47  warning  Try to avoid using              Google.We
+               first-person plural like 'We'.
+EOF
 }
 
 @test "should produce reports" {

lint/README.md

@@ -50,7 +50,7 @@ Add these three things:
 2. A `_my_linter_aspect_impl` function, following the https://bazel.build/extending/aspects#aspect_implementation API.
    This is responsible for selecting which rule types in the graph it "knows how to lint".
    It should call the `my_linter_action` function.
-   It must always return a `rules_lint_report` output group, which is easiest by using the
+   It must always return the correct output groups, which is easiest by using the
    `report_files` helper in `//lint/private:lint_aspect.bzl`.
    The simple lint.sh also relies on the report output filenames containing `AspectRulesLint`, which comes from
    the convention that `AspectRulesLint` is the prefix for all rules_lint linter action mnemonics.

lint/buf.bzl

@@ -12,7 +12,7 @@ buf = buf_lint_aspect(
 """
 
 load("@rules_proto//proto:defs.bzl", "ProtoInfo")
-load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "report_files", "should_visit")
+load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "output_files", "should_visit")
 
 _MNEMONIC = "AspectRulesLintBuf"
 
@@ -87,15 +87,13 @@ def _buf_lint_aspect_impl(target, ctx):
     if not should_visit(ctx.rule, ctx.attr._rule_kinds):
         return []
 
-    report, exit_code, info = report_files(_MNEMONIC, target, ctx)
-    buf_lint_action(
-        ctx,
-        ctx.toolchains[ctx.attr._buf_toolchain].cli,
-        ctx.toolchains["@rules_proto//proto:toolchain_type"].proto.proto_compiler.executable,
-        target,
-        report,
-        exit_code,
-    )
+    buf = ctx.toolchains[ctx.attr._buf_toolchain].cli
+    protoc = ctx.toolchains["@rules_proto//proto:toolchain_type"].proto.proto_compiler.executable
+    outputs, info = output_files(_MNEMONIC, target, ctx)
+
+    # TODO(alex): there should be a reason to run the buf action again rather than just copy the files
+    buf_lint_action(ctx, buf, protoc, target, outputs.human.out, outputs.human.exit_code)
+    buf_lint_action(ctx, buf, protoc, target, outputs.machine.out, outputs.machine.exit_code)
     return [info]
 
 def lint_buf_aspect(config, toolchain = "@rules_buf//tools/protoc-gen-buf-lint:toolchain_type", rule_kinds = ["proto_library"]):

lint/clang_tidy.bzl

@@ -39,7 +39,7 @@ clang_tidy = lint_clang_tidy_aspect(
 
 load("@bazel_tools//tools/build_defs/cc:action_names.bzl", "ACTION_NAMES")
 load("@bazel_tools//tools/cpp:toolchain_utils.bzl", "find_cpp_toolchain")
-load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "dummy_successful_lint_action", "patch_and_report_files", "report_files")
+load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "noop_lint_action", "output_files", "patch_and_output_files")
 
 _MNEMONIC = "AspectRulesLintClangTidy"
 
@@ -238,6 +238,7 @@ def clang_tidy_action(ctx, compilation_context, executable, srcs, stdout, exit_c
     outputs = [stdout]
     env = {}
     env["CLANG_TIDY__STDOUT_STDERR_OUTPUT_FILE"] = stdout.path
+
     if exit_code:
         env["CLANG_TIDY__EXIT_CODE_OUTPUT_FILE"] = exit_code.path
         outputs.append(exit_code)
@@ -308,19 +309,22 @@ def _clang_tidy_aspect_impl(target, ctx):
 
     files_to_lint = _filter_srcs(ctx.rule)
     compilation_context = target[CcInfo].compilation_context
-
     if ctx.attr._options[LintOptionsInfo].fix:
-        patch, report, exit_code, info = patch_and_report_files(_MNEMONIC, target, ctx)
-        if len(files_to_lint) == 0:
-            dummy_successful_lint_action(ctx, report, exit_code, patch)
-        else:
-            clang_tidy_fix(ctx, compilation_context, ctx.executable, files_to_lint, patch, report, exit_code)
+        outputs, info = patch_and_output_files(_MNEMONIC, target, ctx)
+    else:
+        outputs, info = output_files(_MNEMONIC, target, ctx)
+
+    if len(files_to_lint) == 0:
+        noop_lint_action(ctx, outputs)
+        return [info]
+
+    if hasattr(outputs, "patch"):
+        clang_tidy_fix(ctx, compilation_context, ctx.executable, files_to_lint, outputs.patch, outputs.human.out, outputs.human.exit_code)
     else:
-        report, exit_code, info = report_files(_MNEMONIC, target, ctx)
-        if len(files_to_lint) == 0:
-            dummy_successful_lint_action(ctx, report, exit_code)
-        else:
-            clang_tidy_action(ctx, compilation_context, ctx.executable, files_to_lint, report, exit_code)
+        clang_tidy_action(ctx, compilation_context, ctx.executable, files_to_lint, outputs.human.out, outputs.human.exit_code)
+
+    # TODO(alex): if we run with --fix, this will report the issues that were fixed. Does a machine reader want to know about them?
+    clang_tidy_action(ctx, compilation_context, ctx.executable, files_to_lint, outputs.machine.out, outputs.machine.exit_code)
     return [info]
 
 def lint_clang_tidy_aspect(binary, configs = [], global_config = [], header_filter = "", lint_target_headers = False, angle_includes_are_system = True, verbose = False):

lint/eslint.bzl

@@ -55,26 +55,26 @@ See the [react example](https://github.com/bazelbuild/examples/blob/b498bb106b20
 
 load("@aspect_bazel_lib//lib:copy_to_bin.bzl", "COPY_FILE_TO_BIN_TOOLCHAINS", "copy_files_to_bin_actions")
 load("@aspect_rules_js//js:libs.bzl", "js_lib_helpers")
-load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "dummy_successful_lint_action", "filter_srcs", "patch_and_report_files", "report_files", "should_visit")
+load("//lint/private:lint_aspect.bzl", "LintOptionsInfo", "filter_srcs", "output_files", "patch_and_output_files", "should_visit")
 
 _MNEMONIC = "AspectRulesLintESLint"
 
-def _gather_inputs(ctx, srcs):
+def _gather_inputs(ctx, srcs, files):
     inputs = copy_files_to_bin_actions(ctx, srcs)
 
     # Add the config file along with any deps it has on npm packages
     if "gather_files_from_js_providers" in dir(js_lib_helpers):
         # rules_js 1.x
         js_inputs = js_lib_helpers.gather_files_from_js_providers(
-            ctx.attr._config_files + [ctx.attr._workaround_17660, ctx.attr._formatter],
+            ctx.attr._config_files + files,
             include_transitive_sources = True,
             include_declarations = True,
             include_npm_linked_packages = True,
         )
     else:
         # rules_js 2.x
         js_inputs = js_lib_helpers.gather_files_from_js_infos(
-            ctx.attr._config_files + [ctx.attr._workaround_17660, ctx.attr._formatter],
+            ctx.attr._config_files + files,
             include_sources = True,
             include_transitive_sources = True,
             include_types = True,
@@ -84,7 +84,7 @@ def _gather_inputs(ctx, srcs):
     inputs.extend(js_inputs.to_list())
     return inputs
 
-def eslint_action(ctx, executable, srcs, report, exit_code = None):
+def eslint_action(ctx, executable, srcs, stdout, exit_code = None, format = "stylish"):
     """Create a Bazel Action that spawns an eslint process.
 
     Adapter for wrapping Bazel around
@@ -94,51 +94,59 @@ def eslint_action(ctx, executable, srcs, report, exit_code = None):
         ctx: an action context OR aspect context
         executable: struct with an eslint field
         srcs: list of file objects to lint
-        report: output file containing the stdout or --output-file of eslint
+        stdout: output file containing the stdout or --output-file of eslint
         exit_code: output file containing the exit code of eslint.
             If None, then fail the build when eslint exits non-zero.
+        format: value for eslint `--format` CLI flag
     """
 
     args = ctx.actions.args()
+    args.add("--no-warn-ignored")
+    file_inputs = [ctx.attr._workaround_17660]
 
     # TODO: enable if debug config, similar to rules_ts
     # args.add("--debug")
-
-    args.add_all(["--format", "../../../" + ctx.file._formatter.path])
+    if type(format) == "string":
+        args.add_all(["--format", format])
+    else:
+        args.add_all(["--format", "../../../" + format.files.to_list()[0].path])
+        file_inputs.append(format)
     args.add_all([s.short_path for s in srcs])
 
-    env = {"BAZEL_BINDIR": ctx.bin_dir.path}
-
     if not exit_code:
         ctx.actions.run_shell(
-            inputs = _gather_inputs(ctx, srcs),
-            outputs = [report],
+            inputs = _gather_inputs(ctx, srcs, file_inputs),
+            outputs = [stdout],
             tools = [executable._eslint],
             arguments = [args],
-            command = executable._eslint.path + " $@ && touch " + report.path,
-            env = env,
+            command = executable._eslint.path + " $@ && touch " + stdout.path,
+            env = {
+                "BAZEL_BINDIR": ctx.bin_dir.path,
+            },
             mnemonic = _MNEMONIC,
             progress_message = "Linting %{label} with ESLint",
         )
     else:
-        # Workaround: create an empty report file in case eslint doesn't write one
+        # Workaround: create an empty file in case eslint doesn't write one
         # Use `../../..` to return to the execroot?
         args.add_joined(["--node_options", "--require", "../../../" + ctx.file._workaround_17660.path], join_with = "=")
 
-        args.add_all(["--output-file", report.short_path])
-        env["JS_BINARY__EXIT_CODE_OUTPUT_FILE"] = exit_code.path
+        args.add_all(["--output-file", stdout.short_path])
 
         ctx.actions.run(
-            inputs = _gather_inputs(ctx, srcs),
-            outputs = [report, exit_code],
+            inputs = _gather_inputs(ctx, srcs, file_inputs),
+            outputs = [stdout, exit_code],
             executable = executable._eslint,
             arguments = [args],
-            env = env,
+            env = {
+                "BAZEL_BINDIR": ctx.bin_dir.path,
+                "JS_BINARY__EXIT_CODE_OUTPUT_FILE": exit_code.path,
+            },
             mnemonic = _MNEMONIC,
             progress_message = "Linting %{label} with ESLint",
         )
 
-def eslint_fix(ctx, executable, srcs, patch, stdout, exit_code):
+def eslint_fix(ctx, executable, srcs, patch, stdout, exit_code, format = "stylish"):
     """Create a Bazel Action that spawns eslint with --fix.
 
     Args:
@@ -148,22 +156,23 @@ def eslint_fix(ctx, executable, srcs, patch, stdout, exit_code):
         patch: output file containing the applied fixes that can be applied with the patch(1) command.
         stdout: output file containing the stdout or --output-file of eslint
         exit_code: output file containing the exit code of eslint
+        format: value for eslint `--format` CLI flag
     """
     patch_cfg = ctx.actions.declare_file("_{}.patch_cfg".format(ctx.label.name))
 
     ctx.actions.write(
         output = patch_cfg,
         content = json.encode({
             "linter": executable._eslint.path,
-            "args": ["--fix", "--format", "../../../" + ctx.file._formatter.path] + [s.short_path for s in srcs],
+            "args": ["--fix", "--format", format] + [s.short_path for s in srcs],
             "env": {"BAZEL_BINDIR": ctx.bin_dir.path},
             "files_to_diff": [s.path for s in srcs],
             "output": patch.path,
         }),
     )
 
     ctx.actions.run(
-        inputs = _gather_inputs(ctx, srcs) + [patch_cfg],
+        inputs = _gather_inputs(ctx, srcs, [ctx.attr._workaround_17660]) + [patch_cfg],
         outputs = [patch, stdout, exit_code],
         executable = executable._patcher,
         arguments = [patch_cfg.path],
@@ -184,19 +193,19 @@ def _eslint_aspect_impl(target, ctx):
         return []
 
     files_to_lint = filter_srcs(ctx.rule)
-
     if ctx.attr._options[LintOptionsInfo].fix:
-        patch, report, exit_code, info = patch_and_report_files(_MNEMONIC, target, ctx)
-        if len(files_to_lint) == 0:
-            dummy_successful_lint_action(ctx, report, exit_code, patch)
-        else:
-            eslint_fix(ctx, ctx.executable, files_to_lint, patch, report, exit_code)
+        outputs, info = patch_and_output_files(_MNEMONIC, target, ctx)
+    else:
+        outputs, info = output_files(_MNEMONIC, target, ctx)
+
+    # eslint can produce a patch file at the same time it reports the unpatched violations
+    if hasattr(outputs, "patch"):
+        eslint_fix(ctx, ctx.executable, files_to_lint, outputs.patch, outputs.human.out, outputs.human.exit_code)
     else:
-        report, exit_code, info = report_files(_MNEMONIC, target, ctx)
-        if len(files_to_lint) == 0:
-            dummy_successful_lint_action(ctx, report, exit_code)
-        else:
-            eslint_action(ctx, ctx.executable, files_to_lint, report, exit_code)
+        eslint_action(ctx, ctx.executable, files_to_lint, outputs.human.out, outputs.human.exit_code)
+
+    # TODO(alex): if we run with --fix, this will report the issues that were fixed. Does a machine reader want to know about them?
+    eslint_action(ctx, ctx.executable, files_to_lint, outputs.machine.out, outputs.machine.exit_code, format = ctx.attr._formatter)
 
     return [info]