Cargo.toml: add workspace-level clippy config #1399

tarcieri · 2026-01-18T23:10:42Z

Adds a set of workspace-level lints including common ones we've used in the past and others collected via running clippy::pedantic on several of our crates and looking for interesting ones.

Workspace-level lints require explict opt-in from each crate's Cargo.toml, so there's nothing forcing any crate to use them:

[lints]
workspace = true

That said, I have opted every crate in the workspace into these lints and gotten them all to pass except for wycheproof2blb which is an internal utility tool and I didn't feel like it was worth the hassle.

Even then, the lints are all set to "warn", so they can be easily overridden by #[allow(...)] attributes, including #![allow(...)] to shut them off at the granularity of an entire crate.

I managed to fix most of the issues that cargo clippy --fix didn't fix automatically including some missing documentation and documentation formatting issues.

The main thing I didn't fix was adding missing SAFETY comments, which is a problem with a few of the crates in this repo. I disabled a lint at the crate level and added a TODO where we're missing them and it wasn't easily corrected.

tarcieri · 2026-01-18T23:24:05Z

Oh fun, this seems to be incompatible with the minimal-versions lint since it blows away the workspace-level Cargo.toml

tarcieri · 2026-01-18T23:24:51Z

Not sure what's happening here... what?

error: missing documentation for the crate
  --> blobby/tests/mod.rs:1:1
   |
 1 | / #![cfg(feature = "alloc")]
 2 | | #![allow(missing_docs, clippy::panic_in_result_fn)]
 3 | |
 4 | | const ITEMS_LEN: usize = 10;
...  |
31 | |     Ok(())
32 | | }
   | |_^
   |
   = note: `-D missing-docs` implied by `-D warnings`
   = help: to override `-D warnings` add `#[allow(missing_docs)]`

...it's literally right there!

Edit: though this lint seems fixed easily enough by adding a toplevel rustdoc comment

tarcieri · 2026-01-18T23:51:26Z

Well, it's green, though I had to shut off the minimal-versions checks for the workspace-level config to work.

Maybe we can figure out some way to implement that which doesn't involve blowing away the workspace-level Cargo.toml? That seems a little problematic.

tarcieri · 2026-01-19T00:59:58Z

Something else we could use these configs for is setting some allows... like needless_range_loop

Adds a set of workspace-level lints including common ones we've used in the past and others collected via running `clippy::pedantic` on several of our crates and looking for interesting ones. Workspace-level lints require explict opt-in from each crate's Cargo.toml, so there's nothing forcing any crate to use them: [lints] workspace = true That said, I have opted every crate in the workspace into these lints and gotten them all to pass except for `wycheproof2blb` which is an internal utility tool and I didn't feel like it was worth the hassle. Even then, the lints are all set to `"warn"`, so they can be easily overridden by `#[allow(...)]` attributes, including `#![allow(...)]` to shut them off at the granularity of an entire crate. I managed to fix most of the issues that `cargo clippy --fix` didn't fix automatically including some missing documentation and documentation formatting issues. The main thing I didn't fix was adding missing `SAFETY` comments, which is a problem with a few of the crates in this repo. I disabled a lint at the crate level and added a TODO where we're missing them and it wasn't easily corrected.

newpavlov

I think you overuse #[must_use] a fair bit and it should be used in cases where a genuine mistake is likely and can cause silent bugs. See: https://std-dev-guide.rust-lang.org/policy/must-use.html As an example, Vec::len is not annotated with it, but you added it to InOutBuf::len.

Since it's not relevant to fixing the new lints, it's probably better to do in a separate PR.

newpavlov · 2026-01-19T03:13:00Z

wycheproof2blb/src/wycheproof.rs


