From c09e79e226502ad2687163b9581a2034b4a7e225 Mon Sep 17 00:00:00 2001 From: Krishna Veera Reddy Date: Tue, 24 Dec 2019 07:59:35 -0800 Subject: [PATCH] Lint within internal macros without a suggestion --- clippy_lints/src/mem_replace.rs | 27 ++++++++++++++++----------- tests/ui/mem_replace.fixed | 14 -------------- tests/ui/mem_replace.rs | 14 -------------- tests/ui/mem_replace.stderr | 10 +++++----- tests/ui/mem_replace_macro.rs | 23 +++++++++++++++++++++++ tests/ui/mem_replace_macro.stderr | 13 +++++++++++++ 6 files changed, 57 insertions(+), 44 deletions(-) create mode 100644 tests/ui/mem_replace_macro.rs create mode 100644 tests/ui/mem_replace_macro.stderr diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs index 4e0d2c24f..17a088e09 100644 --- a/clippy_lints/src/mem_replace.rs +++ b/clippy_lints/src/mem_replace.rs @@ -1,5 +1,6 @@ use crate::utils::{ - in_macro, 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, snippet_with_applicability, span_help_and_lint, + span_lint_and_sugg, span_lint_and_then, }; use if_chain::if_chain; use rustc::declare_lint_pass; @@ -166,24 +167,28 @@ fn check_replace_with_uninit(cx: &LateContext<'_, '_>, src: &Expr, expr_span: Sp fn check_replace_with_default(cx: &LateContext<'_, '_>, src: &Expr, dest: &Expr, expr_span: Span) { if let ExprKind::Call(ref repl_func, _) = src.kind { if_chain! { - if !in_macro(expr_span) && !in_external_macro(cx.tcx.sess, expr_span); + if !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); then { - let mut applicability = Applicability::MachineApplicable; - - span_lint_and_sugg( + span_lint_and_then( cx, MEM_REPLACE_WITH_DEFAULT, expr_span, "replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`", - "consider using", - format!( - "std::mem::take({})", - snippet_with_applicability(cx, dest.span, "", &mut applicability) - ), - applicability, + |db| { + if !in_macro(expr_span) { + let suggestion = format!("std::mem::take({})", snippet(cx, dest.span, "")); + + db.span_suggestion( + expr_span, + "consider using", + suggestion, + Applicability::MachineApplicable + ); + } + } ); } } diff --git a/tests/ui/mem_replace.fixed b/tests/ui/mem_replace.fixed index 8606e9833..58657b934 100644 --- a/tests/ui/mem_replace.fixed +++ b/tests/ui/mem_replace.fixed @@ -8,7 +8,6 @@ // except according to those terms. // run-rustfix -// aux-build:macro_rules.rs #![allow(unused_imports)] #![warn( clippy::all, @@ -17,17 +16,8 @@ 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(); @@ -41,10 +31,6 @@ 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 c116107a9..eac0ce586 100644 --- a/tests/ui/mem_replace.rs +++ b/tests/ui/mem_replace.rs @@ -8,7 +8,6 @@ // except according to those terms. // run-rustfix -// aux-build:macro_rules.rs #![allow(unused_imports)] #![warn( clippy::all, @@ -17,17 +16,8 @@ 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); @@ -41,10 +31,6 @@ 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 9c925cefb..d5dc66873 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:33:13 + --> $DIR/mem_replace.rs:23: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:35:13 + --> $DIR/mem_replace.rs:25: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:40:13 + --> $DIR/mem_replace.rs:30: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:42:13 + --> $DIR/mem_replace.rs:32: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:43:13 + --> $DIR/mem_replace.rs:33:13 | LL | let _ = std::mem::replace(s, Default::default()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(s)` diff --git a/tests/ui/mem_replace_macro.rs b/tests/ui/mem_replace_macro.rs new file mode 100644 index 000000000..e67e01737 --- /dev/null +++ b/tests/ui/mem_replace_macro.rs @@ -0,0 +1,23 @@ +// aux-build:macro_rules.rs +#![warn(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_with_default() { + let s = &mut String::from("foo"); + take!(s); + take_external!(s); +} + +fn main() { + replace_with_default(); +} diff --git a/tests/ui/mem_replace_macro.stderr b/tests/ui/mem_replace_macro.stderr new file mode 100644 index 000000000..d7830331b --- /dev/null +++ b/tests/ui/mem_replace_macro.stderr @@ -0,0 +1,13 @@ +error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take` + --> $DIR/mem_replace_macro.rs:11:9 + | +LL | std::mem::replace($s, Default::default()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +... +LL | take!(s); + | --------- in this macro invocation + | + = note: `-D clippy::mem-replace-with-default` implied by `-D warnings` + +error: aborting due to previous error +