Prevent mem_replace_with_default lint within macros

Also added test cases for internal and external macros.
rust-lang · Dec 8, 2019 · 8a27af3 · 8a27af3
1 parent 4ad0e5b
commit 8a27af3
Show file tree

Hide file tree

Showing 5 changed files with 44 additions and 9 deletions.
diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs
@@ -1,10 +1,10 @@
 use crate::utils::{
-    match_def_path, match_qpath, paths, snippet_with_applicability, span_help_and_lint, span_lint_and_sugg,
+    in_macro, match_def_path, match_qpath, paths, snippet_with_applicability, span_help_and_lint, span_lint_and_sugg,
 };
 use if_chain::if_chain;
 use rustc::declare_lint_pass;
 use rustc::hir::{BorrowKind, Expr, ExprKind, HirVec, Mutability, QPath};
-use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass};
+use rustc::lint::{in_external_macro, LateContext, LateLintPass, LintArray, LintPass};
 use rustc_errors::Applicability;
 use rustc_session::declare_tool_lint;
 
@@ -163,9 +163,9 @@ fn check_replace_with_uninit(cx: &LateContext<'_, '_>, expr: &'_ Expr, args: &Hi
 }
 
 fn check_replace_with_default(cx: &LateContext<'_, '_>, expr: &'_ Expr, args: &HirVec<Expr>) {
-    if let ExprKind::Call(ref repl_func, ref repl_args) = args[1].kind {
+    if let ExprKind::Call(ref repl_func, _) = args[1].kind {
         if_chain! {
-            if repl_args.is_empty();
+            if !in_macro(expr.span) && !in_external_macro(cx.tcx.sess, expr.span);
             if let ExprKind::Path(ref repl_func_qpath) = repl_func.kind;
             if let Some(repl_def_id) = cx.tables.qpath_res(repl_func_qpath, repl_func.hir_id).opt_def_id();
             if match_def_path(cx, repl_def_id, &paths::DEFAULT_TRAIT_METHOD);

diff --git a/tests/ui/auxiliary/macro_rules.rs b/tests/ui/auxiliary/macro_rules.rs
@@ -39,3 +39,10 @@ macro_rules! string_add {
         let z = y + "...";
     };
 }
+
+#[macro_export]
+macro_rules! take_external {
+    ($s:expr) => {
+        std::mem::replace($s, Default::default())
+    };
+}
diff --git a/tests/ui/mem_replace.fixed b/tests/ui/mem_replace.fixed
@@ -8,6 +8,7 @@
 // except according to those terms.
 
 // run-rustfix
+// aux-build:macro_rules.rs
 #![allow(unused_imports)]
 #![warn(
     clippy::all,
@@ -16,8 +17,17 @@
     clippy::mem_replace_with_default
 )]
 
+#[macro_use]
+extern crate macro_rules;
+
 use std::mem;
 
+macro_rules! take {
+    ($s:expr) => {
+        std::mem::replace($s, Default::default())
+    };
+}
+
 fn replace_option_with_none() {
     let mut an_option = Some(1);
     let _ = an_option.take();
@@ -31,6 +41,10 @@ fn replace_with_default() {
     let s = &mut String::from("foo");
     let _ = std::mem::take(s);
     let _ = std::mem::take(s);
+
+    // dont lint within macros
+    take!(s);
+    take_external!(s);
 }
 
 fn main() {

diff --git a/tests/ui/mem_replace.rs b/tests/ui/mem_replace.rs
@@ -8,6 +8,7 @@
 // except according to those terms.
 
 // run-rustfix
+// aux-build:macro_rules.rs
 #![allow(unused_imports)]
 #![warn(
     clippy::all,
@@ -16,8 +17,17 @@
     clippy::mem_replace_with_default
 )]
 
+#[macro_use]
+extern crate macro_rules;
+
 use std::mem;
 
+macro_rules! take {
+    ($s:expr) => {
+        std::mem::replace($s, Default::default())
+    };
+}
+
 fn replace_option_with_none() {
     let mut an_option = Some(1);
     let _ = mem::replace(&mut an_option, None);
@@ -31,6 +41,10 @@ fn replace_with_default() {
     let s = &mut String::from("foo");
     let _ = std::mem::replace(s, String::default());
     let _ = std::mem::replace(s, Default::default());
+
+    // dont lint within macros
+    take!(s);
+    take_external!(s);
 }
 
 fn main() {

diff --git a/tests/ui/mem_replace.stderr b/tests/ui/mem_replace.stderr
@@ -1,33 +1,33 @@
 error: replacing an `Option` with `None`
-  --> $DIR/mem_replace.rs:23:13
+  --> $DIR/mem_replace.rs:33:13
    |
 LL |     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:25:13
+  --> $DIR/mem_replace.rs:35:13
    |
 LL |     let _ = mem::replace(an_option, None);
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()`
 
 error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
-  --> $DIR/mem_replace.rs:30:13
+  --> $DIR/mem_replace.rs:40:13
    |
 LL |     let _ = std::mem::replace(&mut s, String::default());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut s)`
    |
    = note: `-D clippy::mem-replace-with-default` implied by `-D warnings`
 
 error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
-  --> $DIR/mem_replace.rs:32:13
+  --> $DIR/mem_replace.rs:42:13
    |
 LL |     let _ = std::mem::replace(s, String::default());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(s)`
 
 error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
-  --> $DIR/mem_replace.rs:33:13
+  --> $DIR/mem_replace.rs:43:13
    |
 LL |     let _ = std::mem::replace(s, Default::default());
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(s)`