Skip to content

Commit

Permalink
review comments
Browse files Browse the repository at this point in the history 
Added comments and reworded messages
  • Loading branch information
@estebank
estebank committed Apr 12, 2024
1 parent dea9b50 commit 4c7213c
Show file tree
Hide file tree
Showing 4 changed files with 48 additions and 20 deletions.
23 changes: 15 additions & 8 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1006,7 +1006,9 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
can_suggest_clone
}

fn suggest_cloning_on_spread_operator(
/// We have `S { foo: val, ..base }`, and we suggest instead writing
/// `S { foo: val, bar: base.bar.clone(), .. }` when valid.
fn suggest_cloning_on_functional_record_update(
&self,
err: &mut Diag<'_>,
ty: Ty<'tcx>,
Expand Down Expand Up @@ -1046,7 +1048,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
for field in &variant.fields {
// In practice unless there are more than one field with the same type, we'll be
// suggesting a single field at a type, because we don't aggregate multiple borrow
// checker errors involving the spread operator into a single one.
// checker errors involving the functional record update sytnax into a single one.
let field_ty = field.ty(self.infcx.tcx, args);
let ident = field.ident(self.infcx.tcx);
if field_ty == ty && fields.iter().all(|field| field.ident.name != ident.name) {
Expand Down Expand Up @@ -1088,7 +1090,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
String::new()
};
let msg = format!(
"{prefix}clone the value from the field instead of using the spread operator syntax",
"{prefix}clone the value from the field instead of using the functional record update \
syntax",
);
err.span_suggestion_verbose(span, msg, sugg, Applicability::MachineApplicable);
}
Expand All @@ -1105,7 +1108,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// `Location` that covers both the `S { ... }` literal, all of its fields and the
// `base`. If the move happens because of `S { foo: val, bar: base.bar }` the `expr`
// will already be correct. Instead, we see if we can suggest writing.
self.suggest_cloning_on_spread_operator(err, ty, expr);
self.suggest_cloning_on_functional_record_update(err, ty, expr);
return;
}

Expand Down Expand Up @@ -1225,6 +1228,8 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
.must_apply_modulo_regions()
}

