Skip to content
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

[WIP] remove Location::All #50938

Closed
Show file tree
Hide file tree
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
9 changes: 9 additions & 0 deletions src/librustc/mir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,15 @@ impl<'tcx> Mir<'tcx> {
(&mut self.basic_blocks, &mut self.local_decls)
}

pub fn terminator_location(&self, block: BasicBlock) -> Location {
let num_statements = self.basic_blocks[block].statements.len();
Location { block, statement_index: num_statements }
}

pub fn visitable(&self, location: Location) -> &dyn MirVisitable<'tcx> {
self.basic_blocks[location.block].visitable(location.statement_index)
}

#[inline]
pub fn predecessors(&self) -> ReadGuard<IndexVec<BasicBlock, Vec<BasicBlock>>> {
self.cache.predecessors(self)
Expand Down
1 change: 0 additions & 1 deletion src/librustc_mir/borrow_check/nll/constraint_generation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -276,7 +276,6 @@ impl<'cx, 'cg, 'gcx, 'tcx> ConstraintGeneration<'cx, 'cg, 'gcx, 'tcx> {
span,
ref_region.to_region_vid(),
borrow_region.to_region_vid(),
location.successor_within_block(),
);

if let Some(all_facts) = self.all_facts {
Expand Down
66 changes: 62 additions & 4 deletions src/librustc_mir/borrow_check/nll/explain_borrow/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,8 @@ use borrow_check::nll::region_infer::{Cause, RegionInferenceContext};
use borrow_check::{Context, MirBorrowckCtxt};
use borrow_check::borrow_set::BorrowData;
use rustc::mir::visit::{MirVisitable, PlaceContext, Visitor};
use rustc::mir::{Local, Location, Mir};
use rustc::mir::{BasicBlock, Local, Location, Mir, Statement, Terminator, TerminatorKind};
use rustc::mir::RETURN_PLACE;
use rustc_data_structures::fx::FxHashSet;
use rustc_errors::DiagnosticBuilder;
use util::liveness::{self, DefUse, LivenessMode};
Expand All @@ -38,9 +39,9 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
Cause::LiveVar(local, location) => {
match find_regular_use(mir, regioncx, borrow, location, local) {
Some(p) => {
err.span_label(
mir.source_info(p).span,
format!("borrow later used here"),
mir.visitable(p).apply(
p,
&mut LaterUsedReporter { err, mir },
);
}

Expand Down Expand Up @@ -232,4 +233,61 @@ impl<'tcx> Visitor<'tcx> for DefUseVisitor {
}
}
}

fn visit_terminator(
&mut self,
block: BasicBlock,
terminator: &Terminator<'tcx>,
location: Location,
) {
if let TerminatorKind::Return = terminator.kind {
if self.local == RETURN_PLACE {
self.used = true;
}
}
self.super_terminator(block, terminator, location);
}
}

struct LaterUsedReporter<'cx, 'diag: 'cx, 'tcx: 'cx> {
err: &'cx mut DiagnosticBuilder<'diag>,
mir: &'cx Mir<'tcx>,
}

impl<'cx, 'diag, 'tcx> LaterUsedReporter<'cx, 'diag, 'tcx> {
fn base_report(&mut self, location: Location) {
self.err.span_label(
self.mir.source_info(location).span,
format!("borrow later used here"),
);
}
}

impl<'cx, 'diag, 'tcx> Visitor<'tcx> for LaterUsedReporter<'cx, 'diag, 'tcx> {
fn visit_statement(
&mut self,
_block: BasicBlock,
_statement: &Statement<'tcx>,
location: Location
) {
self.base_report(location);
}

fn visit_terminator(
&mut self,
_block: BasicBlock,
terminator: &Terminator<'tcx>,
location: Location,
) {
match terminator.kind {
TerminatorKind::Return => {
// If the last use is the `Return` terminator
// then for now we don't add any label, as it's
// not clear what to say that is helpful.
}
_ => {
self.base_report(location);
}
}
}
}
11 changes: 8 additions & 3 deletions src/librustc_mir/borrow_check/nll/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,9 +111,14 @@ pub(in borrow_check) fn compute_regions<'cx, 'gcx, 'tcx>(
};

if let Some(all_facts) = &mut all_facts {
all_facts
.universal_region
.extend(universal_regions.universal_regions());
// Declare that each universal region is live at every point.
for ur in universal_regions.universal_regions() {
all_facts.region_live_at.extend(
location_table
.all_points()
.map(|p| (ur, p))
);
}
}

