Skip to content

Fix ptr_arg suggests changes when it's actually better not to bother #15105

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 15 additions & 2 deletions clippy_lints/src/ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -584,7 +584,13 @@ fn check_ptr_arg_usage<'tcx>(cx: &LateContext<'tcx>, body: &Body<'tcx>, args: &[
Some((Node::Stmt(_), _)) => (),
Some((Node::LetStmt(l), _)) => {
// Only trace simple bindings. e.g `let x = y;`
if let PatKind::Binding(BindingMode::NONE, id, _, None) = l.pat.kind {
if let PatKind::Binding(BindingMode::NONE, id, ident, None) = l.pat.kind
// Let's not lint for the current parameter. The user may still intend to mutate
// (or, if not mutate, then perhaps call a method that's not otherwise available
// for) the referenced value behind the parameter through this local let binding
// with the underscore being only temporary.
&& !ident.name.as_str().starts_with('_')
{
self.bindings.insert(id, args_idx);
} else {
set_skip_flag();
Expand Down Expand Up @@ -650,7 +656,14 @@ fn check_ptr_arg_usage<'tcx>(cx: &LateContext<'tcx>, body: &Body<'tcx>, args: &[
.filter_map(|(i, arg)| {
let param = &body.params[arg.idx];
match param.pat.kind {
PatKind::Binding(BindingMode::NONE, id, _, None) if !is_lint_allowed(cx, PTR_ARG, param.hir_id) => {
PatKind::Binding(BindingMode::NONE, id, ident, None)
if !is_lint_allowed(cx, PTR_ARG, param.hir_id)
// Let's not lint for the current parameter. The user may still intend to mutate
// (or, if not mutate, then perhaps call a method that's not otherwise available
// for) the referenced value behind the parameter with the underscore being only
// temporary.
&& !ident.name.as_str().starts_with('_') =>
{
Some((id, i))
},
_ => {
Expand Down
140 changes: 114 additions & 26 deletions tests/ui/ptr_arg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ fn test_cow_with_ref(c: &Cow<[i32]>) {}
//~^ ptr_arg

fn test_cow(c: Cow<[i32]>) {
let _c = c;
let d = c;
}

trait Foo2 {
Expand All @@ -141,36 +141,36 @@ mod issue_5644 {
use std::path::PathBuf;

fn allowed(
#[allow(clippy::ptr_arg)] _v: &Vec<u32>,
#[allow(clippy::ptr_arg)] _s: &String,
#[allow(clippy::ptr_arg)] _p: &PathBuf,
#[allow(clippy::ptr_arg)] _c: &Cow<[i32]>,
#[expect(clippy::ptr_arg)] _expect: &Cow<[i32]>,
#[allow(clippy::ptr_arg)] v: &Vec<u32>,
#[allow(clippy::ptr_arg)] s: &String,
#[allow(clippy::ptr_arg)] p: &PathBuf,
#[allow(clippy::ptr_arg)] c: &Cow<[i32]>,
#[expect(clippy::ptr_arg)] expect: &Cow<[i32]>,
) {
}

fn some_allowed(#[allow(clippy::ptr_arg)] _v: &Vec<u32>, _s: &String) {}
fn some_allowed(#[allow(clippy::ptr_arg)] v: &Vec<u32>, s: &String) {}
//~^ ptr_arg

struct S;
impl S {
fn allowed(
#[allow(clippy::ptr_arg)] _v: &Vec<u32>,
#[allow(clippy::ptr_arg)] _s: &String,
#[allow(clippy::ptr_arg)] _p: &PathBuf,
#[allow(clippy::ptr_arg)] _c: &Cow<[i32]>,
#[expect(clippy::ptr_arg)] _expect: &Cow<[i32]>,
#[allow(clippy::ptr_arg)] v: &Vec<u32>,
#[allow(clippy::ptr_arg)] s: &String,
#[allow(clippy::ptr_arg)] p: &PathBuf,
#[allow(clippy::ptr_arg)] c: &Cow<[i32]>,
#[expect(clippy::ptr_arg)] expect: &Cow<[i32]>,
) {
}
}

trait T {
fn allowed(
#[allow(clippy::ptr_arg)] _v: &Vec<u32>,
#[allow(clippy::ptr_arg)] _s: &String,
#[allow(clippy::ptr_arg)] _p: &PathBuf,
#[allow(clippy::ptr_arg)] _c: &Cow<[i32]>,
#[expect(clippy::ptr_arg)] _expect: &Cow<[i32]>,
#[allow(clippy::ptr_arg)] v: &Vec<u32>,
#[allow(clippy::ptr_arg)] s: &String,
#[allow(clippy::ptr_arg)] p: &PathBuf,
#[allow(clippy::ptr_arg)] c: &Cow<[i32]>,
#[expect(clippy::ptr_arg)] expect: &Cow<[i32]>,
) {
}
}
Expand All @@ -182,22 +182,22 @@ mod issue6509 {
fn foo_vec(vec: &Vec<u8>) {
//~^ ptr_arg

let _ = vec.clone().pop();
let _ = vec.clone().clone();
let a = vec.clone().pop();
let b = vec.clone().clone();
}

fn foo_path(path: &PathBuf) {
//~^ ptr_arg

let _ = path.clone().pop();
let _ = path.clone().clone();
let c = path.clone().pop();
let d = path.clone().clone();
}

fn foo_str(str: &PathBuf) {
fn foo_str(str: &String) {
//~^ ptr_arg

let _ = str.clone().pop();
let _ = str.clone().clone();
let e = str.clone().pop();
let f = str.clone().clone();
}
}

Expand Down Expand Up @@ -340,8 +340,8 @@ mod issue_13308 {
ToOwned::clone_into(source, destination);
}

fn h1(_: &<String as Deref>::Target) {}
fn h2<T: Deref>(_: T, _: &T::Target) {}
fn h1(x: &<String as Deref>::Target) {}
fn h2<T: Deref>(x: T, y: &T::Target) {}

// Other cases that are still ok to lint and ideally shouldn't regress
fn good(v1: &String, v2: &String) {
Expand All @@ -352,3 +352,91 @@ mod issue_13308 {
h2(String::new(), v2);
}
}

mod issue_13489_and_13728 {
// This is a no-lint from now on.
fn foo(_x: &Vec<i32>) {
todo!();
}

// But this still gives us a lint.
fn foo_used(x: &Vec<i32>) {
//~^ ptr_arg

todo!();
}

// This is also a no-lint from now on.
fn foo_local(x: &Vec<i32>) {
let _y = x;

todo!();
}

// But this still gives us a lint.
fn foo_local_used(x: &Vec<i32>) {
//~^ ptr_arg

let y = x;

todo!();
}

// This only lints once from now on.
fn foofoo(_x: &Vec<i32>, y: &String) {
//~^ ptr_arg

todo!();
}

// And this is also a no-lint from now on.
fn foofoo_local(_x: &Vec<i32>, y: &String) {
let _z = y;

todo!();
}
}

mod issue_13489_and_13728_mut {
// This is a no-lint from now on.
fn bar(_x: &mut Vec<u32>) {
todo!()
}

// But this still gives us a lint.
fn bar_used(x: &mut Vec<u32>) {
//~^ ptr_arg

todo!()
}

// This is also a no-lint from now on.
fn bar_local(x: &mut Vec<u32>) {
let _y = x;

todo!()
}

// But this still gives us a lint.
fn bar_local_used(x: &mut Vec<u32>) {
//~^ ptr_arg

let y = x;

todo!()
}

// This only lints once from now on.
fn barbar(_x: &mut Vec<u32>, y: &mut String) {
//~^ ptr_arg

todo!()
}

// And this is also a no-lint from now on.
fn barbar_local(_x: &mut Vec<u32>, y: &mut String) {
let _z = y;

todo!()
}
}
64 changes: 50 additions & 14 deletions tests/ui/ptr_arg.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -127,10 +127,10 @@ 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
--> tests/ui/ptr_arg.rs:152:66
--> tests/ui/ptr_arg.rs:152:64
|
LL | fn some_allowed(#[allow(clippy::ptr_arg)] _v: &Vec<u32>, _s: &String) {}
| ^^^^^^^ help: change this to: `&str`
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
--> tests/ui/ptr_arg.rs:182:21
Expand All @@ -143,8 +143,8 @@ help: change this to
LL ~ fn foo_vec(vec: &[u8]) {
LL |
LL |
LL ~ let _ = vec.to_owned().pop();
LL ~ let _ = vec.to_owned().clone();
LL ~ let a = vec.to_owned().pop();
LL ~ let b = vec.to_owned().clone();
|

error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
Expand All @@ -158,23 +158,23 @@ help: change this to
LL ~ fn foo_path(path: &Path) {
LL |
LL |
LL ~ let _ = path.to_path_buf().pop();
LL ~ let _ = path.to_path_buf().clone();
LL ~ let c = path.to_path_buf().pop();
LL ~ let d = path.to_path_buf().clone();
|

error: writing `&PathBuf` instead of `&Path` involves a new object where a slice will do
error: writing `&String` instead of `&str` involves a new object where a slice will do
--> tests/ui/ptr_arg.rs:196:21
|
LL | fn foo_str(str: &PathBuf) {
| ^^^^^^^^
LL | fn foo_str(str: &String) {
| ^^^^^^^
|
help: change this to
|
LL ~ fn foo_str(str: &Path) {
LL ~ fn foo_str(str: &str) {
LL |
LL |
LL ~ let _ = str.to_path_buf().pop();
LL ~ let _ = str.to_path_buf().clone();
LL ~ let e = str.to_owned().pop();
LL ~ let f = str.to_owned().clone();
|

error: writing `&mut Vec` instead of `&mut [_]` involves a new object where a slice will do
Expand Down Expand Up @@ -231,6 +231,42 @@ error: writing `&String` instead of `&str` involves a new object where a slice w
LL | fn good(v1: &String, v2: &String) {
| ^^^^^^^ help: change this to: `&str`

error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do
--> tests/ui/ptr_arg.rs:363:20
|
LL | fn foo_used(x: &Vec<i32>) {
| ^^^^^^^^^ help: change this to: `&[i32]`

error: writing `&Vec` instead of `&[_]` involves a new object where a slice will do
--> tests/ui/ptr_arg.rs:377:26
|
LL | fn foo_local_used(x: &Vec<i32>) {
| ^^^^^^^^^ help: change this to: `&[i32]`

error: writing `&String` instead of `&str` involves a new object where a slice will do
--> tests/ui/ptr_arg.rs:386:33
|
LL | fn foofoo(_x: &Vec<i32>, y: &String) {
| ^^^^^^^ help: change this to: `&str`

error: writing `&mut Vec` instead of `&mut [_]` involves a new object where a slice will do
--> tests/ui/ptr_arg.rs:407:20
|
LL | fn bar_used(x: &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
--> tests/ui/ptr_arg.rs:421:26
|
LL | fn bar_local_used(x: &mut Vec<u32>) {
| ^^^^^^^^^^^^^ help: change this to: `&mut [u32]`

error: writing `&mut String` instead of `&mut str` involves a new object where a slice will do
--> tests/ui/ptr_arg.rs:430:37
|
LL | fn barbar(_x: &mut Vec<u32>, y: &mut String) {
| ^^^^^^^^^^^ help: change this to: `&mut str`

error: lifetime flowing from input to output with different syntax can be confusing
--> tests/ui/ptr_arg.rs:314:36
|
Expand All @@ -247,5 +283,5 @@ help: one option is to consistently use `'a`
LL | fn cow_good_ret_ty<'a>(input: &'a Cow<'a, str>) -> &'a str {
| ++

error: aborting due to 27 previous errors
error: aborting due to 33 previous errors