diff --git a/CHANGELOG.md b/CHANGELOG.md index 50a7f44ad8e6..05d437d3598c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1173,6 +1173,7 @@ Released 2018-09-13 [`mem_discriminant_non_enum`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_discriminant_non_enum [`mem_forget`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_forget [`mem_replace_option_with_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_none +[`mem_replace_with_default`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default [`mem_replace_with_uninit`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_uninit [`min_max`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_max [`misaligned_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#misaligned_transmute diff --git a/README.md b/README.md index 01fc20f0f27d..215ad6f56301 100644 --- a/README.md +++ b/README.md @@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your [Rust](https://github.com/rust-lang/rust) code. -[There are 342 lints included in this crate!](https://rust-lang.github.io/rust-clippy/master/index.html) +[There are 343 lints included in this crate!](https://rust-lang.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: diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8b8f6b27a95d..2cc1d8d2a21c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -599,6 +599,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf &mem_discriminant::MEM_DISCRIMINANT_NON_ENUM, &mem_forget::MEM_FORGET, &mem_replace::MEM_REPLACE_OPTION_WITH_NONE, + &mem_replace::MEM_REPLACE_WITH_DEFAULT, &mem_replace::MEM_REPLACE_WITH_UNINIT, &methods::CHARS_LAST_CMP, &methods::CHARS_NEXT_CMP, @@ -1182,6 +1183,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&matches::SINGLE_MATCH), LintId::of(&mem_discriminant::MEM_DISCRIMINANT_NON_ENUM), LintId::of(&mem_replace::MEM_REPLACE_OPTION_WITH_NONE), + LintId::of(&mem_replace::MEM_REPLACE_WITH_DEFAULT), LintId::of(&mem_replace::MEM_REPLACE_WITH_UNINIT), LintId::of(&methods::CHARS_LAST_CMP), LintId::of(&methods::CHARS_NEXT_CMP), @@ -1369,6 +1371,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf LintId::of(&matches::MATCH_WILD_ERR_ARM), LintId::of(&matches::SINGLE_MATCH), LintId::of(&mem_replace::MEM_REPLACE_OPTION_WITH_NONE), + LintId::of(&mem_replace::MEM_REPLACE_WITH_DEFAULT), LintId::of(&methods::CHARS_LAST_CMP), LintId::of(&methods::INTO_ITER_ON_REF), LintId::of(&methods::ITER_CLONED_COLLECT), diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs index b7b0538cbaee..5a1e3b73ded7 100644 --- a/clippy_lints/src/mem_replace.rs +++ b/clippy_lints/src/mem_replace.rs @@ -1,12 +1,14 @@ 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, 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; use rustc::hir::{BorrowKind, Expr, ExprKind, 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; +use syntax::source_map::Span; declare_clippy_lint! { /// **What it does:** Checks for `mem::replace()` on an `Option` with @@ -67,80 +69,145 @@ declare_clippy_lint! { "`mem::replace(&mut _, mem::uninitialized())` or `mem::replace(&mut _, mem::zeroed())`" } +declare_clippy_lint! { + /// **What it does:** Checks for `std::mem::replace` on a value of type + /// `T` with `T::default()`. + /// + /// **Why is this bad?** `std::mem` module already has the method `take` to + /// take the current value and replace it with the default value of that type. + /// + /// **Known problems:** None. + /// + /// **Example:** + /// ```rust + /// let mut text = String::from("foo"); + /// let replaced = std::mem::replace(&mut text, String::default()); + /// ``` + /// Is better expressed with: + /// ```rust + /// let mut text = String::from("foo"); + /// let taken = std::mem::take(&mut text); + /// ``` + pub MEM_REPLACE_WITH_DEFAULT, + style, + "replacing a value of type `T` with `T::default()` instead of using `std::mem::take`" +} + declare_lint_pass!(MemReplace => - [MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_WITH_UNINIT]); + [MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_WITH_UNINIT, MEM_REPLACE_WITH_DEFAULT]); + +fn check_replace_option_with_none(cx: &LateContext<'_, '_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) { + if let ExprKind::Path(ref replacement_qpath) = src.kind { + // Check that second argument is `Option::None` + if match_qpath(replacement_qpath, &paths::OPTION_NONE) { + // 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 dest.kind { + ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, ref replaced) => { + if let ExprKind::Path(QPath::Resolved(None, ref replaced_path)) = replaced.kind { + replaced_path + } else { + return; + } + }, + ExprKind::Path(QPath::Resolved(None, ref replaced_path)) => replaced_path, + _ => return, + }; + + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + MEM_REPLACE_OPTION_WITH_NONE, + expr_span, + "replacing an `Option` with `None`", + "consider `Option::take()` instead", + format!( + "{}.take()", + snippet_with_applicability(cx, replaced_path.span, "", &mut applicability) + ), + applicability, + ); + } + } +} + +fn check_replace_with_uninit(cx: &LateContext<'_, '_>, src: &Expr<'_>, expr_span: Span) { + if let ExprKind::Call(ref repl_func, ref repl_args) = src.kind { + if_chain! { + if repl_args.is_empty(); + 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(); + then { + if match_def_path(cx, repl_def_id, &paths::MEM_UNINITIALIZED) { + span_help_and_lint( + cx, + MEM_REPLACE_WITH_UNINIT, + expr_span, + "replacing with `mem::uninitialized()`", + "consider using the `take_mut` crate instead", + ); + } else if match_def_path(cx, repl_def_id, &paths::MEM_ZEROED) && + !cx.tables.expr_ty(src).is_primitive() { + span_help_and_lint( + cx, + MEM_REPLACE_WITH_UNINIT, + expr_span, + "replacing with `mem::zeroed()`", + "consider using a default value or the `take_mut` crate instead", + ); + } + } + } + } +} + +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_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 { + 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`", + |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 + ); + } + } + ); + } + } + } +} 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.kind; - if func_args.len() == 2; if let ExprKind::Path(ref func_qpath) = func.kind; if let Some(def_id) = cx.tables.qpath_res(func_qpath, func.hir_id).opt_def_id(); if match_def_path(cx, def_id, &paths::MEM_REPLACE); - - // Check that second argument is `Option::None` + if let [dest, src] = &**func_args; then { - if let ExprKind::Path(ref replacement_qpath) = func_args[1].kind { - if match_qpath(replacement_qpath, &paths::OPTION_NONE) { - - // 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].kind { - ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, ref replaced) => { - if let ExprKind::Path(QPath::Resolved(None, ref replaced_path)) = replaced.kind { - replaced_path - } else { - return - } - }, - ExprKind::Path(QPath::Resolved(None, ref replaced_path)) => replaced_path, - _ => return, - }; - - let mut applicability = Applicability::MachineApplicable; - span_lint_and_sugg( - cx, - MEM_REPLACE_OPTION_WITH_NONE, - expr.span, - "replacing an `Option` with `None`", - "consider `Option::take()` instead", - format!("{}.take()", snippet_with_applicability(cx, replaced_path.span, "", &mut applicability)), - applicability, - ); - } - } - if let ExprKind::Call(ref repl_func, ref repl_args) = func_args[1].kind { - if_chain! { - if repl_args.is_empty(); - 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(); - then { - if match_def_path(cx, repl_def_id, &paths::MEM_UNINITIALIZED) { - span_help_and_lint( - cx, - MEM_REPLACE_WITH_UNINIT, - expr.span, - "replacing with `mem::uninitialized()`", - "consider using the `take_mut` crate instead", - ); - } else if match_def_path(cx, repl_def_id, &paths::MEM_ZEROED) && - !cx.tables.expr_ty(&func_args[1]).is_primitive() { - span_help_and_lint( - cx, - MEM_REPLACE_WITH_UNINIT, - expr.span, - "replacing with `mem::zeroed()`", - "consider using a default value or the `take_mut` crate instead", - ); - } - } - } - } + check_replace_option_with_none(cx, src, dest, expr.span); + check_replace_with_uninit(cx, src, expr.span); + check_replace_with_default(cx, src, dest, expr.span); } } } diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 08cbff404442..cd3336bdb378 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -6,7 +6,7 @@ pub use lint::Lint; pub use lint::LINT_LEVELS; // begin lint list, do not remove this comment, it’s used in `update_lints` -pub const ALL_LINTS: [Lint; 342] = [ +pub const ALL_LINTS: [Lint; 343] = [ Lint { name: "absurd_extreme_comparisons", group: "correctness", @@ -1099,6 +1099,13 @@ pub const ALL_LINTS: [Lint; 342] = [ deprecation: None, module: "mem_replace", }, + Lint { + name: "mem_replace_with_default", + group: "style", + desc: "replacing a value of type `T` with `T::default()` instead of using `std::mem::take`", + deprecation: None, + module: "mem_replace", + }, Lint { name: "mem_replace_with_uninit", group: "correctness", diff --git a/tests/ui/auxiliary/macro_rules.rs b/tests/ui/auxiliary/macro_rules.rs index 504d6733abfe..eafc68bd6bcf 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 4e47ac95d82d..58657b934fbf 100644 --- a/tests/ui/mem_replace.fixed +++ b/tests/ui/mem_replace.fixed @@ -9,13 +9,31 @@ // run-rustfix #![allow(unused_imports)] -#![warn(clippy::all, clippy::style, clippy::mem_replace_option_with_none)] +#![warn( + clippy::all, + clippy::style, + clippy::mem_replace_option_with_none, + clippy::mem_replace_with_default +)] use std::mem; -fn main() { +fn replace_option_with_none() { let mut an_option = Some(1); let _ = an_option.take(); let an_option = &mut Some(1); let _ = an_option.take(); } + +fn replace_with_default() { + let mut s = String::from("foo"); + let _ = std::mem::take(&mut s); + let s = &mut String::from("foo"); + let _ = std::mem::take(s); + let _ = std::mem::take(s); +} + +fn main() { + replace_option_with_none(); + replace_with_default(); +} diff --git a/tests/ui/mem_replace.rs b/tests/ui/mem_replace.rs index 6824ab18e7fa..eac0ce586a08 100644 --- a/tests/ui/mem_replace.rs +++ b/tests/ui/mem_replace.rs @@ -9,13 +9,31 @@ // run-rustfix #![allow(unused_imports)] -#![warn(clippy::all, clippy::style, clippy::mem_replace_option_with_none)] +#![warn( + clippy::all, + clippy::style, + clippy::mem_replace_option_with_none, + clippy::mem_replace_with_default +)] use std::mem; -fn main() { +fn replace_option_with_none() { 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); } + +fn replace_with_default() { + let mut s = String::from("foo"); + let _ = std::mem::replace(&mut s, String::default()); + let s = &mut String::from("foo"); + let _ = std::mem::replace(s, String::default()); + let _ = std::mem::replace(s, Default::default()); +} + +fn main() { + replace_option_with_none(); + replace_with_default(); +} diff --git a/tests/ui/mem_replace.stderr b/tests/ui/mem_replace.stderr index 791c4d71dbfc..d5dc66873b2f 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:18:13 + --> $DIR/mem_replace.rs:23:13 | LL | let _ = mem::replace(&mut an_option, None); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()` @@ -7,10 +7,30 @@ 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:20:13 + --> $DIR/mem_replace.rs:25:13 | LL | let _ = mem::replace(an_option, None); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `an_option.take()` -error: aborting due to 2 previous errors +error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take` + --> $DIR/mem_replace.rs:30: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 + | +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 + | +LL | let _ = std::mem::replace(s, Default::default()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(s)` + +error: aborting due to 5 previous errors diff --git a/tests/ui/mem_replace_macro.rs b/tests/ui/mem_replace_macro.rs new file mode 100644 index 000000000000..0c09344b80d1 --- /dev/null +++ b/tests/ui/mem_replace_macro.rs @@ -0,0 +1,21 @@ +// aux-build:macro_rules.rs +#![warn(clippy::mem_replace_with_default)] + +#[macro_use] +extern crate macro_rules; + +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 000000000000..a0872b1a6bf4 --- /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:9: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 +