Comparing bazel linting-systems

Context: Aspect is working on first-class Bazel support for formatters and linters across all languages our clients use.

Design invariants:

• General solution that handles all languages.
• Bazel is responsible for hermetic toolchain.
• Minimal fetches. Don't make users wait to pip install the world before they can format a .js file.
• No extra toil. Don't force users to declare lint/format stuff in their BUILD files. Instead, walk existing *_library rules.
  • Or, encourage wrapper macros so that existing load statements for *_library rules change, but the rest of linting setup is tucked into the macro.
• Let gazelle be in charge of BUILD files.
• Config is always shared with editor plugins.
• Easy to assert "the repo is format/lint clean" for teams who want it this way
• Possible to assert "changed files/ranges are format-clean"
• (stretch) possible to assert "changed files/ranges are lint-clean"
  • this is hard because a lint warning can be introduced in an unchanged file
• Workflows can be expressed with bazel commands, optional syntax sugar in aspect CLI.
  • For example, it might be aspect lint if you use our CLI, otherwise bazel build //... --aspects=//tools/linting:aspect.bzl%lint \ --output_groups=report && bazel run @aspect_rules_format//:accept

Top-line design decisions:

• treat formatters and linters together, or separately? They have different DX
• How to detect source files in the repository which aren't included in the *_library rules we walk.
• Users can build this into their CI system - should we also have a free "aspect CI" that just does lint/format checks?
• Other things we learn from dogfooding whatever we make.

Formatter, linter, or test?

The distinctions are useful and foundational to how you build workflows around the tools.

formatter	linter
Only one per language, since they could conflict with each other.	Many per language is fine; results compose.
Invariant: program's behavior is never changed.	Suggested fixes may change behavior.
Developer has no choices. Always blindly accept result.	Fix may be manual, or select from multiple auto-fixes.
Changes must be applied.	Violations can be suppressed.
Operates on a single file at a time.	Can require the dependency graph.
Can always format just changed files / regions	New violations might be introduced in unchanged files.
Fast enough to put in a pre-commit workflow.	Some are slow.

For linter vs. test, we wrote about our reasoning for my Java static analysis tool ErrorProne: http://errorprone.info/docs/criteria. See also our CACM Paper Lessons from building Static Analysis tools at Google and I gave a talk at CapitalOne about it.

linter	test
Think of as WARNING	Think of as ERROR
Best presented as robot review comments	Best presented as test failures
Reviewdog is a good UI	Buildkite is a good UI
Okay to have existing violations	Codebase must be 100% clean
Works at scale: can enable new lints without fixing all	Hard to scale: must have a codemod/auto-fixer to enable new test in a big repo
Okay for tool to produce false positives	False positives very painful as they must be suppressed/commented

thundergolfer/bazel-linting-system

Sample change adding SwiftFormat:
https://github.com/thundergolfer/bazel-linting-system/compare/master…alexeagle:bazel-linting-system:master

Done right:

1. Uses aspects. BUILD files don't have extra load statements or format lines.

load("@build_bazel_rules_swift//swift:swift.bzl", "swift_library")

swift_library(
    name = "swift",
    srcs = ["hello.swift"],
) 

2. Aspect produces data files as cacheable, RBE-friendly action outputs, then separate tooling is concerned with making assertions or applying fixes.
3. Each linter is a rule, has reasonable API

linter(
    name = "swift",
    executable = "@swiftformat",
)

4. Language repos don't have to be linter-aware, and the linting-system has minimal language awareness.

Maybe not right:

1. Can't express depgraph, e.g. mypy is specifically called out as unsupported.

   This project was designed with linters like black and gofmt in mind. Given their behaviour, they're perhaps more accurately called formatters, but to me formatters are a subclass of linters.

2. Only allows for one tool per language.
3. Apply tool didn't work for me
4. Common workflows left for users to encode in Bash scripts they maintain
5. Requires that all files to be formatted are srcs of some *_library rule, rather than just bazel run a tool that can talk to git and look around the whole workspace.

apple/apple_rules_lint

Code: https://github.com/apple/apple_rules_lint

Maybe not right:

1. Users have to put a lint rule into their BUILD file.
2. Every linter has to be a custom rule implementation, not just point to a binary
3. Linter configuration has to be represented as a rule, not just a file

rules_go#nogo

From Son:

Run as part of go_library GoCompilePkg action.
The compilepkg command serves several purposes: transpile cgo code, create test code wrapper, generate test coverage collection, run staticanalysis, compile code etc… Leverages existing Go's https://pkg.go.dev/golang.org/x/tools/go/analysis framework that is widely used in other linters.

Good:

• Result cached with bazel, could be executed incrementally
• Fast, attached to toolchain
  Downside:
• 1 big action, hard to parallelize if many linters added
• Attached to code compile, if lint does not pass, code will not compile
• Could not apply auto fixes
• Lints run on external deps by default
• Global config could not be override at package level, need additional code to generate global config.
• Could not run lints that requires info external to package: check for unused references

Aspect design: formatting

Simple. Just fetch the tools and run them with bazel run. Proof-of-concept: https://github.com/aspect-build/bazel-super-formatter

Aspect CLI could have a format command/plugin but it's small sugar over bazel run @aspect_rules_fmt//fmt.

Aspect design: linting

• Can we start from one or both of the above?
• We'll use an aspect (haha)
• Ask the user to add their VCS "what files changed" command to their --workspace_status_file