best_blame_constraint: prioritize blaming interesting-seeming constraints

Author: dianne

Cargo.lock

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

compiler/rustc_borrowck/src/region_infer/mod.rs

@@ -2026,87 +2026,102 @@ impl<'tcx> RegionInferenceContext<'tcx> {
             | NllRegionVariableOrigin::Existential { from_forall: true } => false,
         };
 
-        let interesting_to_blame = |constraint: &OutlivesConstraint<'tcx>| {
-            !matches!(
-                constraint.category,
-                ConstraintCategory::OpaqueType
-                    | ConstraintCategory::Boring
-                    | ConstraintCategory::BoringNoLocation
-                    | ConstraintCategory::Internal
-                    | ConstraintCategory::Predicate(_)
-                    | ConstraintCategory::Assignment { has_interesting_ty: false }
-            ) && constraint.span.desugaring_kind().is_none_or(|kind| {
-                // Try to avoid blaming constraints from desugarings, since they may not clearly
-                // clearly match what users have written. As an exception, allow blaming returns
-                // generated by `?` desugaring, since the correspondence is fairly clear.
-                kind == DesugaringKind::QuestionMark
-                    && matches!(constraint.category, ConstraintCategory::Return(_))
-            })
+        // To pick a constraint to blame, we organize constraints by how interesting we expect them
+        // to be in diagnostics, then pick the most interesting one closest to either the source or
+        // the target on our constraint path.
+        let constraint_interest = |constraint: &OutlivesConstraint<'tcx>| {
+            // Try to avoid blaming constraints from desugarings, since they may not clearly match
+            // match what users have written. As an exception, allow blaming returns generated by
+            // `?` desugaring, since the correspondence is fairly clear.
+            let category = if let Some(kind) = constraint.span.desugaring_kind()
+                && (kind != DesugaringKind::QuestionMark
+                    || !matches!(constraint.category, ConstraintCategory::Return(_)))
+            {
+                ConstraintCategory::Boring
+            } else {
+                constraint.category
+            };
+
+            match category {
+                // Returns usually provide a type to blame and have specially written diagnostics,
+                // so prioritize them.
+                ConstraintCategory::Return(_) => 0,
+                // Unsizing coercions are interesting, since we have a note for that:
+                // `BorrowExplanation::add_object_lifetime_default_note`.
+                // FIXME(dianne): That note shouldn't depend on a coercion being blamed; see issue
+                // #131008 for an example of where we currently don't emit it but should.
+                // Once the note is handled properly, this case should be removed. Until then, it
+                // should be as limited as possible; the note is prone to false positives and this
+                // constraint usually isn't best to blame.
+                ConstraintCategory::Cast {
+                    unsize_to: Some(unsize_ty),
+                    is_implicit_coercion: true,
+                } if target_region == self.universal_regions().fr_static
+                    // Mirror the note's condition, to minimize how often this diverts blame.
+                    && let ty::Adt(_, args) = unsize_ty.kind()
+                    && args.iter().any(|arg| arg.as_type().is_some_and(|ty| ty.is_trait()))
+                    // Mimic old logic for this, to minimize false positives in tests.
+                    && !path
+                        .iter()
+                        .any(|c| matches!(c.category, ConstraintCategory::TypeAnnotation)) =>
+                {
+                    1
+                }
+                // Between other interesting constraints, order by their position on the `path`.
+                ConstraintCategory::Yield
+                | ConstraintCategory::UseAsConst
+                | ConstraintCategory::UseAsStatic
+                | ConstraintCategory::TypeAnnotation
+                | ConstraintCategory::Cast { .. }
+                | ConstraintCategory::CallArgument(_)
+                | ConstraintCategory::CopyBound
+                | ConstraintCategory::SizedBound
+                | ConstraintCategory::Assignment { has_interesting_ty: true }
+                | ConstraintCategory::Usage
+                | ConstraintCategory::ClosureUpvar(_) => 2,
+                // Give assignments a lower priority when flagged as less likely to be interesting.
+                // In particular, de-prioritize MIR assignments lowered from argument patterns.
+                ConstraintCategory::Assignment { has_interesting_ty: false } => 3,
+                // We handle predicates and opaque types specially; don't prioritize them here.
+                ConstraintCategory::Predicate(_) | ConstraintCategory::OpaqueType => 4,
+                // `Boring` constraints can correspond to user-written code and have useful spans,
+                // but don't provide any other useful information for diagnostics.
+                ConstraintCategory::Boring => 5,
+                // `BoringNoLocation` constraints can point to user-written code, but are less
+                // specific, and are not used for relations that would make sense to blame.
+                ConstraintCategory::BoringNoLocation => 6,
+                // Do not blame internal constraints.
+                ConstraintCategory::Internal => 7,
+                ConstraintCategory::IllegalUniverse => 8,
+            }
         };
 
         let best_choice = if blame_source {
-            path.iter().rposition(interesting_to_blame)
+            path.iter().enumerate().rev().min_by_key(|(_, c)| constraint_interest(c)).unwrap().0
         } else {
-            path.iter().position(interesting_to_blame)
+            path.iter().enumerate().min_by_key(|(_, c)| constraint_interest(c)).unwrap().0
         };
 
         debug!(?best_choice, ?blame_source);
 
