warn if we find multiple clippy configs

[matthiaskrgr]

Fixes #8323

---

Please write a short comment explaining your change (or "none" for internal only changes)

changelog: warn if we find multiple clippy configs

[matthiaskrgr]

Hm, I'm actually wondering now if there are projects that have one clippy config in a root directory and another one in a subproject and whether we should warn in this case or not.. 🤔

[xFrednet]

Hm, I'm actually wondering now if there are projects that have one clippy config in a root directory and another one in a subproject and whether we should warn in this case or not.. thinking

I was also wondering about then when I saw the code. I think those configs should actually be merged. Someone might want to deny functions, macros, and names on a workspace level and have some other configuration for a project. I feel like the configs from the workspace should propagate to projects unless they are overridden 🤔

[xFrednet]

And it looks like high-five bot didn't assign anyone to this one. It looks interesting, I'll procrastinate a bit more assign my self 🙃

r? @xFrednet

[camsteffen]

IMO it's unusual and sub-optimal to continue searching for a config after finding one. Instead we should document which filename takes precedence and maybe have a way (verbose mode?) to print which config file is unused.

In general we can take inspiration from rustfmt (source).

[xFrednet]

I have no strong opinions on how workspace and project configuration files should be handled. I think there are compelling arguments for either side. One thing that speaks for only using one configuration file is that this is the current behavior (I think). In either case, we should definitely document how Clippy selects the configuration file. Thanks for pointing that out 🙃.

Also cc: @rust-lang/clippy maybe someone else has some thoughts on this 🙃

[Manishearth]

Ideally it would be nice to merge configs too. And we should probably support .config/clippy.toml.

But I'm fine with landing this as-is too

[matthiaskrgr]

I think merging configs and figuring out hierarchies probably is a whole other issue.

I think a separate --debug-configs or something like that just to print a second path into the terminal is a little overkill, but I would be find with perhaps making the warning a little less dramatic, like "Note: another config file at {} will be ignored"

[xFrednet]

I think merging configs and figuring out hierarchies probably is a whole other issue.

Agreed, I didn't expect it to be done in this PR, it's just something that came up 🙃.

You could also change to note to just clearly state: "Clippy will be using {} as a config file, others will be ignored". I think it wouldn't hurt to specify it every time. Either message is fine, the current version also works IMO. 🙃

[flip1995]

In general we can take inspiration from rustfmt (source).

I think it's a good idea to handle the Clippy config file in the same way as other config files are handled in the Rust ecosystem. I think a warning if a config file gets ignored is a nice improvement though.

[xFrednet]

Hey @matthiaskrgr, besides the discussion, have you seen my review comment at the start? Once that one is resolved, I think we can merge this PR 🙃

[matthiaskrgr]

I've tweaked the message a bit:

Using config file `{}`\nWarning: `{}` will be ignored.",`

and added some tests.

[xFrednet]

Looks good to me, thank you to the changes 🙃

@bors r+

[bors]

📌 Commit 4f639b2 has been approved by xFrednet

[bors]

⌛ Testing commit 4f639b2 with merge 57999f3...

[bors]

💔 Test failed - checks-action_test

[xFrednet]

diff of stderr:

-Using config file `$SRC_DIR/tests/ui-cargo/multiple_config_files/warn/.clippy.toml`
-Warning: `$SRC_DIR/tests/ui-cargo/multiple_config_files/warn/clippy.toml` will be ignored.
+Using config file `/?/$SRC_DIR/tests/ui-cargo/multiple_config_files/warn/.clippy.toml`
+Warning: `/?/$SRC_DIR/tests/ui-cargo/multiple_config_files/warn/clippy.toml` will be ignored. 

It seems like windows is not quite happy with the .stderr file 🤔

[matthiaskrgr]

mmmh

 stderr:
------------------------------------------
Using config file `\\?\D:\a\rust-clippy\rust-clippy\tests\ui-cargo\multiple_config_files\warn\.clippy.toml`
Warning: `\\?\D:\a\rust-clippy\rust-clippy\tests\ui-cargo\multiple_config_files\warn\clippy.toml` will be ignored.
```

[matthiaskrgr]

I canonicalized the paths now, no idea if that helps but worth a shot. 🤷

[xFrednet]

We'll give it a try 🙃

@bors r+

[bors]

📌 Commit cb758b3 has been approved by xFrednet

[bors]

⌛ Testing commit cb758b3 with merge 261f4bf...

[bors]

💔 Test failed - checks-action_test

[xFrednet]

Well, that doesn't seem to be working. We might have to restrict the tests to only one OS if the replacement doesn't work correctly 🤔

[matthiaskrgr]

:/
I'm wondering why the other tests don't have this problem.
Is there some kind of trick?

[xFrednet]

I'm not sure. Taking a deep dive into compiletest-rs is on my todo list, but so are countless other things as well ^^. I can imagine that the replacement regex tries to only target paths in compiler message and might not be ideal for this case, but I can be wrong.

[matthiaskrgr]

I tried to fiddle a bit with relative paths but couldn't work out a solution (we'd have to un-relativize the path somehow..??) so I just ignored the test on windows now :I

[xFrednet]

Alright, still looks good to me, let's see what bors thinks

@bors r+

[bors]

📌 Commit aae64e9 has been approved by xFrednet

[bors]

⌛ Testing commit aae64e9 with merge 8dc719c...

[bors]

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: xFrednet
Pushing 8dc719c to master...