Auto merge of rust-lang#87244 - jackh726:issue-71883, r=estebank

Better diagnostics with mismatched types due to implicit static lifetime

Fixes rust-lang#78113

I think this is my first diagnostics PR...definitely happy to hear thoughts on the direction/implementation here.

I was originally just trying to solve the error above, where the lifetime on a GAT was causing a cryptic "mismatched types" error. But as I was writing this, I realized that this (unintentionally) also applied to a different case: `wf-in-foreign-fn-decls-issue-80468.rs`. I'm not sure if this diagnostic should get a new error code, or even reuse an existing one. And, there might be some ways to make this even more generalized. Also, the error is a bit more lengthy and verbose than probably needed. So thoughts there are welcome too.

This PR essentially ended up adding a new nice region error pass that triggers if a type doesn't match the self type of an impl which is selected because of a predicate because of an implicit static bound on that self type.

r? `@estebank`

Author: bors

compiler/rustc_ast_lowering/src/path.rs

@@ -336,6 +336,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                             insertion_sp,
                             suggestion,
                         );
+                        err.note("assuming a `'static` lifetime...");
                         err.emit();
                     }
                     AnonymousLifetimeMode::PassThrough | AnonymousLifetimeMode::ReportError => {

compiler/rustc_infer/src/infer/error_reporting/mod.rs

@@ -1590,17 +1590,13 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
             }
         };
         if let Some((expected, found)) = expected_found {
-            let expected_label = match exp_found {
-                Mismatch::Variable(ef) => ef.expected.prefix_string(self.tcx),
-                Mismatch::Fixed(s) => s.into(),
-            };
-            let found_label = match exp_found {
-                Mismatch::Variable(ef) => ef.found.prefix_string(self.tcx),
-                Mismatch::Fixed(s) => s.into(),
-            };
-            let exp_found = match exp_found {
-                Mismatch::Variable(exp_found) => Some(exp_found),
-                Mismatch::Fixed(_) => None,
+            let (expected_label, found_label, exp_found) = match exp_found {
+                Mismatch::Variable(ef) => (
+                    ef.expected.prefix_string(self.tcx),
+                    ef.found.prefix_string(self.tcx),
+                    Some(ef),
+                ),
+                Mismatch::Fixed(s) => (s.into(), s.into(), None),
             };
             match (&terr, expected == found) {
                 (TypeError::Sorts(values), extra) => {

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/mismatched_static_lifetime.rs

@@ -0,0 +1,103 @@
+//! Error Reporting for when the lifetime for a type doesn't match the `impl` selected for a predicate
+//! to hold.
+
+use crate::infer::error_reporting::nice_region_error::NiceRegionError;
+use crate::infer::error_reporting::note_and_explain_region;
+use crate::infer::lexical_region_resolve::RegionResolutionError;
+use crate::infer::{SubregionOrigin, TypeTrace};
+use crate::traits::ObligationCauseCode;
+use rustc_data_structures::stable_set::FxHashSet;
+use rustc_errors::{Applicability, ErrorReported};
+use rustc_hir as hir;
+use rustc_hir::intravisit::Visitor;
+use rustc_middle::ty::{self, TypeVisitor};
+use rustc_span::MultiSpan;
+
+impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
+    pub(super) fn try_report_mismatched_static_lifetime(&self) -> Option<ErrorReported> {
+        let error = self.error.as_ref()?;
+        debug!("try_report_mismatched_static_lifetime {:?}", error);
+
+        let (origin, sub, sup) = match error.clone() {
+            RegionResolutionError::ConcreteFailure(origin, sub, sup) => (origin, sub, sup),
+            _ => return None,
+        };
+        if *sub != ty::RegionKind::ReStatic {
+            return None;
+        }
+        let cause = match origin {
+            SubregionOrigin::Subtype(box TypeTrace { ref cause, .. }) => cause,
+            _ => return None,
+        };
+        let (parent, impl_def_id) = match &cause.code {
+            ObligationCauseCode::MatchImpl(parent, impl_def_id) => (parent, impl_def_id),
+            _ => return None,
+        };
+        let binding_span = match **parent {
+            ObligationCauseCode::BindingObligation(_def_id, binding_span) => binding_span,
+            _ => return None,
+        };
+        let mut err = self.tcx().sess.struct_span_err(cause.span, "incompatible lifetime on type");
+        // FIXME: we should point at the lifetime
+        let mut multi_span: MultiSpan = vec![binding_span].into();
+        multi_span
+            .push_span_label(binding_span, "introduces a `'static` lifetime requirement".into());
+        err.span_note(multi_span, "because this has an unmet lifetime requirement");
+        note_and_explain_region(self.tcx(), &mut err, "", sup, "...");
+        if let Some(impl_node) = self.tcx().hir().get_if_local(*impl_def_id) {
+            // If an impl is local, then maybe this isn't what they want. Try to
+            // be as helpful as possible with implicit lifetimes.
+
+            // First, let's get the hir self type of the impl
+            let impl_self_ty = match impl_node {
+                hir::Node::Item(hir::Item {
+                    kind: hir::ItemKind::Impl(hir::Impl { self_ty, .. }),
+                    ..
+                }) => self_ty,
+                _ => bug!("Node not an impl."),
+            };
+
+            // Next, let's figure out the set of trait objects with implict static bounds
+            let ty = self.tcx().type_of(*impl_def_id);
+            let mut v = super::static_impl_trait::TraitObjectVisitor(FxHashSet::default());
+            v.visit_ty(ty);
+            let mut traits = vec![];
+            for matching_def_id in v.0 {
+                let mut hir_v =
+                    super::static_impl_trait::HirTraitObjectVisitor(&mut traits, matching_def_id);
+                hir_v.visit_ty(&impl_self_ty);
+            }
+
+            if traits.is_empty() {
+                // If there are no trait object traits to point at, either because
+                // there aren't trait objects or because none are implicit, then just
+                // write a single note on the impl itself.
+
+                let impl_span = self.tcx().def_span(*impl_def_id);
+                err.span_note(impl_span, "...does not necessarily outlive the static lifetime introduced by the compatible `impl`");
+            } else {
+                // Otherwise, point at all implicit static lifetimes
+
+                err.note("...does not necessarily outlive the static lifetime introduced by the compatible `impl`");
+                for span in &traits {
+                    err.span_note(*span, "this has an implicit `'static` lifetime requirement");
+                    // It would be nice to put this immediately under the above note, but they get
+                    // pushed to the end.
+                    err.span_suggestion_verbose(
+                        span.shrink_to_hi(),
+                        "consider relaxing the implicit `'static` requirement",
+                        " + '_".to_string(),
+                        Applicability::MaybeIncorrect,
+                    );
+                }
+            }
+        } else {
+            // Otherwise just point out the impl.
+
+            let impl_span = self.tcx().def_span(*impl_def_id);
+            err.span_note(impl_span, "...does not necessarily outlive the static lifetime introduced by the compatible `impl`");
+        }
+        err.emit();
+        Some(ErrorReported)
+    }
+}

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/mod.rs

@@ -7,6 +7,7 @@ use rustc_span::source_map::Span;
 
 mod different_lifetimes;
 pub mod find_anon_type;
+mod mismatched_static_lifetime;
 mod named_anon_conflict;
 mod placeholder_error;
 mod static_impl_trait;
@@ -58,6 +59,7 @@ impl<'cx, 'tcx> NiceRegionError<'cx, 'tcx> {
             .or_else(|| self.try_report_impl_not_conforming_to_trait())
             .or_else(|| self.try_report_anon_anon_conflict())
             .or_else(|| self.try_report_static_impl_trait())
+            .or_else(|| self.try_report_mismatched_static_lifetime())
     }
 
     pub fn regions(&self) -> Option<(Span, ty::Region<'tcx>, ty::Region<'tcx>)> {

compiler/rustc_infer/src/infer/error_reporting/nice_region_error/static_impl_trait.rs

@@ -4,6 +4,7 @@ use crate::infer::error_reporting::nice_region_error::NiceRegionError;
 use crate::infer::lexical_region_resolve::RegionResolutionError;
 use crate::infer::{SubregionOrigin, TypeTrace};
 use crate::traits::{ObligationCauseCode, UnifyReceiverContext};
+use rustc_data_structures::stable_set::FxHashSet;
 use rustc_errors::{struct_span_err, Applicability, DiagnosticBuilder, ErrorReported};
 use rustc_hir::def_id::DefId;
 use rustc_hir::intravisit::{walk_ty, ErasedMap, NestedVisitorMap, Visitor};
@@ -185,17 +186,20 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
             }
         }
         if let SubregionOrigin::Subtype(box TypeTrace { cause, .. }) = &sub_origin {
-            if let ObligationCauseCode::ItemObligation(item_def_id) = cause.code {
+            let code = match &cause.code {
+                ObligationCauseCode::MatchImpl(parent, ..) => &**parent,
+                _ => &cause.code,
+            };
+            if let ObligationCauseCode::ItemObligation(item_def_id) = *code {
                 // Same case of `impl Foo for dyn Bar { fn qux(&self) {} }` introducing a `'static`
                 // lifetime as above, but called using a fully-qualified path to the method:
                 // `Foo::qux(bar)`.
-                let mut v = TraitObjectVisitor(vec![]);
+                let mut v = TraitObjectVisitor(FxHashSet::default());
                 v.visit_ty(param.param_ty);
                 if let Some((ident, self_ty)) =
-                    self.get_impl_ident_and_self_ty_from_trait(item_def_id, &v.0[..])
+                    self.get_impl_ident_and_self_ty_from_trait(item_def_id, &v.0)
                 {
-                    if self.suggest_constrain_dyn_trait_in_impl(&mut err, &v.0[..], ident, self_ty)
-                    {
+                    if self.suggest_constrain_dyn_trait_in_impl(&mut err, &v.0, ident, self_ty) {
                         override_error_code = Some(ident);
                     }
                 }
@@ -336,7 +340,7 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
     fn get_impl_ident_and_self_ty_from_trait(
         &self,
         def_id: DefId,
-        trait_objects: &[DefId],
+        trait_objects: &FxHashSet<DefId>,
     ) -> Option<(Ident, &'tcx hir::Ty<'tcx>)> {
         let tcx = self.tcx();
         match tcx.hir().get_if_local(def_id) {
@@ -373,9 +377,10 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
                                         // multiple `impl`s for the same trait like
                                         // `impl Foo for Box<dyn Bar>` and `impl Foo for dyn Bar`.
                                         // In that case, only the first one will get suggestions.
-                                        let mut hir_v = HirTraitObjectVisitor(vec![], *did);
+                                        let mut traits = vec![];
+                                        let mut hir_v = HirTraitObjectVisitor(&mut traits, *did);
                                         hir_v.visit_ty(self_ty);
-                                        !hir_v.0.is_empty()
+                                        !traits.is_empty()
                                     }) =>
                                     {
                                         Some(self_ty)
@@ -417,33 +422,34 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
             _ => return false,
         };
 
-        let mut v = TraitObjectVisitor(vec![]);
+        let mut v = TraitObjectVisitor(FxHashSet::default());
         v.visit_ty(ty);
 
         // Get the `Ident` of the method being called and the corresponding `impl` (to point at
         // `Bar` in `impl Foo for dyn Bar {}` and the definition of the method being called).
         let (ident, self_ty) =
-            match self.get_impl_ident_and_self_ty_from_trait(instance.def_id(), &v.0[..]) {
+            match self.get_impl_ident_and_self_ty_from_trait(instance.def_id(), &v.0) {
                 Some((ident, self_ty)) => (ident, self_ty),
                 None => return false,
             };
 
         // Find the trait object types in the argument, so we point at *only* the trait object.
-        self.suggest_constrain_dyn_trait_in_impl(err, &v.0[..], ident, self_ty)
+        self.suggest_constrain_dyn_trait_in_impl(err, &v.0, ident, self_ty)
     }
 
     fn suggest_constrain_dyn_trait_in_impl(
         &self,
         err: &mut DiagnosticBuilder<'_>,
-        found_dids: &[DefId],
+        found_dids: &FxHashSet<DefId>,
         ident: Ident,
         self_ty: &hir::Ty<'_>,
     ) -> bool {
         let mut suggested = false;
         for found_did in found_dids {
-            let mut hir_v = HirTraitObjectVisitor(vec![], *found_did);
+            let mut traits = vec![];
+            let mut hir_v = HirTraitObjectVisitor(&mut traits, *found_did);
             hir_v.visit_ty(&self_ty);
-            for span in &hir_v.0 {
+            for span in &traits {
                 let mut multi_span: MultiSpan = vec![*span].into();
                 multi_span.push_span_label(
                     *span,
@@ -468,14 +474,14 @@ impl<'a, 'tcx> NiceRegionError<'a, 'tcx> {
 }
 
 /// Collect all the trait objects in a type that could have received an implicit `'static` lifetime.
-struct TraitObjectVisitor(Vec<DefId>);
+pub(super) struct TraitObjectVisitor(pub(super) FxHashSet<DefId>);
 
 impl TypeVisitor<'_> for TraitObjectVisitor {
     fn visit_ty(&mut self, t: Ty<'_>) -> ControlFlow<Self::BreakTy> {
         match t.kind() {
             ty::Dynamic(preds, RegionKind::ReStatic) => {
                 if let Some(def_id) = preds.principal_def_id() {
-                    self.0.push(def_id);
+                    self.0.insert(def_id);
                 }
                 ControlFlow::CONTINUE
             }
@@ -485,9 +491,9 @@ impl TypeVisitor<'_> for TraitObjectVisitor {
 }
 
 /// Collect all `hir::Ty<'_>` `Span`s for trait objects with an implicit lifetime.
-struct HirTraitObjectVisitor(Vec<Span>, DefId);
+pub(super) struct HirTraitObjectVisitor<'a>(pub(super) &'a mut Vec<Span>, pub(super) DefId);
 
-impl<'tcx> Visitor<'tcx> for HirTraitObjectVisitor {
+impl<'a, 'tcx> Visitor<'tcx> for HirTraitObjectVisitor<'a> {
     type Map = ErasedMap<'tcx>;
 
     fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {

compiler/rustc_middle/src/traits/mod.rs

@@ -333,6 +333,9 @@ pub enum ObligationCauseCode<'tcx> {
     /// This is purely for diagnostic purposes - it is always
     /// correct to use `MiscObligation` instead
     WellFormed(Option<hir::HirId>),
+
+    /// From `match_impl`. The cause for us having to match an impl, and the DefId we are matching against.
+    MatchImpl(Lrc<ObligationCauseCode<'tcx>>, DefId),
 }
 
 impl ObligationCauseCode<'_> {

compiler/rustc_trait_selection/src/traits/error_reporting/suggestions.rs

@@ -1903,7 +1903,8 @@ impl<'a, 'tcx> InferCtxtExt<'tcx> for InferCtxt<'a, 'tcx> {
             | ObligationCauseCode::UnifyReceiver(..)
             | ObligationCauseCode::OpaqueType
             | ObligationCauseCode::MiscObligation
-            | ObligationCauseCode::WellFormed(..) => {}
+            | ObligationCauseCode::WellFormed(..)
+            | ObligationCauseCode::MatchImpl(..) => {}
             ObligationCauseCode::SliceOrArrayElem => {
                 err.note("slice and array elements must have `Sized` type");
             }

compiler/rustc_trait_selection/src/traits/select/mod.rs

@@ -1903,9 +1903,15 @@ impl<'cx, 'tcx> SelectionContext<'cx, 'tcx> {
 
         debug!(?impl_trait_ref, ?placeholder_obligation_trait_ref);
 
+        let cause = ObligationCause::new(
+            obligation.cause.span,
+            obligation.cause.body_id,
+            ObligationCauseCode::MatchImpl(Lrc::new(obligation.cause.code.clone()), impl_def_id),
+        );
+
         let InferOk { obligations, .. } = self
             .infcx
-            .at(&obligation.cause, obligation.param_env)
+            .at(&cause, obligation.param_env)
             .eq(placeholder_obligation_trait_ref, impl_trait_ref)
             .map_err(|e| debug!("match_impl: failed eq_trait_refs due to `{}`", e))?;
         nested_obligations.extend(obligations);

src/test/ui/async-await/async-fn-path-elision.stderr

@@ -3,6 +3,8 @@ error[E0726]: implicit elided lifetime not allowed here
    |
 LL | async fn error(lt: HasLifetime) {
    |                    ^^^^^^^^^^^- help: indicate the anonymous lifetime: `<'_>`
+   |
+   = note: assuming a `'static` lifetime...
 
 error: aborting due to previous error
 

src/test/ui/generic-associated-types/issue-78113-lifetime-mismatch-dyn-trait-box.rs

@@ -0,0 +1,40 @@
+// Test for diagnostics when we have mismatched lifetime due to implict 'static lifetime in GATs
+
+// check-fail
+
+#![feature(generic_associated_types)]
+
+pub trait A {}
+impl A for &dyn A {}
+impl A for Box<dyn A> {}
+
+pub trait B {
+    type T<'a>: A;
+}
+
+impl B for () {
+    // `'a` doesn't match implicit `'static`: suggest `'_`
+    type T<'a> = Box<dyn A + 'a>; //~ incompatible lifetime on type
+}
+
+trait C {}
+impl C for Box<dyn A + 'static> {}
+pub trait D {
+    type T<'a>: C;
+}
+impl D for () {
+    // `'a` doesn't match explicit `'static`: we *should* suggest removing `'static`
+    type T<'a> = Box<dyn A + 'a>; //~ incompatible lifetime on type
+}
+
+trait E {}
+impl E for (Box<dyn A>, Box<dyn A>) {}
+pub trait F {
+    type T<'a>: E;
+}
+impl F for () {
+    // `'a` doesn't match explicit `'static`: suggest `'_`
+    type T<'a> = (Box<dyn A + 'a>, Box<dyn A + 'a>); //~ incompatible lifetime on type
+}
+
+fn main() {}