New lint: mem_replace_option_with_some

CHANGELOG.md

@@ -5812,6 +5812,7 @@ Released 2018-09-13
 [`mem_discriminant_non_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_discriminant_non_enum
 [`mem_forget`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_forget
 [`mem_replace_option_with_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_none
+[`mem_replace_option_with_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_some
 [`mem_replace_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default
 [`mem_replace_with_uninit`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_uninit
 [`min_ident_chars`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_ident_chars

book/src/lint_configuration.md

@@ -764,6 +764,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio
 * [`map_unwrap_or`](https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or)
 * [`map_with_unused_argument_over_ranges`](https://rust-lang.github.io/rust-clippy/master/index.html#map_with_unused_argument_over_ranges)
 * [`match_like_matches_macro`](https://rust-lang.github.io/rust-clippy/master/index.html#match_like_matches_macro)
+* [`mem_replace_option_with_some`](https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_some)
 * [`mem_replace_with_default`](https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default)
 * [`missing_const_for_fn`](https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn)
 * [`needless_borrow`](https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow)

clippy_config/src/conf.rs

@@ -633,6 +633,7 @@ define_Conf! {
         map_unwrap_or,
         map_with_unused_argument_over_ranges,
         match_like_matches_macro,
+        mem_replace_option_with_some,
         mem_replace_with_default,
         missing_const_for_fn,
         needless_borrow,

clippy_lints/src/declared_lints.rs

@@ -362,6 +362,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::matches::WILDCARD_ENUM_MATCH_ARM_INFO,
     crate::matches::WILDCARD_IN_OR_PATTERNS_INFO,
     crate::mem_replace::MEM_REPLACE_OPTION_WITH_NONE_INFO,
+    crate::mem_replace::MEM_REPLACE_OPTION_WITH_SOME_INFO,
     crate::mem_replace::MEM_REPLACE_WITH_DEFAULT_INFO,
     crate::mem_replace::MEM_REPLACE_WITH_UNINIT_INFO,
     crate::methods::BIND_INSTEAD_OF_MAP_INFO,

clippy_lints/src/mem_replace.rs

@@ -8,7 +8,7 @@ use clippy_utils::{
     is_default_equivalent, is_expr_used_or_unified, is_res_lang_ctor, path_res, peel_ref_operators, std_or_core,
 };
 use rustc_errors::Applicability;
-use rustc_hir::LangItem::OptionNone;
+use rustc_hir::LangItem::{OptionNone, OptionSome};
 use rustc_hir::{Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::impl_lint_pass;
@@ -43,6 +43,31 @@ declare_clippy_lint! {
     "replacing an `Option` with `None` instead of `take()`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `mem::replace()` on an `Option` with `Some(…)`.
+    ///
+    /// ### Why is this bad?
+    /// `Option` already has the method `replace()` for
+    /// taking its current value (Some(…) or None) and replacing it with
+    /// `Some(…)`.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let mut an_option = Some(0);
+    /// let replaced = std::mem::replace(&mut an_option, Some(1));
+    /// ```
+    /// Is better expressed with:
+    /// ```no_run
+    /// let mut an_option = Some(0);
+    /// let taken = an_option.replace(1);
+    /// ```
+    #[clippy::version = "1.86.0"]
+    pub MEM_REPLACE_OPTION_WITH_SOME,
+    style,
+    "replacing an `Option` with `Some` instead of `replace()`"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for `mem::replace(&mut _, mem::uninitialized())`
@@ -101,28 +126,67 @@ declare_clippy_lint! {
 }
 
 impl_lint_pass!(MemReplace =>
-    [MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_WITH_UNINIT, MEM_REPLACE_WITH_DEFAULT]);
+    [MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_OPTION_WITH_SOME, MEM_REPLACE_WITH_UNINIT, MEM_REPLACE_WITH_DEFAULT]);
+
+fn check_replace_option_with_none(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) -> bool {
+    if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) {
+        // Since this is a late pass (already type-checked),
+        // and we already know that the second argument is an
+        // `Option`, we do not need to check the first
+        // argument's type. All that's left is to get
+        // the replacee's expr after peeling off the `&mut`
+        let sugg_expr = peel_ref_operators(cx, dest);
+        let mut applicability = Applicability::MachineApplicable;
+        span_lint_and_sugg(
+            cx,
+            MEM_REPLACE_OPTION_WITH_NONE,
+            expr_span,
+            "replacing an `Option` with `None`",
+            "consider `Option::take()` instead",
+            format!(
+                "{}.take()",
+                Sugg::hir_with_context(cx, sugg_expr, expr_span.ctxt(), "", &mut applicability).maybe_par()
+            ),
+            applicability,
+        );
+        true
+    } else {
+        false
+    }
+}
 
-fn check_replace_option_with_none(cx: &LateContext<'_>, dest: &Expr<'_>, expr_span: Span) {
-    // Since this is a late pass (already type-checked),
-    // and we already know that the second argument is an
-    // `Option`, we do not need to check the first
-    // argument's type. All that's left is to get
-    // the replacee's expr after peeling off the `&mut`
-    let sugg_expr = peel_ref_operators(cx, dest);
-    let mut applicability = Applicability::MachineApplicable;
-    span_lint_and_sugg(
-        cx,
-        MEM_REPLACE_OPTION_WITH_NONE,
-        expr_span,
-        "replacing an `Option` with `None`",
-        "consider `Option::take()` instead",
-        format!(
-            "{}.take()",
-            Sugg::hir_with_context(cx, sugg_expr, expr_span.ctxt(), "", &mut applicability).maybe_par()
-        ),
-        applicability,
-    );
+fn check_replace_option_with_some(
+    cx: &LateContext<'_>,
+    src: &Expr<'_>,
+    dest: &Expr<'_>,
+    expr_span: Span,
+    msrv: &Msrv,
+) -> bool {
+    if msrv.meets(msrvs::OPTION_REPLACE)
+        && let ExprKind::Call(src_func, [src_arg]) = src.kind
+        && is_res_lang_ctor(cx, path_res(cx, src_func), OptionSome)
+    {
+        // We do not have to check for a `const` context here, because `core::mem::replace()` and
+        // `Option::replace()` have been const-stabilized simultaneously in version 1.83.0.
+        let sugg_expr = peel_ref_operators(cx, dest);
+        let mut applicability = Applicability::MachineApplicable;
+        span_lint_and_sugg(
+            cx,
+            MEM_REPLACE_OPTION_WITH_SOME,
+            expr_span,
+            "replacing an `Option` with `Some(..)`",
+            "consider `Option::replace()` instead",
+            format!(
+                "{}.replace({})",
+                Sugg::hir_with_context(cx, sugg_expr, expr_span.ctxt(), "_", &mut applicability).maybe_par(),
+                snippet_with_applicability(cx, src_arg.span, "_", &mut applicability)
+            ),
+            applicability,
+        );
+        true
+    } else {
+        false
+    }
 }
 
 fn check_replace_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
@@ -181,34 +245,44 @@ fn check_replace_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'
     }
 }
 
-fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
-    // disable lint for primitives
-    let expr_type = cx.typeck_results().expr_ty_adjusted(src);
-    if is_non_aggregate_primitive_type(expr_type) {
-        return;
-    }
-    if is_default_equivalent(cx, src) && !expr_span.in_external_macro(cx.tcx.sess.source_map()) {
-        let Some(top_crate) = std_or_core(cx) else { return };
+fn check_replace_with_default(
+    cx: &LateContext<'_>,
+    src: &Expr<'_>,
+    dest: &Expr<'_>,
+    expr: &Expr<'_>,
+    msrv: &Msrv,
+) -> bool {
+    if msrv.meets(msrvs::MEM_TAKE) && is_expr_used_or_unified(cx.tcx, expr)
+        // disable lint for primitives
+        && let expr_type = cx.typeck_results().expr_ty_adjusted(src)
+        && !is_non_aggregate_primitive_type(expr_type)
+        && is_default_equivalent(cx, src)
+        && !expr.span.in_external_macro(cx.tcx.sess.source_map())
+        && let Some(top_crate) = std_or_core(cx)
+    {
         span_lint_and_then(
             cx,
             MEM_REPLACE_WITH_DEFAULT,
-            expr_span,
+            expr.span,
             format!(
                 "replacing a value of type `T` with `T::default()` is better expressed using `{top_crate}::mem::take`"
             ),
             |diag| {
-                if !expr_span.from_expansion() {
+                if !expr.span.from_expansion() {
                     let suggestion = format!("{top_crate}::mem::take({})", snippet(cx, dest.span, ""));
 
                     diag.span_suggestion(
-                        expr_span,
+                        expr.span,
                         "consider using",
                         suggestion,
                         Applicability::MachineApplicable,
                     );
                 }
             },
         );
+        true
+    } else {
+        false
     }
 }
 
@@ -233,12 +307,12 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace {
             && cx.tcx.is_diagnostic_item(sym::mem_replace, def_id)
         {
             // Check that second argument is `Option::None`
-            if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) {
-                check_replace_option_with_none(cx, dest, expr.span);
-            } else if self.msrv.meets(msrvs::MEM_TAKE) && is_expr_used_or_unified(cx.tcx, expr) {
-                check_replace_with_default(cx, src, dest, expr.span);
+            if !check_replace_option_with_none(cx, src, dest, expr.span)
+                && !check_replace_option_with_some(cx, src, dest, expr.span, &self.msrv)
+                && !check_replace_with_default(cx, src, dest, expr, &self.msrv)
+            {
+                check_replace_with_uninit(cx, src, dest, expr.span);
             }
-            check_replace_with_uninit(cx, src, dest, expr.span);
         }
     }
     extract_msrv_attr!(LateContext);

clippy_utils/src/msrvs.rs

@@ -58,6 +58,7 @@ msrv_aliases! {
     1,35,0 { OPTION_COPIED, RANGE_CONTAINS }
     1,34,0 { TRY_FROM }
     1,33,0 { UNDERSCORE_IMPORTS }
+    1,31,0 { OPTION_REPLACE }
     1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES }
     1,29,0 { ITER_FLATTEN }
     1,28,0 { FROM_BOOL, REPEAT_WITH }

tests/ui/mem_replace.fixed

@@ -131,3 +131,25 @@ fn issue9824() {
     // replace with default
     let _ = std::mem::take(&mut b.val);
 }
+
+#[clippy::msrv = "1.31"]
+fn mem_replace_option_with_some() {
+    let mut an_option = Some(0);
+    let replaced = an_option.replace(1);
+    //~^ ERROR: replacing an `Option` with `Some(..)`
+
+    let mut an_option = &mut Some(0);
+    let replaced = an_option.replace(1);
+    //~^ ERROR: replacing an `Option` with `Some(..)`
+
+    let (mut opt1, mut opt2) = (Some(0), Some(0));
+    let b = true;
+    let replaced = (if b { &mut opt1 } else { &mut opt2 }).replace(1);
+    //~^ ERROR: replacing an `Option` with `Some(..)`
+}
+
+#[clippy::msrv = "1.30"]
+fn mem_replace_option_with_some_bad_msrv() {
+    let mut an_option = Some(0);
+    let replaced = mem::replace(&mut an_option, Some(1));
+}

tests/ui/mem_replace.rs

@@ -131,3 +131,25 @@ fn issue9824() {
     // replace with default
     let _ = std::mem::replace(&mut b.val, String::default());
 }
+
+#[clippy::msrv = "1.31"]
+fn mem_replace_option_with_some() {
+    let mut an_option = Some(0);
+    let replaced = mem::replace(&mut an_option, Some(1));
+    //~^ ERROR: replacing an `Option` with `Some(..)`
+
+    let mut an_option = &mut Some(0);
+    let replaced = mem::replace(an_option, Some(1));
+    //~^ ERROR: replacing an `Option` with `Some(..)`
+
+    let (mut opt1, mut opt2) = (Some(0), Some(0));
+    let b = true;
+    let replaced = mem::replace(if b { &mut opt1 } else { &mut opt2 }, Some(1));
+    //~^ ERROR: replacing an `Option` with `Some(..)`
+}
+
+#[clippy::msrv = "1.30"]
+fn mem_replace_option_with_some_bad_msrv() {
+    let mut an_option = Some(0);
+    let replaced = mem::replace(&mut an_option, Some(1));
+}

tests/ui/mem_replace.stderr

@@ -160,5 +160,26 @@ error: replacing a value of type `T` with `T::default()` is better expressed usi
 LL |     let _ = std::mem::replace(&mut b.val, String::default());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut b.val)`
 
-error: aborting due to 26 previous errors
+error: replacing an `Option` with `Some(..)`
+  --> tests/ui/mem_replace.rs:138:20
+   |
+LL |     let replaced = mem::replace(&mut an_option, Some(1));
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::replace()` instead: `an_option.replace(1)`
+   |
+   = note: `-D clippy::mem-replace-option-with-some` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::mem_replace_option_with_some)]`
+
+error: replacing an `Option` with `Some(..)`
+  --> tests/ui/mem_replace.rs:142:20
+   |
+LL |     let replaced = mem::replace(an_option, Some(1));
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::replace()` instead: `an_option.replace(1)`
+
+error: replacing an `Option` with `Some(..)`
+  --> tests/ui/mem_replace.rs:147:20
+   |
+LL |     let replaced = mem::replace(if b { &mut opt1 } else { &mut opt2 }, Some(1));
+   |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::replace()` instead: `(if b { &mut opt1 } else { &mut opt2 }).replace(1)`
+
+error: aborting due to 29 previous errors