Auto merge of rust-lang#97778 - compiler-errors:misc-diagnostics-tidy, r=cjgillot

Tidy up miscellaneous bounds suggestions

Just some small fixes to suggestions

- Generalizes `Ty::is_suggestable` into a `TypeVisitor`, so that it can be called on things other than `Ty`
- Makes `impl Trait` in arg position no longer suggestible (generalizing the fix in rust-lang#97640)
- Fixes `impl Trait` not being replaced with fresh type param when it's deeply nested in function signature (fixes rust-lang#97760)
- Fixes some poor handling of `where` clauses with no predicates (also rust-lang#97760)
- Uses `InferCtxt::resolve_numeric_literals_with_default` so we suggest `i32` instead of `{integer}` (fixes rust-lang#97677)

Sorry there aren't many tests the fixes. Most of them would just be duplicates of other tests with empty `where` clauses or `impl Trait` in arg position instead of generic params. Let me know if you'd want more test coverage.

Author: bors

compiler/rustc_ast_lowering/src/item.rs

@@ -1377,7 +1377,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
 
         let mut params: SmallVec<[hir::GenericParam<'hir>; 4]> =
             self.lower_generic_params_mut(&generics.params).collect();
-        let has_where_clause = !generics.where_clause.predicates.is_empty();
+        let has_where_clause_predicates = !generics.where_clause.predicates.is_empty();
         let where_clause_span = self.lower_span(generics.where_clause.span);
         let span = self.lower_span(generics.span);
         let res = f(self);
@@ -1395,7 +1395,7 @@ impl<'hir> LoweringContext<'_, 'hir> {
         let lowered_generics = self.arena.alloc(hir::Generics {
             params: self.arena.alloc_from_iter(params),
             predicates: self.arena.alloc_from_iter(predicates),
-            has_where_clause,
+            has_where_clause_predicates,
             where_clause_span,
             span,
         });

compiler/rustc_ast_lowering/src/lib.rs

@@ -1315,7 +1315,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                 generics: self.arena.alloc(hir::Generics {
                     params: lifetime_defs,
                     predicates: &[],
-                    has_where_clause: false,
+                    has_where_clause_predicates: false,
                     where_clause_span: lctx.lower_span(span),
                     span: lctx.lower_span(span),
                 }),
@@ -1637,7 +1637,7 @@ impl<'a, 'hir> LoweringContext<'a, 'hir> {
                 generics: this.arena.alloc(hir::Generics {
                     params: generic_params,
                     predicates: &[],
-                    has_where_clause: false,
+                    has_where_clause_predicates: false,
                     where_clause_span: this.lower_span(span),
                     span: this.lower_span(span),
                 }),

compiler/rustc_hir/src/hir.rs

@@ -535,7 +535,7 @@ pub struct GenericParamCount {
 pub struct Generics<'hir> {
     pub params: &'hir [GenericParam<'hir>],
     pub predicates: &'hir [WherePredicate<'hir>],
-    pub has_where_clause: bool,
+    pub has_where_clause_predicates: bool,
     pub where_clause_span: Span,
     pub span: Span,
 }
@@ -545,7 +545,7 @@ impl<'hir> Generics<'hir> {
         const NOPE: Generics<'_> = Generics {
             params: &[],
             predicates: &[],
-            has_where_clause: false,
+            has_where_clause_predicates: false,
             where_clause_span: DUMMY_SP,
             span: DUMMY_SP,
         };
@@ -581,21 +581,11 @@ impl<'hir> Generics<'hir> {
         }
     }
 
-    pub fn where_clause_span(&self) -> Option<Span> {
-        if self.predicates.is_empty() { None } else { Some(self.where_clause_span) }
-    }
-
-    /// The `where_span` under normal circumstances points at either the predicates or the empty
-    /// space where the `where` clause should be. Only of use for diagnostic suggestions.
-    pub fn span_for_predicates_or_empty_place(&self) -> Span {
-        self.where_clause_span
-    }
-
     /// `Span` where further predicates would be suggested, accounting for trailing commas, like
     ///  in `fn foo<T>(t: T) where T: Foo,` so we don't suggest two trailing commas.
     pub fn tail_span_for_predicate_suggestion(&self) -> Span {
-        let end = self.span_for_predicates_or_empty_place().shrink_to_hi();
-        if self.has_where_clause {
+        let end = self.where_clause_span.shrink_to_hi();
+        if self.has_where_clause_predicates {
             self.predicates
                 .iter()
                 .filter(|p| p.in_where_clause())
@@ -608,6 +598,17 @@ impl<'hir> Generics<'hir> {
         }
     }
 
+    pub fn add_where_or_trailing_comma(&self) -> &'static str {
+        if self.has_where_clause_predicates {
+            ","
+        } else if self.where_clause_span.is_empty() {
+            " where"
+        } else {
+            // No where clause predicates, but we have `where` token
+            ""
+        }
+    }
+
     pub fn bounds_for_param(
         &self,
         param_def_id: LocalDefId,

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

@@ -2509,11 +2509,7 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                     labeled_user_string
                 );
                 let pred = format!("{}: {}", bound_kind, sub);
-                let suggestion = format!(
-                    "{} {}",
-                    if !generics.predicates.is_empty() { "," } else { " where" },
-                    pred,
-                );
+                let suggestion = format!("{} {}", generics.add_where_or_trailing_comma(), pred,);
                 err.span_suggestion(
                     generics.tail_span_for_predicate_suggestion(),
                     "consider adding a where clause",

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

@@ -367,17 +367,12 @@ impl<'a, 'tcx> InferCtxt<'a, 'tcx> {
                     .collect();
 
                 if !clauses.is_empty() {
-                    let where_clause_span = self
-                        .tcx
-                        .hir()
-                        .get_generics(impl_item_def_id)
-                        .unwrap()
-                        .where_clause_span
-                        .shrink_to_hi();
+                    let generics = self.tcx.hir().get_generics(impl_item_def_id).unwrap();
+                    let where_clause_span = generics.tail_span_for_predicate_suggestion();
 
                     let suggestion = format!(
                         "{} {}",
-                        if !impl_predicates.is_empty() { "," } else { " where" },
+                        generics.add_where_or_trailing_comma(),
                         clauses.join(", "),
                     );
                     err.span_suggestion(

compiler/rustc_lint/src/builtin.rs

@@ -2293,10 +2293,9 @@ impl<'tcx> LateLintPass<'tcx> for ExplicitOutlivesRequirements {
 
             // If all predicates are inferable, drop the entire clause
             // (including the `where`)
-            if hir_generics.has_where_clause && dropped_predicate_count == num_predicates {
-                let where_span = hir_generics
-                    .where_clause_span()
-                    .expect("span of (nonempty) where clause should exist");
+            if hir_generics.has_where_clause_predicates && dropped_predicate_count == num_predicates
+            {
+                let where_span = hir_generics.where_clause_span;
                 // Extend the where clause back to the closing `>` of the
                 // generics, except for tuple struct, which have the `where`
                 // after the fields of the struct.

compiler/rustc_middle/src/ty/diagnostics.rs

@@ -1,9 +1,10 @@
 //! Diagnostics related methods for `Ty`.
 
-use crate::ty::subst::{GenericArg, GenericArgKind};
+use std::ops::ControlFlow;
+
 use crate::ty::{
-    ConstKind, DefIdTree, ExistentialPredicate, ExistentialProjection, ExistentialTraitRef,
-    InferTy, ProjectionTy, Term, Ty, TyCtxt, TypeAndMut,
+    fold::TypeFoldable, Const, ConstKind, DefIdTree, ExistentialPredicate, InferTy,
+    PolyTraitPredicate, Ty, TyCtxt, TypeSuperFoldable, TypeVisitor,
 };
 
 use rustc_data_structures::fx::FxHashMap;
@@ -72,103 +73,55 @@ impl<'tcx> Ty<'tcx> {
             _ => self.is_simple_ty(),
         }
     }
+}
 
-    /// Whether the type can be safely suggested during error recovery.
-    pub fn is_suggestable(self, tcx: TyCtxt<'tcx>) -> bool {
-        fn generic_arg_is_suggestible<'tcx>(arg: GenericArg<'tcx>, tcx: TyCtxt<'tcx>) -> bool {
-            match arg.unpack() {
-                GenericArgKind::Type(ty) => ty.is_suggestable(tcx),
-                GenericArgKind::Const(c) => const_is_suggestable(c.val()),
-                _ => true,
-            }
-        }
-
-        fn const_is_suggestable(kind: ConstKind<'_>) -> bool {
-            match kind {
-                ConstKind::Infer(..)
-                | ConstKind::Bound(..)
-                | ConstKind::Placeholder(..)
-                | ConstKind::Error(..) => false,
-                _ => true,
-            }
-        }
-
-        // FIXME(compiler-errors): Some types are still not good to suggest,
-        // specifically references with lifetimes within the function. Not
-        //sure we have enough information to resolve whether a region is
-        // temporary, so I'll leave this as a fixme.
+pub trait IsSuggestable<'tcx> {
+    /// Whether this makes sense to suggest in a diagnostic.
+    ///
+    /// We filter out certain types and constants since they don't provide
+    /// meaningful rendered suggestions when pretty-printed. We leave some
+    /// nonsense, such as region vars, since those render as `'_` and are
+    /// usually okay to reinterpret as elided lifetimes.
+    fn is_suggestable(self, tcx: TyCtxt<'tcx>) -> bool;
+}
 
-        match self.kind() {
-            FnDef(..)
-            | Closure(..)
-            | Infer(..)
-            | Generator(..)
-            | GeneratorWitness(..)
-            | Bound(_, _)
-            | Placeholder(_)
-            | Error(_) => false,
-            Opaque(did, substs) => {
-                let parent = tcx.parent(*did);
-                if let hir::def::DefKind::TyAlias | hir::def::DefKind::AssocTy = tcx.def_kind(parent)
-                    && let Opaque(parent_did, _) = tcx.type_of(parent).kind()
-                    && parent_did == did
-                {
-                    substs.iter().all(|a| generic_arg_is_suggestible(a, tcx))
-                } else {
-                    false
-                }
-            }
-            Dynamic(dty, _) => dty.iter().all(|pred| match pred.skip_binder() {
-                ExistentialPredicate::Trait(ExistentialTraitRef { substs, .. }) => {
-                    substs.iter().all(|a| generic_arg_is_suggestible(a, tcx))
-                }
-                ExistentialPredicate::Projection(ExistentialProjection {
-                    substs, term, ..
-                }) => {
-                    let term_is_suggestable = match term {
-                        Term::Ty(ty) => ty.is_suggestable(tcx),
-                        Term::Const(c) => const_is_suggestable(c.val()),
-                    };
-                    term_is_suggestable && substs.iter().all(|a| generic_arg_is_suggestible(a, tcx))
-                }
-                _ => true,
-            }),
-            Projection(ProjectionTy { substs: args, .. }) | Adt(_, args) => {
-                args.iter().all(|a| generic_arg_is_suggestible(a, tcx))
-            }
-            Tuple(args) => args.iter().all(|ty| ty.is_suggestable(tcx)),
-            Slice(ty) | RawPtr(TypeAndMut { ty, .. }) | Ref(_, ty, _) => ty.is_suggestable(tcx),
-            Array(ty, c) => ty.is_suggestable(tcx) && const_is_suggestable(c.val()),
-            _ => true,
-        }
+impl<'tcx, T> IsSuggestable<'tcx> for T
+where
+    T: TypeFoldable<'tcx>,
+{
+    fn is_suggestable(self, tcx: TyCtxt<'tcx>) -> bool {
+        self.visit_with(&mut IsSuggestableVisitor { tcx }).is_continue()
     }
 }
 
-pub fn suggest_arbitrary_trait_bound(
+pub fn suggest_arbitrary_trait_bound<'tcx>(
+    tcx: TyCtxt<'tcx>,
     generics: &hir::Generics<'_>,
     err: &mut Diagnostic,
-    param_name: &str,
-    constraint: &str,
+    trait_pred: PolyTraitPredicate<'tcx>,
 ) -> bool {
+    if !trait_pred.is_suggestable(tcx) {
+        return false;
+    }
+
+    let param_name = trait_pred.skip_binder().self_ty().to_string();
+    let constraint = trait_pred.print_modifiers_and_trait_path().to_string();
     let param = generics.params.iter().find(|p| p.name.ident().as_str() == param_name);
-    match (param, param_name) {
-        (Some(_), "Self") => return false,
-        _ => {}
+
+    // Skip, there is a param named Self
+    if param.is_some() && param_name == "Self" {
+        return false;
     }
+
     // Suggest a where clause bound for a non-type parameter.
-    let (action, prefix) = if generics.has_where_clause {
-        ("extending the", ", ")
-    } else {
-        ("introducing a", " where ")
-    };
     err.span_suggestion_verbose(
         generics.tail_span_for_predicate_suggestion(),
         &format!(
-            "consider {} `where` bound, but there might be an alternative better way to express \
+            "consider {} `where` clause, but there might be an alternative better way to express \
              this requirement",
-            action,
+            if generics.where_clause_span.is_empty() { "introducing a" } else { "extending the" },
         ),
-        format!("{}{}: {}", prefix, param_name, constraint),
+        format!("{} {}: {}", generics.add_where_or_trailing_comma(), param_name, constraint),
         Applicability::MaybeIncorrect,
     );
     true
@@ -321,7 +274,7 @@ pub fn suggest_constraining_type_params<'a>(
             continue;
         }
 
-        if generics.has_where_clause {
+        if generics.has_where_clause_predicates {
             // This part is a bit tricky, because using the `where` clause user can
             // provide zero, one or many bounds for the same type parameter, so we
             // have following cases to consider:
@@ -463,3 +416,78 @@ impl<'v> hir::intravisit::Visitor<'v> for StaticLifetimeVisitor<'v> {
         }
     }
 }
+
+pub struct IsSuggestableVisitor<'tcx> {
+    tcx: TyCtxt<'tcx>,
+}
+
+impl<'tcx> TypeVisitor<'tcx> for IsSuggestableVisitor<'tcx> {
+    type BreakTy = ();
+
+    fn visit_ty(&mut self, t: Ty<'tcx>) -> ControlFlow<Self::BreakTy> {
+        match t.kind() {
+            FnDef(..)
+            | Closure(..)
+            | Infer(..)
+            | Generator(..)
+            | GeneratorWitness(..)
+            | Bound(_, _)
+            | Placeholder(_)
+            | Error(_) => {
+                return ControlFlow::Break(());
+            }
+
+            Opaque(did, _) => {
+                let parent = self.tcx.parent(*did);
+                if let hir::def::DefKind::TyAlias | hir::def::DefKind::AssocTy = self.tcx.def_kind(parent)
+                    && let Opaque(parent_did, _) = self.tcx.type_of(parent).kind()
+                    && parent_did == did
+                {
+                    // Okay
+                } else {
+                    return ControlFlow::Break(());
+                }
+            }
+
+            Dynamic(dty, _) => {
+                for pred in *dty {
+                    match pred.skip_binder() {
+                        ExistentialPredicate::Trait(_) | ExistentialPredicate::Projection(_) => {
+                            // Okay
+                        }
+                        _ => return ControlFlow::Break(()),
+                    }
+                }
+            }
+
+            Param(param) => {
+                // FIXME: It would be nice to make this not use string manipulation,
+                // but it's pretty hard to do this, since `ty::ParamTy` is missing
+                // sufficient info to determine if it is synthetic, and we don't
+                // always have a convenient way of getting `ty::Generics` at the call
+                // sites we invoke `IsSuggestable::is_suggestable`.
+                if param.name.as_str().starts_with("impl ") {
+                    return ControlFlow::Break(());
+                }
+            }
+
+            _ => {}
+        }
+
+        t.super_visit_with(self)
+    }
+
+    fn visit_const(&mut self, c: Const<'tcx>) -> ControlFlow<Self::BreakTy> {
+        match c.val() {
+            ConstKind::Infer(..)
+            | ConstKind::Bound(..)
+            | ConstKind::Placeholder(..)
+            | ConstKind::Error(..) => {
+                return ControlFlow::Break(());
+            }
+            _ => {}
+        }
+
+        c.super_visit_with(self)
+    }
+}

compiler/rustc_middle/src/ty/generics.rs

@@ -39,6 +39,13 @@ impl GenericParamDefKind {
             GenericParamDefKind::Type { .. } | GenericParamDefKind::Const { .. } => true,
         }
     }
+
+    pub fn is_synthetic(&self) -> bool {
+        match self {
+            GenericParamDefKind::Type { synthetic, .. } => *synthetic,
+            _ => false,
+        }
+    }
 }
 
 #[derive(Clone, Debug, TyEncodable, TyDecodable, HashStable)]