overhaul `mut_mut` #15417

ada4a · 2025-08-05T12:41:45Z

This PR:

adds structured suggestions to all 3 lint scenarios
adds more examples of linted patterns, and recommended fixes
moves some test cases to _unfixable.rs, to allow running rustfix on the main file
stops the lint from firing multiple times on types like &mut &mut &mut T

Open questions:

I'd like to add a note explaining why chained &mut are useless.. but can't really come up with something consice. I very much don't like the one in the docs -- it's a bit too condescending imo.
see comments in the diff for more

I do realize that the PR ended up being quite wide-scoped, but that's primarily because once I added structured suggestions to one of the lint cases, ui_test started complaining about warnings remaining after the rustfix run, which of course had to with the fact that the other two lint cases didn't actually fix the code they linted, only warned. I guess ui_test could be smarter about this, by understanding that if a warning was created without a suggestion, then that warning will still remain in the fixed file. But oh well.

changelog: [mut_mut]: add structured suggestions, improve docs

fixes #13021

rustbot · 2025-08-05T12:41:50Z

r? @samueltardieu

rustbot has assigned @samueltardieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

ada4a · 2025-08-05T12:43:28Z

tests/ui/mut_mut.fixed

+        //~| mut_mut
+    }
+
+    let mut z = inline!(&mut $(&mut 3u32));


I stopped the lint from firing when inside macros, because the $(&mut 3u32) thing caused a bad suggestion:

&mut 3u32mut $(&mut u32)

should I try bringing the suggestion back, and if so, how?

ada4a · 2025-08-05T12:44:58Z

clippy_lints/src/mut_mut.rs

-                    "this expression mutably borrows a mutable reference. Consider reborrowing",
+                    "this expression mutably borrows a mutable reference",
+                    |diag| {
+                        diag.span_suggestion(expr.span, "reborrow instead", sugg, applicability);


From my experience, not many people know about reborrowing -- maybe the message for the case that recommends it should link to some page that explains it?

Admittedly this is less necessary now that we've got a structured suggestion; people can just search for the term separately if they're interested

An important property of Clippy is that we have an unique position and people learn from Clippy's output. So, they (ideally) come to Clippy with a learner mindset.

With the suggestion, they'll assume that adding &* is called "reborrowing"

ada4a · 2025-08-05T12:45:46Z

clippy_lints/src/mut_mut.rs

                    expr.hir_id,
                    expr.span,
-                    "generally you want to avoid `&mut &mut _` if possible",
+                    "an expression of form `&mut &mut _`",


is this message helpful enough?

Yes, I'd say that its helpful enough. What do you have in mind?

Well it now just states the fact, not really explaining why having double &muts is bad. But I also can't come up with a concise way to explain why it's bad, probably because I can't really imagine how one would end up with such code – unlike the "&mut expr where expr itself has a type &mut T" case, which can happen because you forget the type of expr

github-actions · 2025-08-05T12:51:18Z

Lintcheck changes for dc534c4

Lint	Added	Removed	Changed
`clippy::mut_mut`	0	0	11

This comment will be updated if you push new changes

ada4a · 2025-08-05T12:59:33Z

@cmrschwarz let me know what you think! You can see the change in the diagnostic messages in the .stderr files

cmrschwarz · 2025-08-05T13:05:40Z

@cmrschwarz let me know what you think! You can see the change in the diagnostic messages in the .stderr files

Just looking at the structured suggestions, this looks exactly like what I hoped for.
Thank you for working on this!

ada4a · 2025-08-12T07:58:30Z

Reviewer unavailable, so rerolling

r? clippy

ada4a · 2025-09-07T09:46:20Z

@blyxyas friendly ping in case this flew under the radar^^

blyxyas · 2025-09-07T17:21:04Z

No, this hasn't flown through the radar Currently reviewing this.

blyxyas

Great starting patch!

View changes since this review

blyxyas · 2025-09-07T17:48:50Z

clippy_lints/src/mut_mut.rs

+                loop {
+                    if !e.span.eq_ctxt(e2.span) {
+                        return;
+                    }
+                    if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, next) = e2.kind {
+                        (e, e2) = (e2, next);
+                        many_muts = true;
+                    } else {
+                        break;
+                    }
+                }


