bjoernager/rust
bjoernager/rust
1
Fork 0
Code Issues Pull requests Activity

Detect when move of !Copy value occurs within loop and should likely not be cloned

Browse source 
When encountering a move error on a value within a loop of any kind,
identify if the moved value belongs to a call expression that should not
be cloned and avoid the semantically incorrect suggestion. Also try to
suggest moving the call expression outside of the loop instead.

```
error[E0382]: use of moved value: `vec`
  --> $DIR/recreating-value-in-loop-condition.rs:6:33
   |
LL |     let vec = vec!["one", "two", "three"];
   |         --- move occurs because `vec` has type `Vec<&str>`, which does not implement the `Copy` trait
LL |     while let Some(item) = iter(vec).next() {
   |     ----------------------------^^^--------
   |     |                           |
   |     |                           value moved here, in previous iteration of loop
   |     inside of this loop
   |
note: consider changing this parameter type in function `iter` to borrow instead if owning the value isn't necessary
  --> $DIR/recreating-value-in-loop-condition.rs:1:17
   |
LL | fn iter<T>(vec: Vec<T>) -> impl Iterator<Item = T> {
   |    ----         ^^^^^^ this parameter takes ownership of the value
   |    |
   |    in this function
help: consider moving the expression out of the loop so it is only moved once
   |
LL ~     let mut value = iter(vec);
LL ~     while let Some(item) = value.next() {
   |
```

We use the presence of a `break` in the loop that would be affected by
the moved value as a heuristic for "shouldn't be cloned".

Fix #121466.
This commit is contained in:
Esteban Küber 2024-02-26 20:06:19 +00:00
parent 22e241e32e
commit 14473adf42
13 changed files with 437 additions and 42 deletions
Download patch file Download diff file Expand all files Collapse all files

162
compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
View file

