Skip to content

Commit

Permalink
Rollup merge of #128241 - compiler-errors:clone-sugg, r=jieyouxu
Browse files Browse the repository at this point in the history
Remove logic to suggest clone of function output

I can't exactly tell, but I believe that this suggestion is operating off of a heuristic that the lifetime of a function's input is correlated with the lifetime of a function's output in such a way that cloning would fix an error. I don't think that actually manages to hit the bar of "actually provides useful suggestions" most of the time.

Specifically, I've hit false-positives due to this suggestion *twice* when fixing ICEs in the compiler, so I don't think it's worthwhile having this logic around. Neither of the two affected UI tests are actually fixed by the suggestion.
  • Loading branch information
tgross35 authored Jul 27, 2024
2 parents 9164dbd + e7eae53 commit ee25d99
Show file tree
Hide file tree
Showing 3 changed files with 14 additions and 37 deletions.
31 changes: 0 additions & 31 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1306,37 +1306,6 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, '_, 'infcx, 'tcx> {
// result of `foo(...)` won't help.
break 'outer;
}

// We're suggesting `.clone()` on an borrowed value. See if the expression we have
// is an argument to a function or method call, and try to suggest cloning the
// *result* of the call, instead of the argument. This is closest to what people
// would actually be looking for in most cases, with maybe the exception of things
// like `fn(T) -> T`, but even then it is reasonable.
let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
let mut prev = expr;
while let hir::Node::Expr(parent) = self.infcx.tcx.parent_hir_node(prev.hir_id) {
if let hir::ExprKind::Call(..) | hir::ExprKind::MethodCall(..) = parent.kind
&& let Some(call_ty) = typeck_results.node_type_opt(parent.hir_id)
&& let call_ty = call_ty.peel_refs()
&& (!call_ty
.walk()
.any(|t| matches!(t.unpack(), ty::GenericArgKind::Lifetime(_)))
|| if let ty::Alias(ty::Projection, _) = call_ty.kind() {
// FIXME: this isn't quite right with lifetimes on assoc types,
// but ignore for now. We will only suggest cloning if
// `<Ty as Trait>::Assoc: Clone`, which should keep false positives
// down to a managable ammount.
true
} else {
false
})
&& self.implements_clone(call_ty)
&& self.suggest_cloning_inner(err, call_ty, parent)
{
return;
}
prev = parent;
}
}
}
let ty = ty.peel_refs();
Expand Down
10 changes: 7 additions & 3 deletions tests/ui/associated-types/associated-types-outlives.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,14 @@ LL | drop(x);
LL | return f(y);
| - borrow later used here
|
help: consider cloning the value if the performance cost is acceptable
help: if `T` implemented `Clone`, you could clone the value
--> $DIR/associated-types-outlives.rs:17:21
|
LL | 's: loop { y = denormalise(&x).clone(); break }
| ++++++++
LL | pub fn free_and_use<T: for<'a> Foo<'a>,
| ^ consider constraining this type parameter with `Clone`
...
LL | 's: loop { y = denormalise(&x); break }
| -- you could clone this value

error: aborting due to 1 previous error

Expand Down
10 changes: 7 additions & 3 deletions tests/ui/variance/variance-issue-20533.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -73,10 +73,14 @@ LL | drop(a);
LL | drop(x);
| - borrow later used here
|
help: consider cloning the value if the performance cost is acceptable
note: if `AffineU32` implemented `Clone`, you could clone the value
--> $DIR/variance-issue-20533.rs:26:1
|
LL | let x = bat(&a).clone();
| ++++++++
LL | struct AffineU32(u32);
| ^^^^^^^^^^^^^^^^ consider implementing `Clone` for this type
...
LL | let x = bat(&a);
| -- you could clone this value

error[E0505]: cannot move out of `a` because it is borrowed
--> $DIR/variance-issue-20533.rs:59:14
Expand Down

0 comments on commit ee25d99

Please sign in to comment.