Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should map_clone trigger on copies of double references? #3958

Closed
Manishearth opened this issue Apr 13, 2019 · 5 comments · Fixed by #4043
Closed

Should map_clone trigger on copies of double references? #3958

Manishearth opened this issue Apr 13, 2019 · 5 comments · Fixed by #4043
Labels
C-bug Category: Clippy is not doing the correct thing S-needs-discussion Status: Needs further discussion before merging or work can be started

Comments

@Manishearth
Copy link
Member

fn main() {
    let x = Some(&"abcd");
    let y = x.map(|&p| p);
}

playpen

warning: You are using an explicit closure for cloning elements
 --> src/main.rs:3:13
  |
3 |     let y = x.map(|&p| p);
  |             ^^^^^^^^^^^^^ help: Consider calling the dedicated `cloned` method: `x.cloned()`
  |
  = note: #[warn(clippy::map_clone)] on by default
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#map_clone

So map_clone triggers even for copies, which makes sense, since cloned() works on copies. Here it specificially is triggering on copies of a double reference.

However, we also have clone_double_ref which warns about cloning &&T since that may not do what one expected.

Also .map(|&x| x) is much clearer than .cloned() in such cases, it's not at all obvious that the .cloned() is there to unwrap a reference

I feel that this is a false positive, but I can see others disagreeing. Thoughts?

h/t @hawkw

@Manishearth Manishearth added C-bug Category: Clippy is not doing the correct thing S-needs-discussion Status: Needs further discussion before merging or work can be started labels Apr 13, 2019
@oli-obk
Copy link
Contributor

oli-obk commented Apr 15, 2019

Note that there's also .copied() which would make it much clearer that there's not heavy duty code happening.

@Manishearth
Copy link
Member Author

In that case we should definitely suggest that whenever possible.

@Manishearth
Copy link
Member Author

Fixed in #3970, but we can't land that until .copied() stabilizes.

bors added a commit that referenced this issue Apr 16, 2019
Suggest .copied() instead of .cloned() in map_clone where applicable

partial fix for #3958

changelog: Improve suggestion in `map_clone` to suggest `.copied()` where applicable
@Centril
Copy link
Contributor

Centril commented Apr 28, 2019

.copied was stabilized in 1.36.0.

@Manishearth
Copy link
Member Author

Sweet, put next steps here: #4043

bors added a commit that referenced this issue Apr 28, 2019
Suggest .copied() for map_clone on iterators too

fixes #3958

changelog: Make `map_clone` suggest the newly-stable `Iterator::copied()` when applicable

r? @mikerite @matthiaskrgr
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: Clippy is not doing the correct thing S-needs-discussion Status: Needs further discussion before merging or work can be started
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants