Skip to content

Commit ea3168b

Browse files
committed
Account for move error in the spread operator on struct literals
We attempt to suggest an appropriate clone for move errors on expressions like `S { ..s }` where a field isn't `Copy`. If we can't suggest, we still don't emit the incorrect suggestion of `S { ..s }.clone()`. ``` error[E0509]: cannot move out of type `S<K>`, which implements the `Drop` trait --> $DIR/borrowck-struct-update-with-dtor.rs:28:19 | 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 | LL | let _s2 = S { a: 2, c: s0.c.clone(), ..s0 }; | +++++++++++++++++ ``` ``` error[E0509]: cannot move out of type `S<()>`, which implements the `Drop` trait --> $DIR/borrowck-struct-update-with-dtor.rs:20:19 | LL | let _s2 = S { a: 2, ..s0 }; | ^^^^^^^^^^^^^^^^ | | | cannot move out of here | move occurs because `s0.b` has type `B`, which does not implement the `Copy` trait | note: `B` doesn't implement `Copy` or `Clone` --> $DIR/borrowck-struct-update-with-dtor.rs:4:1 | LL | struct B; | ^^^^^^^^ help: if `B` implemented `Clone`, you could clone the value from the field instead of using the spread operator syntax | LL | let _s2 = S { a: 2, b: s0.b.clone(), ..s0 }; | +++++++++++++++++ ```
1 parent c7f9d74 commit ea3168b

File tree

4 files changed

+343
-39
lines changed

4 files changed

+343
-39
lines changed

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

Lines changed: 107 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -759,13 +759,109 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
759759
true
760760
}
761761

762+
fn suggest_cloning_on_spread_operator(
763+
&self,
764+
err: &mut Diag<'_>,
765+
ty: Ty<'tcx>,
766+
expr: &'cx hir::Expr<'cx>,
767+
) {
768+
let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
769+
let hir::ExprKind::Struct(struct_qpath, fields, Some(base)) = expr.kind else { return };
770+
let hir::QPath::Resolved(_, path) = struct_qpath else { return };
771+
let hir::def::Res::Def(_, def_id) = path.res else { return };
772+
let Some(expr_ty) = typeck_results.node_type_opt(expr.hir_id) else { return };
773+
let ty::Adt(def, args) = expr_ty.kind() else { return };
774+
let hir::ExprKind::Path(hir::QPath::Resolved(None, path)) = base.kind else { return };
775+
let (hir::def::Res::Local(_)
776+
| hir::def::Res::Def(
777+
DefKind::Const | DefKind::ConstParam | DefKind::Static { .. } | DefKind::AssocConst,
778+
_,
779+
)) = path.res
780+
else {
781+
return;
782+
};
783+
let Ok(base_str) = self.infcx.tcx.sess.source_map().span_to_snippet(base.span) else {
784+
return;
785+
};
786+
787+
// 1. look for the fields of type `ty`.
788+
// 2. check if they are clone and add them to suggestion
789+
// 3. check if there are any values left to `..` and remove it if not
790+
// 4. emit suggestion to clone the field directly as `bar: base.bar.clone()`
791+
792+
let mut final_field_count = fields.len();
793+
let Some(variant) = def.variants().iter().find(|variant| variant.def_id == def_id) else {
794+
// When we have an enum, look for the variant that corresponds to the variant the user
795+
// wrote.
796+
return;
797+
};
798+
let mut sugg = vec![];
799+
for field in &variant.fields {
800+
// In practice unless there are more than one field with the same type, we'll be
801+
// suggesting a single field at a type, because we don't aggregate multiple borrow
802+
// checker errors involving the spread operator into a single one.
803+
let field_ty = field.ty(self.infcx.tcx, args);
804+
let ident = field.ident(self.infcx.tcx);
805+
if field_ty == ty && fields.iter().all(|field| field.ident.name != ident.name) {
806+
// Suggest adding field and cloning it.
807+
sugg.push(format!("{ident}: {base_str}.{ident}.clone()"));
808+
final_field_count += 1;
809+
}
810+
}
811+
let (span, sugg) = match fields {
812+
[.., last] => (
813+
if final_field_count == variant.fields.len() {
814+
// We'll remove the `..base` as there aren't any fields left.
815+
last.span.shrink_to_hi().with_hi(base.span.hi())
816+
} else {
817+
last.span.shrink_to_hi()
818+
},
819+
format!(", {}", sugg.join(", ")),
820+
),
821+
// Account for no fields in suggestion span.
822+
[] => (
823+
expr.span.with_lo(struct_qpath.span().hi()),
824+
if final_field_count == variant.fields.len() {
825+
// We'll remove the `..base` as there aren't any fields left.
826+
format!(" {{ {} }}", sugg.join(", "))
827+
} else {
828+
format!(" {{ {}, ..{base_str} }}", sugg.join(", "))
829+
},
830+
),
831+
};
832+
let prefix = if !self.implements_clone(ty) {
833+
let msg = format!("`{ty}` doesn't implement `Copy` or `Clone`");
834+
if let ty::Adt(def, _) = ty.kind() {
835+
err.span_note(self.infcx.tcx.def_span(def.did()), msg);
836+
} else {
837+
err.note(msg);
838+
}
839+
format!("if `{ty}` implemented `Clone`, you could ")
840+
} else {
841+
String::new()
842+
};
843+
let msg = format!(
844+
"{prefix}clone the value from the field instead of using the spread operator syntax",
845+
);
846+
err.span_suggestion_verbose(span, msg, sugg, Applicability::MachineApplicable);
847+
}
848+
762849
pub(crate) fn suggest_cloning(
763850
&self,
764851
err: &mut Diag<'_>,
765852
ty: Ty<'tcx>,
766853
mut expr: &'cx hir::Expr<'cx>,
767854
mut other_expr: Option<&'cx hir::Expr<'cx>>,
768855
) {
856+
if let hir::ExprKind::Struct(_, _, Some(_)) = expr.kind {
857+
// We have `S { foo: val, ..base }`. In `check_aggregate_rvalue` we have a single
858+
// `Location` that covers both the `S { ... }` literal, all of its fields and the
859+
// `base`. If the move happens because of `S { foo: val, bar: base.bar }` the `expr`
860+
// will already be correct. Instead, we see if we can suggest writing.
861+
self.suggest_cloning_on_spread_operator(err, ty, expr);
862+
return;
863+
}
864+
769865
if let Some(some_other_expr) = other_expr
770866
&& let Some(parent_binop) =
771867
self.infcx.tcx.hir().parent_iter(expr.hir_id).find_map(|n| {
@@ -847,11 +943,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
847943
} else {
848944
false
849945
})
850-
&& let Some(clone_trait_def) = self.infcx.tcx.lang_items().clone_trait()
851-
&& self
852-
.infcx
853-
.type_implements_trait(clone_trait_def, [call_ty], self.param_env)
854-
.must_apply_modulo_regions()
946+
&& self.implements_clone(call_ty)
855947
&& self.suggest_cloning_inner(err, call_ty, parent)
856948
{
857949
return;
@@ -861,16 +953,20 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
861953
}
862954
}
863955
let ty = ty.peel_refs();
864-
if let Some(clone_trait_def) = self.infcx.tcx.lang_items().clone_trait()
865-
&& self
866-
.infcx
867-
.type_implements_trait(clone_trait_def, [ty], self.param_env)
868-
.must_apply_modulo_regions()
869-
{
956+
if self.implements_clone(ty) {
870957
self.suggest_cloning_inner(err, ty, expr);
958+
// } else {
959+
// err.note(format!("if `{ty}` implemented `Clone`, you could clone the value"));
871960
}
872961
}
873962

963+
fn implements_clone(&self, ty: Ty<'tcx>) -> bool {
964+
let Some(clone_trait_def) = self.infcx.tcx.lang_items().clone_trait() else { return false };
965+
self.infcx
966+
.type_implements_trait(clone_trait_def, [ty], self.param_env)
967+
.must_apply_modulo_regions()
968+
}
969+
874970
pub(crate) fn clone_on_reference(&self, expr: &hir::Expr<'_>) -> Option<Span> {
875971
let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
876972
if let hir::ExprKind::MethodCall(segment, rcvr, args, span) = expr.kind

tests/ui/borrowck/borrowck-struct-update-with-dtor.rs

Lines changed: 52 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,20 +2,63 @@
22
// move, when the struct implements Drop.
33

44
struct B;
5-
struct S { a: isize, b: B }
6-
impl Drop for S { fn drop(&mut self) { } }
5+
struct S<K> { a: isize, b: B, c: K }
6+
impl<K> Drop for S<K> { fn drop(&mut self) { } }
77

8-
struct T { a: isize, mv: Box<isize> }
8+
struct T { a: isize, b: Box<isize> }
99
impl Drop for T { fn drop(&mut self) { } }
1010

11-
fn f(s0:S) {
12-
let _s2 = S{a: 2, ..s0};
13-
//~^ ERROR [E0509]
11+
struct V<K> { a: isize, b: Box<isize>, c: K }
12+
impl<K> Drop for V<K> { fn drop(&mut self) { } }
13+
14+
#[derive(Clone)]
15+
struct Clonable;
16+
17+
mod not_all_clone {
18+
use super::*;
19+
fn a(s0: S<()>) {
20+
let _s2 = S { a: 2, ..s0 };
21+
//~^ ERROR [E0509]
22+
}
23+
fn b(s0: S<B>) {
24+
let _s2 = S { a: 2, ..s0 };
25+
//~^ ERROR [E0509]
26+
//~| ERROR [E0509]
27+
}
28+
fn c<K: Clone>(s0: S<K>) {
29+
let _s2 = S { a: 2, ..s0 };
30+
//~^ ERROR [E0509]
31+
//~| ERROR [E0509]
32+
}
1433
}
34+
mod all_clone {
35+
use super::*;
36+
fn a(s0: T) {
37+
let _s2 = T { a: 2, ..s0 };
38+
//~^ ERROR [E0509]
39+
}
40+
41+
fn b(s0: T) {
42+
let _s2 = T { ..s0 };
43+
//~^ ERROR [E0509]
44+
}
45+
46+
fn c(s0: T) {
47+
let _s2 = T { a: 2, b: s0.b };
48+
//~^ ERROR [E0509]
49+
}
50+
51+
fn d<K: Clone>(s0: V<K>) {
52+
let _s2 = V { a: 2, ..s0 };
53+
//~^ ERROR [E0509]
54+
//~| ERROR [E0509]
55+
}
1556

16-
fn g(s0:T) {
17-
let _s2 = T{a: 2, ..s0};
18-
//~^ ERROR [E0509]
57+
fn e(s0: V<Clonable>) {
58+
let _s2 = V { a: 2, ..s0 };
59+
//~^ ERROR [E0509]
60+
//~| ERROR [E0509]
61+
}
1962
}
2063

2164
fn main() { }

0 commit comments

Comments
 (0)