Auto merge of rust-lang#12051 - y21:option_as_ref_cloned, r=dswij

new lint: `option_as_ref_cloned`

Closes rust-lang#12009

Adds a new lint that looks for `.as_ref().cloned()` on `Option`s. That's the same as just `.clone()`-ing the option directly.

changelog: new lint: [`option_as_ref_cloned`]

Author: bors

CHANGELOG.md

@@ -5442,6 +5442,7 @@ Released 2018-09-13
 [`only_used_in_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion
 [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref
 [`option_and_then_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_and_then_some
+[`option_as_ref_cloned`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_cloned
 [`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref
 [`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap
 [`option_expect_used`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_expect_used

README.md

@@ -5,7 +5,7 @@
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are over 650 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 Lints are divided into categories, each with a default [lint level](https://doc.rust-lang.org/rustc/lints/levels.html).
 You can choose how much Clippy is supposed to ~~annoy~~ help you by changing the lint level by category.

book/src/README.md

@@ -6,7 +6,7 @@
 A collection of lints to catch common mistakes and improve your
 [Rust](https://github.com/rust-lang/rust) code.
 
-[There are over 650 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
+[There are over 700 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html)
 
 Lints are divided into categories, each with a default [lint
 level](https://doc.rust-lang.org/rustc/lints/levels.html). You can choose how

clippy_lints/src/declared_lints.rs

@@ -410,6 +410,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::methods::NO_EFFECT_REPLACE_INFO,
     crate::methods::OBFUSCATED_IF_ELSE_INFO,
     crate::methods::OK_EXPECT_INFO,
+    crate::methods::OPTION_AS_REF_CLONED_INFO,
     crate::methods::OPTION_AS_REF_DEREF_INFO,
     crate::methods::OPTION_FILTER_MAP_INFO,
     crate::methods::OPTION_MAP_OR_ERR_OK_INFO,

clippy_lints/src/methods/mod.rs

@@ -71,6 +71,7 @@ mod no_effect_replace;
 mod obfuscated_if_else;
 mod ok_expect;
 mod open_options;
+mod option_as_ref_cloned;
 mod option_as_ref_deref;
 mod option_map_or_err_ok;
 mod option_map_or_none;
@@ -3887,6 +3888,31 @@ declare_clippy_lint! {
     "splitting a trimmed string at hard-coded newlines"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `.as_ref().cloned()` and `.as_mut().cloned()` on `Option`s
+    ///
+    /// ### Why is this bad?
+    /// This can be written more concisely by cloning the `Option` directly.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// fn foo(bar: &Option<Vec<u8>>) -> Option<Vec<u8>> {
+    ///     bar.as_ref().cloned()
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// fn foo(bar: &Option<Vec<u8>>) -> Option<Vec<u8>> {
+    ///     bar.clone()
+    /// }
+    /// ```
+    #[clippy::version = "1.77.0"]
+    pub OPTION_AS_REF_CLONED,
+    pedantic,
+    "cloning an `Option` via `as_ref().cloned()`"
+}
+
 pub struct Methods {
     avoid_breaking_exported_api: bool,
     msrv: Msrv,
@@ -4043,6 +4069,7 @@ impl_lint_pass!(Methods => [
     ITER_FILTER_IS_OK,
     MANUAL_IS_VARIANT_AND,
     STR_SPLIT_AT_NEWLINE,
+    OPTION_AS_REF_CLONED,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -4290,7 +4317,10 @@ impl Methods {
                 ("as_mut", []) => useless_asref::check(cx, expr, "as_mut", recv),
                 ("as_ref", []) => useless_asref::check(cx, expr, "as_ref", recv),
                 ("assume_init", []) => uninit_assumed_init::check(cx, expr, recv),
-                ("cloned", []) => cloned_instead_of_copied::check(cx, expr, recv, span, &self.msrv),
+                ("cloned", []) => {
+                    cloned_instead_of_copied::check(cx, expr, recv, span, &self.msrv);
+                    option_as_ref_cloned::check(cx, recv, span);
+                },
                 ("collect", []) if is_trait_method(cx, expr, sym::Iterator) => {
                     needless_collect::check(cx, span, expr, recv, call_span);
                     match method_call(recv) {

clippy_lints/src/methods/option_as_ref_cloned.rs

@@ -0,0 +1,24 @@
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::ty::is_type_diagnostic_item;
+use rustc_errors::Applicability;
+use rustc_hir::Expr;
+use rustc_lint::LateContext;
+use rustc_span::{sym, Span};
+
+use super::{method_call, OPTION_AS_REF_CLONED};
+
+pub(super) fn check(cx: &LateContext<'_>, cloned_recv: &Expr<'_>, cloned_ident_span: Span) {
+    if let Some((method @ ("as_ref" | "as_mut"), as_ref_recv, [], as_ref_ident_span, _)) = method_call(cloned_recv)
+        && is_type_diagnostic_item(cx, cx.typeck_results().expr_ty(as_ref_recv).peel_refs(), sym::Option)
+    {
+        span_lint_and_sugg(
+            cx,
+            OPTION_AS_REF_CLONED,
+            as_ref_ident_span.to(cloned_ident_span),
+            &format!("cloning an `Option<_>` using `.{method}().cloned()`"),
+            "this can be written more concisely by cloning the `Option<_>` directly",
+            "clone".into(),
+            Applicability::MachineApplicable,
+        );
+    }
+}

tests/ui/option_as_ref_cloned.fixed

@@ -0,0 +1,21 @@
+#![warn(clippy::option_as_ref_cloned)]
+#![allow(clippy::clone_on_copy)]
+
+fn main() {
+    let mut x = Some(String::new());
+
+    let _: Option<String> = x.clone();
+    let _: Option<String> = x.clone();
+
+    let y = x.as_ref();
+    let _: Option<&String> = y.clone();
+
+    macro_rules! cloned_recv {
+        () => {
+            x.as_ref()
+        };
+    }
+
+    // Don't lint when part of the expression is from a macro
+    let _: Option<String> = cloned_recv!().cloned();
+}

tests/ui/option_as_ref_cloned.rs

@@ -0,0 +1,21 @@
+#![warn(clippy::option_as_ref_cloned)]
+#![allow(clippy::clone_on_copy)]
+
+fn main() {
+    let mut x = Some(String::new());
+
+    let _: Option<String> = x.as_ref().cloned();
+    let _: Option<String> = x.as_mut().cloned();
+
+    let y = x.as_ref();
+    let _: Option<&String> = y.as_ref().cloned();
+
+    macro_rules! cloned_recv {
+        () => {
+            x.as_ref()
+        };
+    }
+
+    // Don't lint when part of the expression is from a macro
+    let _: Option<String> = cloned_recv!().cloned();
+}

tests/ui/option_as_ref_cloned.stderr

@@ -0,0 +1,37 @@
+error: cloning an `Option<_>` using `.as_ref().cloned()`
+  --> $DIR/option_as_ref_cloned.rs:7:31
+   |
+LL |     let _: Option<String> = x.as_ref().cloned();
+   |                               ^^^^^^^^^^^^^^^
+   |
+   = note: `-D clippy::option-as-ref-cloned` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::option_as_ref_cloned)]`
+help: this can be written more concisely by cloning the `Option<_>` directly
+   |
+LL |     let _: Option<String> = x.clone();
+   |                               ~~~~~
+
+error: cloning an `Option<_>` using `.as_mut().cloned()`
+  --> $DIR/option_as_ref_cloned.rs:8:31
+   |
+LL |     let _: Option<String> = x.as_mut().cloned();
+   |                               ^^^^^^^^^^^^^^^
+   |
+help: this can be written more concisely by cloning the `Option<_>` directly
+   |
+LL |     let _: Option<String> = x.clone();
+   |                               ~~~~~
+
+error: cloning an `Option<_>` using `.as_ref().cloned()`
+  --> $DIR/option_as_ref_cloned.rs:11:32
+   |
+LL |     let _: Option<&String> = y.as_ref().cloned();
+   |                                ^^^^^^^^^^^^^^^
+   |
+help: this can be written more concisely by cloning the `Option<_>` directly
+   |
+LL |     let _: Option<&String> = y.clone();
+   |                                ~~~~~
+
+error: aborting due to 3 previous errors
+