Rollup merge of rust-lang#83174 - camelid:borrow-help, r=oli-obk

matthiaskrgr · web-flow · commit 7c2ed856427b · 2021-12-11T08:20:06.000+01:00
Suggest using a temporary variable to fix borrowck errors Fixes rust-lang#77834. In Rust, nesting method calls with both require `&mut` access to `self` produces a borrow-check error: error[E0499]: cannot borrow `*self` as mutable more than once at a time --> src/lib.rs:7:14 | 7 | self.foo(self.bar()); | ---------^^^^^^^^^^- | | | | | | | second mutable borrow occurs here | | first borrow later used by call | first mutable borrow occurs here That's because Rust has a left-to-right evaluation order, and the method receiver is passed first. Thus, the argument to the method cannot then mutate `self`. There's an easy solution to this error: just extract a local variable for the inner argument: let tmp = self.bar(); self.foo(tmp); However, the error doesn't give any suggestion of how to solve the problem. As a result, new users may assume that it's impossible to express their code correctly and get stuck. This commit adds a (non-structured) suggestion to extract a local variable for the inner argument to solve the error. The suggestion uses heuristics that eliminate most false positives, though there are a few false negatives (cases where the suggestion should be emitted but is not). Those other cases can be implemented in a future change.
diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
@@ -15,16 +15,18 @@ use rustc_span::symbol::sym;
 use rustc_span::{BytePos, MultiSpan, Span, DUMMY_SP};
 use rustc_trait_selection::infer::InferCtxtExt;
 
+use crate::borrow_set::TwoPhaseActivation;
 use crate::borrowck_errors;
 
+use crate::diagnostics::find_all_local_uses;
 use crate::{
     borrow_set::BorrowData, diagnostics::Instance, prefixes::IsPrefixOf,
     InitializationRequiringAction, MirBorrowckCtxt, PrefixSet, WriteKind,
 };
 
 use super::{
-    explain_borrow::BorrowExplanation, FnSelfUseKind, IncludingDowncast, RegionName,
-    RegionNameSource, UseSpans,
+    explain_borrow::{BorrowExplanation, LaterUseKind},
+    FnSelfUseKind, IncludingDowncast, RegionName, RegionNameSource, UseSpans,
 };
 
 #[derive(Debug)]
@@ -768,9 +770,92 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
             Some((issued_span, span)),
         );
 
+        self.suggest_using_local_if_applicable(
+            &mut err,
+            location,
+            (place, span),
+            gen_borrow_kind,
+            issued_borrow,
+            explanation,
+        );
+
         err
     }
 
