Skip to content

Commit

Permalink
Auto merge of #11019 - Centri3:ptr_arg, r=llogiq
Browse files Browse the repository at this point in the history 
[`ptr_arg`]: Don't lint when return type uses `Cow`'s lifetime

Closes #9218

changelog: [`ptr_arg`]: Don't lint when return type uses `Cow`'s lifetime
  • Loading branch information
@bors
bors committed Jun 24, 2023
2 parents ce0a48a + db4efe3 commit 3cee98d
Show file tree
Hide file tree
Showing 3 changed files with 141 additions and 57 deletions.
105 changes: 70 additions & 35 deletions clippy_lints/src/ptr.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use clippy_utils::source::snippet_opt;
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, paths};
use hir::LifetimeName;
use if_chain::if_chain;
use rustc_errors::{Applicability, MultiSpan};
use rustc_hir::def_id::DefId;
Expand All @@ -15,6 +16,7 @@ use rustc_hir::{
ImplItemKind, ItemKind, Lifetime, Mutability, Node, Param, PatKind, QPath, TraitFn, TraitItem, TraitItemKind,
TyKind, Unsafety,
};
use rustc_hir_analysis::hir_ty_to_ty;
use rustc_infer::infer::TyCtxtInferExt;
use rustc_infer::traits::{Obligation, ObligationCause};
use rustc_lint::{LateContext, LateLintPass};
Expand Down Expand Up @@ -166,6 +168,7 @@ impl<'tcx> LateLintPass<'tcx> for Ptr {
cx,
cx.tcx.fn_sig(item.owner_id).subst_identity().skip_binder().inputs(),
sig.decl.inputs,
&sig.decl.output,
&[],
)
.filter(|arg| arg.mutability() == Mutability::Not)
Expand Down Expand Up @@ -218,7 +221,7 @@ impl<'tcx> LateLintPass<'tcx> for Ptr {
check_mut_from_ref(cx, sig, Some(body));
let decl = sig.decl;
let sig = cx.tcx.fn_sig(item_id).subst_identity().skip_binder();
let lint_args: Vec<_> = check_fn_args(cx, sig.inputs(), decl.inputs, body.params)
let lint_args: Vec<_> = check_fn_args(cx, sig.inputs(), decl.inputs, &decl.output, body.params)
.filter(|arg| !is_trait_item || arg.mutability() == Mutability::Not)
.collect();
let results = check_ptr_arg_usage(cx, body, &lint_args);
Expand Down Expand Up @@ -407,29 +410,27 @@ impl<'tcx> DerefTy<'tcx> {
}
}

