Skip to content

Commit

Permalink
needless_borrows_for_generic_args: Fix for &mut
Browse files Browse the repository at this point in the history
This commit fixes a bug introduced in rust-lang#12706, where the behavior of the
lint has been changed, to avoid suggestions that introduce a move. The
motivation in the commit message is quite poor (if the detection for
significant drops is not sufficient because it's not transitive, the
proper fix would be to make it transitive). However, rust-lang#12454, the linked
issue, provides a good reason for the change — if the value being
borrowed is bound to a variable, then moving it will only introduce
friction into future refactorings.

Thus rust-lang#12706 changes the logic so that the lint triggers if the value
being borrowed is Copy, or is the result of a function call, simplifying
the logic to the point where analysing "is this the only use of this
value" isn't necessary.

However, said PR also introduces an undocumented carveout, where
referents that themselves are mutable references are treated as Copy,
to catch some cases that we do want to lint against. However, that is
not sound — it's possible to consume a mutable reference by moving it.

To avoid emitting false suggestions, this PR reintroduces the
referent_used_exactly_once logic and runs that check for referents that
are themselves mutable references.

Thinking about the code shape of &mut x, where x: &mut T, raises the
point that while removing the &mut outright won't work, the extra
indirection is still undesirable, and perhaps instead we should suggest
reborrowing: &mut *x. That, however, is left as possible future work.

Fixes rust-lang#12856
  • Loading branch information
meithecatte committed Jun 6, 2024
1 parent 10d1f32 commit c6cc160
Show file tree
Hide file tree
Showing 3 changed files with 59 additions and 5 deletions.
48 changes: 43 additions & 5 deletions clippy_lints/src/needless_borrows_for_generic_args.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use clippy_config::msrvs::{self, Msrv};
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::mir::PossibleBorrowerMap;
use clippy_utils::mir::{enclosing_mir, expr_local, local_assignments, used_exactly_once, 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};
Expand All @@ -11,6 +11,7 @@ use rustc_hir::{Body, Expr, ExprKind, Mutability, Path, QPath};
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, ParamTy, ProjectionPredicate, Ty,
};
Expand Down Expand Up @@ -105,6 +106,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessBorrowsForGenericArgs<'tcx> {
}
&& let count = needless_borrow_count(
cx,
&mut self.possible_borrowers,
fn_id,
cx.typeck_results().node_args(hir_id),
i,
Expand Down Expand Up @@ -153,9 +155,14 @@ 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 is Copy itself OR not a variable (created by a function call)
/// * The borrowed value is:
/// - `Copy` itself, or
/// - the only use of a mutable reference, or
/// - not a variable (created by a function call)
#[expect(clippy::too_many_arguments)]
fn needless_borrow_count<'tcx>(
cx: &LateContext<'tcx>,
possible_borrowers: &mut Vec<(LocalDefId, PossibleBorrowerMap<'tcx, 'tcx>)>,
fn_id: DefId,
callee_args: ty::GenericArgsRef<'tcx>,
arg_index: usize,
Expand Down Expand Up @@ -230,9 +237,9 @@ fn needless_borrow_count<'tcx>(

let referent_ty = cx.typeck_results().expr_ty(referent);

if (!is_copy(cx, referent_ty) && !referent_ty.is_ref())
&& let ExprKind::AddrOf(_, _, inner) = reference.kind
&& !matches!(inner.kind, ExprKind::Call(..) | ExprKind::MethodCall(..))
if !(is_copy(cx, referent_ty)
|| referent_ty.is_ref() && referent_used_exactly_once(cx, possible_borrowers, reference)
|| matches!(referent.kind, ExprKind::Call(..) | ExprKind::MethodCall(..)))
{
return false;
}
Expand Down Expand Up @@ -335,6 +342,37 @@ 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`.
Expand Down
8 changes: 8 additions & 0 deletions tests/ui/needless_borrows_for_generic_args.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -333,4 +333,12 @@ fn main() {
f(&y); // Don't lint
f("".to_owned()); // Lint
}
{
fn takes_writer<T: std::io::Write>(_: T) {}

fn f(mut buffer: &mut Vec<u8>) {
takes_writer(&mut buffer); // Don't lint, would make buffer unavailable later
buffer.extend(b"\n");
}
}
}
8 changes: 8 additions & 0 deletions tests/ui/needless_borrows_for_generic_args.rs
Original file line number Diff line number Diff line change
Expand Up @@ -333,4 +333,12 @@ fn main() {
f(&y); // Don't lint
f(&"".to_owned()); // Lint
}
{
fn takes_writer<T: std::io::Write>(_: T) {}

fn f(mut buffer: &mut Vec<u8>) {
takes_writer(&mut buffer); // Don't lint, would make buffer unavailable later
buffer.extend(b"\n");
}
}
}

0 comments on commit c6cc160

Please sign in to comment.