From 9271c48db63addb9aa34f0932004ee1606876f88 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 26 Jun 2025 16:25:41 +0200 Subject: [PATCH 01/13] Emit `test_that()` calls as symbols --- ...symbols__tests__symbol_call_test_that.snap | 161 ++++++++++++++++++ crates/ark/src/lsp/symbols.rs | 92 ++++++++++ 2 files changed, 253 insertions(+) create mode 100644 crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_test_that.snap diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_test_that.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_test_that.snap new file mode 100644 index 000000000..279725a42 --- /dev/null +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_test_that.snap @@ -0,0 +1,161 @@ +--- +source: crates/ark/src/lsp/symbols.rs +expression: "test_symbol(\"\ntest_that_not('foo', {\n 1\n})\n\n# title ----\n\ntest_that('foo', {\n # title1 ----\n 1\n # title2 ----\n foo <- function() {\n 2\n }\n})\n\")" +--- +[ + DocumentSymbol { + name: "title", + detail: None, + kind: String, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 5, + character: 0, + }, + end: Position { + line: 14, + character: 2, + }, + }, + selection_range: Range { + start: Position { + line: 5, + character: 0, + }, + end: Position { + line: 14, + character: 2, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "Test: foo", + detail: None, + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 7, + character: 0, + }, + end: Position { + line: 14, + character: 2, + }, + }, + selection_range: Range { + start: Position { + line: 7, + character: 0, + }, + end: Position { + line: 14, + character: 2, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "title1", + detail: None, + kind: String, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 8, + character: 2, + }, + end: Position { + line: 9, + character: 3, + }, + }, + selection_range: Range { + start: Position { + line: 8, + character: 2, + }, + end: Position { + line: 9, + character: 3, + }, + }, + children: Some( + [], + ), + }, + DocumentSymbol { + name: "title2", + detail: None, + kind: String, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 10, + character: 2, + }, + end: Position { + line: 13, + character: 3, + }, + }, + selection_range: Range { + start: Position { + line: 10, + character: 2, + }, + end: Position { + line: 13, + character: 3, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "foo", + detail: Some( + "function()", + ), + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 11, + character: 2, + }, + end: Position { + line: 13, + character: 3, + }, + }, + selection_range: Range { + start: Position { + line: 11, + character: 2, + }, + end: Position { + line: 13, + character: 3, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, + ], + ), + }, + ], + ), + }, +] diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index 26e23ab19..40dab29cb 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -156,6 +156,10 @@ fn collect_symbols( collect_sections(node, contents, current_level, symbols)?; }, + NodeType::Call => { + collect_call(node, contents, symbols)?; + }, + NodeType::BinaryOperator(BinaryOperatorType::LeftAssignment) | NodeType::BinaryOperator(BinaryOperatorType::EqualsAssignment) => { collect_assignment(node, contents, symbols)?; @@ -253,6 +257,72 @@ fn collect_sections( Ok(()) } +fn collect_call( + node: &Node, + contents: &Rope, + symbols: &mut Vec, +) -> anyhow::Result<()> { + let Some(callee) = node.child_by_field_name("function") else { + return Ok(()); + }; + if !callee.is_identifier() { + return Ok(()); + } + + let fun_symbol = contents.node_slice(&callee)?.to_string(); + + match fun_symbol.as_str() { + "test_that" => collect_call_test_that(node, contents, symbols)?, + _ => {}, + } + + Ok(()) +} + +// https://github.com/posit-dev/positron/issues/1428 +fn collect_call_test_that( + node: &Node, + contents: &Rope, + symbols: &mut Vec, +) -> anyhow::Result<()> { + let Some(arguments) = node.child_by_field_name("arguments") else { + return Ok(()); + }; + + // We don't do any argument matching and just consider the first argument if + // a string. First skip over `(`. + let Some(first_argument) = arguments.child(1).and_then(|n| n.child(0)) else { + return Ok(()); + }; + if !first_argument.is_string() { + return Ok(()); + } + + let Some(string) = first_argument.child_by_field_name("content") else { + return Ok(()); + }; + + // Recurse in arguments. We could skip the first one if we wanted. + let mut children = Vec::new(); + let mut cursor = arguments.walk(); + for child in arguments.children_by_field_name("argument", &mut cursor) { + if let Some(value) = child.child_by_field_name("value") { + collect_symbols(&value, contents, 0, &mut children)?; + } + } + + let name = contents.node_slice(&string)?.to_string(); + let name = format!("Test: {name}"); + + let start = convert_point_to_position(contents, node.start_position()); + let end = convert_point_to_position(contents, node.end_position()); + + let symbol = new_symbol_node(name, SymbolKind::FUNCTION, Range { start, end }, children); + symbols.push(symbol); + + Ok(()) +} + fn collect_assignment( node: &Node, contents: &Rope, @@ -587,4 +657,26 @@ z <- 3", assert_eq!(section_b.range.start.line, 4); assert_eq!(section_b.range.end.line, 5); // End of function body } + + #[test] + fn test_symbol_call_test_that() { + insta::assert_debug_snapshot!(test_symbol( + " +test_that_not('foo', { + 1 +}) + +# title ---- + +test_that('foo', { + # title1 ---- + 1 + # title2 ---- + foo <- function() { + 2 + } +}) +" + )); + } } From 1b58bb0a4d0b01af1d70a0d4095377b0c348cf93 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 27 Jun 2025 08:26:51 +0200 Subject: [PATCH 02/13] Expand test with multiple sections --- ...symbols__tests__symbol_call_test_that.snap | 71 +++++++++++++++++-- crates/ark/src/lsp/symbols.rs | 5 ++ 2 files changed, 71 insertions(+), 5 deletions(-) diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_test_that.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_test_that.snap index 279725a42..d177e781c 100644 --- a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_test_that.snap +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_test_that.snap @@ -1,6 +1,6 @@ --- source: crates/ark/src/lsp/symbols.rs -expression: "test_symbol(\"\ntest_that_not('foo', {\n 1\n})\n\n# title ----\n\ntest_that('foo', {\n # title1 ----\n 1\n # title2 ----\n foo <- function() {\n 2\n }\n})\n\")" +expression: "test_symbol(\"\ntest_that_not('foo', {\n 1\n})\n\n# title ----\n\ntest_that('foo', {\n # title1 ----\n 1\n # title2 ----\n foo <- function() {\n 2\n }\n})\n\n# title2 ----\ntest_that('bar', {\n 1\n})\n\")" --- [ DocumentSymbol { @@ -15,8 +15,8 @@ expression: "test_symbol(\"\ntest_that_not('foo', {\n 1\n})\n\n# title ----\n\n character: 0, }, end: Position { - line: 14, - character: 2, + line: 15, + character: 0, }, }, selection_range: Range { @@ -25,8 +25,8 @@ expression: "test_symbol(\"\ntest_that_not('foo', {\n 1\n})\n\n# title ----\n\n character: 0, }, end: Position { - line: 14, - character: 2, + line: 15, + character: 0, }, }, children: Some( @@ -158,4 +158,65 @@ expression: "test_symbol(\"\ntest_that_not('foo', {\n 1\n})\n\n# title ----\n\n ], ), }, + DocumentSymbol { + name: "title2", + detail: None, + kind: String, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 16, + character: 0, + }, + end: Position { + line: 19, + character: 2, + }, + }, + selection_range: Range { + start: Position { + line: 16, + character: 0, + }, + end: Position { + line: 19, + character: 2, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "Test: bar", + detail: None, + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 17, + character: 0, + }, + end: Position { + line: 19, + character: 2, + }, + }, + selection_range: Range { + start: Position { + line: 17, + character: 0, + }, + end: Position { + line: 19, + character: 2, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, ] diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index 40dab29cb..5ecb76391 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -676,6 +676,11 @@ test_that('foo', { 2 } }) + +# title2 ---- +test_that('bar', { + 1 +}) " )); } From 84d2e2ca54b13c5e1d633483bd8d011b7a81d8bb Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 27 Jun 2025 10:45:03 +0200 Subject: [PATCH 03/13] Add named functions as document symbols --- .github/copilot-instructions.md | 3 + ...__symbols__tests__symbol_call_methods.snap | 101 ++++++++++++++++++ crates/ark/src/lsp/symbols.rs | 90 +++++++++++++++- 3 files changed, 193 insertions(+), 1 deletion(-) create mode 100644 crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_methods.snap diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index f4942f25c..ebb815a6e 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -19,3 +19,6 @@ _below_ the calling function. A general goal is to be able to read linearly from top to bottom with the relevant context and main logic first. The code should be organised like a call stack. Of course that's not always possible, use best judgement to produce the clearest code organization. + + Keep the main logic as unnested as possible. Favour Rust's `let ... else` + syntax to return early or continue a loop in the `else` clause, over `if let`. diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_methods.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_methods.snap new file mode 100644 index 000000000..f50436451 --- /dev/null +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_methods.snap @@ -0,0 +1,101 @@ +--- +source: crates/ark/src/lsp/symbols.rs +expression: "test_symbol(\"\n# section ----\nlist(\n foo = function() {\n 1\n }, # matched\n function() {\n 2\n }, # not matched\n bar = function() {\n 3\n }, # matched\n baz = (function() {\n 4\n }) # not matched\n)\n\")" +--- +[ + DocumentSymbol { + name: "section", + detail: None, + kind: String, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 15, + character: 1, + }, + }, + selection_range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 15, + character: 1, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "foo", + detail: Some( + "function()", + ), + kind: Method, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 3, + character: 10, + }, + end: Position { + line: 5, + character: 5, + }, + }, + selection_range: Range { + start: Position { + line: 3, + character: 10, + }, + end: Position { + line: 5, + character: 5, + }, + }, + children: Some( + [], + ), + }, + DocumentSymbol { + name: "bar", + detail: Some( + "function()", + ), + kind: Method, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 9, + character: 10, + }, + end: Position { + line: 11, + character: 5, + }, + }, + selection_range: Range { + start: Position { + line: 9, + character: 10, + }, + end: Position { + line: 11, + character: 5, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, +] diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index 5ecb76391..a75c934ed 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -273,7 +273,65 @@ fn collect_call( match fun_symbol.as_str() { "test_that" => collect_call_test_that(node, contents, symbols)?, - _ => {}, + _ => collect_call_methods(node, contents, symbols)?, + } + + Ok(()) +} + +fn collect_call_methods( + node: &Node, + contents: &Rope, + symbols: &mut Vec, +) -> anyhow::Result<()> { + let Some(arguments) = node.child_by_field_name("arguments") else { + return Ok(()); + }; + + let mut cursor = node.walk(); + for arg in arguments.children(&mut cursor) { + if arg.kind() != "argument" { + continue; + } + + let Some(arg_value) = arg.child_by_field_name("value") else { + continue; + }; + if arg_value.kind() != "function_definition" { + continue; + } + + // Process the function body to extract child symbols. + // We do this even if it's not a "method", i.e. if it's not named. + let body = arg_value.child_by_field_name("body").into_result()?; + let mut children = Vec::new(); + collect_symbols(&body, contents, 0, &mut children)?; + + // There must be a name node, we're only collecting named functions as methods + let Some(arg_name) = arg.child_by_field_name("name") else { + continue; + }; + if !arg_name.is_identifier_or_string() { + continue; + } + let arg_name_str = contents.node_slice(&arg_name)?.to_string(); + + let start = convert_point_to_position(contents, arg_value.start_position()); + let end = convert_point_to_position(contents, arg_value.end_position()); + + let mut symbol = new_symbol_node( + arg_name_str, + SymbolKind::METHOD, + Range { start, end }, + vec![], + ); + + // Don't include whole function as detail as the body often doesn't + // provide useful information and only make the outline more busy (with + // curly braces, newline characters, etc). + symbol.detail = Some(String::from("function()")); + + symbols.push(symbol); } Ok(()) @@ -681,7 +739,37 @@ test_that('foo', { test_that('bar', { 1 }) +" + )); + } + + #[test] + fn test_symbol_call_methods() { + insta::assert_debug_snapshot!(test_symbol( + " +# section ---- +list( + foo = function() { + 1 + }, # matched + function() { + 2 + }, # not matched + bar = function() { + 3 + }, # matched + baz = (function() { + 4 + }) # not matched +) " )); } } + +// chat <- r6::r6class( +// "chat", +// public = list( +// initialize = function() "initialize", +// ) +// ) From de3da46c5247d2b59bc783f7541353d2c2de3e16 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 27 Jun 2025 11:05:44 +0200 Subject: [PATCH 04/13] Collect document symbols in arguments --- ...symbols__tests__symbol_call_arguments.snap | 69 +++++++++++++ crates/ark/src/lsp/symbols.rs | 98 ++++++++++++------- 2 files changed, 129 insertions(+), 38 deletions(-) create mode 100644 crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_arguments.snap diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_arguments.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_arguments.snap new file mode 100644 index 000000000..e316696fa --- /dev/null +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_arguments.snap @@ -0,0 +1,69 @@ +--- +source: crates/ark/src/lsp/symbols.rs +expression: "test_symbol(\"\n# section ----\nlocal({\n a <- function() {\n 1\n }\n})\n\")" +--- +[ + DocumentSymbol { + name: "section", + detail: None, + kind: String, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 6, + character: 2, + }, + }, + selection_range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 6, + character: 2, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "a", + detail: Some( + "function()", + ), + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 3, + character: 4, + }, + end: Position { + line: 5, + character: 5, + }, + }, + selection_range: Range { + start: Position { + line: 3, + character: 4, + }, + end: Position { + line: 5, + character: 5, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, +] diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index a75c934ed..b58f53486 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -273,13 +273,13 @@ fn collect_call( match fun_symbol.as_str() { "test_that" => collect_call_test_that(node, contents, symbols)?, - _ => collect_call_methods(node, contents, symbols)?, + _ => collect_call_arguments(node, contents, symbols)?, } Ok(()) } -fn collect_call_methods( +fn collect_call_arguments( node: &Node, contents: &Rope, symbols: &mut Vec, @@ -297,42 +297,57 @@ fn collect_call_methods( let Some(arg_value) = arg.child_by_field_name("value") else { continue; }; - if arg_value.kind() != "function_definition" { - continue; + + // Recurse into arguments. They might be a braced list, another call + // that might contain functions, etc. + collect_symbols(&arg_value, contents, 0, symbols)?; + + if arg_value.kind() == "function_definition" { + // Functions are not collected by `collect_symbols()` so we deal + // with them here by processing the function body to extract child + // symbols. We do this even if it's not a "method", i.e. if it's not + // named. + let body = arg_value.child_by_field_name("body").into_result()?; + let mut children = Vec::new(); + collect_symbols(&body, contents, 0, &mut children)?; + + // If there is a name node, collect it as a method + if let Some(arg_fun) = arg.child_by_field_name("name") { + collect_method(&arg_fun, &arg_value, contents, symbols)?; + }; } + } - // Process the function body to extract child symbols. - // We do this even if it's not a "method", i.e. if it's not named. - let body = arg_value.child_by_field_name("body").into_result()?; - let mut children = Vec::new(); - collect_symbols(&body, contents, 0, &mut children)?; + Ok(()) +} - // There must be a name node, we're only collecting named functions as methods - let Some(arg_name) = arg.child_by_field_name("name") else { - continue; - }; - if !arg_name.is_identifier_or_string() { - continue; - } - let arg_name_str = contents.node_slice(&arg_name)?.to_string(); +fn collect_method( + arg_fun: &Node, + arg_value: &Node, + contents: &Rope, + symbols: &mut Vec, +) -> anyhow::Result<()> { + if !arg_fun.is_identifier_or_string() { + return Ok(()); + } + let arg_name_str = contents.node_slice(&arg_fun)?.to_string(); - let start = convert_point_to_position(contents, arg_value.start_position()); - let end = convert_point_to_position(contents, arg_value.end_position()); + let start = convert_point_to_position(contents, arg_value.start_position()); + let end = convert_point_to_position(contents, arg_value.end_position()); - let mut symbol = new_symbol_node( - arg_name_str, - SymbolKind::METHOD, - Range { start, end }, - vec![], - ); + let mut symbol = new_symbol_node( + arg_name_str, + SymbolKind::METHOD, + Range { start, end }, + vec![], + ); - // Don't include whole function as detail as the body often doesn't - // provide useful information and only make the outline more busy (with - // curly braces, newline characters, etc). - symbol.detail = Some(String::from("function()")); + // Don't include whole function as detail as the body often doesn't + // provide useful information and only make the outline more busy (with + // curly braces, newline characters, etc). + symbol.detail = Some(String::from("function()")); - symbols.push(symbol); - } + symbols.push(symbol); Ok(()) } @@ -765,11 +780,18 @@ list( " )); } -} -// chat <- r6::r6class( -// "chat", -// public = list( -// initialize = function() "initialize", -// ) -// ) + #[test] + fn test_symbol_call_arguments() { + insta::assert_debug_snapshot!(test_symbol( + " +# section ---- +local({ + a <- function() { + 1 + } +}) +" + )); + } +} From 6b62bbcd2335e23420af5e23c4822af1a129edbb Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 27 Jun 2025 11:52:22 +0200 Subject: [PATCH 05/13] Collect document symbols in RHS of assigned objects --- .../ark__lsp__symbols__tests__symbol_rhs.snap | 164 ++++++++++++++++++ ...ymbols__tests__symbol_rhs_braced_list.snap | 69 ++++++++ crates/ark/src/lsp/symbols.rs | 77 +++++--- 3 files changed, 289 insertions(+), 21 deletions(-) create mode 100644 crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs.snap create mode 100644 crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs_braced_list.snap diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs.snap new file mode 100644 index 000000000..b5e6f1922 --- /dev/null +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs.snap @@ -0,0 +1,164 @@ +--- +source: crates/ark/src/lsp/symbols.rs +expression: "test_symbol(\"\n# section ----\nclass <- r6::r6class(\n 'class',\n public = list(\n initialize = function() 'initialize',\n foo = function() 'foo'\n ),\n private = list(\n bar = function() 'bar'\n )\n)\n\")" +--- +[ + DocumentSymbol { + name: "section", + detail: None, + kind: String, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 11, + character: 1, + }, + }, + selection_range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 11, + character: 1, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "class", + detail: None, + kind: Variable, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 2, + character: 0, + }, + end: Position { + line: 2, + character: 5, + }, + }, + selection_range: Range { + start: Position { + line: 2, + character: 0, + }, + end: Position { + line: 2, + character: 5, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "initialize", + detail: Some( + "function()", + ), + kind: Method, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 5, + character: 17, + }, + end: Position { + line: 5, + character: 40, + }, + }, + selection_range: Range { + start: Position { + line: 5, + character: 17, + }, + end: Position { + line: 5, + character: 40, + }, + }, + children: Some( + [], + ), + }, + DocumentSymbol { + name: "foo", + detail: Some( + "function()", + ), + kind: Method, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 6, + character: 10, + }, + end: Position { + line: 6, + character: 26, + }, + }, + selection_range: Range { + start: Position { + line: 6, + character: 10, + }, + end: Position { + line: 6, + character: 26, + }, + }, + children: Some( + [], + ), + }, + DocumentSymbol { + name: "bar", + detail: Some( + "function()", + ), + kind: Method, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 9, + character: 10, + }, + end: Position { + line: 9, + character: 26, + }, + }, + selection_range: Range { + start: Position { + line: 9, + character: 10, + }, + end: Position { + line: 9, + character: 26, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, + ], + ), + }, +] diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs_braced_list.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs_braced_list.snap new file mode 100644 index 000000000..95c351682 --- /dev/null +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs_braced_list.snap @@ -0,0 +1,69 @@ +--- +source: crates/ark/src/lsp/symbols.rs +expression: "test_symbol(\"\nfoo <- {\n bar <- function() {}\n}\n\")" +--- +[ + DocumentSymbol { + name: "foo", + detail: None, + kind: Variable, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 1, + character: 3, + }, + }, + selection_range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 1, + character: 3, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "bar", + detail: Some( + "function()", + ), + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 2, + character: 4, + }, + end: Position { + line: 2, + character: 24, + }, + }, + selection_range: Range { + start: Position { + line: 2, + character: 4, + }, + end: Position { + line: 2, + character: 24, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, +] diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index b58f53486..aacb55586 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -265,17 +265,18 @@ fn collect_call( let Some(callee) = node.child_by_field_name("function") else { return Ok(()); }; - if !callee.is_identifier() { - return Ok(()); - } - let fun_symbol = contents.node_slice(&callee)?.to_string(); + if callee.is_identifier() { + let fun_symbol = contents.node_slice(&callee)?.to_string(); - match fun_symbol.as_str() { - "test_that" => collect_call_test_that(node, contents, symbols)?, - _ => collect_call_arguments(node, contents, symbols)?, + match fun_symbol.as_str() { + "test_that" => return collect_call_test_that(node, contents, symbols), + _ => {}, // fallthrough + } } + collect_call_arguments(node, contents, symbols)?; + Ok(()) } @@ -401,32 +402,36 @@ fn collect_assignment( contents: &Rope, symbols: &mut Vec, ) -> anyhow::Result<()> { - // Check for assignment - matches!( - node.node_type(), - NodeType::BinaryOperator(BinaryOperatorType::LeftAssignment) | - NodeType::BinaryOperator(BinaryOperatorType::EqualsAssignment) - ) - .into_result()?; + let (NodeType::BinaryOperator(BinaryOperatorType::LeftAssignment) | + NodeType::BinaryOperator(BinaryOperatorType::EqualsAssignment)) = node.node_type() + else { + return Ok(()); + }; - // check for lhs, rhs - let lhs = node.child_by_field_name("lhs").into_result()?; - let rhs = node.child_by_field_name("rhs").into_result()?; + let (Some(lhs), Some(rhs)) = ( + node.child_by_field_name("lhs"), + node.child_by_field_name("rhs"), + ) else { + return Ok(()); + }; - // check for identifier on lhs, function on rhs + // If a function, collect symbol as function let function = lhs.is_identifier_or_string() && rhs.is_function_definition(); - if function { return collect_assignment_with_function(node, contents, symbols); } - // otherwise, just index as generic object + // Otherwise, collect as generic object let name = contents.node_slice(&lhs)?.to_string(); let start = convert_point_to_position(contents, lhs.start_position()); let end = convert_point_to_position(contents, lhs.end_position()); - let symbol = new_symbol(name, SymbolKind::VARIABLE, Range { start, end }); + // Now recurse into RHS + let mut children = Vec::new(); + collect_symbols(&rhs, contents, 0, &mut children)?; + + let symbol = new_symbol_node(name, SymbolKind::VARIABLE, Range { start, end }, children); symbols.push(symbol); Ok(()) @@ -791,6 +796,36 @@ local({ 1 } }) +" + )); + } + + #[test] + fn test_symbol_rhs_braced_list() { + insta::assert_debug_snapshot!(test_symbol( + " +foo <- { + bar <- function() {} +} +" + )); + } + + #[test] + fn test_symbol_rhs_methods() { + insta::assert_debug_snapshot!(test_symbol( + " +# section ---- +class <- r6::r6class( + 'class', + public = list( + initialize = function() 'initialize', + foo = function() 'foo' + ), + private = list( + bar = function() 'bar' + ) +) " )); } From f67b10f1b490d986899631a1aaa01684e3105de5 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 27 Jun 2025 12:35:36 +0200 Subject: [PATCH 06/13] Fix recursion in function bodies --- ...__symbols__tests__symbol_call_methods.snap | 114 +++++++++++- ...p__symbols__tests__symbol_rhs_methods.snap | 164 ++++++++++++++++++ crates/ark/src/lsp/symbols.rs | 25 +-- 3 files changed, 284 insertions(+), 19 deletions(-) create mode 100644 crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs_methods.snap diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_methods.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_methods.snap index f50436451..4cb58b084 100644 --- a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_methods.snap +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_call_methods.snap @@ -1,6 +1,6 @@ --- source: crates/ark/src/lsp/symbols.rs -expression: "test_symbol(\"\n# section ----\nlist(\n foo = function() {\n 1\n }, # matched\n function() {\n 2\n }, # not matched\n bar = function() {\n 3\n }, # matched\n baz = (function() {\n 4\n }) # not matched\n)\n\")" +expression: "test_symbol(\"\n# section ----\nlist(\n foo = function() {\n 1\n # nested section ----\n nested <- function() {}\n }, # matched\n function() {\n 2\n # `nested` is a symbol even if the unnamed method is not\n nested <- function () {\n }\n }, # not matched\n bar = function() {\n 3\n }, # matched\n baz = (function() {\n 4\n }) # not matched\n)\n\")" --- [ DocumentSymbol { @@ -15,7 +15,7 @@ expression: "test_symbol(\"\n# section ----\nlist(\n foo = function() {\n character: 0, }, end: Position { - line: 15, + line: 20, character: 1, }, }, @@ -25,7 +25,7 @@ expression: "test_symbol(\"\n# section ----\nlist(\n foo = function() {\n character: 0, }, end: Position { - line: 15, + line: 20, character: 1, }, }, @@ -45,7 +45,7 @@ expression: "test_symbol(\"\n# section ----\nlist(\n foo = function() {\n character: 10, }, end: Position { - line: 5, + line: 7, character: 5, }, }, @@ -55,7 +55,103 @@ expression: "test_symbol(\"\n# section ----\nlist(\n foo = function() {\n character: 10, }, end: Position { - line: 5, + line: 7, + character: 5, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "nested section", + detail: None, + kind: String, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 5, + character: 8, + }, + end: Position { + line: 6, + character: 31, + }, + }, + selection_range: Range { + start: Position { + line: 5, + character: 8, + }, + end: Position { + line: 6, + character: 31, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "nested", + detail: Some( + "function()", + ), + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 6, + character: 8, + }, + end: Position { + line: 6, + character: 31, + }, + }, + selection_range: Range { + start: Position { + line: 6, + character: 8, + }, + end: Position { + line: 6, + character: 31, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, + ], + ), + }, + DocumentSymbol { + name: "nested", + detail: Some( + "function()", + ), + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 11, + character: 8, + }, + end: Position { + line: 12, + character: 5, + }, + }, + selection_range: Range { + start: Position { + line: 11, + character: 8, + }, + end: Position { + line: 12, character: 5, }, }, @@ -73,21 +169,21 @@ expression: "test_symbol(\"\n# section ----\nlist(\n foo = function() {\n deprecated: None, range: Range { start: Position { - line: 9, + line: 14, character: 10, }, end: Position { - line: 11, + line: 16, character: 5, }, }, selection_range: Range { start: Position { - line: 9, + line: 14, character: 10, }, end: Position { - line: 11, + line: 16, character: 5, }, }, diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs_methods.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs_methods.snap new file mode 100644 index 000000000..b5e6f1922 --- /dev/null +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_rhs_methods.snap @@ -0,0 +1,164 @@ +--- +source: crates/ark/src/lsp/symbols.rs +expression: "test_symbol(\"\n# section ----\nclass <- r6::r6class(\n 'class',\n public = list(\n initialize = function() 'initialize',\n foo = function() 'foo'\n ),\n private = list(\n bar = function() 'bar'\n )\n)\n\")" +--- +[ + DocumentSymbol { + name: "section", + detail: None, + kind: String, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 11, + character: 1, + }, + }, + selection_range: Range { + start: Position { + line: 1, + character: 0, + }, + end: Position { + line: 11, + character: 1, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "class", + detail: None, + kind: Variable, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 2, + character: 0, + }, + end: Position { + line: 2, + character: 5, + }, + }, + selection_range: Range { + start: Position { + line: 2, + character: 0, + }, + end: Position { + line: 2, + character: 5, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "initialize", + detail: Some( + "function()", + ), + kind: Method, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 5, + character: 17, + }, + end: Position { + line: 5, + character: 40, + }, + }, + selection_range: Range { + start: Position { + line: 5, + character: 17, + }, + end: Position { + line: 5, + character: 40, + }, + }, + children: Some( + [], + ), + }, + DocumentSymbol { + name: "foo", + detail: Some( + "function()", + ), + kind: Method, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 6, + character: 10, + }, + end: Position { + line: 6, + character: 26, + }, + }, + selection_range: Range { + start: Position { + line: 6, + character: 10, + }, + end: Position { + line: 6, + character: 26, + }, + }, + children: Some( + [], + ), + }, + DocumentSymbol { + name: "bar", + detail: Some( + "function()", + ), + kind: Method, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 9, + character: 10, + }, + end: Position { + line: 9, + character: 26, + }, + }, + selection_range: Range { + start: Position { + line: 9, + character: 10, + }, + end: Position { + line: 9, + character: 26, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, + ], + ), + }, +] diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index aacb55586..5557f457a 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -304,17 +304,13 @@ fn collect_call_arguments( collect_symbols(&arg_value, contents, 0, symbols)?; if arg_value.kind() == "function_definition" { - // Functions are not collected by `collect_symbols()` so we deal - // with them here by processing the function body to extract child - // symbols. We do this even if it's not a "method", i.e. if it's not - // named. - let body = arg_value.child_by_field_name("body").into_result()?; - let mut children = Vec::new(); - collect_symbols(&body, contents, 0, &mut children)?; - - // If there is a name node, collect it as a method if let Some(arg_fun) = arg.child_by_field_name("name") { + // If this is a named function, collect it as a method collect_method(&arg_fun, &arg_value, contents, symbols)?; + } else { + // Otherwise, just recurse into the function + let body = arg_value.child_by_field_name("body").into_result()?; + collect_symbols(&body, contents, 0, symbols)?; }; } } @@ -336,11 +332,15 @@ fn collect_method( let start = convert_point_to_position(contents, arg_value.start_position()); let end = convert_point_to_position(contents, arg_value.end_position()); + let body = arg_value.child_by_field_name("body").into_result()?; + let mut children = vec![]; + collect_symbols(&body, contents, 0, &mut children)?; + let mut symbol = new_symbol_node( arg_name_str, SymbolKind::METHOD, Range { start, end }, - vec![], + children, ); // Don't include whole function as detail as the body often doesn't @@ -771,9 +771,14 @@ test_that('bar', { list( foo = function() { 1 + # nested section ---- + nested <- function() {} }, # matched function() { 2 + # `nested` is a symbol even if the unnamed method is not + nested <- function () { + } }, # not matched bar = function() { 3 From 3e59d3b77945dd6066b48c1a9b14b31f4656436f Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 27 Jun 2025 14:30:48 +0200 Subject: [PATCH 07/13] Add `CollectContect` arguments --- crates/ark/src/lsp/symbols.rs | 54 +++++++++++++++++++---------------- 1 file changed, 30 insertions(+), 24 deletions(-) diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index 5557f457a..124ede763 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -122,6 +122,8 @@ struct Section { children: Vec, } +struct CollectContext; + pub(crate) fn document_symbols( state: &WorldState, params: &DocumentSymbolParams, @@ -136,7 +138,8 @@ pub(crate) fn document_symbols( let mut result = Vec::new(); // Extract and process all symbols from the AST - if let Err(err) = collect_symbols(&root_node, contents, 0, &mut result) { + let mut ctx = CollectContext; + if let Err(err) = collect_symbols(&mut ctx, &root_node, contents, 0, &mut result) { log::error!("Failed to collect symbols: {err:?}"); return Ok(Vec::new()); } @@ -146,6 +149,7 @@ pub(crate) fn document_symbols( /// Collect all document symbols from a node recursively fn collect_symbols( + ctx: &mut CollectContext, node: &Node, contents: &Rope, current_level: usize, @@ -153,26 +157,26 @@ fn collect_symbols( ) -> anyhow::Result<()> { match node.node_type() { NodeType::Program | NodeType::BracedExpression => { - collect_sections(node, contents, current_level, symbols)?; + collect_sections(ctx, node, contents, current_level, symbols)?; }, NodeType::Call => { - collect_call(node, contents, symbols)?; + collect_call(ctx, node, contents, symbols)?; }, NodeType::BinaryOperator(BinaryOperatorType::LeftAssignment) | NodeType::BinaryOperator(BinaryOperatorType::EqualsAssignment) => { - collect_assignment(node, contents, symbols)?; + collect_assignment(ctx, node, contents, symbols)?; }, // For all other node types, no symbols need to be added _ => {}, } - Ok(()) } fn collect_sections( + ctx: &mut CollectContext, node: &Node, contents: &Rope, current_level: usize, @@ -224,11 +228,11 @@ fn collect_sections( if active_sections.is_empty() { // If no active section, extend current vector of symbols - collect_symbols(&child, contents, current_level, symbols)?; + collect_symbols(ctx, &child, contents, current_level, symbols)?; } else { // Otherwise create new store of symbols for the current section let mut child_symbols = Vec::new(); - collect_symbols(&child, contents, current_level, &mut child_symbols)?; + collect_symbols(ctx, &child, contents, current_level, &mut child_symbols)?; // Nest them inside last section if !child_symbols.is_empty() { @@ -258,6 +262,7 @@ fn collect_sections( } fn collect_call( + ctx: &mut CollectContext, node: &Node, contents: &Rope, symbols: &mut Vec, @@ -268,19 +273,19 @@ fn collect_call( if callee.is_identifier() { let fun_symbol = contents.node_slice(&callee)?.to_string(); - match fun_symbol.as_str() { - "test_that" => return collect_call_test_that(node, contents, symbols), + "test_that" => return collect_call_test_that(ctx, node, contents, symbols), _ => {}, // fallthrough } } - collect_call_arguments(node, contents, symbols)?; + collect_call_arguments(ctx, node, contents, symbols)?; Ok(()) } fn collect_call_arguments( + ctx: &mut CollectContext, node: &Node, contents: &Rope, symbols: &mut Vec, @@ -289,28 +294,24 @@ fn collect_call_arguments( return Ok(()); }; - let mut cursor = node.walk(); + let mut cursor = arguments.walk(); for arg in arguments.children(&mut cursor) { - if arg.kind() != "argument" { - continue; - } - let Some(arg_value) = arg.child_by_field_name("value") else { continue; }; // Recurse into arguments. They might be a braced list, another call // that might contain functions, etc. - collect_symbols(&arg_value, contents, 0, symbols)?; + collect_symbols(ctx, &arg_value, contents, 0, symbols)?; if arg_value.kind() == "function_definition" { if let Some(arg_fun) = arg.child_by_field_name("name") { // If this is a named function, collect it as a method - collect_method(&arg_fun, &arg_value, contents, symbols)?; + collect_method(ctx, &arg_fun, &arg_value, contents, symbols)?; } else { // Otherwise, just recurse into the function let body = arg_value.child_by_field_name("body").into_result()?; - collect_symbols(&body, contents, 0, symbols)?; + collect_symbols(ctx, &body, contents, 0, symbols)?; }; } } @@ -319,6 +320,7 @@ fn collect_call_arguments( } fn collect_method( + ctx: &mut CollectContext, arg_fun: &Node, arg_value: &Node, contents: &Rope, @@ -334,7 +336,7 @@ fn collect_method( let body = arg_value.child_by_field_name("body").into_result()?; let mut children = vec![]; - collect_symbols(&body, contents, 0, &mut children)?; + collect_symbols(ctx, &body, contents, 0, &mut children)?; let mut symbol = new_symbol_node( arg_name_str, @@ -355,6 +357,7 @@ fn collect_method( // https://github.com/posit-dev/positron/issues/1428 fn collect_call_test_that( + ctx: &mut CollectContext, node: &Node, contents: &Rope, symbols: &mut Vec, @@ -381,7 +384,7 @@ fn collect_call_test_that( let mut cursor = arguments.walk(); for child in arguments.children_by_field_name("argument", &mut cursor) { if let Some(value) = child.child_by_field_name("value") { - collect_symbols(&value, contents, 0, &mut children)?; + collect_symbols(ctx, &value, contents, 0, &mut children)?; } } @@ -398,6 +401,7 @@ fn collect_call_test_that( } fn collect_assignment( + ctx: &mut CollectContext, node: &Node, contents: &Rope, symbols: &mut Vec, @@ -418,7 +422,7 @@ fn collect_assignment( // If a function, collect symbol as function let function = lhs.is_identifier_or_string() && rhs.is_function_definition(); if function { - return collect_assignment_with_function(node, contents, symbols); + return collect_assignment_with_function(ctx, node, contents, symbols); } // Otherwise, collect as generic object @@ -429,7 +433,7 @@ fn collect_assignment( // Now recurse into RHS let mut children = Vec::new(); - collect_symbols(&rhs, contents, 0, &mut children)?; + collect_symbols(ctx, &rhs, contents, 0, &mut children)?; let symbol = new_symbol_node(name, SymbolKind::VARIABLE, Range { start, end }, children); symbols.push(symbol); @@ -438,6 +442,7 @@ fn collect_assignment( } fn collect_assignment_with_function( + ctx: &mut CollectContext, node: &Node, contents: &Rope, symbols: &mut Vec, @@ -469,7 +474,7 @@ fn collect_assignment_with_function( // Process the function body to extract child symbols let mut children = Vec::new(); - collect_symbols(&body, contents, 0, &mut children)?; + collect_symbols(ctx, &body, contents, 0, &mut children)?; let mut symbol = new_symbol_node(name, SymbolKind::FUNCTION, range, children); symbol.detail = Some(detail); @@ -536,7 +541,8 @@ mod tests { let node = doc.ast.root_node(); let mut symbols = Vec::new(); - collect_symbols(&node, &doc.contents, 0, &mut symbols).unwrap(); + let mut ctx = CollectContext; + collect_symbols(&mut ctx, &node, &doc.contents, 0, &mut symbols).unwrap(); symbols } From 86585c463a9889c56763958fdb53924e03fad2c4 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 27 Jun 2025 15:03:21 +0200 Subject: [PATCH 08/13] Don't emit assigned objects in nested contexts as document symbols --- ...ts__symbol_assignment_function_nested.snap | 71 ++++++++++ ...ols__tests__symbol_nested_assignments.snap | 71 ++++++++++ crates/ark/src/lsp/symbols.rs | 123 +++++++++--------- 3 files changed, 206 insertions(+), 59 deletions(-) create mode 100644 crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_assignment_function_nested.snap create mode 100644 crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_nested_assignments.snap diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_assignment_function_nested.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_assignment_function_nested.snap new file mode 100644 index 000000000..c12ca2b32 --- /dev/null +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_assignment_function_nested.snap @@ -0,0 +1,71 @@ +--- +source: crates/ark/src/lsp/symbols.rs +expression: "test_symbol(\"foo <- function() { bar <- function() 1 }\")" +--- +[ + DocumentSymbol { + name: "foo", + detail: Some( + "function()", + ), + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 0, + character: 0, + }, + end: Position { + line: 0, + character: 41, + }, + }, + selection_range: Range { + start: Position { + line: 0, + character: 0, + }, + end: Position { + line: 0, + character: 41, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "bar", + detail: Some( + "function()", + ), + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 0, + character: 20, + }, + end: Position { + line: 0, + character: 39, + }, + }, + selection_range: Range { + start: Position { + line: 0, + character: 20, + }, + end: Position { + line: 0, + character: 39, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, +] diff --git a/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_nested_assignments.snap b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_nested_assignments.snap new file mode 100644 index 000000000..4cc8ab1c1 --- /dev/null +++ b/crates/ark/src/lsp/snapshots/ark__lsp__symbols__tests__symbol_nested_assignments.snap @@ -0,0 +1,71 @@ +--- +source: crates/ark/src/lsp/symbols.rs +expression: "test_symbol(\"\nlocal({\n inner1 <- 1 # Not a symbol\n})\na <- function() {\n inner2 <- 2 # Not a symbol\n inner3 <- function() 3 # Symbol\n}\n\")" +--- +[ + DocumentSymbol { + name: "a", + detail: Some( + "function()", + ), + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 4, + character: 0, + }, + end: Position { + line: 7, + character: 1, + }, + }, + selection_range: Range { + start: Position { + line: 4, + character: 0, + }, + end: Position { + line: 7, + character: 1, + }, + }, + children: Some( + [ + DocumentSymbol { + name: "inner3", + detail: Some( + "function()", + ), + kind: Function, + tags: None, + deprecated: None, + range: Range { + start: Position { + line: 6, + character: 2, + }, + end: Position { + line: 6, + character: 24, + }, + }, + selection_range: Range { + start: Position { + line: 6, + character: 2, + }, + end: Position { + line: 6, + character: 24, + }, + }, + children: Some( + [], + ), + }, + ], + ), + }, +] diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index 124ede763..6343ce703 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -122,7 +122,15 @@ struct Section { children: Vec, } -struct CollectContext; +struct CollectContext { + top_level: bool, +} + +impl CollectContext { + fn new() -> Self { + Self { top_level: true } + } +} pub(crate) fn document_symbols( state: &WorldState, @@ -138,8 +146,13 @@ pub(crate) fn document_symbols( let mut result = Vec::new(); // Extract and process all symbols from the AST - let mut ctx = CollectContext; - if let Err(err) = collect_symbols(&mut ctx, &root_node, contents, 0, &mut result) { + if let Err(err) = collect_symbols( + &mut CollectContext::new(), + &root_node, + contents, + 0, + &mut result, + ) { log::error!("Failed to collect symbols: {err:?}"); return Ok(Vec::new()); } @@ -156,7 +169,12 @@ fn collect_symbols( symbols: &mut Vec, ) -> anyhow::Result<()> { match node.node_type() { - NodeType::Program | NodeType::BracedExpression => { + NodeType::Program => { + collect_sections(ctx, node, contents, current_level, symbols)?; + }, + + NodeType::BracedExpression => { + ctx.top_level = false; collect_sections(ctx, node, contents, current_level, symbols)?; }, @@ -425,18 +443,25 @@ fn collect_assignment( return collect_assignment_with_function(ctx, node, contents, symbols); } - // Otherwise, collect as generic object - let name = contents.node_slice(&lhs)?.to_string(); + if ctx.top_level { + // Collect as generic object, but only if we're at top-level. Assigned + // objects in nested functions and blocks cause the outline to become + // too busy. + let name = contents.node_slice(&lhs)?.to_string(); - let start = convert_point_to_position(contents, lhs.start_position()); - let end = convert_point_to_position(contents, lhs.end_position()); + let start = convert_point_to_position(contents, lhs.start_position()); + let end = convert_point_to_position(contents, lhs.end_position()); - // Now recurse into RHS - let mut children = Vec::new(); - collect_symbols(ctx, &rhs, contents, 0, &mut children)?; + // Now recurse into RHS + let mut children = Vec::new(); + collect_symbols(ctx, &rhs, contents, 0, &mut children)?; - let symbol = new_symbol_node(name, SymbolKind::VARIABLE, Range { start, end }, children); - symbols.push(symbol); + let symbol = new_symbol_node(name, SymbolKind::VARIABLE, Range { start, end }, children); + symbols.push(symbol); + } else { + // Recurse into RHS + collect_symbols(ctx, &rhs, contents, 0, symbols)?; + } Ok(()) } @@ -541,8 +566,14 @@ mod tests { let node = doc.ast.root_node(); let mut symbols = Vec::new(); - let mut ctx = CollectContext; - collect_symbols(&mut ctx, &node, &doc.contents, 0, &mut symbols).unwrap(); + collect_symbols( + &mut CollectContext::new(), + &node, + &doc.contents, + 0, + &mut symbols, + ) + .unwrap(); symbols } @@ -620,33 +651,7 @@ mod tests { #[test] fn test_symbol_assignment_function_nested() { - let range = Range { - start: Position { - line: 0, - character: 20, - }, - end: Position { - line: 0, - character: 23, - }, - }; - let bar = new_symbol(String::from("bar"), SymbolKind::VARIABLE, range); - - let range = Range { - start: Position { - line: 0, - character: 0, - }, - end: Position { - line: 0, - character: 30, - }, - }; - let mut foo = new_symbol(String::from("foo"), SymbolKind::FUNCTION, range); - foo.children = Some(vec![bar]); - foo.detail = Some(String::from("function()")); - - assert_eq!(test_symbol("foo <- function() { bar <- 1 }"), vec![foo]); + insta::assert_debug_snapshot!(test_symbol("foo <- function() { bar <- function() 1 }")); } #[test] @@ -664,23 +669,6 @@ foo <- function() { )); } - #[test] - fn test_symbol_braced_list() { - let range = Range { - start: Position { - line: 0, - character: 2, - }, - end: Position { - line: 0, - character: 5, - }, - }; - let foo = new_symbol(String::from("foo"), SymbolKind::VARIABLE, range); - - assert_eq!(test_symbol("{ foo <- 1 }"), vec![foo]); - } - #[test] fn test_symbol_section_ranges_extend() { let symbols = test_symbol( @@ -840,4 +828,21 @@ class <- r6::r6class( " )); } + + #[test] + // Assigned variables in nested contexts are not emitted as symbols + fn test_symbol_nested_assignments() { + insta::assert_debug_snapshot!(test_symbol( + " +local({ + inner1 <- 1 # Not a symbol +}) +a <- function() { + inner2 <- 2 # Not a symbol + inner3 <- function() 3 # Symbol +} +" + )); + assert_eq!(test_symbol("{ foo <- 1 }"), vec![]); + } } From 2ce4f0169f3725389b6c66dee6831db0874ac4a6 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 4 Jul 2025 14:47:05 +0200 Subject: [PATCH 09/13] Add symbols config --- crates/ark/src/lsp/config.rs | 36 +++++++++++++++++++++++++--- crates/ark/src/lsp/handlers.rs | 6 +++++ crates/ark/src/lsp/state_handlers.rs | 31 +++++++++++++++++++++++- 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/crates/ark/src/lsp/config.rs b/crates/ark/src/lsp/config.rs index d9f51ed90..e7041b048 100644 --- a/crates/ark/src/lsp/config.rs +++ b/crates/ark/src/lsp/config.rs @@ -6,9 +6,16 @@ use crate::lsp; use crate::lsp::diagnostics::DiagnosticsConfig; /// Configuration of the LSP -#[derive(Clone, Debug)] +#[derive(Clone, Default, Debug)] pub(crate) struct LspConfig { pub(crate) diagnostics: DiagnosticsConfig, + pub(crate) symbols: SymbolsConfig, +} + +#[derive(Serialize, Deserialize, Clone, Debug)] +pub struct SymbolsConfig { + /// Whether to emit assignments in `{` bloks as document symbols. + pub include_assignments_in_blocks: bool, } /// Configuration of a document. @@ -53,6 +60,12 @@ pub(crate) struct VscDiagnosticsConfig { pub enable: bool, } +#[derive(Serialize, Deserialize, FieldNamesAsArray, Clone, Debug)] +pub(crate) struct VscSymbolsConfig { + // DEV NOTE: Update `section_from_key()` method after adding a field + pub include_assignments_in_blocks: bool, +} + #[derive(Serialize, Deserialize, Clone, Debug)] #[serde(untagged)] pub(crate) enum VscIndentSize { @@ -60,10 +73,10 @@ pub(crate) enum VscIndentSize { Size(usize), } -impl Default for LspConfig { +impl Default for SymbolsConfig { fn default() -> Self { Self { - diagnostics: Default::default(), + include_assignments_in_blocks: false, } } } @@ -134,6 +147,23 @@ impl From for DiagnosticsConfig { } } +impl VscSymbolsConfig { + pub(crate) fn section_from_key(key: &str) -> &str { + match key { + "include_assignments_in_blocks" => "positron.r.symbols.includeAssignmentsInBlocks", + _ => "unknown", // To be caught via downstream errors + } + } +} + +impl From for SymbolsConfig { + fn from(value: VscSymbolsConfig) -> Self { + Self { + include_assignments_in_blocks: value.include_assignments_in_blocks, + } + } +} + pub(crate) fn indent_style_from_lsp(insert_spaces: bool) -> IndentStyle { if insert_spaces { IndentStyle::Space diff --git a/crates/ark/src/lsp/handlers.rs b/crates/ark/src/lsp/handlers.rs index d6a6ea118..7cb115e7c 100644 --- a/crates/ark/src/lsp/handlers.rs +++ b/crates/ark/src/lsp/handlers.rs @@ -48,6 +48,7 @@ use crate::lsp::completions::provide_completions; use crate::lsp::completions::resolve_completion; use crate::lsp::config::VscDiagnosticsConfig; use crate::lsp::config::VscDocumentConfig; +use crate::lsp::config::VscSymbolsConfig; use crate::lsp::definitions::goto_definition; use crate::lsp::document_context::DocumentContext; use crate::lsp::encoding::convert_lsp_range_to_tree_sitter_range; @@ -109,12 +110,17 @@ pub(crate) async fn handle_initialized( VscDocumentConfig::FIELD_NAMES_AS_ARRAY.to_vec(), VscDocumentConfig::section_from_key, ); + let mut config_symbols_regs: Vec = collect_regs( + VscSymbolsConfig::FIELD_NAMES_AS_ARRAY.to_vec(), + VscSymbolsConfig::section_from_key, + ); let mut config_diagnostics_regs: Vec = collect_regs( VscDiagnosticsConfig::FIELD_NAMES_AS_ARRAY.to_vec(), VscDiagnosticsConfig::section_from_key, ); regs.append(&mut config_document_regs); + regs.append(&mut config_symbols_regs); regs.append(&mut config_diagnostics_regs); } diff --git a/crates/ark/src/lsp/state_handlers.rs b/crates/ark/src/lsp/state_handlers.rs index c5f2a20e0..1bca6fcc0 100644 --- a/crates/ark/src/lsp/state_handlers.rs +++ b/crates/ark/src/lsp/state_handlers.rs @@ -43,8 +43,10 @@ use crate::lsp; use crate::lsp::capabilities::Capabilities; use crate::lsp::config::indent_style_from_lsp; use crate::lsp::config::DocumentConfig; +use crate::lsp::config::SymbolsConfig; use crate::lsp::config::VscDiagnosticsConfig; use crate::lsp::config::VscDocumentConfig; +use crate::lsp::config::VscSymbolsConfig; use crate::lsp::diagnostics::DiagnosticsConfig; use crate::lsp::documents::Document; use crate::lsp::encoding::get_position_encoding_kind; @@ -244,6 +246,9 @@ pub(crate) async fn did_change_configuration( // we should just ignore it. Instead we need to pull the settings again for // all URI of interest. + // Note that the client sends notifications for settings for which we have + // declared interest in. This registration is done in `handle_initialized()`. + update_config(workspace_uris(state), client, state) .instrument(tracing::info_span!("did_change_configuration")) .await @@ -296,6 +301,16 @@ async fn update_config( .collect(); items.append(&mut diagnostics_items); + let symbols_keys = VscSymbolsConfig::FIELD_NAMES_AS_ARRAY; + let mut symbols_items: Vec = symbols_keys + .iter() + .map(|key| ConfigurationItem { + scope_uri: None, + section: Some(VscSymbolsConfig::section_from_key(key).into()), + }) + .collect(); + items.append(&mut symbols_items); + // For document configs we collect all pairs of URIs and config keys of // interest in a flat vector let document_keys = VscDocumentConfig::FIELD_NAMES_AS_ARRAY; @@ -316,7 +331,8 @@ async fn update_config( // by chunk let n_document_items = document_keys.len(); let n_diagnostics_items = diagnostics_keys.len(); - let n_items = n_diagnostics_items + (n_document_items * uris.len()); + let n_symbols_items = symbols_keys.len(); + let n_items = n_diagnostics_items + n_symbols_items + (n_document_items * uris.len()); if configs.len() != n_items { return Err(anyhow!( @@ -351,6 +367,19 @@ async fn update_config( lsp::spawn_diagnostics_refresh_all(state.clone()); } + // --- Symbols + let keys = symbols_keys.into_iter(); + let items: Vec = configs.by_ref().take(n_symbols_items).collect(); + + let mut map = serde_json::Map::new(); + std::iter::zip(keys, items).for_each(|(key, item)| { + map.insert(key.into(), item); + }); + + let config: VscSymbolsConfig = serde_json::from_value(serde_json::Value::Object(map))?; + let config: SymbolsConfig = config.into(); + state.config.symbols = config; + // --- Documents // For each document, deserialise the vector of JSON values into a typed config for uri in uris.into_iter() { From a4f5a00c9f0e68c328c37bcbd0edbb0e2646ead9 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Sat, 5 Jul 2025 09:35:52 +0200 Subject: [PATCH 10/13] Make inclusion in outline of assignments in blocks optional --- crates/ark/src/lsp/symbols.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index 6343ce703..4d3000d7d 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -124,11 +124,15 @@ struct Section { struct CollectContext { top_level: bool, + include_assignments_in_blocks: bool, } impl CollectContext { fn new() -> Self { - Self { top_level: true } + Self { + top_level: true, + include_assignments_in_blocks: false, + } } } @@ -145,14 +149,11 @@ pub(crate) fn document_symbols( let root_node = ast.root_node(); let mut result = Vec::new(); + let mut ctx = CollectContext::new(); + ctx.include_assignments_in_blocks = state.config.symbols.include_assignments_in_blocks; + // Extract and process all symbols from the AST - if let Err(err) = collect_symbols( - &mut CollectContext::new(), - &root_node, - contents, - 0, - &mut result, - ) { + if let Err(err) = collect_symbols(&mut ctx, &root_node, contents, 0, &mut result) { log::error!("Failed to collect symbols: {err:?}"); return Ok(Vec::new()); } @@ -443,7 +444,7 @@ fn collect_assignment( return collect_assignment_with_function(ctx, node, contents, symbols); } - if ctx.top_level { + if ctx.top_level || ctx.include_assignments_in_blocks { // Collect as generic object, but only if we're at top-level. Assigned // objects in nested functions and blocks cause the outline to become // too busy. From 816b26020a93ba6929555d9c0feb1164b45f75e3 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Sat, 5 Jul 2025 10:28:05 +0200 Subject: [PATCH 11/13] Simplify LSP settings --- Cargo.lock | 21 ---- crates/ark/Cargo.toml | 1 - crates/ark/src/lsp/config.rs | 145 ++++++++++++--------------- crates/ark/src/lsp/handlers.rs | 41 ++------ crates/ark/src/lsp/state_handlers.rs | 119 +++------------------- 5 files changed, 88 insertions(+), 239 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5664adab1..c51e63d67 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -319,7 +319,6 @@ dependencies = [ "serde", "serde_json", "stdext", - "struct-field-names-as-array", "strum 0.26.2", "strum_macros 0.26.4", "tempfile", @@ -2744,26 +2743,6 @@ version = "0.10.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "73473c0e59e6d5812c5dfe2a064a6444949f089e20eec9a2e5506596494e4623" -[[package]] -name = "struct-field-names-as-array" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "22ba4bae771f9cc992c4f403636c54d2ef13acde6367583e99d06bb336674dd9" -dependencies = [ - "struct-field-names-as-array-derive", -] - -[[package]] -name = "struct-field-names-as-array-derive" -version = "0.3.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a2dbf8b57f3ce20e4bb171a11822b283bdfab6c4bb0fe64fa729f045f23a0938" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.32", -] - [[package]] name = "strum" version = "0.24.1" diff --git a/crates/ark/Cargo.toml b/crates/ark/Cargo.toml index d51c0185b..8a1fb4df5 100644 --- a/crates/ark/Cargo.toml +++ b/crates/ark/Cargo.toml @@ -53,7 +53,6 @@ url = "2.4.1" walkdir = "2" yaml-rust = "0.4.5" winsafe = { version = "0.0.19", features = ["kernel"] } -struct-field-names-as-array = "0.3.0" strum = "0.26.2" strum_macros = "0.26.2" futures = "0.3.30" diff --git a/crates/ark/src/lsp/config.rs b/crates/ark/src/lsp/config.rs index e7041b048..79b8ee886 100644 --- a/crates/ark/src/lsp/config.rs +++ b/crates/ark/src/lsp/config.rs @@ -1,15 +1,75 @@ use serde::Deserialize; use serde::Serialize; -use struct_field_names_as_array::FieldNamesAsArray; +use serde_json::Value; -use crate::lsp; use crate::lsp::diagnostics::DiagnosticsConfig; +pub struct Setting { + pub key: &'static str, + pub set: fn(&mut LspConfig, Value), +} + +// List of LSP settings for which clients can send `didChangeConfiguration` +// notifications. We register our interest in watching over these settings in +// our `initialized` handler. The `set` methods convert from a json `Value` to +// the expected type, using a default value if the conversion fails. +pub static SETTINGS: &[Setting] = &[ + Setting { + key: "editor.insertSpaces", + set: |cfg, v| { + let default_style = IndentationConfig::default().indent_style; + cfg.document.indent.indent_style = if v + .as_bool() + .unwrap_or_else(|| default_style == IndentStyle::Space) + { + IndentStyle::Space + } else { + IndentStyle::Tab + } + }, + }, + Setting { + key: "editor.indentSize", + set: |cfg, v| { + cfg.document.indent.indent_size = v + .as_u64() + .map(|n| n as usize) + .unwrap_or_else(|| IndentationConfig::default().indent_size) + }, + }, + Setting { + key: "editor.tabSize", + set: |cfg, v| { + cfg.document.indent.tab_width = v + .as_u64() + .map(|n| n as usize) + .unwrap_or_else(|| IndentationConfig::default().tab_width) + }, + }, + Setting { + key: "positron.r.diagnostics.enable", + set: |cfg, v| { + cfg.diagnostics.enable = v + .as_bool() + .unwrap_or_else(|| DiagnosticsConfig::default().enable) + }, + }, + Setting { + key: "positron.r.symbols.includeAssignmentsInBlocks", + set: |cfg, v| { + cfg.symbols.include_assignments_in_blocks = v + .as_bool() + .unwrap_or_else(|| SymbolsConfig::default().include_assignments_in_blocks) + }, + }, +]; + /// Configuration of the LSP #[derive(Clone, Default, Debug)] pub(crate) struct LspConfig { pub(crate) diagnostics: DiagnosticsConfig, pub(crate) symbols: SymbolsConfig, + pub(crate) document: DocumentConfig, } #[derive(Serialize, Deserialize, Clone, Debug)] @@ -39,14 +99,14 @@ pub struct IndentationConfig { pub tab_width: usize, } -#[derive(Serialize, Deserialize, Clone, Debug)] +#[derive(PartialEq, Serialize, Deserialize, Clone, Debug)] pub enum IndentStyle { Tab, Space, } /// VS Code representation of a document configuration -#[derive(Serialize, Deserialize, FieldNamesAsArray, Clone, Debug)] +#[derive(Serialize, Deserialize, Clone, Debug)] pub(crate) struct VscDocumentConfig { // DEV NOTE: Update `section_from_key()` method after adding a field pub insert_spaces: bool, @@ -54,13 +114,13 @@ pub(crate) struct VscDocumentConfig { pub tab_size: usize, } -#[derive(Serialize, Deserialize, FieldNamesAsArray, Clone, Debug)] +#[derive(Serialize, Deserialize, Clone, Debug)] pub(crate) struct VscDiagnosticsConfig { // DEV NOTE: Update `section_from_key()` method after adding a field pub enable: bool, } -#[derive(Serialize, Deserialize, FieldNamesAsArray, Clone, Debug)] +#[derive(Serialize, Deserialize, Clone, Debug)] pub(crate) struct VscSymbolsConfig { // DEV NOTE: Update `section_from_key()` method after adding a field pub include_assignments_in_blocks: bool, @@ -91,79 +151,6 @@ impl Default for IndentationConfig { } } -impl VscDocumentConfig { - pub(crate) fn section_from_key(key: &str) -> &str { - match key { - "insert_spaces" => "editor.insertSpaces", - "indent_size" => "editor.indentSize", - "tab_size" => "editor.tabSize", - _ => "unknown", // To be caught via downstream errors - } - } -} - -/// Convert from VS Code representation of a document config to our own -/// representation. Currently one-to-one. -impl From for DocumentConfig { - fn from(x: VscDocumentConfig) -> Self { - let indent_style = indent_style_from_lsp(x.insert_spaces); - - let indent_size = match x.indent_size { - VscIndentSize::Size(size) => size, - VscIndentSize::Alias(var) => { - if var == "tabSize" { - x.tab_size - } else { - lsp::log_warn!("Unknown indent alias {var}, using default"); - 2 - } - }, - }; - - Self { - indent: IndentationConfig { - indent_style, - indent_size, - tab_width: x.tab_size, - }, - } - } -} - -impl VscDiagnosticsConfig { - pub(crate) fn section_from_key(key: &str) -> &str { - match key { - "enable" => "positron.r.diagnostics.enable", - _ => "unknown", // To be caught via downstream errors - } - } -} - -impl From for DiagnosticsConfig { - fn from(value: VscDiagnosticsConfig) -> Self { - Self { - enable: value.enable, - } - } -} - -impl VscSymbolsConfig { - pub(crate) fn section_from_key(key: &str) -> &str { - match key { - "include_assignments_in_blocks" => "positron.r.symbols.includeAssignmentsInBlocks", - _ => "unknown", // To be caught via downstream errors - } - } -} - -impl From for SymbolsConfig { - fn from(value: VscSymbolsConfig) -> Self { - Self { - include_assignments_in_blocks: value.include_assignments_in_blocks, - } - } -} - pub(crate) fn indent_style_from_lsp(insert_spaces: bool) -> IndentStyle { if insert_spaces { IndentStyle::Space diff --git a/crates/ark/src/lsp/handlers.rs b/crates/ark/src/lsp/handlers.rs index 7cb115e7c..c111cfb82 100644 --- a/crates/ark/src/lsp/handlers.rs +++ b/crates/ark/src/lsp/handlers.rs @@ -9,7 +9,6 @@ use anyhow::anyhow; use serde_json::Value; use stdext::unwrap; use stdext::unwrap::IntoResult; -use struct_field_names_as_array::FieldNamesAsArray; use tower_lsp::lsp_types::CodeActionParams; use tower_lsp::lsp_types::CodeActionResponse; use tower_lsp::lsp_types::CompletionItem; @@ -46,9 +45,6 @@ use crate::lsp; use crate::lsp::code_action::code_actions; use crate::lsp::completions::provide_completions; use crate::lsp::completions::resolve_completion; -use crate::lsp::config::VscDiagnosticsConfig; -use crate::lsp::config::VscDocumentConfig; -use crate::lsp::config::VscSymbolsConfig; use crate::lsp::definitions::goto_definition; use crate::lsp::document_context::DocumentContext; use crate::lsp::encoding::convert_lsp_range_to_tree_sitter_range; @@ -106,22 +102,16 @@ pub(crate) async fn handle_initialized( // Note that some settings, such as editor indentation properties, may be // changed by extensions or by the user without changing the actual // underlying setting. Unfortunately we don't receive updates in that case. - let mut config_document_regs = collect_regs( - VscDocumentConfig::FIELD_NAMES_AS_ARRAY.to_vec(), - VscDocumentConfig::section_from_key, - ); - let mut config_symbols_regs: Vec = collect_regs( - VscSymbolsConfig::FIELD_NAMES_AS_ARRAY.to_vec(), - VscSymbolsConfig::section_from_key, - ); - let mut config_diagnostics_regs: Vec = collect_regs( - VscDiagnosticsConfig::FIELD_NAMES_AS_ARRAY.to_vec(), - VscDiagnosticsConfig::section_from_key, - ); - - regs.append(&mut config_document_regs); - regs.append(&mut config_symbols_regs); - regs.append(&mut config_diagnostics_regs); + + use crate::lsp::config::SETTINGS; + + for setting in SETTINGS { + regs.push(Registration { + id: uuid::Uuid::new_v4().to_string(), + method: String::from("workspace/didChangeConfiguration"), + register_options: Some(serde_json::json!({ "section": setting.key })), + }); + } } client @@ -131,17 +121,6 @@ pub(crate) async fn handle_initialized( Ok(()) } -fn collect_regs(fields: Vec<&str>, into_section: impl Fn(&str) -> &str) -> Vec { - fields - .into_iter() - .map(|field| Registration { - id: uuid::Uuid::new_v4().to_string(), - method: String::from("workspace/didChangeConfiguration"), - register_options: Some(serde_json::json!({ "section": into_section(field) })), - }) - .collect() -} - #[tracing::instrument(level = "info", skip_all)] pub(crate) fn handle_symbol( params: WorkspaceSymbolParams, diff --git a/crates/ark/src/lsp/state_handlers.rs b/crates/ark/src/lsp/state_handlers.rs index 1bca6fcc0..b748d038d 100644 --- a/crates/ark/src/lsp/state_handlers.rs +++ b/crates/ark/src/lsp/state_handlers.rs @@ -8,11 +8,8 @@ use std::path::Path; use anyhow::anyhow; -use serde_json::Value; -use struct_field_names_as_array::FieldNamesAsArray; use tower_lsp::lsp_types::CompletionOptions; use tower_lsp::lsp_types::CompletionOptionsCompletionItem; -use tower_lsp::lsp_types::ConfigurationItem; use tower_lsp::lsp_types::DidChangeConfigurationParams; use tower_lsp::lsp_types::DidChangeTextDocumentParams; use tower_lsp::lsp_types::DidCloseTextDocumentParams; @@ -42,12 +39,6 @@ use url::Url; use crate::lsp; use crate::lsp::capabilities::Capabilities; use crate::lsp::config::indent_style_from_lsp; -use crate::lsp::config::DocumentConfig; -use crate::lsp::config::SymbolsConfig; -use crate::lsp::config::VscDiagnosticsConfig; -use crate::lsp::config::VscDocumentConfig; -use crate::lsp::config::VscSymbolsConfig; -use crate::lsp::diagnostics::DiagnosticsConfig; use crate::lsp::documents::Document; use crate::lsp::encoding::get_position_encoding_kind; use crate::lsp::indexer; @@ -284,121 +275,35 @@ pub(crate) fn did_change_formatting_options( // `insert_final_newline` } +use crate::lsp::config::SETTINGS; + async fn update_config( - uris: Vec, + _uris: Vec, client: &tower_lsp::Client, state: &mut WorldState, ) -> anyhow::Result<()> { - let mut items: Vec = vec![]; - - let diagnostics_keys = VscDiagnosticsConfig::FIELD_NAMES_AS_ARRAY; - let mut diagnostics_items: Vec = diagnostics_keys - .iter() - .map(|key| ConfigurationItem { - scope_uri: None, - section: Some(VscDiagnosticsConfig::section_from_key(key).into()), - }) - .collect(); - items.append(&mut diagnostics_items); - - let symbols_keys = VscSymbolsConfig::FIELD_NAMES_AS_ARRAY; - let mut symbols_items: Vec = symbols_keys + // Build the configuration request for global settings + let items: Vec<_> = SETTINGS .iter() - .map(|key| ConfigurationItem { + .map(|mapping| tower_lsp::lsp_types::ConfigurationItem { scope_uri: None, - section: Some(VscSymbolsConfig::section_from_key(key).into()), + section: Some(mapping.key.to_string()), }) .collect(); - items.append(&mut symbols_items); - - // For document configs we collect all pairs of URIs and config keys of - // interest in a flat vector - let document_keys = VscDocumentConfig::FIELD_NAMES_AS_ARRAY; - let mut document_items: Vec = - itertools::iproduct!(uris.iter(), document_keys.iter()) - .map(|(uri, key)| ConfigurationItem { - scope_uri: Some(uri.clone()), - section: Some(VscDocumentConfig::section_from_key(key).into()), - }) - .collect(); - items.append(&mut document_items); let configs = client.configuration(items).await?; - // We got the config items in a flat vector that's guaranteed to be - // ordered in the same way it was sent in. Be defensive and check that - // we've got the expected number of items before we process them chunk - // by chunk - let n_document_items = document_keys.len(); - let n_diagnostics_items = diagnostics_keys.len(); - let n_symbols_items = symbols_keys.len(); - let n_items = n_diagnostics_items + n_symbols_items + (n_document_items * uris.len()); - - if configs.len() != n_items { + if configs.len() != SETTINGS.len() { return Err(anyhow!( "Unexpected number of retrieved configurations: {}/{}", configs.len(), - n_items + SETTINGS.len() )); } - let mut configs = configs.into_iter(); - - // --- Diagnostics - let keys = diagnostics_keys.into_iter(); - let items: Vec = configs.by_ref().take(n_diagnostics_items).collect(); - - // Create a new `serde_json::Value::Object` manually to convert it - // to a `VscDocumentConfig` with `from_value()`. This way serde_json - // can type-check the dynamic JSON value we got from the client. - let mut map = serde_json::Map::new(); - std::iter::zip(keys, items).for_each(|(key, item)| { - map.insert(key.into(), item); - }); - - // Deserialise the VS Code configuration - let config: VscDiagnosticsConfig = serde_json::from_value(serde_json::Value::Object(map))?; - let config: DiagnosticsConfig = config.into(); - - let changed = state.config.diagnostics != config; - state.config.diagnostics = config; - - if changed { - lsp::spawn_diagnostics_refresh_all(state.clone()); - } - - // --- Symbols - let keys = symbols_keys.into_iter(); - let items: Vec = configs.by_ref().take(n_symbols_items).collect(); - - let mut map = serde_json::Map::new(); - std::iter::zip(keys, items).for_each(|(key, item)| { - map.insert(key.into(), item); - }); - - let config: VscSymbolsConfig = serde_json::from_value(serde_json::Value::Object(map))?; - let config: SymbolsConfig = config.into(); - state.config.symbols = config; - - // --- Documents - // For each document, deserialise the vector of JSON values into a typed config - for uri in uris.into_iter() { - let keys = document_keys.into_iter(); - let items: Vec = configs.by_ref().take(n_document_items).collect(); - - let mut map = serde_json::Map::new(); - std::iter::zip(keys, items).for_each(|(key, item)| { - map.insert(key.into(), item); - }); - - // Deserialise the VS Code configuration - let config: VscDocumentConfig = serde_json::from_value(serde_json::Value::Object(map))?; - - // Now convert the VS Code specific type into our own type - let config: DocumentConfig = config.into(); - - // Finally, update the document's config - state.get_document_mut(&uri)?.config = config; + // Apply each config value using its update closure + for (mapping, value) in SETTINGS.iter().zip(configs) { + (mapping.set)(&mut state.config, value); } Ok(()) From 729ec239620d3017a12a626be7b403caa3e97e82 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 7 Jul 2025 13:26:28 +0200 Subject: [PATCH 12/13] Don't include comment sections in workspace symbols by default --- crates/ark/src/lsp/config.rs | 23 +++++++++++++++++++++++ crates/ark/src/lsp/handlers.rs | 3 ++- crates/ark/src/lsp/main_loop.rs | 2 +- crates/ark/src/lsp/symbols.rs | 30 ++++++++++++++++++------------ 4 files changed, 44 insertions(+), 14 deletions(-) diff --git a/crates/ark/src/lsp/config.rs b/crates/ark/src/lsp/config.rs index 79b8ee886..1a269be6c 100644 --- a/crates/ark/src/lsp/config.rs +++ b/crates/ark/src/lsp/config.rs @@ -62,6 +62,14 @@ pub static SETTINGS: &[Setting] = &[ .unwrap_or_else(|| SymbolsConfig::default().include_assignments_in_blocks) }, }, + Setting { + key: "positron.r.workspaceSymbols.includeCommentSections", + set: |cfg, v| { + cfg.workspace_symbols.include_comment_sections = v + .as_bool() + .unwrap_or_else(|| WorkspaceSymbolsConfig::default().include_comment_sections) + }, + }, ]; /// Configuration of the LSP @@ -69,6 +77,7 @@ pub static SETTINGS: &[Setting] = &[ pub(crate) struct LspConfig { pub(crate) diagnostics: DiagnosticsConfig, pub(crate) symbols: SymbolsConfig, + pub(crate) workspace_symbols: WorkspaceSymbolsConfig, pub(crate) document: DocumentConfig, } @@ -78,6 +87,12 @@ pub struct SymbolsConfig { pub include_assignments_in_blocks: bool, } +#[derive(Serialize, Deserialize, Clone, Debug)] +pub struct WorkspaceSymbolsConfig { + /// Whether to include sections like `# My section ---` in workspace symbols. + pub include_comment_sections: bool, +} + /// Configuration of a document. /// /// The naming follows where possible. @@ -141,6 +156,14 @@ impl Default for SymbolsConfig { } } +impl Default for WorkspaceSymbolsConfig { + fn default() -> Self { + Self { + include_comment_sections: false, + } + } +} + impl Default for IndentationConfig { fn default() -> Self { Self { diff --git a/crates/ark/src/lsp/handlers.rs b/crates/ark/src/lsp/handlers.rs index c111cfb82..cc468c5ec 100644 --- a/crates/ark/src/lsp/handlers.rs +++ b/crates/ark/src/lsp/handlers.rs @@ -124,8 +124,9 @@ pub(crate) async fn handle_initialized( #[tracing::instrument(level = "info", skip_all)] pub(crate) fn handle_symbol( params: WorkspaceSymbolParams, + state: &WorldState, ) -> anyhow::Result>> { - symbols::symbols(¶ms) + symbols::symbols(¶ms, state) .map(|res| Some(res)) .or_else(|err| { // Missing doc: Why are we not propagating errors to the frontend? diff --git a/crates/ark/src/lsp/main_loop.rs b/crates/ark/src/lsp/main_loop.rs index ac5588cea..0ee4be1a3 100644 --- a/crates/ark/src/lsp/main_loop.rs +++ b/crates/ark/src/lsp/main_loop.rs @@ -281,7 +281,7 @@ impl GlobalState { respond(tx, || state_handlers::initialize(params, &mut self.lsp_state, &mut self.world), LspResponse::Initialize)?; }, LspRequest::WorkspaceSymbol(params) => { - respond(tx, || handlers::handle_symbol(params), LspResponse::WorkspaceSymbol)?; + respond(tx, || handlers::handle_symbol(params, &self.world), LspResponse::WorkspaceSymbol)?; }, LspRequest::DocumentSymbol(params) => { respond(tx, || handlers::handle_document_symbol(params, &self.world), LspResponse::DocumentSymbol)?; diff --git a/crates/ark/src/lsp/symbols.rs b/crates/ark/src/lsp/symbols.rs index 4d3000d7d..ad70ff049 100644 --- a/crates/ark/src/lsp/symbols.rs +++ b/crates/ark/src/lsp/symbols.rs @@ -56,7 +56,10 @@ fn new_symbol_node( symbol } -pub fn symbols(params: &WorkspaceSymbolParams) -> anyhow::Result> { +pub(crate) fn symbols( + params: &WorkspaceSymbolParams, + state: &WorldState, +) -> anyhow::Result> { let query = ¶ms.query; let mut info: Vec = Vec::new(); @@ -81,18 +84,21 @@ pub fn symbols(params: &WorkspaceSymbolParams) -> anyhow::Result { - info.push(SymbolInformation { - name: title.to_string(), - kind: SymbolKind::STRING, - location: Location { - uri: Url::from_file_path(path).unwrap(), - range: entry.range, - }, - tags: None, - deprecated: None, - container_name: None, - }); + if state.config.workspace_symbols.include_comment_sections { + info.push(SymbolInformation { + name: title.to_string(), + kind: SymbolKind::STRING, + location: Location { + uri: Url::from_file_path(path).unwrap(), + range: entry.range, + }, + tags: None, + deprecated: None, + container_name: None, + }); + } }, + IndexEntryData::Variable { name } => { info.push(SymbolInformation { name: name.clone(), From 120984ce12122f4435bf903b7b0568a42aa0be9a Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 7 Jul 2025 13:55:17 +0200 Subject: [PATCH 13/13] Give priority to non-section entries --- crates/ark/src/lsp/indexer.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/ark/src/lsp/indexer.rs b/crates/ark/src/lsp/indexer.rs index 34f018c7b..dc64e5a79 100644 --- a/crates/ark/src/lsp/indexer.rs +++ b/crates/ark/src/lsp/indexer.rs @@ -121,10 +121,17 @@ fn insert(path: &Path, entry: IndexEntry) -> anyhow::Result<()> { let index = index.entry(path.to_string()).or_default(); - // Retain the first occurrence in the index. In the future we'll track every occurrences and - // their scopes but for now we only track the first definition of an object (in a way, its + // We generally retain only the first occurrence in the index. In the + // future we'll track every occurrences and their scopes but for now we + // only track the first definition of an object (in a way, its // declaration). - if !index.contains_key(&entry.key) { + if let Some(existing_entry) = index.get(&entry.key) { + // Give priority to non-section entries. + if matches!(existing_entry.data, IndexEntryData::Section { .. }) { + index.insert(entry.key.clone(), entry); + } + // Else, ignore. + } else { index.insert(entry.key.clone(), entry); }