Skip to content

Region naming refactoring [6/N] #67476

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.

Sign up for GitHub

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
bors merged 8 commits into from
Jan 18, 2020
Merged

Region naming refactoring [6/N] #67476

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
109c30f
More separation of error reporting from region inference
mark-i-m Dec 29, 2019
c3e74f3
Move some methods to region_infer/mod.rs
mark-i-m Dec 29, 2019
736348a
Move a bunch of methods to inherent impl MirBorrowckCtxt
mark-i-m Dec 21, 2019
786db73
Move report_region_errors to region_errors.rs
mark-i-m Dec 29, 2019
66c5d5b
Privatize the fields of RegionInferenceContext
mark-i-m Dec 29, 2019
234b930
rename nonlexical_regioncx -> regioncx
mark-i-m Dec 29, 2019
a804868
Get rid of RegionErrorNamingContext
mark-i-m Dec 29, 2019
f05e40e
address review comments
mark-i-m Jan 1, 2020
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
Prev Previous commit
Next Next commit
Move report_region_errors to region_errors.rs
  • Loading branch information
@mark-i-m
mark-i-m committed Jan 13, 2020
commit 786db7399fa8214aa2412fbeb18033524bfa8b34
126 changes: 123 additions & 3 deletions src/librustc_mir/borrow_check/diagnostics/region_errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
//! Error reporting machinery for lifetime errors.

use rustc::infer::{error_reporting::nice_region_error::NiceRegionError, NLLRegionVariableOrigin};
use rustc::infer::{
error_reporting::nice_region_error::NiceRegionError, opaque_types, NLLRegionVariableOrigin,
};
use rustc::mir::ConstraintCategory;
use rustc::ty::{self, RegionVid, Ty};
use rustc_errors::{Applicability, DiagnosticBuilder};
Expand Down Expand Up @@ -109,8 +111,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
/// existentially bound, then we check its inferred value and try
/// to find a good name from that. Returns `None` if we can't find
/// one (e.g., this is just some random part of the CFG).
// TODO(mark-i-m): make this private when we move report_region_errors here...
crate fn to_error_region(&self, r: RegionVid) -> Option<ty::Region<'tcx>> {
pub(super) fn to_error_region(&self, r: RegionVid) -> Option<ty::Region<'tcx>> {
self.to_error_region_vid(r)
.and_then(|r| self.nonlexical_regioncx.definitions[r].external_name)
}
Expand Down Expand Up @@ -147,6 +148,125 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, 'tcx> {
false
}

