Add lint_groups_priority lint

[Alexendoo]

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

• cargo clippy not obeying [lints.clippy] from Cargo.toml, but only for some lints #11237
• doc-valid-idents not respected in Cargo.toml [lints] table #11751
• Override clippy::pedantic in Cargo.toml #11830

changelog: Add [lint_groups_priority] lint

r? @flip1995

[flip1995]

Ugh, I really dislike re-parsing the Cargo.toml file. Especially something as complex as the lint table. This should be extended in the future AFAIK. Keeping up with this might be hard. How much we have to keep up with it depends on how temporary this temporary lint is.

Is there the possibility to forward this through cargo-metadata somehow? But then again, calling cargo-metadata on a enabled-by-default lint can have a huge perf impact, making it not really an option... But then again again, having this lint allow-by-default would be pretty useless.¹

The problem with the "temporary" part that I see is: cargo doesn't have the knowledge of what is and isn't a lint group. So moving this lint/warning to cargo seems unrealistic in the near future.

I acknowledge that this is an actual problem with the lint table and that we have to do something about this though.

@epage Asking for your guidance here: Do you have better ideas on how to address this issue in the short-term?

Footnotes

1. I think there were plans to put the Cargo.toml parsing code in a crate and publish it on crates.io. But I also don't think this will happen any time soon(?) ↩

[epage]

rust-lang/cargo#12918 is our issue for this and it links to the future possibility from the RFC for ways that cargo could solve this

[Alexendoo]

Yeah redefining the lint table isn't ideal, it's defined slightly lax by having [lints.rust] and [lints.clippy] hardcoded, so the addition of other keys that don't correspond to tools in [lints] wouldn't break the lint

It does give nicer diagnostics than the cargo lints that read from cargo metadata though

[flip1995]

@epage I saw that linked in the PR description. My question was more about your expectations on when this will be addressed in cargo. Re-parsing parts of the Cargo.toml file in Clippy is really not ideal. So for me to agree to merge this "temporarily" requires one of two things:

1. Some approximate timeline on when cargo wants to address this issue; or
2. Share the Cargo.toml parsing code between Clippy and cargo

The second one would be a long term solution and we could keep this lint permanently in Clippy. But achieving that also seems unrealistic in the short term.

[epage]

For 1, that is too large of an effort for me to slip in among the other work I'm trying to finish up.

For 2, making the schema available as a package is part of that work I'm trying to finish up so crates.io can use it. It won't have any of the other logic in it like filling in of inferred fields or inheriting from workspaces.

[flip1995]

For 2, making the schema available as a package is part of that work I'm trying to finish up so crates.io can use it.

That's great to hear! In that case, IMO we can merge this now and rework it with the package later. (Maybe together with the other cargo lints we have)

@epage Last request: Can I ask you or someone else from the cargo team to review the toml-parsing code in this PR? I don't want to miss something important there.

[epage]

Ill have to check check_table later. As for the serde types, they are more loosely defined which is fine.

Note that this doesn't help with rustdoc.

[epage]

Sorry, lost track of finishing my look at this

[flip1995]

I saw Ed point out the cargo_utils_schema crate here: #10306 (comment)

Can the lint implementation be updated with this, instead of implementing the parsing logic yourself?

https://docs.rs/cargo-util-schemas/latest/cargo_util_schemas/manifest/struct.TomlManifest.html

[Alexendoo]

I don't think we would be able to get span information from cargo_utils_schema, so we could do it using that but we wouldn't be able to produce nice error messages or have an edit suggestion

[epage]

The straightforward way of getting spans for a toml file is to define serde types with fields wrapped by serde_spanned. This is invasive enough that I'm trying to avoid it for cargo-util-schema.

In cargo, we will be experimenting with a less straightforward approach. You can define a serde deserializer that accepts a path to a field and it will error when it gets to that path. You can then get the span from the error.

[flip1995]

Alright, I don't want to block this any longer. Sorry for the long time it took me to get back to this. I just have one clarifying question, then this is ready to go.

[flip1995]

Does this matter? I think that you can pass in -Dwarnings anywhere and it won't change the lint level of any other lint you set before/after, as warnings is not actually a group but some special handled keyword/lint.

[Alexendoo]

Yeah looks like it doesn't matter, removed it 👍

[flip1995]

@bors r+

Thanks! Also a big thanks to Ed for helping with the review!

[bors]

📌 Commit 6619e8c has been approved by flip1995

It is now in the queue for this repository.

[bors]

⌛ Testing commit 6619e8c with merge b58b88c...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: flip1995
Pushing b58b88c to master...