rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.8k stars 2.42k forks source link

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

Open GilShoshan94 opened 11 months ago

GilShoshan94 commented 11 months ago

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

epage commented 11 months ago

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.

pcone commented 2 months ago

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 commented 2 months ago

@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 commented 2 months ago

@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.