Auto merge of #67241 - mark-i-m:simplify-borrow_check-3, r=matthewjasper

Refactorings to borrowck region diagnostic reporting

This PR is a followup to #66886 and #67404

EDIT: updated

In this PR:  Clean up how errors are collected from NLL: introduce `borrow_check::diagnostics::RegionErrors` to collect errors. This is later passed to `MirBorrowckCtx::report_region_errors` after the `MirBorrowckCtx` is created. This will allow us to refactor away some of the extra context structs floating around (but we don't do it in this PR).  `borrow_check::region_infer` is now mostly free of diagnostic generating code. The signatures of most of the functions in `region_infer` lose somewhere between 4 and 7 arguments :)

Left for future (feedback appreciated):

- Merge `ErrorRegionCtx` with `MirBorrowckCtx`, as suggested by @matthewjasper in #66886 (comment)
- Maybe move the contents of `borrow_check::nll` into `borrow_check` and remove the `nll` submodule altogether.
- Find a way to make `borrow_check::diagnostics::region_errors` less of a strange appendage to `RegionInferenceContext`. I'm not really sure how to do this yet. Ideas welcome.

src/librustc_mir/borrow_check/diagnostics/mod.rs

@@ -32,7 +32,7 @@ mod region_errors;
 
 crate use mutability_errors::AccessKind;
 crate use outlives_suggestion::OutlivesSuggestionBuilder;
-crate use region_errors::{ErrorConstraintInfo, ErrorReportingCtx};
+crate use region_errors::{ErrorConstraintInfo, ErrorReportingCtx, RegionErrorKind, RegionErrors};
 crate use region_name::{RegionErrorNamingCtx, RegionName, RegionNameSource};
 
 pub(super) struct IncludingDowncast(pub(super) bool);

src/librustc_mir/borrow_check/diagnostics/outlives_suggestion.rs

@@ -243,7 +243,9 @@ impl OutlivesSuggestionBuilder<'a> {
 
         // If there is only one constraint to suggest, then we already suggested it in the
         // intermediate suggestion above.
-        if self.constraints_to_add.len() == 1 {
+        if self.constraints_to_add.len() == 1
+            && self.constraints_to_add.values().next().unwrap().len() == 1
+        {
             debug!("Only 1 suggestion. Skipping.");
             return;
         }

src/librustc_mir/borrow_check/diagnostics/region_errors.rs

@@ -2,10 +2,11 @@
 
 use rustc::hir::def_id::DefId;
 use rustc::infer::{
-    error_reporting::nice_region_error::NiceRegionError, InferCtxt, NLLRegionVariableOrigin,
+    error_reporting::nice_region_error::NiceRegionError, region_constraints::GenericKind,
+    InferCtxt, NLLRegionVariableOrigin,
 };
 use rustc::mir::{Body, ConstraintCategory, Local, Location};
-use rustc::ty::{self, RegionVid};
+use rustc::ty::{self, RegionVid, Ty};
 use rustc_errors::DiagnosticBuilder;
 use rustc_index::vec::IndexVec;
 use std::collections::VecDeque;
@@ -47,13 +48,74 @@ impl ConstraintDescription for ConstraintCategory {
     }
 }
 
-#[derive(Copy, Clone, PartialEq, Eq)]
+#[derive(Copy, Clone, PartialEq, Eq, Debug)]
 enum Trace {
     StartRegion,
     FromOutlivesConstraint(OutlivesConstraint),
     NotVisited,
 }
 
+/// A collection of errors encountered during region inference. This is needed to efficiently
+/// report errors after borrow checking.
+///
+/// Usually we expect this to either be empty or contain a small number of items, so we can avoid
+/// allocation most of the time.
+crate type RegionErrors<'tcx> = Vec<RegionErrorKind<'tcx>>;
+
+#[derive(Clone, Debug)]
+crate enum RegionErrorKind<'tcx> {
+    /// An error for a type test: `T: 'a` does not live long enough.
+    TypeTestDoesNotLiveLongEnough {
+        /// The span of the type test.
+        span: Span,
+        /// The generic type of the type test.
+        generic: GenericKind<'tcx>,
+    },
+
+    /// A generic bound failure for a type test.
+    TypeTestGenericBoundError {
+        /// The span of the type test.
+        span: Span,
+        /// The generic type of the type test.
+        generic: GenericKind<'tcx>,
+        /// The lower bound region.
+        lower_bound_region: ty::Region<'tcx>,
+    },
+
+    /// An unexpected hidden region for an opaque type.
+    UnexpectedHiddenRegion {
+        /// The def id of the opaque type.
+        opaque_type_def_id: DefId,
+        /// The hidden type.
+        hidden_ty: Ty<'tcx>,
+        /// The unexpected region.
+        member_region: ty::Region<'tcx>,
+    },
+
+    /// Higher-ranked subtyping error.
+    BoundUniversalRegionError {
+        /// The placeholder free region.
+        longer_fr: RegionVid,
+        /// The region that erroneously must be outlived by `longer_fr`.
+        error_region: RegionVid,
+        /// The origin of the placeholder region.
+        fr_origin: NLLRegionVariableOrigin,
+    },
+
+    /// Any other lifetime error.
+    RegionError {
+        /// The origin of the region.
+        fr_origin: NLLRegionVariableOrigin,
+        /// The region that should outlive `shorter_fr`.
+        longer_fr: RegionVid,
+        /// The region that should be shorter, but we can't prove it.
+        shorter_fr: RegionVid,
+        /// Indicates whether this is a reported error. We currently only report the first error
+        /// encountered and leave the rest unreported so as not to overwhelm the user.
+        is_reported: bool,
+    },
+}
+
 /// Various pieces of state used when reporting borrow checker errors.
 pub struct ErrorReportingCtx<'a, 'b, 'tcx> {
     /// The region inference context used for borrow chekcing this MIR body.

