Provide help on closures capturing self causing borrow checker errors

Author: chenyukang

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -1,4 +1,6 @@
+use crate::diagnostics::mutability_errors::mut_borrow_of_mutable_ref;
 use either::Either;
+use hir::Closure;
 use rustc_const_eval::util::CallKind;
 use rustc_data_structures::captures::Captures;
 use rustc_data_structures::fx::FxHashSet;
@@ -20,7 +22,7 @@ use rustc_middle::ty::{self, suggest_constraining_type_params, PredicateKind, Ty
 use rustc_mir_dataflow::move_paths::{InitKind, MoveOutIndex, MovePathIndex};
 use rustc_span::def_id::LocalDefId;
 use rustc_span::hygiene::DesugaringKind;
-use rustc_span::symbol::sym;
+use rustc_span::symbol::{kw, sym};
 use rustc_span::{BytePos, Span, Symbol};
 use rustc_trait_selection::infer::InferCtxtExt;
 
@@ -39,6 +41,8 @@ use super::{
     DescribePlaceOpt, RegionName, RegionNameSource, UseSpans,
 };
 
+use rustc_hir::def::Res;
+
 #[derive(Debug)]
 struct MoveSite {
     /// Index of the "move out" that we found. The `MoveData` can
@@ -356,7 +360,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         if let Some(hir::Node::Item(hir::Item {
             kind: hir::ItemKind::Fn(_, _, body_id),
             ..
-        })) = hir.find(hir.local_def_id_to_hir_id(self.mir_def_id()))
+        })) = hir.find(self.mir_hir_id())
             && let Some(hir::Node::Expr(expr)) = hir.find(body_id.hir_id)
         {
             let place = &self.move_data.move_paths[mpi].place;
@@ -948,7 +952,7 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             }
             (BorrowKind::Mut { .. }, BorrowKind::Shared) => {
                 first_borrow_desc = "immutable ";
-                self.cannot_reborrow_already_borrowed(
+                let mut err = self.cannot_reborrow_already_borrowed(
                     span,
                     &desc_place,
                     &msg_place,
@@ -958,7 +962,13 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
                     "immutable",
                     &msg_borrow,
                     None,
-                )
+                );
+                self.suggest_binding_for_closure_capture_self(
+                    &mut err,
+                    issued_borrow.borrowed_place,
+                    &issued_spans,
+                );
+                err
             }
 
             (BorrowKind::Mut { .. }, BorrowKind::Mut { .. }) => {
@@ -1240,6 +1250,138 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
         }
     }
 
