Skip to content

Commit

Permalink
Rollup merge of rust-lang#39913 - nikomatsakis:inference-error, r=pnk…
Browse files Browse the repository at this point in the history 
…felix

Report full details of inference errors

When the old suggestion machinery was removed by @brson in rust-lang#37057, it was not completely removed. There was a bit of code that had the job of going through errors and finding those for which suggestions were applicable, and it remained, causing us not to emit the full details of such errors.  This PR removes that.

I've also added various lifetime tests to the UI test suite (so you can also see the before/after there). I have some concrete thoughts on how to improve these cases and am planning on writing those up in some mentoring issues (@cengizio has expressed interest in working on those changes, so I plan to work with him on it, at least to start).

cc @jonathandturner
  • Loading branch information
@frewsxcv
frewsxcv authored Feb 20, 2017
2 parents 5b7c556 + 75da4b6 commit 8918ceb
Show file tree
Hide file tree
Showing 18 changed files with 362 additions and 260 deletions.
245 changes: 24 additions & 221 deletions src/librustc/infer/error_reporting.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,9 +65,6 @@ use super::region_inference::ConcreteFailure;
use super::region_inference::SubSupConflict;
use super::region_inference::GenericBoundFailure;
use super::region_inference::GenericKind;
use super::region_inference::ProcessedErrors;
use super::region_inference::ProcessedErrorOrigin;
use super::region_inference::SameRegions;

use hir::map as hir_map;
use hir;
Expand All @@ -77,11 +74,10 @@ use infer;
use middle::region;
use traits::{ObligationCause, ObligationCauseCode};
use ty::{self, TyCtxt, TypeFoldable};
use ty::{Region, ReFree, Issue32330};
use ty::{Region, Issue32330};
use ty::error::TypeError;

use std::fmt;
use syntax::ast;
use syntax_pos::{Pos, Span};
use errors::DiagnosticBuilder;

Expand Down Expand Up @@ -255,8 +251,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {

// try to pre-process the errors, which will group some of them
// together into a `ProcessedErrors` group:
let processed_errors = self.process_errors(errors);
let errors = processed_errors.as_ref().unwrap_or(errors);
let errors = self.process_errors(errors);

debug!("report_region_errors: {} errors after preprocessing", errors.len());

Expand All @@ -278,13 +273,6 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
sub_origin, sub_r,
sup_origin, sup_r);
}

ProcessedErrors(ref origins,
ref same_regions) => {
if !same_regions.is_empty() {
self.report_processed_errors(origins);
}
}
}
}
}
Expand All @@ -300,202 +288,31 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
// duplicates that will be unhelpful to the end-user. But
// obviously it never weeds out ALL errors.
fn process_errors(&self, errors: &Vec<RegionResolutionError<'tcx>>)
-> Option<Vec<RegionResolutionError<'tcx>>> {
-> Vec<RegionResolutionError<'tcx>> {
debug!("process_errors()");
let mut origins = Vec::new();

// we collect up ConcreteFailures and SubSupConflicts that are
// relating free-regions bound on the fn-header and group them
// together into this vector
let mut same_regions = Vec::new();

// here we put errors that we will not be able to process nicely
let mut other_errors = Vec::new();

// we collect up GenericBoundFailures in here.
let mut bound_failures = Vec::new();

for error in errors {
// Check whether we can process this error into some other
// form; if not, fall through.
match *error {
ConcreteFailure(ref origin, sub, sup) => {
debug!("processing ConcreteFailure");
if let SubregionOrigin::CompareImplMethodObligation { .. } = *origin {
// When comparing an impl method against a
// trait method, it is not helpful to suggest
// changes to the impl method. This is
// because the impl method signature is being
// checked using the trait's environment, so
// usually the changes we suggest would
// actually have to be applied to the *trait*
// method (and it's not clear that the trait
// method is even under the user's control).
} else if let Some(same_frs) = free_regions_from_same_fn(self.tcx, sub, sup) {
origins.push(
ProcessedErrorOrigin::ConcreteFailure(
origin.clone(),
sub,
sup));
append_to_same_regions(&mut same_regions, &same_frs);
continue;
}
}
SubSupConflict(ref var_origin, ref sub_origin, sub, ref sup_origin, sup) => {
debug!("processing SubSupConflict sub: {:?} sup: {:?}", sub, sup);
match (sub_origin, sup_origin) {
(&SubregionOrigin::CompareImplMethodObligation { .. }, _) => {
// As above, when comparing an impl method
// against a trait method, it is not helpful
// to suggest changes to the impl method.
}
(_, &SubregionOrigin::CompareImplMethodObligation { .. }) => {
// See above.
}
_ => {
if let Some(same_frs) = free_regions_from_same_fn(self.tcx, sub, sup) {
origins.push(
ProcessedErrorOrigin::VariableFailure(
var_origin.clone()));
append_to_same_regions(&mut same_regions, &same_frs);
continue;
}
}
}
}
GenericBoundFailure(ref origin, ref kind, region) => {
bound_failures.push((origin.clone(), kind.clone(), region));
continue;
}
ProcessedErrors(..) => {
bug!("should not encounter a `ProcessedErrors` yet: {:?}", error)
}
}

// No changes to this error.
other_errors.push(error.clone());
}

// ok, let's pull together the errors, sorted in an order that
// we think will help user the best
let mut processed_errors = vec![];

// first, put the processed errors, if any
if !same_regions.is_empty() {
let common_scope_id = same_regions[0].scope_id;
for sr in &same_regions {
// Since ProcessedErrors is used to reconstruct the function
// declaration, we want to make sure that they are, in fact,
// from the same scope
if sr.scope_id != common_scope_id {
debug!("returning empty result from process_errors because
{} != {}", sr.scope_id, common_scope_id);
return None;
}
}
assert!(origins.len() > 0);
let pe = ProcessedErrors(origins, same_regions);
debug!("errors processed: {:?}", pe);
processed_errors.push(pe);
}

// next, put the other misc errors
processed_errors.extend(other_errors);

// finally, put the `T: 'a` errors, but only if there were no
// other errors. otherwise, these have a very high rate of
// being unhelpful in practice. This is because they are
// basically secondary checks that test the state of the
// region graph after the rest of inference is done, and the
// other kinds of errors indicate that the region constraint
// graph is internally inconsistent, so these test results are
// likely to be meaningless.
if processed_errors.is_empty() {
for (origin, kind, region) in bound_failures {
processed_errors.push(GenericBoundFailure(origin, kind, region));
}
}

// we should always wind up with SOME errors, unless there were no
// errors to start
assert!(if errors.len() > 0 {processed_errors.len() > 0} else {true});

return Some(processed_errors);

#[derive(Debug)]
struct FreeRegionsFromSameFn {
sub_fr: ty::FreeRegion,
sup_fr: ty::FreeRegion,
scope_id: ast::NodeId
}

impl FreeRegionsFromSameFn {
fn new(sub_fr: ty::FreeRegion,
sup_fr: ty::FreeRegion,
scope_id: ast::NodeId)
-> FreeRegionsFromSameFn {
FreeRegionsFromSameFn {
sub_fr: sub_fr,
sup_fr: sup_fr,
scope_id: scope_id
}
}
}

fn free_regions_from_same_fn<'a, 'gcx, 'tcx>(tcx: TyCtxt<'a, 'gcx, 'tcx>,
sub: &'tcx Region,
sup: &'tcx Region)
-> Option<FreeRegionsFromSameFn> {
debug!("free_regions_from_same_fn(sub={:?}, sup={:?})", sub, sup);
let (scope_id, fr1, fr2) = match (sub, sup) {
(&ReFree(fr1), &ReFree(fr2)) => {
if fr1.scope != fr2.scope {
return None
}
assert!(fr1.scope == fr2.scope);
(fr1.scope.node_id(&tcx.region_maps), fr1, fr2)
},
_ => return None
};
let parent = tcx.hir.get_parent(scope_id);
let parent_node = tcx.hir.find(parent);
match parent_node {
Some(node) => match node {
hir_map::NodeItem(item) => match item.node {
hir::ItemFn(..) => {
Some(FreeRegionsFromSameFn::new(fr1, fr2, scope_id))
},
_ => None
},
hir_map::NodeImplItem(..) |
hir_map::NodeTraitItem(..) => {
Some(FreeRegionsFromSameFn::new(fr1, fr2, scope_id))
},
_ => None
},
None => {
debug!("no parent node of scope_id {}", scope_id);
None
}
}
}
// We want to avoid reporting generic-bound failures if we can
// avoid it: these have a very high rate of being unhelpful in
// practice. This is because they are basically secondary
// checks that test the state of the region graph after the
// rest of inference is done, and the other kinds of errors
// indicate that the region constraint graph is internally
// inconsistent, so these test results are likely to be
// meaningless.
//
// Therefore, we filter them out of the list unless they are
// the only thing in the list.

let is_bound_failure = |e: &RegionResolutionError<'tcx>| match *e {
ConcreteFailure(..) => false,
SubSupConflict(..) => false,
GenericBoundFailure(..) => true,
};

fn append_to_same_regions(same_regions: &mut Vec<SameRegions>,
same_frs: &FreeRegionsFromSameFn) {
debug!("append_to_same_regions(same_regions={:?}, same_frs={:?})",
same_regions, same_frs);
let scope_id = same_frs.scope_id;
let (sub_fr, sup_fr) = (same_frs.sub_fr, same_frs.sup_fr);
for sr in same_regions.iter_mut() {
if sr.contains(&sup_fr.bound_region) && scope_id == sr.scope_id {
sr.push(sub_fr.bound_region);
return
}
}
same_regions.push(SameRegions {
scope_id: scope_id,
regions: vec![sub_fr.bound_region, sup_fr.bound_region]
})
if errors.iter().all(|e| is_bound_failure(e)) {
errors.clone()
} else {
errors.iter().filter(|&e| !is_bound_failure(e)).cloned().collect()
}
}

Expand Down Expand Up @@ -1072,20 +889,6 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
self.note_region_origin(&mut err, &sub_origin);
err.emit();
}

fn report_processed_errors(&self,
origins: &[ProcessedErrorOrigin<'tcx>]) {
for origin in origins.iter() {
let mut err = match *origin {
ProcessedErrorOrigin::VariableFailure(ref var_origin) =>
self.report_inference_failure(var_origin.clone()),
ProcessedErrorOrigin::ConcreteFailure(ref sr_origin, sub, sup) =>
self.report_concrete_failure(sr_origin.clone(), sub, sup),
};

err.emit();
}
}
}

impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
Expand Down
36 changes: 1 addition & 35 deletions src/librustc/infer/region_inference/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ use rustc_data_structures::graph::{self, Direction, NodeIndex, OUTGOING};
use rustc_data_structures::unify::{self, UnificationTable};
use middle::free_region::FreeRegionMap;
use ty::{self, Ty, TyCtxt};
use ty::{BoundRegion, Region, RegionVid};
use ty::{Region, RegionVid};
use ty::{ReEmpty, ReStatic, ReFree, ReEarlyBound, ReErased};
use ty::{ReLateBound, ReScope, ReVar, ReSkolemized, BrFresh};

Expand Down Expand Up @@ -171,13 +171,6 @@ pub enum RegionResolutionError<'tcx> {
&'tcx Region,
SubregionOrigin<'tcx>,
&'tcx Region),

/// For subsets of `ConcreteFailure` and `SubSupConflict`, we can derive
/// more specific errors message by suggesting to the user where they
/// should put a lifetime. In those cases we process and put those errors
/// into `ProcessedErrors` before we do any reporting.
ProcessedErrors(Vec<ProcessedErrorOrigin<'tcx>>,
Vec<SameRegions>),
}

#[derive(Clone, Debug)]
Expand All @@ -186,33 +179,6 @@ pub enum ProcessedErrorOrigin<'tcx> {
VariableFailure(RegionVariableOrigin),
}

/// SameRegions is used to group regions that we think are the same and would
/// like to indicate so to the user.
/// For example, the following function
/// ```
/// struct Foo { bar: i32 }
/// fn foo2<'a, 'b>(x: &'a Foo) -> &'b i32 {
/// &x.bar
/// }
/// ```
/// would report an error because we expect 'a and 'b to match, and so we group
/// 'a and 'b together inside a SameRegions struct
#[derive(Clone, Debug)]
pub struct SameRegions {
pub scope_id: ast::NodeId,
pub regions: Vec<BoundRegion>,
}

impl SameRegions {
pub fn contains(&self, other: &BoundRegion) -> bool {
self.regions.contains(other)
}

pub fn push(&mut self, other: BoundRegion) {
self.regions.push(other);
}
}

pub type CombineMap<'tcx> = FxHashMap<TwoRegions<'tcx>, RegionVid>;

pub struct RegionVarBindings<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
Expand Down
5 changes: 1 addition & 4 deletions src/test/compile-fail/issue-17728.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,6 @@ impl Debug for Player {

fn str_to_direction(to_parse: &str) -> RoomDirection {
match to_parse { //~ ERROR match arms have incompatible types
//~^ expected enum `RoomDirection`, found enum `std::option::Option`
//~| expected type `RoomDirection`
//~| found type `std::option::Option<_>`
"w" | "west" => RoomDirection::West,
"e" | "east" => RoomDirection::East,
"n" | "north" => RoomDirection::North,
Expand All @@ -119,7 +116,7 @@ fn str_to_direction(to_parse: &str) -> RoomDirection {
"out" => RoomDirection::Out,
"up" => RoomDirection::Up,
"down" => RoomDirection::Down,
_ => None //~ NOTE match arm with an incompatible type
_ => None
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/test/run-pass/const-enum-vec-index.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,9 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[derive(Copy, Clone)]
enum E { V1(isize), V0 }

const C: &'static [E] = &[E::V0, E::V1(0xDEADBEE)];
static C0: E = C[0];
static C1: E = C[1];
Expand Down
15 changes: 15 additions & 0 deletions src/test/ui/lifetime-errors/ex1-return-one-existing-name-if-else.rs
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// Copyright 2016 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.

fn foo<'a>(x: &'a i32, y: &i32) -> &'a i32 {
if x > y { x } else { y }
}

fn main() { }
Loading

0 comments on commit 8918ceb

Please sign in to comment.