src/librustc_mir/borrow_check/mod.rs

@@ -1,9 +1,7 @@
 //! This query borrow-checks the MIR to (further) ensure it is not broken.
 
-use rustc::hir::def_id::DefId;
-use rustc::hir::Node;
-use rustc::hir::{self, HirId};
-use rustc::infer::InferCtxt;
+use rustc::hir::{self, def_id::DefId, HirId, Node};
+use rustc::infer::{opaque_types, InferCtxt};
 use rustc::lint::builtin::MUTABLE_BORROW_RESERVATION_CONFLICT;
 use rustc::lint::builtin::UNUSED_MUT;
 use rustc::mir::{
@@ -39,8 +37,11 @@ use crate::dataflow::FlowAtLocation;
 use crate::dataflow::MoveDataParamEnv;
 use crate::dataflow::{do_dataflow, DebugFormatted};
 use crate::dataflow::{MaybeInitializedPlaces, MaybeUninitializedPlaces};
+use crate::transform::MirSource;
 
-use self::diagnostics::AccessKind;
+use self::diagnostics::{
+    AccessKind, OutlivesSuggestionBuilder, RegionErrorKind, RegionErrorNamingCtx, RegionErrors,
+};
 use self::flows::Flows;
 use self::location::LocationTable;
 use self::prefixes::PrefixSet;
@@ -202,22 +203,28 @@ fn do_mir_borrowck<'a, 'tcx>(
     let borrow_set =
         Rc::new(BorrowSet::build(tcx, body, locals_are_invalidated_at_exit, &mdpe.move_data));
 
-    // If we are in non-lexical mode, compute the non-lexical lifetimes.
-    let (regioncx, polonius_output, opt_closure_req) = nll::compute_regions(
-        infcx,
-        def_id,
-        free_regions,
-        body,
-        &promoted,
-        &local_names,
-        &upvars,
-        location_table,
-        param_env,
-        &mut flow_inits,
-        &mdpe.move_data,
-        &borrow_set,
-        &mut errors_buffer,
-    );
+    // Compute non-lexical lifetimes.
+    let nll::NllOutput { regioncx, polonius_output, opt_closure_req, nll_errors } =
+        nll::compute_regions(
+            infcx,
+            def_id,
+            free_regions,
+            body,
+            &promoted,
+            location_table,
+            param_env,
+            &mut flow_inits,
+            &mdpe.move_data,
+            &borrow_set,
+        );
+
+    // Dump MIR results into a file, if that is enabled. This let us
+    // write unit-tests, as well as helping with debugging.
+    nll::dump_mir_results(infcx, MirSource::item(def_id), &body, &regioncx, &opt_closure_req);
+
+    // We also have a `#[rustc_nll]` annotation that causes us to dump
+    // information.
+    nll::dump_annotation(infcx, &body, def_id, &regioncx, &opt_closure_req, &mut errors_buffer);
 
     // The various `flow_*` structures can be large. We drop `flow_inits` here
     // so it doesn't overlap with the others below. This reduces peak memory
@@ -288,6 +295,9 @@ fn do_mir_borrowck<'a, 'tcx>(
         local_names,
     };
 
+    // Compute and report region errors, if any.
+    mbcx.report_region_errors(nll_errors);
+
     let mut state = Flows::new(flow_borrows, flow_uninits, flow_ever_inits, polonius_output);
 
     if let Some(errors) = move_errors {
@@ -1464,6 +1474,131 @@ 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`.
+
+        // FIXME(mark-i-m): Would be great to get rid of the naming context.
+        let mut region_naming = RegionErrorNamingCtx::new();
+        let mut outlives_suggestion =
+            OutlivesSuggestionBuilder::new(self.mir_def_id, &self.local_names);
+
+        for nll_error in nll_errors.into_iter() {
+            match nll_error {
+                RegionErrorKind::TypeTestDoesNotLiveLongEnough { span, generic } => {
+                    // 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(span, &format!("`{}` does not live long enough", generic))
+                        .buffer(&mut self.errors_buffer);
+                }
+
+                RegionErrorKind::TypeTestGenericBoundError {
+                    span,
+                    generic,
+                    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,
+                            span,
+                            None,
+                            generic,
+                            lower_bound_region,
+                        )
+                        .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_region,
+                } => {
+                    // 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 {
+                        let db = self.nonlexical_regioncx.report_error(
+                            &self.body,
+                            &self.local_names,
+                            &self.upvars,
+                            self.infcx,
+                            self.mir_def_id,
+                            longer_fr,
+                            fr_origin,
+                            shorter_fr,
+                            &mut outlives_suggestion,
+                            &mut region_naming,
+                        );
+
+                        db.buffer(&mut self.errors_buffer);
+                    } 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.body,
+            &self.nonlexical_regioncx,
+            self.infcx,
+            &mut self.errors_buffer,
+            &mut region_naming,
+        );
+    }
 }
 
 impl<'cx, 'tcx> MirBorrowckCtxt<'cx, 'tcx> {