+    fn suggest_binding_for_closure_capture_self(
+        &self,
+        err: &mut Diagnostic,
+        borrowed_place: Place<'tcx>,
+        issued_spans: &UseSpans<'tcx>,
+    ) {
+        let UseSpans::ClosureUse { capture_kind_span, .. } = issued_spans else { return };
+        let hir = self.infcx.tcx.hir();
+
+        // check whether the borrowed place is capturing `self` by mut reference
+        let local = borrowed_place.local;
+        let Some(_) = self
+            .body
+            .local_decls
+            .get(local)
+            .map(|l| mut_borrow_of_mutable_ref(l, self.local_names[local])) else { return };
+
+        struct ExpressionFinder<'hir> {
+            capture_span: Span,
+            closure_change_spans: Vec<Span>,
+            closure_arg_span: Option<Span>,
+            in_closure: bool,
+            suggest_arg: String,
+            hir: rustc_middle::hir::map::Map<'hir>,
+            closure_local_id: Option<hir::HirId>,
+            closure_call_changes: Vec<(Span, String)>,
+        }
+        impl<'hir> Visitor<'hir> for ExpressionFinder<'hir> {
+            fn visit_expr(&mut self, e: &'hir hir::Expr<'hir>) {
+                if e.span.contains(self.capture_span) {
+                    if let hir::ExprKind::Closure(&Closure {
+                            movability: None,
+                            body,
+                            fn_arg_span,
+                            fn_decl: hir::FnDecl{ inputs, .. },
+                            ..
+                        }) = e.kind &&
+                        let Some(hir::Node::Expr(body )) = self.hir.find(body.hir_id) {
+                            self.suggest_arg = "this: &Self".to_string();
+                            if inputs.len() > 0 {
+                                self.suggest_arg.push_str(", ");
+                            }
+                            self.in_closure = true;
+                            self.closure_arg_span = fn_arg_span;
+                            self.visit_expr(body);
+                            self.in_closure = false;
+                    }
+                }
+                if let hir::Expr { kind: hir::ExprKind::Path(path), .. } = e {
+                    if let hir::QPath::Resolved(_, hir::Path { segments: [seg], ..}) = path &&
+                        seg.ident.name == kw::SelfLower && self.in_closure {
+                            self.closure_change_spans.push(e.span);
+                    }
+                }
+                hir::intravisit::walk_expr(self, e);
+            }
+
+            fn visit_local(&mut self, local: &'hir hir::Local<'hir>) {
+                if let hir::Pat { kind: hir::PatKind::Binding(_, hir_id, _ident, _), .. } = local.pat &&
+                    let Some(init) = local.init
+                {
+                    if let hir::Expr { kind: hir::ExprKind::Closure(&Closure {
+                            movability: None,
+                            ..
+                        }), .. } = init &&
+                        init.span.contains(self.capture_span) {
+                            self.closure_local_id = Some(*hir_id);
+                    }
+                }
+                hir::intravisit::walk_local(self, local);
+            }
+
+            fn visit_stmt(&mut self, s: &'hir hir::Stmt<'hir>) {
+                if let hir::StmtKind::Semi(e) = s.kind &&
+                    let hir::ExprKind::Call(hir::Expr { kind: hir::ExprKind::Path(path), ..}, args) = e.kind &&
+                    let hir::QPath::Resolved(_, hir::Path { segments: [seg], ..}) = path &&
+                    let Res::Local(hir_id) = seg.res &&
+                        Some(hir_id) == self.closure_local_id {
+                        let mut arg_str = "self".to_string();
+                        if args.len() > 0 {
+                            arg_str.push_str(", ");
+                        }
+                        self.closure_call_changes.push((seg.ident.span, arg_str));
+                }
+                hir::intravisit::walk_stmt(self, s);
+            }
+        }
+
+        if let Some(hir::Node::ImplItem(
+                    hir::ImplItem { kind: hir::ImplItemKind::Fn(_fn_sig, body_id), .. }
+                )) = hir.find(self.mir_hir_id()) &&
+            let Some(hir::Node::Expr(expr)) = hir.find(body_id.hir_id) {
+            let mut finder = ExpressionFinder {
+                capture_span: *capture_kind_span,
+                closure_change_spans: vec![],
+                closure_arg_span: None,
+                in_closure: false,
+                suggest_arg: String::new(),
+                closure_local_id: None,
+                closure_call_changes: vec![],
+                hir,
+            };
+            finder.visit_expr(expr);
+
+            if finder.closure_change_spans.is_empty() || finder.closure_call_changes.is_empty() {
+                return;
+            }
+
+            let mut sugg = vec![];
+            let sm = self.infcx.tcx.sess.source_map();
+
+            if let Some(span) = finder.closure_arg_span {
+                sugg.push((sm.next_point(span.shrink_to_lo()).shrink_to_hi(), finder.suggest_arg));
+            }
+            for span in finder.closure_change_spans {
+                sugg.push((span, "this".to_string()));
+            }
+
+            for (span, suggest) in finder.closure_call_changes {
+                if  let Ok(span) = sm.span_extend_while(span, |c| c != '(') {
+                    sugg.push((sm.next_point(span).shrink_to_hi(), suggest));
+                }
+            }
+
+            err.multipart_suggestion_verbose(
+                "try explicitly pass `&Self` into the Closure as an argument",
+                sugg,
+                Applicability::MachineApplicable,
+            );
+        }
+    }
+
     /// Returns the description of the root place for a conflicting borrow and the full
     /// descriptions of the places that caused the conflict.
     ///

