Look at adjusted types instead of fn signature types in ptr_arg

Author: y21

clippy_lints/src/ptr.rs

@@ -1,11 +1,9 @@
 use clippy_utils::diagnostics::{span_lint, span_lint_and_sugg, span_lint_and_then, span_lint_hir_and_then};
 use clippy_utils::source::SpanRangeExt;
-use clippy_utils::ty::expr_sig;
 use clippy_utils::visitors::contains_unsafe_block;
 use clippy_utils::{get_expr_use_or_unification_node, is_lint_allowed, path_def_id, path_to_local};
 use hir::LifetimeName;
 use rustc_errors::{Applicability, MultiSpan};
-use rustc_hir::def_id::DefId;
 use rustc_hir::hir_id::{HirId, HirIdMap};
 use rustc_hir::intravisit::{walk_expr, Visitor};
 use rustc_hir::{
@@ -323,7 +321,6 @@ struct PtrArg<'tcx> {
     idx: usize,
     emission_id: HirId,
     span: Span,
-    ty_did: DefId,
     ty_name: Symbol,
     method_renames: &'static [(&'static str, &'static str)],
     ref_prefix: RefPrefix,
@@ -411,7 +408,6 @@ impl<'tcx> DerefTy<'tcx> {
     }
 }
 
-#[expect(clippy::too_many_lines)]
 fn check_fn_args<'cx, 'tcx: 'cx>(
     cx: &'cx LateContext<'tcx>,
     fn_sig: ty::FnSig<'tcx>,
@@ -514,7 +510,6 @@ fn check_fn_args<'cx, 'tcx: 'cx>(
                     idx: i,
                     emission_id,
                     span: hir_ty.span,
-                    ty_did: adt.did(),
                     ty_name: name.ident.name,
                     method_renames,
                     ref_prefix: RefPrefix { lt: *lt, mutability },
@@ -610,65 +605,50 @@ fn check_ptr_arg_usage<'tcx>(cx: &LateContext<'tcx>, body: &Body<'tcx>, args: &[
                         set_skip_flag();
                     }
                 },
