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

Centril · web-flow · commit 93fb9e6a86e3 · 2019-12-22T13:29:19.000+01:00
Refactorings to borrowck region diagnostic reporting This PR is a followup to rust-lang#66886 and rust-lang#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 rust-lang#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.
diff --git a/src/librustc_mir/borrow_check/diagnostics/mod.rs b/src/librustc_mir/borrow_check/diagnostics/mod.rs
@@ -32,7 +32,7 @@ mod explain_borrow;
 
 crate use mutability_errors::AccessKind;
 crate use region_name::{RegionName, RegionNameSource, RegionErrorNamingCtx};
-crate use region_errors::{ErrorReportingCtx, ErrorConstraintInfo};
+crate use region_errors::{ErrorReportingCtx, ErrorConstraintInfo, RegionErrors, RegionErrorKind};
 crate use outlives_suggestion::OutlivesSuggestionBuilder;
 
 pub(super) struct IncludingDowncast(pub(super) bool);
diff --git a/src/librustc_mir/borrow_check/diagnostics/outlives_suggestion.rs b/src/librustc_mir/borrow_check/diagnostics/outlives_suggestion.rs
@@ -250,7 +250,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;
         }
diff --git a/src/librustc_mir/borrow_check/diagnostics/region_errors.rs b/src/librustc_mir/borrow_check/diagnostics/region_errors.rs
@@ -3,12 +3,13 @@
 use rustc::hir::def_id::DefId;
 use rustc::infer::{
     error_reporting::nice_region_error::NiceRegionError,
+    region_constraints::GenericKind,
     InferCtxt, NLLRegionVariableOrigin,
 };
 use rustc::mir::{
     ConstraintCategory, Local, Location, Body,
 };
-use rustc::ty::{self, RegionVid};
+use rustc::ty::{self, RegionVid, Ty};
 use rustc_index::vec::IndexVec;
 use rustc_errors::DiagnosticBuilder;
 use std::collections::VecDeque;
@@ -53,13 +54,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.
diff --git a/src/librustc_mir/borrow_check/mod.rs b/src/librustc_mir/borrow_check/mod.rs
@@ -3,7 +3,7 @@
 use rustc::hir::{self, HirId};
 use rustc::hir::Node;
 use rustc::hir::def_id::DefId;
-use rustc::infer::InferCtxt;
+use rustc::infer::{opaque_types, InferCtxt};
 use rustc::lint::builtin::UNUSED_MUT;
 use rustc::lint::builtin::{MUTABLE_BORROW_RESERVATION_CONFLICT};
 use rustc::mir::{AggregateKind, BasicBlock, BorrowCheckResult, BorrowKind};
@@ -39,12 +39,15 @@ use crate::dataflow::MoveDataParamEnv;
 use crate::dataflow::{do_dataflow, DebugFormatted};
 use crate::dataflow::EverInitializedPlaces;
 use crate::dataflow::{MaybeInitializedPlaces, MaybeUninitializedPlaces};
+use crate::transform::MirSource;
 
 use self::flows::Flows;
 use self::location::LocationTable;
 use self::prefixes::PrefixSet;
 use self::MutateMode::{JustWrite, WriteAndRead};
-use self::diagnostics::AccessKind;
+use self::diagnostics::{
+    AccessKind, RegionErrors, RegionErrorKind, OutlivesSuggestionBuilder, RegionErrorNamingCtx,
+};
 
 use self::path_utils::*;
 
@@ -211,20 +214,40 @@ 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(
+    // 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,
-        &local_names,
-        &upvars,
         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,
     );
 
@@ -297,6 +320,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,
@@ -1560,6 +1586,129 @@ 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> {
diff --git a/src/librustc_mir/borrow_check/nll.rs b/src/librustc_mir/borrow_check/nll.rs
diff --git a/src/librustc_mir/borrow_check/region_infer/mod.rs b/src/librustc_mir/borrow_check/region_infer/mod.rs
