Merge pull request #3195 from JayKickliter/jsk/mem_replace_opt_w_none

Add lint for `mem::replace(.., None)`.

CHANGELOG.md

@@ -744,6 +744,7 @@ All notable changes to this project will be documented in this file.
 [`match_wild_err_arm`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#match_wild_err_arm
 [`maybe_infinite_iter`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#maybe_infinite_iter
 [`mem_forget`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mem_forget
+[`mem_replace_option_with_none`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#mem_replace_option_with_none
 [`min_max`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#min_max
 [`misaligned_transmute`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#misaligned_transmute
 [`misrefactored_assign_op`]: https://rust-lang-nursery.github.io/rust-clippy/master/index.html#misrefactored_assign_op

README.md

@@ -9,7 +9,7 @@ We are currently in the process of discussing Clippy 1.0 via the RFC process in
 
 A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code.
 
-[There are 276 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
+[There are 277 lints included in this crate!](https://rust-lang-nursery.github.io/rust-clippy/master/index.html)
 
 We have a bunch of lint categories to allow you to choose how much Clippy is supposed to ~~annoy~~ help you:
 

clippy_lints/src/lib.rs

@@ -133,6 +133,7 @@ pub mod map_clone;
 pub mod map_unit_fn;
 pub mod matches;
 pub mod mem_forget;
+pub mod mem_replace;
 pub mod methods;
 pub mod minmax;
 pub mod misc;
@@ -380,6 +381,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
     reg.register_late_lint_pass(box neg_multiply::NegMultiply);
     reg.register_early_lint_pass(box unsafe_removed_from_name::UnsafeNameRemoval);
     reg.register_late_lint_pass(box mem_forget::MemForget);
+    reg.register_late_lint_pass(box mem_replace::MemReplace);
     reg.register_late_lint_pass(box arithmetic::Arithmetic::default());
     reg.register_late_lint_pass(box assign_ops::AssignOps);
     reg.register_late_lint_pass(box let_if_seq::LetIfSeq);
@@ -591,6 +593,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         matches::MATCH_REF_PATS,
         matches::MATCH_WILD_ERR_ARM,
         matches::SINGLE_MATCH,
+        mem_replace::MEM_REPLACE_OPTION_WITH_NONE,
         methods::CHARS_LAST_CMP,
         methods::CHARS_NEXT_CMP,
         methods::CLONE_DOUBLE_REF,
@@ -748,6 +751,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry<'_>, conf: &Conf) {
         matches::MATCH_REF_PATS,
         matches::MATCH_WILD_ERR_ARM,
         matches::SINGLE_MATCH,
+        mem_replace::MEM_REPLACE_OPTION_WITH_NONE,
         methods::CHARS_LAST_CMP,
         methods::GET_UNWRAP,
         methods::ITER_CLONED_COLLECT,

clippy_lints/src/mem_replace.rs

@@ -0,0 +1,83 @@
+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, 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
+/// `None`.
+///
+/// **Why is this bad?** `Option` already has the method `take()` for
+/// taking its current value (Some(..) or None) and replacing it with
+/// `None`.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// let mut an_option = Some(0);
+/// let replaced = mem::replace(&mut an_option, None);
+/// ```
+/// Is better expressed with:
+/// ```rust
+/// let mut an_option = Some(0);
+/// let taken = an_option.take();
+/// ```
+declare_clippy_lint! {
+    pub MEM_REPLACE_OPTION_WITH_NONE,
+    style,
+    "replacing an `Option` with `None` instead of `take()`"
+}
+
+pub struct MemReplace;
+
+impl LintPass for MemReplace {
+    fn get_lints(&self) -> LintArray {
+        lint_array![MEM_REPLACE_OPTION_WITH_NONE]
+    }
+}
+
+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);
+
+            // 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);
+
+            then {
+                // 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 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",
+                    format!("{}.take()", snippet(cx, replaced_path.span, ""))
+                );
+            }
+        }
+    }
+}

clippy_lints/src/utils/paths.rs

@@ -47,6 +47,7 @@ pub const LINKED_LIST: [&str; 4] = ["alloc", "collections", "linked_list", "Link
 pub const LINT: [&str; 3] = ["rustc", "lint", "Lint"];
 pub const LINT_ARRAY: [&str; 3] = ["rustc", "lint", "LintArray"];
 pub const MEM_FORGET: [&str; 3] = ["core", "mem", "forget"];
+pub const MEM_REPLACE: [&str; 3] = ["core", "mem", "replace"];
 pub const MEM_UNINIT: [&str; 3] = ["core", "mem", "uninitialized"];
 pub const MEM_ZEROED: [&str; 3] = ["core", "mem", "zeroed"];
 pub const MUTEX: [&str; 4] = ["std", "sync", "mutex", "Mutex"];

tests/ui/mem_replace.rs

@@ -0,0 +1,11 @@
+#![feature(tool_lints)]
+#![warn(clippy::all, clippy::style, clippy::mem_replace_option_with_none)]
+
+use std::mem;
+
+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);
+}

tests/ui/mem_replace.stderr

@@ -0,0 +1,16 @@
+error: replacing an `Option` with `None`
+ --> $DIR/mem_replace.rs:8:13
+  |
+8 |     let _ = mem::replace(&mut an_option, None);
+  |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()`
+  |
+  = note: `-D clippy::mem-replace-option-with-none` implied by `-D warnings`
+
+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
+