Skip to content

Commit

Permalink
Rollup merge of #122254 - estebank:issue-48677, r=oli-obk
Browse files Browse the repository at this point in the history
Detect calls to .clone() on T: !Clone types on borrowck errors

When encountering a lifetime error on a type that *holds* a type that doesn't implement `Clone`, explore the item's body for potential calls to `.clone()` that are only cloning the reference `&T` instead of `T` because `T: !Clone`. If we find this, suggest `T: Clone`.

```
error[E0502]: cannot borrow `*list` as mutable because it is also borrowed as immutable
  --> $DIR/clone-on-ref.rs:7:5
   |
LL |     for v in list.iter() {
   |              ---- immutable borrow occurs here
LL |         cloned_items.push(v.clone())
   |                             ------- this call doesn't do anything, the result is still `&T` because `T` doesn't implement `Clone`
LL |     }
LL |     list.push(T::default());
   |     ^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
LL |
LL |     drop(cloned_items);
   |          ------------ immutable borrow later used here
   |
help: consider further restricting this bound
   |
LL | fn foo<T: Default + Clone>(list: &mut Vec<T>) {
   |                   +++++++
```
```
error[E0505]: cannot move out of `x` because it is borrowed
  --> $DIR/clone-on-ref.rs:23:10
   |
LL | fn qux(x: A) {
   |        - binding `x` declared here
LL |     let a = &x;
   |             -- borrow of `x` occurs here
LL |     let b = a.clone();
   |               ------- this call doesn't do anything, the result is still `&A` because `A` doesn't implement `Clone`
LL |     drop(x);
   |          ^ move out of `x` occurs here
LL |
LL |     println!("{b:?}");
   |               ----- borrow later used here
   |
help: consider annotating `A` with `#[derive(Clone)]`
   |
LL + #[derive(Clone)]
LL | struct A;
   |