/// Given an expression, check if it is a method call `foo.clone()`, where `foo` and
/// `foo.clone()` both have the same type, returning the span for `.clone()` if so.
pub(crate) fn clone_on_reference(&self, expr: &hir::Expr<'_>) -> Option<Span> {
let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
if let hir::ExprKind::MethodCall(segment, rcvr, args, span) = expr.kind
Expand Down Expand Up @@ -1276,6 +1281,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
};
let mut sugg = Vec::with_capacity(2);
let mut inner_expr = expr;
// Remove uses of `&` and `*` when suggesting `.clone()`.
while let hir::ExprKind::AddrOf(.., inner) | hir::ExprKind::Unary(hir::UnOp::Deref, inner) =
&inner_expr.kind
{
Expand Down Expand Up @@ -1841,13 +1847,14 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
&self,
err: &mut Diag<'_>,
ty: Ty<'tcx>,
def_id: DefId,
trait_def_id: DefId,
span: Span,
) {
self.suggest_adding_bounds(err, ty, def_id, span);
self.suggest_adding_bounds(err, ty, trait_def_id, span);
if let ty::Adt(..) = ty.kind() {
// The type doesn't implement DefId.
let trait_ref = ty::Binder::dummy(ty::TraitRef::new(self.infcx.tcx, def_id, [ty]));
// The type doesn't implement the trait.
let trait_ref =
ty::Binder::dummy(ty::TraitRef::new(self.infcx.tcx, trait_def_id, [ty]));
let obligation = Obligation::new(
self.infcx.tcx,
ObligationCause::dummy(),
Expand Down
21 changes: 21 additions & 0 deletions compiler/rustc_borrowck/src/diagnostics/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1049,6 +1049,27 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
is_loop_message,
},
);
// Check if the move occurs on a value because of a call on a closure that comes
// from a type parameter `F: FnOnce()`. If so, we provide a targeted `note`:
// ```
// error[E0382]: use of moved value: `blk`
// --> $DIR/once-cant-call-twice-on-heap.rs:8:5
// |
// LL | fn foo<F:FnOnce()>(blk: F) {
// | --- move occurs because `blk` has type `F`, which does not implement the `Copy` trait
// LL | blk();
// | ----- `blk` moved due to this call
// LL | blk();
// | ^^^ value used here after move
// |
// note: `FnOnce` closures can only be called once
// --> $DIR/once-cant-call-twice-on-heap.rs:6:10
// |
// LL | fn foo<F:FnOnce()>(blk: F) {
// | ^^^^^^^^ `F` is made to be an `FnOnce` closure here
// LL | blk();
// | ----- this value implements `FnOnce`, which causes it to be moved when called
// ```
if let ty::Param(param_ty) = self_ty.kind()
&& let generics = self.infcx.tcx.generics_of(self.mir_def_id())
&& let param = generics.type_param(param_ty, self.infcx.tcx)
Expand Down
22 changes: 11 additions & 11 deletions tests/ui/borrowck/borrowck-struct-update-with-dtor.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ note: `B` doesn't implement `Copy` or `Clone`
|
LL | struct B;
| ^^^^^^^^
help: if `B` implemented `Clone`, you could clone the value from the field instead of using the spread operator syntax
help: if `B` implemented `Clone`, you could clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = S { a: 2, b: s0.b.clone(), ..s0 };
| +++++++++++++++++
Expand All @@ -31,7 +31,7 @@ note: `B` doesn't implement `Copy` or `Clone`
|
LL | struct B;
| ^^^^^^^^
help: if `B` implemented `Clone`, you could clone the value from the field instead of using the spread operator syntax
help: if `B` implemented `Clone`, you could clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = S { a: 2, b: s0.b.clone(), c: s0.c.clone() };
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand All @@ -50,7 +50,7 @@ note: `B` doesn't implement `Copy` or `Clone`
|
LL | struct B;
| ^^^^^^^^
help: if `B` implemented `Clone`, you could clone the value from the field instead of using the spread operator syntax
help: if `B` implemented `Clone`, you could clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = S { a: 2, b: s0.b.clone(), c: s0.c.clone() };
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Expand All @@ -69,7 +69,7 @@ note: `B` doesn't implement `Copy` or `Clone`
|
LL | struct B;
| ^^^^^^^^
help: if `B` implemented `Clone`, you could clone the value from the field instead of using the spread operator syntax
help: if `B` implemented `Clone`, you could clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = S { a: 2, b: s0.b.clone(), ..s0 };
| +++++++++++++++++
Expand All @@ -83,7 +83,7 @@ LL | let _s2 = S { a: 2, ..s0 };
| cannot move out of here
| move occurs because `s0.c` has type `K`, which does not implement the `Copy` trait
|
help: clone the value from the field instead of using the spread operator syntax
help: clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = S { a: 2, c: s0.c.clone(), ..s0 };
| +++++++++++++++++
Expand All @@ -97,7 +97,7 @@ LL | let _s2 = T { a: 2, ..s0 };
| cannot move out of here
| move occurs because `s0.b` has type `Box<isize>`, which does not implement the `Copy` trait
|
help: clone the value from the field instead of using the spread operator syntax
help: clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = T { a: 2, b: s0.b.clone() };
| ~~~~~~~~~~~~~~~~~
Expand All @@ -111,7 +111,7 @@ LL | let _s2 = T { ..s0 };
| cannot move out of here
| move occurs because `s0.b` has type `Box<isize>`, which does not implement the `Copy` trait
|
help: clone the value from the field instead of using the spread operator syntax
help: clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = T { b: s0.b.clone(), ..s0 };
| ~~~~~~~~~~~~~~~~~~~~~~~~~
Expand Down Expand Up @@ -139,7 +139,7 @@ LL | let _s2 = V { a: 2, ..s0 };
| cannot move out of here
| move occurs because `s0.b` has type `Box<isize>`, which does not implement the `Copy` trait
|
help: clone the value from the field instead of using the spread operator syntax
help: clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = V { a: 2, b: s0.b.clone(), ..s0 };
| +++++++++++++++++
Expand All @@ -153,7 +153,7 @@ LL | let _s2 = V { a: 2, ..s0 };
| cannot move out of here
| move occurs because `s0.c` has type `K`, which does not implement the `Copy` trait
|
help: clone the value from the field instead of using the spread operator syntax
help: clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = V { a: 2, c: s0.c.clone(), ..s0 };
| +++++++++++++++++
Expand All @@ -167,7 +167,7 @@ LL | let _s2 = V { a: 2, ..s0 };
| cannot move out of here
| move occurs because `s0.b` has type `Box<isize>`, which does not implement the `Copy` trait
|
help: clone the value from the field instead of using the spread operator syntax
help: clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = V { a: 2, b: s0.b.clone(), ..s0 };
| +++++++++++++++++
Expand All @@ -181,7 +181,7 @@ LL | let _s2 = V { a: 2, ..s0 };
| cannot move out of here
| move occurs because `s0.c` has type `Clonable`, which does not implement the `Copy` trait
|
help: clone the value from the field instead of using the spread operator syntax
help: clone the value from the field instead of using the functional record update syntax
|
LL | let _s2 = V { a: 2, c: s0.c.clone(), ..s0 };
| +++++++++++++++++
Expand Down
2 changes: 1 addition & 1 deletion tests/ui/functional-struct-update/functional-struct-update-noncopyable.stderr
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ LL | let _b = A { y: Arc::new(3), ..a };
| cannot move out of here
| move occurs because `a.x` has type `Arc<isize>`, which does not implement the `Copy` trait
|
help: clone the value from the field instead of using the spread operator syntax
help: clone the value from the field instead of using the functional record update syntax
|
LL | let _b = A { y: Arc::new(3), x: a.x.clone() };
| ~~~~~~~~~~~~~~~~
Expand Down

0 comments on commit 4c7213c

Please sign in to comment.