Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Packages overriding inherited lints in Cargo.toml + adding lints #13157

Open
GilShoshan94 opened this issue Dec 12, 2023 · 4 comments
Open

Packages overriding inherited lints in Cargo.toml + adding lints #13157

GilShoshan94 opened this issue Dec 12, 2023 · 4 comments
Labels
A-lints-table Area: [lints] table C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-team-input Status: Needs input from team on whether/how to proceed.

Comments

@GilShoshan94
Copy link

GilShoshan94 commented Dec 12, 2023

Problem

Hi,

With the release of 1.74.0 and Lint configuration through Cargo I was interrested to have lints configure for my workspace and to override some of them in specific crates.

I was met with an error in rust-analyzer that stopped working with error :

cannot override workspace.lints in lints, either remove the overrides or lints.workspace = true and manually specify the lints"

I was surprised as my intuition was that it should have work, especially with the level system for each lint. Nothing in the Cargo Book mention that it would be impossible to override lints if inherited from the workspace (I looked in the manifest format and in workspace reference)

After researching a long time in various places, I finally found in the RFC 3389 (here, precisely) the confirmation that this is an hard error:

Currently, it is a hard error to mix workspace = true and lints. We could open this up in the future for the package to override lints from the workspace.

(By the way, after testing, it's not only overriding lints that is problematic, also adding new lints to the inherited lints in a crate is an hard error)

While it is still possible in lib.rs and/or in main.rs to add attributes #![allow(...lints...)] to override lints, I think it should still be added, as with the level system, it seems clear who override who. For me the main reason I prefered to put my "global/crate wide" lints in the cargo.toml manifest, was to get a clear view of wich crate in my workspace override what lint without touching to lib.rs/main.rs

For example we could have a workspace where in general, we wish to deny unsafe code but we could have one very low level crate that override it because we absolutly need it.

Example:

# Cargo.toml of workspace

[workspace.lints.rust]
unsafe_code = { level = "deny", priority = -1 }

[workspace.lints.clippy]
all = { level = "deny", priority = -1 }
cargo = { level = "deny", priority = -1 }
pedantic = { level = "deny", priority = -1 }
too_many_lines = "allow"
# Cargo.toml of a member crate

[lints]
workspace = true

[lints.rust]
unsafe_code = "allow"

Proposed Solution

I see 2 solutions:

  1. We can override and/or add lints in crates that inherited from workspace as I explained above.
  2. We leave the lints in cargo.toml as it is now but we document in the Cargo Book (here and here) the interdiction to add or/and override if lints.workspace = true. And we explain that the idiomatic way to do is to use the attributes #![allow(...lints...), deny(....), ....] in lib.rs/main.rs

I personally prefer solution 1) as it is more intuitive and the level system permits this.

Notes

No response

@GilShoshan94 GilShoshan94 added C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-triage Status: This issue is waiting on initial triage. labels Dec 12, 2023
@epage epage added A-lints-table Area: [lints] table S-needs-team-input Status: Needs input from team on whether/how to proceed. and removed S-triage Status: This issue is waiting on initial triage. labels Dec 12, 2023
@epage
Copy link
Contributor

epage commented Dec 12, 2023

Huh, for some reason I thought we had an issue for this but I'm not seeing it.

Something we'd have to decide is whether we treat this as lints.rust.unsafe_code.extend(workspace.lints.rust.unsafe_code) or whether we sort workspace lints and then sort package lints.

iliana added a commit to oxidecomputer/omicron that referenced this issue May 8, 2024
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.
hawkw pushed a commit to oxidecomputer/omicron that referenced this issue May 9, 2024
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.
@pcone
Copy link

pcone commented Sep 16, 2024

There's a few lints where being able to do this is fairly critical for some use cases.

If I have written a wrapper for some third party/std lib function 'foo' to hook in some extra logic/validation and I want to ensure that all of my code uses the wrapper and not the original version of the function the 'disallowed_' lints are useful, but I need to be able to suppress the warning/error in the implementation of the wrapper, otherwise I'll forever be stuck with 1 error/warning that wont go away.

I'm currently working around this issue by putting just the code I need to ignore the warning for in a separate crate, and giving it a clippy.toml file with an empty disallowed-types array, but it would be nice to not need that and be able to just share the workspace level lints and clippy.toml config and override in source instead.

@epage
Copy link
Contributor

epage commented Sep 16, 2024

@pcone is there a reason you can't just put an #[allow(...)] on that one use?

We actually have a similar situation in Cargo where std::env::var access is banned except for our wrapper (and some odd cases) and that is what we do.

@pcone
Copy link

pcone commented Sep 17, 2024

@pcone is there a reason you can't just put an #[allow(...)] on that one use?

We actually have a similar situation in Cargo where std::env::var access is banned except for our wrapper (and some odd cases) and that is what we do.

Ah you're quite right. This was a case of me not reading the docs. I assumed 'warn' and 'forbid' were the same as 'warn' and 'error' in other languages - I didn't catch that there's actually warn, force-warn, deny and forbid and that you're not allowed to override force-warn and forbid. I went down a rabbit hole of searching and came to this issue which made me think it was related.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-lints-table Area: [lints] table C-feature-request Category: proposal for a feature. Before PR, ping rust-lang/cargo if this is not `Feature accepted` S-needs-team-input Status: Needs input from team on whether/how to proceed.
Projects
None yet
Development

No branches or pull requests

3 participants