Skip to content

Commit ff5907a

Browse files
J-ZhengLiJ-ZhengLi
committed
fix [mem_replace_option_with_none] not considering field variables
1 parent 85d9f17 commit ff5907a

File tree

4 files changed

+86
-44
lines changed

4 files changed

+86
-44
lines changed

‎clippy_lints/src/mem_replace.rs

Lines changed: 27 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,11 @@ use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lin
22
use clippy_utils::msrvs::{self, Msrv};
33
use clippy_utils::source::{snippet, snippet_with_applicability};
44
use clippy_utils::ty::is_non_aggregate_primitive_type;
5-
use clippy_utils::{is_default_equivalent, is_res_lang_ctor, path_res};
5+
use clippy_utils::{is_default_equivalent, is_res_lang_ctor, path_res, peel_ref_operators};
66
use if_chain::if_chain;
77
use rustc_errors::Applicability;
88
use rustc_hir::LangItem::OptionNone;
9-
use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, QPath};
9+
use rustc_hir::{Expr, ExprKind};
1010
use rustc_lint::{LateContext, LateLintPass};
1111
use rustc_middle::lint::in_external_macro;
1212
use rustc_session::{declare_tool_lint, impl_lint_pass};
@@ -101,40 +101,26 @@ declare_clippy_lint! {
101101
impl_lint_pass!(MemReplace =>
102102
[MEM_REPLACE_OPTION_WITH_NONE, MEM_REPLACE_WITH_UNINIT, MEM_REPLACE_WITH_DEFAULT]);
103103

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-
}
104+
fn check_replace_option_with_none(cx: &LateContext<'_>, dest: &Expr<'_>, expr_span: Span) {
105+
// Since this is a late pass (already type-checked),
106+
// and we already know that the second argument is an
107+
// `Option`, we do not need to check the first
108+
// argument's type. All that's left is to get
109+
// the span to replacee's expr after peeling off the `&mut`
110+
let sugg_expr_span = peel_ref_operators(cx, dest).span;
111+
let mut applicability = Applicability::MachineApplicable;
112+
span_lint_and_sugg(
113+
cx,
114+
MEM_REPLACE_OPTION_WITH_NONE,
115+
expr_span,
116+
"replacing an `Option` with `None`",
117+
"consider `Option::take()` instead",
118+
format!(
119+
"{}.take()",
120+
snippet_with_applicability(cx, sugg_expr_span, "", &mut applicability)
121+
),
122+
applicability,
123+
);
138124
}
139125

140126
fn check_replace_with_uninit(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<'_>, expr_span: Span) {
@@ -200,10 +186,6 @@ fn check_replace_with_default(cx: &LateContext<'_>, src: &Expr<'_>, dest: &Expr<
200186
if is_non_aggregate_primitive_type(expr_type) {
201187
return;
202188
}
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-
}
207189
if is_default_equivalent(cx, src) && !in_external_macro(cx.tcx.sess, expr_span) {
208190
span_lint_and_then(
209191
cx,
@@ -246,11 +228,13 @@ impl<'tcx> LateLintPass<'tcx> for MemReplace {
246228
if let Some(def_id) = cx.qpath_res(func_qpath, func.hir_id).opt_def_id();
247229
if cx.tcx.is_diagnostic_item(sym::mem_replace, def_id);
248230
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) {
231+
// Check that second argument is `Option::None`
232+
if is_res_lang_ctor(cx, path_res(cx, src), OptionNone) {
233+
check_replace_option_with_none(cx, dest, expr.span);
234+
} else if self.msrv.meets(msrvs::MEM_TAKE) {
252235
check_replace_with_default(cx, src, dest, expr.span);
253236
}
237+
check_replace_with_uninit(cx, src, dest, expr.span);
254238
}
255239
}
256240
}

‎tests/ui/mem_replace.fixed

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,23 @@ 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(Option<String>);
96+
struct Bar {
97+
opt: Option<u8>,
98+
val: String,
99+
}
100+
101+
let mut f = Foo(Some(String::from("foo")));
102+
let mut b = Bar {
103+
opt: Some(1),
104+
val: String::from("bar"),
105+
};
106+
107+
// replace option with none
108+
let _ = f.0.take();
109+
let _ = b.opt.take();
110+
// replace with default
111+
let _ = std::mem::take(&mut b.val);
112+
}

‎tests/ui/mem_replace.rs

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,23 @@ 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(Option<String>);
96+
struct Bar {
97+
opt: Option<u8>,
98+
val: String,
99+
}
100+
101+
let mut f = Foo(Some(String::from("foo")));
102+
let mut b = Bar {
103+
opt: Some(1),
104+
val: String::from("bar"),
105+
};
106+
107+
// replace option with none
108+
let _ = std::mem::replace(&mut f.0, None);
109+
let _ = std::mem::replace(&mut b.opt, None);
110+
// replace with default
111+
let _ = std::mem::replace(&mut b.val, String::default());
112+
}

‎tests/ui/mem_replace.stderr

Lines changed: 19 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -122,5 +122,23 @@ 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:108: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:109:13
133+
|
134+
LL | let _ = std::mem::replace(&mut b.opt, None);
135+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider `Option::take()` instead: `b.opt.take()`
136+
137+
error: replacing a value of type `T` with `T::default()` is better expressed using `std::mem::take`
138+
--> $DIR/mem_replace.rs:111:13
139+
|
140+
LL | let _ = std::mem::replace(&mut b.val, String::default());
141+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider using: `std::mem::take(&mut b.val)`
142+
143+
error: aborting due to 23 previous errors
126144

0 commit comments

Comments
 (0)