internal: Store function param names in ItemTree

crates/hir/src/display.rs

@@ -12,10 +12,7 @@ use hir_ty::{
     },
     Interner, TraitRefExt, WhereClause,
 };
-use syntax::{
+use syntax::SmolStr;
-    ast::{self, HasName},
-    SmolStr,
-};
 
 use crate::{
     Adt, Const, ConstParam, Enum, Field, Function, GenericParam, HasCrate, HasVisibility,
@@ -69,7 +66,7 @@ impl HirDisplay for Function {
         };
 
         let mut first = true;
-        for (param, type_ref) in self.assoc_fn_params(f.db).into_iter().zip(&data.params) {
+        for (name, type_ref) in &data.params {
             if !first {
                 write!(f, ", ")?;
             } else {
@@ -79,11 +76,9 @@ impl HirDisplay for Function {
                     continue;
                 }
             }
-            match param.pattern_source(f.db) {
+            match name {
-                Some(ast::Pat::IdentPat(p)) if p.name().is_some() => {
+                Some(name) => write!(f, "{}: ", name)?,
-                    write!(f, "{}: ", p.name().unwrap())?
+                None => write!(f, "_: ")?,
-                }
-                _ => write!(f, "_: ")?,
             }
             // FIXME: Use resolved `param.ty` or raw `type_ref`?
             // The former will ignore lifetime arguments currently.

crates/hir/src/lib.rs

@@ -1343,7 +1343,7 @@ impl Function {
             .params
             .iter()
             .enumerate()
-            .map(|(idx, type_ref)| {
+            .map(|(idx, (_, type_ref))| {
                 let ty = Type { krate, env: environment.clone(), ty: ctx.lower_ty(type_ref) };
                 Param { func: self, ty, idx }
             })
@@ -1421,6 +1421,10 @@ impl Param {
         &self.ty
     }
 
+    pub fn name(&self, db: &dyn HirDatabase) -> Option<Name> {
+        db.function_data(self.func.id).params[self.idx].0.clone()
+    }
+
     pub fn as_local(&self, db: &dyn HirDatabase) -> Local {
         let parent = DefWithBodyId::FunctionId(self.func.into());
         let body = db.body(parent);
@@ -1454,7 +1458,7 @@ impl SelfParam {
         func_data
             .params
             .first()
-            .map(|param| match &**param {
+            .map(|(_, param)| match &**param {
                 TypeRef::Reference(.., mutability) => match mutability {
                     hir_def::type_ref::Mutability::Shared => Access::Shared,
                     hir_def::type_ref::Mutability::Mut => Access::Exclusive,

crates/hir_def/src/data.rs

@@ -20,7 +20,7 @@ use crate::{
 #[derive(Debug, Clone, PartialEq, Eq)]
 pub struct FunctionData {
     pub name: Name,
-    pub params: Vec<Interned<TypeRef>>,
+    pub params: Vec<(Option<Name>, Interned<TypeRef>)>,
     pub ret_type: Interned<TypeRef>,
     pub async_ret_type: Option<Interned<TypeRef>>,
     pub attrs: Attrs,
@@ -72,7 +72,7 @@ impl FunctionData {
             params: enabled_params
                 .clone()
                 .filter_map(|id| match &item_tree[id] {
-                    Param::Normal(ty) => Some(ty.clone()),
+                    Param::Normal(name, ty) => Some((name.clone(), ty.clone())),
                     Param::Varargs => None,
                 })
                 .collect(),

crates/hir_def/src/generics.rs

@@ -113,7 +113,7 @@ impl GenericParams {
                 // Don't create an `Expander` nor call `loc.source(db)` if not needed since this
                 // causes a reparse after the `ItemTree` has been created.
                 let mut expander = Lazy::new(|| Expander::new(db, loc.source(db).file_id, module));
-                for param in &func_data.params {
+                for (_, param) in &func_data.params {
                     generic_params.fill_implicit_impl_trait_args(db, &mut expander, param);
                 }
 

crates/hir_def/src/item_tree.rs

@@ -598,7 +598,7 @@ pub struct Function {
 
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub enum Param {
-    Normal(Interned<TypeRef>),
+    Normal(Option<Name>, Interned<TypeRef>),
     Varargs,
 }
 

crates/hir_def/src/item_tree/lower.rs

@@ -269,7 +269,7 @@ impl<'a> Ctx<'a> {
                     }
                 };
                 let ty = Interned::new(self_type);
-                let idx = self.data().params.alloc(Param::Normal(ty));
+                let idx = self.data().params.alloc(Param::Normal(None, ty));
                 self.add_attrs(idx.into(), RawAttrs::new(self.db, &self_param, &self.hygiene));
                 has_self_param = true;
             }
@@ -279,7 +279,19 @@ impl<'a> Ctx<'a> {
                     None => {
                         let type_ref = TypeRef::from_ast_opt(&self.body_ctx, param.ty());
                         let ty = Interned::new(type_ref);
-                        self.data().params.alloc(Param::Normal(ty))
+                        let mut pat = param.pat();
+                        // FIXME: This really shouldn't be here, in fact FunctionData/ItemTree's function shouldn't know about
+                        // pattern names at all
+                        let name = loop {
+                            match pat {
+                                Some(ast::Pat::RefPat(ref_pat)) => pat = ref_pat.pat(),
+                                Some(ast::Pat::IdentPat(ident)) => {
+                                    break ident.name().map(|it| it.as_name())
+                                }
+                                _ => break None,
+                            }
+                        };
+                        self.data().params.alloc(Param::Normal(name, ty))
                     }
                 };
                 self.add_attrs(idx.into(), RawAttrs::new(self.db, &param, &self.hygiene));

crates/hir_def/src/item_tree/pretty.rs

@@ -257,8 +257,11 @@ impl<'a> Printer<'a> {
                         for param in params.clone() {
                             this.print_attrs_of(param);
                             match &this.tree[param] {
-                                Param::Normal(ty) => {
+                                Param::Normal(name, ty) => {
-                                    w!(this, "_: ");
+                                    match name {
+                                        Some(name) => w!(this, "{}: ", name),
+                                        None => w!(this, "_: "),
+                                    }
                                     this.print_type_ref(ty);
                                     wln!(this, ",");
                                 }

crates/hir_ty/src/infer.rs

@@ -697,7 +697,7 @@ impl<'a> InferenceContext<'a> {
         let ctx = crate::lower::TyLoweringContext::new(self.db, &self.resolver)
             .with_impl_trait_mode(ImplTraitLoweringMode::Param);
         let param_tys =
-            data.params.iter().map(|type_ref| ctx.lower_ty(type_ref)).collect::<Vec<_>>();
+            data.params.iter().map(|(_, type_ref)| ctx.lower_ty(type_ref)).collect::<Vec<_>>();
         for (ty, pat) in param_tys.into_iter().zip(body.params.iter()) {
             let ty = self.insert_type_vars(ty);
             let ty = self.normalize_associated_types_in(ty);

crates/hir_ty/src/lower.rs

@@ -1278,7 +1278,7 @@ fn fn_sig_for_fn(db: &dyn HirDatabase, def: FunctionId) -> PolyFnSig {
     let ctx_params = TyLoweringContext::new(db, &resolver)
         .with_impl_trait_mode(ImplTraitLoweringMode::Variable)
         .with_type_param_mode(TypeParamLoweringMode::Variable);
-    let params = data.params.iter().map(|tr| ctx_params.lower_ty(tr)).collect::<Vec<_>>();
+    let params = data.params.iter().map(|(_, tr)| ctx_params.lower_ty(tr)).collect::<Vec<_>>();
     let ctx_ret = TyLoweringContext::new(db, &resolver)
         .with_impl_trait_mode(ImplTraitLoweringMode::Opaque)
         .with_type_param_mode(TypeParamLoweringMode::Variable);

crates/ide_completion/src/render/builder_ext.rs

@@ -1,13 +1,12 @@
 //! Extensions for `Builder` structure required for item rendering.
 
 use itertools::Itertools;
-use syntax::ast::{self, HasName};
 
 use crate::{context::PathKind, item::Builder, patterns::ImmediateLocation, CompletionContext};
 
 #[derive(Debug)]
 pub(super) enum Params {
-    Named(Option<ast::SelfParam>, Vec<(ast::Param, hir::Param)>),
+    Named(Option<hir::SelfParam>, Vec<hir::Param>),
     Anonymous(usize),
 }
 
@@ -80,44 +79,22 @@ impl Builder {
                     let offset = if self_param.is_some() { 2 } else { 1 };
                     let function_params_snippet = params.iter().enumerate().format_with(
                         ", ",
-                        |(index, (param_source, param)), f| {
+                        |(index, param), f| match param.name(ctx.db) {
-                            let name;
-                            let text;
-                            let n = (|| {
-                                let mut pat = param_source.pat()?;
-                                loop {
-                                    match pat {
-                                        ast::Pat::IdentPat(pat) => break pat.name(),
-                                        ast::Pat::RefPat(it) => pat = it.pat()?,
-                                        _ => return None,
-                                    }
-                                }
-                            })();
-                            let (ref_, name) = match n {
                             Some(n) => {
-                                    name = n;
+                                let smol_str = n.to_smol_str();
-                                    text = name.text();
+                                let text = smol_str.as_str().trim_start_matches('_');
-                                    let text = text.as_str().trim_start_matches('_');
                                 let ref_ = ref_of_param(ctx, text, param.ty());
-                                    (ref_, text)
+                                f(&format_args!("${{{}:{}{}}}", index + offset, ref_, text))
                             }
-                                None => ("", "_"),
+                            None => f(&format_args!("${{{}:_}}", index + offset,)),
-                            };
-
-                            f(&format_args!("${{{}:{}{}}}", index + offset, ref_, name))
                         },
                     );
                     match self_param {
                         Some(self_param) => {
-                            let prefix = match self_param.kind() {
-                                ast::SelfParamKind::Owned => "",
-                                ast::SelfParamKind::Ref => "&",
-                                ast::SelfParamKind::MutRef => "&mut ",
-                            };
                             format!(
-                                "{}(${{1:{}self}}{}{})$0",
+                                "{}(${{1:{}}}{}{})$0",
                                 name,
-                                prefix,
+                                self_param.display(ctx.db),
                                 if params.is_empty() { "" } else { ", " },
                                 function_params_snippet
                             )

crates/ide_completion/src/render/function.rs

@@ -1,10 +1,9 @@
 //! Renderer for function calls.
 
-use hir::{AsAssocItem, HasSource, HirDisplay};
+use hir::{AsAssocItem, HirDisplay};
 use ide_db::SymbolKind;
 use itertools::Itertools;
 use stdx::format_to;
-use syntax::ast;
 
 use crate::{
     item::{CompletionItem, CompletionItemKind, CompletionRelevance, ImportEdit},
@@ -41,21 +40,6 @@ struct FunctionRender<'a> {
     name: hir::Name,
     receiver: Option<hir::Name>,
     func: hir::Function,
-    /// NB: having `ast::Fn` here might or might not be a good idea. The problem
-    /// with it is that, to get an `ast::`, you want to parse the corresponding
-    /// source file. So, when flyimport completions suggest a bunch of
-    /// functions, we spend quite some time parsing many files.
-    ///
-    /// We need ast because we want to access parameter names (patterns). We can
-    /// add them to the hir of the function itself, but parameter names are not
-    /// something hir cares otherwise.
-    ///
-    /// Alternatively we can reconstruct params from the function body, but that
-    /// would require parsing anyway.
-    ///
-    /// It seems that just using `ast` is the best choice -- most of parses
-    /// should be cached anyway.
-    param_list: Option<ast::ParamList>,
     is_method: bool,
 }
 
@@ -68,9 +52,8 @@ impl<'a> FunctionRender<'a> {
         is_method: bool,
     ) -> Option<FunctionRender<'a>> {
         let name = local_name.unwrap_or_else(|| fn_.name(ctx.db()));
-        let param_list = fn_.source(ctx.db())?.value.param_list();
 
-        Some(FunctionRender { ctx, name, receiver, func: fn_, param_list, is_method })
+        Some(FunctionRender { ctx, name, receiver, func: fn_, is_method })
     }
 
     fn render(self, import_to_add: Option<ImportEdit>) -> CompletionItem {
@@ -151,22 +134,17 @@ impl<'a> FunctionRender<'a> {
     }
 
     fn params(&self) -> Params {
-        let ast_params = match &self.param_list {
+        let (params, self_param) =
-            Some(it) => it,
+            if self.ctx.completion.has_dot_receiver() || self.receiver.is_some() {
-            None => return Params::Named(None, Vec::new()),
+                (self.func.method_params(self.ctx.db()).unwrap_or_default(), None)
-        };
-        let params = ast_params.params();
-
-        let (params, self_param) = if self.ctx.completion.has_dot_receiver()
-            || self.receiver.is_some()
-        {
-            (params.zip(self.func.method_params(self.ctx.db()).unwrap_or_default()).collect(), None)
             } else {
+                let self_param = self.func.self_param(self.ctx.db());
+
                 let mut assoc_params = self.func.assoc_fn_params(self.ctx.db());
-            if self.func.self_param(self.ctx.db()).is_some() {
+                if self_param.is_some() {
                     assoc_params.remove(0);
                 }
-            (params.zip(assoc_params).collect(), ast_params.self_param())
+                (assoc_params, self_param)
             };
 
         Params::Named(self_param, params)