Could you extract these two blocks into a function? The !e.span.eq_ctxt(.. call can be extracted into a closure passed to the function, being that we checking for context changes in types is not really useful.

You mean in order to share the peeling logic with the types case? But wouldn't that be impossible, given that we peel ExprKinds here, but TyKinds there?

blyxyas · 2025-09-07T18:00:06Z

clippy_lints/src/mut_mut.rs

+                let (mut e, mut e2) = (e, e2);
+                let mut many_muts = false;
+                loop {
+                    if !e.span.eq_ctxt(e2.span) {


(Not a problem, just record-keeping)

One might think that it's an improvement to use SpanData instead of Span, so we don't get the SpanData twice per each span (one for e2 being the argument, and once for it being the object that the method is called on).

But being that there aren't almost any occurences of /&mut &mut (&mut)+/, it's probably better to avoid the cost of getting the two initial spans and pay the price at higher-mut sites (which aren't very frequent).

blyxyas · 2025-09-07T18:05:15Z

clippy_lints/src/mut_mut.rs

                    expr.hir_id,
                    expr.span,
-                    "generally you want to avoid `&mut &mut _` if possible",
+                    "an expression of form `&mut &mut _`",


Yes, I'd say that its helpful enough. What do you have in mind?

clippy_lints/src/mut_mut.rs

for some reason this seems to already be the case with exprs, so only do it for tys

see https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/problem.20when.20suggestion.20only.20removes.20one.20.22level.22.20of.20a.20lint/near/532889973

see https://rust-lang.zulipchat.com/#narrow/channel/25732

as recommended in https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/problem.20when.20suggestion.20only.20removes.20one.20.22level.22.20of.20a.20lint/near/532894687

…ending

rustbot · 2025-09-28T10:12:11Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

blyxyas

LGTM, thanks! ❤️

View changes since this review

blyxyas · 2025-09-29T21:19:26Z

clippy_lints/src/mut_mut.rs

-                    "this expression mutably borrows a mutable reference. Consider reborrowing",
+                    "this expression mutably borrows a mutable reference",
+                    |diag| {
+                        diag.span_suggestion(expr.span, "reborrow instead", sugg, applicability);


An important property of Clippy is that we have an unique position and people learn from Clippy's output. So, they (ideally) come to Clippy with a learner mindset.

With the suggestion, they'll assume that adding &* is called "reborrowing"

rustbot assigned samueltardieu Aug 5, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 5, 2025

ada4a commented Aug 5, 2025

View reviewed changes

ada4a force-pushed the mut-mut branch 2 times, most recently from a81c9a6 to c26eb82 Compare August 5, 2025 12:57

ada4a force-pushed the mut-mut branch from c26eb82 to 58b7dfa Compare August 5, 2025 13:04

ada4a mentioned this pull request Aug 5, 2025

WIP: mut_mut: generalize the from-expansion check #15419

Closed

ada4a force-pushed the mut-mut branch from 58b7dfa to dcb123e Compare August 7, 2025 21:55

rustbot assigned blyxyas and unassigned samueltardieu Aug 12, 2025

ada4a force-pushed the mut-mut branch from dcb123e to 915f4cf Compare August 22, 2025 13:38

blyxyas requested changes Sep 11, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 11, 2025

This comment has been minimized.

Sign in to view

ada4a added 7 commits September 28, 2025 12:11

move unfixable stuff to a separate file

bb1081e

sugg on expr 1

a4fba85

sugg on expr 2

70be286

fix: don't lint in macro calls

269870a

sugg on ty

729d92c

only lint once on 3+ &muts

c6971c2

for some reason this seems to already be the case with exprs, so only do it for tys

misc: import more from hir

4053c44

ada4a added 4 commits September 28, 2025 12:11

peel _all_ extra &muts

9a1b2bc

see https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/problem.20when.20suggestion.20only.20removes.20one.20.22level.22.20of.20a.20lint/near/532889973

remember all the nested types while we loop

5546806

see https://rust-lang.zulipchat.com/#narrow/channel/25732

add an unfixable case

75b6860

as recommended in https://rust-lang.zulipchat.com/#narrow/channel/257328-clippy/topic/problem.20when.20suggestion.20only.20removes.20one.20.22level.22.20of.20a.20lint/near/532894687

doc: add more examples, and a "Use instead" section, don't be condesc…

dc534c4

…ending

ada4a force-pushed the mut-mut branch from 915f4cf to dc534c4 Compare September 28, 2025 10:12

blyxyas approved these changes Sep 29, 2025

View reviewed changes

blyxyas added this pull request to the merge queue Sep 29, 2025

Merged via the queue into rust-lang:master with commit 98a92bf Sep 29, 2025
11 checks passed

ada4a deleted the mut-mut branch September 29, 2025 21:32

overhaul mut_mut #15417

overhaul mut_mut #15417

Uh oh!

Conversation

ada4a commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 5, 2025

Uh oh!

ada4a Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ada4a Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ada4a commented Aug 5, 2025

Uh oh!

cmrschwarz commented Aug 5, 2025

Uh oh!

ada4a commented Aug 12, 2025

Uh oh!

ada4a commented Sep 7, 2025

Uh oh!

blyxyas commented Sep 7, 2025

Uh oh!

blyxyas left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

rustbot commented Sep 28, 2025

Uh oh!

blyxyas left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

overhaul `mut_mut` #15417

overhaul `mut_mut` #15417

ada4a commented Aug 5, 2025 •

edited

Loading

ada4a Aug 5, 2025 •

edited

Loading

ada4a Aug 5, 2025 •

edited

Loading

github-actions bot commented Aug 5, 2025 •

edited

Loading

blyxyas left a comment •

edited by rustbot

Loading

blyxyas left a comment •

edited by rustbot

Loading