From cc80106cb5dda2abc9ab1945639644fff219958d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Esteban=20K=C3=BCber?= Date: Tue, 21 Nov 2023 23:41:50 +0000 Subject: [PATCH] Provide more suggestions for cloning immutable bindings When encountering multiple mutable borrows, suggest cloning and adding derive annotations as needed. ``` error[E0596]: cannot borrow `sm.x` as mutable, as it is behind a `&` reference --> $DIR/accidentally-cloning-ref-borrow-error.rs:32:9 | LL | foo(&mut sm.x); | ^^^^^^^^^ `sm` is a `&` reference, so the data it refers to cannot be borrowed as mutable | help: `Str` doesn't implement `Clone`, so this call clones the reference `&Str` --> $DIR/accidentally-cloning-ref-borrow-error.rs:31:21 | LL | let mut sm = sr.clone(); | ^^^^^^^ help: consider annotating `Str` with `#[derive(Clone)]` | LL + #[derive(Clone)] LL | struct Str { | help: consider specifying this binding's type | LL | let mut sm: &mut Str = sr.clone(); | ++++++++++ ``` ``` error[E0596]: cannot borrow `*inner` as mutable, as it is behind a `&` reference --> $DIR/issue-91206.rs:14:5 | LL | inner.clear(); | ^^^^^ `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable | help: you can `clone` the `Vec` value and consume it, but this might not be your desired behavior --> $DIR/issue-91206.rs:11:17 | LL | let inner = client.get_inner_ref(); | ^^^^^^^^^^^^^^^^^^^^^^ help: consider specifying this binding's type | LL | let inner: &mut Vec = client.get_inner_ref(); | +++++++++++++++++ ``` --- .../src/diagnostics/mutability_errors.rs | 102 +++++++++++++++++- .../accidentally-cloning-ref-borrow-error.rs | 38 +++++++ ...cidentally-cloning-ref-borrow-error.stderr | 30 ++++++ tests/ui/borrowck/issue-85765-closure.rs | 1 + tests/ui/borrowck/issue-85765-closure.stderr | 13 ++- tests/ui/borrowck/issue-85765.rs | 1 + tests/ui/borrowck/issue-85765.stderr | 13 ++- tests/ui/borrowck/issue-91206.rs | 1 + tests/ui/borrowck/issue-91206.stderr | 7 +- 9 files changed, 196 insertions(+), 10 deletions(-) create mode 100644 tests/ui/borrowck/accidentally-cloning-ref-borrow-error.rs create mode 100644 tests/ui/borrowck/accidentally-cloning-ref-borrow-error.stderr diff --git a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs index d9ec2860962..8fe552708ed 100644 --- a/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs @@ -3,8 +3,9 @@ use rustc_errors::{Applicability, Diagnostic, DiagnosticBuilder, ErrorGuaranteed use rustc_hir as hir; use rustc_hir::intravisit::Visitor; use rustc_hir::Node; +use rustc_infer::traits; use rustc_middle::mir::{Mutability, Place, PlaceRef, ProjectionElem}; -use rustc_middle::ty::{self, InstanceDef, Ty, TyCtxt}; +use rustc_middle::ty::{self, InstanceDef, ToPredicate, Ty, TyCtxt}; use rustc_middle::{ hir::place::PlaceBase, mir::{self, BindingForm, Local, LocalDecl, LocalInfo, LocalKind, Location}, @@ -12,6 +13,8 @@ use rustc_middle::{ use rustc_span::symbol::{kw, Symbol}; use rustc_span::{sym, BytePos, DesugaringKind, Span}; use rustc_target::abi::FieldIdx; +use rustc_trait_selection::infer::InferCtxtExt; +use rustc_trait_selection::traits::error_reporting::suggestions::TypeErrCtxtExt; use crate::diagnostics::BorrowedContentSource; use crate::util::FindAssignments; @@ -1212,6 +1215,103 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> { if let Some(hir_id) = hir_id && let Some(hir::Node::Local(local)) = hir_map.find(hir_id) { + let tables = self.infcx.tcx.typeck(def_id.as_local().unwrap()); + if let Some(clone_trait) = self.infcx.tcx.lang_items().clone_trait() + && let Some(expr) = local.init + && let ty = tables.node_type_opt(expr.hir_id) + && let Some(ty) = ty + && let ty::Ref(..) = ty.kind() + { + match self + .infcx + .could_impl_trait(clone_trait, ty.peel_refs(), self.param_env) + .as_deref() + { + Some([]) => { + // The type implements Clone. + err.span_help( + expr.span, + format!( + "you can `clone` the `{}` value and consume it, but this \ + might not be your desired behavior", + ty.peel_refs(), + ), + ); + } + None => { + if let hir::ExprKind::MethodCall(segment, _rcvr, [], span) = + expr.kind + && segment.ident.name == sym::clone + { + err.span_help( + span, + format!( + "`{}` doesn't implement `Clone`, so this call clones \ + the reference `{ty}`", + ty.peel_refs(), + ), + ); + } + // The type doesn't implement Clone. + let trait_ref = ty::Binder::dummy(ty::TraitRef::new( + self.infcx.tcx, + clone_trait, + [ty.peel_refs()], + )); + let obligation = traits::Obligation::new( + self.infcx.tcx, + traits::ObligationCause::dummy(), + self.param_env, + trait_ref, + ); + self.infcx.err_ctxt().suggest_derive( + &obligation, + err, + trait_ref.to_predicate(self.infcx.tcx), + ); + } + Some(errors) => { + if let hir::ExprKind::MethodCall(segment, _rcvr, [], span) = + expr.kind + && segment.ident.name == sym::clone + { + err.span_help( + span, + format!( + "`{}` doesn't implement `Clone` because its \ + implementations trait bounds could not be met, so \ + this call clones the reference `{ty}`", + ty.peel_refs(), + ), + ); + err.note(format!( + "the following trait bounds weren't met: {}", + errors + .iter() + .map(|e| e.obligation.predicate.to_string()) + .collect::>() + .join("\n"), + )); + } + // The type doesn't implement Clone because of unmet obligations. + for error in errors { + if let traits::FulfillmentErrorCode::CodeSelectionError( + traits::SelectionError::Unimplemented, + ) = error.code + && let ty::PredicateKind::Clause(ty::ClauseKind::Trait( + pred, + )) = error.obligation.predicate.kind().skip_binder() + { + self.infcx.err_ctxt().suggest_derive( + &error.obligation, + err, + error.obligation.predicate.kind().rebind(pred), + ); + } + } + } + } + } let (changing, span, sugg) = match local.ty { Some(ty) => ("changing", ty.span, message), None => { diff --git a/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.rs b/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.rs new file mode 100644 index 00000000000..2b25a5b2348 --- /dev/null +++ b/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.rs @@ -0,0 +1,38 @@ +#[derive(Debug)] +struct X(T); + +impl Clone for X { + fn clone(&self) -> X { + X(self.0.clone()) + } +} + +#[derive(Debug)] +struct Y; + +#[derive(Debug)] +struct Str { + x: Option, +} + +fn foo(s: &mut Option) { + if s.is_none() { + *s = Some(0); + } + println!("{:?}", s); +} + +fn bar(s: &mut X) { + println!("{:?}", s); +} +fn main() { + let s = Str { x: None }; + let sr = &s; + let mut sm = sr.clone(); + foo(&mut sm.x); //~ ERROR cannot borrow `sm.x` as mutable, as it is behind a `&` reference + + let x = X(Y); + let xr = &x; + let mut xm = xr.clone(); + bar(&mut xm); //~ ERROR cannot borrow data in a `&` reference as mutable +} diff --git a/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.stderr b/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.stderr new file mode 100644 index 00000000000..7e51a4819ee --- /dev/null +++ b/tests/ui/borrowck/accidentally-cloning-ref-borrow-error.stderr @@ -0,0 +1,30 @@ +error[E0596]: cannot borrow `sm.x` as mutable, as it is behind a `&` reference + --> $DIR/accidentally-cloning-ref-borrow-error.rs:32:9 + | +LL | foo(&mut sm.x); + | ^^^^^^^^^ `sm` is a `&` reference, so the data it refers to cannot be borrowed as mutable + | +help: `Str` doesn't implement `Clone`, so this call clones the reference `&Str` + --> $DIR/accidentally-cloning-ref-borrow-error.rs:31:21 + | +LL | let mut sm = sr.clone(); + | ^^^^^^^ +help: consider annotating `Str` with `#[derive(Clone)]` + | +LL + #[derive(Clone)] +LL | struct Str { + | +help: consider specifying this binding's type + | +LL | let mut sm: &mut Str = sr.clone(); + | ++++++++++ + +error[E0596]: cannot borrow data in a `&` reference as mutable + --> $DIR/accidentally-cloning-ref-borrow-error.rs:37:9 + | +LL | bar(&mut xm); + | ^^^^^^^ cannot borrow as mutable + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0596`. diff --git a/tests/ui/borrowck/issue-85765-closure.rs b/tests/ui/borrowck/issue-85765-closure.rs index f2d1dd0fbc3..edc9eeaffb5 100644 --- a/tests/ui/borrowck/issue-85765-closure.rs +++ b/tests/ui/borrowck/issue-85765-closure.rs @@ -3,6 +3,7 @@ fn main() { let mut test = Vec::new(); let rofl: &Vec> = &mut test; //~^ HELP consider changing this binding's type + //~| HELP you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior rofl.push(Vec::new()); //~^ ERROR cannot borrow `*rofl` as mutable, as it is behind a `&` reference //~| NOTE `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable diff --git a/tests/ui/borrowck/issue-85765-closure.stderr b/tests/ui/borrowck/issue-85765-closure.stderr index 936ddd67bcd..4a6a0e94bec 100644 --- a/tests/ui/borrowck/issue-85765-closure.stderr +++ b/tests/ui/borrowck/issue-85765-closure.stderr @@ -1,16 +1,21 @@ error[E0596]: cannot borrow `*rofl` as mutable, as it is behind a `&` reference - --> $DIR/issue-85765-closure.rs:6:9 + --> $DIR/issue-85765-closure.rs:7:9 | LL | rofl.push(Vec::new()); | ^^^^ `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable | +help: you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior + --> $DIR/issue-85765-closure.rs:4:36 + | +LL | let rofl: &Vec> = &mut test; + | ^^^^^^^^^ help: consider changing this binding's type | LL | let rofl: &mut Vec> = &mut test; | ~~~~~~~~~~~~~~~~~~ error[E0594]: cannot assign to `*r`, which is behind a `&` reference - --> $DIR/issue-85765-closure.rs:13:9 + --> $DIR/issue-85765-closure.rs:14:9 | LL | *r = 0; | ^^^^^^ `r` is a `&` reference, so the data it refers to cannot be written @@ -21,7 +26,7 @@ LL | let r = &mut mutvar; | +++ error[E0594]: cannot assign to `*x`, which is behind a `&` reference - --> $DIR/issue-85765-closure.rs:20:9 + --> $DIR/issue-85765-closure.rs:21:9 | LL | *x = 1; | ^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written @@ -32,7 +37,7 @@ LL | let x: &mut usize = &mut{0}; | ~~~~~~~~~~ error[E0594]: cannot assign to `*y`, which is behind a `&` reference - --> $DIR/issue-85765-closure.rs:27:9 + --> $DIR/issue-85765-closure.rs:28:9 | LL | *y = 1; | ^^^^^^ `y` is a `&` reference, so the data it refers to cannot be written diff --git a/tests/ui/borrowck/issue-85765.rs b/tests/ui/borrowck/issue-85765.rs index 76e0b517354..ce5740bc0e7 100644 --- a/tests/ui/borrowck/issue-85765.rs +++ b/tests/ui/borrowck/issue-85765.rs @@ -2,6 +2,7 @@ fn main() { let mut test = Vec::new(); let rofl: &Vec> = &mut test; //~^ HELP consider changing this binding's type + //~| HELP you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior rofl.push(Vec::new()); //~^ ERROR cannot borrow `*rofl` as mutable, as it is behind a `&` reference //~| NOTE `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable diff --git a/tests/ui/borrowck/issue-85765.stderr b/tests/ui/borrowck/issue-85765.stderr index 57900bfb612..4889f774afa 100644 --- a/tests/ui/borrowck/issue-85765.stderr +++ b/tests/ui/borrowck/issue-85765.stderr @@ -1,16 +1,21 @@ error[E0596]: cannot borrow `*rofl` as mutable, as it is behind a `&` reference - --> $DIR/issue-85765.rs:5:5 + --> $DIR/issue-85765.rs:6:5 | LL | rofl.push(Vec::new()); | ^^^^ `rofl` is a `&` reference, so the data it refers to cannot be borrowed as mutable | +help: you can `clone` the `Vec>` value and consume it, but this might not be your desired behavior + --> $DIR/issue-85765.rs:3:32 + | +LL | let rofl: &Vec> = &mut test; + | ^^^^^^^^^ help: consider changing this binding's type | LL | let rofl: &mut Vec> = &mut test; | ~~~~~~~~~~~~~~~~~~ error[E0594]: cannot assign to `*r`, which is behind a `&` reference - --> $DIR/issue-85765.rs:12:5 + --> $DIR/issue-85765.rs:13:5 | LL | *r = 0; | ^^^^^^ `r` is a `&` reference, so the data it refers to cannot be written @@ -21,7 +26,7 @@ LL | let r = &mut mutvar; | +++ error[E0594]: cannot assign to `*x`, which is behind a `&` reference - --> $DIR/issue-85765.rs:19:5 + --> $DIR/issue-85765.rs:20:5 | LL | *x = 1; | ^^^^^^ `x` is a `&` reference, so the data it refers to cannot be written @@ -32,7 +37,7 @@ LL | let x: &mut usize = &mut{0}; | ~~~~~~~~~~ error[E0594]: cannot assign to `*y`, which is behind a `&` reference - --> $DIR/issue-85765.rs:26:5 + --> $DIR/issue-85765.rs:27:5 | LL | *y = 1; | ^^^^^^ `y` is a `&` reference, so the data it refers to cannot be written diff --git a/tests/ui/borrowck/issue-91206.rs b/tests/ui/borrowck/issue-91206.rs index e062a253767..c60ac62fa34 100644 --- a/tests/ui/borrowck/issue-91206.rs +++ b/tests/ui/borrowck/issue-91206.rs @@ -10,6 +10,7 @@ fn main() { let client = TestClient; let inner = client.get_inner_ref(); //~^ HELP consider specifying this binding's type + //~| HELP you can `clone` the `Vec` value and consume it, but this might not be your desired behavior inner.clear(); //~^ ERROR cannot borrow `*inner` as mutable, as it is behind a `&` reference [E0596] //~| NOTE `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable diff --git a/tests/ui/borrowck/issue-91206.stderr b/tests/ui/borrowck/issue-91206.stderr index f96b0c7d9e1..e3dd65b6419 100644 --- a/tests/ui/borrowck/issue-91206.stderr +++ b/tests/ui/borrowck/issue-91206.stderr @@ -1,9 +1,14 @@ error[E0596]: cannot borrow `*inner` as mutable, as it is behind a `&` reference - --> $DIR/issue-91206.rs:13:5 + --> $DIR/issue-91206.rs:14:5 | LL | inner.clear(); | ^^^^^ `inner` is a `&` reference, so the data it refers to cannot be borrowed as mutable | +help: you can `clone` the `Vec` value and consume it, but this might not be your desired behavior + --> $DIR/issue-91206.rs:11:17 + | +LL | let inner = client.get_inner_ref(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: consider specifying this binding's type | LL | let inner: &mut Vec = client.get_inner_ref();