user0/rust
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

New lint: mem_replace_option_with_some (#14197)

Browse source 
`mem::replace(opt, Some(v))` can be replaced by `opt.replace(v)`.

Close #14195

changelog: [`mem_replace_option_with_some`]: new lint
This commit is contained in:
llogiq 2025-02-12 19:02:06 +00:00 • committed by GitHub
commit 4129f5c824
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
9 changed files with 183 additions and 39 deletions
Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG.md
View file

@ -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

1
book/src/lint_configuration.md
View file

@ -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)

1
clippy_config/src/conf.rs
View file

@ -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,

1
clippy_lints/src/declared_lints.rs
View file

@ -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,

150
clippy_lints/src/mem_replace.rs
View file

@ -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);

1
clippy_utils/src/msrvs.rs
View file

@ -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 }

22
tests/ui/mem_replace.fixed
View file

@ -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));
}

22
tests/ui/mem_replace.rs
View file

@ -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));
}

23
tests/ui/mem_replace.stderr
View file

@ -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