From 2f53aaa5bd663fb572f349ca31ad56c32e222977 Mon Sep 17 00:00:00 2001
From: Jay Kickliter <jay@kickliter.com>
Date: Tue, 18 Sep 2018 16:54:01 -0700
Subject: [PATCH] mem_replace: match on path.

---
 clippy_lints/src/mem_replace.rs | 29 +++++++++++++++++++++++------
 tests/ui/mem_replace.rs         |  2 ++
 tests/ui/mem_replace.stderr     |  8 +++++++-
 3 files changed, 32 insertions(+), 7 deletions(-)

diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs
index f516fc5085c..22460ccb5ea 100644
--- a/clippy_lints/src/mem_replace.rs
+++ b/clippy_lints/src/mem_replace.rs
@@ -1,7 +1,7 @@
 use crate::rustc::hir::{Expr, ExprKind, MutMutable, QPath};
 use crate::rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
 use crate::rustc::{declare_tool_lint, lint_array};
-use crate::utils::{match_def_path, match_qpath, match_type, opt_def_id, paths, snippet, span_lint_and_sugg};
+use crate::utils::{match_def_path, match_qpath, opt_def_id, paths, snippet, span_lint_and_sugg};
 use if_chain::if_chain;
 
 /// **What it does:** Checks for `mem::replace()` on an `Option` with
@@ -40,25 +40,42 @@ fn get_lints(&self) -> LintArray {
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MemReplace {
     fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr) {
         if_chain! {
+            // Check that `expr` is a call to `mem::replace()`
             if let ExprKind::Call(ref func, ref func_args) = expr.node;
             if func_args.len() == 2;
             if let ExprKind::Path(ref func_qpath) = func.node;
             if let Some(def_id) = opt_def_id(cx.tables.qpath_def(func_qpath, func.hir_id));
             if match_def_path(cx.tcx, def_id, &paths::MEM_REPLACE);
-            if let ExprKind::AddrOf(MutMutable, ref replaced) = func_args[0].node;
-            if match_type(cx, cx.tables.expr_ty(replaced), &paths::OPTION);
+
+            // Check that second argument is `Option::None`
             if let ExprKind::Path(ref replacement_qpath) = func_args[1].node;
             if match_qpath(replacement_qpath, &paths::OPTION_NONE);
-            if let ExprKind::Path(QPath::Resolved(None, ref replaced_path)) = replaced.node;
+
             then {
-                let sugg = format!("{}.take()", snippet(cx, replaced_path.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 if the first
+                // argument's type. All that's left is to get
+                // replacee's path.
+                let replaced_path = match func_args[0].node {
+                    ExprKind::AddrOf(MutMutable, ref replaced) => {
+                        if let ExprKind::Path(QPath::Resolved(None, ref replaced_path)) = replaced.node {
+                            replaced_path
+                        } else {
+                            return
+                        }
+                    },
+                    ExprKind::Path(QPath::Resolved(None, ref replaced_path)) => replaced_path,
+                    _ => return,
+                };
+
                 span_lint_and_sugg(
                     cx,
                     MEM_REPLACE_OPTION_WITH_NONE,
                     expr.span,
                     "replacing an `Option` with `None`",
                     "consider `Option::take()` instead",
-                    sugg
+                    format!("{}.take()", snippet(cx, replaced_path.span, ""))
                 );
             }
         }
diff --git a/tests/ui/mem_replace.rs b/tests/ui/mem_replace.rs
index 14f586e71bf..62df42ef2d2 100644
--- a/tests/ui/mem_replace.rs
+++ b/tests/ui/mem_replace.rs
@@ -6,4 +6,6 @@
 fn main() {
     let mut an_option = Some(1);
     let _ = mem::replace(&mut an_option, None);
+    let an_option = &mut Some(1);
+    let _ = mem::replace(an_option, None);
 }
diff --git a/tests/ui/mem_replace.stderr b/tests/ui/mem_replace.stderr
index 1ce9fc38093..8385fa3cb3c 100644
--- a/tests/ui/mem_replace.stderr
+++ b/tests/ui/mem_replace.stderr
@@ -6,5 +6,11 @@ error: replacing an `Option` with `None`
   |
   = note: `-D clippy::mem-replace-option-with-none` implied by `-D warnings`
 
-error: aborting due to previous error
+error: replacing an `Option` with `None`
+  --> $DIR/mem_replace.rs:10:13
+   |
+10 |     let _ = mem::replace(an_option, None);
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()`
+
+error: aborting due to 2 previous errors
 
-- 
2.44.0