Simplify the placeholder error reporting, add skipping logic

Author: amandasystems

compiler/rustc_borrowck/src/diagnostics/region_errors.rs

@@ -104,27 +104,10 @@ pub(crate) enum RegionErrorKind<'tcx> {
     /// A generic bound failure for a type test (`T: 'a`).
     TypeTestError { type_test: TypeTest<'tcx> },
 
-    /// '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,
-    },
+    /// 'a outlives 'b, which does not hold. 'a is always a
+    /// placeholder and 'b is either an existential that cannot name
+    /// 'a, or another placeholder.
+    PlaceholderOutlivesIllegalRegion { longer_fr: RegionVid, illegally_outlived_r: RegionVid },
 
     /// Higher-ranked subtyping error. A placeholder outlives
     /// either a location or a universal region.
@@ -133,8 +116,6 @@ pub(crate) enum RegionErrorKind<'tcx> {
         longer_fr: RegionVid,
         /// The region element that erroneously must be outlived by `longer_fr`.
         error_element: RegionElement,
-        /// The placeholder region.
-        placeholder: ty::PlaceholderRegion,
     },
 
     /// Any other lifetime error.
@@ -361,53 +342,18 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
 
                 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,
-                } => {
-                    debug!(
-                        "Placeholder mismatch: {rvid_a:?} ({origin_a:?}) reaches {rvid_b:?} ({origin_b:?})"
-                    );
-
-                    let cause = self
-                        .regioncx
-                        .best_blame_constraint(
-                            rvid_a,
-                            NllRegionVariableOrigin::Placeholder(origin_a),
-                            rvid_b,
-                        )
-                        .0
-                        .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(
+                RegionErrorKind::PlaceholderOutlivesIllegalRegion {
                     longer_fr,
-                    placeholder,
-                    existential_that_cannot_name_longer,
-                ),
+                    illegally_outlived_r,
+                } => {
+                    self.report_erroneous_rvid_reaches_placeholder(longer_fr, illegally_outlived_r)
+                }
 
                 RegionErrorKind::RegionError { fr_origin, longer_fr, shorter_fr, is_reported } => {
                     if is_reported {

compiler/rustc_borrowck/src/handle_placeholders.rs

@@ -35,20 +35,31 @@ pub(crate) struct LoweredConstraints<'tcx> {
 }
 
 impl<'d, 'tcx, A: scc::Annotation> SccAnnotations<'d, 'tcx, A> {
-    pub(crate) fn init(definitions: &'d IndexVec<RegionVid, RegionDefinition<'tcx>>) -> Self {
-        Self { scc_to_annotation: IndexVec::new(), definitions }
+    pub(crate) fn with_checking_placeholders(
+        definitions: &'d IndexVec<RegionVid, RegionDefinition<'tcx>>,
+    ) -> Self {
+        Self { scc_to_annotation: IndexVec::new(), definitions, check_placeholders: true }
+    }
+
+    pub(crate) fn without_checking_placeholders(
+        definitions: &'d IndexVec<RegionVid, RegionDefinition<'tcx>>,
+    ) -> Self {
+        Self { scc_to_annotation: IndexVec::new(), definitions, check_placeholders: true }
     }
 }
 
 /// A Visitor for SCC annotation construction.
 pub(crate) struct SccAnnotations<'d, 'tcx, A: scc::Annotation> {
     pub(crate) scc_to_annotation: IndexVec<ConstraintSccIndex, A>,
     definitions: &'d IndexVec<RegionVid, RegionDefinition<'tcx>>,
+    /// True if we apply extra validity checking logic to placeholder
+    /// regions, false to skip it. Used only for performance reasons.
+    check_placeholders: bool,
 }
 
 impl scc::Annotations<RegionVid> for SccAnnotations<'_, '_, RegionTracker> {
     fn new(&self, element: RegionVid) -> RegionTracker {
-        RegionTracker::new(element, &self.definitions[element])
+        RegionTracker::new(element, &self.definitions[element], self.check_placeholders)
     }
 
     fn annotate_scc(&mut self, scc: ConstraintSccIndex, annotation: RegionTracker) {
@@ -199,7 +210,11 @@ pub(crate) struct RegionTracker {
 }
 
 impl RegionTracker {
-    pub(crate) fn new(rvid: RegionVid, definition: &RegionDefinition<'_>) -> Self {
+    pub(crate) fn new(
+        rvid: RegionVid,
+        definition: &RegionDefinition<'_>,
+        check_placeholders: bool,
+    ) -> Self {
         use NllRegionVariableOrigin::*;
         use PlaceholderConstraints::*;
         use PlaceholderReachability::*;
@@ -222,13 +237,13 @@ impl RegionTracker {
                     max_placeholder: rvid,
                 },
                 min_max_nameable_universe,
-                exception: Some(Name(definition.universe)),
+                exception: check_placeholders.then_some(Name(definition.universe)),
                 representative,
             },
             Existential { .. } => Self {
                 reachable_placeholders: NoPlaceholders,
                 min_max_nameable_universe,
-                exception: Some(NameableBy(rvid, definition.universe)),
+                exception: check_placeholders.then_some(NameableBy(rvid, definition.universe)),
                 representative,
             },
         }
@@ -439,7 +454,7 @@ pub(crate) fn compute_sccs_applying_placeholder_outlives_constraints<'tcx>(
             )
         };
 
-    let mut scc_annotations = SccAnnotations::init(&definitions);
+    let mut scc_annotations = SccAnnotations::with_checking_placeholders(&definitions);
     let constraint_sccs = compute_sccs(&outlives_constraints, &mut scc_annotations);
 
     // This code structure is a bit convoluted because it allows for a planned
@@ -475,7 +490,7 @@ pub(crate) fn compute_sccs_applying_placeholder_outlives_constraints<'tcx>(
     );
 
     let (constraint_sccs, scc_annotations) = if added_constraints {
-        let mut annotations = SccAnnotations::init(&definitions);
+        let mut annotations = SccAnnotations::without_checking_placeholders(&definitions);
 
         // We changed the constraint set and so must recompute SCCs.
         // Optimisation opportunity: if we can add them incrementally (and that's
@@ -583,44 +598,21 @@ fn find_placeholder_mismatch_errors<'tcx>(
 ) {
     use NllRegionVariableOrigin::Placeholder;
     for (rvid, definition) in definitions.iter_enumerated() {
-        let Placeholder(origin_a) = definition.origin else {
+        let Placeholder(_) = definition.origin else {
             continue;
         };
 
         let scc = sccs.scc(rvid);
         let annotation = annotations.scc_to_annotation[scc];
 
-        if let Some(cannot_name_rvid) = annotation.reaches_existential_that_cannot_name_us() {
-            debug!("Existential {cannot_name_rvid:?} lowered our universe...");
-
-            errors_buffer.push(RegionErrorKind::PlaceholderOutlivesExistentialThatCannotNameIt {
+        if let Some(illegally_outlived_r) = annotation
+            .reaches_existential_that_cannot_name_us()
+            .or_else(|| annotation.reaches_other_placeholder(rvid))
+        {
+            errors_buffer.push(RegionErrorKind::PlaceholderOutlivesIllegalRegion {
                 longer_fr: rvid,
-                existential_that_cannot_name_longer: cannot_name_rvid,
-                placeholder: origin_a,
+                illegally_outlived_r,
             })
         }
-
-        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

@@ -2692,24 +2692,30 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
         }
     }
 
-    /// Report that longer_fr: shorter_fr, which doesn't hold,
-    /// where longer_fr is a placeholder from `placeholder`.
+    /// Report that `longer_fr: error_vid`, which doesn't hold,
+    /// where `longer_fr` is a placeholder.
     fn report_erroneous_rvid_reaches_placeholder(
         &mut self,
         longer_fr: RegionVid,
-        placeholder: ty::Placeholder<ty::BoundRegion>,
         error_vid: RegionVid,
     ) {
+        use NllRegionVariableOrigin::*;
+
+        let origin_longer = self.regioncx.definitions[longer_fr].origin;
+
         // 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;
+        let cause =
+            self.regioncx.best_blame_constraint(longer_fr, origin_longer, error_vid).0.cause;
+
+        let Placeholder(placeholder) = origin_longer else {
+            bug!("Expected {longer_fr:?} to come from placeholder!");
+        };
+
+        // FIXME: Is throwing away the existential region really the best here?
+        let error_region = match self.regioncx.definitions[error_vid].origin {
+            FreeRegion | Existential { .. } => None,
+            Placeholder(other_placeholder) => Some(other_placeholder),
+        };
 
         // 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
@@ -2718,7 +2724,7 @@ impl<'a, 'tcx> MirBorrowckCtxt<'a, '_, 'tcx> {
         self.regioncx.universe_info(placeholder.universe).report_erroneous_element(
             self,
             placeholder,
-            None,
+            error_region,
             cause,
         );
     }

compiler/rustc_borrowck/src/region_infer/mod.rs

@@ -1249,7 +1249,6 @@ impl<'tcx> RegionInferenceContext<'tcx> {
             errors_buffer.push(RegionErrorKind::PlaceholderOutlivesLocationOrUniversal {
                 longer_fr,
                 error_element,
-                placeholder,
             });
         } else {
             debug!("check_bound_universal_region: all bounds satisfied");

compiler/rustc_borrowck/src/region_infer/opaque_types/region_ctxt.rs

@@ -56,7 +56,7 @@ impl<'a, 'tcx> RegionCtxt<'a, 'tcx> {
                 )
             };
 
-        let mut scc_annotations = SccAnnotations::init(&definitions);
+        let mut scc_annotations = SccAnnotations::without_checking_placeholders(&definitions);
         let mut constraint_sccs = compute_sccs(&outlives_constraints, &mut scc_annotations);
 
         let added_constraints = crate::handle_placeholders::rewrite_placeholder_outlives(
@@ -67,7 +67,7 @@ impl<'a, 'tcx> RegionCtxt<'a, 'tcx> {
         );
 
         if added_constraints {
-            scc_annotations = SccAnnotations::init(&definitions);
+            scc_annotations = SccAnnotations::without_checking_placeholders(&definitions);
             constraint_sccs = compute_sccs(&outlives_constraints, &mut scc_annotations);
         }