From 26812f733daaf65e514c058c51138511f6f5b60c Mon Sep 17 00:00:00 2001 From: Krishna Veera Reddy Date: Sun, 8 Dec 2019 11:46:21 -0800 Subject: [PATCH] Prevent `mem_replace_with_default` lint within macros Also added test cases for internal and external macros. --- clippy_lints/src/mem_replace.rs | 8 ++++---- tests/ui/auxiliary/macro_rules.rs | 7 +++++++ tests/ui/mem_replace.fixed | 14 ++++++++++++++ tests/ui/mem_replace.rs | 14 ++++++++++++++ tests/ui/mem_replace.stderr | 10 +++++----- 5 files changed, 44 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs index 5ad41a53b89..ab0bdb4d02c 100644 --- 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) { - 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 index 504d6733abf..eafc68bd6bc 100644 --- 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 index 58657b934fb..8606e98335d 100644 --- 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 index eac0ce586a0..c116107a923 100644 --- 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 index d5dc66873b2..9c925cefb04 100644 --- a/tests/ui/mem_replace.stderr +++ b/tests/ui/mem_replace.stderr @@ -1,5 +1,5 @@ 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()` @@ -7,13 +7,13 @@ LL | let _ = mem::replace(&mut an_option, None); = 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)` @@ -21,13 +21,13 @@ LL | let _ = std::mem::replace(&mut s, String::default()); = 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)`