Auto merge of #17529 - Veykril:fix-17065, r=Veykril

fix: Fix lifetime parameters moving parameter defaults

Fixes #17075, #17515

We were incorrectly filling the default params due to our odd order of lifetime parameters vs type/const params. So this needs some special handling so that we don't shift the defaults around.

Author: bors

crates/hir-ty/src/builder.rs

@@ -63,7 +63,14 @@ impl<D> TyBuilder<D> {
     }
 
     fn build_internal(self) -> (D, Substitution) {
-        assert_eq!(self.vec.len(), self.param_kinds.len(), "{:?}", &self.param_kinds);
+        assert_eq!(
+            self.vec.len(),
+            self.param_kinds.len(),
+            "{} args received, {} expected ({:?})",
+            self.vec.len(),
+            self.param_kinds.len(),
+            &self.param_kinds
+        );
         for (a, e) in self.vec.iter().zip(self.param_kinds.iter()) {
             self.assert_match_kind(a, e);
         }
@@ -297,7 +304,8 @@ impl TyBuilder<hir_def::AdtId> {
     ) -> Self {
         // Note that we're building ADT, so we never have parent generic parameters.
         let defaults = db.generic_defaults(self.data.into());
-        for default_ty in defaults.iter().skip(self.vec.len()) {
+
+        for default_ty in &defaults[self.vec.len()..] {
             // NOTE(skip_binders): we only check if the arg type is error type.
             if let Some(x) = default_ty.skip_binders().ty(Interner) {
                 if x.is_unknown() {

crates/hir-ty/src/generics.rs

@@ -20,6 +20,7 @@ use hir_def::{
     LocalLifetimeParamId, LocalTypeOrConstParamId, Lookup, TypeOrConstParamId, TypeParamId,
 };
 use intern::Interned;
+use stdx::TupleExt;
 
 use crate::{db::HirDatabase, lt_to_placeholder_idx, to_placeholder_idx, Interner, Substitution};
 
@@ -57,7 +58,7 @@ impl Generics {
         self.iter_self().map(|(id, _)| id)
     }
 
-    fn iter_parent_id(&self) -> impl Iterator<Item = GenericParamId> + '_ {
+    pub(crate) fn iter_parent_id(&self) -> impl Iterator<Item = GenericParamId> + '_ {
         self.iter_parent().map(|(id, _)| id)
     }
 
@@ -67,6 +68,16 @@ impl Generics {
         self.params.iter_type_or_consts()
     }
 
+    pub(crate) fn iter_self_type_or_consts_id(
+        &self,
+    ) -> impl DoubleEndedIterator<Item = GenericParamId> + '_ {
+        self.params.iter_type_or_consts().map(from_toc_id(self)).map(TupleExt::head)
+    }
+
+    pub(crate) fn iter_self_lt_id(&self) -> impl DoubleEndedIterator<Item = GenericParamId> + '_ {
+        self.params.iter_lt().map(from_lt_id(self)).map(TupleExt::head)
+    }
+
     /// Iterate over the params followed by the parent params.
     pub(crate) fn iter(
         &self,

crates/hir-ty/src/lower.rs

@@ -812,13 +812,13 @@ impl<'a> TyLoweringContext<'a> {
         infer_args: bool,
         explicit_self_ty: Option<Ty>,
     ) -> Substitution {
-        // Remember that the item's own generic args come before its parent's.
-        let mut substs = Vec::new();
-        let def = if let Some(d) = def {
-            d
-        } else {
-            return Substitution::empty(Interner);
-        };
+        let Some(def) = def else { return Substitution::empty(Interner) };
+
+        // Order is
+        // - Optional Self parameter
+        // - Type or Const parameters
+        // - Lifetime parameters
+        // - Parent parameters
         let def_generics = generics(self.db.upcast(), def);
         let (
             parent_params,
@@ -832,45 +832,46 @@ impl<'a> TyLoweringContext<'a> {
             self_param as usize + type_params + const_params + impl_trait_params + lifetime_params;
         let total_len = parent_params + item_len;
 
-        let ty_error = TyKind::Error.intern(Interner).cast(Interner);
+        let mut substs = Vec::new();
 
-        let mut def_generic_iter = def_generics.iter_id();
+        // we need to iterate the lifetime and type/const params separately as our order of them
+        // differs from the supplied syntax
 
-        let fill_self_params = || {
+        let ty_error = || TyKind::Error.intern(Interner).cast(Interner);
+        let mut def_toc_iter = def_generics.iter_self_type_or_consts_id();
+        let mut def_lt_iter = def_generics.iter_self_lt_id();
+        let fill_self_param = || {
             if self_param {
-                let self_ty =
-                    explicit_self_ty.map(|x| x.cast(Interner)).unwrap_or_else(|| ty_error.clone());
+                let self_ty = explicit_self_ty.map(|x| x.cast(Interner)).unwrap_or_else(ty_error);
 
-                if let Some(id) = def_generic_iter.next() {
-                    assert!(matches!(
-                        id,
-                        GenericParamId::TypeParamId(_) | GenericParamId::LifetimeParamId(_)
-                    ));
+                if let Some(id) = def_toc_iter.next() {
+                    assert!(matches!(id, GenericParamId::TypeParamId(_)));
                     substs.push(self_ty);
                 }
             }
         };
         let mut had_explicit_args = false;
 
-        if let Some(generic_args) = &args_and_bindings {
-            if !generic_args.has_self_type {
-                fill_self_params();
+        let mut lifetimes = SmallVec::<[_; 1]>::new();
+        if let Some(&GenericArgs { ref args, has_self_type, .. }) = args_and_bindings {
+            if !has_self_type {
+                fill_self_param();
             }
-            let expected_num = if generic_args.has_self_type {
+            let expected_num = if has_self_type {
                 self_param as usize + type_params + const_params
             } else {
                 type_params + const_params
             };
-            let skip = if generic_args.has_self_type && !self_param { 1 } else { 0 };
-            // if args are provided, it should be all of them, but we can't rely on that
-            for arg in generic_args
-                .args
+            let skip = if has_self_type && !self_param { 1 } else { 0 };
+            // if non-lifetime args are provided, it should be all of them, but we can't rely on that
+            for arg in args
                 .iter()
                 .filter(|arg| !matches!(arg, GenericArg::Lifetime(_)))
                 .skip(skip)
                 .take(expected_num)
             {
-                if let Some(id) = def_generic_iter.next() {
+                if let Some(id) = def_toc_iter.next() {
+                    had_explicit_args = true;
                     let arg = generic_arg_to_chalk(
                         self.db,
                         id,
@@ -880,20 +881,16 @@ impl<'a> TyLoweringContext<'a> {
                         |_, const_ref, ty| self.lower_const(const_ref, ty),
                         |_, lifetime_ref| self.lower_lifetime(lifetime_ref),
                     );
-                    had_explicit_args = true;
                     substs.push(arg);
                 }
             }
 
-            for arg in generic_args
-                .args
+            for arg in args
                 .iter()
                 .filter(|arg| matches!(arg, GenericArg::Lifetime(_)))
                 .take(lifetime_params)
             {
-                // Taking into the fact that def_generic_iter will always have lifetimes at the end
-                // Should have some test cases tho to test this behaviour more properly
-                if let Some(id) = def_generic_iter.next() {
+                if let Some(id) = def_lt_iter.next() {
                     let arg = generic_arg_to_chalk(
                         self.db,
                         id,
@@ -903,59 +900,61 @@ impl<'a> TyLoweringContext<'a> {
                         |_, const_ref, ty| self.lower_const(const_ref, ty),
                         |_, lifetime_ref| self.lower_lifetime(lifetime_ref),
                     );
-                    had_explicit_args = true;
-                    substs.push(arg);
+                    lifetimes.push(arg);
                 }
             }
         } else {
-            fill_self_params();
+            fill_self_param();
         }
 
-        // These params include those of parent.
-        let remaining_params: SmallVec<[_; 2]> = def_generic_iter
-            .map(|id| match id {
-                GenericParamId::ConstParamId(x) => {
-                    unknown_const_as_generic(self.db.const_param_ty(x))
-                }
-                GenericParamId::TypeParamId(_) => ty_error.clone(),
-                GenericParamId::LifetimeParamId(_) => error_lifetime().cast(Interner),
-            })
-            .collect();
-        assert_eq!(remaining_params.len() + substs.len(), total_len);
-
+        let param_to_err = |id| match id {
+            GenericParamId::ConstParamId(x) => unknown_const_as_generic(self.db.const_param_ty(x)),
+            GenericParamId::TypeParamId(_) => ty_error(),
+            GenericParamId::LifetimeParamId(_) => error_lifetime().cast(Interner),
+        };
         // handle defaults. In expression or pattern path segments without
         // explicitly specified type arguments, missing type arguments are inferred
         // (i.e. defaults aren't used).
         // Generic parameters for associated types are not supposed to have defaults, so we just
         // ignore them.
-        let is_assoc_ty = if let GenericDefId::TypeAliasId(id) = def {
-            let container = id.lookup(self.db.upcast()).container;
-            matches!(container, ItemContainerId::TraitId(_))
-        } else {
-            false
+        let is_assoc_ty = || match def {
+            GenericDefId::TypeAliasId(id) => {
+                matches!(id.lookup(self.db.upcast()).container, ItemContainerId::TraitId(_))
+            }
+            _ => false,
         };
-        if !is_assoc_ty && (!infer_args || had_explicit_args) {
-            let defaults = self.db.generic_defaults(def);
-            assert_eq!(total_len, defaults.len());
+        if (!infer_args || had_explicit_args) && !is_assoc_ty() {
+            let defaults = &*self.db.generic_defaults(def);
+            let (item, _parent) = defaults.split_at(item_len);
+            let (toc, lt) = item.split_at(item_len - lifetime_params);
             let parent_from = item_len - substs.len();
 
-            for (idx, default_ty) in defaults[substs.len()..item_len].iter().enumerate() {
+            let mut rem =
+                def_generics.iter_id().skip(substs.len()).map(param_to_err).collect::<Vec<_>>();
+            // Fill in defaults for type/const params
+            for (idx, default_ty) in toc[substs.len()..].iter().enumerate() {
                 // each default can depend on the previous parameters
                 let substs_so_far = Substitution::from_iter(
                     Interner,
-                    substs.iter().cloned().chain(remaining_params[idx..].iter().cloned()),
+                    substs.iter().cloned().chain(rem[idx..].iter().cloned()),
                 );
                 substs.push(default_ty.clone().substitute(Interner, &substs_so_far));
             }
-
-            // Keep parent's params as unknown.
-            let mut remaining_params = remaining_params;
-            substs.extend(remaining_params.drain(parent_from..));
+            let n_lifetimes = lifetimes.len();
+            substs.extend(lifetimes);
+            // Fill in defaults for lifetime params
+            for default_ty in &lt[n_lifetimes..] {
+                // these are always errors so skipping is fine
+                substs.push(default_ty.skip_binders().clone());
+            }
+            // Fill in remaining def params and parent params
+            substs.extend(rem.drain(parent_from..));
         } else {
-            substs.extend(remaining_params);
+            // Fill in remaining def params and parent params
+            substs.extend(def_generics.iter_id().skip(substs.len()).map(param_to_err));
         }
 
-        assert_eq!(substs.len(), total_len);
+        assert_eq!(substs.len(), total_len, "expected {} substs, got {}", total_len, substs.len());
         Substitution::from_iter(Interner, substs)
     }
 

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

@@ -2018,3 +2018,26 @@ fn tait_async_stack_overflow_17199() {
 "#,
     );
 }
+
+#[test]
+fn lifetime_params_move_param_defaults() {
+    check_types(
+        r#"
+pub struct Thing<'s, T = u32>;
+
+impl <'s> Thing<'s> {
+    pub fn new() -> Thing<'s> {
+        Thing
+      //^^^^^ Thing<'?, u32>
+    }
+}
+
+fn main() {
+    let scope =
+      //^^^^^ &'? Thing<'?, u32>
+                &Thing::new();
+               //^^^^^^^^^^^^ Thing<'?, u32>
+}
+"#,
+    );
+}

crates/hir/src/lib.rs

@@ -3602,9 +3602,9 @@ impl ConstParam {
 }
 
 fn generic_arg_from_param(db: &dyn HirDatabase, id: TypeOrConstParamId) -> Option<GenericArg> {
-    let params = db.generic_defaults(id.parent);
     let local_idx = hir_ty::param_idx(db, id)?;
-    let ty = params.get(local_idx)?.clone();
+    let defaults = db.generic_defaults(id.parent);
+    let ty = defaults.get(local_idx)?.clone();
     let subst = TyBuilder::placeholder_subst(db, id.parent);
     Some(ty.substitute(Interner, &subst))
 }