needless_pass_by_ref_mut does not honor the avoid_breaking_exported_api config

[flip1995]

Summary

The config value avoid_breaking_exported_api configuration is documented as:

avoid-breaking-exported-api: bool(defaults to true): Suppress lints whenever the suggested change would cause breakage for other crates.

However, this lint just produces a warning message that this lint breaks API instead of suppressing it.

Bonus issue: This config argument is not documented with the lint. The lint name has to be added to the macro in conf.rs.

cc @GuillaumeGomez

Lint Name

needless_pass_by_ref_mut

Reproducer

I tried this code: Playground

pub fn foo(s: &mut Vec<u32>, b: &u32, x: &mut u32) {
    *x += *b + s.len() as u32;
}

I saw this happen:

warning: this argument is a mutable reference, but not used mutably
 --> src/lib.rs:1:15
  |
1 | pub fn foo(s: &mut Vec<u32>, b: &u32, x: &mut u32) {
  |               ^^^^^^^^^^^^^ help: consider changing to: `&Vec<u32>`
  |
  = warning: changing this function will impact semver compatibility

I expected to see this happen: no lint emission.

Version

rustc 1.73.0-nightly (07438b092 2023-08-16)
binary: rustc
commit-hash: 07438b0928c6691d6ee734a5a77823ec143be94d
commit-date: 2023-08-16
host: x86_64-unknown-linux-gnu
release: 1.73.0-nightly
LLVM version: 17.0.0

Additional Labels

No response

[GuillaumeGomez]

It is as intended. It's something that we discussed in the original PR iirc and finally we decided to go with this implementation because even if it breaks API, it might still want something the users might want to update (maybe they did a code refactoring and missed that?). So in this case, we simply show a label to let the users decide what they want to do: ignore the lint in this location or fix it and then update crate version accordingly.

[flip1995]

Clippy should never suggest breaking public API. That is the point of this config value. If you should release a new version of your crate, you should temporarily set this config value to false, fix all the API lints that start triggering and after releasing a new version, you should remove it again.

The user should never want to change public API, except when releasing a new version. So Clippy lints on public API are considered noise that you almost always have to allow. Something that you almost always have to allow can also be considered a false positive.

Using this config value in this lint differently than in all other lints is inconsistent as well.

[GuillaumeGomez]

There are a few problems with your position imo:

1. It requires to know that there is config value that you need to turn on in order to fully enjoy this lint (and more clippy lints more globally). Which even when you know about it isn't easy to find, so just imagine people who don't (I discovered it thanks to this lint actually, I had no idea it existed before that).
2. I did have the use for this lint to be used on public crate and was very happy to discover some corner cases. In the cases it was expected, I allowed the lint in these places and that was it.
3. This lint is voluntarily not machine applicable and therefore will never breaks API automatically. It requires someone to actually do the changes suggested.

[flip1995]

1. It requires to know that there is config value that you need to turn on in order to fully enjoy this lint (and more clippy lints more globally)

Yes, this is a more global issue and we should improve our documentation, so that things like this are easier to discover. And this is actively being worked on.

However, that doesn't excuse being inconsistent with this config value across lints and make the config value behave differently than documented.

2. In the cases it was expected, I allowed the lint in these places and that was it.

The config value exists in the first place, because people complained that they don't want to do this. Because more often than not, allowing the lint is the only way to address a lint suggesting to break public API.

3. This lint is voluntarily not machine applicable and therefore will never breaks API automatically.

That is besides the point. Suggesting people to do that might already make them apply the lint without giving it a second thought. That is the problem. IIRC this config value is even from a time where we didn't even care about Applicability.

IMO this lint should be MachineApplicable, if there are no technical reasons against it, as long as the config option behaves as documented.

[GuillaumeGomez]

Yes, this is a more global issue and we should improve our documentation, so that things like this are easier to discover. And this is actively being worked on.

I think I'm on the minority side on this one then: I disable this config on every new project I make because I want to see everything I might have missed. Hiding such valuable information by default is really problematic. But people from the opposite will have pretty much the same argument, so not much to debate here sadly...

However, that doesn't excuse being inconsistent with this config value across lints and make the config value behave differently than documented.

Sadly yes. The lint will become mostly useless but if that's what's wanted, I'll update the lint to be coherent.

IMO this lint should be MachineApplicable, if there are no technical reasons against it, as long as the config option behaves as documented.

I'm not sure it's a good idea but maybe I'm just overthinking it...

[Centri3]

I'm not sure it's a good idea but maybe I'm just overthinking it...

I'm in favor of this once the current version (fixing async blocks) has been in beta for a while. If anything suddenly goes awry, we can revert it back to MaybeIncorrect before it reaches stable (or even beta).

[flip1995]

I have a pretty strong opinion on this, but I'd be happy to discuss this in the next Clippy meeting.

[GuillaumeGomez]

Please ping me when the discussion happens (so I can at least follow it).

[flip1995]

As agreed in the meeting I'll change the behavior to not lint at all, if the config is set.

[GuillaumeGomez]

Thanks!