```

Fix #48677.
  • Loading branch information
matthiaskrgr authored Mar 15, 2024
2 parents 72d7897 + 0953608 commit 9e153cc
Show file tree
Hide file tree
Showing 15 changed files with 294 additions and 53 deletions.
3 changes: 3 additions & 0 deletions compiler/rustc_borrowck/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ borrowck_borrow_due_to_use_closure =
borrowck_borrow_due_to_use_coroutine =
borrow occurs due to use in coroutine
borrowck_calling_operator_moves =
calling this operator moves the value
borrowck_calling_operator_moves_lhs =
calling this operator moves the left-hand side
Expand Down
113 changes: 106 additions & 7 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ use rustc_hir::def::{DefKind, Res};
use rustc_hir::intravisit::{walk_block, walk_expr, Visitor};
use rustc_hir::{CoroutineDesugaring, PatField};
use rustc_hir::{CoroutineKind, CoroutineSource, LangItem};
use rustc_infer::traits::ObligationCause;
use rustc_middle::hir::nested_filter::OnlyBodies;
use rustc_middle::mir::tcx::PlaceTy;
use rustc_middle::mir::{
Expand All @@ -21,16 +20,21 @@ use rustc_middle::mir::{
PlaceRef, ProjectionElem, Rvalue, Statement, StatementKind, Terminator, TerminatorKind,
VarBindingForm,
};
use rustc_middle::ty::{self, suggest_constraining_type_params, PredicateKind, Ty, TyCtxt};
use rustc_middle::ty::{
self, suggest_constraining_type_params, PredicateKind, ToPredicate, Ty, TyCtxt,
TypeSuperVisitable, TypeVisitor,
};
use rustc_middle::util::CallKind;
use rustc_mir_dataflow::move_paths::{InitKind, MoveOutIndex, MovePathIndex};
use rustc_span::def_id::DefId;
use rustc_span::def_id::LocalDefId;
use rustc_span::hygiene::DesugaringKind;
use rustc_span::symbol::{kw, sym, Ident};
use rustc_span::{BytePos, Span, Symbol};
use rustc_trait_selection::infer::InferCtxtExt;
use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt;
use rustc_trait_selection::traits::error_reporting::FindExprBySpan;
use rustc_trait_selection::traits::ObligationCtxt;
use rustc_trait_selection::traits::{Obligation, ObligationCause, ObligationCtxt};
use std::iter;

use crate::borrow_set::TwoPhaseActivation;
Expand Down Expand Up @@ -283,7 +287,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// something that already has `Fn`-like bounds (or is a closure), so we can't
// restrict anyways.
} else {
self.suggest_adding_copy_bounds(&mut err, ty, span);
let copy_did = self.infcx.tcx.require_lang_item(LangItem::Copy, Some(span));
self.suggest_adding_bounds(&mut err, ty, copy_did, span);
}

if needs_note {
Expand Down Expand Up @@ -774,7 +779,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
}
}

fn suggest_adding_copy_bounds(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, span: Span) {
fn suggest_adding_bounds(&self, err: &mut Diag<'_>, ty: Ty<'tcx>, def_id: DefId, span: Span) {
let tcx = self.infcx.tcx;
let generics = tcx.generics_of(self.mir_def_id());

Expand All @@ -787,10 +792,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
};
// Try to find predicates on *generic params* that would allow copying `ty`
let ocx = ObligationCtxt::new(self.infcx);
let copy_did = tcx.require_lang_item(LangItem::Copy, Some(span));
let cause = ObligationCause::misc(span, self.mir_def_id());

ocx.register_bound(cause, self.param_env, ty, copy_did);
ocx.register_bound(cause, self.param_env, ty, def_id);
let errors = ocx.select_all_or_error();

// Only emit suggestion if all required predicates are on generic
Expand Down Expand Up @@ -876,6 +880,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
Some(borrow_span),
None,
);
self.suggest_copy_for_type_in_cloned_ref(&mut err, place);
self.buffer_error(err);
}

Expand Down Expand Up @@ -1214,10 +1219,104 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
);

self.suggest_using_local_if_applicable(&mut err, location, issued_borrow, explanation);
self.suggest_copy_for_type_in_cloned_ref(&mut err, place);

err
}

fn suggest_copy_for_type_in_cloned_ref(&self, err: &mut Diag<'tcx>, place: Place<'tcx>) {
let tcx = self.infcx.tcx;
let hir = tcx.hir();
let Some(body_id) = tcx.hir_node(self.mir_hir_id()).body_id() else { return };
struct FindUselessClone<'hir> {
pub clones: Vec<&'hir hir::Expr<'hir>>,
}
impl<'hir> FindUselessClone<'hir> {
pub fn new() -> Self {
Self { clones: vec![] }
}
}

impl<'v> Visitor<'v> for FindUselessClone<'v> {
fn visit_expr(&mut self, ex: &'v hir::Expr<'v>) {
// FIXME: use `lookup_method_for_diagnostic`?
if let hir::ExprKind::MethodCall(segment, _rcvr, args, _span) = ex.kind
&& segment.ident.name == sym::clone
&& args.len() == 0
{
self.clones.push(ex);
}
hir::intravisit::walk_expr(self, ex);
}
}
let mut expr_finder = FindUselessClone::new();

let body = hir.body(body_id).value;
expr_finder.visit_expr(body);

pub struct Holds<'tcx> {
ty: Ty<'tcx>,
holds: bool,
}

impl<'tcx> TypeVisitor<TyCtxt<'tcx>> for Holds<'tcx> {
type Result = std::ops::ControlFlow<()>;

fn visit_ty(&mut self, t: Ty<'tcx>) -> Self::Result {
if t == self.ty {
self.holds = true;
}
t.super_visit_with(self)
}
}

let mut types_to_constrain = FxIndexSet::default();

let local_ty = self.body.local_decls[place.local].ty;
let typeck_results = tcx.typeck(self.mir_def_id());
let clone = tcx.require_lang_item(LangItem::Clone, Some(body.span));
for expr in expr_finder.clones {
if let hir::ExprKind::MethodCall(_, rcvr, _, span) = expr.kind
&& let Some(rcvr_ty) = typeck_results.node_type_opt(rcvr.hir_id)
&& let Some(ty) = typeck_results.node_type_opt(expr.hir_id)
&& rcvr_ty == ty
&& let ty::Ref(_, inner, _) = rcvr_ty.kind()
&& let inner = inner.peel_refs()
&& let mut v = (Holds { ty: inner, holds: false })
&& let _ = v.visit_ty(local_ty)
&& v.holds
&& let None = self.infcx.type_implements_trait_shallow(clone, inner, self.param_env)
{
err.span_label(
span,
format!(
"this call doesn't do anything, the result is still `{rcvr_ty}` \
because `{inner}` doesn't implement `Clone`",
),
);
types_to_constrain.insert(inner);
}
}
for ty in types_to_constrain {
self.suggest_adding_bounds(err, ty, clone, body.span);
if let ty::Adt(..) = ty.kind() {
// The type doesn't implement Clone.
let trait_ref = ty::Binder::dummy(ty::TraitRef::new(self.infcx.tcx, clone, [ty]));
let obligation = Obligation::new(
self.infcx.tcx,
ObligationCause::dummy(),
self.param_env,
trait_ref,
);
self.infcx.err_ctxt().suggest_derive(
&obligation,
err,
trait_ref.to_predicate(self.infcx.tcx),
);
}
}
}

#[instrument(level = "debug", skip(self, err))]
fn suggest_using_local_if_applicable(
&self,
Expand Down
23 changes: 15 additions & 8 deletions compiler/rustc_borrowck/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1050,7 +1050,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
);
err.subdiagnostic(self.dcx(), CaptureReasonNote::FnOnceMoveInCall { var_span });
}
CallKind::Operator { self_arg, .. } => {
CallKind::Operator { self_arg, trait_id, .. } => {
let self_arg = self_arg.unwrap();
err.subdiagnostic(
self.dcx(),
Expand All @@ -1062,9 +1062,16 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
},
);
if self.fn_self_span_reported.insert(fn_span) {
let lang = self.infcx.tcx.lang_items();
err.subdiagnostic(
self.dcx(),
CaptureReasonNote::LhsMoveByOperator { span: self_arg.span },
if [lang.not_trait(), lang.deref_trait(), lang.neg_trait()]
.contains(&Some(trait_id))
{
CaptureReasonNote::UnOpMoveByOperator { span: self_arg.span }
} else {
CaptureReasonNote::LhsMoveByOperator { span: self_arg.span }
},
);
}
}
Expand Down Expand Up @@ -1226,20 +1233,20 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
{
let msg = match &errors[..] {
[] => "you can `clone` the value and consume it, but this \
might not be your desired behavior"
might not be your desired behavior"
.to_string(),
[error] => {
format!(
"you could `clone` the value and consume it, if \
the `{}` trait bound could be satisfied",
"you could `clone` the value and consume it, if the \
`{}` trait bound could be satisfied",
error.obligation.predicate,
)
}
[errors @ .., last] => {
format!(
"you could `clone` the value and consume it, if \
the following trait bounds could be satisfied: {} \
and `{}`",
"you could `clone` the value and consume it, if the \
following trait bounds could be satisfied: \
{} and `{}`",
errors
.iter()
.map(|e| format!("`{}`", e.obligation.predicate))
Expand Down
5 changes: 5 additions & 0 deletions compiler/rustc_borrowck/src/session_diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -368,6 +368,11 @@ pub(crate) enum CaptureReasonNote {
#[primary_span]
var_span: Span,
},
#[note(borrowck_calling_operator_moves)]
UnOpMoveByOperator {
#[primary_span]
span: Span,
},
#[note(borrowck_calling_operator_moves_lhs)]
LhsMoveByOperator {
#[primary_span]
Expand Down
2 changes: 1 addition & 1 deletion compiler/rustc_mir_build/messages.ftl
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ mir_build_borrow_of_layout_constrained_field_requires_unsafe_unsafe_op_in_unsafe
mir_build_borrow_of_moved_value = borrow of moved value
.label = value moved into `{$name}` here
.occurs_because_label = move occurs because `{$name}` has type `{$ty}` which does not implement the `Copy` trait
.occurs_because_label = move occurs because `{$name}` has type `{$ty}`, which does not implement the `Copy` trait
.value_borrowed_label = value borrowed here after move
.suggestion = borrow this binding in the pattern to avoid moving the value
Expand Down
32 changes: 32 additions & 0 deletions tests/ui/borrowck/clone-on-ref.fixed
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
//@ run-rustfix
fn foo<T: Default + Clone>(list: &mut Vec<T>) {
let mut cloned_items = Vec::new();
for v in list.iter() {
cloned_items.push(v.clone())
}
list.push(T::default());
//~^ ERROR cannot borrow `*list` as mutable because it is also borrowed as immutable
drop(cloned_items);
}
fn bar<T: std::fmt::Display + Clone>(x: T) {
let a = &x;
let b = a.clone();
drop(x);
//~^ ERROR cannot move out of `x` because it is borrowed
println!("{b}");
}
#[derive(Debug)]
#[derive(Clone)]
struct A;
fn qux(x: A) {
let a = &x;
let b = a.clone();
drop(x);
//~^ ERROR cannot move out of `x` because it is borrowed
println!("{b:?}");
}
fn main() {
foo(&mut vec![1, 2, 3]);
bar("");
qux(A);
}
31 changes: 31 additions & 0 deletions tests/ui/borrowck/clone-on-ref.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
//@ run-rustfix
fn foo<T: Default>(list: &mut Vec<T>) {
let mut cloned_items = Vec::new();
for v in list.iter() {
cloned_items.push(v.clone())
}
list.push(T::default());
//~^ ERROR cannot borrow `*list` as mutable because it is also borrowed as immutable
drop(cloned_items);
}
fn bar<T: std::fmt::Display>(x: T) {
let a = &x;
let b = a.clone();
drop(x);
//~^ ERROR cannot move out of `x` because it is borrowed
println!("{b}");
}
#[derive(Debug)]
struct A;
fn qux(x: A) {
let a = &x;
let b = a.clone();
drop(x);
//~^ ERROR cannot move out of `x` because it is borrowed
println!("{b:?}");
}
fn main() {
foo(&mut vec![1, 2, 3]);
bar("");
qux(A);
}
64 changes: 64 additions & 0 deletions tests/ui/borrowck/clone-on-ref.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
error[E0502]: cannot borrow `*list` as mutable because it is also borrowed as immutable
--> $DIR/clone-on-ref.rs:7:5
|
LL | for v in list.iter() {
| ---- immutable borrow occurs here
LL | cloned_items.push(v.clone())
| ------- this call doesn't do anything, the result is still `&T` because `T` doesn't implement `Clone`
LL | }
LL | list.push(T::default());
| ^^^^^^^^^^^^^^^^^^^^^^^ mutable borrow occurs here
LL |
LL | drop(cloned_items);
| ------------ immutable borrow later used here
|
help: consider further restricting this bound
|
LL | fn foo<T: Default + Clone>(list: &mut Vec<T>) {
| +++++++

error[E0505]: cannot move out of `x` because it is borrowed
--> $DIR/clone-on-ref.rs:14:10
|
LL | fn bar<T: std::fmt::Display>(x: T) {
| - binding `x` declared here
LL | let a = &x;
| -- borrow of `x` occurs here
LL | let b = a.clone();
| ------- this call doesn't do anything, the result is still `&T` because `T` doesn't implement `Clone`
LL | drop(x);
| ^ move out of `x` occurs here
LL |
LL | println!("{b}");
| --- borrow later used here
|
help: consider further restricting this bound
|
LL | fn bar<T: std::fmt::Display + Clone>(x: T) {
| +++++++

error[E0505]: cannot move out of `x` because it is borrowed
--> $DIR/clone-on-ref.rs:23:10
|
LL | fn qux(x: A) {
| - binding `x` declared here
LL | let a = &x;
| -- borrow of `x` occurs here
LL | let b = a.clone();
| ------- this call doesn't do anything, the result is still `&A` because `A` doesn't implement `Clone`
LL | drop(x);
| ^ move out of `x` occurs here
LL |
LL | println!("{b:?}");
| ----- borrow later used here
|
help: consider annotating `A` with `#[derive(Clone)]`
|
LL + #[derive(Clone)]
LL | struct A;
|

error: aborting due to 3 previous errors

Some errors have detailed explanations: E0502, E0505.
For more information about an error, try `rustc --explain E0502`.
Loading

0 comments on commit 9e153cc

Please sign in to comment.