Skip to content

Commit

Permalink
Auto merge of #54262 - matthewjasper:explain-in-typeck, r=nikomatsakis
Browse files Browse the repository at this point in the history 
[NLL] Record more infomation on free region constraints in typeck

Changes:

* Makes the span of the MIR return place point to the return type
* Don't try to use a path to a type alias as a path to the adt it aliases (fixes an ICE)
* Don't claim that `self` is declared outside of the function. [see this test](f2995d5#diff-0c9e6b1b204f42129b481df9ce459d44)
* Remove boring/interesting distinction and instead add a `ConstraintCategory` to the constraint.
* Add categories for implicit `Sized` and `Copy` requirements, for closure bounds, for user type annotations and `impl Trait`.
* Don't use the span of the first statement for Locations::All bounds (even if it happens to work on the tests we have)

Future work:

* Fine tuning the heuristic used to choose the place the report the error.
* Reporting multiple places (behind a flag)
* Better closure bounds reporting. This probably requires some discussion.

r? @nikomatsakis
  • Loading branch information
@bors
bors committed Sep 23, 2018
2 parents c6e3d7f + bd0895d commit 576b640
Show file tree
Hide file tree
Showing 70 changed files with 746 additions and 540 deletions.
2 changes: 1 addition & 1 deletion src/librustc/infer/opaque_types/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ impl<'a, 'gcx, 'tcx> InferCtxt<'a, 'gcx, 'tcx> {
}
}

fn constrain_opaque_type<FRR: FreeRegionRelations<'tcx>>(
pub fn constrain_opaque_type<FRR: FreeRegionRelations<'tcx>>(
&self,
def_id: DefId,
opaque_defn: &OpaqueTypeDecl<'tcx>,
Expand Down
7 changes: 5 additions & 2 deletions src/librustc_mir/borrow_check/nll/constraints/graph.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,12 @@
// except according to those terms.

use borrow_check::nll::type_check::Locations;
use borrow_check::nll::constraints::{ConstraintIndex, ConstraintSet, OutlivesConstraint};
use borrow_check::nll::constraints::{ConstraintCategory, ConstraintIndex};
use borrow_check::nll::constraints::{ConstraintSet, OutlivesConstraint};
use rustc::ty::RegionVid;
use rustc_data_structures::graph;
use rustc_data_structures::indexed_vec::IndexVec;
use syntax_pos::DUMMY_SP;

/// The construct graph organizes the constraints by their end-points.
/// It can be used to view a `R1: R2` constraint as either an edge `R1
Expand Down Expand Up @@ -174,7 +176,8 @@ impl<'s, D: ConstraintGraphDirecton> Iterator for Edges<'s, D> {
Some(OutlivesConstraint {
sup: self.static_region,
sub: next_static_idx.into(),
locations: Locations::All,
locations: Locations::All(DUMMY_SP),
category: ConstraintCategory::Internal,
})
} else {
None
Expand Down
39 changes: 39 additions & 0 deletions src/librustc_mir/borrow_check/nll/constraints/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,42 @@ crate struct ConstraintSet {
constraints: IndexVec<ConstraintIndex, OutlivesConstraint>,
}

/// Constraints can be categorized to determine whether and why they are
/// interesting. Order of variants indicates sort order of the category,
/// thereby influencing diagnostic output.
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)]
pub enum ConstraintCategory {
Return,
TypeAnnotation,
Cast,
CallArgument,

/// A constraint that came from checking the body of a closure.
///
/// Ideally we would give an explanation that points to the relevant part
/// of the closure's body.
ClosureBounds,
CopyBound,
SizedBound,
Assignment,
OpaqueType,

/// A "boring" constraint (caused by the given location) is one that
/// the user probably doesn't want to see described in diagnostics,
/// because it is kind of an artifact of the type system setup.
/// Example: `x = Foo { field: y }` technically creates
/// intermediate regions representing the "type of `Foo { field: y
/// }`", and data flows from `y` into those variables, but they
/// are not very interesting. The assignment into `x` on the other
/// hand might be.
Boring,
// Boring and applicable everywhere.
BoringNoLocation,

/// A constraint that doesn't correspond to anything the user sees.
Internal,
}