// Create the region inference context, taking ownership of the region inference
Expand Down
4 changes: 1 addition & 3 deletions src/librustc_mir/borrow_check/nll/region_infer/dump_mir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -82,15 +82,13 @@ impl<'tcx> RegionInferenceContext<'tcx> {
let Constraint {
sup,
sub,
point,
span,
next: _,
} = constraint;
with_msg(&format!(
"{:?}: {:?} @ {:?} due to {:?}",
"{:?}: {:?} due to {:?}",
sup,
sub,
point,
span
))?;
}
Expand Down
3 changes: 0 additions & 3 deletions src/librustc_mir/borrow_check/nll/region_infer/graphviz.rs
Original file line number Diff line number Diff line change
Expand Up @@ -41,9 +41,6 @@ impl<'this, 'tcx> dot::Labeller<'this> for RegionInferenceContext<'tcx> {
fn node_label(&'this self, n: &RegionVid) -> dot::LabelText<'this> {
dot::LabelText::LabelStr(format!("{:?}", n).into_cow())
}
fn edge_label(&'this self, e: &Constraint) -> dot::LabelText<'this> {
dot::LabelText::LabelStr(format!("{:?}", e.point).into_cow())
}
}

impl<'this, 'tcx> dot::GraphWalk<'this> for RegionInferenceContext<'tcx> {
Expand Down
38 changes: 16 additions & 22 deletions src/librustc_mir/borrow_check/nll/region_infer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
// except according to those terms.

use super::universal_regions::UniversalRegions;
use borrow_check::nll::type_check::Locations;
use borrow_check::nll::region_infer::values::ToElementIndex;
use rustc::hir::def_id::DefId;
use rustc::infer::error_reporting::nice_region_error::NiceRegionError;
Expand Down Expand Up @@ -129,9 +130,6 @@ pub struct Constraint {
/// Region that must be outlived.
sub: RegionVid,

/// At this location.
point: Location,

/// Later on, we thread the constraints onto a linked list
/// grouped by their `sub` field. So if you had:
///
Expand Down Expand Up @@ -188,8 +186,8 @@ pub struct TypeTest<'tcx> {
/// The region `'x` that the type must outlive.
pub lower_bound: RegionVid,

/// The point where the outlives relation must hold.
pub point: Location,
/// Where does this type-test have to hold?
pub locations: Locations,

/// Where did this constraint arise?
pub span: Span,
Expand Down Expand Up @@ -383,15 +381,13 @@ impl<'tcx> RegionInferenceContext<'tcx> {
span: Span,
sup: RegionVid,
sub: RegionVid,
point: Location,
) {
debug!("add_outlives({:?}: {:?} @ {:?}", sup, sub, point);
debug!("add_outlives({:?}: {:?})", sup, sub);
assert!(self.inferred_values.is_none(), "values already inferred");
self.constraints.push(Constraint {
span,
sup,
sub,
point,
next: None,
});
}
Expand Down Expand Up @@ -546,7 +542,7 @@ impl<'tcx> RegionInferenceContext<'tcx> {
for type_test in &self.type_tests {
debug!("check_type_test: {:?}", type_test);

if self.eval_region_test(mir, type_test.point, type_test.lower_bound, &type_test.test) {
if self.eval_region_test(mir, type_test.lower_bound, &type_test.test) {
continue;
}

Expand Down Expand Up @@ -618,9 +614,9 @@ impl<'tcx> RegionInferenceContext<'tcx> {
let TypeTest {
generic_kind,
lower_bound,
point: _,
span,
test: _,
locations: _,
} = type_test;

let generic_ty = generic_kind.to_ty(tcx);
Expand Down Expand Up @@ -803,31 +799,30 @@ impl<'tcx> RegionInferenceContext<'tcx> {
fn eval_region_test(
&self,
mir: &Mir<'tcx>,
point: Location,
lower_bound: RegionVid,
test: &RegionTest,
) -> bool {
debug!(
"eval_region_test(point={:?}, lower_bound={:?}, test={:?})",
point, lower_bound, test
"eval_region_test(lower_bound={:?}, test={:?})",
lower_bound, test
);

match test {
RegionTest::IsOutlivedByAllRegionsIn(regions) => regions
.iter()
.all(|&r| self.eval_outlives(mir, r, lower_bound, point)),
.all(|&r| self.eval_outlives(mir, r, lower_bound)),

RegionTest::IsOutlivedByAnyRegionIn(regions) => regions
.iter()
.any(|&r| self.eval_outlives(mir, r, lower_bound, point)),
.any(|&r| self.eval_outlives(mir, r, lower_bound)),

RegionTest::Any(tests) => tests
.iter()
.any(|test| self.eval_region_test(mir, point, lower_bound, test)),
.any(|test| self.eval_region_test(mir, lower_bound, test)),

RegionTest::All(tests) => tests
.iter()
.all(|test| self.eval_region_test(mir, point, lower_bound, test)),
.all(|test| self.eval_region_test(mir, lower_bound, test)),
}
}

Expand All @@ -837,11 +832,10 @@ impl<'tcx> RegionInferenceContext<'tcx> {
_mir: &Mir<'tcx>,
sup_region: RegionVid,
sub_region: RegionVid,
point: Location,
) -> bool {
debug!(
"eval_outlives({:?}: {:?} @ {:?})",
sup_region, sub_region, point
"eval_outlives({:?}: {:?})",
sup_region, sub_region
);

let inferred_values = self.inferred_values
Expand Down Expand Up @@ -1143,8 +1137,8 @@ impl fmt::Debug for Constraint {
fn fmt(&self, formatter: &mut fmt::Formatter) -> fmt::Result {
write!(
formatter,
"({:?}: {:?} @ {:?}) due to {:?}",
self.sup, self.sub, self.point, self.span
"({:?}: {:?}) due to {:?}",
self.sup, self.sub, self.span
)
}
}
Expand Down
30 changes: 10 additions & 20 deletions src/librustc_mir/borrow_check/nll/subtype_constraint_generation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use borrow_check::nll::facts::AllFacts;
use rustc::infer::region_constraints::Constraint;
use rustc::infer::region_constraints::RegionConstraintData;
use rustc::infer::region_constraints::{Verify, VerifyBound};
use rustc::mir::{Location, Mir};
use rustc::mir::Mir;
use rustc::ty;
use std::iter;
use syntax::codemap::Span;
Expand Down Expand Up @@ -90,11 +90,7 @@ impl<'cx, 'tcx> SubtypeConstraintGenerator<'cx, 'tcx> {
givens,
} = data;

let span = self.mir
.source_info(locations.from_location().unwrap_or(Location::START))
.span;

let at_location = locations.at_location().unwrap_or(Location::START);
let span = locations.span(self.mir);

for constraint in constraints.keys() {
debug!("generate: constraint: {:?}", constraint);
Expand All @@ -112,23 +108,19 @@ impl<'cx, 'tcx> SubtypeConstraintGenerator<'cx, 'tcx> {
// reverse direction, because `regioncx` talks about
// "outlives" (`>=`) whereas the region constraints
// talk about `<=`.
self.regioncx.add_outlives(span, b_vid, a_vid, at_location);
self.regioncx.add_outlives(span, b_vid, a_vid);
Copy link
Member

Choose a reason for hiding this comment

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

Am I right in my understanding that one side-effect of no longer passing at_location into add_outlives is that our diagnostics stop being able to point to this location as the place where a particular constraint was introduced, which is what is leading to problems like the regression in diagnostic quality for e.g. a904c55#diff-40124ba66f06953794b406e3bb131b95L6 ?)

(And if that hypothesis is correct, is your thinking that we're going to have to solve that problem more generally anyway, perhaps as part of #51188, but that finding a solution to that problem should not block landing PR's like this one?)

Copy link
Member

Choose a reason for hiding this comment

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

hey @nikomatsakis do you have an answer to my Q above ?


// In the new analysis, all outlives relations etc
// "take effect" at the mid point of the statement
// that requires them, so ignore the `at_location`.
if let Some(all_facts) = all_facts {
if let Some(from_location) = locations.from_location() {
locations.each_point(self.mir, |location| {
// In the new analysis, all outlives relations etc
// "take effect" at the mid point of the
// statement(s) that require them.
all_facts.outlives.push((
b_vid,
a_vid,
self.location_table.mid_index(from_location),
self.location_table.mid_index(location),
));
} else {
for location in self.location_table.all_points() {
all_facts.outlives.push((b_vid, a_vid, location));
}
}
});
}
}

Expand All @@ -154,14 +146,12 @@ impl<'cx, 'tcx> SubtypeConstraintGenerator<'cx, 'tcx> {

let lower_bound = self.to_region_vid(verify.region);

let point = locations.at_location().unwrap_or(Location::START);

let test = self.verify_bound_to_region_test(&verify.bound);

TypeTest {
generic_kind,
lower_bound,
point,
locations: *locations,
span,
test,
}
Expand Down
Loading