-impl std::fmt::Display for CaseResult {
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+impl core::fmt::Display for CaseResult {


Add use core::fmt; to make it a bit less cluttered?

wycheproof2blb/src/mac.rs

newpavlov · 2026-01-19T03:19:38Z

zeroize/tests/zeroize.rs

+#![allow(
+    clippy::missing_safety_doc,
+    clippy::std_instead_of_alloc,
+    clippy::std_instead_of_core,


std_instead_of_core and std_instead_of_alloc could be easily fixed.

newpavlov · 2026-01-19T03:23:41Z

zeroize/tests/zeroize.rs

+    clippy::std_instead_of_alloc,
+    clippy::std_instead_of_core,
+    clippy::undocumented_unsafe_blocks,
+    clippy::unwrap_used


Clippy has option allow-unwrap-in-tests which may be better than allowing this lint in every test.

Aha, interesting

newpavlov · 2026-01-19T03:26:20Z

inout/src/reserved.rs

    /// Transform buffer into [`PaddedInOutBuf`] using padding algorithm `P`.
+    ///
+    /// # Errors
+    /// - if the padding is invalid


I think it's better to not have one entry lists and write simply:

/// # Errors /// If the padding is invalid.

Same for other one-entry error and panic docs.

newpavlov · 2026-01-19T03:35:39Z

block-padding/src/lib.rs


 impl Padding for Pkcs7 {
    #[inline]
+    #[allow(clippy::cast_possible_truncation)]


Replace the cast with u8::try_from(..).expect("...")? I think the compiler should be able to remove the panic, but it's worth to check.

newpavlov · 2026-01-19T03:39:59Z

Cargo.toml

+must_use_candidate = "warn"
+needless_range_loop = "allow"
+implicit_saturating_sub = "warn"
+panic_in_result_fn = "warn"


I am not sure about usefulness of the panic_in_result_fn lint in our context.

I marked a couple of my crates as clippy::panic in this PR. panic_in_result_fn is a bit more manageable. It also won't trip on const asserts.

I'm fine with removing it from the workspace-level, but it does seem good to me. At one point I was trying my best to write panic-free code but the lack of tooling to ensure that's actually the case is somewhat demotivating.

IIUC the lint did not result in any fixes in our code and I can not recall a single case where it could've been legitimate, so I think it's better to remove it and associated allows.

I can remove it, but the general idea that something which returns Result probably shouldn't panic if possible seems like a good one to me

Ideally, sure. But in practice we use Result for cases when user is responsible for the error and panics to guard against potential bugs in our code. Most of such panics get eliminated by optimization passes, but Clippy is unable to see it.

newpavlov · 2026-01-19T03:48:40Z

The minimal versions problem can be probably fixed by tweaking the action to cut [lints] workspace = true from the crate's Cargo.toml.

tarcieri · 2026-01-19T04:01:56Z

I think you overuse #[must_use] a fair bit and it should be used in cases where a genuine mistake is likely and can cause silent bugs. See: https://std-dev-guide.rust-lang.org/policy/must-use.html As an example, Vec::len is not annotated with it, but you added it to InOutBuf::len.

The changes were applied with cargo clippy --fix. This is what the clippy::must_use_candidate lint thinks should have #[must_use], not me. And the same can be said about most of the fixes in this PR besides the documentation.

FWIW, I was sent down this whole road after encountering a bug in ctutils that would've been caught by #[must_use]. I think we should have some kind of automated check for it, although clippy seemingly has two different checks, and clippy::must_use_candidate seems like the more eager one.

tarcieri · 2026-01-19T04:04:18Z

The minimal versions problem can be probably fixed by tweaking the action to cut [lints] workspace = true from the crate's Cargo.toml.

I think it would probably be better if it emitted a dummy workspace-level Cargo.toml that just had:

[workspace]
members = ["crate-under-test"]

That way we don't need to update anything if we add any other workspace-level configs.

newpavlov · 2026-01-19T04:06:41Z

I think it would probably be better if it emitted a dummy workspace-level Cargo.toml

Sure, but it would be a bit harder to implement than just cutting the lines. I think it's fine to start with the simple workaround and improve it later.

The changes were applied with cargo clippy --fix.

Ah, ok. Personally, I think that most of the added #[must_use]s are not necessary, but it's not like they break anything so I am fine with keeping them if you prefer to have them.

tarcieri · 2026-01-19T04:08:25Z

Really? One's just emitting a file from a template where the other you're trying to munge an existing file. Former sure seems easier and more maintainable to me, especially since it will work in perpetuity with other workspace-level configs

newpavlov · 2026-01-19T04:09:58Z

For emitting the file you need to retrieve the crate name, while cutting the lines is one simple command. We also probably want to keep any other settings in the root Cargo.toml (i.e. ideally we should only modify the members field). I am fine with either option. Either way, I would prefer if the minimal versions jobs were fixed before merging.

tarcieri requested a review from newpavlov January 18, 2026 23:10

tarcieri force-pushed the ci/workspace-level-clippy-config branch from e8b32f8 to 41aa651 Compare January 18, 2026 23:20

tarcieri force-pushed the ci/workspace-level-clippy-config branch from 41aa651 to 26fc609 Compare January 18, 2026 23:36

tarcieri force-pushed the ci/workspace-level-clippy-config branch from 26fc609 to fd1cf9d Compare January 19, 2026 02:55

Cargo.toml: allow clippy::needless_range_loop

65bea54

newpavlov reviewed Jan 19, 2026

View reviewed changes

Cargo.toml: add workspace-level clippy config #1399

Are you sure you want to change the base?

Cargo.toml: add workspace-level clippy config #1399

Uh oh!

Conversation

tarcieri commented Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarcieri commented Jan 18, 2026

Uh oh!

tarcieri commented Jan 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarcieri commented Jan 18, 2026

Uh oh!

tarcieri commented Jan 19, 2026

Uh oh!

newpavlov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlov Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlov Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newpavlov commented Jan 19, 2026

Uh oh!

tarcieri commented Jan 19, 2026

Uh oh!

tarcieri commented Jan 19, 2026

Uh oh!

newpavlov commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarcieri commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

newpavlov commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tarcieri commented Jan 18, 2026 •

edited

Loading

tarcieri commented Jan 18, 2026 •

edited

Loading

newpavlov Jan 19, 2026 •

edited

Loading

newpavlov Jan 19, 2026 •

edited

Loading

newpavlov commented Jan 19, 2026 •

edited

Loading

tarcieri commented Jan 19, 2026 •

edited

Loading

newpavlov commented Jan 19, 2026 •

edited

Loading