Skip to content

Commit 1132450

Browse files
committed
An empty `()`, or `()` with >1 expressions is now a syntax error, which is good, but does make it harder for us to give a precise error message. When we have a custom R parser that can report the exact problem in the parse result, we can do much better. For now it's a lot of headache and shooting a moving target.
1 parent 89305a1 commit 1132450

14 files changed

+42
-68
lines changed

Cargo.lock

Lines changed: 1 addition & 1 deletion
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

crates/ark/Cargo.toml

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ stdext = { path = "../stdext" }
4747
tokio = { version = "1.26.0", features = ["full"] }
4848
tower-lsp = "0.19.0"
4949
tree-sitter = "0.23.0"
50-
tree-sitter-r = { git = "https://github.com/r-lib/tree-sitter-r", rev = "9a63f592f0d8736912b474260a49931766542ae4" }
50+
tree-sitter-r = { git = "https://github.com/r-lib/tree-sitter-r", rev = "cbfa8fbce269d6e1c46bddc5c4fcc4cdee6c3e9f" }
5151
uuid = "1.3.0"
5252
url = "2.4.1"
5353
walkdir = "2"

crates/ark/src/lsp/diagnostics.rs

Lines changed: 5 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -690,25 +690,11 @@ fn recurse_parenthesized_expression(
690690
context: &mut DiagnosticContext,
691691
diagnostics: &mut Vec<Diagnostic>,
692692
) -> Result<()> {
693-
let mut n = 0;
694-
let mut cursor = node.walk();
695-
696-
for child in node.children_by_field_name("body", &mut cursor) {
697-
recurse(child, context, diagnostics)?;
698-
n = n + 1;
699-
}
700-
701-
if n > 1 {
702-
// The tree-sitter grammar allows multiple `body` statements, but we warn
703-
// the user about this as it is not allowed by the R parser.
704-
let range = node.range();
705-
let range = convert_tree_sitter_range_to_lsp_range(context.contents, range);
706-
let message = format!("Expected at most 1 statement within parentheses, found {n}.");
707-
let diagnostic = Diagnostic::new_simple(range, message);
708-
diagnostics.push(diagnostic);
709-
}
710-
711-
().ok()
693+
let Some(body) = node.child_by_field_name("body") else {
694+
// Would be unexpected, grammar requires exactly 1 `body`
695+
return Ok(());
696+
};
697+
recurse(body, context, diagnostics)
712698
}
713699

714700
/// Default recursion for arguments of a call-like node

crates/ark/src/lsp/diagnostics_syntax.rs

Lines changed: 17 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -73,39 +73,17 @@ fn recurse_children(
7373
}
7474

7575
fn syntax_diagnostic(node: Node, context: &DiagnosticContext) -> anyhow::Result<Diagnostic> {
76-
if let Some(diagnostic) = syntax_diagnostic_missing_open(node, context)? {
77-
return Ok(diagnostic);
78-
}
76+
// We used to try and analyze the `ERROR` structure of the tree to provide "precise"
77+
// error messages, but this is extremely prone to false positives due to the fact that
78+
// tree-sitter reports a "generic" error range, and it's up to us to try and infer the
79+
// problem based on the surrounding nodes, which tree-sitter often "recovers"
80+
// incorrectly. It's better to wait for us to have a custom R pratt parser, which
81+
// would be able to report precise error locations/messages, because it "knows" why a
82+
// parse error occurred.
7983

8084
Ok(syntax_diagnostic_default(node, context))
8185
}
8286

