Skip to content

Commit

Permalink
Point to immutable arg/fields when trying to use as &mut
Browse files Browse the repository at this point in the history
Point to immutable borrow arguments and fields when trying to use them as
mutable borrows. Add label to primary span on "cannot borrow as mutable"
errors.

Present the following output when trying to access an immutable borrow's
field as mutable:

```
error[E0389]: cannot borrow data mutably in a `&` reference
  --> $DIR/issue-38147-1.rs:27:9
   |
26 | fn f(&self) {
   |      -----  use `&mut self` here to make mutable
27 |     f.s.push('x');
   |     ^^^ assignment into an immutable reference
```

And the following when trying to access an immutable struct field as mutable:

```
error: cannot borrow immutable borrowed content `*self.s` as mutable
  --> $DIR/issue-38147-3.rs:17:9
   |
12 |     s: &'a String
   |     ------------- use `&'a mut String` here to make mutable
...|
16 |     fn f(&self) {
   |          -----  use `&mut self` here to make mutable
17 |         self.s.push('x');
   |         ^^^^^^ cannot borrow as mutable
```
  • Loading branch information
estebank committed Jan 26, 2017
1 parent 491b978 commit e1280d8
Show file tree
Hide file tree
Showing 21 changed files with 310 additions and 67 deletions.
2 changes: 1 addition & 1 deletion src/librustc/hir/map/blocks.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ use syntax_pos::Span;
/// - The default implementation for a trait method.
///
/// To construct one, use the `Code::from_node` function.
#[derive(Copy, Clone)]
#[derive(Copy, Clone, Debug)]
pub struct FnLikeNode<'a> { node: map::Node<'a> }

/// MaybeFnLike wraps a method that indicates if an object
Expand Down
57 changes: 57 additions & 0 deletions src/librustc/middle/mem_categorization.rs
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,63 @@ pub struct cmt_<'tcx> {

pub type cmt<'tcx> = Rc<cmt_<'tcx>>;

impl<'tcx> cmt_<'tcx> {
pub fn get_field(&self, name: ast::Name) -> Option<DefId> {
match self.cat {
Categorization::Deref(ref cmt, ..) |
Categorization::Interior(ref cmt, _) |
Categorization::Downcast(ref cmt, _) => {
if let Categorization::Local(_) = cmt.cat {
if let ty::TyAdt(def, _) = self.ty.sty {
return def.struct_variant().find_field_named(name).map(|x| x.did);
}
None
} else {
cmt.get_field(name)
}
}
_ => None
}
}

pub fn get_field_name(&self) -> Option<ast::Name> {
match self.cat {
Categorization::Interior(_, ref ik) => {
if let InteriorKind::InteriorField(FieldName::NamedField(name)) = *ik {
Some(name)
} else {
None
}
}
Categorization::Deref(ref cmt, ..) |
Categorization::Downcast(ref cmt, _) => {
cmt.get_field_name()
}
_ => None,
}
}

pub fn get_arg_if_immutable(&self, map: &hir_map::Map) -> Option<ast::NodeId> {
match self.cat {
Categorization::Deref(ref cmt, ..) |
Categorization::Interior(ref cmt, _) |
Categorization::Downcast(ref cmt, _) => {
if let Categorization::Local(nid) = cmt.cat {
if let ty::TyAdt(_, _) = self.ty.sty {
if let ty::TyRef(_, ty::TypeAndMut{mutbl: MutImmutable, ..}) = cmt.ty.sty {
return Some(nid);
}
}
None
} else {
cmt.get_arg_if_immutable(map)
}
}
_ => None
}
}
}

pub trait ast_node {
fn id(&self) -> ast::NodeId;
fn span(&self) -> Span;
Expand Down
2 changes: 2 additions & 0 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1325,6 +1325,7 @@ bitflags! {
}
}

#[derive(Debug)]
pub struct VariantDef {
/// The variant's DefId. If this is a tuple-like struct,
/// this is the DefId of the struct's ctor.
Expand All @@ -1335,6 +1336,7 @@ pub struct VariantDef {
pub ctor_kind: CtorKind,
}

#[derive(Debug)]
pub struct FieldDef {
pub did: DefId,
pub name: Name,
Expand Down
2 changes: 1 addition & 1 deletion src/librustc/ty/sty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,7 @@ pub enum TypeVariants<'tcx> {
TyRawPtr(TypeAndMut<'tcx>),

/// A reference; a pointer with an associated lifetime. Written as
/// `&a mut T` or `&'a T`.
/// `&'a mut T` or `&'a T`.
TyRef(&'tcx Region, TypeAndMut<'tcx>),

/// The anonymous type of a function declaration/definition. Each
Expand Down
6 changes: 4 additions & 2 deletions src/librustc_borrowck/borrowck/gather_loans/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -195,15 +195,17 @@ fn check_aliasability<'a, 'tcx>(bccx: &BorrowckCtxt<'a, 'tcx>,
bccx.report_aliasability_violation(
borrow_span,
loan_cause,
mc::AliasableReason::UnaliasableImmutable);
mc::AliasableReason::UnaliasableImmutable,
cmt);
Err(())
}
(mc::Aliasability::FreelyAliasable(alias_cause), ty::UniqueImmBorrow) |
(mc::Aliasability::FreelyAliasable(alias_cause), ty::MutBorrow) => {
bccx.report_aliasability_violation(
borrow_span,
loan_cause,
alias_cause);
alias_cause,
cmt);
Err(())
}
(..) => {
Expand Down
140 changes: 92 additions & 48 deletions src/librustc_borrowck/borrowck/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,7 +540,7 @@ pub fn opt_loan_path<'tcx>(cmt: &mc::cmt<'tcx>) -> Option<Rc<LoanPath<'tcx>>> {
// Errors

