Merge pull request #832 from posit-dev/feature/sync-tree-sitter-r

Sync with tree-sitter-r

Author: DavisVaughan

Cargo.lock

@@ -47,7 +47,7 @@ stdext = { path = "../stdext" }
 tokio = { version = "1.26.0", features = ["full"] }
 tower-lsp = "0.19.0"
 tree-sitter = "0.23.0"
-tree-sitter-r = { git = "https://github.com/r-lib/tree-sitter-r", rev = "c094bd57652f8a08edc31d79a31222268fe798ee" }
+tree-sitter-r = { git = "https://github.com/r-lib/tree-sitter-r", rev = "95aff097aa927a66bb357f715b58cde821be8867" }
 uuid = "1.3.0"
 url = "2.4.1"
 walkdir = "2"
@@ -69,7 +69,7 @@ stdext = { path = "../stdext", features = ["testing"] }
 tempfile = "3.13.0"
 
 [build-dependencies]
-cc = "1.0"
+cc = "1.1.22"
 chrono = "0.4.23"
 embed-resource = "2.5.0"
 

crates/ark/Cargo.toml

@@ -690,80 +690,11 @@ fn recurse_parenthesized_expression(
     context: &mut DiagnosticContext,
     diagnostics: &mut Vec<Diagnostic>,
 ) -> Result<()> {
-    let mut n = 0;
-    let mut cursor = node.walk();
-
-    for child in node.children_by_field_name("body", &mut cursor) {
-        recurse(child, context, diagnostics)?;
-        n = n + 1;
-    }
-
-    if n > 1 {
-        // The tree-sitter grammar allows multiple `body` statements, but we warn
-        // the user about this as it is not allowed by the R parser.
-        let range = node.range();
-        let range = convert_tree_sitter_range_to_lsp_range(context.contents, range);
-        let message = format!("Expected at most 1 statement within parentheses, found {n}.");
-        let diagnostic = Diagnostic::new_simple(range, message);
-        diagnostics.push(diagnostic);
-    }
-
-    ().ok()
-}
-
-// TODO: This should be a syntax check, as the grammar should not allow
-// two `Argument` nodes side by side
-fn check_call_like_next_sibling(
-    child: Node,
-    parent_type: &NodeType,
-    context: &mut DiagnosticContext,
-    diagnostics: &mut Vec<Diagnostic>,
-) -> Result<()> {
-    let Some(next) = child.next_sibling() else {
-        return ().ok();
-    };
-
-    let close = match parent_type {
-        NodeType::Call => ")",
-        NodeType::Subset => "]",
-        NodeType::Subset2 => "]]",
-        _ => bail!("Parent must be a call, subset, or subset2 node."),
-    };
-
-    let ok = match next.node_type() {
-        NodeType::Comma => true,
-        NodeType::Anonymous(kind) if kind.as_str() == close => true,
-        NodeType::Comment => true,
-        // Should be handled elsewhere
-        NodeType::Error => true,
-        _ => false,
-    };
-
-    if ok {
-        return ().ok();
-    }
-
-    // Children can be arbitrarily large, so report the issue between the end of `child`
-    // and the start of `next` (it's not really the child's fault anyways,
-    // it's the fault of the thing right after it).
-    let start_byte = child.end_byte();
-    let start_point = child.end_position();
-    let end_byte = next.start_byte();
-    let end_point = next.start_position();
-
-    let range = Range {
-        start_byte,
-        start_point,
-        end_byte,
-        end_point,
+    let Some(body) = node.child_by_field_name("body") else {
+        // Would be unexpected, grammar requires exactly 1 `body`
+        return Ok(());
     };
-
-    let range = convert_tree_sitter_range_to_lsp_range(context.contents, range);
-    let message = "Expected ',' between expressions.";
-    let diagnostic = Diagnostic::new_simple(range, message.into());
-    diagnostics.push(diagnostic);
-
-    ().ok()
+    recurse(body, context, diagnostics)
 }
 
 /// Default recursion for arguments of a call-like node
@@ -792,8 +723,6 @@ fn recurse_call_like_arguments_default(
     // every function behaves like `list()`, which is our default model of
     // strict evaluation.
     with_in_call_like_arguments(context, |context| {
-        let call_type = node.node_type();
-
         let Some(arguments) = node.child_by_field_name("arguments") else {
             return Ok(());
         };
@@ -803,9 +732,6 @@ fn recurse_call_like_arguments_default(
         let children = arguments.children_by_field_name("argument", &mut cursor);
 
         for child in children {
-            // Warn if the next sibling is neither a comma nor the correct closing delimiter
-            check_call_like_next_sibling(child, &call_type, context, diagnostics)?;
-
             // Recurse into `value`s
             if let Some(value) = child.child_by_field_name("value") {
                 recurse(value, context, diagnostics)?;
@@ -1111,23 +1037,6 @@ foo
         })
     }
 
-    #[test]
-    fn test_expression_after_call_argument() {
-        r_task(|| {
-            let text = "match(1, 2 3)";
-            let document = Document::new(text, None);
-
-            let diagnostics = generate_diagnostics(document, DEFAULT_STATE.clone());
-            assert_eq!(diagnostics.len(), 1);
-
-            // Diagnostic highlights between the `2` and `3`
-            let diagnostic = diagnostics.get(0).unwrap();
-            insta::assert_snapshot!(diagnostic.message);
-            assert_eq!(diagnostic.range.start, Position::new(0, 10));
-            assert_eq!(diagnostic.range.end, Position::new(0, 11));
-        })
-    }
-
     #[test]
     fn test_no_diagnostic_for_dot_dot_i() {
         r_task(|| {

crates/ark/src/lsp/diagnostics.rs

@@ -73,39 +73,17 @@ fn recurse_children(
 }
 
 fn syntax_diagnostic(node: Node, context: &DiagnosticContext) -> anyhow::Result<Diagnostic> {
-    if let Some(diagnostic) = syntax_diagnostic_missing_open(node, context)? {
-        return Ok(diagnostic);
-    }
+    // We used to try and analyze the `ERROR` structure of the tree to provide "precise"
+    // error messages, but this is extremely prone to false positives due to the fact that
+    // tree-sitter reports a "generic" error range, and it's up to us to try and infer the
+    // problem based on the surrounding nodes, which tree-sitter often "recovers"
+    // incorrectly. It's better to wait for us to have a custom R pratt parser, which
+    // would be able to report precise error locations/messages, because it "knows" why a
+    // parse error occurred.
 
     Ok(syntax_diagnostic_default(node, context))
 }
 
-// Use a heuristic that if we see a syntax error and it just contains a `)`, `}`, or `]`,
-// then it is probably a case of missing a matching open token.
-fn syntax_diagnostic_missing_open(
-    node: Node,
-    context: &DiagnosticContext,
-) -> anyhow::Result<Option<Diagnostic>> {
-    let text = context.contents.node_slice(&node)?;
-
-    let open_token = if text == ")" {
-        "("
-    } else if text == "}" {
-        "{"
-    } else if text == "]" {
-        "["
-    } else {
-        // Not an unmatched closing token
-        return Ok(None);
-    };
-
-    let range = node.range();
-
-    Ok(Some(new_missing_open_diagnostic(
-        open_token, range, context,
-    )))
-}
-
 fn syntax_diagnostic_default(node: Node, context: &DiagnosticContext) -> Diagnostic {
     let range = node.range();
     let row_span = range.end_point.row - range.start_point.row;
@@ -309,15 +287,6 @@ fn diagnose_missing_close(
     Ok(())
 }
 
-fn new_missing_open_diagnostic(
-    open_token: &str,
-    range: Range,
-    context: &DiagnosticContext,
-) -> Diagnostic {
-    let message = format!("Unmatched closing delimiter. Missing an opening '{open_token}'.");
-    new_syntax_diagnostic(message, range, context)
-}
-
 fn new_missing_close_diagnostic(
     close_token: &str,
     range: Range,
@@ -436,8 +405,17 @@ identity(1)
         let diagnostic = diagnostics.get(0).unwrap();
         insta::assert_snapshot!(diagnostic.message);
 
+        // tree-sitter grammar knows R doesn't allow empty `()` without a body
         let diagnostics = text_diagnostics("()");
-        assert!(diagnostics.is_empty());
+        assert_eq!(diagnostics.len(), 1);
+        let diagnostic = diagnostics.get(0).unwrap();
+        insta::assert_snapshot!(diagnostic.message);
+
+        // tree-sitter grammar knows R doesn't allow >1 expressions in `()`
+        let diagnostics = text_diagnostics("(1+2\n1+2)");
+        assert_eq!(diagnostics.len(), 1);
+        let diagnostic = diagnostics.get(0).unwrap();
+        insta::assert_snapshot!(diagnostic.message);
 
         let diagnostics = text_diagnostics("( 1 + 2 )");
         assert!(diagnostics.is_empty());
@@ -526,4 +504,67 @@ function(x {
         assert_eq!(diagnostic.range.start, Position::new(3, 0));
         assert_eq!(diagnostic.range.end, Position::new(3, 1));
     }
+
+    #[test]
+    fn test_repeated_call_arguments_without_delimiter() {
+        let text = "match(1, 2 3)";
+
+        let diagnostics = text_diagnostics(text);
+        assert_eq!(diagnostics.len(), 1);
+
+        // Diagnostic highlights the `2`
+        let diagnostic = diagnostics.get(0).unwrap();
+        insta::assert_snapshot!(diagnostic.message);
+        assert_eq!(diagnostic.range.start, Position::new(0, 9));
+        assert_eq!(diagnostic.range.end, Position::new(0, 10));
+    }
+
+    #[test]
+    fn test_no_syntax_diagnostic_on_dots_and_dot_dot_i() {
+        let text = "x$...";
+        let diagnostics = text_diagnostics(text);
+        assert!(diagnostics.is_empty());
+
+        let text = "x$..1";
+        let diagnostics = text_diagnostics(text);
+        assert!(diagnostics.is_empty());
+
+        let text = "x::...";
+        let diagnostics = text_diagnostics(text);
+        assert!(diagnostics.is_empty());
+    }
+
+    #[test]
+    fn test_no_syntax_diagnostic_on_raw_strings() {
+        // https://github.com/r-lib/tree-sitter-r/issues/162
+        let text = r#"
+r"-()-)-"
+r"--()-")--"
+        "#;
+        let diagnostics = text_diagnostics(text);
+        assert!(diagnostics.is_empty());
+    }
+
+    #[test]
+    fn test_no_syntax_diagnostic_on_null_argument_name() {
+        // https://github.com/r-lib/tree-sitter-r/issues/164
+        let text = r#"
+switch(
+  typeof(x),
+  NULL = ,
+)
+        "#;
+        let diagnostics = text_diagnostics(text);
+        assert!(diagnostics.is_empty());
+    }
+
+    #[test]
+    fn test_no_syntax_diagnostic_on_binary_exponent() {
+        // https://github.com/r-lib/tree-sitter-r/issues/159
+        let text = r#"
+0x0p-123
+        "#;
+        let diagnostics = text_diagnostics(text);
+        assert!(diagnostics.is_empty());
+    }
 }

crates/ark/src/lsp/diagnostics_syntax.rs

@@ -2,4 +2,4 @@
 source: crates/ark/src/lsp/diagnostics.rs
 expression: diagnostic.message
 ---
-Unmatched closing delimiter. Missing an opening '{'.
+Syntax error

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

@@ -2,4 +2,4 @@
 source: crates/ark/src/lsp/diagnostics_syntax.rs
 expression: diagnostic.message
 ---
-Unmatched closing delimiter. Missing an opening '('.
+Syntax error

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

@@ -0,0 +1,5 @@
+---
+source: crates/ark/src/lsp/diagnostics_syntax.rs
+expression: diagnostic.message
+---
+Syntax error

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

@@ -2,4 +2,4 @@
 source: crates/ark/src/lsp/diagnostics_syntax.rs
 expression: diagnostic.message
 ---
-Unmatched closing delimiter. Missing an opening '{'.
+Syntax error

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

@@ -2,4 +2,4 @@
 source: crates/ark/src/lsp/diagnostics_syntax.rs
 expression: diagnostic.message
 ---
-Unmatched closing delimiter. Missing an opening '('.
+Syntax error