-                Some((Node::Expr(e), child_id)) => match e.kind {
-                    ExprKind::Call(f, expr_args) => {
-                        let i = expr_args.iter().position(|arg| arg.hir_id == child_id).unwrap_or(0);
-                        if expr_sig(self.cx, f).and_then(|sig| sig.input(i)).map_or(true, |ty| {
-                            match *ty.skip_binder().peel_refs().kind() {
-                                ty::Dynamic(preds, _, _) => !matches_preds(self.cx, args.deref_ty.ty(self.cx), preds),
-                                ty::Param(_) => true,
-                                ty::Adt(def, _) => def.did() == args.ty_did,
-                                _ => false,
-                            }
-                        }) {
-                            // Passed to a function taking the non-dereferenced type.
-                            set_skip_flag();
-                        }
-                    },
-                    ExprKind::MethodCall(name, self_arg, expr_args, _) => {
-                        let i = iter::once(self_arg)
-                            .chain(expr_args.iter())
-                            .position(|arg| arg.hir_id == child_id)
-                            .unwrap_or(0);
-                        if i == 0 {
-                            // Check if the method can be renamed.
-                            let name = name.ident.as_str();
-                            if let Some((_, replacement)) = args.method_renames.iter().find(|&&(x, _)| x == name) {
-                                result.replacements.push(PtrArgReplacement {
-                                    expr_span: e.span,
-                                    self_span: self_arg.span,
-                                    replacement,
-                                });
-                                return;
-                            }
+                Some((Node::Expr(use_expr), child_id)) => {
+                    if let ExprKind::Index(e, ..) = use_expr.kind
+                        && e.hir_id == child_id
+                    {
+                        // Indexing works with both owned and its dereferenced type
+                        return;
+                    }
+
+                    if let ExprKind::MethodCall(name, receiver, ..) = use_expr.kind
+                        && receiver.hir_id == child_id
+                    {
+                        let name = name.ident.as_str();
+
+                        // Check if the method can be renamed.
+                        if let Some((_, replacement)) = args.method_renames.iter().find(|&&(x, _)| x == name) {
+                            result.replacements.push(PtrArgReplacement {
+                                expr_span: use_expr.span,
+                                self_span: receiver.span,
+                                replacement,
+                            });
+                            return;
                         }
 
-                        let Some(id) = self.cx.typeck_results().type_dependent_def_id(e.hir_id) else {
-                            set_skip_flag();
+                        // Some methods exist on both `[T]` and `Vec<T>`, such as `len`, where the receiver type
+                        // doesn't coerce to a slice and our adjusted type check below isn't enough,
+                        // but it would still be valid to call with a slice
+                        if is_allowed_vec_method(self.cx, use_expr) {
                             return;
-                        };
-
-                        match *self.cx.tcx.fn_sig(id).instantiate_identity().skip_binder().inputs()[i]
-                            .peel_refs()
-                            .kind()
-                        {
-                            ty::Dynamic(preds, _, _) if !matches_preds(self.cx, args.deref_ty.ty(self.cx), preds) => {
-                                set_skip_flag();
-                            },
-                            ty::Param(_) => {
-                                set_skip_flag();
-                            },
-                            // If the types match check for methods which exist on both types. e.g. `Vec::len` and
-                            // `slice::len`
-                            ty::Adt(def, _) if def.did() == args.ty_did && !is_allowed_vec_method(self.cx, e) => {
-                                set_skip_flag();
-                            },
-                            _ => (),
                         }
-                    },
-                    // Indexing is fine for currently supported types.
-                    ExprKind::Index(e, _, _) if e.hir_id == child_id => (),
-                    _ => set_skip_flag(),
+                    }
+
+                    let deref_ty = args.deref_ty.ty(self.cx);
+                    let adjusted_ty = self.cx.typeck_results().expr_ty_adjusted(e).peel_refs();
+                    if adjusted_ty == deref_ty {
+                        return;
+                    }
+
+                    if let ty::Dynamic(preds, ..) = adjusted_ty.kind()
+                        && matches_preds(self.cx, deref_ty, preds)
+                    {
+                        return;
+                    }
+
+                    set_skip_flag();
                 },
                 _ => set_skip_flag(),
             }

tests/ui/ptr_arg.rs

@@ -309,3 +309,25 @@ mod issue_11181 {
         extern "C" fn allowed(_v: &Vec<u32>) {}
     }
 }
+
+mod issue_13308 {
+    use std::ops::Deref;
+
+    fn repro(source: &str, destination: &mut String) {
+        source.clone_into(destination);
+    }
+    fn repro2(source: &str, destination: &mut String) {
+        ToOwned::clone_into(source, destination);
+    }
+
+    fn h1(_: &<String as Deref>::Target) {}
+    fn h2<T: Deref>(_: T, _: &T::Target) {}
+
+    // Other cases that are still ok to lint and ideally shouldn't regress
+    fn good(v1: &String, v2: &String) {
+        //~^ ERROR: writing `&String` instead of `&str`
+        //~^^ ERROR: writing `&String` instead of `&str`
+        h1(v1);
+        h2(String::new(), v2);
+    }
+}

tests/ui/ptr_arg.stderr

@@ -221,5 +221,17 @@ error: using a reference to `Cow` is not recommended
 LL |     fn cow_bad_ret_ty_2<'a, 'b>(input: &'a Cow<'a, str>) -> &'b str {
    |                                        ^^^^^^^^^^^^^^^^ help: change this to: `&str`
 
-error: aborting due to 25 previous errors
+error: writing `&String` instead of `&str` involves a new object where a slice will do
+  --> tests/ui/ptr_arg.rs:327:17
+   |
+LL |     fn good(v1: &String, v2: &String) {
+   |                 ^^^^^^^ help: change this to: `&str`
+
+error: writing `&String` instead of `&str` involves a new object where a slice will do
+  --> tests/ui/ptr_arg.rs:327:30
+   |
+LL |     fn good(v1: &String, v2: &String) {
+   |                              ^^^^^^^ help: change this to: `&str`
+
+error: aborting due to 27 previous errors