Move handling of placeholder errors to before region inference

Author: amandasystems

compiler/rustc_borrowck/src/diagnostics/bound_region_errors.rs

@@ -24,7 +24,6 @@ use rustc_traits::{type_op_ascribe_user_type_with_span, type_op_prove_predicate_
 use tracing::{debug, instrument};
 
 use crate::MirBorrowckCtxt;
-use crate::region_infer::values::RegionElement;
 use crate::session_diagnostics::{
     HigherRankedErrorCause, HigherRankedLifetimeError, HigherRankedSubtypeError,
 };
@@ -49,11 +48,12 @@ impl<'tcx> UniverseInfo<'tcx> {
         UniverseInfo::RelateTys { expected, found }
     }
 
+    /// Report an error where an element erroneously made its way into `placeholder`.
     pub(crate) fn report_erroneous_element(
         &self,
         mbcx: &mut MirBorrowckCtxt<'_, '_, 'tcx>,
         placeholder: ty::PlaceholderRegion,
-        error_element: RegionElement,
+        error_element: Option<ty::PlaceholderRegion>,
         cause: ObligationCause<'tcx>,
     ) {
         match *self {
@@ -153,10 +153,17 @@ pub(crate) trait TypeOpInfo<'tcx> {
         &self,
         mbcx: &mut MirBorrowckCtxt<'_, '_, 'tcx>,
         placeholder: ty::PlaceholderRegion,
-        error_element: RegionElement,
+        error_element: Option<ty::PlaceholderRegion>,
         cause: ObligationCause<'tcx>,
     ) {
         let tcx = mbcx.infcx.tcx;
+
+        // FIXME: this logic is convoluted and strange, and
+        // we should probably just use the placeholders we get
+        // as arguments! However, upstream error reporting code
+        // needs adaptations for that to work (this universe is
+        // neither the assigned one, nor the calculated one but
+        // some base-shifted one for some reason?).
         let base_universe = self.base_universe();
         debug!(?base_universe);
 
@@ -172,20 +179,16 @@ pub(crate) trait TypeOpInfo<'tcx> {
             ty::Placeholder { universe: adjusted_universe.into(), bound: placeholder.bound },
         );
 
-        let error_region = if let RegionElement::PlaceholderRegion(error_placeholder) =
-            error_element
-        {
-            let adjusted_universe =
-                error_placeholder.universe.as_u32().checked_sub(base_universe.as_u32());
+        // FIXME: one day this should just be error_element, but see above about the adjusted universes. At that point, this method can probably be removed entirely.
+        let error_region = error_element.and_then(|e| {
+            let adjusted_universe = e.universe.as_u32().checked_sub(base_universe.as_u32());
             adjusted_universe.map(|adjusted| {
                 ty::Region::new_placeholder(
                     tcx,
-                    ty::Placeholder { universe: adjusted.into(), bound: error_placeholder.bound },
+                    ty::Placeholder { universe: adjusted.into(), bound: e.bound },
                 )
             })
-        } else {
-            None
-        };
+        });
 
         debug!(?placeholder_region);
 

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

@@ -104,8 +104,31 @@ pub(crate) enum RegionErrorKind<'tcx> {
     /// A generic bound failure for a type test (`T: 'a`).
     TypeTestError { type_test: TypeTest<'tcx> },
 
-    /// Higher-ranked subtyping error.
-    BoundUniversalRegionError {
+    /// 'a outlives 'b, and both are placeholders.
+    PlaceholderOutlivesPlaceholder {
+        rvid_a: RegionVid,
+        rvid_b: RegionVid,
+        origin_a: ty::PlaceholderRegion,
+        origin_b: ty::PlaceholderRegion,
+    },
+
+    /// Indicates that a placeholder has a universe too large for one
+    /// of its member existentials, or, equivalently, that there is
+    /// a path through the outlives constraint graph from a placeholder
+    /// to an existential region that cannot name it.
+    PlaceholderOutlivesExistentialThatCannotNameIt {
+        /// the placeholder that transitively outlives an
+        /// existential that shouldn't leak into it
+        longer_fr: RegionVid,
+        /// The existential leaking into `longer_fr`.
+        existential_that_cannot_name_longer: RegionVid,
+        // `longer_fr`'s originating placeholder region.
+        placeholder: ty::PlaceholderRegion,
+    },
+
+    /// Higher-ranked subtyping error. A placeholder outlives
+    /// either a location or a universal region.
+    PlaceholderOutlivesLocationOrUniversal {
         /// The placeholder free region.
         longer_fr: RegionVid,
         /// The region element that erroneously must be outlived by `longer_fr`.
@@ -361,30 +384,56 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
                     }
                 }
 
-                RegionErrorKind::BoundUniversalRegionError {
+                RegionErrorKind::PlaceholderOutlivesLocationOrUniversal {
                     longer_fr,
                     placeholder,
                     error_element,
+                } => self.report_erroneous_rvid_reaches_placeholder(
+                    longer_fr,
+                    placeholder,
+                    self.regioncx.region_from_element(longer_fr, &error_element),
+                ),
+                RegionErrorKind::PlaceholderOutlivesPlaceholder {
+                    rvid_a,
+                    rvid_b,
+                    origin_a,
+                    origin_b,
                 } => {
-                    let error_vid = self.regioncx.region_from_element(longer_fr, &error_element);
+                    debug!(
+                        "Placeholder mismatch: {rvid_a:?} ({origin_a:?}) reaches {rvid_b:?} ({origin_b:?})"
+                    );
 
-                    // Find the code to blame for the fact that `longer_fr` outlives `error_fr`.
                     let cause = self
                         .regioncx
                         .best_blame_constraint(
-                            longer_fr,
-                            NllRegionVariableOrigin::Placeholder(placeholder),
-                            error_vid,
+                            rvid_a,
+                            NllRegionVariableOrigin::Placeholder(origin_a),
+                            rvid_b,
                         )
                         .0
                         .cause;
 
-                    let universe = placeholder.universe;
-                    let universe_info = self.regioncx.universe_info(universe);
-
-                    universe_info.report_erroneous_element(self, placeholder, error_element, cause);
+                    // FIXME We may be able to shorten the code path here, and immediately
+                    // report a `RegionResolutionError::UpperBoundUniverseConflict`, but
+                    // that's left for a future refactoring.
+                    self.regioncx.universe_info(origin_a.universe).report_erroneous_element(
+                        self,
+                        origin_a,
+                        Some(origin_b),
+                        cause,
+                    );
                 }
 
+                RegionErrorKind::PlaceholderOutlivesExistentialThatCannotNameIt {
+                    longer_fr,
+                    existential_that_cannot_name_longer,
+                    placeholder,
+                } => self.report_erroneous_rvid_reaches_placeholder(
+                    longer_fr,
+                    placeholder,
+                    existential_that_cannot_name_longer,
+                ),
+
                 RegionErrorKind::RegionError { fr_origin, longer_fr, shorter_fr, is_reported } => {
                     if is_reported {
                         self.report_region_error(

compiler/rustc_borrowck/src/handle_placeholders.rs

@@ -9,11 +9,11 @@ use rustc_index::IndexVec;
 use rustc_infer::infer::RegionVariableOrigin;
 use rustc_middle::mir::ConstraintCategory;
 use rustc_middle::ty::{RegionVid, UniverseIndex};
-use tracing::{debug, trace};
+use tracing::{debug, instrument, trace};
 
 use crate::constraints::{ConstraintSccIndex, OutlivesConstraintSet};
 use crate::consumers::OutlivesConstraint;
-use crate::diagnostics::UniverseInfo;
+use crate::diagnostics::{RegionErrorKind, RegionErrors, UniverseInfo};
 use crate::region_infer::values::{LivenessValues, PlaceholderIndices};
 use crate::region_infer::{ConstraintSccs, RegionDefinition, Representative, TypeTest};
 use crate::ty::VarianceDiagInfo;
@@ -181,6 +181,43 @@ impl RegionTracker {
             !self.max_nameable_universe().can_name(placeholder_universe)
         })
     }
+
+    /// Check for the second and final type of placeholder leak,
+    /// where a placeholder `'p` outlives (transitively) an existential `'e`
+    /// and `'e` cannot name `'p`. This is sort of a dual of `unnameable_placeholder`;
+    /// one of the members of this SCC cannot be named by the SCC.
+    ///
+    /// Returns *a* culprit (though there may be more than one).
+    fn reaches_existential_that_cannot_name_us(&self) -> Option<RegionVid> {
+        let Representative::Placeholder(_p) = self.representative else {
+            return None;
+        };
+
+        let (reachable_lowest_max_u, reachable_lowest_max_u_rvid) = self.max_nameable_universe;
+
+        (!self.reachable_placeholders.can_be_named_by(reachable_lowest_max_u))
+            .then_some(reachable_lowest_max_u_rvid)
+    }
+
+    /// Determine if this SCC reaches a placeholder that isn't `placeholder_rvid`,
+    /// returning it if that is the case. This prefers the placeholder with the
+    /// smallest region variable ID.
+    fn reaches_other_placeholder(&self, placeholder_rvid: RegionVid) -> Option<RegionVid> {
+        match self.reachable_placeholders {
+            PlaceholderReachability::NoPlaceholders => None,
+            PlaceholderReachability::Placeholders { min_placeholder, max_placeholder, .. }
+                if min_placeholder == max_placeholder =>
+            {
+                None
+            }
+            PlaceholderReachability::Placeholders { min_placeholder, max_placeholder, .. }
+                if min_placeholder == placeholder_rvid =>
+            {
+                Some(max_placeholder)
+            }
+            PlaceholderReachability::Placeholders { min_placeholder, .. } => Some(min_placeholder),
+        }
+    }
 }
 
 impl scc::Annotation for RegionTracker {
@@ -270,6 +307,7 @@ pub(crate) fn compute_sccs_applying_placeholder_outlives_constraints<'tcx>(
     constraints: MirTypeckRegionConstraints<'tcx>,
     universal_region_relations: &Frozen<UniversalRegionRelations<'tcx>>,
     infcx: &BorrowckInferCtxt<'tcx>,
+    errors_buffer: &mut RegionErrors<'tcx>,
 ) -> LoweredConstraints<'tcx> {
     let universal_regions = &universal_region_relations.universal_regions;
     let (definitions, has_placeholders) = region_definitions(infcx, universal_regions);
@@ -322,6 +360,13 @@ pub(crate) fn compute_sccs_applying_placeholder_outlives_constraints<'tcx>(
         &mut outlives_constraints,
     );
 
+    find_placeholder_mismatch_errors(
+        &definitions,
+        &constraint_sccs,
+        &scc_annotations,
+        errors_buffer,
+    );
+
     let (constraint_sccs, scc_annotations) = if added_constraints {
         let mut annotations = SccAnnotations::init(&definitions);
 
@@ -419,3 +464,65 @@ fn rewrite_placeholder_outlives<'tcx>(
     }
     added_constraints
 }
+
+/// Identify errors where placeholders illegally reach other regions, and generate
+/// errors stored into `errors_buffer`.
+///
+/// There are two sources of such errors:
+/// 1. A placeholder reaches (possibly transitively) another placeholder.
+/// 2. A placeholder `p` reaches (possibly transitively) an existential `e`,
+///    where `e` has an allowed maximum universe smaller than `p`'s.
+///
+/// There are other potential placeholder errors, but those are detected after
+/// region inference, since it may apply type tests or member constraints that
+/// alter the contents of SCCs and thus can't be detected at this point.
+#[instrument(skip(definitions, sccs, annotations, errors_buffer), level = "debug")]
+fn find_placeholder_mismatch_errors<'tcx>(
+    definitions: &IndexVec<RegionVid, RegionDefinition<'tcx>>,
+    sccs: &Sccs<RegionVid, ConstraintSccIndex>,
+    annotations: &SccAnnotations<'_, '_, RegionTracker>,
+    errors_buffer: &mut RegionErrors<'tcx>,
+) {
+    use NllRegionVariableOrigin::Placeholder;
+    for (rvid, definition) in definitions.iter_enumerated() {
+        let Placeholder(origin_a) = definition.origin else {
+            continue;
+        };
+
+        let scc = sccs.scc(rvid);
+        let annotation = annotations.scc_to_annotation[scc];
+
+        if let Some(existential_that_cannot_name_rvid) =
+            annotation.reaches_existential_that_cannot_name_us()
+        {
+            errors_buffer.push(RegionErrorKind::PlaceholderOutlivesExistentialThatCannotNameIt {
+                longer_fr: rvid,
+                existential_that_cannot_name_longer: existential_that_cannot_name_rvid,
+                placeholder: origin_a,
+            })
+        }
+
+        let Some(other_placeholder) = annotation.reaches_other_placeholder(rvid) else {
+            trace!("{rvid:?} reaches no other placeholders");
+            continue;
+        };
+
+        debug!(
+            "Placeholder {rvid:?} of SCC {scc:?} reaches other placeholder {other_placeholder:?}"
+        );
+
+        // FIXME SURELY there is a neater way to do this?
+        let Placeholder(origin_b) = definitions[other_placeholder].origin else {
+            unreachable!(
+                "Region {rvid:?}, {other_placeholder:?} should be placeholders but aren't!"
+            );
+        };
+
+        errors_buffer.push(RegionErrorKind::PlaceholderOutlivesPlaceholder {
+            rvid_a: rvid,
+            rvid_b: other_placeholder,
+            origin_a,
+            origin_b,
+        });
+    }
+}

compiler/rustc_borrowck/src/lib.rs

@@ -2707,6 +2707,37 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
             tcx.emit_node_span_lint(UNUSED_MUT, lint_root, span, VarNeedNotMut { span: mut_span })
         }
     }
