Fix manual_inspect to consider mutability

SpriteOvO · SpriteOvO · commit 0595b19d0c6b · 2025-01-15T21:33:55.000+01:00
diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs
@@ -270,7 +270,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)
                         {
@@ -339,7 +339,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<_>`
@@ -350,7 +350,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.
@@ -438,7 +438,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
@@ -6,7 +6,7 @@ use clippy_utils::visitors::{for_each_expr, for_each_expr_without_closures};
 use clippy_utils::{ExprUseNode, expr_use_ctxt, is_diag_item_method, is_diag_trait_item, path_to_local_id};
 use core::ops::ControlFlow;
 use rustc_errors::Applicability;
-use rustc_hir::{BindingMode, BorrowKind, ByRef, ClosureKind, Expr, ExprKind, Mutability, Node, PatKind};
+use rustc_hir::{BindingMode, BorrowKind, ByRef, ClosureKind, Expr, ExprKind, HirId, Mutability, Node, PatKind};
 use rustc_lint::LateContext;
 use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, AutoBorrowMutability};
 use rustc_span::{DUMMY_SP, Span, Symbol, sym};
@@ -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 || cx.type_is_copy_modulo_regions(arg_ty))
             // 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.typing_env()))
+            && !has_mut_use
         {
             if requires_deref {
                 edits.push((param.span.shrink_to_lo(), "&".into()));
@@ -207,36 +211,75 @@ 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_mutability(cx, e.hir_id);
     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)
+}
+
+fn use_mutability(cx: &LateContext<'_>, expr_id: HirId) -> Mutability {
+    let adjusted = |expr: &Expr<'_>| -> Mutability {
+        let adj = cx.typeck_results().expr_adjustments(expr);
+        if let Some(Adjust::Borrow(AutoBorrow::Ref(mutbl))) = adj.last().map(|adj| &adj.kind) {
+            (*mutbl).into()
+        } else {
+            Mutability::Not
+        }
+    };
+
+    let mut last_child = None;
+
+    for (_, node) in cx.tcx.hir().parent_iter(expr_id) {
+        if let Node::Expr(expr) = node {
+            match expr.kind {
+                ExprKind::AddrOf(_, mutbl, _) => return mutbl,
+                ExprKind::MethodCall(_, self_arg, _, _) => return adjusted(self_arg),
+                ExprKind::Call(f, args) => return adjusted(args.iter().find(|arg| arg.hir_id == expr_id).unwrap_or(f)),
+                ExprKind::Field(field, _) | ExprKind::Index(field, _, _) => last_child = Some(field.hir_id),
+                ExprKind::Assign(lhs, _, _) | ExprKind::AssignOp(_, lhs, _) => {
+                    let is_lhs = match lhs.kind {
+                        ExprKind::Field(child, _) | ExprKind::Index(child, _, _) => {
+                            last_child.is_some_and(|field_id| field_id == child.hir_id)
+                        },
+                        _ => false,
+                    };
+                    if is_lhs {
+                        return Mutability::Mut;
+                    }
+                    return Mutability::Not;
+                },
+                _ => {},
+            }
+        }
+    }
+    Mutability::Not
 }
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
@@ -2753,7 +2753,7 @@ 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),
                 _ => ExprUseNode::Other,
             },
@@ -2763,6 +2763,7 @@ impl<'tcx> ExprUseCtxt<'tcx> {
 }
 
 /// The node which consumes a value.
+#[derive(Debug)]
 pub enum ExprUseNode<'tcx> {
     /// Assignment to, or initializer for, a local
     LetStmt(&'tcx LetStmt<'tcx>),
@@ -2779,7 +2780,7 @@ 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),
     Other,
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,145 @@ fn main() {
         });
     }
 }
+
+fn issue_13185() {
+    struct T(u32);
+
+    impl T {
+        fn do_immut(&self) {
+            println!("meow~");
+        }
+
+        fn do_immut2(&self, other: &T) {
+            println!("meow~");
+        }
+
+        fn do_mut(&mut self) {
+            self.0 += 514;
+        }
+
+        fn do_mut2(&mut self, other: &mut T) {
+            self.0 += 114;
+            other.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().map(|t| {
+        T(514).do_mut2(t);
+        t
+    });
+
+    _ = Some(T(114)).as_mut().inspect(|t| {
+        t.do_immut();
+    });
+
+    _ = Some(T(114)).as_mut().inspect(|t| {
+        t.do_immut2(t);
+    });
+
+    _ = 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
+    });
+
+    // Array element access
+
+    let mut sample = Some([1919, 810]);
+    _ = sample.as_mut().map(|t| {
+        t[1] += 1;
+        t
+    });
+
+    _ = sample.as_mut().inspect(|t| {
+        let mut a = 1;
+        a += t[1]; // immut
+    });
+
+    // Nested fields access
+    struct N((T, T), [T; 2]);
+
+    let mut sample = Some(N((T(114), T(514)), [T(1919), T(810)]));
+
+    _ = sample.as_mut().map(|n| {
+        n.0.0.do_mut();
+        n
+    });
+
+    _ = sample.as_mut().inspect(|t| {
+        t.0.0.do_immut();
+    });
+
+    _ = sample.as_mut().map(|t| {
+        T::do_mut(&mut t.0.0);
+        t
+    });
+
+    _ = sample.as_mut().inspect(|t| {
+        T::do_immut(&t.0.0);
+    });
+
+    // FnMut
+    let mut state = 1;
+    let immut_fn = Some(|| {
+        println!("meow");
+    });
+    let mut mut_fn = Some(|| {
+        state += 1;
+    });
+    immut_fn.inspect(|f| {
+        f();
+    });
+    mut_fn.as_mut().map(|f| {
+        f();
+        f
+    });
+}
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