#[expect(clippy::too_many_lines)]
fn check_fn_args<'cx, 'tcx: 'cx>(
cx: &'cx LateContext<'tcx>,
tys: &'tcx [Ty<'tcx>],
hir_tys: &'tcx [hir::Ty<'tcx>],
ret_ty: &'tcx FnRetTy<'tcx>,
params: &'tcx [Param<'tcx>],
) -> impl Iterator<Item = PtrArg<'tcx>> + 'cx {
tys.iter()
.zip(hir_tys.iter())
.enumerate()
.filter_map(|(i, (ty, hir_ty))| {
if_chain! {
if let ty::Ref(_, ty, mutability) = *ty.kind();
if let ty::Adt(adt, substs) = *ty.kind();

if let TyKind::Ref(lt, ref ty) = hir_ty.kind;
if let TyKind::Path(QPath::Resolved(None, path)) = ty.ty.kind;

.filter_map(move |(i, (ty, hir_ty))| {
if let ty::Ref(_, ty, mutability) = *ty.kind()
&& let ty::Adt(adt, substs) = *ty.kind()
&& let TyKind::Ref(lt, ref ty) = hir_ty.kind
&& let TyKind::Path(QPath::Resolved(None, path)) = ty.ty.kind
// Check that the name as typed matches the actual name of the type.
// e.g. `fn foo(_: &Foo)` shouldn't trigger the lint when `Foo` is an alias for `Vec`
if let [.., name] = path.segments;
if cx.tcx.item_name(adt.did()) == name.ident.name;

then {
&& let [.., name] = path.segments
&& cx.tcx.item_name(adt.did()) == name.ident.name
{
let emission_id = params.get(i).map_or(hir_ty.hir_id, |param| param.hir_id);
let (method_renames, deref_ty) = match cx.tcx.get_diagnostic_name(adt.did()) {
Some(sym::Vec) => (
Expand All @@ -454,30 +455,65 @@ fn check_fn_args<'cx, 'tcx: 'cx>(
DerefTy::Path,
),
Some(sym::Cow) if mutability == Mutability::Not => {
let ty_name = name.args
if let Some((lifetime, ty)) = name.args
.and_then(|args| {
args.args.iter().find_map(|a| match a {
GenericArg::Type(x) => Some(x),
_ => None,
})
if let [GenericArg::Lifetime(lifetime), ty] = args.args {
return Some((lifetime, ty));
}
None
})
.and_then(|arg| snippet_opt(cx, arg.span))
.unwrap_or_else(|| substs.type_at(1).to_string());
span_lint_hir_and_then(
cx,
PTR_ARG,
emission_id,
hir_ty.span,
"using a reference to `Cow` is not recommended",
|diag| {
diag.span_suggestion(
hir_ty.span,
"change this to",
format!("&{}{ty_name}", mutability.prefix_str()),
Applicability::Unspecified,
);
{
if !lifetime.is_anonymous()
&& let FnRetTy::Return(ret_ty) = ret_ty
&& let ret_ty = hir_ty_to_ty(cx.tcx, ret_ty)
&& ret_ty
.walk()
.filter_map(|arg| {
arg.as_region().and_then(|lifetime| {
match lifetime.kind() {
ty::ReEarlyBound(r) => Some(r.def_id),
ty::ReLateBound(_, r) => r.kind.get_id(),
ty::ReFree(r) => r.bound_region.get_id(),
ty::ReStatic
| ty::ReVar(_)
| ty::RePlaceholder(_)
| ty::ReErased
| ty::ReError(_) => None,
}
})
})
.any(|def_id| {
matches!(
lifetime.res,
LifetimeName::Param(param_def_id) if def_id
.as_local()
.is_some_and(|def_id| def_id == param_def_id),
)
})
{
// `&Cow<'a, T>` when the return type uses 'a is okay
return None;
}
);

let ty_name =
snippet_opt(cx, ty.span()).unwrap_or_else(|| substs.type_at(1).to_string());

span_lint_hir_and_then(
cx,
PTR_ARG,
emission_id,
hir_ty.span,
"using a reference to `Cow` is not recommended",
|diag| {
diag.span_suggestion(
hir_ty.span,
"change this to",
format!("&{}{ty_name}", mutability.prefix_str()),
Applicability::Unspecified,
);
}
);
}
return None;
},
_ => return None,
Expand All @@ -495,7 +531,6 @@ fn check_fn_args<'cx, 'tcx: 'cx>(
},
deref_ty,
});
}
}
None
})
Expand Down
33 changes: 32 additions & 1 deletion tests/ui/ptr_arg.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,10 @@
#![feature(lint_reasons)]
#![allow(unused, clippy::many_single_char_names, clippy::redundant_clone)]
#![allow(
unused,
clippy::many_single_char_names,
clippy::needless_lifetimes,
clippy::redundant_clone
)]
#![warn(clippy::ptr_arg)]

use std::borrow::Cow;
Expand Down Expand Up @@ -235,3 +240,29 @@ fn dyn_trait(a: &mut Vec<u32>, b: &mut String, c: &mut PathBuf) {
takes_dyn(b);
takes_dyn(c);
}

mod issue_9218 {
use std::borrow::Cow;

fn cow_non_elided_lifetime<'a>(input: &Cow<'a, str>) -> &'a str {
todo!()
}

// This one has an anonymous lifetime so it's not okay
fn cow_elided_lifetime<'a>(input: &'a Cow<str>) -> &'a str {
todo!()
}

// These two's return types don't use use 'a so it's not okay
fn cow_bad_ret_ty_1<'a>(input: &'a Cow<'a, str>) -> &'static str {
todo!()
}
fn cow_bad_ret_ty_2<'a, 'b>(input: &'a Cow<'a, str>) -> &'b str {
todo!()
}

