Skip to content

Commit de5c6d6

Browse files
borsbors
committed
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
2 parents 2b05f79 + 008e07d commit de5c6d6

File tree

4 files changed

+121
-44
lines changed

4 files changed

+121
-44
lines changed

‎clippy_lints/src/mem_replace.rs

+28-43
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,13 @@
11
use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then};
22
use clippy_utils::msrvs::{self, Msrv};
33
use clippy_utils::source::{snippet, snippet_with_applicability};
4+
use clippy_utils::sugg::Sugg;
45
use clippy_utils::ty::is_non_aggregate_primitive_type;
5-
use clippy_utils::{is_default_equivalent, is_res_lang_ctor, path_res};
6+
use clippy_utils::{is_default_equivalent, is_res_lang_ctor, path_res, peel_ref_operators};
67
use if_chain::if_chain;
78
use rustc_errors::Applicability;
89
use rustc_hir::LangItem::OptionNone;
9-
use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, QPath};
10+
use rustc_hir::{Expr, ExprKind};
1011
use rustc_lint::{LateContext, LateLintPass};
1112
use rustc_middle::lint::in_external_macro;
1213
use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -101,40 +102,26 @@ declare_clippy_lint! {
101102
impl_lint_pass!(MemReplace =>
102103
[MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_WITH_UNINIT, MEM_REPLACE_WITH_DEFAULT]);
103104

104-
fn check_replace_option_with_none(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
105-
// Check that second argument is `Option::None`
106-
if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) {
107-
// Since this is a late pass (already type-checked),
108-
// and we already know that the second argument is an
109-
// `Option`, we do not need to check the first
110-
// argument's type. All that's left is to get
111-
// replacee's path.
112-
let replaced_path = match dest.kind {
113-
ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, replaced) => {
114-
if let ExprKind::Path(QPath::Resolved(None, replaced_path)) = replaced.kind {
115-
replaced_path
116-
} else {
117-
return;
118-
}
119-
},
120-
ExprKind::Path(QPath::Resolved(None, replaced_path)) => replaced_path,
121-
_ => return,
122-
};
123-
124-
let mut applicability = Applicability::MachineApplicable;
125-
span_lint_and_sugg(
126-
cx,
127-
MEM_REPLACE_OPTION_WITH_NONE,
128-
expr_span,
129-
"replacing an `Option` with `None`",
130-
"consider `Option::take()` instead",
131-
format!(
132-
"{}.take()",
133-
snippet_with_applicability(cx, replaced_path.span, "", &mut applicability)
134-
),
135-
applicability,
136-
);
137-
}
105+
fn check_replace_option_with_none(cx: &LateContext<'_>, dest: &Expr<'_>, expr_span: Span) {
106+
// Since this is a late pass (already type-checked),
107+
// and we already know that the second argument is an
108+
// `Option`, we do not need to check the first
109+
// argument's type. All that's left is to get
110+
// the replacee's expr after peeling off the `&mut`
111+
let sugg_expr = peel_ref_operators(cx, dest);
112+
let mut applicability = Applicability::MachineApplicable;
113+
span_lint_and_sugg(
114+
cx,
115+
MEM_REPLACE_OPTION_WITH_NONE,
116+
expr_span,
117+
"replacing an `Option` with `None`",
118+
"consider `Option::take()` instead",
119+
format!(
120+
"{}.take()",
121+
Sugg::hir_with_context(cx, sugg_expr, expr_span.ctxt(), "", &mut applicability).maybe_par()
122+
),
123+
applicability,
124+
);
138125
}
139126

