Allow multiple clippy.tomls and make them inherit from lower priority ones

[Centri3]

This is a bit of a pain to test, but from my own testing it seems to work 🙂
Just one issue I know currently, see https://github.com/Centri3/rust-clippy/blob/560b6b7122f8de1295c4bfe66c8e7d314ae868b1/clippy_lints/src/utils/conf.rs#L571-L574

Closes #7353

changelog: Enhancement: clippy.toml can now be modified on a per-package basis

[rustbot]

r? @xFrednet

(rustbot has picked a reviewer for you, use r? to override)

[xFrednet]

Thank you for the PR, I would like to first discuss this feature in the next meeting before I do a full review. :)

@rustbot label +I-nominated

[bors]

☔ The latest upstream changes (presumably #10916) made this pull request unmergeable. Please resolve the merge conflicts.

[Centri3]

I've made the new behavior opt-in through the workspace key. It can only be defined in the root config, i.e., the one with the lowest priority, otherwise it will fail to parse

I think that was the only issue before.

[xFrednet]

I'll be on vacation for most of the week. I'll give this a proper review next week.

cc: @blyxyas If you want you can review this PR :)

[blyxyas]

Yeah, will review it (it will take some time though, it's a big PR), thanks for trying such a big project ❤️

[blyxyas]

As far as I can tell, it's a very good starting point. We should discuss a little bit some specifics. I think we should also include documentation about this feature in this same PR (or create a new one that gets merged before the rustc sync)

@rustbot label: +I-nominated

[blyxyas]

Extending this comment with the reason why an error will be emitted if uncommented would be very helpful (not having priority 0 AFAIK)

[blyxyas]

Could you add a directory at the same level than inner with a different clippy.toml, to show that it should have the same inherited configs, but different local ones?

[blyxyas]

I'm not sure this option should be named workspace, maybe a little bit confusing with Cargo workspaces. I think we can discuss about this in the next meeting.

[Centri3]

Yeah I don't like the name all too much either, but that was the only thing that came to mind

[xFrednet]

Maybe something like inherit_... or extend_... could work? These are also not totally convincing. Sometimes I stumble upon a good name while writing documentation, just based on which words I'm using there :)

[Centri3]

I was thinking something like inherit = true but this feels odd, as inherit usually means "this is the path to the thing I'm inheriting from". extend could work maybe. Maybe inheritable or extendable, I don't know. Naming things is often the hardest part of this 😅

[bors]

☔ The latest upstream changes (presumably #10426) made this pull request unmergeable. Please resolve the merge conflicts.

[blyxyas]

Oh this is assigned on me, I forgot 💀.
@Centri3, could you fix the merge conflicts before another review?

[blyxyas]

Ok, just as a reminder of what was said about this in the 2023-06-27 meeting:

• The option is in the parent clippy.toml
• The final name is inheritable
• If a key appears in two clippy.toml, the child key will override the parent.

[Centri3]

Last I checked it was still pretty indecisive, so are we sure we're going with the top-level clippy.toml now?

[blyxyas]

It was the winning option in the poll by 1 vote. (Maybe there has been another poll I haven't seen, but I think that's the conclusion (?))

[flip1995]

Now that I had the chance to think about this more, I also prefer the child level inherit.

• It is immediately clear from the clippy.toml file you're currently looking at, that there are more configs
• Building tree-like structures of config files is easier/better "definable": What if you want to have a top-level clippy.toml that should be inherited by all but one crate in your workspace. And that crate wants to start a new config and copy nothing. If the top-level sets inheritable=true is it then possible for the crate to overwrite that config as a whole or does it have to do that value-by-value? With an inherit in the child configs this can just be done by leaving out the inherit.
• There seems to be prior-art with ts-config that also does that on a leaf level.
• In cargo you also have to explicitly inherit.

This brings the vote to a tie and doesn't help anyone 😅 So I think there is more room for discussion...

[Centri3]

In cargo you also have to explicitly inherit.

Not in the case of config.toml, iirc that's implicit. In the case of unstable features, you must specify them in the parent Cargo.toml which is what this feels most similar to.

The rest are great though, making it explicit in each child would make it more powerful (but also harder to implement ^^)

[flip1995]

I was more thinking about workspace attributes of crates, like authors = ["bla"] in the top level Cargo.toml and authors.workspace = true in the crates.

[Centri3]

Yeah I know, though I was pointing out that it doesn't really apply to cargo's own config (which is much similar to clippy.toml than workspace attributes). Inheriting the workspace also makes sense since you generally don't want to do it on every key, rather than here where you would usually want to.

[blyxyas]

It is immediately clear from the clippy.toml file you're currently looking at, that there are more configs

Mmmm... This is pretty correct. Maybe the user is already aware that they're in a workspace by the [workspace] table and the file structure that inheritable could be redundant.

[blyxyas]

Just to be clear, I also agree that it should be in the child, moving my vote from parent to child. Meaning that the propierty should be inherits or something similar and it should be in the child member rather than the root.

[xFrednet]

I'm also in favor of declaring it in the child 👍

[blyxyas]

235 days since this was open, I think that we can safely declare this as stale. I'll close it, it can always be restarted! ヾ(=｀ω´=)ノ”

[xFrednet]

Good call!

Tiny nit: In these cases, it's always a good idea, to also add the S-inactive-closed label :D

Meow Meow =°w°=