Honor `avoid-breaking-exported-api` in `needless_pass_by_ref_mut`

[flip1995]

Until now, the lint only emitted a warning, when breaking public API. Now it doesn't lint at all when the config value is not set to false, bringing it in line with the other lints using this config value.

Also ensures that this config value is documented in the lint.

changelog: none (I don't think a changelog is necessary, since this lint is in nursery)

---

Fixes https://github.com/rust-lang/rust-clippy/issues/11374

cc @GuillaumeGomez

Marking as draft: Does this lint even break public API? If I change a function signature from fn foo(x: &mut T) to fn foo(x: &T), I can still call it with foo(&mut x). The only "breaking" thing is that the clippy::unnecessary_mut_passed lint will complain that &mut at the callsite is not necessary, possibly trickling down to the crate user having to remote a mut from a variable. Playground.

Are there examples where this actually breaks public API, that I'm missing?

[rustbot]

r? @Centri3

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

[GuillaumeGomez]

Marking as draft: Does this lint even break public API? If I change a function signature from fn foo(x: &mut T) to fn foo(x: &T), I can still call it with foo(&mut x). The only "breaking" thing is that the clippy::unnecessary_mut_passed lint will complain that &mut at the callsite is not necessary, possibly trickling down to the crate user having to remote a mut from a variable. Playground.

Are there examples where this actually breaks public API, that I'm missing?

That's actually a good question... The API changed, so I suppose it's breaking semver but it doesn't break it for the API users. Interesting.

[Centri3]

The function will no longer be FnMut but Fn, or smth like that, so it can break API. It's probably pretty rare though and this is why I was in favor of linting anyway

Don't have any examples of this but it's probably possible when passing the function to some iterator methods, like map or filter.

[flip1995]

The function will no longer be FnMut but Fn

Huh, isn't this only about what the function captures? But I think you're hinting to the right thing: You won't be able to pass the function as an argument to another function, expecting a Fn/FnOnce/FnMut with a &mut argument, instead of a & argument: Playground

I will run lintcheck for this lint (modified to ONLY lint public items). Let's look at some data.

[flip1995]

So lintcheck gave some interesting discoveries: Passing the function is only a semi-issue: You can work around that by doing something like bar(|x| foo(x)) instead of bar(foo). But that makes the code on the callsite longer. The h2 crate has an almost-use-case for this together with the futures_util::poll_fn.

Further than that, the lock_api crated showed where the &mut has a different purpose than just the mutability: To ensure that only a single reference exists at the time of calling some of the functions.

Here are the results:

clippy 0.1.75 (5ec94d2043a 2023-10-09)

Reports

target/lintcheck/sources/h2-0.3.21/src/client.rs:1431:13 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/h2-0.3.21/src/share.rs:422:37 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/mutex.rs:558:30 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/mutex.rs:611:35 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/mutex.rs:629:20 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/remutex.rs:678:30 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/remutex.rs:722:35 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/remutex.rs:740:20 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/rwlock.rs:1255:30 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/rwlock.rs:1297:35 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/rwlock.rs:1315:20 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/rwlock.rs:1545:30 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/rwlock.rs:1629:35 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/rwlock.rs:1647:20 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/rwlock.rs:1890:30 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/rwlock.rs:1964:35 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/lock_api-0.4.10/src/rwlock.rs:1982:20 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/nom-7.1.3/src/multi/mod.rs:625:40 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/parking_lot-0.12.1/src/condvar.rs:255:48 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/parking_lot-0.12.1/src/condvar.rs:285:22 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/parking_lot-0.12.1/src/condvar.rs:381:22 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/socket2-0.5.4/src/lib.rs:683:38 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"
target/lintcheck/sources/want-0.3.1/src/lib.rs:192:37 clippy::needless_pass_by_ref_mut "this argument is a mutable reference, but not used mutably"

Stats:

lint	count
clippy::needless_pass_by_ref_mut	23

---

In conclusion, I would say that we don't want to break public API, because passing a function with &mut arguments is definitely a use case. Also the shared-unique vs immutable-mutable difference probably matters more on public API than private API, even though I see this as another big issue, where it might even be impossible for Clippy to tell when what the intention behind the &mut is. But we can work around this with applicability, lint group and a note on the lint emission.

[Centri3]

I'm not sure whether that's actually possible by just changing the signature, since yeah that is based on what it captures. It might just be that any function implements Fn + FnMut + FnOnce no matter what.

What you showed is a larger concern though which I forgot about and is already checked for by the lint for cases of this in the current crate. This might be rare enough that we can just always give a note on public items, I can't really think of any non-contrived example where somebody passes a function pointer to smth expecting Fn with those specific parameters (rather than T). Also a mutable reference to one of those parameters.

[flip1995]

is already checked for by the lint for cases of this in the current crate.

Ah right, I forgot about that. This is good, but on public items we can't tell. So we should definitely emit a warning there, document it in the lint documentation and apply the exported API config correctly IMO.

I can't really think of any non-contrived example where somebody passes a function pointer to smth expecting Fn with those specific parameters

See my comment above with an example from the futures_util crate.

[Centri3]

Further than that, the lock_api crated showed where the &mut has a different purpose than just the mutability: To ensure that only a single reference exists at the time of calling some of the functions.

This would not be linted as it has an unsafe block. #11624

If it's passed to another function that's both "safe" and takes &, that's unsound, even if it's encapsulated. So I think this is a non-issue. If there's no unsafe it's safe to assume exclusivity is unnecessary

[flip1995]

So I think this is a non-issue. If there's no unsafe it's safe to assume exclusivity is unnecessary

You are way ahead of me :D You're right.

[flip1995]

I'll add the warning and documentation about this in this PR and then mark it as ready for review some time this week. (I'll be at EuroRust, where I might get some time for this and other Clippy TODOs I still have)

