diff --git a/README.md b/README.md index 398cce7f..39b841d8 100644 --- a/README.md +++ b/README.md @@ -9,20 +9,11 @@ Features: Instead, users simply lint their existing `*_library` targets. - Lint checks and fixes are run as normal Bazel actions, which means they support Remote Execution and the outputs are stored in the Remote Cache. - Lint results can be **presented in various ways**, such as Code Review comments or failing tests. - See [Usage](https://github.com/aspect-build/rules_lint/blob/main/docs/linting.md#usage) below. + See [Usage](https://github.com/aspect-build/rules_lint/blob/main/docs/linting.md#usage). - **Can format files not known to Bazel**. Formatting just runs directly on the file tree. No need to create `sh_library` targets for your shell scripts, for example. - Honors the same configuration files you use for these tools outside Bazel (e.g. in the editor) -This project is inspired by the design for [Tricorder]. -This is how Googlers get their static analysis results in code review (Critique). -https://github.com/google/shipshape is an old, abandoned attempt to open-source Tricorder. -It is also inspired by . - -[aspect cli]: https://docs.aspect.build/v/cli -[tricorder]: https://static.googleusercontent.com/media/research.google.com/en/pubs/archive/43322.pdf -[reviewdog]: https://github.com/reviewdog/reviewdog - ## Supported tools | Language | Formatter | Linter(s) | @@ -82,9 +73,14 @@ Thanks!! > We'll add documentation on adding formatters as well. -## Design +## Installation -Formatting and Linting work a bit differently. +Follow instructions from the release you wish to use: + + +## Usage + +Formatting and Linting are inherently different, which leads to differences in how they are used in rules_lint. | Formatter | Linter | | ----------------------------------------------------------------- | ------------------------------------------------------ | @@ -96,49 +92,25 @@ Formatting and Linting work a bit differently. | 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. | -This leads to some minor differences in how they are used in rules_lint. - -We treat type-checkers as a build tool, not as a linter. This is for a few reasons: - -- They are commonly distributed along with compilers. - In compiled languages like Java, types are required in order for the compiler to emit executable bytecode at all. - In interpreted languages they're still often linked, e.g. TypeScript does both "compiling" to JavaScript and also type-checking. - This suggests that rules for a language should include the type-checker, - e.g. we expect Sorbet to be integrated with rules_ruby. -- We think most developers want "error" semantics for type-checks: - the whole repository should successfully type-check or you cannot commit the change. - rules_lint is optimized for "warning" semantics, where we produce report files and it's up to the - Dev Infra team how to present those, for example only on changed files. -- You can only type-check a library if its dependencies were checkable, which means short-circuiting - execution. rules_lint currently runs linters on every node in the dependency graph, including any - whose dependencies have lint warnings. - -## Installation - -Follow instructions from the release you wish to use: - - -## Usage - ### Format To format files, run the target you create when you install rules_lint. -We recommend using a Git pre-commit hook to format changed files, by running `bazel run //:format [changed file ...]`. +We recommend using a Git pre-commit hook to format changed files, and [Aspect Workflows] to provide the check on CI. [![asciicast](https://asciinema.org/a/vGTpzD0obvhILEcSxYAVrlpqT.svg)](https://asciinema.org/a/vGTpzD0obvhILEcSxYAVrlpqT) -See [Formatting](./docs/formatting.md) for more ways to use the formatter, such as a pre-commit hook or a CI check. +See [Formatting](./docs/formatting.md) for more ways to use the formatter. ### Lint -To lint code, we recommend using the Aspect CLI to get the missing `lint` command. +To lint code, we recommend using the [Aspect CLI] to get the missing `lint` command, and [Aspect Workflows] to provide first-class support for "linters as code reviewers". For example, running `bazel lint //src:all` prints lint warnings to the terminal for all targets in the `//src` package: [![asciicast](https://asciinema.org/a/xQWU1Wc1JINOubeguDDQbBqcq.svg)](https://asciinema.org/a/xQWU1Wc1JINOubeguDDQbBqcq) -See [Linting](./docs/linting.md) for more ways to use the linter, such as running as a test target, or presenting results as code review comments. +See [Linting](./docs/linting.md) for more ways to use the linter. ### Ignoring files @@ -162,3 +134,25 @@ But we're not trying to stop anyone, either! You could probably configure the editor to always run the same Bazel command, any time a file is changed. Instructions to do this are out-of-scope for this repo, particularly since they have to be formulated and updated for so many editors. + +## FAQ + +### What about type-checking? + +We consider type-checkers as a build tool, not as a linter. This is for a few reasons: + +- They are commonly distributed along with compilers. + In compiled languages like Java, types are required in order for the compiler to emit executable bytecode at all. + In interpreted languages they're still often linked, e.g. TypeScript does both "compiling" to JavaScript and also type-checking. + This suggests that rules for a language should include the type-checker, + e.g. we expect Sorbet to be integrated with rules_ruby and mypy/pyright to be integrated with rules_python or Aspect's rules_py. +- We think most developers want "build error" semantics for type-checks: + the whole repository should successfully type-check or you cannot commit the change. + rules_lint is optimized for "warning" semantics, where we produce report files and it's up to the + Dev Infra team how to present those, for example only on changed files. +- You can only type-check a library if its dependencies were checkable, which means short-circuiting + execution. rules_lint currently runs linters on every node in the dependency graph, including any + whose dependencies have lint warnings. + +[aspect workflows]: https://docs.aspect.build/workflows +[aspect cli]: https://docs.aspect.build/cli diff --git a/docs/formatting.md b/docs/formatting.md index 02a2734a..534437f0 100644 --- a/docs/formatting.md +++ b/docs/formatting.md @@ -88,7 +88,10 @@ $ chmod u+x .git/hooks/pre-commit ### Check that files are already formatted -There are two ways. +We recommend using [Aspect Workflows] to hook up the CI check to notify developers of formatting changes, +and offer to apply them as a suggestion in the code review thread. + +To set this up manually, there are two supported methods: #### 1: `run` target @@ -138,3 +141,5 @@ format_test( Then run `bazel test //tools/format/...` to check that all files are formatted. [Gazelle]: https://github.com/bazelbuild/bazel-gazelle +[Aspect Workflows]: https://docs.aspect.build/workflows +[Aspect CLI]: https://docs.aspect.build/cli diff --git a/docs/linting.md b/docs/linting.md index 5bfb5703..de5c3365 100644 --- a/docs/linting.md +++ b/docs/linting.md @@ -18,16 +18,32 @@ Finally, register those linter aspects in the lint runner. See details below. ## Usage -### 1. (Coming Soon) Linter as Code Review Bot +### 1. Linter as Code Review Bot -An upcoming release of [Aspect Workflows](https://docs.aspect.build/workflows/) will include a `lint` task, which wires the reports from bazel-out directly into your code review. +[Aspect Workflows] includes a `lint` task, which wires the reports from bazel-out directly into your code review. The fixes produced by the tool are shown as suggested edits, so you can just accept without a context-switch back to your development machine. ![lint suggestions in code review](./lint_workflow.png) +We recommend this workflow for several reasons: + +1. Forcing engineers to fix lint violations on code they're still iterating is a waste of time. + Code review is the right time to consider whether the code changes meet your teams quality bar. +2. Code review has at least two parties, which means that comments aren't simply ignored. + The reviewer has to agree with the author whether a suggested lint violation should be corrected. + Compare this with the usual complaint "developers just ignore warnings" - that's because the warnings were presented in their local build. +3. Adding a new linter (or adjusting its configuration to be stricter) doesn't require that you fix or suppress all existing warnings in the repository. + This makes it more feasible for an enthusiastic engineer to setup a linter, without having the burden of "making it green" on all existing code. +4. Linters shouldn't be required to have zero false-positives. Some lint checks are quite valuable when they detect a problem, but cannot always avoid overdetection. + Since code review comments are always subject to human review, it's the right time to evaluate the suggestions and ignore those which don't make sense. +5. This is how Google does it, in the [Tricorder] tool that's integrated into code review (Critique) to present static analysis results. + With [Aspect Workflows] we've provided a similar experience. + +[Tricorder]: https://static.googleusercontent.com/media/research.google.com/en/pubs/archive/43322.pdf + ### 2. Warnings in the terminal with `bazel lint` -Aspect CLI adds the missing 'lint' command, so users just type `bazel lint //path/to:targets`. +[Aspect CLI] adds the missing 'lint' command, so users just type `bazel lint //path/to:targets`. Reports are then written to the terminal. @@ -45,9 +61,9 @@ lint: ### 3. Warnings in the terminal with a wrapper -If you don't use Aspect CLI, you can use vanilla Bazel with some wrapper like a shell script that runs the linter aspects over the requested targets. +If you don't use [Aspect CLI], you can use vanilla Bazel with some wrapper like a shell script that runs the linter aspects over the requested targets. -See the `example/lint.sh` file as an example. +See the `example/lint.sh` file as an example, and pay attention to the comments at the top about fitting it into your repo. [![asciicast](https://asciinema.org/a/gUUuQTCGIu85YMl6zz2GJIgD8.svg)](https://asciinema.org/a/gUUuQTCGIu85YMl6zz2GJIgD8) @@ -59,7 +75,7 @@ This is the same flag many linters support. ### 4. Errors during `bazel build` -Add `--@aspect_rules_lint//lint:fail_on_violation` to the command-line, +Add `--@aspect_rules_lint//lint:fail_on_violation` to the command-line or to your `.bazelrc` file to cause all linter aspects to honor the exit code of the lint tool. This makes the build fail when any lint violations are present. @@ -80,3 +96,6 @@ To bypass this filter, add `tags=["lint-genfiles"]` to a target to force all the Some linters honor the debug flag in this repo. To enable it, add a Bazel flag: `--@aspect_rules_lint//lint:debug` + +[Aspect Workflows]: https://docs.aspect.build/workflows +[Aspect CLI]: https://docs.aspect.build/cli diff --git a/example/tools/format/BUILD.bazel b/example/tools/format/BUILD.bazel index 05b47a84..8e6849f1 100644 --- a/example/tools/format/BUILD.bazel +++ b/example/tools/format/BUILD.bazel @@ -88,6 +88,5 @@ format_test( starlark = "@buildifier_prebuilt//:buildifier", # FIXME: not hermetic: error while loading shared libraries: libFoundation.so # swift = ":swiftformat", - terraform = ":terraform", workspace = "//:.shellcheckrc", # A file in the workspace root, where the no_sandbox mode will run the formatter )