Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
193 changes: 170 additions & 23 deletions crates/ark/src/lsp/statement_range.rs
Original file line number Diff line number Diff line change
@@ -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.
//
//

Expand All @@ -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;
Expand All @@ -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>,
Expand All @@ -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 }
}
Expand Down Expand Up @@ -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();

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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}"
);
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand All @@ -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>>
",
);
}
Expand All @@ -1123,6 +1172,7 @@ if (a > b)
",
);

// TODO: This should run the whole if statement because there are no braces
statement_range_test(
"
{
Expand All @@ -1149,6 +1199,7 @@ if (a > b)
",
);

// TODO: This should run the whole if statement because there are no braces
statement_range_test(
"
{
Expand All @@ -1175,6 +1226,7 @@ if (a > b)
",
);

// TODO: This should run the whole if statement because there are no braces
statement_range_test(
"
{
Expand Down Expand Up @@ -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;
Expand Down
Loading