// Errors that can occur
#[derive(PartialEq)]
#[derive(Debug, PartialEq)]
pub enum bckerr_code<'tcx> {
err_mutbl,
/// superscope, subscope, loan cause
Expand All @@ -550,7 +550,7 @@ pub enum bckerr_code<'tcx> {

// Combination of an error code and the categorization of the expression
// that caused it
#[derive(PartialEq)]
#[derive(Debug, PartialEq)]
pub struct BckError<'tcx> {
span: Span,
cause: AliasableViolationKind,
Expand Down Expand Up @@ -601,12 +601,8 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
_ => { }
}

// General fallback.
let span = err.span.clone();
let mut db = self.struct_span_err(
err.span,
&self.bckerr_to_string(&err));
self.note_and_explain_bckerr(&mut db, err, span);
let mut db = self.bckerr_to_diag(&err);
self.note_and_explain_bckerr(&mut db, err);
db.emit();
}

Expand Down Expand Up @@ -771,8 +767,11 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
self.tcx.sess.span_err_with_code(s, msg, code);
}

pub fn bckerr_to_string(&self, err: &BckError<'tcx>) -> String {
match err.code {
pub fn bckerr_to_diag(&self, err: &BckError<'tcx>) -> DiagnosticBuilder<'a> {
let span = err.span.clone();
let mut immutable_field = None;

let msg = &match err.code {
err_mutbl => {
let descr = match err.cmt.note {
mc::NoteClosureEnv(_) | mc::NoteUpvarRef(_) => {
Expand All @@ -783,6 +782,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
format!("{} {}",
err.cmt.mutbl.to_user_str(),
self.cmt_to_string(&err.cmt))

}
Some(lp) => {
format!("{} {} `{}`",
Expand All @@ -807,6 +807,19 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
BorrowViolation(euv::AutoUnsafe) |
BorrowViolation(euv::ForLoop) |
BorrowViolation(euv::MatchDiscriminant) => {
// Check for this field's definition to see if it is an immutable reference
// and suggest making it mutable if that is the case.
immutable_field = err.cmt.get_field_name()
.and_then(|name| err.cmt.get_field(name))
.and_then(|did| self.tcx.hir.as_local_node_id(did))
.and_then(|nid| {
if let hir_map::Node::NodeField(ref field) = self.tcx.hir.get(nid) {
return self.suggest_mut_for_immutable(&field.ty)
.map(|msg| (self.tcx.hir.span(nid), msg));
}
None
});

format!("cannot borrow {} as mutable", descr)
}
BorrowViolation(euv::ClosureInvocation) => {
Expand All @@ -830,13 +843,20 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
its contents can be safely reborrowed",
descr)
}
};

let mut db = self.struct_span_err(span, msg);
if let Some((span, msg)) = immutable_field {
db.span_label(span, &msg);
}
db
}

pub fn report_aliasability_violation(&self,
span: Span,
kind: AliasableViolationKind,
cause: mc::AliasableReason) {
cause: mc::AliasableReason,
cmt: mc::cmt<'tcx>) {
let mut is_closure = false;
let prefix = match kind {
MutabilityViolation => {
Expand Down Expand Up @@ -903,6 +923,9 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
self.tcx.sess, span, E0389,
"{} in a `&` reference", prefix);
e.span_label(span, &"assignment into an immutable reference");
if let Some(nid) = cmt.get_arg_if_immutable(&self.tcx.hir) {
self.immutable_argument_should_be_mut(nid, &mut e);
}
e
}
};
Expand All @@ -913,6 +936,55 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
err.emit();
}

/// Given a type, if it is an immutable reference, return a suggestion to make it mutable
fn suggest_mut_for_immutable(&self, pty: &hir::Ty) -> Option<String> {
// Check wether the argument is an immutable reference
if let hir::TyRptr(opt_lifetime, hir::MutTy {
mutbl: hir::Mutability::MutImmutable,
ref ty
}) = pty.node {
// Account for existing lifetimes when generating the message
if let Some(lifetime) = opt_lifetime {
if let Ok(snippet) = self.tcx.sess.codemap().span_to_snippet(ty.span) {
if let Ok(lifetime_snippet) = self.tcx.sess.codemap()
.span_to_snippet(lifetime.span) {
return Some(format!("use `&{} mut {}` here to make mutable",
lifetime_snippet,
snippet));
}
}
} else if let Ok(snippet) = self.tcx.sess.codemap().span_to_snippet(pty.span) {
if snippet.starts_with("&") {
return Some(format!("use `{}` here to make mutable",
snippet.replace("&", "&mut ")));
}
} else {
bug!("couldn't find a snippet for span: {:?}", pty.span);
}
}
None
}

fn immutable_argument_should_be_mut(&self, nid: ast::NodeId, db: &mut DiagnosticBuilder) {
let parent = self.tcx.hir.get_parent_node(nid);
let parent_node = self.tcx.hir.get(parent);

// The parent node is like a fn
if let Some(fn_like) = FnLikeNode::from_node(parent_node) {
// `nid`'s parent's `Body`
let fn_body = self.tcx.hir.body(fn_like.body());
// Get the position of `nid` in the arguments list
let arg_pos = fn_body.arguments.iter().position(|arg| arg.pat.id == nid);
if let Some(i) = arg_pos {
// The argument's `Ty`
let arg_ty = &fn_like.decl().inputs[i];
if let Some(msg) = self.suggest_mut_for_immutable(&arg_ty) {
db.span_label(arg_ty.span, &msg);
}
}
}
}

fn report_out_of_scope_escaping_closure_capture(&self,
err: &BckError<'tcx>,
capture_span: Span)
Expand Down Expand Up @@ -961,8 +1033,8 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
}
}

pub fn note_and_explain_bckerr(&self, db: &mut DiagnosticBuilder, err: BckError<'tcx>,
error_span: Span) {
pub fn note_and_explain_bckerr(&self, db: &mut DiagnosticBuilder, err: BckError<'tcx>) {
let error_span = err.span.clone();
match err.code {
err_mutbl => self.note_and_explain_mutbl_error(db, &err, &error_span),
err_out_of_scope(super_scope, sub_scope, cause) => {
Expand Down Expand Up @@ -1103,41 +1175,13 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {
}
}
_ => {
if let Categorization::Deref(ref inner_cmt, ..) = err.cmt.cat {
if let Categorization::Local(local_id) = inner_cmt.cat {
let parent = self.tcx.hir.get_parent_node(local_id);

if let Some(fn_like) = FnLikeNode::from_node(self.tcx.hir.get(parent)) {
if let Some(i) = self.tcx.hir.body(fn_like.body()).arguments.iter()
.position(|arg| arg.pat.id == local_id) {
let arg_ty = &fn_like.decl().inputs[i];
if let hir::TyRptr(
opt_lifetime,
hir::MutTy{mutbl: hir::Mutability::MutImmutable, ref ty}) =
arg_ty.node {
if let Some(lifetime) = opt_lifetime {
if let Ok(snippet) = self.tcx.sess.codemap()
.span_to_snippet(ty.span) {
if let Ok(lifetime_snippet) = self.tcx.sess.codemap()
.span_to_snippet(lifetime.span) {
db.span_label(arg_ty.span,
&format!("use `&{} mut {}` \
here to make mutable",
lifetime_snippet,
snippet));
}
}
}
else if let Ok(snippet) = self.tcx.sess.codemap()
.span_to_snippet(arg_ty.span) {
if snippet.starts_with("&") {
db.span_label(arg_ty.span,
&format!("use `{}` here to make mutable",
snippet.replace("&", "&mut ")));
}
}
}
}
if let Categorization::Deref(..) = err.cmt.cat {
db.span_label(*error_span, &"cannot borrow as mutable");
if let Some(local_id) = err.cmt.get_arg_if_immutable(&self.tcx.hir) {
self.immutable_argument_should_be_mut(local_id, db);
} else if let Categorization::Deref(ref inner_cmt, ..) = err.cmt.cat {
if let Categorization::Local(local_id) = inner_cmt.cat {
self.immutable_argument_should_be_mut(local_id, db);
}
}
} else if let Categorization::Local(local_id) = err.cmt.cat {
Expand Down
31 changes: 31 additions & 0 deletions src/test/ui/did_you_mean/issue-38147-1.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
// Copyright 2017 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

struct Pass<'a> {
s: &'a mut String
}

impl<'a> Pass<'a> {
fn f(&mut self) {
self.s.push('x');
}
}

struct Foo<'a> {
s: &'a mut String
}

impl<'a> Foo<'a> {
fn f(&self) {
self.s.push('x');
}
}

fn main() {}
10 changes: 10 additions & 0 deletions src/test/ui/did_you_mean/issue-38147-1.stderr
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
error[E0389]: cannot borrow data mutably in a `&` reference
--> $DIR/issue-38147-1.rs:27:9
|
26 | fn f(&self) {
| ----- use `&mut self` here to make mutable
27 | self.s.push('x');
| ^^^^^^ assignment into an immutable reference

error: aborting due to previous error

Loading

0 comments on commit e1280d8

Please sign in to comment.