# 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](https://cacm.acm.org/magazines/2018/4/226371-lessons-from-building-static-analysis-tools-at-google/fulltext?mobile=false) and I gave a [talk at CapitalOne](https://docs.google.com/presentation/d/1psV8yA9Z3NN-UymnMQ6dkvxZzcQtSpQxz4hLa6YuHxw/htmlpresent) 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 |

[Reviewdog]: https://medium.com/@haya14busa/reviewdog-a-code-review-dog-who-keeps-your-codebase-healthy-d957c471938b#.8xctbaw5u

## 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`