Merge #11067

11067: internal: Store function param names in ItemTree r=Veykril a=Veykril

This prevents us reparsing source files for completions, sometimes slowing them down massively if the source file is not cached at the expense of a slightly bigger memory usage.

related info https://rust-lang.zulipchat.com/#narrow/stream/185405-t-compiler.2Frust-analyzer/topic/Completion.20performance

Co-authored-by: Lukas Wirth <lukastw97@gmail.com>

12 files changed, 79 insertions, 104 deletions

diff --git a/crates/hir/src/display.rs b/crates/hir/src/display.rs
index 91ff1713120..46815cff864 100644
--- a/crates/hir/src/display.rs
+++ b/crates/hir/src/display.rs
@@ -12,10 +12,7 @@ use hir_ty::{
     },
     Interner, TraitRefExt, WhereClause,
 };
-use syntax::{
-    ast::{self, HasName},
-    SmolStr,
-};
+use syntax::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) {
-                Some(ast::Pat::IdentPat(p)) if p.name().is_some() => {
-                    write!(f, "{}: ", p.name().unwrap())?
-                }
-                _ => write!(f, "_: ")?,
+            match name {
+                Some(name) => write!(f, "{}: ", name)?,
+                None => write!(f, "_: ")?,
             }
             // FIXME: Use resolved `param.ty` or raw `type_ref`?
             // The former will ignore lifetime arguments currently.
diff --git a/crates/hir/src/lib.rs b/crates/hir/src/lib.rs
index a80668f1fe5..b57f49690ac 100644
--- a/crates/hir/src/lib.rs
+++ b/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,
diff --git a/crates/hir_def/src/data.rs b/crates/hir_def/src/data.rs
index bf8bd931a57..aa2844461b6 100644
--- a/crates/hir_def/src/data.rs
+++ b/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(),
diff --git a/crates/hir_def/src/generics.rs b/crates/hir_def/src/generics.rs
index 98eafa9000c..c4bd5b39c5b 100644
--- a/crates/hir_def/src/generics.rs
+++ b/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);
                 }
 
diff --git a/crates/hir_def/src/item_tree.rs b/crates/hir_def/src/item_tree.rs
index 2449e4f3e22..aafda496934 100644
--- a/crates/hir_def/src/item_tree.rs
+++ b/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,
 }
 
diff --git a/crates/hir_def/src/item_tree/lower.rs b/crates/hir_def/src/item_tree/lower.rs
index 4396801116c..acc26bcf633 100644
--- a/crates/hir_def/src/item_tree/lower.rs
+++ b/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));
diff --git a/crates/hir_def/src/item_tree/pretty.rs b/crates/hir_def/src/item_tree/pretty.rs
index 4b462837436..eaaff5a21f7 100644
--- a/crates/hir_def/src/item_tree/pretty.rs
+++ b/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) => {
-                                    w!(this, "_: ");
+                                Param::Normal(name, ty) => {
+                                    match name {
+                                        Some(name) => w!(this, "{}: ", name),
+                                        None => w!(this, "_: "),
+                                    }
                                     this.print_type_ref(ty);
                                     wln!(this, ",");
                                 }
diff --git a/crates/hir_def/src/item_tree/tests.rs b/crates/hir_def/src/item_tree/tests.rs
index 1969913bf94..63d177eaabd 100644
--- a/crates/hir_def/src/item_tree/tests.rs
+++ b/crates/hir_def/src/item_tree/tests.rs
@@ -176,7 +176,7 @@ trait Tr: SuperTrait + 'lifetime {
             // flags = 0x2
             pub(self) fn f(
                 #[attr]  // AttrId { is_doc_comment: false, ast_index: 0 }
-                _: u8,
+                arg: u8,
                 _: (),
             ) -> ();
 
@@ -341,7 +341,7 @@ trait Tr<'a, T: 'a>: Super where Self: for<'a> Tr<'a, T> {}
             {
                 // flags = 0x2
                 pub(self) fn f<G>(
-                    _: impl Copy,
+                    arg: impl Copy,
                 ) -> impl Copy
                 where
                     G: 'a;
diff --git a/crates/hir_ty/src/infer.rs b/crates/hir_ty/src/infer.rs
index 54c3590f04b..a6c91f9d27e 100644
--- a/crates/hir_ty/src/infer.rs
+++ b/crates/hir_ty/src/infer.rs
@@ -718,7 +718,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);
diff --git a/crates/hir_ty/src/lower.rs b/crates/hir_ty/src/lower.rs
index a22d15dd81b..a450f6c75cc 100644
--- a/crates/hir_ty/src/lower.rs
+++ b/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);
diff --git a/crates/ide_completion/src/render/builder_ext.rs b/crates/ide_completion/src/render/builder_ext.rs
index 4d040544454..8f8bd1dfcda 100644
--- a/crates/ide_completion/src/render/builder_ext.rs
+++ b/crates/ide_completion/src/render/builder_ext.rs
@@ -1,21 +1,19 @@
 //! Extensions for `Builder` structure required for item rendering.
 
-use either::Either;
 use itertools::Itertools;
