Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Override clippy::pedantic in Cargo.toml #11830

Closed
davidhewitt opened this issue Nov 17, 2023 · 5 comments
Closed

Override clippy::pedantic in Cargo.toml #11830

davidhewitt opened this issue Nov 17, 2023 · 5 comments

Comments

@davidhewitt
Copy link

Description

I'm testing the new [lints] functionality in Cargo.toml, which I'm very excited to have on stable!

I've just tried the following Cargo.toml configuration:

[lints.clippy]
pedantic = "warn"
similar_names = "allow"
# and a number of other "allow" lints from the pedantic category

The idea is that I want to lint against the whole pedantic category but then whitelist specific lints from that category which I'm ok with not complying with.

However, this doesn't work, the simliar_names lint is still reported:

warning: binding's name is too similar to existing binding
   --> src/validators/union.rs:353:17
    |
353 |             let tag_repr = choice_key.repr()?.to_string();
    |                 ^^^^^^^^
    |
note: existing binding defined here
   --> src/validators/union.rs:344:17
    |
344 |         let mut tags_repr = String::with_capacity(50);
    |                 ^^^^^^^^^
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#similar_names
    = note: `-W clippy::similar-names` implied by `-W clippy::pedantic`
    = help: to override `-W clippy::pedantic` add `#[allow(clippy::similar_names)]`

I believe this is because the order which Cargo has passed the flags to clippy on the command line has placed the allow lints before the warn.

 Running `/home/david/.rustup/toolchains/stable-x86_64-unknown-linux-gnu/bin/clippy-driver [...] '--allow=clippy::similar_names' '--warn=clippy::pedantic' [...]

One fix I can imagine is that clippy should prioritize individual lint flags over their category flags, ignoring the command line order for this case, but this is probably a (minor) breaking change, so there would need to be discussion on whether it's valid to do so.

Version

rustc 1.74.0 (79e9716c9 2023-11-13)
binary: rustc
commit-hash: 79e9716c980570bfd1f666e3b16ac583f0168962
commit-date: 2023-11-13
host: x86_64-unknown-linux-gnu
release: 1.74.0
LLVM version: 17.0.4

Additional Labels

@rustbot-label +C-enhancement

@y21
Copy link
Member

y21 commented Nov 18, 2023

You should be able to override pedantic either by giving that a lower priority (eg. -1) or set a higher priority for individual lints, eg. similar_names = { level = "allow", priority = 1 }. Having multiple lint entries of the same category with the same implicit priority seems like it should be a warning since the order is arbitrary/unspecified...
(EDIT: ooops, there's already a PR that implements a lint for this, missed that 😅)

@kristof-mattei
Copy link
Contributor

You should be able to override pedantic either by giving that a lower priority (eg. -1) or set a higher priority for individual lints, eg. similar_names = { level = "allow", priority = 1 }. Having multiple lint entries of the same category with the same implicit priority seems like it should be a warning since the order is arbitrary/unspecified... (EDIT: ooops, there's already a PR that implements a lint for this, missed that 😅)

Rust itself works fine with this one:

[lints.rust]
warnings = "deny"
dead-code = "allow"

Seems like rust itself has a ruleset that individual lint settings stump the group's setting.

Can't we adopt this in Clippy? That way we don't have to deal with the priority for simpler cases.

@nyurik
Copy link
Contributor

nyurik commented Nov 20, 2023

I think a better interim solution is to always use pedantic = { level = "warn", priority = -1 } -- this way all the specific ones will have higher priority. Seems to work ok. I am surprised other lint overrides still work though - probably due to sorting.

@y21
Copy link
Member

y21 commented Nov 20, 2023

@kristof-mattei warnings, how I understand it, isn't a group and is somewhat special/has different rules (edit: apparently it's listed as a group, I'm not entirely sure what it's doing differently, edit 2: looks like this specific entry is hardcoded in the code that generates the table and is not actually a group: source).

If I try this with a different group, then this also fails for rustc much in the same way:

[lints.rust]
unused = "deny"
unused_variables = "allow"
error: unused variable: `x`
  --> src/main.rs:16:9
   |
16 |     let x = 1;
   |         ^ help: if this is intentional, prefix it with an underscore: `_x`

If I understand the RFC correctly, the order in which lint cap args are passed to rustc within the same group are unspecified and cargo might just happen to sort them in reverse lexicographic order right now (unused_variables > unused)

@davidhewitt
Copy link
Author

pedantic = { level = "warn", priority = -1 } works great, thanks @y21 @nyurik - I'd completely missed that priority was available as an extended option.

bors added a commit that referenced this issue Jan 31, 2024
Add `lint_groups_priority` lint

Warns when a lint group in Cargo.toml's `[lints]` section shares the same priority as a lint. This is in the cargo section but is categorised as `correctness` so it's on by default, it doesn't call `cargo metadata` though and parses the `Cargo.toml` directly

The lint should be temporary until rust-lang/cargo#12918 is resolved, but in the meanwhile this is an common issue to run into

- #11237
- #11751
- #11830

changelog: Add [`lint_groups_priority`] lint

r? `@flip1995`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants