Skip to content

Modify compile-fail/E0389 error message WIP #48914

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

Merged
merged 24 commits into from
Apr 10, 2018
Merged
Show file tree
Hide file tree
Changes from 7 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
2 changes: 1 addition & 1 deletion src/librustc_borrowck/borrowck/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -839,7 +839,7 @@ impl<'a, 'tcx> BorrowckCtxt<'a, 'tcx> {

let mut db = match err.cause {
MutabilityViolation => {
let mut db = self.cannot_assign(error_span, &descr, Origin::Ast);
let mut db = self.cannot_assign(error_span, &descr, Origin::Ast, false);
if let mc::NoteClosureEnv(upvar_id) = err.cmt.note {
let node_id = self.tcx.hir.hir_to_node_id(upvar_id.var_id);
let sp = self.tcx.hir.span(node_id);
Expand Down
80 changes: 62 additions & 18 deletions src/librustc_mir/borrow_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use rustc::hir::map::definitions::DefPathData;
use rustc::infer::InferCtxt;
use rustc::ty::{self, ParamEnv, TyCtxt};
use rustc::ty::maps::Providers;
use rustc::mir::{AssertMessage, BasicBlock, BorrowKind, Location, Place};
use rustc::mir::{AssertMessage, BasicBlock, BorrowKind, Local, Location, Place};
use rustc::mir::{Mir, Mutability, Operand, Projection, ProjectionElem, Rvalue};
use rustc::mir::{Field, Statement, StatementKind, Terminator, TerminatorKind};
use rustc::mir::ClosureRegionRequirements;
Expand All @@ -43,6 +43,7 @@ use dataflow::indexes::BorrowIndex;
use dataflow::move_paths::{IllegalMoveOriginKind, MoveError};
use dataflow::move_paths::{HasMoveData, LookupResult, MoveData, MovePathIndex};
use util::borrowck_errors::{BorrowckErrors, Origin};
use util::collect_writes::FindAssignments;

use std::iter;

Expand Down Expand Up @@ -1421,6 +1422,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
}
}

fn get_main_error_message(&self, place:&Place<'tcx>) -> String{
match self.describe_place(place) {
Some(name) => format!("immutable item `{}`", name),
None => "immutable item".to_owned(),
}
}

/// Currently MoveData does not store entries for all places in
/// the input MIR. For example it will currently filter out
/// places that are Copy; thus we do not track places of shared
Expand Down Expand Up @@ -1551,12 +1559,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
self.is_mutable(place, is_local_mutation_allowed)
{
error_reported = true;

let item_msg = match self.describe_place(place) {
Some(name) => format!("immutable item `{}`", name),
None => "immutable item".to_owned(),
};

let item_msg = self.get_main_error_message(place);
let mut err = self.tcx
.cannot_borrow_path_as_mutable(span, &item_msg, Origin::Mir);
err.span_label(span, "cannot borrow as mutable");
Expand All @@ -1573,21 +1576,61 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
if let Err(place_err) = self.is_mutable(place, is_local_mutation_allowed) {
error_reported = true;

let item_msg = match self.describe_place(place) {
Some(name) => format!("immutable item `{}`", name),
None => "immutable item".to_owned(),
let err_info = match *place_err {
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how deeply nested this is, it might make sense to move it to its own method, or at least adding a comment on top describing what the code is supposed to be doing.

Place::Projection(ref proj) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

You can match on multiple levels to avoid deeply nested structures:

enum Foo {
    A { f: Bar },
    B(Bar),
}
enum Bar {
    X,
    Y,
}
let x = Foo::B(Bar::X);
match x {
    Foo::A { f: Bar::Y } => 1,
    _ => 0,
}

match proj.elem {
ProjectionElem::Deref => {
match proj.base {
Place::Local(local) => {
let locations = self.mir.find_assignments(local);
if locations.len() > 0 {
let item_msg = if error_reported {
if let Some(name) =
self.describe_place(place_err) {
let var = str::replace(&name, "*", "");
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I'm not crazy about the string munging =) Maybe we should just do describe_place(&place.base)?

format!("`&`-reference `{}`", var)
} else {
self.get_main_error_message(place)
Copy link
Contributor

Choose a reason for hiding this comment

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

I would pull this into a helper function and I would change these get_main_error_message cases to just return None. The helper would be something like:

fn specialized_description(..) -> Option<String>

then if it is None you can use get_main_error_message(place).

}
} else {
self.get_main_error_message(place)
};
Some((self.mir.source_info(locations[0]).span,
"consider changing this to be a \
mutable reference: `&mut`", item_msg,
"cannot assign through `&`-reference"))
} else {
None
}
}
_ => None,
}
}
_ => None,
}
}
_ => None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Given how many branches there are here, it might make sense to make err_info mutable with a default value of None, and override where applicable, but this might not be needed if the amount of branches is reduced.

};

let mut err = self.tcx.cannot_assign(span, &item_msg, Origin::Mir);
err.span_label(span, "cannot mutate");

if place != place_err {
if let Some(name) = self.describe_place(place_err) {
err.note(&format!("Value not mutable causing this error: `{}`", name));
if let Some((err_help_span, err_help_stmt, item_msg, sec_span)) = err_info {
let mut err = self.tcx.cannot_assign(span, &item_msg, Origin::Mir, true);
err.span_suggestion(err_help_span, err_help_stmt, format!(""));
if place != place_err {
err.span_label(span, sec_span);
}
err.emit()
}else{
Copy link
Contributor

Choose a reason for hiding this comment

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

spaces around else

let item_msg_ = self.get_main_error_message(place);
let mut err = self.tcx.cannot_assign(span, &item_msg_, Origin::Mir, false);
err.span_label(span, "cannot mutate");
if place != place_err {
if let Some(name) = self.describe_place(place_err) {
err.note(&format!("Value not mutable causing this error: `{}`",
Copy link
Contributor

Choose a reason for hiding this comment

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

lowercase sentences: value not mutable

name));
}
}
err.emit();
}

err.emit();
}
}
Reservation(WriteKind::Move)
Expand Down Expand Up @@ -2230,3 +2273,4 @@ impl ContextKind {
}
}
}

1 change: 1 addition & 0 deletions src/librustc_mir/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ Rust MIR: a lowered representation of Rust. Also: an experiment!
#![feature(collection_placement)]
#![feature(nonzero)]
#![feature(underscore_lifetimes)]
#![feature(crate_visibility_modifier)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this needed?

Copy link
Author

@gaurikholkar-zz gaurikholkar-zz Mar 10, 2018

Choose a reason for hiding this comment

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

yes, due to the usage here crate trait FindAssignments { in collect_writes.rs


extern crate arena;
#[macro_use]
Expand Down
15 changes: 11 additions & 4 deletions src/librustc_mir/util/borrowck_errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,18 +284,25 @@ pub trait BorrowckErrors {
self.cancel_if_wrong_origin(err, o)
}

fn cannot_assign(&self, span: Span, desc: &str, o: Origin) -> DiagnosticBuilder
fn cannot_assign(&self, span: Span, desc: &str, o: Origin, is_reference:bool)
-> DiagnosticBuilder
{
let msg = if is_reference {
"through"
} else {
"to"
};

let err = struct_span_err!(self, span, E0594,
"cannot assign to {}{OGN}",
desc, OGN=o);
"cannot assign {} {}{OGN}",
msg, desc, OGN=o);
self.cancel_if_wrong_origin(err, o)
}

fn cannot_assign_static(&self, span: Span, desc: &str, o: Origin)
-> DiagnosticBuilder
{
self.cannot_assign(span, &format!("immutable static item `{}`", desc), o)
self.cannot_assign(span, &format!("immutable static item `{}`", desc), o, false)
}

fn cannot_move_out_of(&self, move_from_span: Span, move_from_desc: &str, o: Origin)
Expand Down
65 changes: 65 additions & 0 deletions src/librustc_mir/util/collect_writes.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
// Copyright 2018 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.

use rustc::mir::{Local, Location};
use rustc::mir::Mir;
use rustc::mir::visit::PlaceContext;
use rustc::mir::visit::Visitor;

// The Visitor walks the MIR to return the assignment statements corresponding
// to a Local.
pub struct FindLocalAssignmentVisitor {
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not need to be public, it is an impl detail. Just make it private.

needle: Local,
locations: Vec<Location>,
}

impl<'tcx> Visitor<'tcx> for FindLocalAssignmentVisitor {
fn visit_local(&mut self,
local: &Local,
place_context: PlaceContext<'tcx>,
location: Location) {
if self.needle != *local {
return;
}

match place_context {
PlaceContext::Store | PlaceContext::Call => {
self.locations.push(location);
}
PlaceContext::AsmOutput |
PlaceContext::Drop |
PlaceContext::Inspect |
PlaceContext::Borrow { .. } |
PlaceContext::Projection(..) |
PlaceContext::Copy |
PlaceContext::Move |
PlaceContext::StorageLive |
PlaceContext::StorageDead |
PlaceContext::Validate => {
// TO-DO
// self.super_local(local)
}
}
}
// TO-DO
// fn super_local()
}

crate trait FindAssignments {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this to the top of the file -- this is what readers care about. Also, I would add a comment to the method:


Finds all statements that assign directly to local (i.e., X = ...) and returns their locations.

fn find_assignments(&self, local: Local) -> Vec<Location>;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: formatting


impl<'tcx> FindAssignments for Mir<'tcx>{
fn find_assignments(&self, local: Local) -> Vec<Location>{
let mut visitor = FindLocalAssignmentVisitor{ needle: local, locations: vec![]};
visitor.visit_mir(self);
visitor.locations
}
}
1 change: 1 addition & 0 deletions src/librustc_mir/util/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ mod alignment;
mod graphviz;
pub(crate) mod pretty;
pub mod liveness;
pub mod collect_writes;

pub use self::alignment::is_disaligned;
pub use self::pretty::{dump_enabled, dump_mir, write_mir_pretty, PassWhere};
Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/borrowck/borrowck-issue-14498.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ fn indirect_write_to_imm_box() {
let y: Box<_> = box &mut x;
let p = &y;
***p = 2; //[ast]~ ERROR cannot assign to data in a `&` reference
//[mir]~^ ERROR cannot assign to immutable item `***p`
//[mir]~^ ERROR cannot assign through `&`-reference `p`
Copy link
Contributor

Choose a reason for hiding this comment

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

Not convinced about this change...

Copy link
Contributor

Choose a reason for hiding this comment

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

certainly better than it was ("imutable item"), but not I think perfect yet -- and not necessarily better than AST variant, I don't think

Copy link
Contributor

Choose a reason for hiding this comment

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

The AST based error is this:

error[E0389]: cannot assign to data in a `&` reference
 --> src/main.rs:8:5
  |
8 |     ***p = 2;
  |     ^^^^^^^^ assignment into an immutable reference

which I think has too much jargon, but some promising elements. This also intersects your style guide @estebank. Personally, my preference is that the "main" message is kind of generic and formal, like it is here, telling you nothing of the program itself, but that those details are revealed in the "underlines" and in a "conversational" tone. Maybe something like this?

error[E0389]: cannot assign to data in a `&` reference
 --> src/main.rs:8:5
  |
8 |     ***p = 2;
  |     ^^^^^^^^ `p` is a `&` reference, so the data it refers to cannot be written

Copy link
Author

Choose a reason for hiding this comment

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

Is this the common message for all such cases or just for the above case?

Copy link
Author

Choose a reason for hiding this comment

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

error[E0594]: cannot assign to data in a `&` reference
  --> src/test/compile-fail/borrowck/borrowck-issue-14498.rs:29:5
   |
28 |     let p = &y;
   |             -- help: consider changing this to be a mutable reference: `&mut`
29 |     ***p = 2;
   |     ^^^^^^^^ `p` is a `&` reference, so the data it refers to cannot be written

is how the message looks like now @nikomatsakis

drop(p);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,5 +70,5 @@ fn main() {
};
s[2] = 20;
//[ast]~^ ERROR cannot assign to immutable indexed content
//[mir]~^^ ERROR cannot assign to immutable item
//[mir]~^^ ERROR cannot assign through immutable item
Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure about the look of this test though.

Copy link
Contributor

Choose a reason for hiding this comment

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

This one seems ok-ish to me, but we might be able to improve upon it. I'll think about it.

}