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

Consider not warning for map_clone with Arc::clone #12528

Closed
stevenengler opened this issue Mar 21, 2024 · 4 comments · Fixed by #12529
Closed

Consider not warning for map_clone with Arc::clone #12528

stevenengler opened this issue Mar 21, 2024 · 4 comments · Fixed by #12529
Assignees
Labels
C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have

Comments

@stevenengler
Copy link
Contributor

stevenengler commented Mar 21, 2024

Summary

Similar to how some people prefer to use the fully qualified Arc::clone(&foo) rather than foo.clone() for readability, people may also prefer foo.map(Arc::clone) rather than foo.cloned() to be more explicit that the Arc is being cloned. I think that the map_clone lint should not trigger for foo.map(Arc::clone) or for foo.map(Rc::clone).

This is of course subjective, but it would be a subjective choice to not show a warning, which I think is nicer.

Lint Name

map_clone

Reproducer

I tried this code:

use std::sync::Arc;

struct Foo;

fn main() {
    let x = Arc::new(Foo);
    let y = Some(&x);
    let _z = y.map(Arc::clone);
}

I saw this happen:

warning: you are explicitly cloning with `.map()`
 --> src/main.rs:8:14
  |
8 |     let _z = y.map(Arc::clone);
  |              ^^^^^^^^^^^^^^^^^ help: consider calling the dedicated `cloned` method: `y.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

I expected to see this happen:

No lint.

Version

rustc 1.79.0-nightly (1388d7a06 2024-03-20)
binary: rustc
commit-hash: 1388d7a069d872bcfe5e5dd97ef61fa0a586fac0
commit-date: 2024-03-20
host: x86_64-unknown-linux-gnu
release: 1.79.0-nightly
LLVM version: 18.1.2

Additional Labels

No response

@stevenengler stevenengler added C-bug Category: Clippy is not doing the correct thing I-false-positive Issue: The lint was triggered on code it shouldn't have labels Mar 21, 2024
@samueltardieu
Copy link
Contributor

@rustbot claim

@y21
Copy link
Member

y21 commented Mar 22, 2024

That raises the question if we should also do this for useless_asref then, which also currently lints on

opt /* : Option<Rc<_>> */
  .as_ref()
  .map(Rc::clone)

and suggests .clone() instead

@bors bors closed this as completed in 52b2a5e Mar 22, 2024
@stevenengler
Copy link
Contributor Author

stevenengler commented Mar 22, 2024

Thanks for the quick change!

@samueltardieu
Copy link
Contributor

That raises the question if we should also do this for useless_asref then, which also currently lints on

opt /* : Option<Rc<_>> */
  .as_ref()
  .map(Rc::clone)

and suggests .clone() instead

Yes, probably. I'll submit a PR to test this, after factoring the test.

bors added a commit that referenced this issue Mar 23, 2024
`useless_asref`: do not lint `.as_ref().map(Arc::clone)`

This applies to `Arc`, `Rc`, and their weak variants. Using `.clone()` would be less idiomatic.

This follows the discussion in <#12528 (comment)>.

changelog: [`useless_asref`]: do not lint `.as_ref().map(Arc::clone)` and similar
stevenengler added a commit to stevenengler/shadow that referenced this issue Jun 8, 2024
This was made unnecessary by
rust-lang/rust-clippy#12528.
stevenengler added a commit to stevenengler/shadow that referenced this issue Jun 12, 2024
This was made unnecessary by
rust-lang/rust-clippy#12528.
stevenengler added a commit to stevenengler/shadow that referenced this issue Sep 9, 2024
This was made unnecessary by
rust-lang/rust-clippy#12528.
stevenengler added a commit to stevenengler/shadow that referenced this issue Sep 9, 2024
This was made unnecessary by
rust-lang/rust-clippy#12528.
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 I-false-positive Issue: The lint was triggered on code it shouldn't have
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants