Auto merge of #15054 - ponyii:fix/implement-missing-members-do-not-transform-const-params, r=lowr

fix: implement missing members doesn't transform const params and default types

Fixes https://github.com/rust-lang/rust-analyzer/issues/13363

crates/ide-assists/src/handlers/add_missing_impl_members.rs

@@ -400,6 +400,72 @@ impl<'x, 'y, T, V, U: Default> Trait<'x, 'y, T, V, U> for () {
         );
     }
 
+    #[test]
+    fn test_const_substitution() {
+        check_assist(
+            add_missing_default_members,
+            r#"
+struct Bar<const: N: bool> {
+    bar: [i32, N]
+}
+
+trait Foo<const N: usize, T> {
+    fn get_n_sq(&self, arg: &T) -> usize { N * N }
+    fn get_array(&self, arg: Bar<N>) -> [i32; N] { [1; N] }
+}
+
+struct S<T> {
+    wrapped: T
+}
+
+impl<const X: usize, Y, Z> Foo<X, Z> for S<Y> {
+    $0
+}"#,
+            r#"
+struct Bar<const: N: bool> {
+    bar: [i32, N]
+}
+
+trait Foo<const N: usize, T> {
+    fn get_n_sq(&self, arg: &T) -> usize { N * N }
+    fn get_array(&self, arg: Bar<N>) -> [i32; N] { [1; N] }
+}
+
+struct S<T> {
+    wrapped: T
+}
+
+impl<const X: usize, Y, Z> Foo<X, Z> for S<Y> {
+    $0fn get_n_sq(&self, arg: &Z) -> usize { X * X }
+
+    fn get_array(&self, arg: Bar<X>) -> [i32; X] { [1; X] }
+}"#,
+        )
+    }
+
+    #[test]
+    fn test_const_substitution_2() {
+        check_assist(
+            add_missing_default_members,
+            r#"
+trait Foo<const N: usize, const M: usize, T> {
+    fn get_sum(&self, arg: &T) -> usize { N + M }
+}
+
+impl<X> Foo<42, {20 + 22}, X> for () {
+    $0
+}"#,
+            r#"
+trait Foo<const N: usize, const M: usize, T> {
+    fn get_sum(&self, arg: &T) -> usize { N + M }
+}
+
+impl<X> Foo<42, {20 + 22}, X> for () {
+    $0fn get_sum(&self, arg: &X) -> usize { 42 + {20 + 22} }
+}"#,
+        )
+    }
+
     #[test]
     fn test_cursor_after_empty_impl_def() {
         check_assist(
@@ -781,6 +847,115 @@ fn bar(&self, this: &T, that: &Self) {
         )
     }
 
+    #[test]
+    fn test_qualify_generic_default_parameter() {
+        check_assist(
+            add_missing_impl_members,
+            r#"
+mod m {
+    pub struct S;
+    pub trait Foo<T = S> {
+        fn bar(&self, other: &T);
+    }
+}
+
+struct S;
+impl m::Foo for S { $0 }"#,
+            r#"
+mod m {
+    pub struct S;
+    pub trait Foo<T = S> {
+        fn bar(&self, other: &T);
+    }
+}
+
+struct S;
+impl m::Foo for S {
+    fn bar(&self, other: &m::S) {
+        ${0:todo!()}
+    }
+}"#,
+        )
+    }
+
+    #[test]
+    fn test_qualify_generic_default_parameter_2() {
+        check_assist(
+            add_missing_impl_members,
+            r#"
+mod m {
+    pub struct Wrapper<T, V> {
+        one: T,
+        another: V
+    };
+    pub struct S;
+    pub trait Foo<T = Wrapper<S, bool>> {
+        fn bar(&self, other: &T);
+    }
+}
+
+struct S;
+impl m::Foo for S { $0 }"#,
+            r#"
+mod m {
+    pub struct Wrapper<T, V> {
+        one: T,
+        another: V
+    };
+    pub struct S;
+    pub trait Foo<T = Wrapper<S, bool>> {
+        fn bar(&self, other: &T);
+    }
+}
+
+struct S;
+impl m::Foo for S {
+    fn bar(&self, other: &m::Wrapper<m::S, bool>) {
+        ${0:todo!()}
+    }
+}"#,
+        );
+    }
+
+    #[test]
+    fn test_qualify_generic_default_parameter_3() {
+        check_assist(
+            add_missing_impl_members,
+            r#"
+mod m {
+    pub struct Wrapper<T, V> {
+        one: T,
+        another: V
+    };
+    pub struct S;
+    pub trait Foo<T = S, V = Wrapper<T, S>> {
+        fn bar(&self, other: &V);
+    }
+}
+
+struct S;
+impl m::Foo for S { $0 }"#,
+            r#"
+mod m {
+    pub struct Wrapper<T, V> {
+        one: T,
+        another: V
+    };
+    pub struct S;
+    pub trait Foo<T = S, V = Wrapper<T, S>> {
+        fn bar(&self, other: &V);
+    }
+}
+
+struct S;
+impl m::Foo for S {
+    fn bar(&self, other: &m::Wrapper<m::S, m::S>) {
+        ${0:todo!()}
+    }
+}"#,
+        );
+    }
+
     #[test]
     fn test_assoc_type_bounds_are_removed() {
         check_assist(

crates/ide-assists/src/handlers/inline_call.rs

@@ -958,7 +958,6 @@ fn main() {
         );
     }
 
-    // FIXME: const generics aren't being substituted, this is blocked on better support for them
     #[test]
     fn inline_substitutes_generics() {
         check_assist(
@@ -982,7 +981,7 @@ fn foo<T, const N: usize>() {
 fn bar<U, const M: usize>() {}
 
 fn main() {
-    bar::<usize, N>();
+    bar::<usize, {0}>();
 }
 "#,
         );

crates/ide-db/src/path_transform.rs

@@ -11,10 +11,15 @@
 
 #[derive(Default)]
 struct AstSubsts {
-    types: Vec<ast::TypeArg>,
+    types_and_consts: Vec<TypeOrConst>,
     lifetimes: Vec<ast::LifetimeArg>,
 }
 
+enum TypeOrConst {
+    Either(ast::TypeArg), // indistinguishable type or const param
+    Const(ast::ConstArg),
+}
+
 type LifetimeName = String;
 
 /// `PathTransform` substitutes path in SyntaxNodes in bulk.
@@ -108,8 +113,10 @@ fn build_ctx(&self) -> Ctx<'a> {
             Some(hir::GenericDef::Trait(_)) => 1,
             _ => 0,
         };
-        let type_substs: FxHashMap<_, _> = self
+        let mut type_substs: FxHashMap<hir::TypeParam, ast::Type> = Default::default();
-            .generic_def
+        let mut const_substs: FxHashMap<hir::ConstParam, SyntaxNode> = Default::default();
+        let mut default_types: Vec<hir::TypeParam> = Default::default();
+        self.generic_def
             .into_iter()
             .flat_map(|it| it.type_params(db))
             .skip(skip)
@@ -119,21 +126,41 @@ fn build_ctx(&self) -> Ctx<'a> {
             // can still hit those trailing values and check if they actually have
             // a default type. If they do, go for that type from `hir` to `ast` so
             // the resulting change can be applied correctly.
-            .zip(self.substs.types.iter().map(Some).chain(std::iter::repeat(None)))
+            .zip(self.substs.types_and_consts.iter().map(Some).chain(std::iter::repeat(None)))
-            .filter_map(|(k, v)| match k.split(db) {
+            .for_each(|(k, v)| match (k.split(db), v) {
-                Either::Left(_) => None, // FIXME: map const types too
+                (Either::Right(k), Some(TypeOrConst::Either(v))) => {
-                Either::Right(t) => match v {
+                    if let Some(ty) = v.ty() {
-                    Some(v) => Some((k, v.ty()?.clone())),
+                        type_substs.insert(k, ty.clone());
-                    None => {
-                        let default = t.default(db)?;
-                        let v = ast::make::ty(
-                            &default.display_source_code(db, source_module.into(), false).ok()?,
-                        );
-                        Some((k, v))
                     }
-                },
+                }
-            })
+                (Either::Right(k), None) => {
-            .collect();
+                    if let Some(default) = k.default(db) {
+                        if let Some(default) =
+                            &default.display_source_code(db, source_module.into(), false).ok()
+                        {
+                            type_substs.insert(k, ast::make::ty(default).clone_for_update());
+                            default_types.push(k);
+                        }
+                    }
+                }
+                (Either::Left(k), Some(TypeOrConst::Either(v))) => {
+                    if let Some(ty) = v.ty() {
+                        const_substs.insert(k, ty.syntax().clone());
+                    }
+                }
+                (Either::Left(k), Some(TypeOrConst::Const(v))) => {
+                    if let Some(expr) = v.expr() {
+                        // FIXME: expressions in curly brackets can cause ambiguity after insertion
+                        // (e.g. `N * 2` -> `{1 + 1} * 2`; it's unclear whether `{1 + 1}`
+                        // is a standalone statement or a part of another expresson)
+                        // and sometimes require slight modifications; see
+                        // https://doc.rust-lang.org/reference/statements.html#expression-statements
+                        const_substs.insert(k, expr.syntax().clone());
+                    }
+                }
+                (Either::Left(_), None) => (), // FIXME: get default const value
+                _ => (),                       // ignore mismatching params
+            });
         let lifetime_substs: FxHashMap<_, _> = self
             .generic_def
             .into_iter()
@@ -141,50 +168,61 @@ fn build_ctx(&self) -> Ctx<'a> {
             .zip(self.substs.lifetimes.clone())
             .filter_map(|(k, v)| Some((k.name(db).display(db.upcast()).to_string(), v.lifetime()?)))
             .collect();
-        Ctx { type_substs, lifetime_substs, target_module, source_scope: self.source_scope }
+        let ctx = Ctx {
+            type_substs,
+            const_substs,
+            lifetime_substs,
+            target_module,
+            source_scope: self.source_scope,
+        };
+        ctx.transform_default_type_substs(default_types);
+        ctx
     }
 }
 
 struct Ctx<'a> {
-    type_substs: FxHashMap<hir::TypeOrConstParam, ast::Type>,
+    type_substs: FxHashMap<hir::TypeParam, ast::Type>,
+    const_substs: FxHashMap<hir::ConstParam, SyntaxNode>,
     lifetime_substs: FxHashMap<LifetimeName, ast::Lifetime>,
     target_module: hir::Module,
     source_scope: &'a SemanticsScope<'a>,
 }
 
+fn postorder(item: &SyntaxNode) -> impl Iterator<Item = SyntaxNode> {
+    item.preorder().filter_map(|event| match event {
+        syntax::WalkEvent::Enter(_) => None,
+        syntax::WalkEvent::Leave(node) => Some(node),
+    })
+}
+
 impl<'a> Ctx<'a> {
     fn apply(&self, item: &SyntaxNode) {
         // `transform_path` may update a node's parent and that would break the
         // tree traversal. Thus all paths in the tree are collected into a vec
         // so that such operation is safe.
-        let paths = item
+        let paths = postorder(item).filter_map(ast::Path::cast).collect::<Vec<_>>();
-            .preorder()
-            .filter_map(|event| match event {
-                syntax::WalkEvent::Enter(_) => None,
-                syntax::WalkEvent::Leave(node) => Some(node),
-            })
-            .filter_map(ast::Path::cast)
-            .collect::<Vec<_>>();
-
         for path in paths {
             self.transform_path(path);
         }
 
-        item.preorder()
+        postorder(item).filter_map(ast::Lifetime::cast).for_each(|lifetime| {
-            .filter_map(|event| match event {
+            if let Some(subst) = self.lifetime_substs.get(&lifetime.syntax().text().to_string()) {
-                syntax::WalkEvent::Enter(_) => None,
+                ted::replace(lifetime.syntax(), subst.clone_subtree().clone_for_update().syntax());
-                syntax::WalkEvent::Leave(node) => Some(node),
+            }
-            })
+        });
-            .filter_map(ast::Lifetime::cast)
+    }
-            .for_each(|lifetime| {
+
-                if let Some(subst) = self.lifetime_substs.get(&lifetime.syntax().text().to_string())
+    fn transform_default_type_substs(&self, default_types: Vec<hir::TypeParam>) {
-                {
+        for k in default_types {
-                    ted::replace(
+            let v = self.type_substs.get(&k).unwrap();
-                        lifetime.syntax(),
+            // `transform_path` may update a node's parent and that would break the
-                        subst.clone_subtree().clone_for_update().syntax(),
+            // tree traversal. Thus all paths in the tree are collected into a vec
-                    );
+            // so that such operation is safe.
-                }
+            let paths = postorder(&v.syntax()).filter_map(ast::Path::cast).collect::<Vec<_>>();
-            });
+            for path in paths {
+                self.transform_path(path);
+            }
+        }
     }
 
     fn transform_path(&self, path: ast::Path) -> Option<()> {
@@ -203,7 +241,7 @@ fn transform_path(&self, path: ast::Path) -> Option<()> {
 
         match resolution {
             hir::PathResolution::TypeParam(tp) => {
-                if let Some(subst) = self.type_substs.get(&tp.merge()) {
+                if let Some(subst) = self.type_substs.get(&tp) {
                     let parent = path.syntax().parent()?;
                     if let Some(parent) = ast::Path::cast(parent.clone()) {
                         // Path inside path means that there is an associated
@@ -270,8 +308,12 @@ fn transform_path(&self, path: ast::Path) -> Option<()> {
                 }
                 ted::replace(path.syntax(), res.syntax())
             }
+            hir::PathResolution::ConstParam(cp) => {
+                if let Some(subst) = self.const_substs.get(&cp) {
+                    ted::replace(path.syntax(), subst.clone_subtree().clone_for_update());
+                }
+            }
             hir::PathResolution::Local(_)
-            | hir::PathResolution::ConstParam(_)
             | hir::PathResolution::SelfType(_)
             | hir::PathResolution::Def(_)
             | hir::PathResolution::BuiltinAttr(_)
@@ -298,9 +340,18 @@ fn get_syntactic_substs(impl_def: ast::Impl) -> Option<AstSubsts> {
 fn get_type_args_from_arg_list(generic_arg_list: ast::GenericArgList) -> Option<AstSubsts> {
     let mut result = AstSubsts::default();
     generic_arg_list.generic_args().for_each(|generic_arg| match generic_arg {
-        ast::GenericArg::TypeArg(type_arg) => result.types.push(type_arg),
+        // Const params are marked as consts on definition only,
+        // being passed to the trait they are indistguishable from type params;
+        // anyway, we don't really need to distinguish them here.
+        ast::GenericArg::TypeArg(type_arg) => {
+            result.types_and_consts.push(TypeOrConst::Either(type_arg))
+        }
+        // Some const values are recognized correctly.
+        ast::GenericArg::ConstArg(const_arg) => {
+            result.types_and_consts.push(TypeOrConst::Const(const_arg));
+        }
         ast::GenericArg::LifetimeArg(l_arg) => result.lifetimes.push(l_arg),
-        _ => (), // FIXME: don't filter out const params
+        _ => (),
     });
 
     Some(result)