Analyze completion site re: trailing parens (#680)

* Analyze completion site re: trailing parens

* Start dismantling measures that just aided development

* More parallel with the other branch

* Get inspired by the less nested approach in PR #369

* Must have been leftover from an experiment

* Add tests

* Be consistent

* Try to get a bit more info on the Windows test failure

* Flatten this out too

* Not my problem to solve: Revert "Try to get a bit more info on the Windows test failure"

This reverts commit dea921d.

This is unrelated to this PR and is being tracked in #678.

* Remove rest of temporary logging

* Extract out `node_find_containing_call()`

* Rework on top of `parameter_hints()` idea

* Add `ParameterHints` enum

And fix a bug with imports environment functions not respecting `parameter_hints`

* Move tests to `parameter_hints.rs`

It seems obvious to me that we are testing a feature that "officially" lives here now

* Tweak examples probed in tests

* Add comment

* Use original name of function

---------

Co-authored-by: Davis Vaughan <[email protected]>

Author: jennybc

.vscode/launch.json

@@ -14,7 +14,8 @@
             },
             "windows": {
                 "program": "ark.exe"
-            }
+            },
+            "sourceLanguages": ["rust"]
         },
         {
             "type": "lldb",

crates/ark/src/lsp/completions.rs

@@ -6,6 +6,7 @@
 //
 
 mod completion_item;
+mod parameter_hints;
 mod provide;
 mod resolve;
 mod sources;

crates/ark/src/lsp/completions/completion_item.rs

@@ -40,6 +40,7 @@ use tower_lsp::lsp_types::Range;
 use tower_lsp::lsp_types::TextEdit;
 use tree_sitter::Node;
 
+use crate::lsp::completions::parameter_hints::ParameterHints;
 use crate::lsp::completions::types::CompletionData;
 use crate::lsp::completions::types::PromiseStrategy;
 use crate::lsp::document_context::DocumentContext;
@@ -169,6 +170,7 @@ pub(super) unsafe fn completion_item_from_package(
 pub(super) fn completion_item_from_function(
     name: &str,
     package: Option<&str>,
+    parameter_hints: ParameterHints,
 ) -> Result<CompletionItem> {
     let label = format!("{}", name);
     let mut item = completion_item(label, CompletionData::Function {
@@ -182,17 +184,23 @@ pub(super) fn completion_item_from_function(
     item.label_details = Some(label_details);
 
     let insert_text = sym_quote_invalid(name);
-    item.insert_text_format = Some(InsertTextFormat::SNIPPET);
-    item.insert_text = Some(format!("{insert_text}($0)"));
 
-    // provide parameter completions after completing function
-    item.command = Some(Command {
-        title: "Trigger Parameter Hints".to_string(),
-        command: "editor.action.triggerParameterHints".to_string(),
-        ..Default::default()
-    });
+    if parameter_hints.is_enabled() {
+        item.insert_text_format = Some(InsertTextFormat::SNIPPET);
+        item.insert_text = Some(format!("{insert_text}($0)"));
 
-    return Ok(item);
+        // provide parameter completions after completing function
+        item.command = Some(Command {
+            title: "Trigger Parameter Hints".to_string(),
+            command: "editor.action.triggerParameterHints".to_string(),
+            ..Default::default()
+        });
+    } else {
+        item.insert_text_format = Some(InsertTextFormat::PLAIN_TEXT);
+        item.insert_text = Some(insert_text);
+    }
+
+    Ok(item)
 }
 
 fn item_details(package: Option<&str>) -> CompletionItemLabelDetails {
@@ -245,9 +253,17 @@ pub(super) unsafe fn completion_item_from_object(
     envir: SEXP,
     package: Option<&str>,
     promise_strategy: PromiseStrategy,
+    parameter_hints: ParameterHints,
 ) -> Result<CompletionItem> {
     if r_typeof(object) == PROMSXP {
-        return completion_item_from_promise(name, object, envir, package, promise_strategy);
+        return completion_item_from_promise(
+            name,
+            object,
+            envir,
+            package,
+            promise_strategy,
+            parameter_hints,
+        );
     }
 
     // TODO: For some functions (e.g. S4 generics?) the help file might be
@@ -256,7 +272,7 @@ pub(super) unsafe fn completion_item_from_object(
     // In other words, when creating a completion item for these functions,
     // we should also figure out where we can receive the help from.
     if Rf_isFunction(object) != 0 {
-        return completion_item_from_function(name, package);
+        return completion_item_from_function(name, package, parameter_hints);
     }
 
     let mut item = completion_item(name, CompletionData::Object {
@@ -287,12 +303,20 @@ pub(super) unsafe fn completion_item_from_promise(
     envir: SEXP,
     package: Option<&str>,
     promise_strategy: PromiseStrategy,
+    parameter_hints: ParameterHints,
 ) -> Result<CompletionItem> {
     if r_promise_is_forced(object) {
         // Promise has already been evaluated before.
         // Generate completion item from underlying value.
         let object = PRVALUE(object);
-        return completion_item_from_object(name, object, envir, package, promise_strategy);
+        return completion_item_from_object(
+            name,
+            object,
+            envir,
+            package,
+            promise_strategy,
+            parameter_hints,
+        );
     }
 
     if promise_strategy == PromiseStrategy::Force && r_promise_is_lazy_load_binding(object) {
@@ -302,7 +326,14 @@ pub(super) unsafe fn completion_item_from_promise(
         // important for functions, where we also set a `CompletionItem::command()`
         // to display function signature help after the completion.
         let object = r_promise_force_with_rollback(object)?;
-        return completion_item_from_object(name, object.sexp, envir, package, promise_strategy);
+        return completion_item_from_object(
+            name,
+            object.sexp,
+            envir,
+            package,
+            promise_strategy,
+            parameter_hints,
+        );
     }
 
     // Otherwise we never want to force promises, so we return a fairly
@@ -342,19 +373,28 @@ pub(super) unsafe fn completion_item_from_namespace(
     name: &str,
     namespace: SEXP,
     package: &str,
+    parameter_hints: ParameterHints,
 ) -> Result<CompletionItem> {
     // First, look in the namespace itself.
-    if let Some(item) =
-        completion_item_from_symbol(name, namespace, Some(package), PromiseStrategy::Force)
-    {
+    if let Some(item) = completion_item_from_symbol(
+        name,
+        namespace,
+        Some(package),
+        PromiseStrategy::Force,
+        parameter_hints,
+    ) {
         return item;
     }
 
     // Otherwise, try the imports environment.
     let imports = ENCLOS(namespace);
-    if let Some(item) =
-        completion_item_from_symbol(name, imports, Some(package), PromiseStrategy::Force)
-    {
+    if let Some(item) = completion_item_from_symbol(
+        name,
+        imports,
+        Some(package),
+        PromiseStrategy::Force,
+        parameter_hints,
+    ) {
         return item;
     }
 
@@ -376,7 +416,10 @@ pub(super) unsafe fn completion_item_from_lazydata(
     // long time to load.
     let promise_strategy = PromiseStrategy::Simple;
 
-    match completion_item_from_symbol(name, env, Some(package), promise_strategy) {
+    // Lazydata objects are never functions, so this doesn't really matter
+    let parameter_hints = ParameterHints::Enabled;
+
+    match completion_item_from_symbol(name, env, Some(package), promise_strategy, parameter_hints) {
         Some(item) => item,
         None => {
             // Should be impossible, but we'll be extra safe
@@ -390,6 +433,7 @@ pub(super) unsafe fn completion_item_from_symbol(
     envir: SEXP,
     package: Option<&str>,
     promise_strategy: PromiseStrategy,
+    parameter_hints: ParameterHints,
 ) -> Option<Result<CompletionItem>> {
     let symbol = r_symbol!(name);
 
@@ -422,6 +466,7 @@ pub(super) unsafe fn completion_item_from_symbol(
         envir,
         package,
         promise_strategy,
+        parameter_hints,
     ))
 }