feat: Add `rust_lint_group` and `cargo_lints` rule to define lint groups #2993

ParkMyCar · 2024-11-10T06:04:41Z

Chatted about this previously in Slack. The goal with this PR is to make it easier to define and share lint configurations for your build.

Description

The PR adds two new rules, rust_lint_group and cargo_lints. It also adds a "lints" attr to rust_library and rust_binary rules that expects a new LintsInfo provider.

rust_lint_group allows you to define Rust, Clippy, and Rustdoc lints in a BUILD file.
cargo_lints automatically generates a set of lints by reading a crate's Cargo.toml, and optionally the workspace's Cargo.toml for workspace inheritance.

Then the rustc, clippy, and rustdoc actions are all updated to check for the LintsInfo provider and appends the correct arguments so the lints take effect.

Design

Honestly this is my first large change to a set of Bazel rules, so I definitely could have done something wrong here!

The change is split into two commits, the first introduces rust_lint_group which IMO is relatively straight forward. Some attributes are defined on a rule which are then formatted into command line flags and passed around with a provider.

The second commit adds cargo_lints and is much larger, a few things worth considering:

I was back and forth a bit on using a repository_rule or a regular rule. While the repository_rule maps well to a Cargo Workspace and you'd only need to define it once, not everyone uses workspaces and so I figured a regular rule was more general.
Parsing a Cargo.toml is done via a new Rust binary called cargo_toml_info. I tried to keep the external dependencies on this binary to a minimum, it only has one at the moment which is cargo_toml and I based this change largely off of cargo_toml_env_vars: rule for generating env vars from a Cargo.toml file #2772. I tried to make the tool general though so other Cargo metadata rules could use it in the future.

Tests

There aren't any! I wasn't sure where the best place to start was, any guidance here is appreciated!

illicitonion

This generally looks super reasonable, thanks so much for putting it together!

I just have a few comments which are basically all naming.

In terms of testing, one problem we have in Bazel in general is it's pretty hard to write tests that things which should fail do fail (particularly without them being wildly slow).

There are generally three styles of tests you can probably write for this functionality:

I would recommend mostly writing unit tests - you can see an example of how we write unit tests in https://github.com/bazelbuild/rules_rust/tree/main/test/unit/extra_rustc_flags - your test would probably look pretty similar, just testing that some extra flags are present.

For the rust code, you can just write some unit tests like normal.

And you can write a script that checks failing things do fail - we have an example for clippy that runs on CI:

rules_rust/.bazelci/presubmit.yml

Lines 367 to 371 in 60515a1

    
           clippy_failure: 
        
             name: Negative Clippy Tests 
        
             platform: ubuntu2004 
        
             run_targets: 
        
               - "//test/clippy:clippy_failure_test"

- these ones definitely don't have to be exhaustive, just one example showing each style of thing can fail (and ideally fails with a useful error message) is probably sufficient :)

rust/private/providers.bzl

rust/private/rust.bzl

illicitonion · 2024-11-14T19:05:28Z

rust/defs.bzl

@@ -157,4 +161,7 @@ rustfmt_test = _rustfmt_test
 rust_stdlib_filegroup = _rust_stdlib_filegroup
 # See @rules_rust//rust:toolchain.bzl for a complete description.

+rust_lint_group = _rust_lint_group


I'd be include to call this rust_lint_config rather than rust_lint_group - WTDY?

Yeah IMO "config" is definitely better than "group", changed!

illicitonion · 2024-11-14T19:07:44Z

cargo/cargo_toml_info/main.rs

@@ -0,0 +1,272 @@
+//! Command line tool invoked by Bazel to read metadata out of a `Cargo.toml` file.


Let's move this file into a private or internal package, maybe cargo/private/cargo_toml_info/main.rs? I don't think we want to advertise this binary's API as something we publicly support and offer compatibility for, it's just an implementation detail of our internals.

Done! Also repinned all dependencies which created a number of changes, sorry for the noise in the commit

