diff options
4 files changed, 237 insertions, 8 deletions
diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 9823657c038..88cdb2996ab 100644 --- a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs +++ b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs @@ -1,3 +1,5 @@ +use std::iter; + use either::Either; use rustc_data_structures::captures::Captures; use rustc_data_structures::fx::FxIndexSet; @@ -10,25 +12,25 @@ use rustc_hir::intravisit::{walk_block, walk_expr, Visitor}; use rustc_hir::{AsyncGeneratorKind, GeneratorKind, LangItem}; use rustc_infer::infer::TyCtxtInferExt; use rustc_infer::traits::ObligationCause; +use rustc_middle::hir::nested_filter::OnlyBodies; use rustc_middle::mir::tcx::PlaceTy; use rustc_middle::mir::{ self, AggregateKind, BindingForm, BorrowKind, ClearCrossCrate, ConstraintCategory, FakeReadCause, LocalDecl, LocalInfo, LocalKind, Location, Operand, Place, PlaceRef, ProjectionElem, Rvalue, Statement, StatementKind, Terminator, TerminatorKind, VarBindingForm, }; -use rustc_middle::ty::{self, suggest_constraining_type_params, PredicateKind, Ty}; +use rustc_middle::ty::{self, suggest_constraining_type_params, PredicateKind, Ty, TypeckResults}; use rustc_middle::util::CallKind; use rustc_mir_dataflow::move_paths::{InitKind, MoveOutIndex, MovePathIndex}; use rustc_span::def_id::LocalDefId; use rustc_span::hygiene::DesugaringKind; -use rustc_span::symbol::{kw, sym}; +use rustc_span::symbol::{kw, sym, Ident}; use rustc_span::{BytePos, Span, Symbol}; use rustc_trait_selection::infer::InferCtxtExt; use rustc_trait_selection::traits::ObligationCtxt; use crate::borrow_set::TwoPhaseActivation; use crate::borrowck_errors; - use crate::diagnostics::conflict_errors::StorageDeadOrDrop::LocalStorageDead; use crate::diagnostics::{find_all_local_uses, CapturedMessageOpt}; use crate::{ @@ -959,6 +961,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { None, ); self.suggest_binding_for_closure_capture_self(&mut err, &issued_spans); + self.suggest_using_closure_argument_instead_of_capture( + &mut err, + issued_borrow.borrowed_place, + &issued_spans, + ); err } @@ -977,6 +984,11 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { place, issued_borrow.borrowed_place, ); + self.suggest_using_closure_argument_instead_of_capture( + &mut err, + issued_borrow.borrowed_place, + &issued_spans, + ); err } @@ -1263,6 +1275,173 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> { } } + /// Suggest using closure argument instead of capture. + /// + /// For example: + /// ```ignore (illustrative) + /// struct S; + /// + /// impl S { + /// fn call(&mut self, f: impl Fn(&mut Self)) { /* ... */ } + /// fn x(&self) {} + /// } + /// + /// let mut v = S; + /// v.call(|this: &mut S| v.x()); + /// // ^\ ^-- help: try using the closure argument: `this` + /// // *-- error: cannot borrow `v` as mutable because it is also borrowed as immutable + /// ``` + fn suggest_using_closure_argument_instead_of_capture( + &self, + err: &mut Diagnostic, + borrowed_place: Place<'tcx>, + issued_spans: &UseSpans<'tcx>, + ) { + let &UseSpans::ClosureUse { capture_kind_span, .. } = issued_spans else { return }; + let tcx = self.infcx.tcx; + let hir = tcx.hir(); + + // Get the type of the local that we are trying to borrow + let local = borrowed_place.local; + let local_ty = self.body.local_decls[local].ty; + + // Get the body the error happens in + let &body_id = + if let hir::Node::Item(hir::Item { kind, .. }) = hir.get(self.mir_hir_id()) + && let hir::ItemKind::Static(_, _, body_id) + | hir::ItemKind::Const(_, body_id) + | hir::ItemKind::Fn(_, _, body_id) = kind + { + body_id + } else if let hir::Node::TraitItem(hir::TraitItem { kind, .. }) = hir.get(self.mir_hir_id()) + && let hir::TraitItemKind::Const(_, Some(body_id)) + | hir::TraitItemKind::Fn(_, hir::TraitFn::Provided(body_id)) = kind + { + body_id + }else if let hir::Node::ImplItem(hir::ImplItem { kind, .. }) = hir.get(self.mir_hir_id()) + && let hir::ImplItemKind::Const(_, body_id) + | hir::ImplItemKind::Fn(_, body_id) = kind + { + body_id + } else { + return + }; + + let body_expr = hir.body(body_id).value; + + struct ClosureFinder<'hir> { + hir: rustc_middle::hir::map::Map<'hir>, + borrow_span: Span, + res: Option<(&'hir hir::Expr<'hir>, &'hir hir::Closure<'hir>)>, + /// The path expression with the `borrow_span` span + error_path: Option<(&'hir hir::Expr<'hir>, &'hir hir::QPath<'hir>)>, + } + impl<'hir> Visitor<'hir> for ClosureFinder<'hir> { + type NestedFilter = OnlyBodies; + + fn nested_visit_map(&mut self) -> Self::Map { + self.hir + } + + fn visit_expr(&mut self, ex: &'hir hir::Expr<'hir>) { + if let hir::ExprKind::Path(qpath) = &ex.kind + && ex.span == self.borrow_span + { + self.error_path = Some((ex, qpath)); + } + + if let hir::ExprKind::Closure(closure) = ex.kind + && ex.span.contains(self.borrow_span) + // To support cases like `|| { v.call(|this| v.get()) }` + // FIXME: actually support such cases (need to figure out how to move from the capture place to original local) + && self.res.as_ref().map_or(true, |(prev_res, _)| prev_res.span.contains(ex.span)) + { + self.res = Some((ex, closure)); + } + + hir::intravisit::walk_expr(self, ex); + } + } + + // Find the closure that most tightly wraps `capture_kind_span` + let mut finder = + ClosureFinder { hir, borrow_span: capture_kind_span, res: None, error_path: None }; + finder.visit_expr(body_expr); + let Some((closure_expr, closure)) = finder.res else { return }; + + let typeck_results: &TypeckResults<'_> = + tcx.typeck_opt_const_arg(self.body.source.with_opt_param().as_local().unwrap()); + + // Check that the parent of the closure is a method call, + // with receiver matching with local's type (modulo refs) + let parent = hir.parent_id(closure_expr.hir_id); + if let hir::Node::Expr(parent) = hir.get(parent) { + if let hir::ExprKind::MethodCall(_, recv, ..) = parent.kind { + let recv_ty = typeck_results.expr_ty(recv); + + if recv_ty.peel_refs() != local_ty { + return; + } + } + } + + // Get closure's arguments + let ty::Closure(_, substs) = typeck_results.expr_ty(closure_expr).kind() else { unreachable!() }; + let sig = substs.as_closure().sig(); + let tupled_params = + tcx.erase_late_bound_regions(sig.inputs().iter().next().unwrap().map_bound(|&b| b)); + let ty::Tuple(params) = tupled_params.kind() else { return }; + + // Find the first argument with a matching type, get its name + let Some((_, this_name)) = params + .iter() + .zip(hir.body_param_names(closure.body)) + .find(|(param_ty, name)|{ + // FIXME: also support deref for stuff like `Rc` arguments + param_ty.peel_refs() == local_ty && name != &Ident::empty() + }) + else { return }; + + let spans; + if let Some((_path_expr, qpath)) = finder.error_path + && let hir::QPath::Resolved(_, path) = qpath + && let hir::def::Res::Local(local_id) = path.res + { + // Find all references to the problematic variable in this closure body + + struct VariableUseFinder { + local_id: hir::HirId, + spans: Vec<Span>, + } + impl<'hir> Visitor<'hir> for VariableUseFinder { + fn visit_expr(&mut self, ex: &'hir hir::Expr<'hir>) { + if let hir::ExprKind::Path(qpath) = &ex.kind + && let hir::QPath::Resolved(_, path) = qpath + && let hir::def::Res::Local(local_id) = path.res + && local_id == self.local_id + { + self.spans.push(ex.span); + } + + hir::intravisit::walk_expr(self, ex); + } + } + + let mut finder = VariableUseFinder { local_id, spans: Vec::new() }; + finder.visit_expr(hir.body(closure.body).value); + + spans = finder.spans; + } else { + spans = vec![capture_kind_span]; + } + + err.multipart_suggestion( + "try using the closure argument", + iter::zip(spans, iter::repeat(this_name.to_string())).collect(), + Applicability::MaybeIncorrect, + ); + } + fn suggest_binding_for_closure_capture_self( &self, err: &mut Diagnostic, diff --git a/tests/ui/borrowck/issue-109271-pass-self-into-closure.fixed b/tests/ui/borrowck/issue-109271-pass-self-into-closure.fixed new file mode 100644 index 00000000000..4a8831dab95 --- /dev/null +++ b/tests/ui/borrowck/issue-109271-pass-self-into-closure.fixed @@ -0,0 +1,39 @@ +// run-rustfix +#![allow(unused)] +struct S; + +impl S { + fn call(&mut self, f: impl FnOnce((), &mut Self)) { + // change state or something ... + f((), self); + // change state or something ... + } + + fn get(&self) {} + fn set(&mut self) {} +} + +fn main() { + let mut v = S; + + v.call(|(), this: &mut S| this.get()); + //~^ error: cannot borrow `v` as mutable because it is also borrowed as immutable + v.call(|(), this: &mut S| this.set()); + //~^ error: cannot borrow `v` as mutable more than once at a time + //~| error: cannot borrow `v` as mutable more than once at a time + + v.call(|(), this: &mut S| { + //~^ error: cannot borrow `v` as mutable more than once at a time + //~| error: cannot borrow `v` as mutable more than once at a time + + _ = this; + this.set(); + this.get(); + S::get(&this); + + use std::ops::Add; + let v = 0u32; + _ = v + v; + _ = v.add(3); + }); +} diff --git a/tests/ui/borrowck/issue-109271-pass-self-into-closure.rs b/tests/ui/borrowck/issue-109271-pass-self-into-closure.rs index 16167914512..fcd855f862d 100644 --- a/tests/ui/borrowck/issue-109271-pass-self-into-closure.rs +++ b/tests/ui/borrowck/issue-109271-pass-self-into-closure.rs @@ -1,3 +1,4 @@ +// run-rustfix #![allow(unused)] struct S; diff --git a/tests/ui/borrowck/issue-109271-pass-self-into-closure.stderr b/tests/ui/borrowck/issue-109271-pass-self-into-closure.stderr index 8abbecad02a..25974e0d008 100644 --- a/tests/ui/borrowck/issue-109271-pass-self-into-closure.stderr +++ b/tests/ui/borrowck/issue-109271-pass-self-into-closure.stderr @@ -1,27 +1,29 @@ error[E0502]: cannot borrow `v` as mutable because it is also borrowed as immutable - --> $DIR/issue-109271-pass-self-into-closure.rs:18:5 + --> $DIR/issue-109271-pass-self-into-closure.rs:19:5 | LL | v.call(|(), this: &mut S| v.get()); | ^^----^------------------^-^^^^^^^ | | | | | | | | | first borrow occurs due to use of `v` in closure + | | | | help: try using the closure argument: `this` | | | immutable borrow occurs here | | immutable borrow later used by call | mutable borrow occurs here error[E0499]: cannot borrow `v` as mutable more than once at a time - --> $DIR/issue-109271-pass-self-into-closure.rs:20:5 + --> $DIR/issue-109271-pass-self-into-closure.rs:21:5 | LL | v.call(|(), this: &mut S| v.set()); | ^^----^------------------^-^^^^^^^ | | | | | | | | | first borrow occurs due to use of `v` in closure + | | | | help: try using the closure argument: `this` | | | first mutable borrow occurs here | | first borrow later used by call | second mutable borrow occurs here error[E0499]: cannot borrow `v` as mutable more than once at a time - --> $DIR/issue-109271-pass-self-into-closure.rs:20:12 + --> $DIR/issue-109271-pass-self-into-closure.rs:21:12 | LL | v.call(|(), this: &mut S| v.set()); | -------^^^^^^^^^^^^^^^^^^--------- @@ -32,7 +34,7 @@ LL | v.call(|(), this: &mut S| v.set()); | first mutable borrow occurs here error[E0499]: cannot borrow `v` as mutable more than once at a time - --> $DIR/issue-109271-pass-self-into-closure.rs:24:5 + --> $DIR/issue-109271-pass-self-into-closure.rs:25:5 | LL | v.call(|(), this: &mut S| { | ^ ---- ------------------ first mutable borrow occurs here @@ -49,9 +51,17 @@ LL | | v.set(); LL | | _ = v.add(3); LL | | }); | |______^ second mutable borrow occurs here + | +help: try using the closure argument + | +LL ~ _ = this; +LL ~ this.set(); +LL ~ this.get(); +LL ~ S::get(&this); + | error[E0499]: cannot borrow `v` as mutable more than once at a time - --> $DIR/issue-109271-pass-self-into-closure.rs:24:12 + --> $DIR/issue-109271-pass-self-into-closure.rs:25:12 | LL | v.call(|(), this: &mut S| { | - ---- ^^^^^^^^^^^^^^^^^^ second mutable borrow occurs here |