Auto merge of #133858 - dianne:better-blame-constraints-for-static, r=lcnr

`best_blame_constraint`: Blame better constraints when the region graph has cycles from invariance or `'static`

This fixes #132749 by changing which constraint is blamed for region errors in several cases. `best_blame_constraint` had a heuristic that tried to pinpoint the constraint causing an error by filtering out any constraints where the outliving region is unified with the ultimate target region being outlived. However, it used the SCCs of the region graph to do this, which is unreliable; in particular, if the target region is `'static`, or if there are cycles from the presence of invariant types, it was skipping over the constraints it should be blaming. As is the case in that issue, this could lead to confusing diagnostics. The simplest fix seems to work decently, judging by test stderr: this makes `best_blame_constraint` no longer filter constraints by their outliving region's SCC.

There are admittedly some quirks in the test output. In many cases, subdiagnostics that depend on the particular constraint being blamed have either started or stopped being emitted. After starting at this for quite a while, I think anything too fickle about whether it outputs based on the particular constraint being blamed should instead be looking at the constraint path as a whole, similar to what's done for [the placeholder-from-predicate note](master...dianne:rust:better-blame-constraints-for-static#diff-3c0de6462469af483c9ecdf2c4b00cb26192218ef2d5c62a0fde75107a74caaeR506).

Very many tests involving invariant types gained a note pointing out the types' invariance, but in a few cases it was lost. A particularly illustrative example is [tests/ui/lifetimes/copy_modulo_regions.stderr](https://github.com/rust-lang/rust/compare/master...dianne:rust:better-blame-constraints-for-static?expand=1#diff-96e1f8b29789b3c4ce2f77a5e0fba248829b97ef9d1ce39e7d2b4aa57b2cf4f0); I'd argue the new constraint is a better one to blame, but it lacks the variance diagnostic information that's elsewhere in the constraint path. If desired, I can try making that note check the whole path rather than just the blamed constraint.

The subdiagnostic [`BorrowExplanation::add_object_lifetime_default_note`](https://doc.rust-lang.org/nightly/nightly-rustc/rustc_borrowck/diagnostics/explain_borrow/enum.BorrowExplanation.html#method.add_object_lifetime_default_note) depends on a `Cast` being blamed, so [a special case](364ca7f) was necessary to keep it from disappearing from tests specifically testing for it. However, see the FIXME comment in that commit; I think the special case should be removed once that subdiagnostic works properly, but it's nontrivial enough to warrant a separate PR. Incidentally, this removes the note from a test where it was being added erroneously: in [tests/ui/borrowck/two-phase-surprise-no-conflict.stderr](https://github.com/rust-lang/rust/compare/master...dianne:rust:better-blame-constraints-for-static?expand=1#diff-8cf085af8203677de6575a45458c9e6b03412a927df879412adec7e4f7ff5e14), the object lifetime is explicitly provided and it's not `'static`.

Author: bors

Cargo.lock

@@ -4115,7 +4115,6 @@ name = "rustc_middle"
 version = "0.0.0"
 dependencies = [
  "bitflags",
- "derive-where",
  "either",
  "field-offset",
  "gsgdt",

compiler/rustc_borrowck/src/diagnostics/conflict_errors.rs

@@ -1516,15 +1516,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         });
 
         self.explain_why_borrow_contains_point(location, borrow, None)
-            .add_explanation_to_diagnostic(
-                self.infcx.tcx,
-                self.body,
-                &self.local_names,
-                &mut err,
-                "",
-                Some(borrow_span),
-                None,
-            );
+            .add_explanation_to_diagnostic(&self, &mut err, "", Some(borrow_span), None);
         self.suggest_copy_for_type_in_cloned_ref(&mut err, place);
         let typeck_results = self.infcx.tcx.typeck(self.mir_def_id());
         if let Some(expr) = self.find_expr(borrow_span) {
@@ -1591,15 +1583,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         });
 
         self.explain_why_borrow_contains_point(location, borrow, None)