// Inferred to be `&'a str`, afaik.
fn cow_good_ret_ty<'a>(input: &'a Cow<'a, str>) -> &str {
todo!()
}
}
60 changes: 39 additions & 21 deletions tests/ui/ptr_arg.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,49 +1,49 @@
error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:8:14
--> $DIR/ptr_arg.rs:13:14
|
LL | fn do_vec(x: &Vec<i64>) {
| ^^^^^^^^^ help: change this to: `&[i64]`
|
= note: `-D clippy::ptr-arg` implied by `-D warnings`

error: writing `&mut Vec` instead of `&mut [_]` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:12:18
--> $DIR/ptr_arg.rs:17:18
|
LL | fn do_vec_mut(x: &mut Vec<i64>) {
| ^^^^^^^^^^^^^ help: change this to: `&mut [i64]`

error: writing `&String` instead of `&str` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:16:14
--> $DIR/ptr_arg.rs:21:14
|
LL | fn do_str(x: &String) {
| ^^^^^^^ help: change this to: `&str`

error: writing `&mut String` instead of `&mut str` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:20:18
--> $DIR/ptr_arg.rs:25:18
|
LL | fn do_str_mut(x: &mut String) {
| ^^^^^^^^^^^ help: change this to: `&mut str`

error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:24:15
--> $DIR/ptr_arg.rs:29:15
|
LL | fn do_path(x: &PathBuf) {
| ^^^^^^^^ help: change this to: `&Path`

error: writing `&mut PathBuf` instead of `&mut Path` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:28:19
--> $DIR/ptr_arg.rs:33:19
|
LL | fn do_path_mut(x: &mut PathBuf) {
| ^^^^^^^^^^^^ help: change this to: `&mut Path`

error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:36:18
--> $DIR/ptr_arg.rs:41:18
|
LL | fn do_vec(x: &Vec<i64>);
| ^^^^^^^^^ help: change this to: `&[i64]`

error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:49:14
--> $DIR/ptr_arg.rs:54:14
|
LL | fn cloned(x: &Vec<u8>) -> Vec<u8> {
| ^^^^^^^^
Expand All @@ -60,7 +60,7 @@ LL ~ x.to_owned()
|

error: writing `&String` instead of `&str` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:58:18
--> $DIR/ptr_arg.rs:63:18
|
LL | fn str_cloned(x: &String) -> String {
| ^^^^^^^
Expand All @@ -76,7 +76,7 @@ LL ~ x.to_owned()
|

error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:66:19
--> $DIR/ptr_arg.rs:71:19
|
LL | fn path_cloned(x: &PathBuf) -> PathBuf {
| ^^^^^^^^
Expand All @@ -92,7 +92,7 @@ LL ~ x.to_path_buf()
|

error: writing `&String` instead of `&str` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:74:44
--> $DIR/ptr_arg.rs:79:44
|
LL | fn false_positive_capacity(x: &Vec<u8>, y: &String) {
| ^^^^^^^
Expand All @@ -106,19 +106,19 @@ LL ~ let c = y;
|

error: using a reference to `Cow` is not recommended
--> $DIR/ptr_arg.rs:88:25
--> $DIR/ptr_arg.rs:93:25
|
LL | fn test_cow_with_ref(c: &Cow<[i32]>) {}
| ^^^^^^^^^^^ help: change this to: `&[i32]`

error: writing `&String` instead of `&str` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:117:66
--> $DIR/ptr_arg.rs:122:66
|
LL | fn some_allowed(#[allow(clippy::ptr_arg)] _v: &Vec<u32>, _s: &String) {}
| ^^^^^^^ help: change this to: `&str`

error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:146:21
--> $DIR/ptr_arg.rs:151:21
|
LL | fn foo_vec(vec: &Vec<u8>) {
| ^^^^^^^^
Expand All @@ -131,7 +131,7 @@ LL ~ let _ = vec.to_owned().clone();
|

error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:151:23
--> $DIR/ptr_arg.rs:156:23
|
LL | fn foo_path(path: &PathBuf) {
| ^^^^^^^^
Expand All @@ -144,7 +144,7 @@ LL ~ let _ = path.to_path_buf().clone();
|

error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:156:21
--> $DIR/ptr_arg.rs:161:21
|
LL | fn foo_str(str: &PathBuf) {
| ^^^^^^^^
Expand All @@ -157,28 +157,46 @@ LL ~ let _ = str.to_path_buf().clone();
|

error: writing `&mut Vec` instead of `&mut [_]` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:162:29
--> $DIR/ptr_arg.rs:167:29
|
LL | fn mut_vec_slice_methods(v: &mut Vec<u32>) {
| ^^^^^^^^^^^^^ help: change this to: `&mut [u32]`

error: writing `&mut Vec` instead of `&mut [_]` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:224:17
--> $DIR/ptr_arg.rs:229:17
|
LL | fn dyn_trait(a: &mut Vec<u32>, b: &mut String, c: &mut PathBuf) {
| ^^^^^^^^^^^^^ help: change this to: `&mut [u32]`

error: writing `&mut String` instead of `&mut str` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:224:35
--> $DIR/ptr_arg.rs:229:35
|
LL | fn dyn_trait(a: &mut Vec<u32>, b: &mut String, c: &mut PathBuf) {
| ^^^^^^^^^^^ help: change this to: `&mut str`

error: writing `&mut PathBuf` instead of `&mut Path` involves a new object where a slice will do
--> $DIR/ptr_arg.rs:224:51
--> $DIR/ptr_arg.rs:229:51
|
LL | fn dyn_trait(a: &mut Vec<u32>, b: &mut String, c: &mut PathBuf) {
| ^^^^^^^^^^^^ help: change this to: `&mut Path`

error: aborting due to 20 previous errors
error: using a reference to `Cow` is not recommended
--> $DIR/ptr_arg.rs:252:39
|
LL | fn cow_elided_lifetime<'a>(input: &'a Cow<str>) -> &'a str {
| ^^^^^^^^^^^^ help: change this to: `&str`

error: using a reference to `Cow` is not recommended
--> $DIR/ptr_arg.rs:257:36
|
LL | fn cow_bad_ret_ty_1<'a>(input: &'a Cow<'a, str>) -> &'static str {
| ^^^^^^^^^^^^^^^^ help: change this to: `&str`

error: using a reference to `Cow` is not recommended
--> $DIR/ptr_arg.rs:260:40
|
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 23 previous errors

0 comments on commit 3cee98d

Please sign in to comment.