From 15495ebf42a2d9748ae25774a73f28c43e47fd0b Mon Sep 17 00:00:00 2001 From: y21 <30553356+y21@users.noreply.github.com> Date: Mon, 26 Aug 2024 19:24:13 +0200 Subject: [PATCH] Look at adjusted types instead of fn signature types in `ptr_arg` --- clippy_lints/src/ptr.rs | 100 ++++++++++++++++------------------------ tests/ui/ptr_arg.rs | 22 +++++++++ tests/ui/ptr_arg.stderr | 14 +++++- 3 files changed, 75 insertions(+), 61 deletions(-) diff --git a/clippy_lints/src/ptr.rs b/clippy_lints/src/ptr.rs index 7d24d25dc2fa..cd8e921347cc 100644 --- a/clippy_lints/src/ptr.rs +++ b/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`, 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(), } diff --git a/tests/ui/ptr_arg.rs b/tests/ui/ptr_arg.rs index e6ef62681212..e8b42d3b913b 100644 --- a/tests/ui/ptr_arg.rs +++ b/tests/ui/ptr_arg.rs @@ -309,3 +309,25 @@ mod issue_11181 { extern "C" fn allowed(_v: &Vec) {} } } + +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(_: &::Target) {} + fn h2(_: 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); + } +} diff --git a/tests/ui/ptr_arg.stderr b/tests/ui/ptr_arg.stderr index 4246453e64ca..1a6b3aa1f8d4 100644 --- a/tests/ui/ptr_arg.stderr +++ b/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