Auto merge of rust-lang#10594 - J-ZhengLi:issue9824, r=Jarcho

fix [`mem_replace_option_with_none`] not considering field variables fixes: rust-lang#9824 --- changelog: fix [`mem_replace_option_with_none`] not considering field variables
flip1995 · Apr 6, 2023 · de5c6d6 · de5c6d6
2 parents 2b05f79 + 008e07d
commit de5c6d6
Show file tree

Hide file tree

Showing 4 changed files with 121 additions and 44 deletions.
diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs
@@ -1,12 +1,13 @@
 use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
 use clippy_utils::msrvs::{self, Msrv};
 use clippy_utils::source::{snippet, snippet_with_applicability};
+use clippy_utils::sugg::Sugg;
 use clippy_utils::ty::is_non_aggregate_primitive_type;
-use clippy_utils::{is_default_equivalent, is_res_lang_ctor, path_res};
+use clippy_utils::{is_default_equivalent, is_res_lang_ctor, path_res, peel_ref_operators};
 use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir::LangItem::OptionNone;
-use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, QPath};
+use rustc_hir::{Expr, ExprKind};
 use rustc_lint::{LateContext, LateLintPass};
 use rustc_middle::lint::in_external_macro;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -101,40 +102,26 @@ declare_clippy_lint! {
 impl_lint_pass!(MemReplace =>
     [MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_WITH_UNINIT, MEM_REPLACE_WITH_DEFAULT]);
 
-fn check_replace_option_with_none(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
-    // Check that second argument is `Option::None`
-    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
-        // replacee's path.
-        let replaced_path = match dest.kind {
-            ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, replaced) => {
-                if let ExprKind::Path(QPath::Resolved(None, replaced_path)) = replaced.kind {
-                    replaced_path
-                } else {
-                    return;
-                }
-            },
-            ExprKind::Path(QPath::Resolved(None, replaced_path)) => replaced_path,
-            _ => return,
-        };
-
-        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()",
-                snippet_with_applicability(cx, replaced_path.span, "", &mut applicability)
-            ),
-            applicability,
-        );
-    }
+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_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
@@ -200,10 +187,6 @@ fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<
     if is_non_aggregate_primitive_type(expr_type) {
         return;
     }
-    // disable lint for Option since it is covered in another lint
-    if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) {
-        return;
-    }
     if is_default_equivalent(cx, src) && !in_external_macro(cx.tcx.sess, expr_span) {
         span_lint_and_then(
             cx,
@@ -246,11 +229,13 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace {
             if let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id();
             if cx.tcx.is_diagnostic_item(sym::mem_replace, def_id);
             then {
-                check_replace_option_with_none(cx, src, dest, expr.span);
-                check_replace_with_uninit(cx, src, dest, expr.span);
-                if self.msrv.meets(msrvs::MEM_TAKE) {
+                // 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) {
                     check_replace_with_default(cx, src, dest, expr.span);
                 }
+                check_replace_with_uninit(cx, src, dest, expr.span);
             }
         }
     }

diff --git a/tests/ui/mem_replace.fixed b/tests/ui/mem_replace.fixed
@@ -90,3 +90,37 @@ fn msrv_1_40() {
     let mut s = String::from("foo");
     let _ = std::mem::take(&mut s);
 }
+
+fn issue9824() {
+    struct Foo<'a>(Option<&'a str>);
+    impl<'a> std::ops::Deref for Foo<'a> {
+        type Target = Option<&'a str>;
+
+        fn deref(&self) -> &Self::Target {
+            &self.0
+        }
+    }
+    impl<'a> std::ops::DerefMut for Foo<'a> {
+        fn deref_mut(&mut self) -> &mut Self::Target {
+            &mut self.0
+        }
+    }
+
+    struct Bar {
+        opt: Option<u8>,
+        val: String,
+    }
+
+    let mut f = Foo(Some("foo"));
+    let mut b = Bar {
+        opt: Some(1),
+        val: String::from("bar"),
+    };
+
+    // replace option with none
+    let _ = f.0.take();
+    let _ = (*f).take();
+    let _ = b.opt.take();
+    // replace with default
+    let _ = std::mem::take(&mut b.val);
+}
diff --git a/tests/ui/mem_replace.rs b/tests/ui/mem_replace.rs
@@ -90,3 +90,37 @@ fn msrv_1_40() {
     let mut s = String::from("foo");
     let _ = std::mem::replace(&mut s, String::default());
 }
+
+fn issue9824() {
+    struct Foo<'a>(Option<&'a str>);
+    impl<'a> std::ops::Deref for Foo<'a> {
+        type Target = Option<&'a str>;
+
+        fn deref(&self) -> &Self::Target {
+            &self.0
+        }
+    }
+    impl<'a> std::ops::DerefMut for Foo<'a> {
+        fn deref_mut(&mut self) -> &mut Self::Target {
+            &mut self.0
+        }
+    }
+
+    struct Bar {
+        opt: Option<u8>,
+        val: String,
+    }
+
+    let mut f = Foo(Some("foo"));
+    let mut b = Bar {
+        opt: Some(1),
+        val: String::from("bar"),
+    };
+
+    // replace option with none
+    let _ = std::mem::replace(&mut f.0, None);
+    let _ = std::mem::replace(&mut *f, None);
+    let _ = std::mem::replace(&mut b.opt, None);
+    // replace with default
+    let _ = std::mem::replace(&mut b.val, String::default());
+}
diff --git a/tests/ui/mem_replace.stderr b/tests/ui/mem_replace.stderr
@@ -122,5 +122,29 @@ error: replacing a value of type `T` with `T::default()` is better expressed usi
 LL |     let _ = std::mem::replace(&mut s, String::default());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut s)`
 
-error: aborting due to 20 previous errors
+error: replacing an `Option` with `None`
+  --> $DIR/mem_replace.rs:121:13
+   |
+LL |     let _ = std::mem::replace(&mut f.0, None);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `f.0.take()`
+
+error: replacing an `Option` with `None`
+  --> $DIR/mem_replace.rs:122:13
+   |
+LL |     let _ = std::mem::replace(&mut *f, None);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `(*f).take()`
+
+error: replacing an `Option` with `None`
+  --> $DIR/mem_replace.rs:123:13
+   |
+LL |     let _ = std::mem::replace(&mut b.opt, None);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `b.opt.take()`
+
+error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
+  --> $DIR/mem_replace.rs:125:13
+   |
+LL |     let _ = std::mem::replace(&mut b.val, String::default());
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut b.val)`
+
+error: aborting due to 24 previous errors