impl ConstraintSet {
crate fn push(&mut self, constraint: OutlivesConstraint) {
debug!(
Expand Down Expand Up @@ -87,6 +123,9 @@ pub struct OutlivesConstraint {

/// Where did this constraint arise?
pub locations: Locations,

/// What caused this constraint?
pub category: ConstraintCategory,
}

impl fmt::Debug for OutlivesConstraint {
Expand Down
4 changes: 3 additions & 1 deletion src/librustc_mir/borrow_check/nll/region_infer/dump_mir.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,13 @@ impl<'tcx> RegionInferenceContext<'tcx> {
sup,
sub,
locations,
category,
} = constraint;
with_msg(&format!(
"{:?}: {:?} due to {:?}",
"{:?}: {:?} due to {:?} at {:?}",
sup,
sub,
category,
locations,
))?;
}
Expand Down
152 changes: 22 additions & 130 deletions src/librustc_mir/borrow_check/nll/region_infer/error_reporting/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,13 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use borrow_check::nll::constraints::OutlivesConstraint;
use borrow_check::nll::constraints::{OutlivesConstraint, ConstraintCategory};
use borrow_check::nll::region_infer::RegionInferenceContext;
use borrow_check::nll::type_check::Locations;
use rustc::hir::def_id::DefId;
use rustc::infer::error_reporting::nice_region_error::NiceRegionError;
use rustc::infer::InferCtxt;
use rustc::mir::{self, Location, Mir, Place, Rvalue, StatementKind, TerminatorKind};
use rustc::ty::{self, TyCtxt, RegionVid};
use rustc::mir::{Location, Mir};
use rustc::ty::{self, RegionVid};
use rustc_data_structures::indexed_vec::IndexVec;
use rustc_errors::{Diagnostic, DiagnosticBuilder};
use std::collections::VecDeque;
Expand All @@ -29,19 +28,6 @@ mod var_name;

use self::region_name::RegionName;

/// Constraints that are considered interesting can be categorized to
/// determine why they are interesting. Order of variants indicates
/// sort order of the category, thereby influencing diagnostic output.
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord)]
enum ConstraintCategory {
Cast,
Assignment,
Return,
CallArgument,
Other,
Boring,
}

impl fmt::Display for ConstraintCategory {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
// Must end with a space. Allows for empty names to be provided.
Expand All @@ -50,7 +36,14 @@ impl fmt::Display for ConstraintCategory {
ConstraintCategory::Return => write!(f, "returning this value "),
ConstraintCategory::Cast => write!(f, "cast "),
ConstraintCategory::CallArgument => write!(f, "argument "),
_ => write!(f, ""),
ConstraintCategory::TypeAnnotation => write!(f, "type annotation "),
ConstraintCategory::ClosureBounds => write!(f, "closure body "),
ConstraintCategory::SizedBound => write!(f, "proving this value is `Sized` "),
ConstraintCategory::CopyBound => write!(f, "copying this value "),
ConstraintCategory::OpaqueType => write!(f, "opaque type "),
ConstraintCategory::Boring
| ConstraintCategory::BoringNoLocation
| ConstraintCategory::Internal => write!(f, ""),
}
}
}
Expand All @@ -72,7 +65,6 @@ impl<'tcx> RegionInferenceContext<'tcx> {
fn best_blame_constraint(
&self,
mir: &Mir<'tcx>,
tcx: TyCtxt<'_, '_, 'tcx>,
from_region: RegionVid,
target_test: impl Fn(RegionVid) -> bool,
) -> (ConstraintCategory, Span, RegionVid) {
Expand All @@ -97,7 +89,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
// Classify each of the constraints along the path.
let mut categorized_path: Vec<(ConstraintCategory, Span)> = path
.iter()
.map(|&index| self.classify_constraint(index, mir, tcx))
.map(|constraint| (constraint.category, constraint.locations.span(mir)))
.collect();
debug!(
"best_blame_constraint: categorized_path={:#?}",
Expand Down Expand Up @@ -130,12 +122,10 @@ impl<'tcx> RegionInferenceContext<'tcx> {
let constraint_sup_scc = self.constraint_sccs.scc(constraint.sup);

match categorized_path[i].0 {
ConstraintCategory::Boring => false,
ConstraintCategory::Other => {
// other isn't interesting when the two lifetimes
// are unified.
constraint_sup_scc != self.constraint_sccs.scc(constraint.sub)
}
ConstraintCategory::OpaqueType
| ConstraintCategory::Boring
| ConstraintCategory::BoringNoLocation
| ConstraintCategory::Internal => false,
_ => constraint_sup_scc != target_scc,
}
});
Expand Down Expand Up @@ -221,106 +211,6 @@ impl<'tcx> RegionInferenceContext<'tcx> {
None
}

/// This function will return true if a constraint is interesting and false if a constraint
/// is not. It is useful in filtering constraint paths to only interesting points.
fn constraint_is_interesting(&self, constraint: OutlivesConstraint) -> bool {
debug!(
"constraint_is_interesting: locations={:?} constraint={:?}",
constraint.locations, constraint
);

match constraint.locations {
Locations::Interesting(_) | Locations::All => true,
_ => false,
}
}

/// This function classifies a constraint from a location.
fn classify_constraint(
&self,
constraint: OutlivesConstraint,
mir: &Mir<'tcx>,
tcx: TyCtxt<'_, '_, 'tcx>,
) -> (ConstraintCategory, Span) {
debug!("classify_constraint: constraint={:?}", constraint);
let span = constraint.locations.span(mir);
let location = constraint
.locations
.from_location()
.unwrap_or(Location::START);

if !self.constraint_is_interesting(constraint) {
return (ConstraintCategory::Boring, span);
}

let data = &mir[location.block];
debug!(
"classify_constraint: location={:?} data={:?}",
location, data
);
let category = if location.statement_index == data.statements.len() {
if let Some(ref terminator) = data.terminator {
debug!("classify_constraint: terminator.kind={:?}", terminator.kind);
match terminator.kind {
TerminatorKind::DropAndReplace { .. } => ConstraintCategory::Assignment,
// Classify calls differently depending on whether or not
// the sub region appears in the destination type (so the
// sup region is in the return type). If the return type
// contains the sub-region, then this is either an
// assignment or a return, depending on whether we are
// writing to the RETURN_PLACE or not.
//
// The idea here is that the region is being propagated
// from an input into the output place, so it's a kind of
// assignment. Otherwise, if the sub-region only appears in
// the argument types, then use the CallArgument
// classification.
TerminatorKind::Call { destination: Some((ref place, _)), .. } => {
if tcx.any_free_region_meets(
&place.ty(mir, tcx).to_ty(tcx),
|region| self.to_region_vid(region) == constraint.sub,
) {
match place {
Place::Local(mir::RETURN_PLACE) => ConstraintCategory::Return,
_ => ConstraintCategory::Assignment,
}
} else {
ConstraintCategory::CallArgument
}
}
TerminatorKind::Call { destination: None, .. } => {
ConstraintCategory::CallArgument
}
_ => ConstraintCategory::Other,
}
} else {
ConstraintCategory::Other
}
} else {
let statement = &data.statements[location.statement_index];
debug!("classify_constraint: statement.kind={:?}", statement.kind);
match statement.kind {
StatementKind::Assign(ref place, ref rvalue) => {
debug!("classify_constraint: place={:?} rvalue={:?}", place, rvalue);
if *place == Place::Local(mir::RETURN_PLACE) {
ConstraintCategory::Return
} else {
match rvalue {
Rvalue::Cast(..) => ConstraintCategory::Cast,
Rvalue::Use(..) | Rvalue::Aggregate(..) => {
ConstraintCategory::Assignment
}
_ => ConstraintCategory::Other,
}
}
}
_ => ConstraintCategory::Other,
}
};

(category, span)
}

/// Report an error because the universal region `fr` was required to outlive
/// `outlived_fr` but it is not known to do so. For example:
///
Expand All @@ -342,7 +232,6 @@ impl<'tcx> RegionInferenceContext<'tcx> {

let (category, span, _) = self.best_blame_constraint(
mir,
infcx.tcx,
fr,
|r| r == outlived_fr
);
Expand Down Expand Up @@ -392,7 +281,11 @@ impl<'tcx> RegionInferenceContext<'tcx> {

let escapes_from = if infcx.tcx.is_closure(mir_def_id) { "closure" } else { "function" };

if fr_name_and_span.is_none() && outlived_fr_name_and_span.is_none() {
// Revert to the normal error in these cases.
// Assignments aren't "escapes" in function items.
if (fr_name_and_span.is_none() && outlived_fr_name_and_span.is_none())
|| (category == ConstraintCategory::Assignment && escapes_from == "function")
{
return self.report_general_error(mir, infcx, mir_def_id,
fr, true, outlived_fr, false,
category, span, errors_buffer);
Expand Down Expand Up @@ -572,11 +465,10 @@ impl<'tcx> RegionInferenceContext<'tcx> {
crate fn find_outlives_blame_span(
&self,
mir: &Mir<'tcx>,
tcx: TyCtxt<'_, '_, 'tcx>,
fr1: RegionVid,
fr2: RegionVid,
) -> Span {
let (_, span, _) = self.best_blame_constraint(mir, tcx, fr1, |r| r == fr2);
let (_, span, _) = self.best_blame_constraint(mir, fr1, |r| r == fr2);
span
}
}
27 changes: 17 additions & 10 deletions src/librustc_mir/borrow_check/nll/region_infer/error_reporting/region_name.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -452,16 +452,23 @@ impl<'tcx> RegionInferenceContext<'tcx> {
ty::Adt(_adt_def, substs),
hir::TyKind::Path(hir::QPath::Resolved(None, path)),
) => {
if let Some(last_segment) = path.segments.last() {
if let Some(name) = self.match_adt_and_segment(
substs,
needle_fr,
last_segment,
counter,
diag,
search_stack,
) {
return Some(name);
match path.def {
// Type parameters of the type alias have no reason to
// be the same as those of the ADT.
// FIXME: We should be able to do something similar to
// match_adt_and_segment in this case.
hir::def::Def::TyAlias(_) => (),
_ => if let Some(last_segment) = path.segments.last() {
if let Some(name) = self.match_adt_and_segment(
substs,
needle_fr,
last_segment,
counter,
diag,
search_stack,
) {
return Some(name);
}
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions src/librustc_mir/borrow_check/nll/region_infer/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1062,7 +1062,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
longer_fr, shorter_fr,
);

let blame_span = self.find_outlives_blame_span(mir, infcx.tcx, longer_fr, shorter_fr);
let blame_span = self.find_outlives_blame_span(mir, longer_fr, shorter_fr);

if let Some(propagated_outlives_requirements) = propagated_outlives_requirements {
// Shrink `fr` until we find a non-local region (if we do).
Expand Down Expand Up @@ -1147,7 +1147,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
};

// Find the code to blame for the fact that `longer_fr` outlives `error_fr`.
let span = self.find_outlives_blame_span(mir, infcx.tcx, longer_fr, error_region);
let span = self.find_outlives_blame_span(mir, longer_fr, error_region);

// Obviously, this error message is far from satisfactory.
// At present, though, it only appears in unit tests --
Expand Down
Loading

0 comments on commit 576b640

Please sign in to comment.