From cd9d76e0caa84ed0f94a68a7da896687c090e2f7 Mon Sep 17 00:00:00 2001 From: Lukas Wirth Date: Mon, 20 Dec 2021 15:24:37 +0100 Subject: [PATCH] internal: Store function param names in ItemTree --- crates/hir/src/display.rs | 15 ++---- crates/hir/src/lib.rs | 8 +++- crates/hir_def/src/data.rs | 4 +- crates/hir_def/src/generics.rs | 2 +- crates/hir_def/src/item_tree.rs | 2 +- crates/hir_def/src/item_tree/lower.rs | 16 ++++++- crates/hir_def/src/item_tree/pretty.rs | 7 ++- crates/hir_ty/src/infer.rs | 2 +- crates/hir_ty/src/lower.rs | 2 +- .../ide_completion/src/render/builder_ext.rs | 45 +++++------------ crates/ide_completion/src/render/function.rs | 48 +++++-------------- 11 files changed, 60 insertions(+), 91 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 { + 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>, + pub params: Vec<(Option, Interned)>, pub ret_type: Interned, pub async_ret_type: Option>, 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), + Normal(Option, Interned), 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, ¶m, &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_ty/src/infer.rs b/crates/hir_ty/src/infer.rs index 1bc19323da9..3022291d9fe 100644 --- a/crates/hir_ty/src/infer.rs +++ b/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::>(); + data.params.iter().map(|(_, type_ref)| ctx.lower_ty(type_ref)).collect::>(); 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::>(); + let params = data.params.iter().map(|(_, tr)| ctx_params.lower_ty(tr)).collect::>(); 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 0c6cbf7cb2d..8f8bd1dfcda 100644 --- a/crates/ide_completion/src/render/builder_ext.rs +++ b/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, Vec<(ast::Param, hir::Param)>), + Named(Option, Vec), 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| { - 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; - 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 + offset, 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,)), }, ); 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 ) diff --git a/crates/ide_completion/src/render/function.rs b/crates/ide_completion/src/render/function.rs index 5a243cbdd32..918210f2f36 100644 --- a/crates/ide_completion/src/render/function.rs +++ b/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, 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, is_method: bool, } @@ -68,9 +52,8 @@ impl<'a> FunctionRender<'a> { is_method: bool, ) -> Option> { 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) -> CompletionItem { @@ -151,23 +134,18 @@ impl<'a> FunctionRender<'a> { } fn params(&self) -> Params { - let ast_params = match &self.param_list { - Some(it) => it, - None => return Params::Named(None, Vec::new()), - }; - let params = ast_params.params(); + 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 (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 mut assoc_params = self.func.assoc_fn_params(self.ctx.db()); - if self.func.self_param(self.ctx.db()).is_some() { - assoc_params.remove(0); - } - (params.zip(assoc_params).collect(), ast_params.self_param()) - }; + 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(self_param, params) }