[GuillaumeGomez]

For the unsafe fn/block, we still need to wait for https://github.com/rust-lang/rust-clippy/pull/11624 to be merged.

PS: I'll be at eurorust too. :D

[bors]

:umbrella: The latest upstream changes (presumably #11622) made this pull request unmergeable. Please resolve the merge conflicts.

[flip1995]

@Centri3 rebased and ready for final review.

[bors]

:umbrella: The latest upstream changes (presumably #11685) made this pull request unmergeable. Please resolve the merge conflicts.

[Centri3]

Seems this only needs to be blessed, after that this is ready

[flip1995]

The failing test has a should not lint comment above it. And I have no idea how my changes could've changed the lint behavior here...

[bors]

:umbrella: The latest upstream changes (presumably #11879) made this pull request unmergeable. Please resolve the merge conflicts.

[flip1995]

I've spent some time investigating this.

fn bar() {}  // or a `mod bar {}` or a `use` item
fn foo() {}

async fn _f(v: &mut Vec<()>) {
    let x = || v.pop();
    _ = || || x;
}

When removing anything from that reproducer, like the functions foo OR bar, the lint no longer triggers for _f. Also when changing the function body of _f, like removing the _ = || || x; makes the lint stop triggering. Moving one of the functions to after _f also makes the lint stop triggering.

So it seems like, in order to hit this, there must be at least 2 items before _f. Changing one of the functions to a module still triggers the lint. I have no idea how my changes in this PR have any influence on the behavior. I tried moving the check for exported(_api) to right before the span_lint_hir_and_then call, to rule out that my changes skip any side effects, but to no avail.

[GuillaumeGomez]

I'll try to take a look in the next days.

[bors]

:umbrella: The latest upstream changes (presumably #12306) made this pull request unmergeable. Please resolve the merge conflicts.

[xFrednet]

Hey @GuillaumeGomez, since you've been mentioned in this PR, do you want to give it a review? (There is no time pressure, first enjoy your time in the UK)

r? @xFrednet

[GuillaumeGomez]

I completely forgot about it. ^^'

Putting it back to my to do list.

[xFrednet]

Sorry, I totally forgot about this PR, since it wasn't in my inbox anymore :see_no_evil: I'll add it to my todo list. You'll get a review next week!

[xFrednet]

I totally missed this, since it wasn't in my inbox :see_no_evil: I'll try to review it tomorrow or otherwise early next week.

Also ping @GuillaumeGomez, if you still want to help with the review

[xFrednet]

Could you also rebase the branch? :)

[flip1995]

Rebased and this weird bug: https://github.com/rust-lang/rust-clippy/pull/11647#issuecomment-1837121238 is also gone. Infinite monkey problem solving never disappoints!

[xFrednet]

LGTM, thank you for the quick update =^.^=

Now, let's give our favorite non money something to do.

---

Roses are red, Bors is a bot, Not a monkey, Just automated testing slots

[bors]

:pushpin: Commit 125c778d6d01824fde3fe96f765a4dd9341af036 has been approved by xFrednet

It is now in the queue for this repository.

[bors]

:hourglass: Testing commit 125c778d6d01824fde3fe96f765a4dd9341af036 with merge 51200cbab628c6deb3f977cfacfc4ed64836c308...

[bors]

:broken_heart: Test failed - checks-action_test