dnrops
/
rust-clippy
mirror of https://github.com/rust-lang/rust-clippy.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Auto merge of #10594 - J-ZhengLi:issue9824, r=Jarcho 

fix [`mem_replace_option_with_none`] not considering field variables

fixes: #9824

---

changelog: fix [`mem_replace_option_with_none`] not considering field variables
This commit is contained in:
bors 2023-04-06 14:01:08 +00:00
parent 2b05f794e7 008e07d4e8
commit de5c6d6b1e
4 changed files with 121 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

71
clippy_lints/src/mem_replace.rs
View File

@ -1,12 +1,13 @@
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
use clippy_utils::msrvs::{self, Msrv};
use clippy_utils::source::{snippet, snippet_with_applicability};
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_non_aggregate_primitive_type;
use clippy_utils::{is_default_equivalent, is_res_lang_ctor, path_res};
use clippy_utils::{is_default_equivalent, is_res_lang_ctor, path_res, peel_ref_operators};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::LangItem::OptionNone;
use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, QPath};
use rustc_hir::{Expr, ExprKind};
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::lint::in_external_macro;
use rustc_session::{declare_tool_lint, impl_lint_pass};
@ -101,40 +102,26 @@ declare_clippy_lint! {
impl_lint_pass!(MemReplace =>
[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) {
// Check that second argument is `Option::None`
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
// replacee's path.
let replaced_path = match dest.kind {
ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, replaced) => {
if let ExprKind::Path(QPath::Resolved(None, replaced_path)) = replaced.kind {
replaced_path
} else {
return;
}
},
ExprKind::Path(QPath::Resolved(None, 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_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_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
@ -200,10 +187,6 @@ fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<
if is_non_aggregate_primitive_type(expr_type) {
return;
}
// disable lint for Option since it is covered in another lint
if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) {
return;
}
if is_default_equivalent(cx, src) && !in_external_macro(cx.tcx.sess, expr_span) {
span_lint_and_then(
cx,
@ -246,11 +229,13 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace {
if let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id();
if cx.tcx.is_diagnostic_item(sym::mem_replace, def_id);
then {
check_replace_option_with_none(cx, src, dest, expr.span);
check_replace_with_uninit(cx, src, dest, expr.span);
if self.msrv.meets(msrvs::MEM_TAKE) {
// 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) {
check_replace_with_default(cx, src, dest, expr.span);
}
check_replace_with_uninit(cx, src, dest, expr.span);
}
}
}

34
tests/ui/mem_replace.fixed
View File

@ -90,3 +90,37 @@ fn msrv_1_40() {
let mut s = String::from("foo");
let _ = std::mem::take(&mut s);
}
fn issue9824() {
struct Foo<'a>(Option<&'a str>);
impl<'a> std::ops::Deref for Foo<'a> {
type Target = Option<&'a str>;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl<'a> std::ops::DerefMut for Foo<'a> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
struct Bar {
opt: Option<u8>,
val: String,
}
let mut f = Foo(Some("foo"));
let mut b = Bar {
opt: Some(1),
val: String::from("bar"),
};
// replace option with none
let _ = f.0.take();
let _ = (*f).take();
let _ = b.opt.take();
// replace with default
let _ = std::mem::take(&mut b.val);
}

34
tests/ui/mem_replace.rs
View File

@ -90,3 +90,37 @@ fn msrv_1_40() {
let mut s = String::from("foo");
let _ = std::mem::replace(&mut s, String::default());
}
fn issue9824() {
struct Foo<'a>(Option<&'a str>);
impl<'a> std::ops::Deref for Foo<'a> {
type Target = Option<&'a str>;
fn deref(&self) -> &Self::Target {
&self.0
}
}
impl<'a> std::ops::DerefMut for Foo<'a> {
fn deref_mut(&mut self) -> &mut Self::Target {
&mut self.0
}
}
struct Bar {
opt: Option<u8>,
val: String,
}
let mut f = Foo(Some("foo"));
let mut b = Bar {
opt: Some(1),
val: String::from("bar"),
};
// replace option with none
let _ = std::mem::replace(&mut f.0, None);
let _ = std::mem::replace(&mut *f, None);
let _ = std::mem::replace(&mut b.opt, None);
// replace with default
let _ = std::mem::replace(&mut b.val, String::default());
}

26
tests/ui/mem_replace.stderr
View File

@ -122,5 +122,29 @@ error: replacing a value of type `T` with `T::default()` is better expressed usi
LL | let _ = std::mem::replace(&mut s, String::default());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut s)`
error: aborting due to 20 previous errors
error: replacing an `Option` with `None`
--> $DIR/mem_replace.rs:121:13
|
LL | let _ = std::mem::replace(&mut f.0, None);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `f.0.take()`
error: replacing an `Option` with `None`
--> $DIR/mem_replace.rs:122:13
|
LL | let _ = std::mem::replace(&mut *f, None);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `(*f).take()`
error: replacing an `Option` with `None`
--> $DIR/mem_replace.rs:123:13
|
LL | let _ = std::mem::replace(&mut b.opt, None);
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `b.opt.take()`
error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
--> $DIR/mem_replace.rs:125:13
|
LL | let _ = std::mem::replace(&mut b.val, String::default());
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut b.val)`
error: aborting due to 24 previous errors