-use syntax::ast::{self, HasName};
 
 use crate::{context::PathKind, item::Builder, patterns::ImmediateLocation, CompletionContext};
 
 #[derive(Debug)]
 pub(super) enum Params {
-    Named(Vec<(Either<ast::SelfParam, ast::Param>, hir::Param)>),
+    Named(Option<hir::SelfParam>, Vec<hir::Param>),
     Anonymous(usize),
 }
 
 impl Params {
     pub(super) fn len(&self) -> usize {
         match self {
-            Params::Named(xs) => xs.len(),
+            Params::Named(selv, params) => params.len() + if selv.is_some() { 1 } else { 0 },
             Params::Anonymous(len) => *len,
         }
     }
@@ -77,48 +75,34 @@ impl Builder {
         } else {
             self.trigger_call_info();
             let snippet = match (ctx.config.add_call_argument_snippets, params) {
-                (true, Params::Named(params)) => {
+                (true, Params::Named(self_param, params)) => {
+                    let offset = if self_param.is_some() { 2 } else { 1 };
                     let function_params_snippet = params.iter().enumerate().format_with(
                         ", ",
-                        |(index, (param_source, param)), f| {
-                            let name;
-                            let text;
-                            let (ref_, name) = match param_source {
-                                Either::Left(self_param) => (
-                                    match self_param.kind() {
-                                        ast::SelfParamKind::Owned => "",
-                                        ast::SelfParamKind::Ref => "&",
-                                        ast::SelfParamKind::MutRef => "&mut ",
-                                    },
-                                    "self",
-                                ),
-                                Either::Right(it) => {
-                                    let n = (|| {
-                                        let mut pat = it.pat()?;
-                                        loop {
-                                            match pat {
-                                                ast::Pat::IdentPat(pat) => break pat.name(),
-                                                ast::Pat::RefPat(it) => pat = it.pat()?,
-                                                _ => return None,
-                                            }
-                                        }
-                                    })();
-                                    match n {
-                                        Some(n) => {
-                                            name = n;
-                                            text = name.text();
-                                            let text = text.as_str().trim_start_matches('_');
-                                            let ref_ = ref_of_param(ctx, text, param.ty());
-                                            (ref_, text)
-                                        }
-                                        None => ("", "_"),
-                                    }
-                                }
-                            };
-                            f(&format_args!("${{{}:{}{}}}", index + 1, ref_, name))
+                        |(index, param), f| match param.name(ctx.db) {
+                            Some(n) => {
+                                let smol_str = n.to_smol_str();
+                                let text = smol_str.as_str().trim_start_matches('_');
+                                let ref_ = ref_of_param(ctx, text, param.ty());
+                                f(&format_args!("${{{}:{}{}}}", index + offset, ref_, text))
+                            }
+                            None => f(&format_args!("${{{}:_}}", index + offset,)),
                         },
                     );
-                    format!("{}({})$0", name, function_params_snippet)
+                    match self_param {
+                        Some(self_param) => {
+                            format!(
+                                "{}(${{1:{}}}{}{})$0",
+                                name,
+                                self_param.display(ctx.db),
+                                if params.is_empty() { "" } else { ", " },
+                                function_params_snippet
+                            )
+                        }
+                        None => {
+                            format!("{}({})$0", name, function_params_snippet)
+                        }
+                    }
                 }
                 _ => {
                     cov_mark::hit!(suppress_arg_snippets);
diff --git a/crates/ide_completion/src/render/function.rs b/crates/ide_completion/src/render/function.rs
index 86dfbf7fc8f..918210f2f36 100644
--- a/crates/ide_completion/src/render/function.rs
+++ b/crates/ide_completion/src/render/function.rs
@@ -1,11 +1,9 @@
 //! Renderer for function calls.
 
-use either::Either;
-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},
@@ -42,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.
-    ast_node: ast::Fn,
     is_method: bool,
 }
 
@@ -69,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 ast_node = fn_.source(ctx.db())?.value;
 
-        Some(FunctionRender { ctx, name, receiver, func: fn_, ast_node, is_method })
+        Some(FunctionRender { ctx, name, receiver, func: fn_, is_method })
     }
 
     fn render(self, import_to_add: Option<ImportEdit>) -> CompletionItem {
@@ -152,25 +134,20 @@ impl<'a> FunctionRender<'a> {
     }
 
     fn params(&self) -> Params {
-        let ast_params = match self.ast_node.param_list() {
-            Some(it) => it,
-            None => return Params::Named(Vec::new()),
-        };
-        let params = ast_params.params().map(Either::Right);
-
-        let params = if self.ctx.completion.has_dot_receiver() || self.receiver.is_some() {
-            params.zip(self.func.method_params(self.ctx.db()).unwrap_or_default()).collect()
-        } else {
-            ast_params
-                .self_param()
-                .map(Either::Left)
-                .into_iter()
-                .chain(params)
-                .zip(self.func.assoc_fn_params(self.ctx.db()))
-                .collect()
-        };
+        let (params, self_param) =
+            if self.ctx.completion.has_dot_receiver() || self.receiver.is_some() {
+                (self.func.method_params(self.ctx.db()).unwrap_or_default(), 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_param.is_some() {
+                    assoc_params.remove(0);
+                }
+                (assoc_params, self_param)
+            };
 
-        Params::Named(params)
+        Params::Named(self_param, params)
     }
 
     fn kind(&self) -> CompletionItemKind {