Auto merge of rust-lang#16877 - Veykril:stackoverflow, r=Veykril

fix: Fix `impl Trait<Self>` causing stackoverflows

Fixes rust-lang/rust-analyzer#15646

Author: bors

Cargo.toml

@@ -22,6 +22,7 @@ smol_str.opt-level = 3
 text-size.opt-level = 3
 # This speeds up `cargo xtask dist`.
 miniz_oxide.opt-level = 3
+salsa.opt-level = 3
 
 [profile.release]
 incremental = true

crates/hir-ty/src/display.rs

@@ -28,7 +28,7 @@ use intern::{Internable, Interned};
 use itertools::Itertools;
 use la_arena::ArenaMap;
 use smallvec::SmallVec;
-use stdx::never;
+use stdx::{never, IsNoneOr};
 use triomphe::Arc;
 
 use crate::{
@@ -41,9 +41,9 @@ use crate::{
     mir::pad16,
     primitive, to_assoc_type_id,
     utils::{self, detect_variant_from_bytes, generics, ClosureSubst},
-    AdtId, AliasEq, AliasTy, Binders, CallableDefId, CallableSig, Const, ConstScalar, ConstValue,
-    DomainGoal, FnAbi, GenericArg, GenericArgData, ImplTraitId, Interner, Lifetime, LifetimeData,
-    LifetimeOutlives, MemoryMap, Mutability, OpaqueTy, ProjectionTy, ProjectionTyExt,
+    AdtId, AliasEq, AliasTy, Binders, CallableDefId, CallableSig, ConcreteConst, Const,
+    ConstScalar, ConstValue, DomainGoal, FnAbi, GenericArg, ImplTraitId, Interner, Lifetime,
+    LifetimeData, LifetimeOutlives, MemoryMap, Mutability, OpaqueTy, ProjectionTy, ProjectionTyExt,
     QuantifiedWhereClause, Scalar, Substitution, TraitEnvironment, TraitRef, TraitRefExt, Ty,
     TyExt, WhereClause,
 };
@@ -60,11 +60,18 @@ impl HirWrite for String {}
 impl HirWrite for fmt::Formatter<'_> {}
 
 pub struct HirFormatter<'a> {
+    /// The database handle
     pub db: &'a dyn HirDatabase,
+    /// The sink to write into
     fmt: &'a mut dyn HirWrite,
+    /// A buffer to intercept writes with, this allows us to track the overall size of the formatted output.
     buf: String,
+    /// The current size of the formatted output.
     curr_size: usize,
-    pub(crate) max_size: Option<usize>,
+    /// Size from which we should truncate the output.
+    max_size: Option<usize>,
+    /// When rendering something that has a concept of "children" (like fields in a struct), this limits
+    /// how many should be rendered.
     pub entity_limit: Option<usize>,
     omit_verbose_types: bool,
     closure_style: ClosureStyle,
@@ -304,7 +311,6 @@ impl DisplayTarget {
 #[derive(Debug)]
 pub enum DisplaySourceCodeError {
     PathNotFound,
-    UnknownType,
     Coroutine,
     OpaqueType,
 }
@@ -418,6 +424,7 @@ impl HirDisplay for ProjectionTy {
         let proj_params = &self.substitution.as_slice(Interner)[..proj_params_count];
         if !proj_params.is_empty() {
             write!(f, "<")?;
+            // FIXME use `hir_fmt_generics` here
             f.write_joined(proj_params, ", ")?;
             write!(f, ">")?;
         }
@@ -967,6 +974,7 @@ impl HirDisplay for Ty {
                             .chain(fn_params)
                             .flatten();
                         write!(f, "<")?;
+                        // FIXME use `hir_fmt_generics` here
                         f.write_joined(params, ", ")?;
                         write!(f, ">")?;
                     }
@@ -1037,6 +1045,7 @@ impl HirDisplay for Ty {
                     // FIXME: reconsider the generic args order upon formatting?
                     if parameters.len(Interner) > 0 {
                         write!(f, "<")?;
+                        // FIXME use `hir_fmt_generics` here
                         f.write_joined(parameters.as_slice(Interner), ", ")?;
                         write!(f, ">")?;
                     }
@@ -1287,11 +1296,10 @@ impl HirDisplay for Ty {
             }
             TyKind::Error => {
                 if f.display_target.is_source_code() {
-                    return Err(HirDisplayError::DisplaySourceCodeError(
-                        DisplaySourceCodeError::UnknownType,
-                    ));
+                    f.write_char('_')?;
+                } else {
+                    write!(f, "{{unknown}}")?;
                 }
-                write!(f, "{{unknown}}")?;
             }
             TyKind::InferenceVar(..) => write!(f, "_")?,
             TyKind::Coroutine(_, subst) => {
@@ -1331,98 +1339,90 @@ fn hir_fmt_generics(
     parameters: &Substitution,
     generic_def: Option<hir_def::GenericDefId>,
 ) -> Result<(), HirDisplayError> {
-    let db = f.db;
-    if parameters.len(Interner) > 0 {
-        use std::cmp::Ordering;
-        let param_compare =
-            |a: &GenericArg, b: &GenericArg| match (a.data(Interner), b.data(Interner)) {
-                (crate::GenericArgData::Lifetime(_), crate::GenericArgData::Lifetime(_)) => {
-                    Ordering::Equal
-                }
-                (crate::GenericArgData::Lifetime(_), _) => Ordering::Less,
-                (_, crate::GenericArgData::Lifetime(_)) => Ordering::Less,
-                (_, _) => Ordering::Equal,
-            };
-        let parameters_to_write = if f.display_target.is_source_code() || f.omit_verbose_types() {
-            match generic_def
-                .map(|generic_def_id| db.generic_defaults(generic_def_id))
-                .filter(|defaults| !defaults.is_empty())
-            {
-                None => parameters.as_slice(Interner),
-                Some(default_parameters) => {
-                    fn should_show(
-                        parameter: &GenericArg,
-                        default_parameters: &[Binders<GenericArg>],
-                        i: usize,
-                        parameters: &Substitution,
-                    ) -> bool {
-                        if parameter.ty(Interner).map(|it| it.kind(Interner))
-                            == Some(&TyKind::Error)
-                        {
-                            return true;
-                        }
-                        if let Some(ConstValue::Concrete(c)) =
-                            parameter.constant(Interner).map(|it| &it.data(Interner).value)
-                        {
-                            if c.interned == ConstScalar::Unknown {
-                                return true;
-                            }
-                        }
-                        if let Some(crate::LifetimeData::Static | crate::LifetimeData::Error) =
-                            parameter.lifetime(Interner).map(|it| it.data(Interner))
-                        {
-                            return true;
+    if parameters.is_empty(Interner) {
+        return Ok(());
+    }
+
+    let parameters_to_write =
+        generic_args_sans_defaults(f, generic_def, parameters.as_slice(Interner));
+    if !parameters_to_write.is_empty() {
+        write!(f, "<")?;
+        hir_fmt_generic_arguments(f, parameters_to_write)?;
+        write!(f, ">")?;
+    }
+
+    Ok(())
+}
+
+fn generic_args_sans_defaults<'ga>(
+    f: &mut HirFormatter<'_>,
+    generic_def: Option<hir_def::GenericDefId>,
+    parameters: &'ga [GenericArg],
+) -> &'ga [GenericArg] {
+    if f.display_target.is_source_code() || f.omit_verbose_types() {
+        match generic_def
+            .map(|generic_def_id| f.db.generic_defaults(generic_def_id))
+            .filter(|it| !it.is_empty())
+        {
+            None => parameters,
+            Some(default_parameters) => {
+                let should_show = |arg: &GenericArg, i: usize| {
+                    let is_err = |arg: &GenericArg| match arg.data(Interner) {
+                        chalk_ir::GenericArgData::Lifetime(it) => {
+                            *it.data(Interner) == LifetimeData::Error
                         }
-                        let default_parameter = match default_parameters.get(i) {
-                            Some(it) => it,
-                            None => return true,
-                        };
-                        let actual_default =
-                            default_parameter.clone().substitute(Interner, &parameters);
-                        parameter != &actual_default
+                        chalk_ir::GenericArgData::Ty(it) => *it.kind(Interner) == TyKind::Error,
+                        chalk_ir::GenericArgData::Const(it) => matches!(
+                            it.data(Interner).value,
+                            ConstValue::Concrete(ConcreteConst {
+                                interned: ConstScalar::Unknown,
+                                ..
+                            })
+                        ),
+                    };
+                    // if the arg is error like, render it to inform the user
+                    if is_err(arg) {
+                        return true;
                     }
-                    let mut default_from = 0;
-                    for (i, parameter) in parameters.iter(Interner).enumerate() {
-                        if should_show(parameter, &default_parameters, i, parameters) {
-                            default_from = i + 1;
-                        }
+                    // otherwise, if the arg is equal to the param default, hide it (unless the
+                    // default is an error which can happen for the trait Self type)
+                    default_parameters.get(i).is_none_or(|default_parameter| {
+                        // !is_err(default_parameter.skip_binders())
+                        //     &&
+                        arg != &default_parameter.clone().substitute(Interner, &parameters)
+                    })
+                };
+                let mut default_from = 0;
+                for (i, parameter) in parameters.iter().enumerate() {
+                    if should_show(parameter, i) {
+                        default_from = i + 1;
                     }
-                    &parameters.as_slice(Interner)[0..default_from]
-                }
-            }
-        } else {
-            parameters.as_slice(Interner)
-        };
-        //FIXME: Should handle the ordering of lifetimes when creating substitutions
-        let mut parameters_to_write = parameters_to_write.to_vec();
-        parameters_to_write.sort_by(param_compare);
-        if !parameters_to_write.is_empty() {
-            write!(f, "<")?;
-            let mut first = true;
-            for generic_arg in parameters_to_write {
-                if !first {
-                    write!(f, ", ")?;
-                }
-                first = false;
-                if f.display_target.is_source_code() {
-                    match generic_arg.data(Interner) {
-                        GenericArgData::Lifetime(l)
-                            if matches!(l.data(Interner), LifetimeData::Error) =>
-                        {
-                            write!(f, "'_")
-                        }
-                        GenericArgData::Ty(t) if matches!(t.kind(Interner), TyKind::Error) => {
-                            write!(f, "_")
-                        }
-                        _ => generic_arg.hir_fmt(f),
-                    }?
-                } else {
-                    generic_arg.hir_fmt(f)?;
                 }
+                &parameters[0..default_from]
             }
+        }
+    } else {
+        parameters
+    }
+}
 
-            write!(f, ">")?;
+fn hir_fmt_generic_arguments(
+    f: &mut HirFormatter<'_>,
+    parameters: &[GenericArg],
+) -> Result<(), HirDisplayError> {
+    let mut first = true;
+    let lifetime_offset = parameters.iter().position(|arg| arg.lifetime(Interner).is_some());
+
+    let (ty_or_const, lifetimes) = match lifetime_offset {
+        Some(offset) => parameters.split_at(offset),
+        None => (parameters, &[][..]),
+    };
+    for generic_arg in lifetimes.iter().chain(ty_or_const) {
+        if !first {
+            write!(f, ", ")?;
         }
+        first = false;
+        generic_arg.hir_fmt(f)?;
     }
     Ok(())
 }
@@ -1544,20 +1544,29 @@ fn write_bounds_like_dyn_trait(
                 f.start_location_link(trait_.into());
                 write!(f, "{}", f.db.trait_data(trait_).name.display(f.db.upcast()))?;
                 f.end_location_link();
-                if let [_, params @ ..] = trait_ref.substitution.as_slice(Interner) {
-                    if is_fn_trait {
+                if is_fn_trait {
+                    if let [_self, params @ ..] = trait_ref.substitution.as_slice(Interner) {
                         if let Some(args) =
                             params.first().and_then(|it| it.assert_ty_ref(Interner).as_tuple())
                         {
                             write!(f, "(")?;
-                            f.write_joined(args.as_slice(Interner), ", ")?;
+                            hir_fmt_generic_arguments(f, args.as_slice(Interner))?;
                             write!(f, ")")?;
                         }
-                    } else if !params.is_empty() {
-                        write!(f, "<")?;
-                        f.write_joined(params, ", ")?;
-                        // there might be assoc type bindings, so we leave the angle brackets open
-                        angle_open = true;
+                    }
+                } else {
+                    let params = generic_args_sans_defaults(
+                        f,
+                        Some(trait_.into()),
+                        trait_ref.substitution.as_slice(Interner),
+                    );
+                    if let [_self, params @ ..] = params {
+                        if !params.is_empty() {
+                            write!(f, "<")?;
+                            hir_fmt_generic_arguments(f, params)?;
+                            // there might be assoc type bindings, so we leave the angle brackets open
+                            angle_open = true;
+                        }
                     }
                 }
             }
@@ -1609,9 +1618,9 @@ fn write_bounds_like_dyn_trait(
                     let proj_arg_count = generics(f.db.upcast(), assoc_ty_id.into()).len_self();
                     if proj_arg_count > 0 {
                         write!(f, "<")?;
-                        f.write_joined(
+                        hir_fmt_generic_arguments(
+                            f,
                             &proj.substitution.as_slice(Interner)[..proj_arg_count],
-                            ", ",
                         )?;
                         write!(f, ">")?;
                     }
@@ -1670,6 +1679,7 @@ fn fmt_trait_ref(
     f.end_location_link();
     if tr.substitution.len(Interner) > 1 {
         write!(f, "<")?;
+        // FIXME use `hir_fmt_generics` here
         f.write_joined(&tr.substitution.as_slice(Interner)[1..], ", ")?;
         write!(f, ">")?;
     }
@@ -1728,15 +1738,16 @@ impl HirDisplay for Lifetime {
 impl HirDisplay for LifetimeData {
     fn hir_fmt(&self, f: &mut HirFormatter<'_>) -> Result<(), HirDisplayError> {
         match self {
-            LifetimeData::BoundVar(idx) => idx.hir_fmt(f),
-            LifetimeData::InferenceVar(_) => write!(f, "_"),
             LifetimeData::Placeholder(idx) => {
                 let id = lt_from_placeholder_idx(f.db, *idx);
                 let generics = generics(f.db.upcast(), id.parent);
                 let param_data = &generics.params[id.local_id];
                 write!(f, "{}", param_data.name.display(f.db.upcast()))?;
                 Ok(())
             }
+            _ if f.display_target.is_source_code() => write!(f, "'_"),
+            LifetimeData::BoundVar(idx) => idx.hir_fmt(f),
+            LifetimeData::InferenceVar(_) => write!(f, "_"),
             LifetimeData::Static => write!(f, "'static"),
             LifetimeData::Error => write!(f, "'{{error}}"),
             LifetimeData::Erased => Ok(()),

crates/hir-ty/src/lib.rs

@@ -56,7 +56,6 @@ use base_db::salsa::impl_intern_value_trivial;
 use chalk_ir::{
     fold::{Shift, TypeFoldable},
     interner::HasInterner,
-    visit::{TypeSuperVisitable, TypeVisitable, TypeVisitor},
     NoSolution,
 };
 use either::Either;
@@ -98,7 +97,9 @@ pub use traits::TraitEnvironment;
 pub use utils::{all_super_traits, is_fn_unsafe_to_call};
 
 pub use chalk_ir::{
-    cast::Cast, AdtId, BoundVar, DebruijnIndex, Mutability, Safety, Scalar, TyVariableKind,
+    cast::Cast,
+    visit::{TypeSuperVisitable, TypeVisitable, TypeVisitor},
+    AdtId, BoundVar, DebruijnIndex, Mutability, Safety, Scalar, TyVariableKind,
 };
 
 pub type ForeignDefId = chalk_ir::ForeignDefId<Interner>;

crates/hir-ty/src/tests/display_source_code.rs

@@ -85,7 +85,7 @@ fn render_dyn_for_ty() {
 trait Foo<'a> {}
 
 fn foo(foo: &dyn for<'a> Foo<'a>) {}
-    // ^^^ &dyn Foo<'{error}>
+    // ^^^ &dyn Foo<'_>
 "#,
     );
 }

crates/hir/src/lib.rs

@@ -4711,10 +4711,12 @@ impl Type {
                 if let WhereClause::Implemented(trait_ref) = pred.skip_binders() {
                     cb(type_.clone());
                     // skip the self type. it's likely the type we just got the bounds from
-                    for ty in
-                        trait_ref.substitution.iter(Interner).skip(1).filter_map(|a| a.ty(Interner))
-                    {
-                        walk_type(db, &type_.derived(ty.clone()), cb);
+                    if let [self_ty, params @ ..] = trait_ref.substitution.as_slice(Interner) {
+                        for ty in
+                            params.iter().filter(|&ty| ty != self_ty).filter_map(|a| a.ty(Interner))
+                        {
+                            walk_type(db, &type_.derived(ty.clone()), cb);
+                        }
                     }
                 }
             }