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!

* 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

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

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

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 added 2 commits November 9, 2024 16:21

start, add new rust_lint_group rule

ba085d7

* 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

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 added 2 commits November 18, 2024 09:22

responding to GitHub feedback

1a1703b

* 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

dc2b46a

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

	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

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