compiler/rustc_borrowck/src/diagnostics/mutability_errors.rs

@@ -1094,7 +1094,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
     }
 }
 
-fn mut_borrow_of_mutable_ref(local_decl: &LocalDecl<'_>, local_name: Option<Symbol>) -> bool {
+pub fn mut_borrow_of_mutable_ref(local_decl: &LocalDecl<'_>, local_name: Option<Symbol>) -> bool {
     debug!("local_info: {:?}, ty.kind(): {:?}", local_decl.local_info, local_decl.ty.kind());
 
     match local_decl.local_info.as_deref() {

tests/ui/suggestions/issue-105761-suggest-self-for-closure.fixed

@@ -0,0 +1,28 @@
+//run-rustfix
+#![allow(unused)]
+
+struct S;
+impl S {
+    fn foo(&mut self) {
+        let x = |this: &Self, v: i32| {
+            this.bar();
+            this.hel();
+        };
+        self.qux(); //~ ERROR cannot borrow `*self` as mutable because it is also borrowed as immutable
+        x(self, 1);
+        x(self, 3);
+    }
+    fn bar(&self) {}
+    fn hel(&self) {}
+    fn qux(&mut self) {}
+
+    fn hello(&mut self) {
+        let y = |this: &Self| {
+            this.bar();
+        };
+        self.qux(); //~ ERROR cannot borrow `*self` as mutable because it is also borrowed as immutable
+        y(self);
+    }
+}
+
+fn main() {}

tests/ui/suggestions/issue-105761-suggest-self-for-closure.rs

@@ -0,0 +1,28 @@
+//run-rustfix
+#![allow(unused)]
+
+struct S;
+impl S {
+    fn foo(&mut self) {
+        let x = |v: i32| {
+            self.bar();
+            self.hel();
+        };
+        self.qux(); //~ ERROR cannot borrow `*self` as mutable because it is also borrowed as immutable
+        x(1);
+        x(3);
+    }
+    fn bar(&self) {}
+    fn hel(&self) {}
+    fn qux(&mut self) {}
+
+    fn hello(&mut self) {
+        let y = || {
+            self.bar();
+        };
+        self.qux(); //~ ERROR cannot borrow `*self` as mutable because it is also borrowed as immutable
+        y();
+    }
+}
+
+fn main() {}

tests/ui/suggestions/issue-105761-suggest-self-for-closure.stderr

@@ -0,0 +1,49 @@
+error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
+  --> $DIR/issue-105761-suggest-self-for-closure.rs:11:9
+   |
+LL |         let x = |v: i32| {
+   |                 -------- immutable borrow occurs here
+LL |             self.bar();
+   |             ---- first borrow occurs due to use of `self` in closure
+...
+LL |         self.qux();
+   |         ^^^^^^^^^^ mutable borrow occurs here
+LL |         x(1);
+   |         - immutable borrow later used here
+   |
+help: try explicitly pass `&Self` into the Closure as an argument
+   |
+LL ~         let x = |this: &Self, v: i32| {
+LL ~             this.bar();
+LL ~             this.hel();
+LL |         };
+LL |         self.qux();
+LL ~         x(self, 1);
+LL ~         x(self, 3);
+   |
+
+error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
+  --> $DIR/issue-105761-suggest-self-for-closure.rs:23:9
+   |
+LL |         let y = || {
+   |                 -- immutable borrow occurs here
+LL |             self.bar();
+   |             ---- first borrow occurs due to use of `self` in closure
+LL |         };
+LL |         self.qux();
+   |         ^^^^^^^^^^ mutable borrow occurs here
+LL |         y();
+   |         - immutable borrow later used here
+   |
+help: try explicitly pass `&Self` into the Closure as an argument
+   |
+LL ~         let y = |this: &Self| {
+LL ~             this.bar();
+LL |         };
+LL |         self.qux();
+LL ~         y(self);
+   |
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0502`.