/// Produces nice borrowck error diagnostics for all the errors collected in `nll_errors`.
pub(in crate::borrow_check) fn report_region_errors(&mut self, nll_errors: RegionErrors<'tcx>) {
// Iterate through all the errors, producing a diagnostic for each one. The diagnostics are
// buffered in the `MirBorrowckCtxt`.

// TODO(mark-i-m): Would be great to get rid of the naming context.
let mut region_naming = RegionErrorNamingCtx::new();
let mut outlives_suggestion = OutlivesSuggestionBuilder::default();
Copy link
Contributor

@matthewjasper matthewjasper Jan 1, 2020

Choose a reason for hiding this comment

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

Can you reset next_region_name to 1 here?

Copy link
Contributor Author

@mark-i-m mark-i-m Jan 1, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I thought about this, as it is the cause of the one test out change... but I'm not really sure we want to. It seems more correct to use the previous counter.

EDIT: Ignore this comment... I thought we were talking about resetting in explain_borrow to preserve the old behavior.

Copy link
Contributor

@matthewjasper matthewjasper Jan 4, 2020

Choose a reason for hiding this comment

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

I think I would prefer to have errors that we report be independent of each other.
The errors can potentially be reordered, and we will have to reset the counter and map between different item bodies anyway.

Copy link
Contributor Author

@mark-i-m mark-i-m Jan 13, 2020

Choose a reason for hiding this comment

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

Actually, looking at this again now. Won't do_mirborrowck only ever be called once on a MIR def id? In that case, the counter will reset for the next def we check in the constructor of MirBorrowckCtxt, right?

Copy link
Contributor Author

@mark-i-m mark-i-m Jan 15, 2020

Choose a reason for hiding this comment

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

@matthewjasper I just tried resetting the counter forcibly, and none of the test output changed. So I think my second comment is correct. The counter will be recreated starting at 1 each time we borrowck a new MIR def, and we do not need to reset manually.

Copy link
Contributor

@matthewjasper matthewjasper Jan 17, 2020

Choose a reason for hiding this comment

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

I guess I wasn't being clear with "we will have to reset the counter and map between different item bodies anyway." I really meant that we can't store the counter somewhere global and share it between borrow checks, so we also shouldn't persist it between errors.

I guess if this matters then someone will raise an issue.

Copy link
Contributor Author

@mark-i-m mark-i-m Jan 17, 2020

Choose a reason for hiding this comment

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

I really meant that we can't store the counter somewhere global and share it between borrow checks, so we also shouldn't persist it between errors.

Ah I see what you mean now. I argue that we don't want to. Having consistent numbering between error messages on the same MIR body makes sense IMHO. Additionally, outlives suggestions will sometimes output a combined suggestion across errors, in which case we do want consistent naming across errors.


for nll_error in nll_errors.into_iter() {
match nll_error {
RegionErrorKind::TypeTestError { type_test } => {
// Try to convert the lower-bound region into something named we can print for the user.
let lower_bound_region = self.to_error_region(type_test.lower_bound);

// Skip duplicate-ish errors.
let type_test_span = type_test.locations.span(&self.body);

if let Some(lower_bound_region) = lower_bound_region {
let region_scope_tree = &self.infcx.tcx.region_scope_tree(self.mir_def_id);
self.infcx
.construct_generic_bound_failure(
region_scope_tree,
type_test_span,
None,
type_test.generic_kind,
lower_bound_region,
)
.buffer(&mut self.errors_buffer);
} else {
// FIXME. We should handle this case better. It
// indicates that we have e.g., some region variable
// whose value is like `'a+'b` where `'a` and `'b` are
// distinct unrelated univesal regions that are not
// known to outlive one another. It'd be nice to have
// some examples where this arises to decide how best
// to report it; we could probably handle it by
// iterating over the universal regions and reporting
// an error that multiple bounds are required.
self.infcx
.tcx
.sess
.struct_span_err(
type_test_span,
&format!("`{}` does not live long enough", type_test.generic_kind),
)
.buffer(&mut self.errors_buffer);
}
}

RegionErrorKind::UnexpectedHiddenRegion {
opaque_type_def_id,
hidden_ty,
member_region,
} => {
let region_scope_tree = &self.infcx.tcx.region_scope_tree(self.mir_def_id);
opaque_types::unexpected_hidden_region_diagnostic(
self.infcx.tcx,
Some(region_scope_tree),
opaque_type_def_id,
hidden_ty,
member_region,
)
.buffer(&mut self.errors_buffer);
}

RegionErrorKind::BoundUniversalRegionError {
longer_fr,
fr_origin,
error_element,
} => {
let error_region =
self.nonlexical_regioncx.region_from_element(longer_fr, error_element);

// Find the code to blame for the fact that `longer_fr` outlives `error_fr`.
let (_, span) = self.nonlexical_regioncx.find_outlives_blame_span(
&self.body,
longer_fr,
fr_origin,
error_region,
);

// FIXME: improve this error message
self.infcx
.tcx
.sess
.struct_span_err(span, "higher-ranked subtype error")
.buffer(&mut self.errors_buffer);
}

RegionErrorKind::RegionError { fr_origin, longer_fr, shorter_fr, is_reported } => {
if is_reported {
self.report_error(
longer_fr,
fr_origin,
shorter_fr,
&mut outlives_suggestion,
&mut region_naming,
);
} else {
// We only report the first error, so as not to overwhelm the user. See
// `RegRegionErrorKind` docs.
//
// FIXME: currently we do nothing with these, but perhaps we can do better?
// FIXME: try collecting these constraints on the outlives suggestion
// builder. Does it make the suggestions any better?
debug!(
"Unreported region error: can't prove that {:?}: {:?}",
longer_fr, shorter_fr
);
}
}
}
}

// Emit one outlives suggestions for each MIR def we borrowck
outlives_suggestion.add_suggestion(self, &mut region_naming);
}

/// 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 Down
123 changes: 2 additions & 121 deletions src/librustc_mir/borrow_check/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
//! This query borrow-checks the MIR to (further) ensure it is not broken.

