mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Auto merge of #12706 - pacak:less-aggressive-needless-borrows, r=dswij

Browse Source 
less aggressive needless_borrows_for_generic_args

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 copy values or values created in place     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 https://github.com/rust-lang/rust-clippy/issues/12454

changelog: [`needless_borrows_for_generic_args`]: avoid moving variables
This commit is contained in:
bors 2024-05-15 15:51:27 +00:00
commit e669d97494
4 changed files with 91 additions and 64 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

45
clippy_lints/src/needless_borrows_for_generic_args.rs
View File

@ -1,6 +1,6 @@
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::mir::{enclosing_mir, expr_local, local_assignments, used_exactly_once, PossibleBorrowerMap};
use clippy_utils::mir::PossibleBorrowerMap;
use clippy_utils::source::snippet_with_context;
use clippy_utils::ty::{implements_trait, is_copy};
use clippy_utils::{expr_use_ctxt, peel_n_hir_expr_refs, DefinedTy, ExprUseNode};
@ -11,7 +11,6 @@
use rustc_index::bit_set::BitSet;
use rustc_infer::infer::TyCtxtInferExt;
use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::mir::{Rvalue, StatementKind};
use rustc_middle::ty::{
self, ClauseKind, EarlyBinder, FnSig, GenericArg, GenericArgKind, List, ParamTy, ProjectionPredicate, Ty,
};
@ -106,7 +105,6 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
}
&& let count = needless_borrow_count(
cx,
&mut self.possible_borrowers,
fn_id,
cx.typeck_results().node_args(hir_id),
i,
@ -155,11 +153,9 @@ fn path_has_args(p: &QPath<'_>) -> bool {
/// The following constraints will be checked:
/// * The borrowed expression meets all the generic type's constraints.
/// * The generic type appears only once in the functions signature.
/// * The borrowed value will not be moved if it is used later in the function.
#[expect(clippy::too_many_arguments)]
/// * The borrowed value is Copy itself OR not a variable (created by a function call)
fn needless_borrow_count<'tcx>(
cx: &LateContext<'tcx>,
possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
fn_id: DefId,
callee_args: &'tcx List<GenericArg<'tcx>>,
arg_index: usize,
@ -234,9 +230,9 @@ fn needless_borrow_count<'tcx>(
let referent_ty = cx.typeck_results().expr_ty(referent);
if !is_copy(cx, referent_ty)
&& (referent_ty.has_significant_drop(cx.tcx, cx.param_env)
|| !referent_used_exactly_once(cx, possible_borrowers, reference))
if (!is_copy(cx, referent_ty) && !referent_ty.is_ref())
&& let ExprKind::AddrOf(_, _, inner) = reference.kind
&& !matches!(inner.kind, ExprKind::Call(..) | ExprKind::MethodCall(..))
{
return false;
}
@ -339,37 +335,6 @@ fn is_mixed_projection_predicate<'tcx>(
}
}
fn referent_used_exactly_once<'tcx>(
cx: &LateContext<'tcx>,
possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
reference: &Expr<'tcx>,
) -> bool {
if let Some(mir) = enclosing_mir(cx.tcx, reference.hir_id)
&& let Some(local) = expr_local(cx.tcx, reference)
&& let [location] = *local_assignments(mir, local).as_slice()
&& let block_data = &mir.basic_blocks[location.block]
&& let Some(statement) = block_data.statements.get(location.statement_index)
&& let StatementKind::Assign(box (_, Rvalue::Ref(_, _, place))) = statement.kind
&& !place.is_indirect_first_projection()
{
let body_owner_local_def_id = cx.tcx.hir().enclosing_body_owner(reference.hir_id);
if possible_borrowers
.last()
.map_or(true, |&(local_def_id, _)| local_def_id != body_owner_local_def_id)
{
possible_borrowers.push((body_owner_local_def_id, PossibleBorrowerMap::new(cx, mir)));
}
let possible_borrower = &mut possible_borrowers.last_mut().unwrap().1;
// If `only_borrowers` were used here, the `copyable_iterator::warn` test would fail. The reason is
// that `PossibleBorrowerVisitor::visit_terminator` considers `place.local` a possible borrower of
// itself. See the comment in that method for an explanation as to why.
possible_borrower.bounded_borrowers(&[local], &[local, place.local], place.local, location)
&& used_exactly_once(mir, place.local).unwrap_or(false)
} else {
false
}
}
// Iteratively replaces `param_ty` with `new_ty` in `args`, and similarly for each resulting
// projected type that is a type parameter. Returns `false` if replacing the types would have an
// effect on the function signature beyond substituting `new_ty` for `param_ty`.

