Use a "function context" to create more responsive completions (#819)

* Use a "function context" to create more responsive completions

* Oops, I meant to keep this logging

* Consistency pass re: text edits

* Move `use` calls into `mod` blocks to avoid `#[cfg(test)]` proliferation (#826)

* Try to convince copilot to stop adding custom messages to assertions

* Handle usage that is technically a call, but still looks like a reference

Also modify the helper that post-processes `sort_text` to recognize when we've already applied an intentional prefix

* Add tooling around `sort_text` and work on the tests

* There's an existing helper that gets the job done

* Only mention filename

* Remove unnecessary `.clone()`

* Rename this node

Co-authored-by: Davis Vaughan <davis@rstudio.com>

* Explicitly handle case like `dplyr::@` + some renaming and refactoring

* Be more explicit about what sort of node we are prepared to handle

* Introduce `cursor_at_end` field into function context + more

The main substantive move here is adding a boolean `cursor_at_end` field into the function context. The new test re: an exact match for a 3-letter function like `dir()` is the real-world usage that made me realize we needed this.

Also implemented lots of feedback from @lionel-'s review at the same time.

* Fix this comment

* Prepare the way to use this test helper elsewhere and more flexibly

* Streamline namespace tests

`completions_from_namespace()` gets a new signature in this PR. Lets also streamline the tests with utilities similar to those in the function context tests.

---------

Co-authored-by: Davis Vaughan <davis@rstudio.com>

Author: jennybc

.github/copilot-instructions.md

@@ -7,3 +7,7 @@ For error messages and logging, prefer direct formatting syntax: `Err(anyhow!("M
 Use `log::trace!` instead of `log::debug!`.
 
 Use fully qualified result types (`anyhow::Result`) instead of importing them.
+
+When writing tests, prefer simple assertion macros without custom error messages:
+- Use `assert_eq!(actual, expected);` instead of `assert_eq!(actual, expected, "custom message");`
+- Use `assert!(condition);` instead of `assert!(condition, "custom message");`

crates/ark/src/lsp/completions.rs

@@ -7,11 +7,14 @@
 
 mod completion_context;
 mod completion_item;
-mod parameter_hints;
+mod function_context;
 mod provide;
 mod resolve;
 mod sources;
 mod types;
 
+#[cfg(test)]
+mod tests;
+
 pub(crate) use provide::provide_completions;
 pub(crate) use resolve::resolve_completion;

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

@@ -9,50 +9,39 @@ use std::cell::OnceCell;
 
 use tree_sitter::Node;
 
-use crate::lsp::completions::parameter_hints::ParameterHints;
-use crate::lsp::completions::parameter_hints::{self};
+use crate::lsp::completions::function_context::FunctionContext;
 use crate::lsp::completions::sources::composite::pipe::find_pipe_root;
 use crate::lsp::completions::sources::composite::pipe::PipeRoot;
 use crate::lsp::document_context::DocumentContext;
 use crate::lsp::state::WorldState;
 use crate::treesitter::node_find_containing_call;
-
 pub(crate) struct CompletionContext<'a> {
     pub(crate) document_context: &'a DocumentContext<'a>,
     pub(crate) state: &'a WorldState,
-    parameter_hints_cell: OnceCell<ParameterHints>,
     pipe_root_cell: OnceCell<Option<PipeRoot>>,
     containing_call_cell: OnceCell<Option<Node<'a>>>,
+    function_context_cell: OnceCell<FunctionContext>,
 }
 
 impl<'a> CompletionContext<'a> {
     pub fn new(document_context: &'a DocumentContext, state: &'a WorldState) -> Self {
         Self {
             document_context,
             state,
-            parameter_hints_cell: OnceCell::new(),
             pipe_root_cell: OnceCell::new(),
             containing_call_cell: OnceCell::new(),
+            function_context_cell: OnceCell::new(),
         }
     }
 
-    pub fn parameter_hints(&self) -> &ParameterHints {
-        self.parameter_hints_cell.get_or_init(|| {
-            parameter_hints::parameter_hints(
-                self.document_context.node,
-                &self.document_context.document.contents,
-            )
-        })
-    }
-
     pub fn pipe_root(&self) -> Option<PipeRoot> {
         let call_node = self.containing_call_node();
 
         self.pipe_root_cell
             .get_or_init(|| match find_pipe_root(self.document_context, call_node) {
                 Ok(root) => root,
                 Err(e) => {
-                    log::error!("Error trying to find pipe root: {e}");
+                    log::trace!("Error trying to find pipe root: {e}");
                     None
                 },
             })
@@ -64,4 +53,9 @@ impl<'a> CompletionContext<'a> {
             .containing_call_cell
             .get_or_init(|| node_find_containing_call(self.document_context.node))
     }
+
+    pub fn function_context(&self) -> &FunctionContext {
+        self.function_context_cell
+            .get_or_init(|| FunctionContext::new(&self.document_context))
+    }
 }

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

@@ -37,7 +37,9 @@ 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::function_context::ArgumentsStatus;
+use crate::lsp::completions::function_context::FunctionContext;
+use crate::lsp::completions::function_context::FunctionRefUsage;
 use crate::lsp::completions::types::CompletionData;
 use crate::lsp::completions::types::PromiseStrategy;
 use crate::lsp::document_context::DocumentContext;
@@ -119,6 +121,8 @@ pub(super) fn completion_item_from_assignment(
     item.documentation = Some(Documentation::MarkupContent(markup));
     item.kind = Some(CompletionItemKind::VARIABLE);
 
+    // TODO: This ad hoc completion item construction for a function does not
+    // benefit from the logic in completion_item_from_function() :(
     if rhs.node_type() == NodeType::FunctionDefinition {
         if let Some(parameters) = rhs.child_by_field_name("parameters") {
             let parameters = context
@@ -167,36 +171,125 @@ pub(super) unsafe fn completion_item_from_package(
 pub(super) fn completion_item_from_function(
     name: &str,
     package: Option<&str>,
-    parameter_hints: &ParameterHints,
+    function_context: &FunctionContext,
 ) -> anyhow::Result<CompletionItem> {
-    let label = format!("{}", name);
+    let label = name.to_string();
     let mut item = completion_item(label, CompletionData::Function {
         name: name.to_string(),
         package: package.map(|s| s.to_string()),
     })?;
 
     item.kind = Some(CompletionItemKind::FUNCTION);
 
+    let insert_text = sym_quote_invalid(name);
+
     let label_details = item_details(package);
     item.label_details = Some(label_details);
 
-    let insert_text = sym_quote_invalid(name);
+    // Are we forming a completion item that's an exact match for an existing
+    // function name that is already in the document?
+    // This identifies scenarios where we need to edit text, not just insert it.
+    let item_is_an_edit = name == function_context.name && !function_context.cursor_is_at_end;
+
+    // These settings are part of the trick we use to make it easy to accept
+    // this matching completion item, which will feel like we're just moving
+    // the cursor past existing text.
+    if item_is_an_edit {
+        item.sort_text = Some(format!("0-{name}"));
+        item.preselect = Some(true);
+    }
 
-    if parameter_hints.is_enabled() {
-        item.insert_text_format = Some(InsertTextFormat::SNIPPET);
-        item.insert_text = Some(format!("{insert_text}($0)"));
+    if function_context.arguments_status == ArgumentsStatus::Absent {
+        if item_is_an_edit {
+            // This is a case like `dplyr::@across` or
+            // `debug(dplyr::@across`), i.e. adding the `dplyr::`
+            // namespace qualification after-the-fact.
+            // We need to consume the existing function name (e.g. `across`) and
+            // move the cursor to its end. We don't add parentheses, both
+            // because it feels presumptuous and because we don't have a
+            // practical way of doing so, in any case. We leave the arguments
+            // just as we found them: absent.
+            let text_edit = TextEdit {
+                range: function_context.range,
+                new_text: insert_text,
+            };
+            item.text_edit = Some(CompletionTextEdit::Edit(text_edit));
+
+            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);
+        // These are the two most common, plain vanilla function completion
+        // scenarios: typing out a known call or reference for the first time.
+        match function_context.usage {
+            FunctionRefUsage::Call => {
+                item.insert_text_format = Some(InsertTextFormat::SNIPPET);
+                item.insert_text = Some(format!("{insert_text}($0)"));
+                item.command = Some(Command {
+                    title: "Trigger Parameter Hints".to_string(),
+                    command: "editor.action.triggerParameterHints".to_string(),
+                    ..Default::default()
+                });
+            },
+            FunctionRefUsage::Value => {
+                item.insert_text_format = Some(InsertTextFormat::PLAIN_TEXT);
+                item.insert_text = Some(insert_text);
+            },
+        }
+
+        return Ok(item);
+    }
+
+    // Addresses a sequence that starts like this:
+    // some_thing()
+    // User realizes they want a different function and backspaces.
+    // some_@()
+    // User accepts one of the offered completions.
+    // some_other_thing(@)
+    // User is back on the Happy Path.
+    // Also handles the case of editing `some_thing(foo = "bar")`, i.e. where
+    // something already exists inside the parentheses.
+    // Also handles the case of adding `pkg::` after the fact to an existing
+    // call like `pkg::@fcn()` or `pkg::@fcn(foo = "bar)`.
+    if function_context.usage == FunctionRefUsage::Call {
+        // Tweak the range to cover the opening parenthesis "(" and
+        // include the opening parenthesis in the new text.
+        // The effect is to move the cursor inside the parentheses.
+        let text_edit = TextEdit {
+            range: {
+                let mut range = function_context.range;
+                range.end.character += 1;
+                range
+            },
+            new_text: format!("{}(", insert_text),
+        };
+        item.text_edit = Some(CompletionTextEdit::Edit(text_edit));
+        if function_context.arguments_status == ArgumentsStatus::Empty {
+            item.command = Some(Command {
+                title: "Trigger Parameter Hints".to_string(),
+                command: "editor.action.triggerParameterHints".to_string(),
+                ..Default::default()
+            });
+        }
+
+        return Ok(item);
     }
 
+    // Fallback case which should really never happen, but let's be safe
+    log::trace!(
+        "completion_item_from_function() - Unexpected case:
+         usage={usage:?},
+         arguments_status={arguments_status:?},
+         name='{name}',
+         package={package:?},
+         cursor_at_end={cursor_is_at_end}",
+        usage = function_context.usage,
+        arguments_status = function_context.arguments_status,
+        cursor_is_at_end = function_context.cursor_is_at_end
+    );
+
+    item.insert_text_format = Some(InsertTextFormat::PLAIN_TEXT);
+    item.insert_text = Some(insert_text);
+
     Ok(item)
 }
 
@@ -250,7 +343,7 @@ pub(super) unsafe fn completion_item_from_object(
     envir: SEXP,
     package: Option<&str>,
     promise_strategy: PromiseStrategy,
-    parameter_hints: &ParameterHints,
+    function_context: &FunctionContext,
 ) -> anyhow::Result<CompletionItem> {
     if r_typeof(object) == PROMSXP {
         return completion_item_from_promise(
@@ -259,7 +352,7 @@ pub(super) unsafe fn completion_item_from_object(
             envir,
             package,
             promise_strategy,
-            parameter_hints,
+            function_context,
         );
     }
 
@@ -269,7 +362,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, parameter_hints);
+        return completion_item_from_function(name, package, function_context);
     }
 
     let mut item = completion_item(name, CompletionData::Object {
@@ -300,7 +393,7 @@ pub(super) unsafe fn completion_item_from_promise(
     envir: SEXP,
     package: Option<&str>,
     promise_strategy: PromiseStrategy,
-    parameter_hints: &ParameterHints,
+    function_context: &FunctionContext,
 ) -> anyhow::Result<CompletionItem> {
     if r_promise_is_forced(object) {
         // Promise has already been evaluated before.
@@ -312,7 +405,7 @@ pub(super) unsafe fn completion_item_from_promise(
             envir,
             package,
             promise_strategy,
-            parameter_hints,
+            function_context,
         );
     }
 
@@ -329,7 +422,7 @@ pub(super) unsafe fn completion_item_from_promise(
             envir,
             package,
             promise_strategy,
-            parameter_hints,
+            function_context,
         );
     }
 
@@ -370,7 +463,7 @@ pub(super) unsafe fn completion_item_from_namespace(
     name: &str,
     namespace: SEXP,
     package: &str,
-    parameter_hints: &ParameterHints,
+    function_context: &FunctionContext,
 ) -> anyhow::Result<CompletionItem> {
     // We perform two passes to locate the object. It is normal for the first pass to
     // error when the `namespace` doesn't have a binding for `name` because the associated
@@ -385,7 +478,7 @@ pub(super) unsafe fn completion_item_from_namespace(
         namespace,
         Some(package),
         PromiseStrategy::Force,
-        parameter_hints,
+        function_context,
     ) {
         Ok(item) => return Ok(item),
         Err(error) => error,
@@ -398,7 +491,7 @@ pub(super) unsafe fn completion_item_from_namespace(
         imports,
         Some(package),
         PromiseStrategy::Force,
-        parameter_hints,
+        function_context,
     ) {
         Ok(item) => return Ok(item),
         Err(error) => error,
@@ -423,10 +516,21 @@ pub(super) unsafe fn completion_item_from_lazydata(
     let promise_strategy = PromiseStrategy::Simple;
 
     // Lazydata objects are never functions, so this doesn't really matter
-    let parameter_hints = ParameterHints::Disabled;
+    let dummy_function_context = FunctionContext {
+        name: String::new(),
+        range: Default::default(),
+        usage: FunctionRefUsage::Call,
+        arguments_status: ArgumentsStatus::Absent,
+        cursor_is_at_end: true,
+    };
 
-    match completion_item_from_symbol(name, env, Some(package), promise_strategy, &parameter_hints)
-    {
+    match completion_item_from_symbol(
+        name,
+        env,
+        Some(package),
+        promise_strategy,
+        &dummy_function_context,
+    ) {
         Ok(item) => Ok(item),
         Err(err) => {
             // Should be impossible, but we'll be extra safe
@@ -440,7 +544,7 @@ pub(super) unsafe fn completion_item_from_symbol(
     envir: SEXP,
     package: Option<&str>,
     promise_strategy: PromiseStrategy,
-    parameter_hints: &ParameterHints,
+    function_context: &FunctionContext,
 ) -> anyhow::Result<CompletionItem> {
     let symbol = r_symbol!(name);
 
@@ -477,7 +581,7 @@ pub(super) unsafe fn completion_item_from_symbol(
         envir,
         package,
         promise_strategy,
-        parameter_hints,
+        function_context,
     )
 }
 
@@ -553,12 +657,11 @@ fn completion_item_from_dot_dot_dot(
         start: position,
         end: position,
     };
-    let textedit = TextEdit {
+    let text_edit = TextEdit {
         range,
         new_text: "".to_string(),
     };
-    let textedit = CompletionTextEdit::Edit(textedit);
-    item.text_edit = Some(textedit);
+    item.text_edit = Some(CompletionTextEdit::Edit(text_edit));
 
     Ok(item)
 }