From f17c5f16da5bdd44c20d10c4cad3d962bb5d73d7 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Fri, 24 May 2024 10:48:03 -0400 Subject: [PATCH 1/2] Avoid autocompleting `()` after `?` and some known special functions --- .../src/lsp/completions/completion_item.rs | 209 ++++++++++++++++-- .../sources/composite/search_path.rs | 5 + .../sources/composite/workspace.rs | 7 +- .../completions/sources/unique/namespace.rs | 11 +- 4 files changed, 207 insertions(+), 25 deletions(-) diff --git a/crates/ark/src/lsp/completions/completion_item.rs b/crates/ark/src/lsp/completions/completion_item.rs index 4a6204762..ab8503a62 100644 --- a/crates/ark/src/lsp/completions/completion_item.rs +++ b/crates/ark/src/lsp/completions/completion_item.rs @@ -27,6 +27,7 @@ use libr::ENCLOS; use libr::PROMSXP; use libr::PRVALUE; use libr::SEXP; +use ropey::Rope; use stdext::*; use tower_lsp::lsp_types::Command; use tower_lsp::lsp_types::CompletionItem; @@ -45,8 +46,10 @@ use crate::lsp::completions::types::PromiseStrategy; use crate::lsp::document_context::DocumentContext; use crate::lsp::encoding::convert_point_to_position; use crate::lsp::traits::rope::RopeExt; +use crate::treesitter::BinaryOperatorType; use crate::treesitter::NodeType; use crate::treesitter::NodeTypeExt; +use crate::treesitter::UnaryOperatorType; pub(super) fn completion_item( label: impl AsRef, @@ -164,6 +167,8 @@ pub(super) unsafe fn completion_item_from_package( pub(super) fn completion_item_from_function>( name: &str, + node: &Node, + contents: &Rope, package: Option<&str>, parameters: &[T], ) -> Result { @@ -179,19 +184,95 @@ pub(super) fn completion_item_from_function>( item.detail = Some(detail); let insert_text = r_symbol_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 node_needs_parentheses(node, contents) { + 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() + }); + } else { + item.insert_text_format = Some(InsertTextFormat::PLAIN_TEXT); + item.insert_text = Some(insert_text); + } return Ok(item); } +fn node_needs_parentheses(node: &Node, contents: &Rope) -> bool { + if node.node_type() != NodeType::Identifier { + return true; + } + + let Some(node) = node.parent() else { + // If no parent, assume we want parentheses + return true; + }; + + if node.node_type() == NodeType::UnaryOperator(UnaryOperatorType::Help) { + // `?`, don't add parentheses + return false; + } + if node.node_type() == NodeType::BinaryOperator(BinaryOperatorType::Help) { + // `?`, don't add parentheses on either side + return false; + } + + if node.node_type() == NodeType::Argument { + // Special `debug()`, don't add parentheses + return argument_node_needs_parentheses(&node, contents); + } + + return true; +} + +// Known list where we don't want to add parentheses to the first argument when it is +// a function +const FUNCTIONS_NO_PARENTHESES: &[&str] = &["debug", "debugonce", "View", "str", "trace", "help"]; + +fn argument_node_needs_parentheses(node: &Node, contents: &Rope) -> bool { + // Work our way up from the `Argument` node to the `Call` node + + // `Argument` -> `Arguments` + // The early return branches shouldn't really happen. + let node = match node.parent() { + Some(parent) if parent.node_type() == NodeType::Arguments => parent, + Some(_) => return true, + None => return true, + }; + + // `Arguments` -> `Call` + // The `Some(_)` case can occur with `Subset` and `Subset2` parent nodes. + let node = match node.parent() { + Some(parent) if parent.node_type() == NodeType::Call => parent, + Some(_) => return true, + None => return true, + }; + + // Get the `function` field of the call node, which is the call name + let Some(node) = node.child_by_field_name("function") else { + return true; + }; + + // Ok, now get the function name + let text = unwrap!(contents.node_slice(&node), Err(err) => { + log::error!("Failed to slice node {node:?} with contents {contents} due to {err}."); + return true; + }); + let text = text.to_string(); + + if FUNCTIONS_NO_PARENTHESES.contains(&text.as_str()) { + // One of the special ones where we don't add parentheses + return false; + } + + return true; +} + // TODO pub(super) unsafe fn completion_item_from_dataset(name: &str) -> Result { let mut item = completion_item(name.to_string(), CompletionData::Unknown)?; @@ -223,13 +304,23 @@ pub(super) unsafe fn completion_item_from_data_variable( pub(super) unsafe fn completion_item_from_object( name: &str, + node: &Node, + contents: &Rope, object: SEXP, envir: SEXP, package: Option<&str>, promise_strategy: PromiseStrategy, ) -> Result { if r_typeof(object) == PROMSXP { - return completion_item_from_promise(name, object, envir, package, promise_strategy); + return completion_item_from_promise( + name, + node, + contents, + object, + envir, + package, + promise_strategy, + ); } // TODO: For some functions (e.g. S4 generics?) the help file might be @@ -243,7 +334,7 @@ pub(super) unsafe fn completion_item_from_object( .iter() .map(|formal| formal.name.as_str()) .collect::>(); - return completion_item_from_function(name, package, &arguments); + return completion_item_from_function(name, node, contents, package, &arguments); } let mut item = completion_item(name, CompletionData::Object { @@ -262,6 +353,8 @@ pub(super) unsafe fn completion_item_from_object( pub(super) unsafe fn completion_item_from_promise( name: &str, + node: &Node, + contents: &Rope, object: SEXP, envir: SEXP, package: Option<&str>, @@ -271,7 +364,15 @@ pub(super) unsafe fn completion_item_from_promise( // 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, + node, + contents, + object, + envir, + package, + promise_strategy, + ); } if promise_strategy == PromiseStrategy::Force && r_promise_is_lazy_load_binding(object) { @@ -281,7 +382,15 @@ 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, envir, package, promise_strategy); + return completion_item_from_object( + name, + node, + contents, + object, + envir, + package, + promise_strategy, + ); } // Otherwise we never want to force promises, so we return a fairly @@ -319,21 +428,33 @@ pub(super) fn completion_item_from_active_binding(name: &str) -> Result Result { // 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, + node, + contents, + namespace, + Some(package), + PromiseStrategy::Force, + ) { 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, + node, + contents, + imports, + Some(package), + PromiseStrategy::Force, + ) { return item; } @@ -347,6 +468,8 @@ pub(super) unsafe fn completion_item_from_namespace( pub(super) unsafe fn completion_item_from_lazydata( name: &str, + node: &Node, + contents: &Rope, env: SEXP, package: &str, ) -> Result { @@ -355,7 +478,7 @@ 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) { + match completion_item_from_symbol(name, node, contents, env, Some(package), promise_strategy) { Some(item) => item, None => { // Should be impossible, but we'll be extra safe @@ -366,6 +489,8 @@ pub(super) unsafe fn completion_item_from_lazydata( pub(super) unsafe fn completion_item_from_symbol( name: &str, + node: &Node, + contents: &Rope, envir: SEXP, package: Option<&str>, promise_strategy: PromiseStrategy, @@ -397,6 +522,8 @@ pub(super) unsafe fn completion_item_from_symbol( Some(completion_item_from_object( name, + node, + contents, object, envir, package, @@ -485,3 +612,47 @@ fn completion_item_from_dot_dot_dot( Ok(item) } + +#[cfg(test)] +mod tests { + use ropey::Rope; + use tree_sitter::Parser; + + use crate::lsp::completions::completion_item::node_needs_parentheses; + use crate::lsp::completions::completion_item::FUNCTIONS_NO_PARENTHESES; + use crate::lsp::traits::node::NodeExt; + use crate::test::point_from_cursor; + + #[test] + fn test_node_needs_parentheses() { + let language = tree_sitter_r::language(); + + let mut parser = Parser::new(); + parser + .set_language(&language) + .expect("failed to create parser"); + + let mut test_node_needs_parentheses = |text: &str| { + let (text, point) = point_from_cursor(text); + let contents = Rope::from_str(text.as_str()); + let tree = parser.parse(text.as_str(), None).unwrap(); + let node = tree.root_node().find_closest_node_to_point(point).unwrap(); + node_needs_parentheses(&node, &contents) + }; + + // `fn@`, i.e. typical function case + assert!(test_node_needs_parentheses("fn@")); + + // No parentheses after help + assert!(!test_node_needs_parentheses("?fn@")); + assert!(!test_node_needs_parentheses("foo?fn@")); + assert!(!test_node_needs_parentheses("foo@?fn")); + + // No parentheses after a set of special functions + // i.e., `debug(fn@)` + for function in FUNCTIONS_NO_PARENTHESES { + let text = format!("{function}(fn@)"); + assert!(!test_node_needs_parentheses(text.as_str())); + } + } +} diff --git a/crates/ark/src/lsp/completions/sources/composite/search_path.rs b/crates/ark/src/lsp/completions/sources/composite/search_path.rs index b0d8edfd4..32efef604 100644 --- a/crates/ark/src/lsp/completions/sources/composite/search_path.rs +++ b/crates/ark/src/lsp/completions/sources/composite/search_path.rs @@ -36,6 +36,9 @@ pub(super) fn completions_from_search_path( "if", "else", "for", "in", "while", "repeat", "break", "next", "return", "function", ]; + let node = &context.node; + let contents = &context.document.contents; + unsafe { // Iterate through environments starting from the global environment. let mut envir = R_GlobalEnv; @@ -73,6 +76,8 @@ pub(super) fn completions_from_search_path( // Add the completion item. let Some(item) = completion_item_from_symbol( symbol, + node, + contents, envir, Some(name.as_str()), promise_strategy.clone(), diff --git a/crates/ark/src/lsp/completions/sources/composite/workspace.rs b/crates/ark/src/lsp/completions/sources/composite/workspace.rs index 56028d45e..267168b2e 100644 --- a/crates/ark/src/lsp/completions/sources/composite/workspace.rs +++ b/crates/ark/src/lsp/completions/sources/composite/workspace.rs @@ -28,7 +28,8 @@ pub(super) fn completions_from_workspace( ) -> Result>> { log::info!("completions_from_workspace()"); - let node = context.node; + let node = &context.node; + let contents = &context.document.contents; if node.is_namespace_operator() { log::error!("Should have already been handled by namespace completions source"); @@ -49,7 +50,7 @@ pub(super) fn completions_from_workspace( let mut completions = vec![]; let token = if node.is_identifier() { - context.document.contents.node_slice(&node)?.to_string() + context.document.contents.node_slice(node)?.to_string() } else { "".to_string() }; @@ -63,7 +64,7 @@ pub(super) fn completions_from_workspace( match &entry.data { indexer::IndexEntryData::Function { name, arguments } => { - let mut completion = unwrap!(completion_item_from_function(name, None, arguments), Err(error) => { + let mut completion = unwrap!(completion_item_from_function(name, node, contents, None, arguments), Err(error) => { error!("{:?}", error); return; }); diff --git a/crates/ark/src/lsp/completions/sources/unique/namespace.rs b/crates/ark/src/lsp/completions/sources/unique/namespace.rs index 5138d90ef..625c05b40 100644 --- a/crates/ark/src/lsp/completions/sources/unique/namespace.rs +++ b/crates/ark/src/lsp/completions/sources/unique/namespace.rs @@ -15,6 +15,7 @@ use libr::R_lsInternal; use libr::Rboolean_TRUE; use libr::Rf_findVarInFrame; use libr::SEXP; +use ropey::Rope; use tower_lsp::lsp_types::CompletionItem; use tree_sitter::Node; use tree_sitter::Point; @@ -36,6 +37,7 @@ pub fn completions_from_namespace( log::info!("completions_from_namespace()"); let node = context.node; + let contents = &context.document.contents; // We expect `DocumentContext` to have drilled down into the CST to the anonymous node, // we will find the actual `NamespaceOperator` node here @@ -79,7 +81,8 @@ pub fn completions_from_namespace( let strings = unsafe { symbols.to::>()? }; for string in strings.iter() { - let item = unsafe { completion_item_from_namespace(string, *namespace, package) }; + let item = + unsafe { completion_item_from_namespace(string, &node, contents, *namespace, package) }; match item { Ok(item) => completions.push(item), Err(error) => log::error!("{:?}", error), @@ -89,7 +92,7 @@ pub fn completions_from_namespace( if exports_only { // `pkg:::object` doesn't return lazy objects, so we don't want // to show lazydata completions if we are inside `:::` - let lazydata = completions_from_namespace_lazydata(*namespace, package)?; + let lazydata = completions_from_namespace_lazydata(*namespace, package, &node, contents)?; if let Some(mut lazydata) = lazydata { completions.append(&mut lazydata); } @@ -162,6 +165,8 @@ fn namespace_node_from_identifier(node: Node) -> NamespaceNodeKind { fn completions_from_namespace_lazydata( namespace: SEXP, package: &str, + node: &Node, + contents: &Rope, ) -> Result>> { log::info!("completions_from_namespace_lazydata()"); @@ -185,7 +190,7 @@ fn completions_from_namespace_lazydata( let mut completions: Vec = vec![]; for name in names.iter() { - match completion_item_from_lazydata(name, env, package) { + match completion_item_from_lazydata(name, node, contents, env, package) { Ok(item) => completions.push(item), Err(error) => log::error!("{:?}", error), } From d57c6803e579c1d6a3161bd3443609b91370c7b6 Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Fri, 24 May 2024 13:03:21 -0400 Subject: [PATCH 2/2] Ensure that namespace completions inside special functions don't add `()` --- .../src/lsp/completions/completion_item.rs | 4 --- .../completions/sources/unique/namespace.rs | 28 +++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) diff --git a/crates/ark/src/lsp/completions/completion_item.rs b/crates/ark/src/lsp/completions/completion_item.rs index ab8503a62..5a9c3666b 100644 --- a/crates/ark/src/lsp/completions/completion_item.rs +++ b/crates/ark/src/lsp/completions/completion_item.rs @@ -204,10 +204,6 @@ pub(super) fn completion_item_from_function>( } fn node_needs_parentheses(node: &Node, contents: &Rope) -> bool { - if node.node_type() != NodeType::Identifier { - return true; - } - let Some(node) = node.parent() else { // If no parent, assume we want parentheses return true; diff --git a/crates/ark/src/lsp/completions/sources/unique/namespace.rs b/crates/ark/src/lsp/completions/sources/unique/namespace.rs index 625c05b40..a9ed67d53 100644 --- a/crates/ark/src/lsp/completions/sources/unique/namespace.rs +++ b/crates/ark/src/lsp/completions/sources/unique/namespace.rs @@ -222,11 +222,13 @@ fn list_namespace_exports(namespace: SEXP) -> RObject { #[cfg(test)] mod tests { + use tower_lsp::lsp_types::InsertTextFormat; use tree_sitter::Point; use crate::lsp::completions::sources::unique::namespace::completions_from_namespace; use crate::lsp::document_context::DocumentContext; use crate::lsp::documents::Document; + use crate::test::point_from_cursor; use crate::test::r_test; #[test] @@ -312,4 +314,30 @@ mod tests { assert!(completions.is_empty()); }) } + + #[test] + fn test_completions_dont_add_parentheses_inside_special_functions() { + r_test(|| { + let (text, point) = point_from_cursor("debug(base::ab@)"); + + let document = Document::new(text.as_str(), None); + let context = DocumentContext::new(&document, point, None); + + let completions = completions_from_namespace(&context).unwrap().unwrap(); + let completion = completions + .iter() + .find(|item| item.label == "abs") + .unwrap() + .clone(); + + // Not a snippet, and not with `()`, and no extra command to trigger + // parameter hints! + assert_eq!(completion.insert_text.unwrap(), String::from("abs")); + assert_eq!( + completion.insert_text_format.unwrap(), + InsertTextFormat::PLAIN_TEXT + ); + assert!(completion.command.is_none()); + }) + } }