140127
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<
200187
if is_non_aggregate_primitive_type(expr_type) {
201188
return;
202189
}
203-
// disable lint for Option since it is covered in another lint
204-
if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) {
205-
return;
206-
}
207190
if is_default_equivalent(cx, src) && !in_external_macro(cx.tcx.sess, expr_span) {
208191
span_lint_and_then(
209192
cx,
@@ -246,11 +229,13 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace {
246229
if let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id();
247230
if cx.tcx.is_diagnostic_item(sym::mem_replace, def_id);
248231
then {
249-
check_replace_option_with_none(cx, src, dest, expr.span);
250-
check_replace_with_uninit(cx, src, dest, expr.span);
251-
if self.msrv.meets(msrvs::MEM_TAKE) {
232+
// Check that second argument is `Option::None`
233+
if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) {
234+
check_replace_option_with_none(cx, dest, expr.span);
235+
} else if self.msrv.meets(msrvs::MEM_TAKE) {
252236
check_replace_with_default(cx, src, dest, expr.span);
253237
}
238+
check_replace_with_uninit(cx, src, dest, expr.span);
254239
}
255240
}
256241
}

‎tests/ui/mem_replace.fixed

+34
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,37 @@ fn msrv_1_40() {
9090
let mut s = String::from("foo");
9191
let _ = std::mem::take(&mut s);
9292
}
93+
94+
fn issue9824() {
95+
struct Foo<'a>(Option<&'a str>);
96+
impl<'a> std::ops::Deref for Foo<'a> {
97+
type Target = Option<&'a str>;
98+
99+
fn deref(&self) -> &Self::Target {
100+
&self.0
101+
}
102+
}
103+
impl<'a> std::ops::DerefMut for Foo<'a> {
104+
fn deref_mut(&mut self) -> &mut Self::Target {
105+
&mut self.0
106+
}
107+
}
108+
109+
struct Bar {
110+
opt: Option<u8>,
111+
val: String,
112+
}
113+
114+
let mut f = Foo(Some("foo"));
115+
let mut b = Bar {
116+
opt: Some(1),
117+
val: String::from("bar"),
118+
};
119+
120+
// replace option with none
121+
let _ = f.0.take();
122+
let _ = (*f).take();
123+
let _ = b.opt.take();
124+
// replace with default
125+
let _ = std::mem::take(&mut b.val);
126+
}

‎tests/ui/mem_replace.rs

+34
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,37 @@ fn msrv_1_40() {
9090
let mut s = String::from("foo");
9191
let _ = std::mem::replace(&mut s, String::default());
9292
}
93+
94+
fn issue9824() {
95+
struct Foo<'a>(Option<&'a str>);
96+
impl<'a> std::ops::Deref for Foo<'a> {
97+
type Target = Option<&'a str>;
98+
99+
fn deref(&self) -> &Self::Target {
100+
&self.0
101+
}
102+
}
103+
impl<'a> std::ops::DerefMut for Foo<'a> {
104+
fn deref_mut(&mut self) -> &mut Self::Target {
105+
&mut self.0
106+
}
107+
}
108+
109+
struct Bar {
110+
opt: Option<u8>,
111+
val: String,
112+
}
113+
114+
let mut f = Foo(Some("foo"));
115+
let mut b = Bar {
116+
opt: Some(1),
117+
val: String::from("bar"),
118+
};
119+
120+
// replace option with none
121+
let _ = std::mem::replace(&mut f.0, None);
122+
let _ = std::mem::replace(&mut *f, None);
123+
let _ = std::mem::replace(&mut b.opt, None);
124+
// replace with default
125+
let _ = std::mem::replace(&mut b.val, String::default());
126+
}

‎tests/ui/mem_replace.stderr

+25-1
Original file line numberDiff line numberDiff line change
@@ -122,5 +122,29 @@ error: replacing a value of type `T` with `T::default()` is better expressed usi
122122
LL | let _ = std::mem::replace(&mut s, String::default());
123123
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut s)`
124124

125-
error: aborting due to 20 previous errors
125+
error: replacing an `Option` with `None`
126+
--> $DIR/mem_replace.rs:121:13
127+
|
128+
LL | let _ = std::mem::replace(&mut f.0, None);
129+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `f.0.take()`
130+
131+
error: replacing an `Option` with `None`
132+
--> $DIR/mem_replace.rs:122:13
133+
|
134+
LL | let _ = std::mem::replace(&mut *f, None);
135+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `(*f).take()`
136+
137+
error: replacing an `Option` with `None`
138+
--> $DIR/mem_replace.rs:123:13
139+
|
140+
LL | let _ = std::mem::replace(&mut b.opt, None);
141+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `b.opt.take()`
142+
143+
error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
144+
--> $DIR/mem_replace.rs:125:13
145+
|
146+
LL | let _ = std::mem::replace(&mut b.val, String::default());
147+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut b.val)`
148+
149+
error: aborting due to 24 previous errors
126150

0 commit comments

Comments
 (0)