diff --git a/CHANGELOG.md b/CHANGELOG.md index 2d37e0da37f0..ab5409aba8ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5812,6 +5812,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_option_with_some`]: https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_some [`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_ident_chars`]: https://rust-lang.github.io/rust-clippy/master/index.html#min_ident_chars diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 978fb4f3b34d..0e264cdcd4a3 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -784,6 +784,7 @@ The minimum rust version that the project supports. Defaults to the `rust-versio * [`map_unwrap_or`](https://rust-lang.github.io/rust-clippy/master/index.html#map_unwrap_or) * [`map_with_unused_argument_over_ranges`](https://rust-lang.github.io/rust-clippy/master/index.html#map_with_unused_argument_over_ranges) * [`match_like_matches_macro`](https://rust-lang.github.io/rust-clippy/master/index.html#match_like_matches_macro) +* [`mem_replace_option_with_some`](https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_option_with_some) * [`mem_replace_with_default`](https://rust-lang.github.io/rust-clippy/master/index.html#mem_replace_with_default) * [`missing_const_for_fn`](https://rust-lang.github.io/rust-clippy/master/index.html#missing_const_for_fn) * [`needless_borrow`](https://rust-lang.github.io/rust-clippy/master/index.html#needless_borrow) diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index e2f5574d5669..cac4408fff0e 100644 --- a/clippy_config/src/conf.rs +++ b/clippy_config/src/conf.rs @@ -639,6 +639,7 @@ define_Conf! { map_unwrap_or, map_with_unused_argument_over_ranges, match_like_matches_macro, + mem_replace_option_with_some, mem_replace_with_default, missing_const_for_fn, needless_borrow, diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index ac3e4bd90dc1..90be1e8876bd 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -362,6 +362,7 @@ pub static LINTS: &[&crate::LintInfo] = &[ crate::matches::WILDCARD_ENUM_MATCH_ARM_INFO, crate::matches::WILDCARD_IN_OR_PATTERNS_INFO, crate::mem_replace::MEM_REPLACE_OPTION_WITH_NONE_INFO, + crate::mem_replace::MEM_REPLACE_OPTION_WITH_SOME_INFO, crate::mem_replace::MEM_REPLACE_WITH_DEFAULT_INFO, crate::mem_replace::MEM_REPLACE_WITH_UNINIT_INFO, crate::methods::BIND_INSTEAD_OF_MAP_INFO, diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs index 41528c5dee3a..2fe5f6a3a37a 100644 --- a/clippy_lints/src/mem_replace.rs +++ b/clippy_lints/src/mem_replace.rs @@ -8,7 +8,7 @@ use clippy_utils::{ is_default_equivalent, is_expr_used_or_unified, is_res_lang_ctor, path_res, peel_ref_operators, std_or_core, }; use rustc_errors::Applicability; -use rustc_hir::LangItem::OptionNone; +use rustc_hir::LangItem::{OptionNone, OptionSome}; use rustc_hir::{Expr, ExprKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::impl_lint_pass; @@ -43,6 +43,31 @@ declare_clippy_lint! { "replacing an `Option` with `None` instead of `take()`" } +declare_clippy_lint! { + /// ### What it does + /// Checks for `mem::replace()` on an `Option` with `Some(…)`. + /// + /// ### Why is this bad? + /// `Option` already has the method `replace()` for + /// taking its current value (Some(…) or None) and replacing it with + /// `Some(…)`. + /// + /// ### Example + /// ```no_run + /// let mut an_option = Some(0); + /// let replaced = std::mem::replace(&mut an_option, Some(1)); + /// ``` + /// Is better expressed with: + /// ```no_run + /// let mut an_option = Some(0); + /// let taken = an_option.replace(1); + /// ``` + #[clippy::version = "1.86.0"] + pub MEM_REPLACE_OPTION_WITH_SOME, + style, + "replacing an `Option` with `Some` instead of `replace()`" +} + declare_clippy_lint! { /// ### What it does /// Checks for `mem::replace(&mut _, mem::uninitialized())` @@ -101,28 +126,67 @@ declare_clippy_lint! { } impl_lint_pass!(MemReplace => - [MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_WITH_UNINIT, MEM_REPLACE_WITH_DEFAULT]); + [MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_OPTION_WITH_SOME, MEM_REPLACE_WITH_UNINIT, MEM_REPLACE_WITH_DEFAULT]); -fn check_replace_option_with_none(cx: &LateContext<'_>, dest: &Expr<'_>, expr_span: Span) { - // 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 - // the replacee's expr after peeling off the `&mut` - let sugg_expr = peel_ref_operators(cx, dest); - 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()", - Sugg::hir_with_context(cx, sugg_expr, expr_span.ctxt(), "", &mut applicability).maybe_par() - ), - applicability, - ); +fn check_replace_option_with_none(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) -> bool { + if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) { + // 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 + // the replacee's expr after peeling off the `&mut` + let sugg_expr = peel_ref_operators(cx, dest); + 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()", + Sugg::hir_with_context(cx, sugg_expr, expr_span.ctxt(), "", &mut applicability).maybe_par() + ), + applicability, + ); + true + } else { + false + } +} + +fn check_replace_option_with_some( + cx: &LateContext<'_>, + src: &Expr<'_>, + dest: &Expr<'_>, + expr_span: Span, + msrv: &Msrv, +) -> bool { + if msrv.meets(msrvs::OPTION_REPLACE) + && let ExprKind::Call(src_func, [src_arg]) = src.kind + && is_res_lang_ctor(cx, path_res(cx, src_func), OptionSome) + { + // We do not have to check for a `const` context here, because `core::mem::replace()` and + // `Option::replace()` have been const-stabilized simultaneously in version 1.83.0. + let sugg_expr = peel_ref_operators(cx, dest); + let mut applicability = Applicability::MachineApplicable; + span_lint_and_sugg( + cx, + MEM_REPLACE_OPTION_WITH_SOME, + expr_span, + "replacing an `Option` with `Some(..)`", + "consider `Option::replace()` instead", + format!( + "{}.replace({})", + Sugg::hir_with_context(cx, sugg_expr, expr_span.ctxt(), "_", &mut applicability).maybe_par(), + snippet_with_applicability(cx, src_arg.span, "_", &mut applicability) + ), + applicability, + ); + true + } else { + false + } } fn check_replace_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) { @@ -181,27 +245,34 @@ fn check_replace_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<' } } -fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) { - // disable lint for primitives - let expr_type = cx.typeck_results().expr_ty_adjusted(src); - if is_non_aggregate_primitive_type(expr_type) { - return; - } - if is_default_equivalent(cx, src) && !expr_span.in_external_macro(cx.tcx.sess.source_map()) { - let Some(top_crate) = std_or_core(cx) else { return }; +fn check_replace_with_default( + cx: &LateContext<'_>, + src: &Expr<'_>, + dest: &Expr<'_>, + expr: &Expr<'_>, + msrv: &Msrv, +) -> bool { + if msrv.meets(msrvs::MEM_TAKE) && is_expr_used_or_unified(cx.tcx, expr) + // disable lint for primitives + && let expr_type = cx.typeck_results().expr_ty_adjusted(src) + && !is_non_aggregate_primitive_type(expr_type) + && is_default_equivalent(cx, src) + && !expr.span.in_external_macro(cx.tcx.sess.source_map()) + && let Some(top_crate) = std_or_core(cx) + { span_lint_and_then( cx, MEM_REPLACE_WITH_DEFAULT, - expr_span, + expr.span, format!( "replacing a value of type `T` with `T::default()` is better expressed using `{top_crate}::mem::take`" ), |diag| { - if !expr_span.from_expansion() { + if !expr.span.from_expansion() { let suggestion = format!("{top_crate}::mem::take({})", snippet(cx, dest.span, "")); diag.span_suggestion( - expr_span, + expr.span, "consider using", suggestion, Applicability::MachineApplicable, @@ -209,6 +280,9 @@ fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr< } }, ); + true + } else { + false } } @@ -233,12 +307,12 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace { && cx.tcx.is_diagnostic_item(sym::mem_replace, def_id) { // Check that second argument is `Option::None` - if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) { - check_replace_option_with_none(cx, dest, expr.span); - } else if self.msrv.meets(msrvs::MEM_TAKE) && is_expr_used_or_unified(cx.tcx, expr) { - check_replace_with_default(cx, src, dest, expr.span); + if !check_replace_option_with_none(cx, src, dest, expr.span) + && !check_replace_option_with_some(cx, src, dest, expr.span, &self.msrv) + && !check_replace_with_default(cx, src, dest, expr, &self.msrv) + { + check_replace_with_uninit(cx, src, dest, expr.span); } - check_replace_with_uninit(cx, src, dest, expr.span); } } extract_msrv_attr!(LateContext); diff --git a/clippy_utils/src/msrvs.rs b/clippy_utils/src/msrvs.rs index 87c8165eae25..2e8bcfaa7af0 100644 --- a/clippy_utils/src/msrvs.rs +++ b/clippy_utils/src/msrvs.rs @@ -58,6 +58,7 @@ msrv_aliases! { 1,35,0 { OPTION_COPIED, RANGE_CONTAINS } 1,34,0 { TRY_FROM } 1,33,0 { UNDERSCORE_IMPORTS } + 1,31,0 { OPTION_REPLACE } 1,30,0 { ITERATOR_FIND_MAP, TOOL_ATTRIBUTES } 1,29,0 { ITER_FLATTEN } 1,28,0 { FROM_BOOL, REPEAT_WITH } diff --git a/tests/ui/mem_replace.fixed b/tests/ui/mem_replace.fixed index 248ecd5d85f4..02b0d9c7045f 100644 --- a/tests/ui/mem_replace.fixed +++ b/tests/ui/mem_replace.fixed @@ -131,3 +131,25 @@ fn issue9824() { // replace with default let _ = std::mem::take(&mut b.val); } + +#[clippy::msrv = "1.31"] +fn mem_replace_option_with_some() { + let mut an_option = Some(0); + let replaced = an_option.replace(1); + //~^ ERROR: replacing an `Option` with `Some(..)` + + let mut an_option = &mut Some(0); + let replaced = an_option.replace(1); + //~^ ERROR: replacing an `Option` with `Some(..)` + + let (mut opt1, mut opt2) = (Some(0), Some(0)); + let b = true; + let replaced = (if b { &mut opt1 } else { &mut opt2 }).replace(1); + //~^ ERROR: replacing an `Option` with `Some(..)` +} + +#[clippy::msrv = "1.30"] +fn mem_replace_option_with_some_bad_msrv() { + let mut an_option = Some(0); + let replaced = mem::replace(&mut an_option, Some(1)); +} diff --git a/tests/ui/mem_replace.rs b/tests/ui/mem_replace.rs index 486d2ba1b6a5..1bb72b5de239 100644 --- a/tests/ui/mem_replace.rs +++ b/tests/ui/mem_replace.rs @@ -131,3 +131,25 @@ fn issue9824() { // replace with default let _ = std::mem::replace(&mut b.val, String::default()); } + +#[clippy::msrv = "1.31"] +fn mem_replace_option_with_some() { + let mut an_option = Some(0); + let replaced = mem::replace(&mut an_option, Some(1)); + //~^ ERROR: replacing an `Option` with `Some(..)` + + let mut an_option = &mut Some(0); + let replaced = mem::replace(an_option, Some(1)); + //~^ ERROR: replacing an `Option` with `Some(..)` + + let (mut opt1, mut opt2) = (Some(0), Some(0)); + let b = true; + let replaced = mem::replace(if b { &mut opt1 } else { &mut opt2 }, Some(1)); + //~^ ERROR: replacing an `Option` with `Some(..)` +} + +#[clippy::msrv = "1.30"] +fn mem_replace_option_with_some_bad_msrv() { + let mut an_option = Some(0); + let replaced = mem::replace(&mut an_option, Some(1)); +} diff --git a/tests/ui/mem_replace.stderr b/tests/ui/mem_replace.stderr index a60f2253d13e..42ab546a5d77 100644 --- a/tests/ui/mem_replace.stderr +++ b/tests/ui/mem_replace.stderr @@ -160,5 +160,26 @@ error: replacing a value of type `T` with `T::default()` is better expressed usi LL | let _ = std::mem::replace(&mut b.val, String::default()); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut b.val)` -error: aborting due to 26 previous errors +error: replacing an `Option` with `Some(..)` + --> tests/ui/mem_replace.rs:138:20 + | +LL | let replaced = mem::replace(&mut an_option, Some(1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::replace()` instead: `an_option.replace(1)` + | + = note: `-D clippy::mem-replace-option-with-some` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::mem_replace_option_with_some)]` + +error: replacing an `Option` with `Some(..)` + --> tests/ui/mem_replace.rs:142:20 + | +LL | let replaced = mem::replace(an_option, Some(1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::replace()` instead: `an_option.replace(1)` + +error: replacing an `Option` with `Some(..)` + --> tests/ui/mem_replace.rs:147:20 + | +LL | let replaced = mem::replace(if b { &mut opt1 } else { &mut opt2 }, Some(1)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::replace()` instead: `(if b { &mut opt1 } else { &mut opt2 }).replace(1)` + +error: aborting due to 29 previous errors