83-
// Use a heuristic that if we see a syntax error and it just contains a `)`, `}`, or `]`,
84-
// then it is probably a case of missing a matching open token.
85-
fn syntax_diagnostic_missing_open(
86-
node: Node,
87-
context: &DiagnosticContext,
88-
) -> anyhow::Result<Option<Diagnostic>> {
89-
let text = context.contents.node_slice(&node)?;
90-
91-
let open_token = if text == ")" {
92-
"("
93-
} else if text == "}" {
94-
"{"
95-
} else if text == "]" {
96-
"["
97-
} else {
98-
// Not an unmatched closing token
99-
return Ok(None);
100-
};
101-
102-
let range = node.range();
103-
104-
Ok(Some(new_missing_open_diagnostic(
105-
open_token, range, context,
106-
)))
107-
}
108-
10987
fn syntax_diagnostic_default(node: Node, context: &DiagnosticContext) -> Diagnostic {
11088
let range = node.range();
11189
let row_span = range.end_point.row - range.start_point.row;
@@ -309,15 +287,6 @@ fn diagnose_missing_close(
309287
Ok(())
310288
}
311289

312-
fn new_missing_open_diagnostic(
313-
open_token: &str,
314-
range: Range,
315-
context: &DiagnosticContext,
316-
) -> Diagnostic {
317-
let message = format!("Unmatched closing delimiter. Missing an opening '{open_token}'.");
318-
new_syntax_diagnostic(message, range, context)
319-
}
320-
321290
fn new_missing_close_diagnostic(
322291
close_token: &str,
323292
range: Range,
@@ -436,8 +405,17 @@ identity(1)
436405
let diagnostic = diagnostics.get(0).unwrap();
437406
insta::assert_snapshot!(diagnostic.message);
438407

408+
// tree-sitter grammar knows R doesn't allow empty `()` without a body
439409
let diagnostics = text_diagnostics("()");
440-
assert!(diagnostics.is_empty());
410+
assert_eq!(diagnostics.len(), 1);
411+
let diagnostic = diagnostics.get(0).unwrap();
412+
insta::assert_snapshot!(diagnostic.message);
413+
414+
// tree-sitter grammar knows R doesn't allow >1 expressions in `()`
415+
let diagnostics = text_diagnostics("(1+2\n1+2)");
416+
assert_eq!(diagnostics.len(), 1);
417+
let diagnostic = diagnostics.get(0).unwrap();
418+
insta::assert_snapshot!(diagnostic.message);
441419

442420
let diagnostics = text_diagnostics("( 1 + 2 )");
443421
assert!(diagnostics.is_empty());

crates/ark/src/lsp/snapshots/ark__lsp__diagnostics__tests__mixed_syntax_and_semantic_diagnostics.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
source: crates/ark/src/lsp/diagnostics.rs
33
expression: diagnostic.message
44
---
5-
Unmatched closing delimiter. Missing an opening '{'.
5+
Syntax error

crates/ark/src/lsp/snapshots/ark__lsp__diagnostics_syntax__tests__error_precision.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
source: crates/ark/src/lsp/diagnostics_syntax.rs
33
expression: diagnostic.message
44
---
5-
Unmatched closing delimiter. Missing an opening '('.
5+
Syntax error

crates/ark/src/lsp/snapshots/ark__lsp__diagnostics_syntax__tests__unmatched_binary_operator.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
source: crates/ark/src/lsp/diagnostics_syntax.rs
33
expression: diagnostic.message
44
---
5-
Unmatched closing delimiter. Missing an opening '{'.
5+
Syntax error

crates/ark/src/lsp/snapshots/ark__lsp__diagnostics_syntax__tests__unmatched_closing_token-2.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
source: crates/ark/src/lsp/diagnostics_syntax.rs
33
expression: diagnostic.message
44
---
5-
Unmatched closing delimiter. Missing an opening '('.
5+
Syntax error

crates/ark/src/lsp/snapshots/ark__lsp__diagnostics_syntax__tests__unmatched_closing_token-3.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
source: crates/ark/src/lsp/diagnostics_syntax.rs
33
expression: diagnostic.message
44
---
5-
Unmatched closing delimiter. Missing an opening '['.
5+
Syntax error

crates/ark/src/lsp/snapshots/ark__lsp__diagnostics_syntax__tests__unmatched_closing_token.snap

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,4 +2,4 @@
22
source: crates/ark/src/lsp/diagnostics_syntax.rs
33
expression: diagnostic.message
44
---
5-
Unmatched closing delimiter. Missing an opening '{'.
5+
Syntax error

0 commit comments

Comments
 (0)