Change generic parameter/argument order

This commit "inverts" the order of generic parameters/arguments of an
item and its parent. This is to fulfill chalk's expectation on the
order of `Substitution` for generic associated types and it's one step
forward for their support (hopefully).

Although chalk doesn't put any constraint on the order of `Substitution`
for other items, it feels natural to get everything aligned rather 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.

Quick comparison of how this commit changes `Substitution`:

```rust
trait Trait<TP, const CP: usize> {
  type Type<TC, const CC: usize> = ();
  fn f<TC, const CC: usize>() {}
}
```

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

Author: lowr

crates/hir-ty/src/builder.rs

@@ -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
     }
 
@@ -116,20 +138,6 @@ impl<D> TyBuilder<D> {
         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)
@@ -178,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 {
@@ -235,25 +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()) {
             // 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
@@ -268,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 {
@@ -279,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 {
@@ -289,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 {
@@ -300,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)
     }
 }

crates/hir-ty/src/lower.rs

@@ -1641,6 +1641,19 @@ pub enum ValueTyDefId {
 }
 impl_from!(FunctionId, StructId, UnionId, EnumVariantId, ConstId, StaticId for ValueTyDefId);
 
+impl ValueTyDefId {
+    pub(crate) fn to_generic_def_id(self) -> Option<GenericDefId> {
+        match self {
+            Self::FunctionId(id) => Some(id.into()),
+            Self::StructId(id) => Some(id.into()),
+            Self::UnionId(id) => Some(id.into()),
+            Self::EnumVariantId(var) => Some(var.parent.into()),
+            Self::ConstId(id) => Some(id.into()),
+            Self::StaticId(_) => None,
+        }
+    }
+}
+
 /// Build the declared type of an item. This depends on the namespace; e.g. for
 /// `struct Foo(usize)`, we have two types: The type of the struct itself, and
 /// the constructor function `(usize) -> Foo` which lives in the values

crates/hir-ty/src/utils.rs

@@ -220,36 +220,49 @@ impl Generics {
         })
     }
 
-    /// Iterator over types and const params of parent, then self.
+    /// Iterator over types and const params of self, then parent.
     pub(crate) fn iter<'a>(
         &'a self,
     ) -> impl DoubleEndedIterator<Item = (TypeOrConstParamId, &'a TypeOrConstParamData)> + 'a {
         let to_toc_id = |it: &'a Generics| {
             move |(local_id, p)| (TypeOrConstParamId { parent: it.def, local_id }, p)
         };
-        self.parent_generics()
-            .into_iter()
-            .flat_map(move |it| it.params.iter().map(to_toc_id(it)))
-            .chain(self.params.iter().map(to_toc_id(self)))
+        self.params.iter().map(to_toc_id(self)).chain(self.iter_parent())
+    }
+
+    /// Iterate over types and const params without parent params.
+    pub(crate) fn iter_self<'a>(
+        &'a self,
+    ) -> impl DoubleEndedIterator<Item = (TypeOrConstParamId, &'a TypeOrConstParamData)> + 'a {
+        let to_toc_id = |it: &'a Generics| {
+            move |(local_id, p)| (TypeOrConstParamId { parent: it.def, local_id }, p)
+        };
+        self.params.iter().map(to_toc_id(self))
     }
 
     /// Iterator over types and const params of parent.
     pub(crate) fn iter_parent<'a>(
         &'a self,
-    ) -> impl Iterator<Item = (TypeOrConstParamId, &'a TypeOrConstParamData)> + 'a {
+    ) -> impl DoubleEndedIterator<Item = (TypeOrConstParamId, &'a TypeOrConstParamData)> + 'a {
         self.parent_generics().into_iter().flat_map(|it| {
             let to_toc_id =
                 move |(local_id, p)| (TypeOrConstParamId { parent: it.def, local_id }, p);
             it.params.iter().map(to_toc_id)
         })
     }
 
+    /// Returns total number of generic parameters in scope, including those from parent.
     pub(crate) fn len(&self) -> usize {
         let parent = self.parent_generics().map_or(0, Generics::len);
         let child = self.params.type_or_consts.len();
         parent + child
     }
 
+    /// Returns numbers of generic parameters excluding those from parent.
+    pub(crate) fn len_self(&self) -> usize {
+        self.params.type_or_consts.len()
+    }
+
     /// (parent total, self param, type param list, const param list, impl trait)
     pub(crate) fn provenance_split(&self) -> (usize, usize, usize, usize, usize) {
         let ty_iter = || self.params.iter().filter_map(|x| x.1.type_param());
@@ -274,10 +287,12 @@ impl Generics {
         if param.parent == self.def {
             let (idx, (_local_id, data)) =
                 self.params.iter().enumerate().find(|(_, (idx, _))| *idx == param.local_id)?;
-            let parent_len = self.parent_generics().map_or(0, Generics::len);
-            Some((parent_len + idx, data))
+            Some((idx, data))
         } else {
-            self.parent_generics().and_then(|g| g.find_param(param))
+            self.parent_generics()
+                .and_then(|g| g.find_param(param))
+                // Remember that parent parameters come after parameters for self.
+                .map(|(idx, data)| (self.len_self() + idx, data))
         }
     }