-        let best_constraint = match best_choice {
-            Some(i)
-                if let Some(next) = path.get(i + 1)
-                    && matches!(path[i].category, ConstraintCategory::Return(_))
-                    && next.category == ConstraintCategory::OpaqueType =>
-            {
-                // The return expression is being influenced by the return type being
-                // impl Trait, point at the return type and not the return expr.
-                *next
-            }
-
-            Some(i)
-                if path[i].category == ConstraintCategory::Return(ReturnConstraint::Normal)
-                    && let Some(field) = path.iter().find_map(|p| {
-                        if let ConstraintCategory::ClosureUpvar(f) = p.category {
-                            Some(f)
-                        } else {
-                            None
-                        }
-                    }) =>
-            {
-                OutlivesConstraint {
-                    category: ConstraintCategory::Return(ReturnConstraint::ClosureUpvar(field)),
-                    ..path[i]
-                }
-            }
-
-            Some(_)
-                if target_region == self.universal_regions().fr_static
-                    && let Some(old_best) = path.iter().min_by_key(|p| p.category)
-                    && matches!(old_best.category, ConstraintCategory::Cast {
-                        is_implicit_coercion: true,
-                        unsize_to: Some(_)
-                    }) =>
-            {
-                // FIXME(dianne): This is a hack in order to emit the subdiagnostic
-                // `BorrowExplanation::add_object_lifetime_default_note` more often, e.g. on
-                // `tests/ui/traits/trait-object-lifetime-default-note.rs`. The subdiagnostic
-                // depends on a coercion being blamed, so we fall back to an earlier version of this
-                // function's blaming logic to keep the test result the same. A proper fix will
-                // require rewriting the subdiagnostic not to rely on a coercion being blamed.
-                // For examples of where notes are missing, see #131008 and
-                // `tests/ui/suggestions/impl-on-dyn-trait-with-implicit-static-bound-needing-more-suggestions.rs`.
-                // As part of fixing those, this case should be removed.
-                *old_best
-            }
-
-            Some(i) => path[i],
-
-            None => {
-                // If that search fails, the only constraints on the path are those that we try not
-                // to blame. In that case, find what appears to be the most interesting point to
-                // report to the user via an even more ad-hoc guess.
-                *path.iter().min_by_key(|p| p.category).unwrap()
+        let best_constraint = if let Some(next) = path.get(best_choice + 1)
+            && matches!(path[best_choice].category, ConstraintCategory::Return(_))
+            && next.category == ConstraintCategory::OpaqueType
+        {
+            // The return expression is being influenced by the return type being
+            // impl Trait, point at the return type and not the return expr.
+            *next
+        } else if path[best_choice].category == ConstraintCategory::Return(ReturnConstraint::Normal)
+            && let Some(field) = path.iter().find_map(|p| {
+                if let ConstraintCategory::ClosureUpvar(f) = p.category { Some(f) } else { None }
+            })
+        {
+            OutlivesConstraint {
+                category: ConstraintCategory::Return(ReturnConstraint::ClosureUpvar(field)),
+                ..path[best_choice]
             }
+        } else {
+            path[best_choice]
         };
 
         let blame_constraint = BlameConstraint {

compiler/rustc_middle/Cargo.toml

@@ -6,7 +6,6 @@ edition = "2021"
 [dependencies]
 # tidy-alphabetical-start
 bitflags = "2.4.1"
-derive-where = "1.2.7"
 either = "1.5.0"
 field-offset = "0.3.5"
 gsgdt = "0.1.2"

compiler/rustc_middle/src/mir/query.rs

@@ -3,7 +3,6 @@
 use std::cell::Cell;
 use std::fmt::{self, Debug};
 
-use derive_where::derive_where;
 use rustc_abi::{FieldIdx, VariantIdx};
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_errors::ErrorGuaranteed;
@@ -225,7 +224,6 @@ rustc_data_structures::static_assert_size!(ConstraintCategory<'_>, 16);
 /// See also `rustc_const_eval::borrow_check::constraints`.
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 #[derive(TyEncodable, TyDecodable, HashStable, TypeVisitable, TypeFoldable)]
-#[derive_where(PartialOrd, Ord)]
 pub enum ConstraintCategory<'tcx> {
     Return(ReturnConstraint),
     Yield,
@@ -237,12 +235,11 @@ pub enum ConstraintCategory<'tcx> {
         is_implicit_coercion: bool,
         /// Whether this is an unsizing coercion and if yes, this contains the target type.
         /// Region variables are erased to ReErased.
-        #[derive_where(skip)]
         unsize_to: Option<Ty<'tcx>>,
     },
 
     /// Contains the function type if available.
-    CallArgument(#[derive_where(skip)] Option<Ty<'tcx>>),
+    CallArgument(Option<Ty<'tcx>>),
     CopyBound,
     SizedBound,
     Assignment {
@@ -276,7 +273,7 @@ pub enum ConstraintCategory<'tcx> {
     IllegalUniverse,
 }
 
-#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash)]
+#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash)]
 #[derive(TyEncodable, TyDecodable, HashStable, TypeVisitable, TypeFoldable)]
 pub enum ReturnConstraint {
     Normal,

tests/ui/cast/ptr-to-trait-obj-different-regions-lt-ext.stderr

@@ -4,7 +4,7 @@ error: lifetime may not live long enough
 LL | fn bad_cast<'a>(x: *const dyn Static<'static>) -> *const dyn Static<'a> {
    |             -- lifetime `'a` defined here
 LL |     x as _
-   |     ^^^^^^ cast requires that `'a` must outlive `'static`
+   |     ^^^^^^ returning this value requires that `'a` must outlive `'static`
 
 error: aborting due to 1 previous error
 

tests/ui/cast/ptr-to-trait-obj-different-regions-misc.stderr

@@ -6,9 +6,12 @@ LL | fn change_lt<'a, 'b>(x: *mut dyn Trait<'a>) -> *mut dyn Trait<'b> {
    |              |
    |              lifetime `'a` defined here
 LL |     x as _
-   |     ^^^^^^ cast requires that `'b` must outlive `'a`
+   |     ^^^^^^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'b`
    |
    = help: consider adding the following bound: `'b: 'a`
+   = note: requirement occurs because of a mutable pointer to `dyn Trait<'_>`
+   = note: mutable pointers are invariant over their type parameter
+   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
 
 error: lifetime may not live long enough
   --> $DIR/ptr-to-trait-obj-different-regions-misc.rs:6:5
@@ -35,9 +38,12 @@ LL | fn change_lt_ab<'a: 'b, 'b>(x: *mut dyn Trait<'a>) -> *mut dyn Trait<'b> {
    |                 |
    |                 lifetime `'a` defined here
 LL |     x as _
-   |     ^^^^^^ cast requires that `'b` must outlive `'a`
+   |     ^^^^^^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'b`
    |
    = help: consider adding the following bound: `'b: 'a`
+   = note: requirement occurs because of a mutable pointer to `dyn Trait<'_>`
+   = note: mutable pointers are invariant over their type parameter
+   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
 
 error: lifetime may not live long enough
   --> $DIR/ptr-to-trait-obj-different-regions-misc.rs:15:5
@@ -85,9 +91,12 @@ LL | fn change_assoc_0<'a, 'b>(
    |                   lifetime `'a` defined here
 ...
 LL |     x as _
-   |     ^^^^^^ cast requires that `'b` must outlive `'a`
+   |     ^^^^^^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'b`
    |
    = help: consider adding the following bound: `'b: 'a`
+   = note: requirement occurs because of a mutable pointer to `dyn Assocked<Assoc = dyn Send>`
+   = note: mutable pointers are invariant over their type parameter
+   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
 
 error: lifetime may not live long enough
   --> $DIR/ptr-to-trait-obj-different-regions-misc.rs:31:5
@@ -118,9 +127,12 @@ LL | fn change_assoc_1<'a, 'b>(
    |                   lifetime `'a` defined here
 ...
 LL |     x as _
-   |     ^^^^^^ cast requires that `'b` must outlive `'a`
+   |     ^^^^^^ function was supposed to return data with lifetime `'a` but it is returning data with lifetime `'b`
    |
    = help: consider adding the following bound: `'b: 'a`
+   = note: requirement occurs because of a mutable pointer to `dyn Assocked<Assoc = dyn Trait<'_>>`
+   = note: mutable pointers are invariant over their type parameter
+   = help: see <https://doc.rust-lang.org/nomicon/subtyping.html> for more information about variance
 
 error: lifetime may not live long enough
   --> $DIR/ptr-to-trait-obj-different-regions-misc.rs:38:5

tests/ui/fn/fn_def_coercion.rs

@@ -46,8 +46,8 @@ fn j<'a, 'b, 'c: 'a + 'b>(a: &'a (), b: &'b (), c: &'c ()) {
 
 fn k<'a, 'b, 'c: 'a + 'b>(a: &'a (), b: &'b (), c: &'c ()) {
     let x = match true {
-        true => foo::<&'c ()>,
-        false => foo::<&'a ()>, //~ ERROR lifetime may not live long enough
+        true => foo::<&'c ()>, //~ ERROR lifetime may not live long enough
+        false => foo::<&'a ()>,
     };
 
     x(a);