@ -447,8 +447,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
err.span_note(
span,
format!(
"consider changing this parameter type in {descr} `{ident}` to \
borrow instead if owning the value isn't necessary",
"consider changing this parameter type in {descr} `{ident}` to borrow \
instead if owning the value isn't necessary",
),
);
}
@ -747,8 +747,166 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
true
}
/// In a move error that occurs on a call wihtin a loop, we try to identify cases where cloning
/// the value would lead to a logic error. We infer these cases by seeing if the moved value is
/// part of the logic to break the loop, either through an explicit `break` or if the expression
/// is part of a `while let`.
fn suggest_hoisting_call_outside_loop(&self, err: &mut Diag<'_>, expr: &hir::Expr<'_>) -> bool {
let tcx = self.infcx.tcx;
let mut can_suggest_clone = true;
// If the moved value is a locally declared binding, we'll look upwards on the expression
// tree until the scope where it is defined, and no further, as suggesting to move the
// expression beyond that point would be illogical.
let local_hir_id = if let hir::ExprKind::Path(hir::QPath::Resolved(
_,
hir::Path { res: hir::def::Res::Local(local_hir_id), .. },
)) = expr.kind
{
Some(local_hir_id)
} else {
// This case would be if the moved value comes from an argument binding, we'll just
// look within the entire item, that's fine.
None
};
/// This will allow us to look for a specific `HirId`, in our case `local_hir_id` where the
/// binding was declared, within any other expression. We'll use it to search for the
/// binding declaration within every scope we inspect.
struct Finder {
hir_id: hir::HirId,
found: bool,
}
impl<'hir> Visitor<'hir> for Finder {
fn visit_pat(&mut self, pat: &'hir hir::Pat<'hir>) {
if pat.hir_id == self.hir_id {
self.found = true;
}
hir::intravisit::walk_pat(self, pat);
}
fn visit_expr(&mut self, ex: &'hir hir::Expr<'hir>) {
if ex.hir_id == self.hir_id {
self.found = true;
}
hir::intravisit::walk_expr(self, ex);
}
}
// The immediate HIR parent of the moved expression. We'll look for it to be a call.
let mut parent = None;
// The top-most loop where the moved expression could be moved to a new binding.
let mut outer_most_loop: Option<&hir::Expr<'_>> = None;
for (_, node) in tcx.hir().parent_iter(expr.hir_id) {
let e = match node {
hir::Node::Expr(e) => e,
hir::Node::Local(hir::Local { els: Some(els), .. }) => {
let mut finder = BreakFinder { found_break: false };
finder.visit_block(els);
if finder.found_break {
// Don't suggest clone as it could be will likely end in an infinite
// loop.
// let Some(_) = foo(non_copy.clone()) else { break; }
// --- ^^^^^^^^ -----
can_suggest_clone = false;
}
continue;
}
_ => continue,
};
if let Some(&hir_id) = local_hir_id {
let mut finder = Finder { hir_id, found: false };
finder.visit_expr(e);
if finder.found {
// The current scope includes the declaration of the binding we're accessing, we
// can't look up any further for loops.
break;
}
}
if parent.is_none() {
parent = Some(e);
}
match e.kind {
hir::ExprKind::Let(_) => {
match tcx.parent_hir_node(e.hir_id) {
hir::Node::Expr(hir::Expr {
kind: hir::ExprKind::If(cond, ..), ..
}) => {
let mut finder = Finder { hir_id: expr.hir_id, found: false };
finder.visit_expr(cond);
if finder.found {
// The expression where the move error happened is in a `while let`
// condition Don't suggest clone as it will likely end in an
// infinite loop.
// while let Some(_) = foo(non_copy.clone()) { }
// --------- ^^^^^^^^
can_suggest_clone = false;
}
}
_ => {}
}
}
hir::ExprKind::Loop(..) => {
outer_most_loop = Some(e);
}
_ => {}
}
}
/// Look for `break` expressions within any arbitrary expressions. We'll do this to infer
/// whether this is a case where the moved value would affect the exit of a loop, making it
/// unsuitable for a `.clone()` suggestion.
struct BreakFinder {
found_break: bool,
}
impl<'hir> Visitor<'hir> for BreakFinder {
fn visit_expr(&mut self, ex: &'hir hir::Expr<'hir>) {
if let hir::ExprKind::Break(..) = ex.kind {
self.found_break = true;
}
hir::intravisit::walk_expr(self, ex);
}
}
if let Some(in_loop) = outer_most_loop
&& let Some(parent) = parent
&& let hir::ExprKind::MethodCall(..) | hir::ExprKind::Call(..) = parent.kind
{
// FIXME: We could check that the call's *parent* takes `&mut val` to make the
// suggestion more targeted to the `mk_iter(val).next()` case. Maybe do that only to
// check for wheter to suggest `let value` or `let mut value`.
let span = in_loop.span;
let mut finder = BreakFinder { found_break: false };
finder.visit_expr(in_loop);
let sm = tcx.sess.source_map();
if (finder.found_break || true)
&& let Ok(value) = sm.span_to_snippet(parent.span)
{
// We know with high certainty that this move would affect the early return of a
// loop, so we suggest moving the expression with the move out of the loop.
let indent = if let Some(indent) = sm.indentation_before(span) {
format!("\n{indent}")
} else {
" ".to_string()
};
err.multipart_suggestion(
"consider moving the expression out of the loop so it is only moved once",
vec![
(parent.span, "value".to_string()),
(span.shrink_to_lo(), format!("let mut value = {value};{indent}")),
],
Applicability::MaybeIncorrect,
);
}
}
can_suggest_clone
}
fn suggest_cloning(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, expr: &hir::Expr<'_>, span: Span) {
let tcx = self.infcx.tcx;
if !self.suggest_hoisting_call_outside_loop(err, expr) {
// The place where the the type moves would be misleading to suggest clone. (#121466)
return;
}
// Try to find predicates on *generic params* that would allow copying `ty`
let suggestion =
if let Some(symbol) = tcx.hir().maybe_get_struct_pattern_shorthand_field(expr) {