Fix manual_inspect to consider mutability

SpriteOvO · SpriteOvO · commit d45f4ef763e9 · 2024-10-27T02:49:48.000+01:00
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
@@ -269,7 +269,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
                     RefOp::Deref if use_cx.same_ctxt => {
                         let use_node = use_cx.use_node(cx);
                         let sub_ty = typeck.expr_ty(sub_expr);
-                        if let ExprUseNode::FieldAccess(name) = use_node
+                        if let ExprUseNode::FieldAccess(_, name) = use_node
                             && !use_cx.moved_before_use
                             && !ty_contains_field(sub_ty, name.name)
                         {
@@ -338,7 +338,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
                             TyCoercionStability::for_defined_ty(cx, ty, use_node.is_return())
                         });
                         let can_auto_borrow = match use_node {
-                            ExprUseNode::FieldAccess(_)
+                            ExprUseNode::FieldAccess(_, _)
                                 if !use_cx.moved_before_use && matches!(sub_expr.kind, ExprKind::Field(..)) =>
                             {
                                 // `DerefMut` will not be automatically applied to `ManuallyDrop<_>`
@@ -349,7 +349,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
                                 // deref through `ManuallyDrop<_>` will not compile.
                                 !adjust_derefs_manually_drop(use_cx.adjustments, expr_ty)
                             },
-                            ExprUseNode::Callee | ExprUseNode::FieldAccess(_) if !use_cx.moved_before_use => true,
+                            ExprUseNode::Callee | ExprUseNode::FieldAccess(_, _) if !use_cx.moved_before_use => true,
                             ExprUseNode::MethodArg(hir_id, _, 0) if !use_cx.moved_before_use => {
                                 // Check for calls to trait methods where the trait is implemented
                                 // on a reference.
@@ -437,7 +437,7 @@ impl<'tcx> LateLintPass<'tcx> for Dereferencing<'tcx> {
                                     count: deref_count - required_refs,
                                     msg,
                                     stability,
-                                    for_field_access: if let ExprUseNode::FieldAccess(name) = use_node
+                                    for_field_access: if let ExprUseNode::FieldAccess(_, name) = use_node
                                         && !use_cx.moved_before_use
                                     {
                                         Some(name.name)
diff --git a/clippy_lints/src/methods/manual_inspect.rs b/clippy_lints/src/methods/manual_inspect.rs
@@ -34,6 +34,7 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name:
     {
         let mut requires_copy = false;
         let mut requires_deref = false;
+        let mut has_mut_use = false;
 
         // The number of unprocessed return expressions.
         let mut ret_count = 0u32;
@@ -47,7 +48,8 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name:
                 // Nested closures don't need to treat returns specially.
                 let _: Option<!> = for_each_expr(cx, cx.tcx.hir().body(c.body).value, |e| {
                     if path_to_local_id(e, arg_id) {
-                        let (kind, same_ctxt) = check_use(cx, e);
+                        let (kind, mutbl, same_ctxt) = check_use(cx, e);
+                        has_mut_use |= mutbl.is_mut();
                         match (kind, same_ctxt && e.span.ctxt() == ctxt) {
                             (_, false) | (UseKind::Deref | UseKind::Return(..), true) => {
                                 requires_copy = true;
@@ -65,7 +67,8 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name:
             } else if matches!(e.kind, ExprKind::Ret(_)) {
                 ret_count += 1;
             } else if path_to_local_id(e, arg_id) {
-                let (kind, same_ctxt) = check_use(cx, e);
+                let (kind, mutbl, same_ctxt) = check_use(cx, e);
+                has_mut_use |= mutbl.is_mut();
                 match (kind, same_ctxt && e.span.ctxt() == ctxt) {
                     (UseKind::Return(..), false) => {
                         return ControlFlow::Break(());
@@ -161,6 +164,7 @@ pub(crate) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, arg: &Expr<'_>, name:
             && (!requires_copy || arg_ty.is_copy_modulo_regions(cx.tcx, cx.param_env))
             // This case could be handled, but a fair bit of care would need to be taken.
             && (!requires_deref || arg_ty.is_freeze(cx.tcx, cx.param_env))
+            && !has_mut_use
         {
             if requires_deref {
                 edits.push((param.span.shrink_to_lo(), "&".into()));
@@ -207,36 +211,37 @@ enum UseKind<'tcx> {
     FieldAccess(Symbol, &'tcx Expr<'tcx>),
 }
 
-/// Checks how the value is used, and whether it was used in the same `SyntaxContext`.
-fn check_use<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (UseKind<'tcx>, bool) {
+/// Checks how the value is used, mutability, and whether it was used in the same `SyntaxContext`.
+fn check_use<'tcx>(cx: &LateContext<'tcx>, e: &'tcx Expr<'_>) -> (UseKind<'tcx>, Mutability, bool) {
     let use_cx = expr_use_ctxt(cx, e);
+    let mutbl = use_cx.use_mutability(cx);
     if use_cx
         .adjustments
         .first()
         .is_some_and(|a| matches!(a.kind, Adjust::Deref(_)))
     {
-        return (UseKind::AutoBorrowed, use_cx.same_ctxt);
+        return (UseKind::AutoBorrowed, mutbl, use_cx.same_ctxt);
     }
     let res = match use_cx.use_node(cx) {
         ExprUseNode::Return(_) => {
             if let ExprKind::Ret(Some(e)) = use_cx.node.expect_expr().kind {
                 UseKind::Return(e.span)
             } else {
-                return (UseKind::Return(DUMMY_SP), false);
+                return (UseKind::Return(DUMMY_SP), mutbl, false);
             }
         },
-        ExprUseNode::FieldAccess(name) => UseKind::FieldAccess(name.name, use_cx.node.expect_expr()),
+        ExprUseNode::FieldAccess(_, name) => UseKind::FieldAccess(name.name, use_cx.node.expect_expr()),
         ExprUseNode::Callee | ExprUseNode::MethodArg(_, _, 0)
             if use_cx
                 .adjustments
                 .first()
-                .is_some_and(|a| matches!(a.kind, Adjust::Borrow(AutoBorrow::Ref(_, AutoBorrowMutability::Not)))) =>
+                .is_some_and(|a| matches!(a.kind, Adjust::Borrow(AutoBorrow::Ref(_, _)))) =>
         {
             UseKind::AutoBorrowed
         },
         ExprUseNode::Callee | ExprUseNode::MethodArg(_, _, 0) => UseKind::WillAutoDeref,
         ExprUseNode::AddrOf(BorrowKind::Ref, _) => UseKind::Borrowed(use_cx.node.expect_expr().span),
         _ => UseKind::Deref,
     };
-    (res, use_cx.same_ctxt)
+    (res, mutbl, use_cx.same_ctxt)
 }
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -91,7 +91,7 @@ use std::sync::{Mutex, MutexGuard, OnceLock};
 
 use clippy_config::types::DisallowedPath;
 use itertools::Itertools;
-use rustc_ast::ast::{self, LitKind, RangeLimits};
+use rustc_ast::ast::{self, BinOpKind, LitKind, RangeLimits};
 use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::packed::Pu128;
 use rustc_data_structures::unhash::UnhashMap;
@@ -2791,16 +2791,46 @@ impl<'tcx> ExprUseCtxt<'tcx> {
                         .position(|arg| arg.hir_id == self.child_id)
                         .map_or(0, |i| i + 1),
                 ),
-                ExprKind::Field(_, name) => ExprUseNode::FieldAccess(name),
+                ExprKind::Field(_, name) => ExprUseNode::FieldAccess(use_expr.hir_id, name),
                 ExprKind::AddrOf(kind, mutbl, _) => ExprUseNode::AddrOf(kind, mutbl),
+                ExprKind::Assign(lhs, rhs, _) => {
+                    debug_assert!(lhs.hir_id == self.child_id || rhs.hir_id == self.child_id);
+                    #[allow(clippy::bool_to_int_with_if)]
+                    let idx = if lhs.hir_id == self.child_id { 0 } else { 1 };
+                    ExprUseNode::Assign(idx)
+                },
+                ExprKind::AssignOp(op, lhs, rhs) => {
+                    debug_assert!(lhs.hir_id == self.child_id || rhs.hir_id == self.child_id);
+                    #[allow(clippy::bool_to_int_with_if)]
+                    let idx = if lhs.hir_id == self.child_id { 0 } else { 1 };
+                    ExprUseNode::AssignOp(op.node, idx)
+                },
                 _ => ExprUseNode::Other,
             },
             _ => ExprUseNode::Other,
         }
     }
+
+    pub fn use_mutability(&self, cx: &LateContext<'tcx>) -> Mutability {
+        match self.use_node(cx) {
+            ExprUseNode::FieldAccess(parent, _) => {
+                let parent = cx.tcx.hir().expect_expr(parent);
+                expr_use_ctxt(cx, parent).use_mutability(cx)
+            },
+            ExprUseNode::AssignOp(_, 0) | ExprUseNode::Assign(0) => Mutability::Mut,
+            ExprUseNode::AddrOf(_, mutbl) => mutbl,
+            ExprUseNode::FnArg(_, _) | ExprUseNode::MethodArg(_, _, _) => {
+                let child_expr = cx.tcx.hir().expect_expr(self.child_id);
+                let ty = cx.typeck_results().expr_ty_adjusted(child_expr);
+                ty.ref_mutability().unwrap_or(Mutability::Not)
+            },
+            _ => Mutability::Not,
+        }
+    }
 }
 
 /// The node which consumes a value.
+#[derive(Debug)]
 pub enum ExprUseNode<'tcx> {
     /// Assignment to, or initializer for, a local
     LetStmt(&'tcx LetStmt<'tcx>),
@@ -2817,9 +2847,13 @@ pub enum ExprUseNode<'tcx> {
     /// The callee of a function call.
     Callee,
     /// Access of a field.
-    FieldAccess(Ident),
+    FieldAccess(HirId, Ident),
     /// Borrow expression.
     AddrOf(ast::BorrowKind, Mutability),
+    /// Assignment.
+    Assign(usize),
+    /// Assignment with an operator.
+    AssignOp(BinOpKind, usize),
     Other,
 }
 impl<'tcx> ExprUseNode<'tcx> {
@@ -2896,7 +2930,13 @@ impl<'tcx> ExprUseNode<'tcx> {
                 let sig = cx.tcx.fn_sig(id).skip_binder();
                 Some(DefinedTy::Mir(cx.tcx.param_env(id).and(sig.input(i))))
             },
-            Self::LetStmt(_) | Self::FieldAccess(..) | Self::Callee | Self::Other | Self::AddrOf(..) => None,
+            Self::LetStmt(_)
+            | Self::FieldAccess(..)
+            | Self::Callee
+            | Self::Other
+            | Self::AddrOf(..)
+            | Self::Assign(_)
+            | Self::AssignOp(..) => None,
         }
     }
 }
diff --git a/tests/ui/manual_inspect.fixed b/tests/ui/manual_inspect.fixed
@@ -1,5 +1,10 @@
 #![warn(clippy::manual_inspect)]
-#![allow(clippy::no_effect, clippy::op_ref)]
+#![allow(
+    clippy::no_effect,
+    clippy::op_ref,
+    clippy::explicit_auto_deref,
+    clippy::needless_borrow
+)]
 
 fn main() {
     let _ = Some(0).inspect(|&x| {
@@ -172,3 +177,96 @@ fn main() {
         });
     }
 }
+
+fn issue_13185() {
+    struct T(u32);
+
+    impl T {
+        fn do_immut(&self) {
+            println!("meow~");
+        }
+
+        fn do_mut(&mut self) {
+            self.0 += 514;
+        }
+    }
+
+    _ = Some(T(114)).as_mut().inspect(|t| {
+        t.0 + 514;
+    });
+
+    _ = Some(T(114)).as_mut().map(|t| {
+        t.0 = 514;
+        t
+    });
+
+    _ = Some(T(114)).as_mut().map(|t| {
+        t.0 += 514;
+        t
+    });
+
+    // FIXME: It's better to lint this case
+    _ = Some(T(114)).as_mut().map(|t| {
+        let indirect = t;
+        indirect.0 + 514;
+        indirect
+    });
+
+    _ = Some(T(114)).as_mut().map(|t| {
+        let indirect = t;
+        indirect.0 += 514;
+        indirect
+    });
+
+    _ = Some(T(114)).as_mut().map(|t| {
+        t.do_mut();
+        t
+    });
+
+    _ = Some(T(114)).as_mut().inspect(|t| {
+        t.do_immut();
+    });
+
+    _ = Some(T(114)).as_mut().map(|t| {
+        T::do_mut(t);
+        t
+    });
+
+    _ = Some(T(114)).as_mut().inspect(|t| {
+        T::do_immut(t);
+    });
+
+    // FIXME: It's better to lint this case
+    _ = Some(T(114)).as_mut().map(|t| {
+        let indirect = t;
+        indirect.do_immut();
+        indirect
+    });
+
+    // FIXME: It's better to lint this case
+    _ = Some(T(114)).as_mut().map(|t| {
+        (&*t).do_immut();
+        t
+    });
+
+    // Nested fields access
+    struct N(T);
+
+    _ = Some(N(T(114))).as_mut().map(|t| {
+        t.0.do_mut();
+        t
+    });
+
+    _ = Some(N(T(114))).as_mut().inspect(|t| {
+        t.0.do_immut();
+    });
+
+    _ = Some(N(T(114))).as_mut().map(|t| {
+        T::do_mut(&mut t.0);
+        t
+    });
+
+    _ = Some(N(T(114))).as_mut().inspect(|t| {
+        T::do_immut(&t.0);
+    });
+}
diff --git a/tests/ui/manual_inspect.rs b/tests/ui/manual_inspect.rs
diff --git a/tests/ui/manual_inspect.stderr b/tests/ui/manual_inspect.stderr
