Quote module/function/record names in completion, signature help (#71)

Summary:
This improves the display of signature help and fixes label (and edit) values for completions of functions or record names that need quoting, for example `#'basic.publish'{}` from the Rabbit codebase or calling a function on an Elixir module from Erlang.

Also included is a refactor of `base_db::to_quoted_string` to return `Cow<str>` instead, avoiding allocation in the common case that an identifier doesn't need quoting. This saves a few unnecessary allocations.

Pull Request resolved: #71

Reviewed By: michalmuskala

Differential Revision: D67380119

Pulled By: robertoaloi

fbshipit-source-id: 9ecea6b5bd0fff0f127a3a78d25b63f07d8274cf

crates/base_db/src/lib.rs

@@ -7,6 +7,7 @@
  * of this source tree.
  */
 
+use std::borrow::Cow;
 use std::sync::Arc;
 
 use elp_project_model::AppName;
@@ -496,15 +497,15 @@ impl<T: SourceDatabaseExt> FileLoader for FileLoaderDelegate<&'_ T> {
 
 /// If the `input` string represents an atom, and needs quoting, quote
 /// it.
-pub fn to_quoted_string(input: &str) -> String {
+pub fn to_quoted_string(input: &str) -> Cow<str> {
     fn is_valid_atom(input: &str) -> bool {
         let mut chars = input.chars();
         chars.next().map_or(false, |c| c.is_lowercase())
             && chars.all(|c| char::is_alphanumeric(c) || c == '_' || c == '@')
     }
     if is_valid_atom(input) {
-        input.to_string()
+        Cow::Borrowed(input)
     } else {
-        format!("'{}'", &input)
+        Cow::Owned(format!("'{}'", &input))
     }
 }

crates/base_db/src/module_index.rs

@@ -8,6 +8,7 @@
  */
 
 use std::borrow::Borrow;
+use std::borrow::Cow;
 use std::fmt;
 use std::hash::Hash;
 use std::ops::Deref;
@@ -32,7 +33,7 @@ impl ModuleName {
         self
     }
 
-    pub fn to_quoted_string(&self) -> String {
+    pub fn to_quoted_string(&self) -> Cow<str> {
         to_quoted_string(self.as_str())
     }
 }

crates/hir/src/name.rs

@@ -67,9 +67,9 @@ impl Name {
         &self.0
     }
 
-    pub fn to_quoted_string(&self) -> String {
+    pub fn to_quoted_string(&self) -> Cow<str> {
         if self == &Self::MISSING {
-            self.to_string()
+            Cow::Borrowed(self.as_str())
         } else {
             to_quoted_string(self.as_str())
         }

crates/ide/src/codemod_helpers.rs

@@ -375,8 +375,8 @@ impl MFA {
         let call_module = call_target.module?;
         let na = call_target.name;
         Some(MFA {
-            module: call_module.to_quoted_string(),
-            name: na.name().to_quoted_string(),
+            module: call_module.to_quoted_string().into_owned(),
+            name: na.name().to_quoted_string().into_owned(),
             arity: na.arity(),
         })
     }

crates/ide/src/signature_help.rs

@@ -215,8 +215,13 @@ fn build_signature_help(
         active_parameter,
     };
     match &module_name {
-        Some(m) => format_to!(help.signature, "{m}:{fun_name}("),
-        None => format_to!(help.signature, "{fun_name}("),
+        Some(m) => format_to!(
+            help.signature,
+            "{}:{}(",
+            m.to_quoted_string(),
+            fun_name.to_quoted_string()
+        ),
+        None => format_to!(help.signature, "{}(", fun_name.to_quoted_string()),
     }
     if let Some(parameters) = def.arg_names(db) {
         for parameter in parameters {
@@ -527,6 +532,48 @@ main() ->
         );
     }
 
+    #[test]
+    fn test_fn_signature_quoted_remote_two_args() {
+        check(
+            r#"
+//- /Elixir.One.erl
+-module('Elixir.One').
+
+-compile(export_all).
+
+-spec add(integer(), integer()) -> integer().
+add(This, That) ->
+  add(This, That, 0).
+
+-spec add(integer(), integer(), integer()) -> integer().
+add(This, That, Extra) ->
+  This + That + Extra.
+
+//- /two.erl
+-module(two).
+
+main() ->
+  'Elixir.One':add(~, That).
+"#,
+            expect![[r#"
+                ```erlang
+                -spec add(integer(), integer()) -> integer().
+                ```
+                ------
+                'Elixir.One':add(This, That)
+                                 ^^^^  ----
+                ======
+                ```erlang
+                -spec add(integer(), integer(), integer()) -> integer().
+                ```
+                ------
+                'Elixir.One':add(This, That, Extra)
+                                 ^^^^  ----  -----
+                ======
+            "#]],
+        );
+    }
+
     // Due to the way the current grammar currently works, this is
     // currently not returning any results since the cursor is not
     // identified as part of the EXPR_ARGS.

crates/ide_completion/src/functions.rs

@@ -143,7 +143,7 @@ pub(crate) fn add_completions(
                 .get_functions_in_scope()
                 .filter(|(na, _)| na.name().starts_with(function_prefix.text()))
                 .filter_map(|(na, module)| {
-                    let function_name = na.name();
+                    let function_name = na.name().to_quoted_string();
                     let module_file_id = module
                         .and_then(|module| {
                             Some(
@@ -167,7 +167,7 @@ pub(crate) fn add_completions(
                             let contents = helpers::function_contents(
                                 sema.db.upcast(),
                                 &def,
-                                function_name,
+                                &function_name,
                                 helpers::should_include_args(next_token),
                             )?;
                             Some(Completion {
@@ -935,4 +935,20 @@ foo(X, Y) -> ok.
             expect!["{label:is_internal/1, kind:Function, contents:SameAsLabel, position:None}"],
         );
     }
+
+    #[test]
+    fn test_quoted_local_call() {
+        check(
+            r#"
+    -module(sample).
+    test() ->
+        fo~(something).
+    'foo.bar'(X) -> ok.
+            "#,
+            None,
+            expect![[
+                r#"{label:'foo.bar'/1, kind:Function, contents:Snippet("'foo.bar'"), position:Some(FilePosition { file_id: FileId(0), offset: 46 })}"#
+            ]],
+        );
+    }
 }

crates/ide_completion/src/helpers.rs

@@ -109,7 +109,7 @@ pub(crate) fn name_arity_to_call_completion(
 
     if na.name().starts_with(prefix) {
         let contents = def.map_or(Some(format_call(na.name(), na.arity())), |def| {
-            function_contents(db, def, na.name(), include_args)
+            function_contents(db, def, &na.name().to_quoted_string(), include_args)
         })?;
         Some(Completion {
             label: na.to_string(),

crates/ide_completion/src/records.rs

@@ -83,7 +83,7 @@ fn add_token_based_completions(
             .iter()
             .filter(|(name, _)| name.starts_with(name_prefix))
             .map(|(name, _)| Completion {
-                label: name.to_string(),
+                label: name.to_quoted_string().into_owned(),
                 kind: Kind::Record,
                 contents: Contents::SameAsLabel,
                 position: None,
@@ -335,6 +335,23 @@ mod test {
         );
     }
 
+    #[test]
+    fn test_quoted_record_name() {
+        // Irregular names are quoted.
+        check(
+            r#"
+        -module(sample).
+        -record('this.record', {field1=1, field2=2}).
+        -record('that$record', {}).
+        foo(X) -> #~
+        "#,
+            None,
+            expect![[r#"
+                {label:'that$record', kind:Record, contents:SameAsLabel, position:None}
+                {label:'this.record', kind:Record, contents:SameAsLabel, position:None}"#]],
+        );
+    }
+
     #[test]
     fn test_record_error_recovery() {
         check(