illicitonion · 2024-11-14T19:13:37Z

cargo/cargo_toml_info/main.rs

+        let manifest = Manifest::from_path(&workspace_path)?;
+        let workspace_details = Some((&manifest, workspace_path.as_path()));
+
+        // TODO(parkmycar): Fix cargo_toml so we inherit lints from our workspace.


Oh dear, is there an issue we can link to for this? In the mean time, what behaviour do we see?

In the meantime we "manually" inherit lints by checking both the manifest and workspace Cargo.toml so the behavior is correct, but ideally this would be handled internally by cargo_toml. Filed an issue upstream and left a comment here

illicitonion · 2024-11-14T19:13:46Z

cargo/cargo_toml_info/main.rs

+    output_rustc_lints: PathBuf,
+    output_clippy_lints: PathBuf,
+    output_rustdoc_lints: PathBuf,
+    // TODO(parkmycar): Support Rust's new --check-cfg once cargo-toml is updated.


Can we link to an issue?

Filed and linked to cargo_toml#34!

cargo/private/cargo_lints.bzl

ParkMyCar · 2024-11-18T14:28:08Z

Thanks for the review @illicitonion! Didn't get a chance to add tests (although I did test manually by using this in my project), will follow up with that!

illicitonion · 2024-11-23T14:11:15Z

Looks good, thanks! If we can get those tests in, let's get this merged! 🎉

ParkMyCar · 2024-11-26T17:47:58Z

Had a free cycle so I added an initial unit test for rust_lint_group more than happy to add more when I have some time!

* allows defining rustc, rustc check-cfg, clippy, and rustdoc lints * add a 'lints' attribute to the common rust attributes * rust_doc and rust_clippy rules use the lints from the crate they target

* adds a new Rust binary cargo_toml_info which can parse a Cargo.toml and inherit from the workspace * adds a new cargo_lints rule which invokes the cargo_toml_info binary to get lints * update the LintsInfo provider to include CLI args via files

* rename fields on the LintsInfo provider * rename some methods * add comments linking to cargo_toml issues * move the cargo_toml_info binary into private

* update after renaming attribute from 'lints' to 'lint_config' * don't set rustc lints if building in the exec configuration * fix unpretty rules

* the new check-cfg linting is only available in Rust >1.80, the current min is Rust 1.72

* fix visiblity by moving cargo_toml_info dependencies into cargo/deps.bzl * small changes to appease the liner

* add new cargo/private/internal_extensions.bzl so we can include the relevant dependencies for cargo_toml_info * rename cargo_toml vendored crates repository to rrcti to help avoid long path names

* adds standalone example using WORKSPACE * extends existing example using bzlmod

ParkMyCar · 2024-12-14T21:42:57Z

@illicitonion whenever you have some time (no rush!) this is ready for a re-review! Sorry for all the commits, there were a bunch of tiny tweaks to get CI green. Overall you can probably skip the first 5 commits, nothing there changed since your last review.

What changed since your last review is:

Added bzlmod support for extract_cargo_lints by including the relevant repositories in rules_rust's root MODULE.bazel.
Adjusted the flags set for Clippy to also include rustc lints, this mimics what Cargo does.
Added a unit test to exercise the rust_lint_config rule, in test/unit/lint_flags
Added an example to exercise the extract_cargo_lints rule, in examples/cargo_lints
Updated an existing example to make sure extract_cargo_lints worked in bzlmod, in examples/bazel_env.

ParkMyCar changed the title ~~feat: Add rust_lint_group and cargo_lints rule to define~~ feat: Add rust_lint_group and cargo_lints rule to define lint groups Nov 10, 2024

illicitonion reviewed Nov 14, 2024

View reviewed changes

ParkMyCar marked this pull request as ready for review November 26, 2024 17:47

ParkMyCar force-pushed the rust-lints branch 3 times, most recently from ab79609 to cde68ac Compare December 1, 2024 03:19