+    #[instrument(level = "debug", skip(self, err))]
+    fn suggest_using_local_if_applicable(
+        &self,
+        err: &mut DiagnosticBuilder<'_>,
+        location: Location,
+        (place, span): (Place<'tcx>, Span),
+        gen_borrow_kind: BorrowKind,
+        issued_borrow: &BorrowData<'tcx>,
+        explanation: BorrowExplanation,
+    ) {
+        let used_in_call =
+            matches!(explanation, BorrowExplanation::UsedLater(LaterUseKind::Call, _call_span, _));
+        if !used_in_call {
+            debug!("not later used in call");
+            return;
+        }
+
+        let outer_call_loc =
+            if let TwoPhaseActivation::ActivatedAt(loc) = issued_borrow.activation_location {
+                loc
+            } else {
+                issued_borrow.reserve_location
+            };
+        let outer_call_stmt = self.body.stmt_at(outer_call_loc);
+
+        let inner_param_location = location;
+        let Some(inner_param_stmt) = self.body.stmt_at(inner_param_location).left() else {
+            debug!("`inner_param_location` {:?} is not for a statement", inner_param_location);
+            return;
+        };
+        let Some(&inner_param) = inner_param_stmt.kind.as_assign().map(|(p, _)| p) else {
+            debug!(
+                "`inner_param_location` {:?} is not for an assignment: {:?}",
+                inner_param_location, inner_param_stmt
+            );
+            return;
+        };
+        let inner_param_uses = find_all_local_uses::find(self.body, inner_param.local);
+        let Some((inner_call_loc,inner_call_term)) = inner_param_uses.into_iter().find_map(|loc| {
+            let Either::Right(term) = self.body.stmt_at(loc) else {
+                debug!("{:?} is a statement, so it can't be a call", loc);
+                return None;
+            };
+            let TerminatorKind::Call { args, .. } = &term.kind else {
+                debug!("not a call: {:?}", term);
+                return None;
+            };
+            debug!("checking call args for uses of inner_param: {:?}", args);
+            if args.contains(&Operand::Move(inner_param)) {
+                Some((loc,term))
+            } else {
+                None
+            }
+        }) else {
+            debug!("no uses of inner_param found as a by-move call arg");
+            return;
+        };
+        debug!("===> outer_call_loc = {:?}, inner_call_loc = {:?}", outer_call_loc, inner_call_loc);
+
+        let inner_call_span = inner_call_term.source_info.span;
+        let outer_call_span = outer_call_stmt.either(|s| s.source_info, |t| t.source_info).span;
+        if outer_call_span == inner_call_span || !outer_call_span.contains(inner_call_span) {
+            // FIXME: This stops the suggestion in some cases where it should be emitted.
+            //        Fix the spans for those cases so it's emitted correctly.
+            debug!(
+                "outer span {:?} does not strictly contain inner span {:?}",
+                outer_call_span, inner_call_span
+            );
+            return;
+        }
+        err.span_help(inner_call_span, "try adding a local storing this argument...");
+        err.span_help(outer_call_span, "...and then using that local as the argument to this call");
+    }
+
     fn suggest_split_at_mut_if_applicable(
         &self,
         err: &mut DiagnosticBuilder<'_>,
diff --git a/compiler/rustc_borrowck/src/diagnostics/find_all_local_uses.rs b/compiler/rustc_borrowck/src/diagnostics/find_all_local_uses.rs
@@ -0,0 +1,26 @@
+use std::collections::BTreeSet;
+
+use rustc_middle::mir::visit::{PlaceContext, Visitor};
+use rustc_middle::mir::{Body, Local, Location};
+
+/// Find all uses of (including assignments to) a [`Local`].
+///
+/// Uses `BTreeSet` so output is deterministic.
+pub(super) fn find<'tcx>(body: &Body<'tcx>, local: Local) -> BTreeSet<Location> {
+    let mut visitor = AllLocalUsesVisitor { for_local: local, uses: BTreeSet::default() };
+    visitor.visit_body(body);
+    visitor.uses
+}
+
+struct AllLocalUsesVisitor {
+    for_local: Local,
+    uses: BTreeSet<Location>,
+}
+
+impl<'tcx> Visitor<'tcx> for AllLocalUsesVisitor {
+    fn visit_local(&mut self, local: &Local, _context: PlaceContext, location: Location) {
+        if *local == self.for_local {
+            self.uses.insert(location);
+        }
+    }
+}
diff --git a/compiler/rustc_borrowck/src/diagnostics/mod.rs b/compiler/rustc_borrowck/src/diagnostics/mod.rs
@@ -19,6 +19,7 @@ use rustc_target::abi::VariantIdx;
 use super::borrow_set::BorrowData;
 use super::MirBorrowckCtxt;
 
+mod find_all_local_uses;
 mod find_use;
 mod outlives_suggestion;
 mod region_name;
diff --git a/compiler/rustc_middle/src/mir/mod.rs b/compiler/rustc_middle/src/mir/mod.rs
@@ -12,6 +12,7 @@ use crate::ty::print::{FmtPrinter, Printer};
 use crate::ty::subst::{Subst, SubstsRef};
 use crate::ty::{self, List, Ty, TyCtxt};
 use crate::ty::{AdtDef, InstanceDef, Region, ScalarInt, UserTypeAnnotationIndex};
+
 use rustc_hir::def::{CtorKind, Namespace};
 use rustc_hir::def_id::{DefId, CRATE_DEF_INDEX};
 use rustc_hir::{self, GeneratorKind};
@@ -30,6 +31,9 @@ use rustc_serialize::{Decodable, Encodable};
 use rustc_span::symbol::Symbol;
 use rustc_span::{Span, DUMMY_SP};
 use rustc_target::asm::InlineAsmRegOrRegClass;
+
+use either::Either;
+
 use std::borrow::Cow;
 use std::convert::TryInto;
 use std::fmt::{self, Debug, Display, Formatter, Write};
@@ -503,6 +507,16 @@ impl<'tcx> Body<'tcx> {
         Location { block: bb, statement_index: self[bb].statements.len() }
     }
 
+    pub fn stmt_at(&self, location: Location) -> Either<&Statement<'tcx>, &Terminator<'tcx>> {
+        let Location { block, statement_index } = location;
+        let block_data = &self.basic_blocks[block];
+        block_data
+            .statements
+            .get(statement_index)
+            .map(Either::Left)
+            .unwrap_or_else(|| Either::Right(block_data.terminator()))
+    }
+
     #[inline]
     pub fn predecessors(&self) -> &Predecessors {
         self.predecessor_cache.compute(&self.basic_blocks)
diff --git a/src/test/ui/borrowck/suggest-local-var-double-mut.rs b/src/test/ui/borrowck/suggest-local-var-double-mut.rs
@@ -0,0 +1,27 @@
+// See issue #77834.
+
+#![crate_type = "lib"]
+
+mod method_syntax {
+    struct Foo;
+
+    impl Foo {
+        fn foo(&mut self, _: f32) -> i32 { todo!() }
+        fn bar(&mut self) -> f32 { todo!() }
+        fn baz(&mut self) {
+            self.foo(self.bar()); //~ ERROR
+        }
+    }
+}
+
+mod fully_qualified_syntax {
+    struct Foo;
+
+    impl Foo {
+        fn foo(&mut self, _: f32) -> i32 { todo!() }
+        fn bar(&mut self) -> f32 { todo!() }
+        fn baz(&mut self) {
+            Self::foo(self, Self::bar(self)); //~ ERROR
+        }
+    }
+}
diff --git a/src/test/ui/borrowck/suggest-local-var-double-mut.stderr b/src/test/ui/borrowck/suggest-local-var-double-mut.stderr
@@ -0,0 +1,44 @@
+error[E0499]: cannot borrow `*self` as mutable more than once at a time
+  --> $DIR/suggest-local-var-double-mut.rs:12:22
+   |
+LL |             self.foo(self.bar());
+   |             ---------^^^^^^^^^^-
+   |             |    |   |
+   |             |    |   second mutable borrow occurs here
+   |             |    first borrow later used by call
+   |             first mutable borrow occurs here
+   |
+help: try adding a local storing this argument...
+  --> $DIR/suggest-local-var-double-mut.rs:12:22
+   |
+LL |             self.foo(self.bar());
+   |                      ^^^^^^^^^^
+help: ...and then using that local as the argument to this call
+  --> $DIR/suggest-local-var-double-mut.rs:12:13
+   |
+LL |             self.foo(self.bar());
+   |             ^^^^^^^^^^^^^^^^^^^^
+
+error[E0499]: cannot borrow `*self` as mutable more than once at a time
+  --> $DIR/suggest-local-var-double-mut.rs:24:39
+   |
+LL |             Self::foo(self, Self::bar(self));
+   |             --------- ----            ^^^^ second mutable borrow occurs here
+   |             |         |
+   |             |         first mutable borrow occurs here
+   |             first borrow later used by call
+   |
+help: try adding a local storing this argument...
+  --> $DIR/suggest-local-var-double-mut.rs:24:29
+   |
+LL |             Self::foo(self, Self::bar(self));
+   |                             ^^^^^^^^^^^^^^^
+help: ...and then using that local as the argument to this call
+  --> $DIR/suggest-local-var-double-mut.rs:24:13
+   |
+LL |             Self::foo(self, Self::bar(self));
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0499`.
diff --git a/src/test/ui/borrowck/suggest-local-var-imm-and-mut.nll.stderr b/src/test/ui/borrowck/suggest-local-var-imm-and-mut.nll.stderr
@@ -0,0 +1,33 @@
+error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
+  --> $DIR/suggest-local-var-imm-and-mut.rs:12:22
+   |
+LL |             self.foo(self.bar());
+   |             ---------^^^^^^^^^^-
+   |             |    |   |
+   |             |    |   mutable borrow occurs here
+   |             |    immutable borrow later used by call
+   |             immutable borrow occurs here
+   |
+help: try adding a local storing this argument...
+  --> $DIR/suggest-local-var-imm-and-mut.rs:12:22
+   |
+LL |             self.foo(self.bar());
+   |                      ^^^^^^^^^^
+help: ...and then using that local as the argument to this call
+  --> $DIR/suggest-local-var-imm-and-mut.rs:12:13
+   |
+LL |             self.foo(self.bar());
+   |             ^^^^^^^^^^^^^^^^^^^^
+
+error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
+  --> $DIR/suggest-local-var-imm-and-mut.rs:24:39
+   |
+LL |             Self::foo(self, Self::bar(self));
+   |             --------- ----            ^^^^ mutable borrow occurs here
+   |             |         |
+   |             |         immutable borrow occurs here
+   |             immutable borrow later used by call
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0502`.
diff --git a/src/test/ui/borrowck/suggest-local-var-imm-and-mut.rs b/src/test/ui/borrowck/suggest-local-var-imm-and-mut.rs
@@ -0,0 +1,27 @@
+// See issue #77834.
+
+#![crate_type = "lib"]
+
+mod method_syntax {
+    struct Foo;
+
+    impl Foo {
+        fn foo(&self, _: f32) -> i32 { todo!() }
+        fn bar(&mut self) -> f32 { todo!() }
+        fn baz(&mut self) {
+            self.foo(self.bar()); //~ ERROR
+        }
+    }
+}
+
+mod fully_qualified_syntax {
+    struct Foo;
+
+    impl Foo {
+        fn foo(&self, _: f32) -> i32 { todo!() }
+        fn bar(&mut self) -> f32 { todo!() }
+        fn baz(&mut self) {
+            Self::foo(self, Self::bar(self)); //~ ERROR
+        }
+    }
+}
diff --git a/src/test/ui/borrowck/suggest-local-var-imm-and-mut.stderr b/src/test/ui/borrowck/suggest-local-var-imm-and-mut.stderr
@@ -0,0 +1,22 @@
+error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
+  --> $DIR/suggest-local-var-imm-and-mut.rs:12:22
+   |
+LL |             self.foo(self.bar());
+   |             ---------^^^^^^^^^^-
+   |             |    |   |
+   |             |    |   mutable borrow occurs here
+   |             |    immutable borrow later used by call
+   |             immutable borrow occurs here
+
+error[E0502]: cannot borrow `*self` as mutable because it is also borrowed as immutable
+  --> $DIR/suggest-local-var-imm-and-mut.rs:24:29
+   |
+LL |             Self::foo(self, Self::bar(self));
+   |             --------- ----  ^^^^^^^^^^^^^^^ mutable borrow occurs here
+   |             |         |
+   |             |         immutable borrow occurs here
+   |             immutable borrow later used by call
+
+error: aborting due to 2 previous errors
+
+For more information about this error, try `rustc --explain E0502`.
diff --git a/src/test/ui/borrowck/two-phase-cannot-nest-mut-self-calls.stderr b/src/test/ui/borrowck/two-phase-cannot-nest-mut-self-calls.stderr
@@ -13,6 +13,23 @@ LL | |
 LL | |         0
 LL | |     });
    | |______- immutable borrow occurs here
