Support semi colons on single line in statement range detection (#638)

* Handle multiple expressions on single line in statement range

* Add failing tests for multiline expressions

* Mention in changelog

* Use tree-sitter approach to expand across semicolons (#644)

---------

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

CHANGELOG.md

@@ -2,6 +2,10 @@
 
 # 0.1.9000
 
+## 2024-12
+
+- LSP: The statement range provider now has better support for expressions separated by `;` on a single line (posit-dev/positron#4317).
+
 ## 2024-11
 
 - LSP: Assignments in function calls (e.g. `list(x <- 1)`) are now detected by the missing symbol linter to avoid annoying false positive diagnostics (https://github.com/posit-dev/positron/issues/3048). The downside is that this causes false negatives when the assignment happens in a call with local scope, e.g. in `local()` or `test_that()`. We prefer to be overly permissive than overly cautious in these matters.

crates/ark/src/lsp/statement_range.rs

@@ -1,7 +1,7 @@
 //
 // statement_range.rs
 //
-// Copyright (C) 2023 Posit Software, PBC. All rights reserved.
+// Copyright (C) 2023-2024 Posit Software, PBC. All rights reserved.
 //
 //
 
@@ -13,8 +13,8 @@ use ropey::Rope;
 use serde::Deserialize;
 use serde::Serialize;
 use stdext::unwrap;
+use tower_lsp::lsp_types;
 use tower_lsp::lsp_types::Position;
-use tower_lsp::lsp_types::Range;
 use tower_lsp::lsp_types::VersionedTextDocumentIdentifier;
 use tree_sitter::Node;
 use tree_sitter::Point;
@@ -40,7 +40,7 @@ pub struct StatementRangeParams {
 #[serde(rename_all = "camelCase")]
 pub struct StatementRangeResponse {
     /// The document range the statement covers.
-    pub range: Range,
+    pub range: lsp_types::Range,
     /// Optionally, code to be executed for this `range` if it differs from
     /// what is actually pointed to by the `range` (i.e. roxygen examples).
     pub code: Option<String>,
@@ -61,30 +61,32 @@ pub(crate) fn statement_range(
     // with `code` stripped of the leading `#' ` if we detect that we are in
     // `@examples`.
     if let Some((node, code)) = find_roxygen_comment_at_point(&root, contents, point) {
-        return Ok(Some(new_statement_range_response(&node, contents, code)));
+        let range = node.range();
+        return Ok(Some(new_statement_range_response(range, contents, code)));
     }
 
     if let Some(node) = find_statement_range_node(&root, row) {
-        return Ok(Some(new_statement_range_response(&node, contents, None)));
+        let range = expand_range_across_semicolons(node);
+        return Ok(Some(new_statement_range_response(range, contents, None)));
     };
 
     Ok(None)
 }
 
 fn new_statement_range_response(
-    node: &Node,
+    range: tree_sitter::Range,
     contents: &Rope,
     code: Option<String>,
 ) -> StatementRangeResponse {
     // Tree-sitter `Point`s
-    let start = node.start_position();
-    let end = node.end_position();
+    let start = range.start_point;
+    let end = range.end_point;
 
     // To LSP `Position`s
     let start = convert_point_to_position(contents, start);
     let end = convert_point_to_position(contents, end);
 
-    let range = Range { start, end };
+    let range = lsp_types::Range { start, end };
 
     StatementRangeResponse { range, code }
 }
@@ -184,6 +186,49 @@ fn find_roxygen_comment_at_point<'tree>(
     return Some((node, code));
 }
 
+/// Assuming `node` is the first node on a line, `expand_across_semicolons()`
+/// checks to see if there are any other non-comment nodes after `node` that
+/// share its line number. If there are, that means the nodes are separated by
+/// a `;`, and that we should expand the range to also include the node after
+/// the `;`.
+fn expand_range_across_semicolons(mut node: Node) -> tree_sitter::Range {
+    let start_byte = node.start_byte();
+    let start_point = node.start_position();
+
+    let mut end_byte = node.end_byte();
+    let mut end_point = node.end_position();
+
+    // We know `node` is at the start of a line, but it's possible the node
+    // ends with a `;` and needs to be extended
+    while let Some(next) = node.next_sibling() {
+        let next_start_point = next.start_position();
+
+        if end_point.row != next_start_point.row {
+            // Next sibling is on a different row, we are safe
+            break;
+        }
+        if next.is_comment() {
+            // Next sibling is a trailing comment, we are safe
+            break;
+        }
+
+        // Next sibling is on the same line as us, must be separated
+        // by a semicolon. Extend end of range to include next sibling.
+        end_byte = next.end_byte();
+        end_point = next.end_position();
+
+        // Update ending `node` and recheck (i.e. `1; 2; 3`)
+        node = next;
+    }
+
+    tree_sitter::Range {
+        start_byte,
+        end_byte,
+        start_point,
+        end_point,
+    }
+}
+
 fn find_statement_range_node<'tree>(root: &'tree Node, row: usize) -> Option<Node<'tree>> {
     let mut cursor = root.walk();
 
@@ -501,6 +546,7 @@ mod tests {
     use tree_sitter::Parser;
     use tree_sitter::Point;
 
+    use crate::lsp::statement_range::expand_range_across_semicolons;
     use crate::lsp::statement_range::find_roxygen_comment_at_point;
     use crate::lsp::statement_range::find_statement_range_node;
     use crate::lsp::traits::rope::RopeExt;
@@ -666,14 +712,15 @@ mod tests {
         let root = ast.root_node();
 
         let node = find_statement_range_node(&root, cursor.unwrap().row).unwrap();
+        let range = expand_range_across_semicolons(node);
 
         assert_eq!(
-            node.start_position(),
+            range.start_point,
             sel_start.unwrap(),
             "Failed on test {original}"
         );
         assert_eq!(
-            node.end_position(),
+            range.end_point,
             sel_end.unwrap(),
             "Failed on test {original}"
         );
@@ -1039,13 +1086,14 @@ if (a > b) {
     }
 
     #[test]
-    fn test_if_statements_without_braces_can_run_individual_expressions() {
+    fn test_if_statements_without_braces_should_run_the_whole_if_statement() {
         statement_range_test(
             "
 <<if (@a > b)
     1 + 1>>",
         );
 
+        // TODO: This should run the whole if statement because there are no braces
         statement_range_test(
             "
 if (a > b)
@@ -1055,7 +1103,7 @@ if (a > b)
     }
 
     #[test]
-    fn test_top_level_if_else_statements_without_braces_can_run_individual_expressions_1() {
+    fn test_top_level_if_else_statements_without_braces_should_run_the_whole_if_statement() {
         statement_range_test(
             "
 <<if @(a > b)
@@ -1064,46 +1112,47 @@ if (a > b)
 ",
         );
 
+        // TODO: This should run the whole if statement because there are no braces
         statement_range_test(
             "
 if (a > b)
-  <<@1 + 1>> else if (b > c)
-  2 + 2 else 4 + 4
+  <<@1 + 1 else if (b > c)
+  2 + 2 else 4 + 4>>
 ",
         );
 
-        // TODO: I'm not exactly sure what this should run, but this seems strange
+        // TODO: This should run the whole if statement because there are no braces
         statement_range_test(
             "
 if (a > b)
-  <<1 + 1>> else if @(b > c)
-  2 + 2 else 4 + 4
+  <<1 + 1 else if @(b > c)
+  2 + 2 else 4 + 4>>
 ",
         );
 
         statement_range_test(
             "
 if (a > b)
   1 + 1 else if (b > c)
-  <<2 + @2>> else 4 + 4
+  <<2 + @2 else 4 + 4>>
 ",
         );
 
-        // TODO: I'm not exactly sure what this should run, but this seems strange
+        // TODO: This should run the whole if statement because there are no braces
         statement_range_test(
             "
 if (a > b)
   1 + 1 else if (b > c)
-  <<2 + 2>> else@ 4 + 4
+  <<2 + 2 else@ 4 + 4>>
 ",
         );
 
-        // TODO: I'm not exactly sure what this should run, but this seems strange
+        // TODO: This should run the whole if statement because there are no braces
         statement_range_test(
             "
 if (a > b)
   1 + 1 else if (b > c)
-  <<2 + 2>> else 4 @+ 4
+  <<2 + 2 else 4 @+ 4>>
 ",
         );
     }
@@ -1123,6 +1172,7 @@ if (a > b)
 ",
         );
 
+        // TODO: This should run the whole if statement because there are no braces
         statement_range_test(
             "
 {
@@ -1149,6 +1199,7 @@ if (a > b)
 ",
         );
 
+        // TODO: This should run the whole if statement because there are no braces
         statement_range_test(
             "
 {
@@ -1175,6 +1226,7 @@ if (a > b)
 ",
         );
 
+        // TODO: This should run the whole if statement because there are no braces
         statement_range_test(
             "
 {
@@ -1373,6 +1425,101 @@ test_that('stuff', {
         );
     }
 
+    #[test]
+    fn test_multiple_expressions_on_one_line() {
+        // https://github.com/posit-dev/positron/issues/4317
+        statement_range_test(
+            "
+<<1@; 2; 3>>
+",
+        );
+        statement_range_test(
+            "
+<<1; @2; 3>>
+",
+        );
+        statement_range_test(
+            "
+<<1; 2; 3@>>
+",
+        );
+
+        // Empty lines don't prevent finding complete lines
+        statement_range_test(
+            "
+@
+
+<<1; 2; 3>>
+    ",
+        );
+    }
+
+    #[test]
+    fn test_multiple_expressions_on_one_line_nested_case() {
+        statement_range_test(
+            "
+list({
+  @<<1; 2; 3>>
+})
+    ",
+        );
+        statement_range_test(
+            "
+list({
+  <<1; @2; 3>>
+})
+    ",
+        );
+    }
+
+    #[test]
+    fn test_multiple_expressions_after_multiline_expression() {
+        // Selects everything
+        statement_range_test(
+            "
+@<<{
+  1
+}; 2>>
+    ",
+        );
+
+        // Select up through the second brace
+        statement_range_test(
+            "
+@<<{
+  1
+}; {
+  2
+}>>
+    ",
+        );
+
+        // Only selects `1`
+        statement_range_test(
+            "
+{
+  @<<1>>
+}; {
+  2
+}
+    ",
+        );
+    }
+
+    #[test]
+    fn test_multiple_expressions_on_one_line_doesnt_select_trailing_comment() {
+        statement_range_test(
+            "
+@<<1>> # trailing
+    ",
+        );
+        statement_range_test(
+            "
+@<<1; 2>> # trailing
+    ",
+        );
+    }
+
     #[test]
     fn test_no_top_level_statement() {
         let row = 2;