Fix several bugs (#464)

REditorSupport · Aug 9, 2021 · b2148e3 · b2148e3
1 parent 05e7e67
commit b2148e3
Show file tree

Hide file tree

Showing 9 changed files with 242 additions and 18 deletions.
diff --git a/R/completion.R b/R/completion.R
@@ -313,17 +313,17 @@ workspace_completion <- function(workspace, token,
 }
 
 scope_completion_symbols_xpath <- paste(
-    "*[self::FUNCTION or self::OP-LAMBDA]/following-sibling::SYMBOL_FORMALS",
+    "(*|descendant-or-self::exprlist/*)[self::FUNCTION or self::OP-LAMBDA]/following-sibling::SYMBOL_FORMALS",
+    "(*|descendant-or-self::exprlist/*)/LEFT_ASSIGN[not(following-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA])]/preceding-sibling::expr[count(*)=1]/SYMBOL",
+    "(*|descendant-or-self::exprlist/*)/RIGHT_ASSIGN[not(preceding-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA])]/following-sibling::expr[count(*)=1]/SYMBOL",
+    "(*|descendant-or-self::exprlist/*)/EQ_ASSIGN[not(following-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA])]/preceding-sibling::expr[count(*)=1]/SYMBOL",
     "forcond/SYMBOL",
-    "*/LEFT_ASSIGN[not(following-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA])]/preceding-sibling::expr[count(*)=1]/SYMBOL",
-    "*/RIGHT_ASSIGN[not(preceding-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA])]/following-sibling::expr[count(*)=1]/SYMBOL",
-    "*/EQ_ASSIGN[not(following-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA])]/preceding-sibling::expr[count(*)=1]/SYMBOL",
     sep = "|")
 
 scope_completion_functs_xpath <- paste(
-    "*/LEFT_ASSIGN[following-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA]]/preceding-sibling::expr[count(*)=1]/SYMBOL",
-    "*/RIGHT_ASSIGN[preceding-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA]]/following-sibling::expr[count(*)=1]/SYMBOL",
-    "*/EQ_ASSIGN[following-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA]]/preceding-sibling::expr[count(*)=1]/SYMBOL",
+    "(*|descendant-or-self::exprlist/*)/LEFT_ASSIGN[following-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA]]/preceding-sibling::expr[count(*)=1]/SYMBOL",
+    "(*|descendant-or-self::exprlist/*)/RIGHT_ASSIGN[preceding-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA]]/following-sibling::expr[count(*)=1]/SYMBOL",
+    "(*|descendant-or-self::exprlist/*)/EQ_ASSIGN[following-sibling::expr/*[self::FUNCTION or self::OP-LAMBDA]]/preceding-sibling::expr[count(*)=1]/SYMBOL",
     sep = "|")
 
 scope_completion <- function(uri, workspace, token, point, snippet_support = NULL) {

diff --git a/R/definition.R b/R/definition.R
@@ -1,8 +1,8 @@
 definition_xpath <- paste(
-    "*[self::FUNCTION or self::OP-LAMBDA]/following-sibling::SYMBOL_FORMALS[text() = '{token_quote}' and @line1 <= {row}]",
-    "*[LEFT_ASSIGN/preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and LEFT_ASSIGN/following-sibling::expr[@start > {start} or @end < {end}]]",
-    "*[RIGHT_ASSIGN/following-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and RIGHT_ASSIGN/preceding-sibling::expr[@start > {start} or @end < {end}]]",
-    "*[EQ_ASSIGN/preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and EQ_ASSIGN/following-sibling::expr[@start > {start} or @end < {end}]]",
+    "(*|descendant-or-self::exprlist/*)[self::FUNCTION or self::OP-LAMBDA]/following-sibling::SYMBOL_FORMALS[text() = '{token_quote}' and @line1 <= {row}]",
+    "(*|descendant-or-self::exprlist/*)[LEFT_ASSIGN[preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and following-sibling::expr[@start > {start} or @end < {end}]]]",
+    "(*|descendant-or-self::exprlist/*)[RIGHT_ASSIGN[following-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and preceding-sibling::expr[@start > {start} or @end < {end}]]]",
+    "(*|descendant-or-self::exprlist/*)[EQ_ASSIGN[preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and following-sibling::expr[@start > {start} or @end < {end}]]]",
     "forcond/SYMBOL[text() = '{token_quote}' and @line1 <= {row}]",
     sep = "|")
 

diff --git a/R/document.R b/R/document.R
@@ -402,6 +402,8 @@ parse_document <- function(uri, content) {
     if (is_rmarkdown(uri)) {
         content <- purl(content)
     }
+    # replace tab with a space since the width of a tab is 1 in LSP but 8 in getParseData().
+    content <- gsub("\t", " ", content, fixed = TRUE)
     expr <- tryCatch(parse(text = content, keep.source = TRUE), error = function(e) NULL)
     if (!is.null(expr)) {
         parse_env <- function() {

diff --git a/R/hover.R b/R/hover.R
@@ -1,8 +1,8 @@
 hover_xpath <- paste(
-    "*[self::FUNCTION or self::OP-LAMBDA][following-sibling::SYMBOL_FORMALS[text() = '{token_quote}' and @line1 <= {row}]]/parent::expr",
-    "*[LEFT_ASSIGN/preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and LEFT_ASSIGN/following-sibling::expr[@start > {start} or @end < {end}]]",
-    "*[RIGHT_ASSIGN/following-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and RIGHT_ASSIGN/preceding-sibling::expr[@start > {start} or @end < {end}]]",
-    "*[EQ_ASSIGN/preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and EQ_ASSIGN/following-sibling::expr[@start > {start} or @end < {end}]]",
+    "(*|descendant-or-self::exprlist/*)[self::FUNCTION or self::OP-LAMBDA][following-sibling::SYMBOL_FORMALS[text() = '{token_quote}' and @line1 <= {row}]]/parent::expr",
+    "(*|descendant-or-self::exprlist/*)[LEFT_ASSIGN[preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and following-sibling::expr[@start > {start} or @end < {end}]]]",
+    "(*|descendant-or-self::exprlist/*)[RIGHT_ASSIGN[following-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and preceding-sibling::expr[@start > {start} or @end < {end}]]]",
+    "(*|descendant-or-self::exprlist/*)[EQ_ASSIGN[preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}] and following-sibling::expr[@start > {start} or @end < {end}]]]",
     "forcond/SYMBOL[text() = '{token_quote}' and @line1 <= {row}]",
     sep = "|")
 
@@ -23,7 +23,7 @@ hover_reply <- function(id, uri, workspace, document, point) {
     resolved <- FALSE
 
     version <- workspace$get_parse_data(uri)$version
-    logger$info("hover:", list(uri = uri, version = version))
+    logger$info("hover:", list(uri = uri, version = version, token = token_result))
 
     xdoc <- workspace$get_parse_data(uri)$xml_doc
 
@@ -206,6 +206,9 @@ hover_reply <- function(id, uri, workspace, document, point) {
             } else if (token_name == "COMMENT") {
                 # comment
                 resolved <- TRUE
+            } else if (startsWith(token_name, "OP-")) {
+                # operators
+                resolved <- TRUE
             }
         }
     }

diff --git a/R/signature.R b/R/signature.R
@@ -1,6 +1,6 @@
 signature_xpath <- paste(
-    "*[LEFT_ASSIGN/preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}]]/expr[FUNCTION | OP-LAMBDA]",
-    "*[EQ_ASSIGN/preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}]]/expr[FUNCTION | OP-LAMBDA]",
+    "(*|descendant-or-self::exprlist/*)[LEFT_ASSIGN/preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}]]/expr[FUNCTION|OP-LAMBDA]",
+    "(*|descendant-or-self::exprlist/*)[EQ_ASSIGN/preceding-sibling::expr[count(*)=1]/SYMBOL[text() = '{token_quote}' and @line1 <= {row}]]/expr[FUNCTION|OP-LAMBDA]",
     sep = "|")
 
 #' the response to a textDocument/signatureHelp Request

diff --git a/tests/testthat/test-completion.R b/tests/testthat/test-completion.R
@@ -524,6 +524,50 @@ test_that("Completion of symbols in scope works", {
         discard(~ .$kind == CompletionItemKind$Text), 1)
 })
 
+test_that("Completion of symbols in scope works with semi-colons", {
+    skip_on_cran()
+    client <- language_client()
+
+    temp_file <- withr::local_tempfile(fileext = ".R")
+    writeLines(
+        c(
+            "xvar0 <- rnorm(10);",
+            "my_fun <- function(xvar1) {",
+            "    xvar2 = 1;",
+            "    2 -> xvar3;",
+            "    for (xvar4 in 1:10) {",
+            "        xvar",
+            "    }",
+            "}"
+        ),
+        temp_file
+    )
+
+    client %>% did_save(temp_file)
+
+    result <- client %>% respond_completion(
+        temp_file, c(5, 12),
+        retry_when = function(result) length(result) == 0 || length(result$items) == 0
+    )
+
+    expect_length(result$items %>% discard(~ .$kind == CompletionItemKind$Text), 5)
+    expect_length(result$items %>%
+        keep(~ .$label == "xvar0") %>%
+        discard(~ .$kind == CompletionItemKind$Text), 1)
+    expect_length(result$items %>%
+        keep(~ .$label == "xvar1") %>%
+        discard(~ .$kind == CompletionItemKind$Text), 1)
+    expect_length(result$items %>%
+        keep(~ .$label == "xvar2") %>%
+        discard(~ .$kind == CompletionItemKind$Text), 1)
+    expect_length(result$items %>%
+        keep(~ .$label == "xvar3") %>%
+        discard(~ .$kind == CompletionItemKind$Text), 1)
+    expect_length(result$items %>%
+        keep(~ .$label == "xvar4") %>%
+        discard(~ .$kind == CompletionItemKind$Text), 1)
+})
+
 test_that("Completion inside a package works", {
     skip_on_cran()
     wd <- path_real(path_package("languageserver", "projects", "mypackage"))

diff --git a/tests/testthat/test-definition.R b/tests/testthat/test-definition.R
@@ -161,6 +161,56 @@ test_that("Go to Definition works in scope with different assignment operators",
     expect_equal(result$range$end, list(line = 4, character = 11))
 })
 
+test_that("Go to Definition works in scope with semi-colons", {
+    skip_on_cran()
+    client <- language_client()
+
+    single_file <- withr::local_tempfile(fileext = ".R")
+    writeLines(c(
+        "my_fn <- function(var1) {",
+        "  var2 <- 1;",
+        "  var3 = 2;",
+        "  3 -> var4;",
+        "  for (var5 in 1:10) {",
+        "    var1 + var2 + var3 + var4 + var5;",
+        "  }",
+        "}",
+        "my_fn(1)"
+    ), single_file)
+
+    client %>% did_save(single_file)
+
+    # first query a known function to make sure the file is processed
+    result <- client %>% respond_definition(single_file, c(8, 0))
+
+    expect_equal(result$range$start, list(line = 0, character = 0))
+    expect_equal(result$range$end, list(line = 7, character = 1))
+
+    result <- client %>% respond_definition(single_file, c(5, 5))
+
+    expect_equal(result$range$start, list(line = 0, character = 18))
+    expect_equal(result$range$end, list(line = 0, character = 22))
+
+    result <- client %>% respond_definition(single_file, c(5, 12))
+
+    expect_equal(result$range$start, list(line = 1, character = 2))
+    expect_equal(result$range$end, list(line = 1, character = 11))
+
+    result <- client %>% respond_definition(single_file, c(5, 20))
+
+    expect_equal(result$range$start, list(line = 2, character = 2))
+    expect_equal(result$range$end, list(line = 2, character = 10))
+
+    result <- client %>% respond_definition(single_file, c(5, 26))
+
+    expect_equal(result$range$start, list(line = 3, character = 2))
+    expect_equal(result$range$end, list(line = 3, character = 11))
+
+    result <- client %>% respond_definition(single_file, c(5, 34))
+
+    expect_equal(result$range$start, list(line = 4, character = 7))
+    expect_equal(result$range$end, list(line = 4, character = 11))
+})
 
 test_that("Go to Definition works on both sides of assignment", {
     skip_on_cran()

diff --git a/tests/testthat/test-hover.R b/tests/testthat/test-hover.R
@@ -92,6 +92,85 @@ test_that("Hover on variable works", {
     expect_equal(result$contents, "```r\nvar1 <- 0\n```")
 })
 
+test_that("Hover on variable with leading tabs works", {
+    skip_on_cran()
+    client <- language_client()
+
+    temp_file <- withr::local_tempfile(fileext = ".R")
+    writeLines(
+        c(
+            "index1 <- 1:10",
+            "\tindex1 + 1",
+            "\t\tindex1 + 2"
+        ),
+        temp_file
+    )
+
+    client %>% did_save(temp_file)
+
+    result <- client %>% respond_hover(temp_file, c(0, 1))
+    expect_equal(result$range$start, list(line = 0, character = 0))
+    expect_equal(result$range$end, list(line = 0, character = 6))
+    expect_equal(result$contents, "```r\nindex1 <- 1:10\n```")
+
+    result <- client %>% respond_hover(temp_file, c(1, 4))
+    expect_equal(result$range$start, list(line = 1, character = 1))
+    expect_equal(result$range$end, list(line = 1, character = 7))
+    expect_equal(result$contents, "```r\nindex1 <- 1:10\n```")
+
+    result <- client %>% respond_hover(temp_file, c(2, 7))
+    expect_equal(result$range$start, list(line = 2, character = 2))
+    expect_equal(result$range$end, list(line = 2, character = 8))
+    expect_equal(result$contents, "```r\nindex1 <- 1:10\n```")
+})
+
+test_that("Hover on variable works with semi-colons", {
+    skip_on_cran()
+    client <- language_client()
+
+    temp_file <- withr::local_tempfile(fileext = ".R")
+    writeLines(
+        c(
+            "var1 <- 1:10;",
+            "f(var1)",
+            "local({",
+            "   var2 <- 2:10;",
+            "   f(var1, var2)",
+            "   var1 <- 0;",
+            "   f(var1, var2)",
+            "})"
+        ),
+        temp_file
+    )
+
+    client %>% did_save(temp_file)
+
+    result <- client %>% respond_hover(temp_file, c(0, 1))
+    expect_equal(result$range$start, list(line = 0, character = 0))
+    expect_equal(result$range$end, list(line = 0, character = 4))
+    expect_equal(result$contents, "```r\nvar1 <- 1:10;\n```")
+
+    result <- client %>% respond_hover(temp_file, c(1, 3))
+    expect_equal(result$range$start, list(line = 1, character = 2))
+    expect_equal(result$range$end, list(line = 1, character = 6))
+    expect_equal(result$contents, "```r\nvar1 <- 1:10;\n```")
+
+    result <- client %>% respond_hover(temp_file, c(4, 6))
+    expect_equal(result$range$start, list(line = 4, character = 5))
+    expect_equal(result$range$end, list(line = 4, character = 9))
+    expect_equal(result$contents, "```r\nvar1 <- 1:10;\n```")
+
+    result <- client %>% respond_hover(temp_file, c(4, 13))
+    expect_equal(result$range$start, list(line = 4, character = 11))
+    expect_equal(result$range$end, list(line = 4, character = 15))
+    expect_equal(result$contents, "```r\nvar2 <- 2:10;\n```")
+
+    result <- client %>% respond_hover(temp_file, c(6, 6))
+    expect_equal(result$range$start, list(line = 6, character = 5))
+    expect_equal(result$range$end, list(line = 6, character = 9))
+    expect_equal(result$contents, "```r\nvar1 <- 0;\n```")
+})
+
 test_that("Hover works in scope with different assignment operators", {
     skip_on_cran()
     client <- language_client()
@@ -308,6 +387,33 @@ test_that("Hover works with local function", {
     ))
 })
 
+test_that("Hover on operator is ignored", {
+    skip_on_cran()
+    client <- language_client()
+
+    temp_file <- withr::local_tempfile(fileext = ".R")
+    writeLines(
+        c(
+            "for (ll in 1:3) {",
+            " p = ll",
+            " I = array(0:0, dim=c(p,p))",
+            "}"
+        ),
+        temp_file
+    )
+
+    client %>% did_save(temp_file)
+
+    result <- client %>% respond_hover(temp_file, c(2, 22),
+        retry_when = function(result) length(result) > 0)
+    expect_equal(result, NULL)
+
+    result <- client %>% respond_hover(temp_file, c(2, 23))
+    expect_equal(result$range$start, list(line = 2, character = 22))
+    expect_equal(result$range$end, list(line = 2, character = 23))
+    expect_equal(result$contents, "```r\np = ll\n```")
+})
+
 test_that("Hover works across multiple files", {
     skip_on_cran()
     client <- language_client()

diff --git a/tests/testthat/test-signature.R b/tests/testthat/test-signature.R
@@ -46,6 +46,25 @@ test_that("Signature of user function works", {
     expect_equal(result$signatures[[1]]$label, "foo(x, y = 3)")
 })
 
+test_that("Signature of user function works with semi-colon", {
+    skip_on_cran()
+    client <- language_client()
+
+    defn_file <- withr::local_tempfile(fileext = ".R")
+    temp_file <- withr::local_tempfile(fileext = ".R")
+
+    writeLines(c("foo <- function(x, y = 3) { x + y };"), defn_file)
+    writeLines(c("foo(3, "), temp_file)
+
+    client %>% did_save(defn_file) %>% did_save(temp_file)
+
+    result <- client %>% respond_signature(
+        temp_file, c(0, 7),
+        retry_when = function(result) length(result) == 0 || length(result$signatures) == 0)
+    expect_length(result$signatures, 1)
+    expect_equal(result$signatures[[1]]$label, "foo(x, y = 3)")
+})
+
 test_that("Signature in Rmarkdown works", {
     skip_on_cran()
     client <- language_client()