+
+    /// Report that longer_fr: shorter_fr, which doesn't hold,
+    /// where longer_fr is a placeholder from `placeholder`.
+    fn report_erroneous_rvid_reaches_placeholder(
+        &mut self,
+        longer_fr: RegionVid,
+        placeholder: ty::Placeholder<ty::BoundRegion>,
+        error_vid: RegionVid,
+    ) {
+        // Find the code to blame for the fact that `longer_fr` outlives `error_fr`.
+        let cause = self
+            .regioncx
+            .best_blame_constraint(
+                longer_fr,
+                NllRegionVariableOrigin::Placeholder(placeholder),
+                error_vid,
+            )
+            .0
+            .cause;
+
+        // FIXME these methods should have better names, and also probably not be this generic.
+        // FIXME note that we *throw away* the error element here! We probably want to
+        // thread it through the computation further down and use it, but there currently isn't
+        // anything there to receive it.
+        self.regioncx.universe_info(placeholder.universe).report_erroneous_element(
+            self,
+            placeholder,
+            None,
+            cause,
+        );
+    }
 }
 
 /// The degree of overlap between 2 places for borrow-checking.

compiler/rustc_borrowck/src/nll.rs

@@ -126,10 +126,13 @@ pub(crate) fn compute_regions<'tcx>(
     let polonius_output = root_cx.consumer.as_ref().map_or(false, |c| c.polonius_output())
         || infcx.tcx.sess.opts.unstable_opts.polonius.is_legacy_enabled();
 
+    let mut placeholder_errors = RegionErrors::new(infcx.tcx);
+
     let lowered_constraints = compute_sccs_applying_placeholder_outlives_constraints(
         constraints,
         &universal_region_relations,
         infcx,
+        &mut placeholder_errors,
     );
 
     // If requested, emit legacy polonius facts.
@@ -179,9 +182,17 @@ pub(crate) fn compute_regions<'tcx>(
     });
 
     // Solve the region constraints.
-    let (closure_region_requirements, nll_errors) =
+    let (closure_region_requirements, region_inference_errors) =
         regioncx.solve(infcx, body, polonius_output.clone());
 
+    let nll_errors = if region_inference_errors.is_empty() {
+        // Only flag the higher-kinded bounds errors if there are no borrowck errors.
+        placeholder_errors
+    } else {
+        debug!("Errors already reported, skipping these: {placeholder_errors:?}");
+        region_inference_errors
+    };
+
     NllOutput {
         regioncx,
         polonius_input: polonius_facts.map(Box::new),