Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suggest using a temporary variable to fix borrowck errors #83174

Merged
merged 1 commit into from
Dec 11, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
89 changes: 87 additions & 2 deletions compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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, _));
Comment on lines +795 to +796
Copy link
Member Author

@camelid camelid Oct 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One idea I have is to track the call's Location in the UsedLater, instead of (or in addition to) the span. However, the "later use" is sometimes incorrect for our purposes; e.g., it may refer to a closure that also borrows place, even if it's not the outer call.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could point at both spans, would that help?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or just at the new span if it isn't in a closure?

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<'_>,
Expand Down
26 changes: 26 additions & 0 deletions compiler/rustc_borrowck/src/diagnostics/find_all_local_uses.rs
Original file line number Diff line number Diff line change
@@ -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);
}
}
}
1 change: 1 addition & 0 deletions compiler/rustc_borrowck/src/diagnostics/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
14 changes: 14 additions & 0 deletions compiler/rustc_middle/src/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand All @@ -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};
Expand Down Expand Up @@ -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)
Expand Down
27 changes: 27 additions & 0 deletions src/test/ui/borrowck/suggest-local-var-double-mut.rs
Original file line number Diff line number Diff line change
@@ -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
}
}
}
44 changes: 44 additions & 0 deletions src/test/ui/borrowck/suggest-local-var-double-mut.stderr
Original file line number Diff line number Diff line change
@@ -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
camelid marked this conversation as resolved.
Show resolved Hide resolved
|
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());
| ^^^^^^^^^^^^^^^^^^^^
camelid marked this conversation as resolved.
Show resolved Hide resolved

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`.
33 changes: 33 additions & 0 deletions src/test/ui/borrowck/suggest-local-var-imm-and-mut.nll.stderr
Original file line number Diff line number Diff line change
@@ -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());
| ^^^^^^^^^^^^^^^^^^^^
Comment on lines +11 to +20
Copy link
Member Author

@camelid camelid Dec 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's intriguing that the suggestion doesn't show up in normal mode but does (correctly) show up in nll mode.


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`.
27 changes: 27 additions & 0 deletions src/test/ui/borrowck/suggest-local-var-imm-and-mut.rs
Original file line number Diff line number Diff line change
@@ -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
}
}
}
22 changes: 22 additions & 0 deletions src/test/ui/borrowck/suggest-local-var-imm-and-mut.stderr
Original file line number Diff line number Diff line change
@@ -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
camelid marked this conversation as resolved.
Show resolved Hide resolved

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`.
17 changes: 17 additions & 0 deletions src/test/ui/borrowck/two-phase-cannot-nest-mut-self-calls.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
11 changes: 11 additions & 0 deletions src/test/ui/codemap_tests/one_line.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -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());
| ^^^^^^^
camelid marked this conversation as resolved.
Show resolved Hide resolved
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

Expand Down