-            .add_explanation_to_diagnostic(
-                self.infcx.tcx,
-                self.body,
-                &self.local_names,
-                &mut err,
-                "",
-                None,
-                None,
-            );
+            .add_explanation_to_diagnostic(&self, &mut err, "", None, None);
         err
     }
 
@@ -1886,9 +1870,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         }
 
         explanation.add_explanation_to_diagnostic(
-            self.infcx.tcx,
-            self.body,
-            &self.local_names,
+            &self,
             &mut err,
             first_borrow_desc,
             None,
@@ -3046,15 +3028,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
 
             if let BorrowExplanation::MustBeValidFor { .. } = explanation {
             } else {
-                explanation.add_explanation_to_diagnostic(
-                    self.infcx.tcx,
-                    self.body,
-                    &self.local_names,
-                    &mut err,
-                    "",
-                    None,
-                    None,
-                );
+                explanation.add_explanation_to_diagnostic(&self, &mut err, "", None, None);
             }
         } else {
             err.span_label(borrow_span, "borrowed value does not live long enough");
@@ -3067,15 +3041,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
                 }
             });
 
-            explanation.add_explanation_to_diagnostic(
-                self.infcx.tcx,
-                self.body,
-                &self.local_names,
-                &mut err,
-                "",
-                Some(borrow_span),
-                None,
-            );
+            explanation.add_explanation_to_diagnostic(&self, &mut err, "", Some(borrow_span), None);
         }
 
         err
@@ -3128,15 +3094,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
             _ => {}
         }
 
-        explanation.add_explanation_to_diagnostic(
-            self.infcx.tcx,
-            self.body,
-            &self.local_names,
-            &mut err,
-            "",
-            None,
-            None,
-        );
+        explanation.add_explanation_to_diagnostic(&self, &mut err, "", None, None);
 
         self.buffer_error(err);
     }
@@ -3309,15 +3267,7 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
             }
             _ => {}
         }
-        explanation.add_explanation_to_diagnostic(
-            self.infcx.tcx,
-            self.body,
-            &self.local_names,
-            &mut err,
-            "",
-            None,
-            None,
-        );
+        explanation.add_explanation_to_diagnostic(&self, &mut err, "", None, None);
 
         borrow_spans.args_subdiag(&mut err, |args_span| {
             crate::session_diagnostics::CaptureArgLabel::Capture {
@@ -3808,15 +3758,8 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
             }
         });
 
-        self.explain_why_borrow_contains_point(location, loan, None).add_explanation_to_diagnostic(
-            self.infcx.tcx,
-            self.body,
-            &self.local_names,
-            &mut err,
-            "",
-            None,
-            None,
-        );
+        self.explain_why_borrow_contains_point(location, loan, None)
+            .add_explanation_to_diagnostic(&self, &mut err, "", None, None);
 
         self.explain_deref_coercion(loan, &mut err);
 

compiler/rustc_borrowck/src/diagnostics/explain_borrow.rs

@@ -8,7 +8,6 @@ use std::assert_matches::assert_matches;
 use rustc_errors::{Applicability, Diag};
 use rustc_hir as hir;
 use rustc_hir::intravisit::Visitor;
-use rustc_index::IndexSlice;
 use rustc_infer::infer::NllRegionVariableOrigin;
 use rustc_middle::middle::resolve_bound_vars::ObjectLifetimeDefault;
 use rustc_middle::mir::{
@@ -18,14 +17,15 @@ use rustc_middle::mir::{
 use rustc_middle::ty::adjustment::PointerCoercion;
 use rustc_middle::ty::{self, RegionVid, Ty, TyCtxt};
 use rustc_middle::util::CallKind;
-use rustc_span::{DesugaringKind, Span, Symbol, kw, sym};
+use rustc_span::{DesugaringKind, Span, kw, sym};
 use rustc_trait_selection::error_reporting::traits::FindExprBySpan;
 use tracing::{debug, instrument};
 
 use super::{RegionName, UseSpans, find_use};
 use crate::borrow_set::BorrowData;
+use crate::constraints::OutlivesConstraint;
 use crate::nll::ConstraintDescription;
-use crate::region_infer::{BlameConstraint, Cause, ExtraConstraintInfo};
+use crate::region_infer::{BlameConstraint, Cause};
 use crate::{MirBorrowckCtxt, WriteKind};
 
 #[derive(Debug)]
@@ -43,7 +43,7 @@ pub(crate) enum BorrowExplanation<'tcx> {
         span: Span,
         region_name: RegionName,
         opt_place_desc: Option<String>,
-        extra_info: Vec<ExtraConstraintInfo>,
+        path: Vec<OutlivesConstraint<'tcx>>,
     },
     Unexplained,
 }
