Skip to content

[NLL] Improve closure region bound errors #54798

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 2 commits into from
Oct 9, 2018
Merged
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
18 changes: 17 additions & 1 deletion src/librustc/ich/impls_mir.rs
Original file line number Diff line number Diff line change
@@ -550,7 +550,23 @@ impl_stable_hash_for!(struct mir::ClosureRegionRequirements<'tcx> {
impl_stable_hash_for!(struct mir::ClosureOutlivesRequirement<'tcx> {
subject,
outlived_free_region,
blame_span
blame_span,
category
});

impl_stable_hash_for!(enum mir::ConstraintCategory {
Return,
TypeAnnotation,
Cast,
ClosureBounds,
CallArgument,
CopyBound,
SizedBound,
Assignment,
OpaqueType,
Boring,
BoringNoLocation,
Internal,
});

impl<'a, 'gcx> HashStable<StableHashingContext<'a>> for mir::ClosureOutlivesSubject<'gcx> {
44 changes: 42 additions & 2 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
@@ -2639,11 +2639,51 @@ pub struct ClosureOutlivesRequirement<'tcx> {
// This region or type ...
pub subject: ClosureOutlivesSubject<'tcx>,

// .. must outlive this one.
// ... must outlive this one.
pub outlived_free_region: ty::RegionVid,

// If not, report an error here.
// If not, report an error here ...
pub blame_span: Span,

// ... due to this reason.
pub category: ConstraintCategory,
}

/// Outlives constraints can be categorized to determine whether and why they
/// are interesting (for error reporting). Order of variants indicates sort
/// order of the category, thereby influencing diagnostic output.
///
/// See also [rustc_mir::borrow_check::nll::constraints]
#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, RustcEncodable, RustcDecodable)]
pub enum ConstraintCategory {
Return,
TypeAnnotation,
Cast,

/// A constraint that came from checking the body of a closure.
///
/// We try to get the category that the closure used when reporting this.
ClosureBounds,
CallArgument,
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,
}

/// The subject of a ClosureOutlivesRequirement -- that is, the thing
3 changes: 2 additions & 1 deletion src/librustc_mir/borrow_check/nll/constraints/graph.rs
Original file line number Diff line number Diff line change
@@ -9,8 +9,9 @@
// except according to those terms.

