Auto merge of #13335 - lowr:patch/change-generic-param-order, r=Veykril

internal: change generic parameter order

tl;dr: This PR changes the `Substitution` for trait items and methods like so:

```rust
trait Trait<TP, const CP: usize> { // note the implicit Self as first parameter
  type Type<TC, const CC: usize>;
  fn f<TC, const CC: usize>() {}
}
impl<TP, const CP: usize> S {
  fn f<TC, const CC: usize>() {}
}
```

- before this PR: `[Self, TP, CP, TC, CC]` for each trait item, `[TP, CP, TC, CC]` for `S::f`
- after this PR: `[TC, CC, Self, TP, CP]` for each trait item, `[TC, CC, TP, CP]` for `S::f`

---

This PR "inverts" the generic parameters/arguments of an item and its parent. This is to fulfill [chalk's expectation](https://github.com/rust-lang/chalk/blob/d875af0ff196dd6430b5f5fd87a640fa5ab59d1e/chalk-solve/src/rust_ir.rs#L498-L502) on the order of generic arguments in `Substitution`s for generic associated types and it's one step forward for GATs support (hopefully). Although chalk doesn't put any constraint for other items, it feels more natural to get everything aligned than special casing GATs.

One complication is that `TyBuilder` now demands its users to pass in parent's `Substitution` upon construction unless it's obvious that the the item has no parent (e.g. an ADT never has parent). All users *should* already know the parent of the item in question, and without this, it cannot be easily reasoned about whether we're pushing the argument for the item or for its parent.

Some additional notes:
- f8f5a5e: This isn't related to the change, but I felt it's nicer.

- 78977cd: There's one major change here other than the generic param order: Default arguments are now bound by the same `Binder` as the item in question rather than a `Binder` limited to parameters they can refer to (i.e. arguments that syntactically appear before them). Now that the order of generic parameters is changed, it would be somewhat complicated to make such `Binder`s as before, and the "full" `Binder`s shouldn't be a problem because we already make sure that the default arguments don't refer to the generic arguments after them with `fallback_bound_vars()`.

- 7556f74: This is split from 4385d3d to make it easy to revert if it turns out that the GATs with const generics panic is actually not resolved with this PR. cc #11878 #11957

Author: bors

crates/hir-ty/src/autoderef.rs

@@ -123,13 +123,14 @@ fn deref_by_trait(table: &mut InferenceTable<'_>, ty: Ty) -> Option<Ty> {
     let target = db.trait_data(deref_trait).associated_type_by_name(&name![Target])?;
 
     let projection = {
-        let b = TyBuilder::assoc_type_projection(db, target);
+        let b = TyBuilder::subst_for_def(db, deref_trait, None);
         if b.remaining() != 1 {
             // the Target type + Deref trait should only have one generic parameter,
             // namely Deref's Self type
             return None;
         }
-        b.push(ty).build()
+        let deref_subst = b.push(ty).build();
+        TyBuilder::assoc_type_projection(db, target, Some(deref_subst)).build()
     };
 
     // Check that the type implements Deref at all

crates/hir-ty/src/builder.rs

@@ -6,7 +6,7 @@ use chalk_ir::{
     cast::{Cast, CastTo, Caster},
     fold::TypeFoldable,
     interner::HasInterner,
-    AdtId, BoundVar, DebruijnIndex, Scalar,
+    AdtId, DebruijnIndex, Scalar,
 };
 use hir_def::{
     builtin_type::BuiltinType, generics::TypeOrConstParamData, ConstParamId, DefWithBodyId,
@@ -16,9 +16,9 @@ use smallvec::SmallVec;
 
 use crate::{
     consteval::unknown_const_as_generic, db::HirDatabase, infer::unify::InferenceTable, primitive,
-    to_assoc_type_id, to_chalk_trait_id, utils::generics, Binders, CallableSig, ConstData,
-    ConstValue, GenericArg, GenericArgData, Interner, ProjectionTy, Substitution, TraitRef, Ty,
-    TyDefId, TyExt, TyKind, ValueTyDefId,
+    to_assoc_type_id, to_chalk_trait_id, utils::generics, Binders, BoundVar, CallableSig,
+    GenericArg, Interner, ProjectionTy, Substitution, TraitRef, Ty, TyDefId, TyExt, TyKind,
+    ValueTyDefId,
 };
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -34,31 +34,51 @@ pub struct TyBuilder<D> {
     data: D,
     vec: SmallVec<[GenericArg; 2]>,
     param_kinds: SmallVec<[ParamKind; 2]>,
+    parent_subst: Substitution,
 }
 
 impl<A> TyBuilder<A> {
     fn with_data<B>(self, data: B) -> TyBuilder<B> {
-        TyBuilder { data, param_kinds: self.param_kinds, vec: self.vec }
+        TyBuilder {
+            data,
+            vec: self.vec,
+            param_kinds: self.param_kinds,
+            parent_subst: self.parent_subst,
+        }
     }
 }
 
 impl<D> TyBuilder<D> {
-    fn new(data: D, param_kinds: SmallVec<[ParamKind; 2]>) -> TyBuilder<D> {
-        TyBuilder { data, vec: SmallVec::with_capacity(param_kinds.len()), param_kinds }
+    fn new(
+        data: D,
+        param_kinds: SmallVec<[ParamKind; 2]>,
+        parent_subst: Option<Substitution>,
+    ) -> Self {
+        let parent_subst = parent_subst.unwrap_or_else(|| Substitution::empty(Interner));
+        Self { data, vec: SmallVec::with_capacity(param_kinds.len()), param_kinds, parent_subst }
+    }
+
+    fn new_empty(data: D) -> Self {
+        TyBuilder::new(data, SmallVec::new(), None)
     }
 
     fn build_internal(self) -> (D, Substitution) {
         assert_eq!(self.vec.len(), self.param_kinds.len());
         for (a, e) in self.vec.iter().zip(self.param_kinds.iter()) {
             self.assert_match_kind(a, e);
         }
-        let subst = Substitution::from_iter(Interner, self.vec);
+        let subst = Substitution::from_iter(
+            Interner,
+            self.vec.into_iter().chain(self.parent_subst.iter(Interner).cloned()),
+        );
         (self.data, subst)
     }
 
     pub fn push(mut self, arg: impl CastTo<GenericArg>) -> Self {
+        assert!(self.remaining() > 0);
         let arg = arg.cast(Interner);
         let expected_kind = &self.param_kinds[self.vec.len()];
+
         let arg_kind = match arg.data(Interner) {
             chalk_ir::GenericArgData::Ty(_) => ParamKind::Type,
             chalk_ir::GenericArgData::Lifetime(_) => panic!("Got lifetime in TyBuilder::push"),
@@ -68,7 +88,9 @@ impl<D> TyBuilder<D> {
             }
         };
         assert_eq!(*expected_kind, arg_kind);
+
         self.vec.push(arg);
+
         self
     }
 
@@ -79,20 +101,12 @@ impl<D> TyBuilder<D> {
     pub fn fill_with_bound_vars(self, debruijn: DebruijnIndex, starting_from: usize) -> Self {
         // self.fill is inlined to make borrow checker happy
         let mut this = self;
-        let other = this.param_kinds.iter().skip(this.vec.len());
+        let other = &this.param_kinds[this.vec.len()..];
         let filler = (starting_from..).zip(other).map(|(idx, kind)| match kind {
-            ParamKind::Type => {
-                GenericArgData::Ty(TyKind::BoundVar(BoundVar::new(debruijn, idx)).intern(Interner))
-                    .intern(Interner)
+            ParamKind::Type => BoundVar::new(debruijn, idx).to_ty(Interner).cast(Interner),
+            ParamKind::Const(ty) => {
+                BoundVar::new(debruijn, idx).to_const(Interner, ty.clone()).cast(Interner)
             }
-            ParamKind::Const(ty) => GenericArgData::Const(
-                ConstData {
-                    value: ConstValue::BoundVar(BoundVar::new(debruijn, idx)),
-                    ty: ty.clone(),
-                }
-                .intern(Interner),
-            )
-            .intern(Interner),
         });
         this.vec.extend(filler.take(this.remaining()).casted(Interner));
         assert_eq!(this.remaining(), 0);
@@ -102,8 +116,8 @@ impl<D> TyBuilder<D> {
     pub fn fill_with_unknown(self) -> Self {
         // self.fill is inlined to make borrow checker happy
         let mut this = self;
-        let filler = this.param_kinds.iter().skip(this.vec.len()).map(|x| match x {
-            ParamKind::Type => GenericArgData::Ty(TyKind::Error.intern(Interner)).intern(Interner),
+        let filler = this.param_kinds[this.vec.len()..].iter().map(|x| match x {
+            ParamKind::Type => TyKind::Error.intern(Interner).cast(Interner),
             ParamKind::Const(ty) => unknown_const_as_generic(ty.clone()),
         });
         this.vec.extend(filler.casted(Interner));
@@ -113,33 +127,17 @@ impl<D> TyBuilder<D> {
 
     pub(crate) fn fill_with_inference_vars(self, table: &mut InferenceTable<'_>) -> Self {
         self.fill(|x| match x {
-            ParamKind::Type => GenericArgData::Ty(table.new_type_var()).intern(Interner),
-            ParamKind::Const(ty) => {
-                GenericArgData::Const(table.new_const_var(ty.clone())).intern(Interner)
-            }
+            ParamKind::Type => table.new_type_var().cast(Interner),
+            ParamKind::Const(ty) => table.new_const_var(ty.clone()).cast(Interner),
         })
     }
 
     pub fn fill(mut self, filler: impl FnMut(&ParamKind) -> GenericArg) -> Self {
-        self.vec.extend(self.param_kinds.iter().skip(self.vec.len()).map(filler));
+        self.vec.extend(self.param_kinds[self.vec.len()..].iter().map(filler));
         assert_eq!(self.remaining(), 0);
         self
     }
 
-    pub fn use_parent_substs(mut self, parent_substs: &Substitution) -> Self {
-        assert!(self.vec.is_empty());
-        assert!(parent_substs.len(Interner) <= self.param_kinds.len());
-        self.extend(parent_substs.iter(Interner).cloned());
-        self
-    }
-
-    fn extend(&mut self, it: impl Iterator<Item = GenericArg> + Clone) {
-        for x in it.clone().zip(self.param_kinds.iter().skip(self.vec.len())) {
-            self.assert_match_kind(&x.0, &x.1);
-        }
-        self.vec.extend(it);
-    }
-
     fn assert_match_kind(&self, a: &chalk_ir::GenericArg<Interner>, e: &ParamKind) {
         match (a.data(Interner), e) {
             (chalk_ir::GenericArgData::Ty(_), ParamKind::Type)
@@ -188,53 +186,44 @@ impl TyBuilder<()> {
         params.placeholder_subst(db)
     }
 
-    pub fn subst_for_def(db: &dyn HirDatabase, def: impl Into<GenericDefId>) -> TyBuilder<()> {
-        let def = def.into();
-        let params = generics(db.upcast(), def);
-        TyBuilder::new(
-            (),
-            params
-                .iter()
-                .map(|(id, data)| match data {
-                    TypeOrConstParamData::TypeParamData(_) => ParamKind::Type,
-                    TypeOrConstParamData::ConstParamData(_) => {
-                        ParamKind::Const(db.const_param_ty(ConstParamId::from_unchecked(id)))
-                    }
-                })
-                .collect(),
-        )
+    pub fn subst_for_def(
+        db: &dyn HirDatabase,
+        def: impl Into<GenericDefId>,
+        parent_subst: Option<Substitution>,
+    ) -> TyBuilder<()> {
+        let generics = generics(db.upcast(), def.into());
+        // FIXME: this assertion should hold but some adjustment around
+        // `ValueTyDefId::EnumVariantId` is needed.
+        // assert!(generics.parent_generics().is_some() == parent_subst.is_some());
+        let params = generics
+            .iter_self()
+            .map(|(id, data)| match data {
+                TypeOrConstParamData::TypeParamData(_) => ParamKind::Type,
+                TypeOrConstParamData::ConstParamData(_) => {
+                    ParamKind::Const(db.const_param_ty(ConstParamId::from_unchecked(id)))
+                }
+            })
+            .collect();
+        TyBuilder::new((), params, parent_subst)
     }
 
     /// Creates a `TyBuilder` to build `Substitution` for a generator defined in `parent`.
     ///
     /// A generator's substitution consists of:
-    /// - generic parameters in scope on `parent`
     /// - resume type of generator
     /// - yield type of generator ([`Generator::Yield`](std::ops::Generator::Yield))
     /// - return type of generator ([`Generator::Return`](std::ops::Generator::Return))
+    /// - generic parameters in scope on `parent`
     /// in this order.
     ///
     /// This method prepopulates the builder with placeholder substitution of `parent`, so you
     /// should only push exactly 3 `GenericArg`s before building.
     pub fn subst_for_generator(db: &dyn HirDatabase, parent: DefWithBodyId) -> TyBuilder<()> {
-        let parent_subst = match parent.as_generic_def_id() {
-            Some(parent) => generics(db.upcast(), parent).placeholder_subst(db),
-            // Static initializers *may* contain generators.
-            None => Substitution::empty(Interner),
-        };
-        let builder = TyBuilder::new(
-            (),
-            parent_subst
-                .iter(Interner)
-                .map(|arg| match arg.constant(Interner) {
-                    Some(c) => ParamKind::Const(c.data(Interner).ty.clone()),
-                    None => ParamKind::Type,
-                })
-                // These represent resume type, yield type, and return type of generator.
-                .chain(std::iter::repeat(ParamKind::Type).take(3))
-                .collect(),
-        );
-        builder.use_parent_substs(&parent_subst)
+        let parent_subst =
+            parent.as_generic_def_id().map(|p| generics(db.upcast(), p).placeholder_subst(db));
+        // These represent resume type, yield type, and return type of generator.
+        let params = std::iter::repeat(ParamKind::Type).take(3).collect();
+        TyBuilder::new((), params, parent_subst)
     }
 
     pub fn build(self) -> Substitution {
@@ -245,24 +234,35 @@ impl TyBuilder<()> {
 
 impl TyBuilder<hir_def::AdtId> {
     pub fn adt(db: &dyn HirDatabase, def: hir_def::AdtId) -> TyBuilder<hir_def::AdtId> {
-        TyBuilder::subst_for_def(db, def).with_data(def)
+        TyBuilder::subst_for_def(db, def, None).with_data(def)
     }
 
     pub fn fill_with_defaults(
         mut self,
         db: &dyn HirDatabase,
         mut fallback: impl FnMut() -> Ty,
     ) -> Self {
+        // Note that we're building ADT, so we never have parent generic parameters.
         let defaults = db.generic_defaults(self.data.into());
+        let dummy_ty = TyKind::Error.intern(Interner).cast(Interner);
         for default_ty in defaults.iter().skip(self.vec.len()) {
-            if let GenericArgData::Ty(x) = default_ty.skip_binders().data(Interner) {
+            // 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() {
                     self.vec.push(fallback().cast(Interner));
                     continue;
                 }
-            };
-            // each default can depend on the previous parameters
-            let subst_so_far = Substitution::from_iter(Interner, self.vec.clone());
+            }
+            // Each default can only depend on the previous parameters.
+            // FIXME: we don't handle const generics here.
+            let subst_so_far = Substitution::from_iter(
+                Interner,
+                self.vec
+                    .iter()
+                    .cloned()
+                    .chain(iter::repeat(dummy_ty.clone()))
+                    .take(self.param_kinds.len()),
+            );
             self.vec.push(default_ty.clone().substitute(Interner, &subst_so_far).cast(Interner));
         }
         self
@@ -277,7 +277,7 @@ impl TyBuilder<hir_def::AdtId> {
 pub struct Tuple(usize);
 impl TyBuilder<Tuple> {
     pub fn tuple(size: usize) -> TyBuilder<Tuple> {
-        TyBuilder::new(Tuple(size), iter::repeat(ParamKind::Type).take(size).collect())
+        TyBuilder::new(Tuple(size), iter::repeat(ParamKind::Type).take(size).collect(), None)
     }
 
     pub fn build(self) -> Ty {
@@ -288,7 +288,7 @@ impl TyBuilder<Tuple> {
 
 impl TyBuilder<TraitId> {
     pub fn trait_ref(db: &dyn HirDatabase, def: TraitId) -> TyBuilder<TraitId> {
-        TyBuilder::subst_for_def(db, def).with_data(def)
+        TyBuilder::subst_for_def(db, def, None).with_data(def)
     }
 
     pub fn build(self) -> TraitRef {
@@ -298,8 +298,12 @@ impl TyBuilder<TraitId> {
 }
 
 impl TyBuilder<TypeAliasId> {
-    pub fn assoc_type_projection(db: &dyn HirDatabase, def: TypeAliasId) -> TyBuilder<TypeAliasId> {
-        TyBuilder::subst_for_def(db, def).with_data(def)
+    pub fn assoc_type_projection(
+        db: &dyn HirDatabase,
+        def: TypeAliasId,
+        parent_subst: Option<Substitution>,
+    ) -> TyBuilder<TypeAliasId> {
+        TyBuilder::subst_for_def(db, def, parent_subst).with_data(def)
     }
 
     pub fn build(self) -> ProjectionTy {
@@ -309,35 +313,48 @@ impl TyBuilder<TypeAliasId> {
 }
 
 impl<T: HasInterner<Interner = Interner> + TypeFoldable<Interner>> TyBuilder<Binders<T>> {
-    fn subst_binders(b: Binders<T>) -> Self {
-        let param_kinds = b
-            .binders
-            .iter(Interner)
-            .map(|x| match x {
-                chalk_ir::VariableKind::Ty(_) => ParamKind::Type,
-                chalk_ir::VariableKind::Lifetime => panic!("Got lifetime parameter"),
-                chalk_ir::VariableKind::Const(ty) => ParamKind::Const(ty.clone()),
-            })
-            .collect();
-        TyBuilder::new(b, param_kinds)
-    }
-
     pub fn build(self) -> T {
         let (b, subst) = self.build_internal();
         b.substitute(Interner, &subst)
     }
 }
 
 impl TyBuilder<Binders<Ty>> {
-    pub fn def_ty(db: &dyn HirDatabase, def: TyDefId) -> TyBuilder<Binders<Ty>> {
-        TyBuilder::subst_binders(db.ty(def))
+    pub fn def_ty(
+        db: &dyn HirDatabase,
+        def: TyDefId,
+        parent_subst: Option<Substitution>,
+    ) -> TyBuilder<Binders<Ty>> {
+        let poly_ty = db.ty(def);
+        let id: GenericDefId = match def {
+            TyDefId::BuiltinType(_) => {
+                assert!(parent_subst.is_none());
+                return TyBuilder::new_empty(poly_ty);
+            }
+            TyDefId::AdtId(id) => id.into(),
+            TyDefId::TypeAliasId(id) => id.into(),
+        };
+        TyBuilder::subst_for_def(db, id, parent_subst).with_data(poly_ty)
     }
 
     pub fn impl_self_ty(db: &dyn HirDatabase, def: hir_def::ImplId) -> TyBuilder<Binders<Ty>> {
-        TyBuilder::subst_binders(db.impl_self_ty(def))
+        TyBuilder::subst_for_def(db, def, None).with_data(db.impl_self_ty(def))
     }
 
-    pub fn value_ty(db: &dyn HirDatabase, def: ValueTyDefId) -> TyBuilder<Binders<Ty>> {
-        TyBuilder::subst_binders(db.value_ty(def))
+    pub fn value_ty(
+        db: &dyn HirDatabase,
+        def: ValueTyDefId,
+        parent_subst: Option<Substitution>,
+    ) -> TyBuilder<Binders<Ty>> {
+        let poly_value_ty = db.value_ty(def);
+        let id = match def.to_generic_def_id() {
+            Some(id) => id,
+            None => {
+                // static items
+                assert!(parent_subst.is_none());
+                return TyBuilder::new_empty(poly_value_ty);
+            }
+        };
+        TyBuilder::subst_for_def(db, id, parent_subst).with_data(poly_value_ty)
     }
 }