From 008e07d4e8e434933e1ba25a5c324a3a9d46dd7b Mon Sep 17 00:00:00 2001
From: J-ZhengLi <lizheng135@huawei.com>
Date: Thu, 6 Apr 2023 16:16:22 +0800
Subject: [PATCH] fix [mem_replace_option_with_none] not considering field
variables
---
clippy_lints/src/mem_replace.rs | 71 +++++++++++++--------------------
tests/ui/mem_replace.fixed | 34 ++++++++++++++++
tests/ui/mem_replace.rs | 34 ++++++++++++++++
tests/ui/mem_replace.stderr | 26 +++++++++++-
4 files changed, 121 insertions(+), 44 deletions(-)
diff --git a/clippy_lints/src/mem_replace.rs b/clippy_lints/src/mem_replace.rs
index 35024ec1224..8a921d4af16 100644
--- a/clippy_lints/src/mem_replace.rs
+++ b/clippy_lints/src/mem_replace.rs
@@ -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);
}
}
}
diff --git a/tests/ui/mem_replace.fixed b/tests/ui/mem_replace.fixed
index 874d5584330..7fd340173af 100644
--- a/tests/ui/mem_replace.fixed
+++ b/tests/ui/mem_replace.fixed
@@ -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);
+}
diff --git a/tests/ui/mem_replace.rs b/tests/ui/mem_replace.rs
index f4f3bff5144..fa2903addbc 100644
--- a/tests/ui/mem_replace.rs
+++ b/tests/ui/mem_replace.rs
@@ -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());
+}
diff --git a/tests/ui/mem_replace.stderr b/tests/ui/mem_replace.stderr
index caa127f76ee..58b57be7507 100644
--- a/tests/ui/mem_replace.stderr
+++ b/tests/ui/mem_replace.stderr
@@ -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