Merge #5415

5415: Remove FunctionSignature r=matklad a=matklad



bors r+
🤖

Co-authored-by: Aleksey Kladov <[email protected]>

7 files changed, 87 insertions, 379 deletions

diff --git a/crates/ra_hir/src/code_model.rs b/crates/ra_hir/src/code_model.rs
index eb6a14eda..0f6953158 100644
--- a/crates/ra_hir/src/code_model.rs
+++ b/crates/ra_hir/src/code_model.rs
@@ -1552,6 +1552,9 @@ impl Callable {
         let param_list = src.value.param_list()?;
         param_list.self_param()
     }
+    pub fn n_params(&self) -> usize {
+        self.sig.params().len() - if self.is_bound_method { 1 } else { 0 }
+    }
     pub fn params(
         &self,
         db: &dyn HirDatabase,
diff --git a/crates/ra_ide/src/completion/complete_trait_impl.rs b/crates/ra_ide/src/completion/complete_trait_impl.rs
index 90f5b1c25..cf716540f 100644
--- a/crates/ra_ide/src/completion/complete_trait_impl.rs
+++ b/crates/ra_ide/src/completion/complete_trait_impl.rs
@@ -43,7 +43,7 @@ use crate::{
     completion::{
         CompletionContext, CompletionItem, CompletionItemKind, CompletionKind, Completions,
     },
-    display::function_signature::FunctionSignature,
+    display::function_declaration,
 };
 
 pub(crate) fn complete_trait_impl(acc: &mut Completions, ctx: &CompletionContext) {
@@ -125,8 +125,6 @@ fn add_function_impl(
     ctx: &CompletionContext,
     func: hir::Function,
 ) {
-    let signature = FunctionSignature::from_hir(ctx.db, func);
-
     let fn_name = func.name(ctx.db).to_string();
 
     let label = if !func.params(ctx.db).is_empty() {
@@ -146,13 +144,14 @@ fn add_function_impl(
     };
     let range = TextRange::new(fn_def_node.text_range().start(), ctx.source_range().end());
 
+    let function_decl = function_declaration(&func.source(ctx.db).value);
     match ctx.config.snippet_cap {
         Some(cap) => {
-            let snippet = format!("{} {{\n    $0\n}}", signature);
+            let snippet = format!("{} {{\n    $0\n}}", function_decl);
             builder.snippet_edit(cap, TextEdit::replace(range, snippet))
         }
         None => {
-            let header = format!("{} {{", signature);
+            let header = format!("{} {{", function_decl);
             builder.text_edit(TextEdit::replace(range, header))
         }
     }
diff --git a/crates/ra_ide/src/completion/presentation.rs b/crates/ra_ide/src/completion/presentation.rs
index e29b82017..c7b74e635 100644
--- a/crates/ra_ide/src/completion/presentation.rs
+++ b/crates/ra_ide/src/completion/presentation.rs
@@ -11,7 +11,7 @@ use crate::{
         completion_item::Builder, CompletionContext, CompletionItem, CompletionItemKind,
         CompletionKind, Completions,
     },
-    display::{const_label, function_signature::FunctionSignature, macro_label, type_label},
+    display::{const_label, function_declaration, macro_label, type_label},
     CompletionScore, RootDatabase,
 };
 
@@ -195,7 +195,6 @@ impl Completions {
 
         let name = local_name.unwrap_or_else(|| func.name(ctx.db).to_string());
         let ast_node = func.source(ctx.db).value;
-        let function_signature = FunctionSignature::from(&ast_node);
 
         let mut builder =
             CompletionItem::new(CompletionKind::Reference, ctx.source_range(), name.clone())
@@ -206,13 +205,14 @@ impl Completions {
                 })
                 .set_documentation(func.docs(ctx.db))
                 .set_deprecated(is_deprecated(func, ctx.db))
-                .detail(function_signature.to_string());
+                .detail(function_declaration(&ast_node));
 
-        let params = function_signature
-            .parameter_names
-            .iter()
-            .skip(if function_signature.has_self_param { 1 } else { 0 })
-            .map(|name| name.trim_start_matches('_').into())
+        let params = ast_node
+            .param_list()
+            .into_iter()
+            .flat_map(|it| it.params())
+            .flat_map(|it| it.pat())
+            .map(|pat| pat.to_string().trim_start_matches('_').into())
             .collect();
 
         builder = builder.add_call_parens(ctx, name, Params::Named(params));
diff --git a/crates/ra_ide/src/display.rs b/crates/ra_ide/src/display.rs
index 1ec946369..6d4151dd8 100644
--- a/crates/ra_ide/src/display.rs
+++ b/crates/ra_ide/src/display.rs
@@ -1,7 +1,6 @@
 //! This module contains utilities for turning SyntaxNodes and HIR types
 //! into types that may be used to render in a UI.
 
-pub(crate) mod function_signature;
 mod navigation_target;
 mod short_label;
 
@@ -10,13 +9,49 @@ use ra_syntax::{
     SyntaxKind::{ATTR, COMMENT},
 };
 
-pub(crate) use navigation_target::{ToNav, TryToNav};
-pub(crate) use short_label::ShortLabel;
+use ast::VisibilityOwner;
+use stdx::format_to;
 
 pub use navigation_target::NavigationTarget;
+pub(crate) use navigation_target::{ToNav, TryToNav};
+pub(crate) use short_label::ShortLabel;
 
-pub(crate) fn function_label(node: &ast::FnDef) -> String {
-    function_signature::FunctionSignature::from(node).to_string()
+pub(crate) fn function_declaration(node: &ast::FnDef) -> String {
+    let mut buf = String::new();
+    if let Some(vis) = node.visibility() {
+        format_to!(buf, "{} ", vis);
+    }
+    if node.async_token().is_some() {
+        format_to!(buf, "async ");
+    }
+    if node.const_token().is_some() {
+        format_to!(buf, "const ");
+    }
+    if node.unsafe_token().is_some() {
+        format_to!(buf, "unsafe ");
+    }
+    if let Some(abi) = node.abi() {
+        // Keyword `extern` is included in the string.
+        format_to!(buf, "{} ", abi);
+    }
+    if let Some(name) = node.name() {
+        format_to!(buf, "fn {}", name)
+    }
+    if let Some(type_params) = node.type_param_list() {
+        format_to!(buf, "{}", type_params);
+    }
+    if let Some(param_list) = node.param_list() {
+        format_to!(buf, "{}", param_list);
+    }
+    if let Some(ret_type) = node.ret_type() {
+        if ret_type.type_ref().is_some() {
+            format_to!(buf, " {}", ret_type);
+        }
+    }
+    if let Some(where_clause) = node.where_clause() {
+        format_to!(buf, "\n{}", where_clause);
+    }
+    buf
 }
 
 pub(crate) fn const_label(node: &ast::ConstDef) -> String {
@@ -41,23 +76,6 @@ pub(crate) fn type_label(node: &ast::TypeAliasDef) -> String {
     label.trim().to_owned()
 }
 
-pub(crate) fn generic_parameters<N: TypeParamsOwner>(node: &N) -> Vec<String> {
-    let mut res = vec![];
-    if let Some(type_params) = node.type_param_list() {
-        res.extend(type_params.lifetime_params().map(|p| p.syntax().text().to_string()));
-        res.extend(type_params.type_params().map(|p| p.syntax().text().to_string()));
-    }
-    res
-}
-
-pub(crate) fn where_predicates<N: TypeParamsOwner>(node: &N) -> Vec<String> {
-    let mut res = vec![];
-    if let Some(clause) = node.where_clause() {
-        res.extend(clause.predicates().map(|p| p.syntax().text().to_string()));
-    }
-    res
-}
-
 pub(crate) fn macro_label(node: &ast::MacroCall) -> String {
     let name = node.name().map(|name| name.syntax().text().to_string()).unwrap_or_default();
     let vis = if node.has_atom_attr("macro_export") { "#[macro_export]\n" } else { "" };
diff --git a/crates/ra_ide/src/display/function_signature.rs b/crates/ra_ide/src/display/function_signature.rs
deleted file mode 100644
index 9b7220d1f..000000000
--- a/crates/ra_ide/src/display/function_signature.rs
+++ /dev/null
@@ -1,298 +0,0 @@
-//! FIXME: write short doc here
-
-// FIXME: this modules relies on strings and AST way too much, and it should be
-// rewritten (matklad 2020-05-07)
-use std::{
-    convert::From,
-    fmt::{self, Display},
-};
-
-use hir::{Docs, Documentation, HasSource, HirDisplay};
-use ra_ide_db::RootDatabase;
-use ra_syntax::ast::{self, AstNode, NameOwner, VisibilityOwner};
-use stdx::{split_delim, SepBy};
-
-use crate::display::{generic_parameters, where_predicates};
-
-#[derive(Debug)]
-pub(crate) enum CallableKind {
-    Function,
-    StructConstructor,
-    VariantConstructor,
-}
-
-/// Contains information about a function signature
-#[derive(Debug)]
-pub(crate) struct FunctionSignature {
-    pub(crate) kind: CallableKind,
-    /// Optional visibility
-    pub(crate) visibility: Option<String>,
-    /// Qualifiers like `async`, `unsafe`, ...
-    pub(crate) qualifier: FunctionQualifier,
-    /// Name of the function
-    pub(crate) name: Option<String>,
-    /// Documentation for the function
-    pub(crate) doc: Option<Documentation>,
-    /// Generic parameters
-    pub(crate) generic_parameters: Vec<String>,
-    /// Parameters of the function
-    pub(crate) parameters: Vec<String>,
-    /// Parameter names of the function
-    pub(crate) parameter_names: Vec<String>,
-    /// Parameter types of the function
-    pub(crate) parameter_types: Vec<String>,
-    /// Optional return type
-    pub(crate) ret_type: Option<String>,
-    /// Where predicates
-    pub(crate) where_predicates: Vec<String>,
-    /// Self param presence
-    pub(crate) has_self_param: bool,
-}
-
-#[derive(Debug, Default)]
-pub(crate) struct FunctionQualifier {
-    // `async` and `const` are mutually exclusive. Do we need to enforcing it here?
-    pub(crate) is_async: bool,
-    pub(crate) is_const: bool,
-    pub(crate) is_unsafe: bool,
-    /// The string `extern ".."`
-    pub(crate) extern_abi: Option<String>,
-}
-
-impl FunctionSignature {
-    pub(crate) fn from_hir(db: &RootDatabase, function: hir::Function) -> Self {
-        let ast_node = function.source(db).value;
-        let mut res = FunctionSignature::from(&ast_node);
-        res.doc = function.docs(db);
-        res
-    }
-
-    pub(crate) fn from_struct(db: &RootDatabase, st: hir::Struct) -> Option<Self> {
-        let node: ast::StructDef = st.source(db).value;
-        if let ast::StructKind::Record(_) = node.kind() {
-            return None;
-        };
-
-        let mut params = vec![];
-        let mut parameter_types = vec![];
-        for field in st.fields(db).into_iter() {
-            let ty = field.signature_ty(db);
-            let raw_param = format!("{}", ty.display(db));
-
-            if let Some(param_type) = raw_param.split(':').nth(1).and_then(|it| it.get(1..)) {
-                parameter_types.push(param_type.to_string());
-            } else {
-                // useful when you have tuple struct
-                parameter_types.push(raw_param.clone());
-            }
-            params.push(raw_param);
-        }
-
-        Some(FunctionSignature {
-            kind: CallableKind::StructConstructor,
-            visibility: node.visibility().map(|n| n.syntax().text().to_string()),
-            // Do we need `const`?
-            qualifier: Default::default(),
-            name: node.name().map(|n| n.text().to_string()),
-            ret_type: node.name().map(|n| n.text().to_string()),
-            parameters: params,
-            parameter_names: vec![],
-            parameter_types,
-            generic_parameters: generic_parameters(&node),
-            where_predicates: where_predicates(&node),
-            doc: st.docs(db),
-            has_self_param: false,
-        })
-    }
-
-    pub(crate) fn from_enum_variant(db: &RootDatabase, variant: hir::EnumVariant) -> Option<Self> {
-        let node: ast::EnumVariant = variant.source(db).value;
-        match node.kind() {
-            ast::StructKind::Record(_) | ast::StructKind::Unit => return None,
-            _ => (),
-        };
-
-        let parent_name = variant.parent_enum(db).name(db).to_string();
-
-        let name = format!("{}::{}", parent_name, variant.name(db));
-
-        let mut params = vec![];
-        let mut parameter_types = vec![];
-        for field in variant.fields(db).into_iter() {
-            let ty = field.signature_ty(db);
-            let raw_param = format!("{}", ty.display(db));
-            if let Some(param_type) = raw_param.split(':').nth(1).and_then(|it| it.get(1..)) {
-                parameter_types.push(param_type.to_string());
-            } else {
-                // The unwrap_or_else is useful when you have tuple
-                parameter_types.push(raw_param);
-            }
-            let name = field.name(db);
-
-            params.push(format!("{}: {}", name, ty.display(db)));
-        }
-
-        Some(FunctionSignature {
-            kind: CallableKind::VariantConstructor,
-            visibility: None,
-            // Do we need `const`?
-            qualifier: Default::default(),
-            name: Some(name),
-            ret_type: None,
-            parameters: params,
-            parameter_names: vec![],
-            parameter_types,
-            generic_parameters: vec![],
-            where_predicates: vec![],
-            doc: variant.docs(db),
-            has_self_param: false,
-        })
-    }
-}
-
-impl From<&'_ ast::FnDef> for FunctionSignature {
-    fn from(node: &ast::FnDef) -> FunctionSignature {
-        fn param_list(node: &ast::FnDef) -> (bool, Vec<String>, Vec<String>) {
-            let mut res = vec![];
-            let mut res_types = vec![];
-            let mut has_self_param = false;
-            if let Some(param_list) = node.param_list() {
-                if let Some(self_param) = param_list.self_param() {
-                    has_self_param = true;
-                    let raw_param = self_param.syntax().text().to_string();
-
-                    res_types.push(
-                        raw_param
-                            .split(':')
-                            .nth(1)
-                            .and_then(|it| it.get(1..))
-                            .unwrap_or_else(|| "Self")
-                            .to_string(),
-                    );
-                    res.push(raw_param);
-                }
-
-                // macro-generated functions are missing whitespace
-                fn fmt_param(param: ast::Param) -> String {
-                    let text = param.syntax().text().to_string();
-                    match split_delim(&text, ':') {
-                        Some((left, right)) => format!("{}: {}", left.trim(), right.trim()),
-                        _ => text,
-                    }
-                }
-
-                res.extend(param_list.params().map(fmt_param));
-                res_types.extend(param_list.params().map(|param| {
-                    let param_text = param.syntax().text().to_string();
-                    match param_text.split(':').nth(1).and_then(|it| it.get(1..)) {
-                        Some(it) => it.to_string(),
-                        None => param_text,
-                    }
-                }));
-            }
-            (has_self_param, res, res_types)
-        }
-
-        fn param_name_list(node: &ast::FnDef) -> Vec<String> {
-            let mut res = vec![];
-            if let Some(param_list) = node.param_list() {
-                if let Some(self_param) = param_list.self_param() {
-                    res.push(self_param.syntax().text().to_string())
-                }
-
-                res.extend(
-                    param_list
-                        .params()
-                        .map(|param| {
-                            Some(
-                                param
-                                    .pat()?
-                                    .syntax()
-                                    .descendants()
-                                    .find_map(ast::Name::cast)?
-                                    .text()
-                                    .to_string(),
-                            )
-                        })
-                        .map(|param| param.unwrap_or_default()),
-                );
-            }
-            res
-        }
-
-        let (has_self_param, parameters, parameter_types) = param_list(node);
-
-        FunctionSignature {
-            kind: CallableKind::Function,
-            visibility: node.visibility().map(|n| n.syntax().text().to_string()),
-            qualifier: FunctionQualifier {
-                is_async: node.async_token().is_some(),
-                is_const: node.const_token().is_some(),
-                is_unsafe: node.unsafe_token().is_some(),
-                extern_abi: node.abi().map(|n| n.to_string()),
-            },
-            name: node.name().map(|n| n.text().to_string()),
-            ret_type: node
-                .ret_type()
-                .and_then(|r| r.type_ref())
-                .map(|n| n.syntax().text().to_string()),
-            parameters,
-            parameter_names: param_name_list(node),
-            parameter_types,
-            generic_parameters: generic_parameters(node),
-            where_predicates: where_predicates(node),
-            // docs are processed separately
-            doc: None,
-            has_self_param,
-        }
-    }
-}
-
-impl Display for FunctionSignature {
-    fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
-        if let Some(t) = &self.visibility {
-            write!(f, "{} ", t)?;
-        }
-
-        if self.qualifier.is_async {
-            write!(f, "async ")?;
-        }
-
-        if self.qualifier.is_const {
-            write!(f, "const ")?;
-        }
-
-        if self.qualifier.is_unsafe {
-            write!(f, "unsafe ")?;
-        }
-
-        if let Some(extern_abi) = &self.qualifier.extern_abi {
-            // Keyword `extern` is included in the string.
-            write!(f, "{} ", extern_abi)?;
-        }
-
-        if let Some(name) = &self.name {
-            match self.kind {
-                CallableKind::Function => write!(f, "fn {}", name)?,
-                CallableKind::StructConstructor => write!(f, "struct {}", name)?,
-                CallableKind::VariantConstructor => write!(f, "{}", name)?,
-            }
-        }
-
-        if !self.generic_parameters.is_empty() {
-            write!(f, "{}", self.generic_parameters.iter().sep_by(", ").surround_with("<", ">"))?;
-        }
-
-        write!(f, "{}", self.parameters.iter().sep_by(", ").surround_with("(", ")"))?;
-
-        if let Some(t) = &self.ret_type {
-            write!(f, " -> {}", t)?;
-        }
-
-        if !self.where_predicates.is_empty() {
-            write!(f, "\nwhere {}", self.where_predicates.iter().sep_by(",\n      "))?;
-        }
-
-        Ok(())
-    }
-}
diff --git a/crates/ra_ide/src/display/short_label.rs b/crates/ra_ide/src/display/short_label.rs
index d37260e96..5588130a1 100644
--- a/crates/ra_ide/src/display/short_label.rs
+++ b/crates/ra_ide/src/display/short_label.rs
@@ -9,7 +9,7 @@ pub(crate) trait ShortLabel {
 
 impl ShortLabel for ast::FnDef {
     fn short_label(&self) -> Option<String> {
-        Some(crate::display::function_label(self))
+        Some(crate::display::function_declaration(self))
     }
 }
 
diff --git a/crates/ra_ide/src/inlay_hints.rs b/crates/ra_ide/src/inlay_hints.rs
index ae5695f61..cec3b04e8 100644
--- a/crates/ra_ide/src/inlay_hints.rs
+++ b/crates/ra_ide/src/inlay_hints.rs
@@ -1,4 +1,4 @@
-use hir::{Adt, HirDisplay, Semantics, Type};
+use hir::{Adt, Callable, HirDisplay, Semantics, Type};
 use ra_ide_db::RootDatabase;
 use ra_prof::profile;
 use ra_syntax::{
@@ -7,7 +7,9 @@ use ra_syntax::{
 };
 use stdx::to_lower_snake_case;
 
-use crate::{display::function_signature::FunctionSignature, FileId};
+use crate::FileId;
+use ast::NameOwner;
+use either::Either;
 
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct InlayHintsConfig {
@@ -150,23 +152,26 @@ fn get_param_name_hints(
         _ => return None,
     };
 
-    let fn_signature = get_fn_signature(sema, &expr)?;
-    let n_params_to_skip =
-        if fn_signature.has_self_param && matches!(&expr, ast::Expr::MethodCallExpr(_)) {
-            1
-        } else {
-            0
-        };
-    let hints = fn_signature
-        .parameter_names
-        .iter()
-        .skip(n_params_to_skip)
+    let callable = get_callable(sema, &expr)?;
+    let hints = callable
+        .params(sema.db)
+        .into_iter()
         .zip(args)
-        .filter(|(param, arg)| should_show_param_name_hint(sema, &fn_signature, param, &arg))
+        .filter_map(|((param, _ty), arg)| match param? {
+            Either::Left(self_param) => Some((self_param.to_string(), arg)),
+            Either::Right(pat) => {
+                let param_name = match pat {
+                    ast::Pat::BindPat(it) => it.name()?.to_string(),
+                    it => it.to_string(),
+                };
+                Some((param_name, arg))
+            }
+        })
+        .filter(|(param_name, arg)| should_show_param_name_hint(sema, &callable, &param_name, &arg))
         .map(|(param_name, arg)| InlayHint {
             range: arg.syntax().text_range(),
             kind: InlayKind::ParameterHint,
-            label: param_name.into(),
+            label: param_name.to_string().into(),
         });
 
     acc.extend(hints);
@@ -250,28 +255,26 @@ fn should_not_display_type_hint(db: &RootDatabase, bind_pat: &ast::BindPat, pat_
 
 fn should_show_param_name_hint(
     sema: &Semantics<RootDatabase>,
-    fn_signature: &FunctionSignature,
+    callable: &Callable,
     param_name: &str,
     argument: &ast::Expr,
 ) -> bool {
     let param_name = param_name.trim_start_matches('_');
+    let fn_name = match callable.kind() {
+        hir::CallableKind::Function(it) => Some(it.name(sema.db).to_string()),
+        hir::CallableKind::TupleStruct(_) | hir::CallableKind::TupleEnumVariant(_) => None,
+    };
     if param_name.is_empty()
-        || Some(param_name) == fn_signature.name.as_ref().map(|s| s.trim_start_matches('_'))
+        || Some(param_name) == fn_name.as_ref().map(|s| s.trim_start_matches('_'))
         || is_argument_similar_to_param_name(sema, argument, param_name)
         || param_name.starts_with("ra_fixture")
     {
         return false;
     }
 
-    let parameters_len = if fn_signature.has_self_param {
-        fn_signature.parameters.len() - 1
-    } else {
-        fn_signature.parameters.len()
-    };
-
     // avoid displaying hints for common functions like map, filter, etc.
     // or other obvious words used in std
-    !(parameters_len == 1 && is_obvious_param(param_name))
+    !(callable.n_params() == 1 && is_obvious_param(param_name))
 }
 
 fn is_argument_similar_to_param_name(
@@ -318,27 +321,10 @@ fn is_obvious_param(param_name: &str) -> bool {
     param_name.len() == 1 || is_obvious_param_name
 }
 
-fn get_fn_signature(sema: &Semantics<RootDatabase>, expr: &ast::Expr) -> Option<FunctionSignature> {
+fn get_callable(sema: &Semantics<RootDatabase>, expr: &ast::Expr) -> Option<Callable> {
     match expr {
-        ast::Expr::CallExpr(expr) => {
-            // FIXME: Type::as_callable is broken for closures
-            let callable = sema.type_of_expr(&expr.expr()?)?.as_callable(sema.db)?;
-            match callable.kind() {
-                hir::CallableKind::Function(it) => {
-                    Some(FunctionSignature::from_hir(sema.db, it.into()))
-                }
-                hir::CallableKind::TupleStruct(it) => {
-                    FunctionSignature::from_struct(sema.db, it.into())
-                }
-                hir::CallableKind::TupleEnumVariant(it) => {
-                    FunctionSignature::from_enum_variant(sema.db, it.into())
-                }
-            }
-        }
-        ast::Expr::MethodCallExpr(expr) => {
-            let fn_def = sema.resolve_method_call(&expr)?;
-            Some(FunctionSignature::from_hir(sema.db, fn_def))
-        }
+        ast::Expr::CallExpr(expr) => sema.type_of_expr(&expr.expr()?)?.as_callable(sema.db),
+        ast::Expr::MethodCallExpr(expr) => sema.resolve_method_call_as_callable(expr),
         _ => None,
     }
 }
@@ -360,7 +346,7 @@ mod tests {
         let inlay_hints = analysis.inlay_hints(file_id, &config).unwrap();
         let actual =
             inlay_hints.into_iter().map(|it| (it.range, it.label.to_string())).collect::<Vec<_>>();
-        assert_eq!(expected, actual);
+        assert_eq!(expected, actual, "\nExpected:\n{:#?}\n\nActual:\n{:#?}", expected, actual);
     }
 
     fn check_expect(config: InlayHintsConfig, ra_fixture: &str, expect: Expect) {