ParkMyCar added 15 commits December 14, 2024 14:42

start, add new rust_lint_group rule

5d1db20

* allows defining rustc, rustc check-cfg, clippy, and rustdoc lints * add a 'lints' attribute to the common rust attributes * rust_doc and rust_clippy rules use the lints from the crate they target

responding to GitHub feedback

78c3593

* rename fields on the LintsInfo provider * rename some methods * add comments linking to cargo_toml issues * move the cargo_toml_info binary into private

rename cargo_lints to extract_cargo_lints

78204fc

couple of fixes

3510cb0

* update after renaming attribute from 'lints' to 'lint_config' * don't set rustc lints if building in the exec configuration * fix unpretty rules

add unit test for rust_lint_config

73bd2ff

fix for clippy.bzl

5b4e3fc

fix clippy for cargo_toml_info

e3229c5

Regenerate documentation

12839e1

adjust CI to not exercise new tests with min Rust version

8b212c1

* the new check-cfg linting is only available in Rust >1.80, the current min is Rust 1.72

buildifier

5a4cde8

adjust CI once more, leave TODO to figure out list inheritence

180adf5

fix final lints, adjust how users add dependencies for cargo_toml_info

a58d2e7

* fix visiblity by moving cargo_toml_info dependencies into cargo/deps.bzl * small changes to appease the liner

support bzlmod

02a15c2

* add new cargo/private/internal_extensions.bzl so we can include the relevant dependencies for cargo_toml_info * rename cargo_toml vendored crates repository to rrcti to help avoid long path names

pass through rustc lints when building clippy, mimics Cargo's behavior

b92dc48

ParkMyCar force-pushed the rust-lints branch 2 times, most recently from e5acd95 to 745d926 Compare December 14, 2024 21:22

add examples of extracting lints from Cargo

2e60553

* adds standalone example using WORKSPACE * extends existing example using bzlmod

ParkMyCar force-pushed the rust-lints branch from 745d926 to 2e60553 Compare December 14, 2024 21:24

ParkMyCar requested a review from illicitonion December 14, 2024 21:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add `rust_lint_group` and `cargo_lints` rule to define lint groups #2993

feat: Add `rust_lint_group` and `cargo_lints` rule to define lint groups #2993

ParkMyCar commented Nov 10, 2024 •

edited

Loading

illicitonion left a comment

illicitonion Nov 14, 2024

ParkMyCar Nov 18, 2024

illicitonion Nov 14, 2024

ParkMyCar Nov 18, 2024

illicitonion Nov 14, 2024

ParkMyCar Nov 18, 2024

illicitonion Nov 14, 2024

ParkMyCar Nov 18, 2024

ParkMyCar commented Nov 18, 2024

illicitonion commented Nov 23, 2024

ParkMyCar commented Nov 26, 2024

ParkMyCar commented Dec 14, 2024

	clippy_failure:
	name: Negative Clippy Tests
	platform: ubuntu2004
	run_targets:
	- "//test/clippy:clippy_failure_test"

		@@ -0,0 +1,272 @@
		//! Command line tool invoked by Bazel to read metadata out of a `Cargo.toml` file.

feat: Add rust_lint_group and cargo_lints rule to define lint groups #2993

Are you sure you want to change the base?

feat: Add rust_lint_group and cargo_lints rule to define lint groups #2993

Conversation

ParkMyCar commented Nov 10, 2024 • edited Loading

Description

Design

Tests

illicitonion left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParkMyCar commented Nov 18, 2024

illicitonion commented Nov 23, 2024

ParkMyCar commented Nov 26, 2024

ParkMyCar commented Dec 14, 2024

feat: Add `rust_lint_group` and `cargo_lints` rule to define lint groups #2993

feat: Add `rust_lint_group` and `cargo_lints` rule to define lint groups #2993

ParkMyCar commented Nov 10, 2024 •

edited

Loading