From e27315268b10c9ef2f4c3d815dfc79f513327def Mon Sep 17 00:00:00 2001 From: Noah Lev Date: Mon, 15 Mar 2021 15:09:06 -0700 Subject: [PATCH] Suggest using a temporary variable to fix borrowck errors 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. --- .../src/diagnostics/conflict_errors.rs | 89 ++++++++++++++++++- .../src/diagnostics/find_all_local_uses.rs | 26 ++++++ .../rustc_borrowck/src/diagnostics/mod.rs | 1 + compiler/rustc_middle/src/mir/mod.rs | 14 +++ .../borrowck/suggest-local-var-double-mut.rs | 27 ++++++ .../suggest-local-var-double-mut.stderr | 44 +++++++++ .../suggest-local-var-imm-and-mut.nll.stderr | 33 +++++++ .../borrowck/suggest-local-var-imm-and-mut.rs | 27 ++++++ .../suggest-local-var-imm-and-mut.stderr | 22 +++++ ...wo-phase-cannot-nest-mut-self-calls.stderr | 17 ++++ src/test/ui/codemap_tests/one_line.stderr | 11 +++ 11 files changed, 309 insertions(+), 2 deletions(-) create mode 100644 compiler/rustc_borrowck/src/diagnostics/find_all_local_uses.rs create mode 100644 src/test/ui/borrowck/suggest-local-var-double-mut.rs create mode 100644 src/test/ui/borrowck/suggest-local-var-double-mut.stderr create mode 100644 src/test/ui/borrowck/suggest-local-var-imm-and-mut.nll.stderr create mode 100644 src/test/ui/borrowck/suggest-local-var-imm-and-mut.rs create mode 100644 src/test/ui/borrowck/suggest-local-var-imm-and-mut.stderr diff --git a/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs b/compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs index 8c4508ed18882..98c619cdd291c 100644 --- 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 new file mode 100644 index 0000000000000..49d9caae71144 --- /dev/null +++ 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 { + let mut visitor = AllLocalUsesVisitor { for_local: local, uses: BTreeSet::default() }; + visitor.visit_body(body); + visitor.uses +} + +struct AllLocalUsesVisitor { + for_local: Local, + uses: BTreeSet, +} + +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 index 42f5d55754208..dec1940ace881 100644 --- 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 index d4530883b6a10..afd8083dfe46a 100644 --- 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 new file mode 100644 index 0000000000000..d5996ba68be53 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..3a43c18a7ed52 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..2ba0b6b28aaaa --- /dev/null +++ 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 new file mode 100644 index 0000000000000..bf167ba79f311 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..eb934e7b72b08 --- /dev/null +++ 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 index a89bb941532b6..85c7159952ffa 100644 --- 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 index 1ee612184def2..6fe6e26135b92 100644 --- 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