fix [mem_replace_option_with_none] not considering field variables

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

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

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());
+}

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