42
tests/ui/needless_borrows_for_generic_args.fixed
View File

@ -141,8 +141,8 @@ fn main() {
let f = |arg| {
let loc = "loc".to_owned();
let _ = std::fs::write("x", &env); // Don't lint. In environment
let _ = std::fs::write("x", arg);
let _ = std::fs::write("x", loc);
let _ = std::fs::write("x", &arg);
let _ = std::fs::write("x", &loc);
};
let _ = std::fs::write("x", &env); // Don't lint. Borrowed by `f`
f(arg);
@ -158,13 +158,13 @@ fn main() {
fn f(_: impl Debug) {}
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
}
{
fn f(_: impl AsRef<str>) {}
let x = String::new();
f(x);
f(&x);
}
{
fn f(_: impl AsRef<str>) {}
@ -299,4 +299,38 @@ fn main() {
check_str(&owner.0); // Don't lint. `owner` can't be partially moved because it impl Drop
}
}
{
#[derive(Debug)]
struct X(Vec<u8>);
fn f(_: impl Debug) {}
let x = X(vec![]);
f(&x); // Don't lint, makes x unavailable later
}
{
#[derive(Debug)]
struct X;
impl Drop for X {
fn drop(&mut self) {}
}
fn f(_: impl Debug) {}
#[derive(Debug)]
struct Y(X);
let y = Y(X);
f(&y); // Don't lint. Not copy, passed by a reference to value
}
{
fn f(_: impl AsRef<str>) {}
let x = String::new();
f(&x); // Don't lint, not a copy, makes it unavailable later
f(String::new()); // Lint, makes no difference
let y = "".to_owned();
f(&y); // Don't lint
f("".to_owned()); // Lint
}
}

36
tests/ui/needless_borrows_for_generic_args.rs
View File

@ -158,7 +158,7 @@ fn drop(&mut self) {}
fn f(_: impl Debug) {}
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
}
{
fn f(_: impl AsRef<str>) {}
@ -299,4 +299,38 @@ fn test() {
check_str(&owner.0); // Don't lint. `owner` can't be partially moved because it impl Drop
}
}
{
#[derive(Debug)]
struct X(Vec<u8>);
fn f(_: impl Debug) {}
let x = X(vec![]);
f(&x); // Don't lint, makes x unavailable later
}
{
#[derive(Debug)]
struct X;
impl Drop for X {
fn drop(&mut self) {}
}
fn f(_: impl Debug) {}
#[derive(Debug)]
struct Y(X);
let y = Y(X);
f(&y); // Don't lint. Not copy, passed by a reference to value
}
{
fn f(_: impl AsRef<str>) {}
let x = String::new();
f(&x); // Don't lint, not a copy, makes it unavailable later
f(&String::new()); // Lint, makes no difference
let y = "".to_owned();
f(&y); // Don't lint
f(&"".to_owned()); // Lint
}
}

32
tests/ui/needless_borrows_for_generic_args.stderr
View File

@ -49,29 +49,23 @@ error: the borrowed expression implements the required traits
LL | let _ = Command::new("ls").args(&["-a", "-l"]).status().unwrap();
| ^^^^^^^^^^^^^ help: change this to: `["-a", "-l"]`
error: the borrowed expression implements the required traits
--> tests/ui/needless_borrows_for_generic_args.rs:144:41
|
LL | let _ = std::fs::write("x", &arg);
| ^^^^ help: change this to: `arg`
error: the borrowed expression implements the required traits
--> tests/ui/needless_borrows_for_generic_args.rs:145:41
|
LL | let _ = std::fs::write("x", &loc);
| ^^^^ help: change this to: `loc`
error: the borrowed expression implements the required traits
--> tests/ui/needless_borrows_for_generic_args.rs:167:11
|
LL | f(&x);
| ^^ help: change this to: `x`
error: the borrowed expression implements the required traits
--> tests/ui/needless_borrows_for_generic_args.rs:247:13
|
LL | foo(&a);
| ^^ help: change this to: `a`
error: aborting due to 12 previous errors
error: the borrowed expression implements the required traits
--> tests/ui/needless_borrows_for_generic_args.rs:331:11
|
LL | f(&String::new()); // Lint, makes no difference
| ^^^^^^^^^^^^^^ help: change this to: `String::new()`
error: the borrowed expression implements the required traits
--> tests/ui/needless_borrows_for_generic_args.rs:334:11
|
LL | f(&"".to_owned()); // Lint
| ^^^^^^^^^^^^^^ help: change this to: `"".to_owned()`
error: aborting due to 11 previous errors