Skip to content
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

clippy: less aggressive needless_borrows_for_generic_args #124292

Closed
wants to merge 1 commit into from

Conversation

pacak
Copy link
Contributor

@pacak pacak commented Apr 23, 2024

Current implementation looks for significant drops, that can change the
behavior, but that's not enough - value might not have a Drop itself but
one of its children might have it.

A good example is passing a reference to PathBuf to std::fs::File::open.
There's no benefits to pass PathBuf by value, but since clippy can't
see Drop on Vec several layers down it complains forcing pass by
value and making it impossible to use the same name later.

New implementation only looks at variables and only those that are copy,
so existing variable will never be moved but things that take a string
reference created and value is created inplace &"".to_owned() will
make it to suggest to use "".to_owned() still.

Fixes rust-lang/rust-clippy#12454

@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2024

r? @jieyouxu

rustbot has assigned @jieyouxu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 23, 2024
@rustbot
Copy link
Collaborator

rustbot commented Apr 23, 2024

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

Some changes occurred in run-make tests.

cc @jieyouxu

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

Current implementation looks for significant drops, that can change the
behavior, but that's not enough - value might not have a Drop itself but
one of its children might have it.

A good example is passing a reference to `PathBuf` to `std::fs::File::open`.
There's no benefits to pass `PathBuf` by value, but since clippy can't
see `Drop` on `Vec` several layers down it complains forcing pass by
value and making it impossible to use the same name later.

New implementation only looks at variables and only those that are copy,
so existing variable will never be moved but things that take a string
reference created and value is created inplace `&"".to_owned()` will
make it to suggest to use `"".to_owned()` still.

Fixes rust-lang/rust-clippy#12454
@pacak pacak force-pushed the clippy-borrows-for-generic branch from e544aac to 9e29a30 Compare April 23, 2024 13:22
@pacak
Copy link
Contributor Author

pacak commented Apr 23, 2024

Oops, included a commit from other pull request by accident. Sorry for the noise, gone now.

@oli-obk
Copy link
Contributor

oli-obk commented Apr 23, 2024

Since this only touches clippy, you open a PR on the clippy repo. Or is there a specific reason this is opened here?

@compiler-errors

This comment was marked as duplicate.

@pacak
Copy link
Contributor Author

pacak commented Apr 23, 2024

Since this only touches clippy, you open a PR on the clippy repo. Or is there a specific reason this is opened here?

I wasn't sure where to open it :) Will reopen it against clippy.

@pacak pacak closed this Apr 23, 2024
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

FAILED TEST: tests/ui/needless_borrows_for_generic_args.rs
command: "checking tests/ui/needless_borrows_for_generic_args.fixed"

error: actual output differed from expected
Execute `cargo uibless` to update `tests/ui/needless_borrows_for_generic_args.fixed` to the actual output
--- tests/ui/needless_borrows_for_generic_args.fixed
+++ <fixed output>
 #![warn(clippy::needless_borrows_for_generic_args)]
... 7 lines skipped ...
 use std::ffi::OsStr;
 use std::ffi::OsStr;
 use std::fmt::{Debug, Display};
-use std::path::{Path, PathBuf};
+use std::path::Path;
 
... 144 lines skipped ...
 
         let x = X;
         let x = X;
-        f(&x); // Don't lint. Has significant drop
+        f(&x); // Don't lint, not copy, passed by a reference to a variable
     {
... 159 lines skipped ...
 
 
         let y = Y(X);
-        f(&y); // Don't lint. Has significant drop
+        f(&y); // Don't lint. Not copy, passed by a reference to value
 }


full stderr:

@pacak pacak deleted the clippy-borrows-for-generic branch April 23, 2024 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
6 participants