Wrong suggestion for clippy::map_clone

[jgpaiva]

Summary

For this repro, I see clippy suggesting a change that leads to broken code. In fact, I'm even convinced the lint should not even match for this particular case. I believe this started happening on 1.77.
According to playground, it also repros in beta, but not in nightly, so maybe it's already fixed? If so, apologies for wasting your time, but I couldn't find other closed bug submissions for this bug.

Reproducer

I tried this code:

#![allow(dead_code)]
#![allow(clippy::needless_borrow)]

use std::sync::Arc;

pub struct S1 {
    v1: Option<S2>,
}

impl S1 {
    fn v2(&self) -> Option<Arc<String>> {
        self.v1().map(|v1| Arc::clone(&v1.v2))
    }

    fn v1(&self) -> Option<&S2> {
        match &self.v1 {
            None => None,
            Some(v) => Some(&v),
        }
    }
}

pub struct S2 {
    v2: Arc<String>,
}

I expected to see this happen:

• Maybe nothing? I don't think this lint applies in this case.

Instead, this happened:

    Checking playground v0.0.1 (/playground)
warning: you are explicitly cloning with `.map()`
  --> src/lib.rs:12:9
   |
12 |         self.v1().map(|v1| Arc::clone(&v1.v2))
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `self.v1().cloned()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_clone
   = note: `#[warn(clippy::map_clone)]` on by default

warning: `playground` (lib) generated 1 warning (run `cargo clippy --fix --lib -p playground` to apply 1 suggestion)
    Finished dev [unoptimized + debuginfo] target(s) in 0.91s

Version

rustc 1.77.0 (aedd173a2 2024-03-17)
binary: rustc
commit-hash: aedd173a2c086e558c2b66d3743b344f977621a7
commit-date: 2024-03-17
host: x86_64-apple-darwin
release: 1.77.0
LLVM version: 17.0.6

Additional Labels

@rustbot label +I-suggestion-causes-error

[jgpaiva]

oh, I think this may be a dupe of #12560

[GuillaumeGomez]

It seems to have been fixed in #12535. I'll add regression tests and then we can close it.

[jgpaiva]

@GuillaumeGomez thanks for linking the PR!

Just to make sure this doesn't go under the radar: to me, the more important problem is the fact that it recommends the wrong fix.
Here's an example using a String instead of an Arc:

#![allow(dead_code)]
#![allow(clippy::needless_borrow)]

pub struct S1 {
    v1: Option<S2>,
}

impl S1 {
    fn v2(&self) -> Option<String> {
        self.v1().map(|v1| String::clone(&v1.v2))
    }

    fn v1(&self) -> Option<&S2> {
        match &self.v1 {
            None => None,
            Some(v) => Some(&v),
        }
    }
}

pub struct S2 {
    v2: String,
}

In this case, it returns

warning: you are explicitly cloning with `.map()`
  --> src/lib.rs:10:9
   |
10 |         self.v1().map(|v1| String::clone(&v1.v2))
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `self.v1().cloned()`
   |
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_clone
   = note: `#[warn(clippy::map_clone)]` on by default

but applying the suggestion generates wrong code that doesn't compile.

I also think this is already fixed in nightly, since I can't repro it on playground nightly. But I couldn't find the corresponding issue so I'm just trying to make sure I'm not misunderstanding.

[GuillaumeGomez]

Let me add a check for this case as well.

[jgpaiva]

Thanks!

[GuillaumeGomez]

This case was fixed as well. I added it as part of #12647.

[Jarcho]

Fixed with #12282.