Added rustfmt rules and aspects.

[UebelAndre]

This pull request implements the following new rules

• rustfmt_aspect: An aspect for parsing information from rust targets for use in the rustfmt rule.
• rustfmt_test: A test rule for checking the formatting of a list of targets.

The intended use of these rules is to have users define a --@rules_rust//:rustfmt.toml= flag in their .bazelrc file and begin to run the @rules_rust//tools/rustfmt target to start formatting rust source files. I strongly feel that formatting errors should be an opt-in failure mode to keep iterating on changes fast and free from distracting (non-functional) failures. This is why there are two outuput groups for rustfmt_aspect, rustfmt_manifest (non fatal), and rustfmt_checks (fatal). To make sure formatting is totally up to date, users can use the rustfmt_aspect with the rustfmt_checks output group in CI to have all source targets checked. To test targets individually, the rustfmt_test rule can be used for more granular control.

Closes #634
Closes #87

[UebelAndre]

@hlopko This is still in a draft phase but would love to get your thoughts on this approach for rustfmt support. The thing I care most about is rustfmt being something we can optionally enable but always run. As a developer I don't want my build to fail because of rustfmt as I iterate on changes. I want to have finished writing my code and tests and have CI remind me that I need to run rustfmt or have a commit hook run it for me or something. This is what drew me to aspects and this design of rustfmt_aspect vs rustfmt_check_aspect. The intent is that users would be able to have a .bazelrc for their CI environment which uses --aspects=@rules_rust//rust:defs.bzl%rustfmt_check_aspect. There's some documentation I need to write for this and some polish in the runner but am looking for feedback in the overall approach. 🙏

[hlopko]

Very brief review only

[hlopko]

I don't yet understand why rustfmt_aspect exists. rustfmt_check_aspect deals with traversing the graph and generating validation actions, right? I imagine I'd be using it by passing --aspects=@rules_rust//rust:defs.bzl%rustfmt_check_aspect locally if I wanted to double check, and I imagine we'll have a CI job passing that aspect too.

When should I use --aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect?

I LGTMed the formatting PR, please merge and sync this one so there are only relevant files to review. Thank you!

[UebelAndre]

When should I use --aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect?

This aspect is used to gather information about a target for use with a rustfmt target (ie @rules_rust//tools/rustfmt:rustfmt). The key information that's being gathered by that aspect is the edition which cargo fmt otherwise gets from the Cargo.toml file. I could remove it if I were able to get this information in a query or something but I don't think that's possible.

Again, one of my goals is to have a workflow where users can easily apply rustfmt fixes but not be unable to determine that their code successfully compiles or their tests pass. Using --aspects=@rules_rust//rust:defs.bzl%rustfmt_aspect is a non fatal aspect for generating information for running rustfmt where --aspects=@rules_rust//rust:defs.bzl%rustfmt_check_aspect is the fatal check.

[UebelAndre]

@hlopko Maybe https://github.com/UebelAndre/rules_rust/blob/fmt/docs/rust_fmt.md could clear some things up?

[UebelAndre]

I've updated the test/rustfmt tests to sufficiently test the use of --edition 2015 vs --edition 2018. The use of async when formatting using edition 2015 produces the following error:

error[E0670]: `async fn` is not permitted in Rust 2015
  --> /Users/user/Code/rules_rust/test/rustfmt/srcs/2015/formatted.rs:34:1
   |
34 | async fn edition() -> i32 {
   | ^^^^^ to use `async fn`, switch to Rust 2018 or later
   |
   = help: set `edition = "2018"` in `Cargo.toml`
   = note: for more on editions, read https://doc.rust-lang.org/edition-guide

rustfmt can parse the edition from a Cargo.toml file but in the case of Bazel, we don't use them. This is the purpose of rustfmt_aspect. This aspect provides the appropriate edition to use and avoids errors like the one above.

[UebelAndre]

1. I'm not sure why there are two near-identical aspects - could we drive the check vs fix based on which output group you're requesting, rather than by specifying different aspects? One aspect could register both actions, but only the ones driven by the selected `OutputGroup` would actually run.

I didn't realize I could use the output groups to control what actions were run. I thought it was an all or nothing process. It seems that by having creating rustfmt_manifest and rustfmt_checks output groups, I'm able to run the formatter without it first failing saying my code is not formatted. This is exactly what I want.

[UebelAndre]

2\. Is there prior art for the `_check` rule-name suffix? In my group we've generally been calling this kind of rule things like `rustfmt_test` so that querying for `_test$` shows them up, because we often want to run them at the same time as tests.

I dislike the use of _test for targets that are not actually Bazel test targets. I've used check to signify that this is not actually a test target (that would be picked up by bazel test //...). If there's an easy way to turn this rule into a test target, then I'd love to do that.

[illicitonion]

1. I'm not sure why there are two near-identical aspects - could we drive the check vs fix based on which output group you're requesting, rather than by specifying different aspects? One aspect could register both actions, but only the ones driven by the selected `OutputGroup` would actually run.

I didn't realize I could use the output groups to control what actions were run. I thought it was an all or nothing process. It seems that by having creating rustfmt_manifest and rustfmt_checks output groups, I'm able to run the formatter without it first failing saying my code is not formatted. This is exactly what I want.

Hurrah! Yeah, the general idea is the aspect registers more optional actions that could be run, and the OutputGroups drive which of those actions actually get run, based on what's needed to produce those outputs :)

[illicitonion]

2\. Is there prior art for the `_check` rule-name suffix? In my group we've generally been calling this kind of rule things like `rustfmt_test` so that querying for `_test$` shows them up, because we often want to run them at the same time as tests.

I dislike the use of _test for targets that are not actually Bazel test targets. I've used check to signify that this is not actually a test target (that would be picked up by bazel test //...). If there's an easy way to turn this rule into a test target, then I'd love to do that.

If we're having to enumerate the lintable targets in the rustfmt_check target anyway, it feels like that could be a test rule, and the test rule implementation could look at the CrateInfo provider of each target to get the information it needs... The down-side is that the unit of caching would be the entire rustfmt_check target, rather than per-rust_* target...

Then again, at least the way my org would be likely to use this is we'd replace rust_library and friends with our own macros which would generate a rust_library target and a rustfmt_test target each time someone makes a rust_library (that's what we've done for other languages' linters), so that would work fine for us!

[UebelAndre]

If we're having to enumerate the lintable targets in the rustfmt_check target anyway, it feels like that could be a test rule, and the test rule implementation could look at the CrateInfo provider of each target to get the information it needs...

I wasn't even sure adding a rule like this was a good idea. How do you pass test targets into it? I'd definitely want to make sure my rust_test targets are also formatted.

[illicitonion]

This looks great!

Am I understanding right that there are now two independent ways of running rustfmt? One is to use an aspect + output_group, and the other is to make a rustfmt_test target which depends on all of the targets you want to rustfmt, which will allow bazel test target to do the checking with no aspect flag needed?

[UebelAndre]

Am I understanding right that there are now two independent ways of running rustfmt? One is to use an aspect + output_group, and the other is to make a rustfmt_test target which depends on all of the targets you want to rustfmt, which will allow bazel test target to do the checking with no aspect flag needed?

Correct. I just updated the PR description. You can run rustfmt with no setup required in your repo, and you can either use rustfmt_test or setup the aspect and rustfmt_checks output group to test your sources.

[illicitonion]

This looks great to me!