use borrow_check::nll::type_check::Locations;
use borrow_check::nll::constraints::{ConstraintCategory, ConstraintIndex};
use borrow_check::nll::constraints::ConstraintIndex;
use borrow_check::nll::constraints::{ConstraintSet, OutlivesConstraint};
use rustc::mir::ConstraintCategory;
use rustc::ty::RegionVid;
use rustc_data_structures::graph;
use rustc_data_structures::indexed_vec::IndexVec;
37 changes: 1 addition & 36 deletions src/librustc_mir/borrow_check/nll/constraints/mod.rs
Original file line number Diff line number Diff line change
@@ -8,6 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use rustc::mir::ConstraintCategory;
use rustc::ty::RegionVid;
use rustc_data_structures::graph::scc::Sccs;
use rustc_data_structures::indexed_vec::{Idx, IndexVec};
@@ -23,42 +24,6 @@ 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!(
2 changes: 2 additions & 0 deletions src/librustc_mir/borrow_check/nll/mod.rs
Original file line number Diff line number Diff line change
@@ -138,6 +138,7 @@ pub(in borrow_check) fn compute_regions<'cx, 'gcx, 'tcx>(
let MirTypeckRegionConstraints {
mut liveness_constraints,
outlives_constraints,
closure_bounds_mapping,
type_tests,
} = constraints;

@@ -157,6 +158,7 @@ pub(in borrow_check) fn compute_regions<'cx, 'gcx, 'tcx>(
universal_region_relations,
mir,
outlives_constraints,
closure_bounds_mapping,
type_tests,
liveness_constraints,
elements,
Original file line number Diff line number Diff line change
@@ -8,17 +8,17 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use borrow_check::nll::constraints::{OutlivesConstraint, ConstraintCategory};
use borrow_check::nll::constraints::{OutlivesConstraint};
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::{Location, Mir};
use rustc::mir::{ConstraintCategory, Location, Mir};
use rustc::ty::{self, RegionVid};
use rustc_data_structures::indexed_vec::IndexVec;
use rustc_errors::{Diagnostic, DiagnosticBuilder};
use std::collections::VecDeque;
use std::fmt;
use syntax::symbol::keywords;
use syntax_pos::Span;
use syntax::errors::Applicability;
@@ -28,22 +28,26 @@ mod var_name;

use self::region_name::RegionName;

impl fmt::Display for ConstraintCategory {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
trait ConstraintDescription {
fn description(&self) -> &'static str;
}

impl ConstraintDescription for ConstraintCategory {
fn description(&self) -> &'static str {
// Must end with a space. Allows for empty names to be provided.
match self {
ConstraintCategory::Assignment => write!(f, "assignment "),
ConstraintCategory::Return => write!(f, "returning this value "),
ConstraintCategory::Cast => write!(f, "cast "),
ConstraintCategory::CallArgument => write!(f, "argument "),
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::Assignment => "assignment ",
ConstraintCategory::Return => "returning this value ",
ConstraintCategory::Cast => "cast ",
ConstraintCategory::CallArgument => "argument ",
ConstraintCategory::TypeAnnotation => "type annotation ",
ConstraintCategory::ClosureBounds => "closure body ",
ConstraintCategory::SizedBound => "proving this value is `Sized` ",
ConstraintCategory::CopyBound => "copying this value ",
ConstraintCategory::OpaqueType => "opaque type ",
ConstraintCategory::Boring
| ConstraintCategory::BoringNoLocation
| ConstraintCategory::Internal => write!(f, ""),
| ConstraintCategory::Internal => "",
}
}
}
@@ -89,7 +93,13 @@ impl<'tcx> RegionInferenceContext<'tcx> {
// Classify each of the constraints along the path.
let mut categorized_path: Vec<(ConstraintCategory, Span)> = path
.iter()
.map(|constraint| (constraint.category, constraint.locations.span(mir)))
.map(|constraint| {
if constraint.category == ConstraintCategory::ClosureBounds {
self.retrieve_closure_constraint_info(mir, &constraint)
} else {
(constraint.category, constraint.locations.span(mir))
}
})
.collect();
debug!(
"best_blame_constraint: categorized_path={:#?}",
@@ -358,7 +368,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
_ => {
diag.span_label(span, format!(
"{}requires that `{}` must outlive `{}`",
category, fr_name, outlived_fr_name,
category.description(), fr_name, outlived_fr_name,
));
},
}
@@ -470,8 +480,24 @@ impl<'tcx> RegionInferenceContext<'tcx> {
mir: &Mir<'tcx>,
fr1: RegionVid,
fr2: RegionVid,
) -> Span {
let (_, span, _) = self.best_blame_constraint(mir, fr1, |r| r == fr2);
span
) -> (ConstraintCategory, Span) {
let (category, span, _) = self.best_blame_constraint(mir, fr1, |r| r == fr2);
(category, span)
}

fn retrieve_closure_constraint_info(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why can't we assign the correct category to begin with?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it doesn't really save us very much, since the Constraint doesn't have anywhere to store the Span (I guess one could be added) and there is a PR that I'll be making soon that needs to know when bounds come from closure constraints.

&self,
mir: &Mir<'tcx>,
constraint: &OutlivesConstraint
) -> (ConstraintCategory, Span) {
let loc = match constraint.locations {
Locations::All(span) => return (constraint.category, span),
Locations::Single(loc) => loc,
};

let opt_span_category = self
.closure_bounds_mapping[&loc]
.get(&(constraint.sup, constraint.sub));
*opt_span_category.unwrap_or(&(constraint.category, mir.source_info(loc).span))
}
}
29 changes: 22 additions & 7 deletions src/librustc_mir/borrow_check/nll/region_infer/mod.rs
Original file line number Diff line number Diff line change
@@ -19,15 +19,17 @@ use rustc::infer::canonical::QueryRegionConstraint;
use rustc::infer::region_constraints::{GenericKind, VarInfos, VerifyBound};
use rustc::infer::{InferCtxt, NLLRegionVariableOrigin, RegionVariableOrigin};
use rustc::mir::{
ClosureOutlivesRequirement, ClosureOutlivesSubject, ClosureRegionRequirements, Local, Location,
Mir,
ClosureOutlivesRequirement, ClosureOutlivesSubject, ClosureRegionRequirements,
ConstraintCategory, Local, Location, Mir,
};
use rustc::ty::{self, RegionVid, Ty, TyCtxt, TypeFoldable};
use rustc::util::common;
use rustc_data_structures::bit_set::BitSet;
use rustc_data_structures::fx::FxHashMap;
use rustc_data_structures::graph::scc::Sccs;
use rustc_data_structures::indexed_vec::IndexVec;
use rustc_errors::{Diagnostic, DiagnosticBuilder};
use syntax_pos::Span;

use std::rc::Rc;

@@ -60,10 +62,16 @@ pub struct RegionInferenceContext<'tcx> {
/// the SCC (see `constraint_sccs`) and for error reporting.
constraint_graph: Rc<NormalConstraintGraph>,

/// The SCC computed from `constraints` and the constraint graph. Used to compute the values
/// of each region.
/// The SCC computed from `constraints` and the constraint graph. Used to
/// compute the values of each region.
constraint_sccs: Rc<Sccs<RegionVid, ConstraintSccIndex>>,

/// Map closure bounds to a `Span` that should be used for error reporting.
closure_bounds_mapping: FxHashMap<
Location,
FxHashMap<(RegionVid, RegionVid), (ConstraintCategory, Span)>,
>,

/// Contains the minimum universe of any variable within the same
/// SCC. We will ensure that no SCC contains values that are not
/// visible from this index.
@@ -187,6 +195,10 @@ impl<'tcx> RegionInferenceContext<'tcx> {
universal_region_relations: Rc<UniversalRegionRelations<'tcx>>,
_mir: &Mir<'tcx>,
outlives_constraints: ConstraintSet,
closure_bounds_mapping: FxHashMap<
Location,
FxHashMap<(RegionVid, RegionVid), (ConstraintCategory, Span)>,
>,
type_tests: Vec<TypeTest<'tcx>>,
liveness_constraints: LivenessValues<RegionVid>,
elements: &Rc<RegionValueElements>,
@@ -220,6 +232,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
constraints,
constraint_graph,
constraint_sccs,
closure_bounds_mapping,
scc_universes,
scc_representatives,
scc_values,
@@ -727,6 +740,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
subject,
outlived_free_region: non_local_ub,
blame_span: locations.span(mir),
category: ConstraintCategory::Boring,
};
debug!("try_promote_type_test: pushing {:#?}", requirement);
propagated_outlives_requirements.push(requirement);
@@ -1125,7 +1139,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
longer_fr, shorter_fr,
);