+   |
+help: try adding a local storing this argument...
+  --> $DIR/two-phase-cannot-nest-mut-self-calls.rs:16:9
+   |
+LL |         vec.push(2);
+   |         ^^^^^^^^^^^
+help: ...and then using that local as the argument to this call
+  --> $DIR/two-phase-cannot-nest-mut-self-calls.rs:14:5
+   |
+LL | /     vec.get({
+LL | |
+LL | |         vec.push(2);
+LL | |
+LL | |
+LL | |         0
+LL | |     });
+   | |______^
 
 error: aborting due to previous error
 
diff --git a/src/test/ui/codemap_tests/one_line.stderr b/src/test/ui/codemap_tests/one_line.stderr
@@ -7,6 +7,17 @@ LL |     v.push(v.pop().unwrap());
    |     | |    second mutable borrow occurs here
    |     | first borrow later used by call
    |     first mutable borrow occurs here
+   |
+help: try adding a local storing this argument...
+  --> $DIR/one_line.rs:3:12
+   |
+LL |     v.push(v.pop().unwrap());
+   |            ^^^^^^^
+help: ...and then using that local as the argument to this call
+  --> $DIR/one_line.rs:3:5
+   |
+LL |     v.push(v.pop().unwrap());
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: aborting due to previous error
 
