Aspect Lint - HackMD

# Aspect Lint Status: **DRAFT** | UNDER REVIEW | REVIEWED | PUBLISHED Date: _Month dd, yyyy_ Authors: _Jesse Tatasciore \<jesse@aspect.dev\>, Thulio Assis \<thulio@aspect.dev\>_ Reviewers: ... Summary: _Use a short sentence to summarize the design doc._ --- **This document follows the conventions from [RFC 2119](https://www.ietf.org/rfc/rfc2119.txt).** ## Background _Use a neutral language to succinctly explain the design you are proposing in this document. Use at most 2 paragraphs. Avoid using acronyms here and for the rest of the document. Assume the readers are going to be a mix of technical and non-technical stakeholders. Remember to always use facts in this document. Avoid using any kind of emotional or personal tone (this is a technical document!)._ Linting is often a topic that brings out divides between developers. Couple this with questions regarding the role that a build system should play in linting and you are left with a highly divisive topic. In this document I will explain why.... ## Goals and non-goals _Succinct bullet points for the goals of this design. Non-goals should be things that are out-of-scope from this design that reviewers could have asked about. Making them clear non-goals here avoids unnecessary discussions._ _Alternatively, you can create another section named "Alternatives considered" that explains in further detail ideas that were abandoned._ - First customer: Lucid, make the thing they need ## Use cases - Dogfooding: replace what we use in our own repos (pre-commit.com) - e.g. Aspect repos where PRs weren't checked by shellcheck - I want it to fail bazel build - For example, running type-checking - I want to fail bazel test - I want CI to hard fail if there's a lint warning ANYWHERE in my files - I want to run a separate lint command - Filter results based on files I edited - See git-clang-format - Apply fixes - `--all-files` option ## Terminology - formatter: engineers should blindly apply changes - linter: may have false positives, requires judgement - syntactic: works on one file at a time (or a range of lines) - semantic: requires the dependency graph ## Possible execution models - RH: bazel build some-linter; ./bazel-out/some-linter some-files/** - Aspects, where the targets aren't in the graph - Edit the build graph (maybe with wrapper macros) so the lint targets are there ## Proposal _Here you put the lengthy (or not) design proposal. The previous sections were preparing the readers for this main section. Again, try to be succinct. A succinct document is more pleasant to read and will attract more attention and comments._ _Focus on expanding the goals and what the requirements are. Make them clear enough that managers can use this document to decide whether to proceed with the idea presented or not. Implementers should also be able to use this document to produce estimatives for the effort to implement the proposal. This document should not contain broken down tasks or implementation plannings._ ### TLDR Proposing that we create an `aspect lint` command. This command will run linters over a bazel repository. Said linters will be configured in bazel and to help with that we create a `rules_lint` repo that contains ready to use bazel targets that users can rely on. ### Creating the `rules_lint` repository The idea behind `rules_lint` is that we want an easy to use bazel repository that people can import to gain access to lint targets. As part of this, the repository would contain an aspect cli plugin which would provide `aspect lint` functionality. We will go into more details about the plugin in the next section. The repository will provide linting in 2 forms. `One -> Many` linting and `Many -> Many` linting. #### One -> Many Linters of this form would be a single target that could take in a list of files and lint them. The best way to perform this linting would likely be using aspects. By using aspects we can hook onto regular bazel builds and tests. The aspect rule should take in the files, lint them and then output the linting results into a file that can be read by the CLI plugin. The CLI plugin can then decide what to do with the linting result #### Many -> Many Linters of this form would be added into macros for their given language. Each linting target should take in the srcs of the macro and only the srcs. We do not want to include dependencies as well as it would break caching of the linting results. Each linting target should output a file containing the results of linting the incoming files. These files can then be read by the CLI plugin which will decide what to do with the results. #### Bringing them together Both modes will function in essentially the same way. The cli plugin will call a linter with a set of files. The output of the linting should be a file that can then be read by the aspect plugin. #### Fix targets Fix targets should be made available only when the linter being used has a `--fix` mode built in. This is so that if there are lint failures and the user calls `aspect lint --fix` we can call these targets automatically. Fix targets should function in the exact same way as a regular lint target with the only change being an added flag when running. To ensure the plugin can find the target, it should be named the same as the original linting target with a suffix of `_fix`. Fix targets should be extra targets added for the given linting targets. Having a fix target should not mean removing the original lint target. If a user runs *What happens if the tool is able to produce multiple fix options* ### Creating the `aspect lint` command The `aspect lint` command will come from a CLI plugin that lives in the `rules_lint` repository. There are a few reasons behind this decision. Firstly, it means that we will not have built linting directly into the CLI which may make it a less divisive topic. If people want to use our solution, they can easily opt into it, but it wont feel like we are forcing it onto anyone. Secondly, it will help pave the way for other rulesets to also ship with a CLI plugin. Having rulesets provide an aspect CLI plugin will hopefully increase CLI adoption and allow for rulesets to improve their user experiences. - Compile all of the linting logs together so that they can be shown easily to the user - When running `aspect lint` it makes sense to have those errors be printed after the run. If the user wants autofixing to happen then do we still print the errors? If any cannot be autofixed then definitely but the ones that were autofixed? Maybe in a log file somewhere but not in the main terminal output - When running `aspect build` or `aspect test` it might make sense to have the lint errors put somewhere as well. Maybe have a config setting? If the user wants then print the lint errors here as well. If not then save the lint errors to a log file somewhere that the user can check? Not sure in the value there - Do we want the autofix to be the default? Maybe when running a regular `aspect build` it shows the lint failures but if I run an `aspect lint` it always autofixes. This sounds like another config setting that could be asked of the user - Since the linters themselves will not fail the build in any way, the plugin has control of failing or not failing the build. Do we check what files have changed and only fail if they have linting errors? Do we allow lint failures from certain linters to fail the build? Others just warnings? Do we prompt the user on a `aspect lint` saying "we found the follow lint errors that may have an autofix. Attempt autofix?". If we have this prompt it should mean that we have a non-interactive mode for this for CI. We would check the - Do we want to prompt the user after a `aspect build` to see if they want to fix linting errors? Rather than just on an `aspect lint`? Might be a good idea - Run a bazel build with `--build_tag_filters`. To include lint targets just add `aspect_lint` tag to the targets and then filer for those. If `--fix` is specified then the tags could be `aspect_lint,-aspect_lint_has_fix,aspect_lint_fix`. Can track the BEP from the build to get the resulting files and then check those. - Need to include the bazel aspect linters as well. For those ones ### When to propagate linting errors from bazel There are 2 ways the linting could be run under bazel (not including when running `aspect lint`). During a `bazel build` or during a `bazel test`. Both have benefits and drawbacks which we will explore. When implementing our solution we should default to having linting run during `bazel build` but allow for the user to opt into having the linting targets run as tests. ##### `bazel build` During a `bazel build` the obvious answer would be to use validation actions. Using validation actions presents several problems however. 1. All of the validation actions would be run and the validation failures would fail the build. This does not work for projects that only want linting on changed files. This is explained in more detail in the next section 2. The outputted from validation actions are not part of the typical output group. It is therefore difficult to access. When testing I was unable to aquire the output file. This creates a problem because should a user run `aspect lint` they will want their linting failures displayed in an easy to read way. We therefore need to be able to access the files that contain the results of the linting The easiest solution is to mimic the validation action behaviour in that we will create a file for a given rule that contains the linting results. However we will return it from the standard output group. By doing this it means that the build will not fail and we will be able to access the the files using information from the BEP. Due to the fact that the build is no longer going to fail we must now fail the build accordingly using the aspect CLI. We can either process the BEP live or afterwards. Might make sense to process it live to keep things speedy. Since we are now in a situation where the aspect CLI will pass or fail the build we have an oportunity to selectively fail based on more input than just the result of linting. We will go into more details on this in the next section. ##### `bazel test` During a `bazel test` the solution is obvious, if there are lint errors then fail the test that is running the linting. The benefit of running linting as tests is that users will get feedback about linting within any intervention from the CLI. The role of the CLI in this senario is to facilitate the fixing of those linting errors. The downside to running linting using `bazel test` is that you would typically need to run all of the linting tests. This might work for some projects but not all of this. This is explained in more detail in the next section. ### Implementing in a large project One of the issues with linting a large project is that applying lint changes to the entire project is often not an option. Reasons include: - Landing the change without conflicts would be extremely difficult as an entire repo worth of changes would need to land in one PR - The person making the lint changes would not have context across the entire repo and therefore would not be able to ensure that the given changes work for every area of code The current non-bazel solution to this is that larger repositories only run linting against files that have been changed on a given PR. They do not run linting across everything in the repository. Our bazel solution should be similar. We may not want to fail the build when there are linting failures that happened outside of the scope of the changes made. To do this we can leverage the `bazel build` method for propagating errors that is mentioned above. If we run linting and save the results to known file but dont fail the build, we can now use the aspect CLI to fail the build. The CLI can check the changes that have been made on the current branch and only fail the build if one of those files had a corresponding linting failure. We could have 2 modes here 1. If running with `bazel build` or `bazel test` and there is already existing linting results to check then we do the checking live or afterwards using the BEP and checking the git changes 2. If running with `aspect lint` we could check the git changes first and run some form of query to figure out which targets to actually build. Would make it quicker than doing a `//...`. Or could we do a build with a "only build targets containing a certain tag sort of thing". Might make it quicker if we only built the linting targets Additionally maybe we could have a mode with a `--uncommitted` or something like that where we only run the linting against the local uncommited changes rather than against the changes vs the target branch All that said, we also need to ensure that repositories that want to lint the entire repository are able to do so. We should therefore have a config setting that will allow users to switch between the 2 linting modes.  ### Formatting. Should the CLI support it? Topics such as linting, formatting and auto-formatting can be very volitile. Many people have strong opinons on all three of these things and how they should fit together, if at all. While I have some strong opinions as well, now is not the time for that discussion. In regards to `aspect lint` and what should it do, it should support it. Why? Because by not supporting it we will be alienating many people who might have used our solution. Most people do not care about the arguments we might make on why linting or formatting or autofixing should work a certain way. They just want it to work and to be easy to use. If our solution doesnt have feature parity with what people have come to expect from linter and formatters then we will have a hard time with adoption. #### Formatters Many linters have formatters built in to them. Where there are not formatters built in, formatting is often suplimented in some way (by calling a formatter after linting is done). To say that we dont support formatting would be incorrect as formatters could be setup in the exact same way as linters. Just have the formatter populate a file if it finds any changes to be made and let the plugin decide what to do with them. #### Autofixing We should have the ability to apply automatic formatting and linting fixes for the sake of feature parity. To be clear, we do NOT want to add autofixing logic to the lint plugin itself but in the case where the linter or formatter being called has a `--fix` mode, we need to support calling it. If we dont have this ability it will create a terrible user experience compared to what people do now. We do NOT want this mode to be called by default. We could prompt the user after failed linting OR only run the fixers when we receive `aspect lint --fix`. This fix mode will be handled using the fix targets that are explained above. ### Changes needed for the CLI 1. Allow plugins to provide flags for existing verbs. We would need this functionality so that we could pass a flag such as `--lint-fix` to an `aspect build`. The plugin should handle functionality of the new flag. 2. Allow plugins to update a bazel command before it is run. The case for this is as follows. If have linting rules output a file containing the result of linting the given files, we want to make sure we can take advantage of caching. However, if the user has something like `--remote_download_minimal` then the linting results might not be downloaded. A solution to this would be to add linting results to a "linting" output group. We could then request that output group be downloaded. In order to do this we would want to add arguements to the `bazel build` or `bazel test` commands that are about to be run. ## Alternative Considered ### https://github.com/apple/apple_rules_lint This repo is problematic for several reasons: 1. It is confusing to get started. Trying to follow the setup instructions, it is not entirely clear how linting with their repository works. Something we should strive for with our solution are simple and clear instructions that our users can understand and follow 2. The repo is designed to run linting as test targets. This will not work for repositories where we dont want to run linting across the entire repository at the same time. We will get into the reasons for this more later. TODO: Add a reference here to where we talk about it more? ### https://github.com/thundergolfer/bazel-linting-system