diff --git a/CHANGELOG.md b/CHANGELOG.md index 50a7f44ad8e..05d437d3598 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/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 49101695a58..abcddc88527 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, @@ -1594,6 +1595,7 @@ pub fn register_plugins(store: &mut lint::LintStore, sess: &Session, conf: &Conf store.register_group(true, "clippy::nursery", Some("clippy_nursery"), vec![ LintId::of(&attrs::EMPTY_LINE_AFTER_OUTER_ATTR), LintId::of(&fallible_impl_from::FALLIBLE_IMPL_FROM), + LintId::of(&mem_replace::MEM_REPLACE_WITH_DEFAULT), LintId::of(&missing_const_for_fn::MISSING_CONST_FOR_FN), LintId::of(&mul_add::MANUAL_MUL_ADD), LintId::of(&mutex_atomic::MUTEX_INTEGER), diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs index b7b0538cbae..5ad41a53b89 100644 --- a/clippy_lints/src/mem_replace.rs +++ b/clippy_lints/src/mem_replace.rs @@ -3,7 +3,7 @@ }; use if_chain::if_chain; use rustc::declare_lint_pass; -use rustc::hir::{BorrowKind, Expr, ExprKind, Mutability, QPath}; +use rustc::hir::{BorrowKind, Expr, ExprKind, HirVec, Mutability, QPath}; use rustc::lint::{LateContext, LateLintPass, LintArray, LintPass}; use rustc_errors::Applicability; use rustc_session::declare_tool_lint; @@ -67,8 +67,127 @@ "`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, + nursery, + "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<'_, '_>, expr: &'_ Expr, args: &HirVec) { + if let ExprKind::Path(ref replacement_qpath) = args[1].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 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, + ); + } + } +} + +fn check_replace_with_uninit(cx: &LateContext<'_, '_>, expr: &'_ Expr, args: &HirVec) { + if let ExprKind::Call(ref repl_func, ref repl_args) = 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(&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", + ); + } + } + } + } +} + +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_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(); + if match_def_path(cx, repl_def_id, &paths::DEFAULT_TRAIT_METHOD); + then { + let mut applicability = Applicability::MachineApplicable; + + span_lint_and_sugg( + 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, args[0].span, "", &mut applicability) + ), + applicability, + ); + } + } + } +} impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MemReplace { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { @@ -80,67 +199,10 @@ fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) { 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` 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, expr, &func_args); + check_replace_with_uninit(cx, expr, &func_args); + check_replace_with_default(cx, expr, &func_args); } } } diff --git a/src/lintlist/mod.rs b/src/lintlist/mod.rs index 08cbff40444..4b820250c8d 100644 --- a/src/lintlist/mod.rs +++ b/src/lintlist/mod.rs @@ -1099,6 +1099,13 @@ deprecation: None, module: "mem_replace", }, + Lint { + name: "mem_replace_with_default", + group: "nursery", + 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/mem_replace.fixed b/tests/ui/mem_replace.fixed index 4e47ac95d82..19205dc3822 100644 --- a/tests/ui/mem_replace.fixed +++ b/tests/ui/mem_replace.fixed @@ -9,13 +9,30 @@ // 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); +} + +fn main() { + replace_option_with_none(); + replace_with_default(); +} diff --git a/tests/ui/mem_replace.rs b/tests/ui/mem_replace.rs index 6824ab18e7f..97ac283abc6 100644 --- a/tests/ui/mem_replace.rs +++ b/tests/ui/mem_replace.rs @@ -9,13 +9,30 @@ // 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()); +} + +fn main() { + replace_option_with_none(); + replace_with_default(); +} diff --git a/tests/ui/mem_replace.stderr b/tests/ui/mem_replace.stderr index 791c4d71dbf..44495a973c8 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,24 @@ 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: aborting due to 4 previous errors