use rustc::infer::{opaque_types, InferCtxt};
use rustc::infer::InferCtxt;
use rustc::lint::builtin::MUTABLE_BORROW_RESERVATION_CONFLICT;
use rustc::lint::builtin::UNUSED_MUT;
use rustc::mir::{
Expand Down Expand Up @@ -39,9 +39,7 @@ use crate::dataflow::{do_dataflow, DebugFormatted};
use crate::dataflow::{MaybeInitializedPlaces, MaybeUninitializedPlaces};
use crate::transform::MirSource;

use self::diagnostics::{
AccessKind, OutlivesSuggestionBuilder, RegionErrorKind, RegionErrorNamingCtx, RegionErrors,
};
use self::diagnostics::AccessKind;
use self::flows::Flows;
use self::location::LocationTable;
use self::prefixes::PrefixSet;
Expand Down Expand Up @@ -1465,123 +1463,6 @@ impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
// initial reservation.
}
}

/// Produces nice borrowck error diagnostics for all the errors collected in `nll_errors`.
fn report_region_errors(&mut self, nll_errors: RegionErrors<'tcx>) {
// Iterate through all the errors, producing a diagnostic for each one. The diagnostics are
// buffered in the `MirBorrowckCtxt`.

// TODO(mark-i-m): Would be great to get rid of the naming context.
let mut region_naming = RegionErrorNamingCtx::new();
let mut outlives_suggestion = OutlivesSuggestionBuilder::default();

for nll_error in nll_errors.into_iter() {
match nll_error {
RegionErrorKind::TypeTestError { type_test } => {
// Try to convert the lower-bound region into something named we can print for the user.
let lower_bound_region = self.to_error_region(type_test.lower_bound);

// Skip duplicate-ish errors.
let type_test_span = type_test.locations.span(&self.body);

if let Some(lower_bound_region) = lower_bound_region {
let region_scope_tree = &self.infcx.tcx.region_scope_tree(self.mir_def_id);
self.infcx
.construct_generic_bound_failure(
region_scope_tree,
type_test_span,
None,
type_test.generic_kind,
lower_bound_region,
)
.buffer(&mut self.errors_buffer);
} else {
// FIXME. We should handle this case better. It indicates that we have
// e.g., some region variable whose value is like `'a+'b` where `'a` and
// `'b` are distinct unrelated univesal regions that are not known to
// outlive one another. It'd be nice to have some examples where this
// arises to decide how best to report it; we could probably handle it by
// iterating over the universal regions and reporting an error that
// multiple bounds are required.
self.infcx
.tcx
.sess
.struct_span_err(
type_test_span,
&format!("`{}` does not live long enough", type_test.generic_kind),
)
.buffer(&mut self.errors_buffer);
}
}

RegionErrorKind::UnexpectedHiddenRegion {
opaque_type_def_id,
hidden_ty,
member_region,
} => {
let region_scope_tree = &self.infcx.tcx.region_scope_tree(self.mir_def_id);
opaque_types::unexpected_hidden_region_diagnostic(
self.infcx.tcx,
Some(region_scope_tree),
opaque_type_def_id,
hidden_ty,
member_region,
)
.buffer(&mut self.errors_buffer);
}

RegionErrorKind::BoundUniversalRegionError {
longer_fr,
fr_origin,
error_element,
} => {
let error_region =
self.nonlexical_regioncx.region_from_element(longer_fr, error_element);

// Find the code to blame for the fact that `longer_fr` outlives `error_fr`.
let (_, span) = self.nonlexical_regioncx.find_outlives_blame_span(
&self.body,
longer_fr,
fr_origin,
error_region,
);

// FIXME: improve this error message
self.infcx
.tcx
.sess
.struct_span_err(span, "higher-ranked subtype error")
.buffer(&mut self.errors_buffer);
}

RegionErrorKind::RegionError { fr_origin, longer_fr, shorter_fr, is_reported } => {
if is_reported {
self.report_error(
longer_fr,
fr_origin,
shorter_fr,
&mut outlives_suggestion,
&mut region_naming,
);
} else {
// We only report the first error, so as not to overwhelm the user. See
// `RegRegionErrorKind` docs.
//
// FIXME: currently we do nothing with these, but perhaps we can do better?
// FIXME: try collecting these constraints on the outlives suggestion
// builder. Does it make the suggestions any better?
debug!(
"Unreported region error: can't prove that {:?}: {:?}",
longer_fr, shorter_fr
);
}
}
}
}

// Emit one outlives suggestions for each MIR def we borrowck
outlives_suggestion.add_suggestion(self, &mut region_naming);
}
}

impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {
Expand Down