move clippy args to [workspace.lints.clippy]
#5715
Conversation
| # a `static Atomic`). However, it is also subject to false positives (e.g., | ||
| # idiomatically declaring a static array of atomics uses `const Atomic`). We | ||
| # warn on this to catch the former, and expect any uses of the latter to allow | ||
| # this locally. |
There was a problem hiding this comment.
take it or leave it: maybe worth also mentioning http::HeaderName explicitly, since that's probably one of the most common false positives we'll hit in omicron? it would be nice if someone who adds a const HeaderName in the future to immediately see "no, just allow that lint" written down here.
| # | ||
| [workspace.lints.clippy] | ||
| # Clippy's style nits are useful, but not worth keeping in CI. | ||
| style = { level = "allow", priority = -1 } |
There was a problem hiding this comment.
what does priority = -1 do? i assume it allows this to be overridden, but why do we only need to set it here?
There was a problem hiding this comment.
Until [lints], all methods for modifying lint levels had inherent ordering; either command line flags, or attributes in code. TOML tables are explicitly unordered, so Rust had to do something different here. Per the RFC any sorting beyond user-defined priorities is undefined (more detail); style has to come first because otherwise it would re-allow the style lints that happen to come before it once sorted.
The default priority for any key in this table is 0, so setting this to -1 means it is ordered before the rest of the keys.
|
Ooh just saw this one — really concerned that we're going to miss workspace = true in new crates. How hard would it be to address this before landing? We already have the lint which checks that dependencies are specified with workspace = true, can that be expanded? |
|
Probably! |
This moves our Clippy configuration out of xtask and into Cargo's relatively-new lint configuration functionality. https://blog.rust-lang.org/2023/11/16/Rust-1.74.0.html#lint-configuration-through-cargo Unfortunately this requires littering `[lints] workspace = true` in every Cargo.toml, at least until implicit workspace inheritance is figured out. This adds a check to `cargo xtask check-workspace-deps` to ensure this is the case in CI. As part of this I removed the `#![allow(clippy::style)]` attributes from nexus and sled-agent, which revealed that this was overriding any style lints we re-enabled in the xtask code. One issue is that workspace crates can't override these lints yet (rust-lang/cargo#13157), but they can continue to override them with `#![allow(clippy::whatever)]` attributes.
This moves our Clippy configuration out of xtask and into Cargo's relatively-new lint configuration functionality. https://blog.rust-lang.org/2023/11/16/Rust-1.74.0.html#lint-configuration-through-cargo
This seeks to solve a similar root issue as #5714, which is "please, rust-analyzer, do not tell me about clippy lints we don't care about in CI".
Unfortunately this requires littering
[lints] workspace = truein every Cargo.toml, at least until implicit workspace inheritance is figured out, which is most of the noise of this PR. We may want a CI check to ensure that this is set in all packages.As part of this I removed the
#![allow(clippy::style)]attributes from nexus and sled-agent, which revealed that this was overriding any style lints we re-enabled in the xtask code.One issue is that workspace crates can't override these lints yet (rust-lang/cargo#13157), but they can continue to override them with
#![allow(clippy::whatever)]attributes.