let blame_span = self.find_outlives_blame_span(mir, longer_fr, shorter_fr);
let blame_span_category = 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).
@@ -1150,7 +1164,8 @@ impl<'tcx> RegionInferenceContext<'tcx> {
propagated_outlives_requirements.push(ClosureOutlivesRequirement {
subject: ClosureOutlivesSubject::Region(fr_minus),
outlived_free_region: shorter_fr_plus,
blame_span: blame_span,
blame_span: blame_span_category.1,
category: blame_span_category.0,
});
return;
}
@@ -1213,7 +1228,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, 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 --
Original file line number Diff line number Diff line change
@@ -8,7 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use borrow_check::nll::constraints::{ConstraintCategory, ConstraintSet, OutlivesConstraint};
use borrow_check::nll::constraints::{ConstraintSet, OutlivesConstraint};
use borrow_check::nll::region_infer::TypeTest;
use borrow_check::nll::type_check::Locations;
use borrow_check::nll::universal_regions::UniversalRegions;
@@ -17,6 +17,7 @@ use rustc::infer::outlives::env::RegionBoundPairs;
use rustc::infer::outlives::obligations::{TypeOutlives, TypeOutlivesDelegate};
use rustc::infer::region_constraints::{GenericKind, VerifyBound};
use rustc::infer::{self, SubregionOrigin};
use rustc::mir::ConstraintCategory;
use rustc::ty::subst::UnpackedKind;
use rustc::ty::{self, TyCtxt};
use syntax_pos::DUMMY_SP;
Original file line number Diff line number Diff line change
@@ -12,11 +12,11 @@ use borrow_check::nll::type_check::constraint_conversion;
use borrow_check::nll::type_check::{Locations, MirTypeckRegionConstraints};
use borrow_check::nll::universal_regions::UniversalRegions;
use borrow_check::nll::ToRegionVid;
use borrow_check::nll::constraints::ConstraintCategory;
use rustc::infer::canonical::QueryRegionConstraint;
use rustc::infer::outlives::free_region_map::FreeRegionRelations;
use rustc::infer::region_constraints::GenericKind;
use rustc::infer::InferCtxt;
use rustc::mir::ConstraintCategory;
use rustc::traits::query::outlives_bounds::{self, OutlivesBound};
use rustc::traits::query::type_op::{self, TypeOp};
use rustc::ty::{self, RegionVid, Ty};
Loading