@@ -63,14 +63,16 @@ impl<'tcx> BorrowExplanation<'tcx> {
     }
     pub(crate) fn add_explanation_to_diagnostic(
         &self,
-        tcx: TyCtxt<'tcx>,
-        body: &Body<'tcx>,
-        local_names: &IndexSlice<Local, Option<Symbol>>,
+        cx: &MirBorrowckCtxt<'_, '_, 'tcx>,
         err: &mut Diag<'_>,
         borrow_desc: &str,
         borrow_span: Option<Span>,
         multiple_borrow_span: Option<(Span, Span)>,
     ) {
+        let tcx = cx.infcx.tcx;
+        let body = cx.body;
+        let local_names = &cx.local_names;
+
         if let Some(span) = borrow_span {
             let def_id = body.source.def_id();
             if let Some(node) = tcx.hir().get_if_local(def_id)
@@ -306,7 +308,7 @@ impl<'tcx> BorrowExplanation<'tcx> {
                 ref region_name,
                 ref opt_place_desc,
                 from_closure: _,
-                ref extra_info,
+                ref path,
             } => {
                 region_name.highlight_region_name(err);
 
@@ -328,13 +330,8 @@ impl<'tcx> BorrowExplanation<'tcx> {
                     );
                 };
 
-                for extra in extra_info {
-                    match extra {
-                        ExtraConstraintInfo::PlaceholderFromPredicate(span) => {
-                            err.span_note(*span, "due to current limitations in the borrow checker, this implies a `'static` lifetime");
-                        }
-                    }
-                }
+                cx.add_placeholder_from_predicate_note(err, &path);
+                cx.add_sized_or_copy_bound_info(err, category, &path);
 
                 if let ConstraintCategory::Cast {
                     is_implicit_coercion: true,
@@ -487,8 +484,9 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
         &self,
         borrow_region: RegionVid,
         outlived_region: RegionVid,
-    ) -> (ConstraintCategory<'tcx>, bool, Span, Option<RegionName>, Vec<ExtraConstraintInfo>) {
-        let (blame_constraint, extra_info) = self.regioncx.best_blame_constraint(
+    ) -> (ConstraintCategory<'tcx>, bool, Span, Option<RegionName>, Vec<OutlivesConstraint<'tcx>>)
+    {
+        let (blame_constraint, path) = self.regioncx.best_blame_constraint(
             borrow_region,
             NllRegionVariableOrigin::FreeRegion,
             |r| self.regioncx.provides_universal_region(r, borrow_region, outlived_region),
@@ -497,7 +495,7 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
 
         let outlived_fr_name = self.give_region_a_name(outlived_region);
 
-        (category, from_closure, cause.span, outlived_fr_name, extra_info)
+        (category, from_closure, cause.span, outlived_fr_name, path)
     }
 
     /// Returns structured explanation for *why* the borrow contains the
@@ -596,7 +594,7 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
 
             None => {
                 if let Some(region) = self.to_error_region_vid(borrow_region_vid) {
-                    let (category, from_closure, span, region_name, extra_info) =
+                    let (category, from_closure, span, region_name, path) =
                         self.free_region_constraint_info(borrow_region_vid, region);
                     if let Some(region_name) = region_name {
                         let opt_place_desc = self.describe_place(borrow.borrowed_place.as_ref());
@@ -606,7 +604,7 @@ impl<'tcx> MirBorrowckCtxt<'_, '_, 'tcx> {
                             span,
                             region_name,
                             opt_place_desc,
-                            extra_info,
+                            path,
                         }
                     } else {
                         debug!("Could not generate a region name");

compiler/rustc_borrowck/src/diagnostics/mod.rs

@@ -8,14 +8,16 @@ use rustc_errors::{Applicability, Diag, MultiSpan};
 use rustc_hir::def::{CtorKind, Namespace};
 use rustc_hir::{self as hir, CoroutineKind, LangItem};
 use rustc_index::IndexSlice;
-use rustc_infer::infer::BoundRegionConversionTime;
+use rustc_infer::infer::{
+    BoundRegionConversionTime, NllRegionVariableOrigin, RegionVariableOrigin,
+};
 use rustc_infer::traits::SelectionError;
 use rustc_middle::bug;
 use rustc_middle::mir::tcx::PlaceTy;
 use rustc_middle::mir::{
-    AggregateKind, CallSource, ConstOperand, FakeReadCause, Local, LocalInfo, LocalKind, Location,
-    Operand, Place, PlaceRef, ProjectionElem, Rvalue, Statement, StatementKind, Terminator,
-    TerminatorKind,
+    AggregateKind, CallSource, ConstOperand, ConstraintCategory, FakeReadCause, Local, LocalInfo,
+    LocalKind, Location, Operand, Place, PlaceRef, ProjectionElem, Rvalue, Statement,
+    StatementKind, Terminator, TerminatorKind,
 };
 use rustc_middle::ty::print::Print;
 use rustc_middle::ty::{self, Instance, Ty, TyCtxt};
@@ -33,7 +35,9 @@ use tracing::debug;
 
 use super::MirBorrowckCtxt;
 use super::borrow_set::BorrowData;
+use crate::constraints::OutlivesConstraint;
 use crate::fluent_generated as fluent;
+use crate::nll::ConstraintDescription;
 use crate::session_diagnostics::{
     CaptureArgLabel, CaptureReasonLabel, CaptureReasonNote, CaptureReasonSuggest, CaptureVarCause,
     CaptureVarKind, CaptureVarPathUseCause, OnClosureNote,
@@ -619,6 +623,52 @@ impl<'infcx, 'tcx> MirBorrowckCtxt<'_, 'infcx, 'tcx> {
         region.print(&mut printer).unwrap();
         printer.into_buffer()
     }
+
+    /// Add a note to region errors and borrow explanations when higher-ranked regions in predicates
+    /// implicitly introduce an "outlives `'static`" constraint.
+    fn add_placeholder_from_predicate_note(
+        &self,
+        err: &mut Diag<'_>,
+        path: &[OutlivesConstraint<'tcx>],
+    ) {
+        let predicate_span = path.iter().find_map(|constraint| {
+            let outlived = constraint.sub;
+            if let Some(origin) = self.regioncx.var_infos.get(outlived)
+                && let RegionVariableOrigin::Nll(NllRegionVariableOrigin::Placeholder(_)) =
+                    origin.origin
+                && let ConstraintCategory::Predicate(span) = constraint.category
+            {
+                Some(span)
+            } else {
+                None
+            }
+        });
+
+        if let Some(span) = predicate_span {
+            err.span_note(span, "due to current limitations in the borrow checker, this implies a `'static` lifetime");
+        }
+    }
+
+    /// Add a label to region errors and borrow explanations when outlives constraints arise from
+    /// proving a type implements `Sized` or `Copy`.
+    fn add_sized_or_copy_bound_info(
+        &self,
+        err: &mut Diag<'_>,
+        blamed_category: ConstraintCategory<'tcx>,
+        path: &[OutlivesConstraint<'tcx>],
+    ) {
+        for sought_category in [ConstraintCategory::SizedBound, ConstraintCategory::CopyBound] {
+            if sought_category != blamed_category
+                && let Some(sought_constraint) = path.iter().find(|c| c.category == sought_category)
+            {
+                let label = format!(
+                    "requirement occurs due to {}",
+                    sought_category.description().trim_end()
+                );
+                err.span_label(sought_constraint.span, label);
+            }
+        }
+    }
 }
 
 /// The span(s) associated to a use of a place.