From 2d232cbe359626717fc1ac767825c223f8cabdcc Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 12:58:18 +0800 Subject: [PATCH 01/22] Support unscoped_functions and library_functions --- R/document.R | 70 +++++++++++++++++++++++++++++++++++++++------------- R/zzz.R | 11 +++++++++ 2 files changed, 64 insertions(+), 17 deletions(-) create mode 100644 R/zzz.R diff --git a/R/document.R b/R/document.R index 60fd0f00..32f2bc7a 100644 --- a/R/document.R +++ b/R/document.R @@ -186,27 +186,59 @@ get_range_text <- function(content, line1, col1, line2, col2) { lines } +is_ns_call <- function(x) { + length(x) == 3L && is.symbol(x[[1L]]) && as.character(x[[1L]]) %in% c("::", ":::") +} + +# Check if an expression is a simple call like `foo(bar)` or `pkg::foo(bar)` +# This rules out anonymous function call like `(function(x) x + 1)(bar)` +is_simple_call <- function(x) { + is.call(x) && (is.symbol(x[[1L]]) || is_ns_call(x[[1]])) +} + +# We should handle base function specially as users may use base::fun form +# The reason that we only take care of `base` (not `utils`) is that only `base` calls can generate symbols +# Check if the lang is in base::fun form +is_base_call <- function(x) { + is_ns_call(x) && as.character(x[[2L]]) == "base" +} + +# Handle `base` function specically by removing the `base::` prefix +fun_string <- function(x) { + if (is_base_call(x)) as.character(x[[3L]]) else deparse(x) +} + +parse_config <- new.env() +parse_config$unscoped_functions <- c( + "system.time", + "try", + "tryCatch", + "withCallingHandlers", + "withRestarts", + "allowInterrupts", + "suspendInterrupts", + "suppressPackageStartupMessages", + "suppressMessages", + "suppressWarnings", + "assertError", + "assertWarning", + "assertCondition" +) + +parse_config$library_functions <- c( + "library", + "require", + "pacman::p_load" +) + parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcref")) { if (length(expr) == 0L || is.symbol(expr)) { - return(env) - } - # We should handle base function specially as users may use base::fun form - # The reason that we only take care of `base` (not `utils`) is that only `base` calls can generate symbols - # Check if the lang is in base::fun form - is_base_call <- function(x) { - length(x) == 3L && as.character(x[[1L]]) %in% c("::", ":::") && as.character(x[[2L]]) == "base" - } - # Be able to handle `pkg::name` case (note `::` is a function) - is_symbol <- function(x) { - is.symbol(x) || is_base_call(x) - } - # Handle `base` function specically by removing the `base::` prefix - fun_string <- function(x) { - if (is_base_call(x)) as.character(x[[3L]]) else as.character(x) + return(env) } + for (i in seq_along(expr)) { e <- expr[[i]] - if (missing(e) || !is.call(e) || !is_symbol(e[[1L]])) next + if (missing(e) || !is_simple_call(e)) next f <- fun_string(e[[1L]]) cur_srcref <- if (level == 0L) srcref[[i]] else srcref if (f %in% c("{", "(")) { @@ -227,6 +259,10 @@ parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcr Recall(content, e[[3L]], env, level + 1L, cur_srcref) } else if (f == "repeat") { Recall(content, e[[2L]], env, level + 1L, cur_srcref) + } else if (f %in% parse_config$unscoped_functions) { + if (length(e) >= 2L) { + Recall(content, e[[2L]], env, level + 1L, cur_srcref) + } } else if (f %in% c("<-", "=", "delayedAssign", "makeActiveBinding", "assign")) { # to see the pos/env/assign.env of assigning functions is set or not # if unset, it means using the default value, which is top-level @@ -297,7 +333,7 @@ parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcr } else { env$nonfuncts <- c(env$nonfuncts, symbol) } - } else if (f %in% c("library", "require") && length(e) == 2L) { + } else if (f %in% parse_config$library_functions && length(e) == 2L) { pkg <- as.character(e[[2L]]) if (!(pkg %in% env$packages)) { env$packages <- c(env$packages, pkg) diff --git a/R/zzz.R b/R/zzz.R new file mode 100644 index 00000000..19a7a0a5 --- /dev/null +++ b/R/zzz.R @@ -0,0 +1,11 @@ +.onLoad <- function(...) { + parse_config$unscoped_functions <- union( + parse_config$unscoped_functions, + getOption("languageserver.extra_unscoped_functions") + ) + + parse_config$library_functions <- union( + parse_config$library_functions, + getOption("languageserver.extra_library_functions") + ) +} From 94e4ab8e2f81aa9e6df7b9cbd0281b1d2b1ee8a3 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 15:52:26 +0800 Subject: [PATCH 02/22] Add a initial test case --- tests/testthat/test-completion.R | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/tests/testthat/test-completion.R b/tests/testthat/test-completion.R index d2f5a7d7..d080486d 100644 --- a/tests/testthat/test-completion.R +++ b/tests/testthat/test-completion.R @@ -118,6 +118,30 @@ test_that("Simple completion is case insensitive", { expect_length(result$items %>% keep(~ .$label == "mtcars"), 1) }) +test_that("Completion of functions in attached packages work", { + skip_on_cran() + client <- language_client() + + temp_file <- withr::local_tempfile(fileext = ".R") + writeLines( + c( + "library(jsonlite)", + "require('xml2')", + "fromJS", + "read_xm" + ), + temp_file) + + client %>% did_save(temp_file) + + result <- client %>% respond_completion(temp_file, c(2, 6)) + + expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) + + result <- client %>% respond_completion(temp_file, c(3, 7)) + expect_length(result$items %>% keep(~ .$label == "read_xml"), 1) +}) + test_that("Completion of function arguments works", { skip_on_cran() client <- language_client() From 645d24643569f5060a7288bad2329d3a8b6d9780 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 16:10:24 +0800 Subject: [PATCH 03/22] Move out is_top_level --- R/document.R | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/R/document.R b/R/document.R index 32f2bc7a..b67a73a2 100644 --- a/R/document.R +++ b/R/document.R @@ -208,6 +208,21 @@ fun_string <- function(x) { if (is_base_call(x)) as.character(x[[3L]]) else deparse(x) } +# to see the pos/env/assign.env of assigning functions is set or not +# if unset, it means using the default value, which is top-level +# if set, we should compare to a vector of known "top-level" candidates +is_top_level <- function(arg_env, ...) { + if (is.null(arg_env)) return(TRUE) + default <- list( + quote(parent.frame(1)), quote(parent.frame(1L)), + quote(environment()), + quote(.GlobalEnv), quote(globalenv()) + ) + extra <- substitute(list(...))[-1L] + top_level_envs <- c(default, as.list(extra)) + any(vapply(top_level_envs, identical, x = arg_env, FUN.VALUE = logical(1L))) +} + parse_config <- new.env() parse_config$unscoped_functions <- c( "system.time", @@ -264,21 +279,6 @@ parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcr Recall(content, e[[2L]], env, level + 1L, cur_srcref) } } else if (f %in% c("<-", "=", "delayedAssign", "makeActiveBinding", "assign")) { - # to see the pos/env/assign.env of assigning functions is set or not - # if unset, it means using the default value, which is top-level - # if set, we should compare to a vector of known "top-level" candidates - is_top_level <- function(arg_env, ...) { - if (is.null(arg_env)) return(TRUE) - default <- list( - quote(parent.frame(1)), quote(parent.frame(1L)), - quote(environment()), - quote(.GlobalEnv), quote(globalenv()) - ) - extra <- substitute(list(...))[-1L] - top_level_envs <- c(default, as.list(extra)) - any(vapply(top_level_envs, identical, x = arg_env, FUN.VALUE = logical(1L))) - } - type <- NULL if (f %in% c("<-", "=")) { From 73f79d596e60722aa5f3694761c61f9fb286a872 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 17:15:20 +0800 Subject: [PATCH 04/22] More flexible unscoped functions and library functions --- R/document.R | 73 +++++++++++++++++++++++++++++++++++----------------- R/zzz.R | 12 +++------ 2 files changed, 54 insertions(+), 31 deletions(-) diff --git a/R/document.R b/R/document.R index b67a73a2..263b51fc 100644 --- a/R/document.R +++ b/R/document.R @@ -224,26 +224,40 @@ is_top_level <- function(arg_env, ...) { } parse_config <- new.env() -parse_config$unscoped_functions <- c( - "system.time", - "try", - "tryCatch", - "withCallingHandlers", - "withRestarts", - "allowInterrupts", - "suspendInterrupts", - "suppressPackageStartupMessages", - "suppressMessages", - "suppressWarnings", - "assertError", - "assertWarning", - "assertCondition" +parse_config$unscoped_functions <- list( + system.time = "expr", + try = "expr", + tryCatch = c("expr", "finally"), + withCallingHandlers = "expr", + withRestarts = "expr", + allowInterrupts = "expr", + suspendInterrupts = "expr", + suppressPackageStartupMessages = "expr", + suppressMessages = "expr", + suppressWarnings = "expr" ) parse_config$library_functions <- c( - "library", - "require", - "pacman::p_load" + "library" = function(call) { + call <- match.call(base::library, call) + if (!isTRUE(call$character.only)) { + as.character(call$package) + } + }, + "require" = function(call) { + call <- match.call(base::require, call) + if (!isTRUE(call$character.only)) { + as.character(call$package) + } + }, + "pacman::p_load" = function(call) { + if (requireNamespace("pacman")) { + call <- match.call(pacman::p_load, call, expand.dots = FALSE) + if (!isTRUE(call$character.only)) { + vapply(call[["..."]], as.character, character(1L)) + } + } + } ) parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcref")) { @@ -274,9 +288,18 @@ parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcr Recall(content, e[[3L]], env, level + 1L, cur_srcref) } else if (f == "repeat") { Recall(content, e[[2L]], env, level + 1L, cur_srcref) - } else if (f %in% parse_config$unscoped_functions) { + } else if (f %in% names(parse_config$unscoped_functions)) { if (length(e) >= 2L) { - Recall(content, e[[2L]], env, level + 1L, cur_srcref) + fun <- tryCatch(eval(e[[1L]], globalenv()), error = function(e) NULL) + if (is.function(fun)) { + call <- match.call(fun, e, expand.dots = FALSE) + captures <- parse_config$unscoped_functions[[f]] + for (capture in captures) { + if (is.call(call[[capture]])) { + Recall(content, call[[capture]], env, level + 1L, cur_srcref) + } + } + } } } else if (f %in% c("<-", "=", "delayedAssign", "makeActiveBinding", "assign")) { type <- NULL @@ -333,10 +356,14 @@ parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcr } else { env$nonfuncts <- c(env$nonfuncts, symbol) } - } else if (f %in% parse_config$library_functions && length(e) == 2L) { - pkg <- as.character(e[[2L]]) - if (!(pkg %in% env$packages)) { - env$packages <- c(env$packages, pkg) + } else if (f %in% names(parse_config$library_functions) && length(e) >= 2L) { + pkgs <- tryCatch( + parse_config$library_functions[[f]](e), + error = function(e) NULL + ) + pkgs <- pkgs[nzchar(pkgs)] + if (length(pkgs)) { + env$packages <- union(env$packages, pkgs) } } } diff --git a/R/zzz.R b/R/zzz.R index 19a7a0a5..a97c6702 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -1,11 +1,7 @@ .onLoad <- function(...) { - parse_config$unscoped_functions <- union( - parse_config$unscoped_functions, - getOption("languageserver.extra_unscoped_functions") - ) + extra_unscoped_functions <- getOption("languageserver.extra_unscoped_functions") + parse_config$unscoped_functions[names(extra_unscoped_functions)] <- extra_unscoped_functions - parse_config$library_functions <- union( - parse_config$library_functions, - getOption("languageserver.extra_library_functions") - ) + extra_library_functions <- getOption("languageserver.extra_library_functions") + parse_config$library_functions[names(extra_library_functions)] <- extra_library_functions } From 6121985b6bf31340007a1e22a0d85ac899216bdf Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 17:34:49 +0800 Subject: [PATCH 05/22] Update pacman::p_load spec --- R/document.R | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/R/document.R b/R/document.R index 263b51fc..0131de9d 100644 --- a/R/document.R +++ b/R/document.R @@ -251,11 +251,13 @@ parse_config$library_functions <- c( } }, "pacman::p_load" = function(call) { - if (requireNamespace("pacman")) { - call <- match.call(pacman::p_load, call, expand.dots = FALSE) - if (!isTRUE(call$character.only)) { - vapply(call[["..."]], as.character, character(1L)) - } + fun <- if (requireNamespace("pacman")) pacman::p_load else + function(..., char, install = TRUE, + update = getOption("pac_update"), + character.only = FALSE) NULL + call <- match.call(fun, call, expand.dots = FALSE) + if (!isTRUE(call$character.only)) { + vapply(call[["..."]], as.character, character(1L)) } } ) From fdf8d8fc3a12636fca1c31288ed789e9bdebb832 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 17:38:38 +0800 Subject: [PATCH 06/22] Update option names --- R/zzz.R | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/R/zzz.R b/R/zzz.R index a97c6702..30a9ae6c 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -1,7 +1,7 @@ .onLoad <- function(...) { - extra_unscoped_functions <- getOption("languageserver.extra_unscoped_functions") - parse_config$unscoped_functions[names(extra_unscoped_functions)] <- extra_unscoped_functions + unscoped_functions <- getOption("languageserver.unscoped_functions") + parse_config$unscoped_functions[names(unscoped_functions)] <- unscoped_functions - extra_library_functions <- getOption("languageserver.extra_library_functions") - parse_config$library_functions[names(extra_library_functions)] <- extra_library_functions + library_functions <- getOption("languageserver.library_functions") + parse_config$library_functions[names(library_functions)] <- library_functions } From 9165b446c9e9df88722f9bcc0e69cda4b150ec04 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 17:48:53 +0800 Subject: [PATCH 07/22] Add more test cases --- tests/testthat/test-completion.R | 48 ++++++++++++++++++++++++++++++-- 1 file changed, 46 insertions(+), 2 deletions(-) diff --git a/tests/testthat/test-completion.R b/tests/testthat/test-completion.R index d080486d..c1d31e16 100644 --- a/tests/testthat/test-completion.R +++ b/tests/testthat/test-completion.R @@ -118,7 +118,7 @@ test_that("Simple completion is case insensitive", { expect_length(result$items %>% keep(~ .$label == "mtcars"), 1) }) -test_that("Completion of functions in attached packages work", { +test_that("Completion of attached package functions works", { skip_on_cran() client <- language_client() @@ -133,13 +133,57 @@ test_that("Completion of functions in attached packages work", { temp_file) client %>% did_save(temp_file) + Sys.sleep(1) result <- client %>% respond_completion(temp_file, c(2, 6)) - expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) result <- client %>% respond_completion(temp_file, c(3, 7)) expect_length(result$items %>% keep(~ .$label == "read_xml"), 1) + + temp_file <- withr::local_tempfile(fileext = ".R") + writeLines( + c( + "pacman::p_load(jsonlite, xml2)", + "fromJS", + "read_xm" + ), + temp_file) + + client %>% did_save(temp_file) + Sys.sleep(1) + + result <- client %>% respond_completion(temp_file, c(1, 6)) + expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) + + result <- client %>% respond_completion(temp_file, c(2, 7)) + expect_length(result$items %>% keep(~ .$label == "read_xml"), 1) +}) + +test_that("Completion of package functions attached in unscoped functions works", { + skip_on_cran() + client <- language_client() + + temp_file <- withr::local_tempfile(fileext = ".R") + writeLines( + c( + "suppressPackageStartupMessages({", + " library(jsonlite)", + " require('xml2')", + "})", + "fromJS", + "read_xm" + ), + temp_file) + + client %>% did_save(temp_file) + Sys.sleep(1) + + result <- client %>% respond_completion(temp_file, c(4, 6)) + expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) + + result <- client %>% respond_completion(temp_file, c(5, 7)) + expect_length(result$items %>% keep(~ .$label == "read_xml"), 1) }) test_that("Completion of function arguments works", { From be79c3f4ab958bd52412051a22de22598038522c Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 18:07:17 +0800 Subject: [PATCH 08/22] Parse assignment value --- R/document.R | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/R/document.R b/R/document.R index 0131de9d..34d2bd91 100644 --- a/R/document.R +++ b/R/document.R @@ -305,17 +305,20 @@ parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcr } } else if (f %in% c("<-", "=", "delayedAssign", "makeActiveBinding", "assign")) { type <- NULL + recall <- FALSE if (f %in% c("<-", "=")) { if (length(e) != 3L || !is.symbol(e[[2L]])) next symbol <- as.character(e[[2L]]) value <- e[[3L]] + recall <- TRUE } else if (f == "delayedAssign") { call <- match.call(base::delayedAssign, as.call(e)) if (!is.character(call$x)) next if (!is_top_level(call$assign.env)) next symbol <- call$x value <- call$value + recall <- TRUE } else if (f == "assign") { call <- match.call(base::assign, as.call(e)) if (!is.character(call$x)) next @@ -323,6 +326,7 @@ parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcr if (!is_top_level(call$envir)) next symbol <- call$x value <- call$value + recall <- TRUE } else if (f == "makeActiveBinding") { call <- match.call(base::makeActiveBinding, as.call(e)) if (!is.character(call$sym)) next @@ -358,6 +362,10 @@ parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcr } else { env$nonfuncts <- c(env$nonfuncts, symbol) } + + if (recall && is.call(value)) { + Recall(content, value, env, level + 1L, cur_srcref) + } } else if (f %in% names(parse_config$library_functions) && length(e) >= 2L) { pkgs <- tryCatch( parse_config$library_functions[[f]](e), From 4621dc0558fc262be05af3a51b83f3a469179ee0 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 23:15:34 +0800 Subject: [PATCH 09/22] Fix parse_expr handling simple call --- R/document.R | 212 +++++++++++++++++++++++++++------------------------ 1 file changed, 112 insertions(+), 100 deletions(-) diff --git a/R/document.R b/R/document.R index 34d2bd91..98fe7e5f 100644 --- a/R/document.R +++ b/R/document.R @@ -262,120 +262,132 @@ parse_config$library_functions <- c( } ) -parse_expr <- function(content, expr, env, level = 0L, srcref = attr(expr, "srcref")) { - if (length(expr) == 0L || is.symbol(expr)) { +parse_expr <- function(content, expr, env, srcref = attr(expr, "srcref")) { + if (length(expr) == 0L || is.symbol(expr) || is.atomic(expr)) { return(env) } - for (i in seq_along(expr)) { - e <- expr[[i]] - if (missing(e) || !is_simple_call(e)) next - f <- fun_string(e[[1L]]) - cur_srcref <- if (level == 0L) srcref[[i]] else srcref - if (f %in% c("{", "(")) { - Recall(content, e[-1L], env, level + 1L, cur_srcref) - } else if (f == "if") { - Recall(content, e[[2L]], env, level + 1L, cur_srcref) - Recall(content, e[[3L]], env, level + 1L, cur_srcref) - if (length(e) == 4L) { - Recall(content, e[[4L]], env, level + 1L, cur_srcref) - } - } else if (f == "for") { - if (is.symbol(e[[2L]])) { - env$nonfuncts <- c(env$nonfuncts, as.character(e[[2L]])) - } - Recall(content, e[[4L]], env, level + 1L, cur_srcref) - } else if (f == "while") { - Recall(content, e[[2L]], env, level + 1L, cur_srcref) - Recall(content, e[[3L]], env, level + 1L, cur_srcref) - } else if (f == "repeat") { - Recall(content, e[[2L]], env, level + 1L, cur_srcref) - } else if (f %in% names(parse_config$unscoped_functions)) { - if (length(e) >= 2L) { - fun <- tryCatch(eval(e[[1L]], globalenv()), error = function(e) NULL) - if (is.function(fun)) { - call <- match.call(fun, e, expand.dots = FALSE) - captures <- parse_config$unscoped_functions[[f]] - for (capture in captures) { - if (is.call(call[[capture]])) { - Recall(content, call[[capture]], env, level + 1L, cur_srcref) + if (is.expression(expr)) { + for (i in seq_along(expr)) { + Recall(content, expr[[i]], env, srcref[[i]]) + } + } else if (is.list(expr)) { + for (i in seq_along(expr)) { + e <- expr[[i]] + if (missing(e)) next + Recall(content, e, env, srcref) + } + } else if (is.call(expr)) { + if (is_simple_call(expr)) { + e <- expr + f <- fun_string(e[[1]]) + if (f %in% c("{", "(")) { + Recall(content, as.list(e)[-1L], env, srcref) + } else if (f == "if") { + Recall(content, e[[2L]], env, srcref) + Recall(content, e[[3L]], env, srcref) + if (length(e) == 4L) { + Recall(content, e[[4L]], env, srcref) + } + } else if (f == "for") { + if (is.symbol(e[[2L]])) { + env$nonfuncts <- c(env$nonfuncts, as.character(e[[2L]])) + } + Recall(content, e[[4L]], env, srcref) + } else if (f == "while") { + Recall(content, e[[2L]], env, srcref) + Recall(content, e[[3L]], env, srcref) + } else if (f == "repeat") { + Recall(content, e[[2L]], env, srcref) + } else if (f %in% names(parse_config$unscoped_functions)) { + if (length(e) >= 2L) { + fun <- tryCatch(eval(e[[1L]], globalenv()), error = function(e) NULL) + if (is.function(fun)) { + call <- match.call(fun, e, expand.dots = FALSE) + captures <- parse_config$unscoped_functions[[f]] + for (capture in captures) { + if (is.call(call[[capture]])) { + Recall(content, call[[capture]], env, srcref) + } } } } - } - } else if (f %in% c("<-", "=", "delayedAssign", "makeActiveBinding", "assign")) { - type <- NULL - recall <- FALSE - - if (f %in% c("<-", "=")) { - if (length(e) != 3L || !is.symbol(e[[2L]])) next - symbol <- as.character(e[[2L]]) - value <- e[[3L]] - recall <- TRUE - } else if (f == "delayedAssign") { - call <- match.call(base::delayedAssign, as.call(e)) - if (!is.character(call$x)) next - if (!is_top_level(call$assign.env)) next - symbol <- call$x - value <- call$value - recall <- TRUE - } else if (f == "assign") { - call <- match.call(base::assign, as.call(e)) - if (!is.character(call$x)) next - if (!is_top_level(call$pos, -1L, -1)) next # -1 is the default - if (!is_top_level(call$envir)) next - symbol <- call$x - value <- call$value - recall <- TRUE - } else if (f == "makeActiveBinding") { - call <- match.call(base::makeActiveBinding, as.call(e)) - if (!is.character(call$sym)) next - if (!is_top_level(call$env)) next - symbol <- call$sym - value <- call$fun - type <- "variable" - } + } else if (f %in% c("<-", "=", "delayedAssign", "makeActiveBinding", "assign")) { + type <- NULL + recall <- FALSE + + if (f %in% c("<-", "=")) { + if (length(e) != 3L || !is.symbol(e[[2L]])) return(env) + symbol <- as.character(e[[2L]]) + value <- e[[3L]] + recall <- TRUE + } else if (f == "delayedAssign") { + call <- match.call(base::delayedAssign, as.call(e)) + if (!is.character(call$x)) return(env) + if (!is_top_level(call$assign.env)) return(env) + symbol <- call$x + value <- call$value + recall <- TRUE + } else if (f == "assign") { + call <- match.call(base::assign, as.call(e)) + if (!is.character(call$x)) return(env) + if (!is_top_level(call$pos, -1, -1)) return(env) # -1 is the default + if (!is_top_level(call$envir)) return(env) + symbol <- call$x + value <- call$value + recall <- TRUE + } else if (f == "makeActiveBinding") { + call <- match.call(base::makeActiveBinding, as.call(e)) + if (!is.character(call$sym)) return(env) + if (!is_top_level(call$env)) return(env) + symbol <- call$sym + value <- call$fun + type <- "variable" + } - if (is.null(type)) { - type <- get_expr_type(value) - } + if (is.null(type)) { + type <- get_expr_type(value) + } - env$objects <- c(env$objects, symbol) + env$objects <- c(env$objects, symbol) - expr_range <- expr_range(cur_srcref) - env$definitions[[symbol]] <- list( - name = symbol, - type = type, - range = expr_range - ) + expr_range <- expr_range(srcref) + env$definitions[[symbol]] <- list( + name = symbol, + type = type, + range = expr_range + ) - doc_line1 <- detect_comments(content, expr_range$start$line) + 1 - if (doc_line1 <= expr_range$start$line) { - comment <- content[seq.int(doc_line1, expr_range$start$line)] - env$documentation[[symbol]] <- convert_comment_to_documentation(comment) - } + doc_line1 <- detect_comments(content, expr_range$start$line) + 1 + if (doc_line1 <= expr_range$start$line) { + comment <- content[seq.int(doc_line1, expr_range$start$line)] + env$documentation[[symbol]] <- convert_comment_to_documentation(comment) + } - if (type == "function") { - env$functs <- c(env$functs, symbol) - env$formals[[symbol]] <- value[[2L]] - env$signatures[[symbol]] <- get_signature(symbol, value) - } else { - env$nonfuncts <- c(env$nonfuncts, symbol) - } + if (type == "function") { + env$functs <- c(env$functs, symbol) + env$formals[[symbol]] <- value[[2L]] + env$signatures[[symbol]] <- get_signature(symbol, value) + } else { + env$nonfuncts <- c(env$nonfuncts, symbol) + } - if (recall && is.call(value)) { - Recall(content, value, env, level + 1L, cur_srcref) - } - } else if (f %in% names(parse_config$library_functions) && length(e) >= 2L) { - pkgs <- tryCatch( - parse_config$library_functions[[f]](e), - error = function(e) NULL - ) - pkgs <- pkgs[nzchar(pkgs)] - if (length(pkgs)) { - env$packages <- union(env$packages, pkgs) + if (recall && is.call(value)) { + Recall(content, value, env, srcref) + } + } else if (f %in% names(parse_config$library_functions) && length(e) >= 2L) { + pkgs <- tryCatch( + parse_config$library_functions[[f]](e), + error = function(e) NULL + ) + pkgs <- pkgs[nzchar(pkgs)] + if (length(pkgs)) { + env$packages <- union(env$packages, pkgs) + } } } + } else { + stop("Invalid type") } env } From 4d30404a12ed76375b25a3c8947286de3cc3d6fe Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 23:17:52 +0800 Subject: [PATCH 10/22] Update parse_expr --- R/document.R | 194 +++++++++++++++++++++++++-------------------------- 1 file changed, 95 insertions(+), 99 deletions(-) diff --git a/R/document.R b/R/document.R index 98fe7e5f..06f2562a 100644 --- a/R/document.R +++ b/R/document.R @@ -277,117 +277,113 @@ parse_expr <- function(content, expr, env, srcref = attr(expr, "srcref")) { if (missing(e)) next Recall(content, e, env, srcref) } - } else if (is.call(expr)) { - if (is_simple_call(expr)) { - e <- expr - f <- fun_string(e[[1]]) - if (f %in% c("{", "(")) { - Recall(content, as.list(e)[-1L], env, srcref) - } else if (f == "if") { - Recall(content, e[[2L]], env, srcref) - Recall(content, e[[3L]], env, srcref) - if (length(e) == 4L) { - Recall(content, e[[4L]], env, srcref) - } - } else if (f == "for") { - if (is.symbol(e[[2L]])) { - env$nonfuncts <- c(env$nonfuncts, as.character(e[[2L]])) - } + } else if (is_simple_call(expr)) { + e <- expr + f <- fun_string(e[[1]]) + if (f %in% c("{", "(")) { + Recall(content, as.list(e)[-1L], env, srcref) + } else if (f == "if") { + Recall(content, e[[2L]], env, srcref) + Recall(content, e[[3L]], env, srcref) + if (length(e) == 4L) { Recall(content, e[[4L]], env, srcref) - } else if (f == "while") { - Recall(content, e[[2L]], env, srcref) - Recall(content, e[[3L]], env, srcref) - } else if (f == "repeat") { - Recall(content, e[[2L]], env, srcref) - } else if (f %in% names(parse_config$unscoped_functions)) { - if (length(e) >= 2L) { - fun <- tryCatch(eval(e[[1L]], globalenv()), error = function(e) NULL) - if (is.function(fun)) { - call <- match.call(fun, e, expand.dots = FALSE) - captures <- parse_config$unscoped_functions[[f]] - for (capture in captures) { - if (is.call(call[[capture]])) { - Recall(content, call[[capture]], env, srcref) - } + } + } else if (f == "for") { + if (is.symbol(e[[2L]])) { + env$nonfuncts <- c(env$nonfuncts, as.character(e[[2L]])) + } + Recall(content, e[[4L]], env, srcref) + } else if (f == "while") { + Recall(content, e[[2L]], env, srcref) + Recall(content, e[[3L]], env, srcref) + } else if (f == "repeat") { + Recall(content, e[[2L]], env, srcref) + } else if (f %in% names(parse_config$unscoped_functions)) { + if (length(e) >= 2L) { + fun <- tryCatch(eval(e[[1L]], globalenv()), error = function(e) NULL) + if (is.function(fun)) { + call <- match.call(fun, e, expand.dots = FALSE) + captures <- parse_config$unscoped_functions[[f]] + for (capture in captures) { + if (is.call(call[[capture]])) { + Recall(content, call[[capture]], env, srcref) } } } - } else if (f %in% c("<-", "=", "delayedAssign", "makeActiveBinding", "assign")) { - type <- NULL - recall <- FALSE - - if (f %in% c("<-", "=")) { - if (length(e) != 3L || !is.symbol(e[[2L]])) return(env) - symbol <- as.character(e[[2L]]) - value <- e[[3L]] - recall <- TRUE - } else if (f == "delayedAssign") { - call <- match.call(base::delayedAssign, as.call(e)) - if (!is.character(call$x)) return(env) - if (!is_top_level(call$assign.env)) return(env) - symbol <- call$x - value <- call$value - recall <- TRUE - } else if (f == "assign") { - call <- match.call(base::assign, as.call(e)) - if (!is.character(call$x)) return(env) - if (!is_top_level(call$pos, -1, -1)) return(env) # -1 is the default - if (!is_top_level(call$envir)) return(env) - symbol <- call$x - value <- call$value - recall <- TRUE - } else if (f == "makeActiveBinding") { - call <- match.call(base::makeActiveBinding, as.call(e)) - if (!is.character(call$sym)) return(env) - if (!is_top_level(call$env)) return(env) - symbol <- call$sym - value <- call$fun - type <- "variable" - } + } + } else if (f %in% c("<-", "=", "delayedAssign", "makeActiveBinding", "assign")) { + type <- NULL + recall <- FALSE + + if (f %in% c("<-", "=")) { + if (length(e) != 3L || !is.symbol(e[[2L]])) return(env) + symbol <- as.character(e[[2L]]) + value <- e[[3L]] + recall <- TRUE + } else if (f == "delayedAssign") { + call <- match.call(base::delayedAssign, as.call(e)) + if (!is.character(call$x)) return(env) + if (!is_top_level(call$assign.env)) return(env) + symbol <- call$x + value <- call$value + recall <- TRUE + } else if (f == "assign") { + call <- match.call(base::assign, as.call(e)) + if (!is.character(call$x)) return(env) + if (!is_top_level(call$pos, -1, -1)) return(env) # -1 is the default + if (!is_top_level(call$envir)) return(env) + symbol <- call$x + value <- call$value + recall <- TRUE + } else if (f == "makeActiveBinding") { + call <- match.call(base::makeActiveBinding, as.call(e)) + if (!is.character(call$sym)) return(env) + if (!is_top_level(call$env)) return(env) + symbol <- call$sym + value <- call$fun + type <- "variable" + } - if (is.null(type)) { - type <- get_expr_type(value) - } + if (is.null(type)) { + type <- get_expr_type(value) + } - env$objects <- c(env$objects, symbol) + env$objects <- c(env$objects, symbol) - expr_range <- expr_range(srcref) - env$definitions[[symbol]] <- list( - name = symbol, - type = type, - range = expr_range - ) + expr_range <- expr_range(srcref) + env$definitions[[symbol]] <- list( + name = symbol, + type = type, + range = expr_range + ) - doc_line1 <- detect_comments(content, expr_range$start$line) + 1 - if (doc_line1 <= expr_range$start$line) { - comment <- content[seq.int(doc_line1, expr_range$start$line)] - env$documentation[[symbol]] <- convert_comment_to_documentation(comment) - } + doc_line1 <- detect_comments(content, expr_range$start$line) + 1 + if (doc_line1 <= expr_range$start$line) { + comment <- content[seq.int(doc_line1, expr_range$start$line)] + env$documentation[[symbol]] <- convert_comment_to_documentation(comment) + } - if (type == "function") { - env$functs <- c(env$functs, symbol) - env$formals[[symbol]] <- value[[2L]] - env$signatures[[symbol]] <- get_signature(symbol, value) - } else { - env$nonfuncts <- c(env$nonfuncts, symbol) - } + if (type == "function") { + env$functs <- c(env$functs, symbol) + env$formals[[symbol]] <- value[[2L]] + env$signatures[[symbol]] <- get_signature(symbol, value) + } else { + env$nonfuncts <- c(env$nonfuncts, symbol) + } - if (recall && is.call(value)) { - Recall(content, value, env, srcref) - } - } else if (f %in% names(parse_config$library_functions) && length(e) >= 2L) { - pkgs <- tryCatch( - parse_config$library_functions[[f]](e), - error = function(e) NULL - ) - pkgs <- pkgs[nzchar(pkgs)] - if (length(pkgs)) { - env$packages <- union(env$packages, pkgs) - } + if (recall && is.call(value)) { + Recall(content, value, env, srcref) + } + } else if (f %in% names(parse_config$library_functions) && length(e) >= 2L) { + pkgs <- tryCatch( + parse_config$library_functions[[f]](e), + error = function(e) NULL + ) + pkgs <- pkgs[nzchar(pkgs)] + if (length(pkgs)) { + env$packages <- union(env$packages, pkgs) } } - } else { - stop("Invalid type") } env } From 4a63357a6042e5f2aa795e7dddda2f3fbf0c2d0d Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 23:19:27 +0800 Subject: [PATCH 11/22] Update test cases --- tests/testthat/test-completion.R | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/tests/testthat/test-completion.R b/tests/testthat/test-completion.R index c1d31e16..6f5a5909 100644 --- a/tests/testthat/test-completion.R +++ b/tests/testthat/test-completion.R @@ -165,6 +165,19 @@ test_that("Completion of package functions attached in unscoped functions works" client <- language_client() temp_file <- withr::local_tempfile(fileext = ".R") + writeLines( + c( + "suppressPackageStartupMessages(library(jsonlite))", + "fromJS", + ), + temp_file) + + client %>% did_save(temp_file) + Sys.sleep(1) + + result <- client %>% respond_completion(temp_file, c(1, 6)) + expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) + writeLines( c( "suppressPackageStartupMessages({", From 766c5b3d62131bb4da1b4982bfa4e47fe7f9cd47 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 23:27:03 +0800 Subject: [PATCH 12/22] Fix test case --- tests/testthat/test-completion.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/testthat/test-completion.R b/tests/testthat/test-completion.R index 6f5a5909..189ad0be 100644 --- a/tests/testthat/test-completion.R +++ b/tests/testthat/test-completion.R @@ -168,7 +168,7 @@ test_that("Completion of package functions attached in unscoped functions works" writeLines( c( "suppressPackageStartupMessages(library(jsonlite))", - "fromJS", + "fromJS" ), temp_file) From 8617ce14317f1eaf2c27339d1eaa2bbdc1414817 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Thu, 8 Jul 2021 23:57:38 +0800 Subject: [PATCH 13/22] Fix test case --- R/document.R | 2 +- tests/testthat/test-symbol.R | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/R/document.R b/R/document.R index 06f2562a..7c5f0ea7 100644 --- a/R/document.R +++ b/R/document.R @@ -330,7 +330,7 @@ parse_expr <- function(content, expr, env, srcref = attr(expr, "srcref")) { } else if (f == "assign") { call <- match.call(base::assign, as.call(e)) if (!is.character(call$x)) return(env) - if (!is_top_level(call$pos, -1, -1)) return(env) # -1 is the default + if (!is_top_level(call$pos, -1L, -1)) return(env) # -1 is the default if (!is_top_level(call$envir)) return(env) symbol <- call$x value <- call$value diff --git a/tests/testthat/test-symbol.R b/tests/testthat/test-symbol.R index f1ae8730..568b3b6e 100644 --- a/tests/testthat/test-symbol.R +++ b/tests/testthat/test-symbol.R @@ -59,7 +59,7 @@ test_that("Recognize symbols created by delayedAssign()/assign()/makeActiveBindi "makeActiveBinding('a5', function() 5, .GlobalEnv)", "makeActiveBinding('a6', function() 6, new.env())", "assign(value = '1', x = 'assign1')", - "assign('assign2', 2, pos = -1L)", + "assign('assign2', 2, pos = -1)", "assign('assign3', 3, pos = environment())", "assign('assign4', 4, pos = new.env())" ), defn_file) From 47c1b0a7e88be0db5ad65bfc8d0388b53f91b196 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Fri, 9 Jul 2021 00:00:33 +0800 Subject: [PATCH 14/22] Use list instead of c --- R/document.R | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/R/document.R b/R/document.R index 7c5f0ea7..6b7675cd 100644 --- a/R/document.R +++ b/R/document.R @@ -237,7 +237,7 @@ parse_config$unscoped_functions <- list( suppressWarnings = "expr" ) -parse_config$library_functions <- c( +parse_config$library_functions <- list( "library" = function(call) { call <- match.call(base::library, call) if (!isTRUE(call$character.only)) { From b9c0542937e535b9ccb04e431e7d85e49da1e018 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Fri, 9 Jul 2021 07:37:03 +0800 Subject: [PATCH 15/22] Update test cases --- tests/testthat/test-completion.R | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/tests/testthat/test-completion.R b/tests/testthat/test-completion.R index 189ad0be..2a4d08c4 100644 --- a/tests/testthat/test-completion.R +++ b/tests/testthat/test-completion.R @@ -151,12 +151,13 @@ test_that("Completion of attached package functions works", { temp_file) client %>% did_save(temp_file) - Sys.sleep(1) - result <- client %>% respond_completion(temp_file, c(1, 6)) + result <- client %>% respond_completion(temp_file, c(1, 6), + retry_when = function(result) result$items %>% keep(~ .$label == "fromJSON") %>% length() == 0) expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) - result <- client %>% respond_completion(temp_file, c(2, 7)) + result <- client %>% respond_completion(temp_file, c(2, 7), + retry_when = function(result) result$items %>% keep(~ .$label == "read_xml") %>% length() == 0) expect_length(result$items %>% keep(~ .$label == "read_xml"), 1) }) @@ -173,9 +174,9 @@ test_that("Completion of package functions attached in unscoped functions works" temp_file) client %>% did_save(temp_file) - Sys.sleep(1) - result <- client %>% respond_completion(temp_file, c(1, 6)) + result <- client %>% respond_completion(temp_file, c(1, 6), + retry_when = function(result) result$items %>% keep(~ .$label == "fromJSON") %>% length() == 0) expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) writeLines( @@ -192,10 +193,12 @@ test_that("Completion of package functions attached in unscoped functions works" client %>% did_save(temp_file) Sys.sleep(1) - result <- client %>% respond_completion(temp_file, c(4, 6)) + result <- client %>% respond_completion(temp_file, c(4, 6), + retry_when = function(result) result$items %>% keep(~ .$label == "fromJSON") %>% length() == 0) expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) - result <- client %>% respond_completion(temp_file, c(5, 7)) + result <- client %>% respond_completion(temp_file, c(5, 7), + retry_when = function(result) result$items %>% keep(~ .$label == "read_xml") %>% length() == 0) expect_length(result$items %>% keep(~ .$label == "read_xml"), 1) }) From 22426a667cc6ac78c0cc1b9c46b09f1d4b0e7fc8 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Fri, 9 Jul 2021 08:18:32 +0800 Subject: [PATCH 16/22] Update test cases --- tests/testthat/test-completion.R | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/testthat/test-completion.R b/tests/testthat/test-completion.R index 2a4d08c4..199051ea 100644 --- a/tests/testthat/test-completion.R +++ b/tests/testthat/test-completion.R @@ -133,12 +133,13 @@ test_that("Completion of attached package functions works", { temp_file) client %>% did_save(temp_file) - Sys.sleep(1) - result <- client %>% respond_completion(temp_file, c(2, 6)) + result <- client %>% respond_completion(temp_file, c(2, 6), + retry_when = function(result) result$items %>% keep(~ .$label == "fromJSON") %>% length() == 0) expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) - result <- client %>% respond_completion(temp_file, c(3, 7)) + result <- client %>% respond_completion(temp_file, c(3, 7), + retry_when = function(result) result$items %>% keep(~ .$label == "read_xml") %>% length() == 0) expect_length(result$items %>% keep(~ .$label == "read_xml"), 1) temp_file <- withr::local_tempfile(fileext = ".R") @@ -191,7 +192,6 @@ test_that("Completion of package functions attached in unscoped functions works" temp_file) client %>% did_save(temp_file) - Sys.sleep(1) result <- client %>% respond_completion(temp_file, c(4, 6), retry_when = function(result) result$items %>% keep(~ .$label == "fromJSON") %>% length() == 0) From 3003c829b91817dda1604a872555ccc585e6796b Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Sat, 10 Jul 2021 15:29:23 +0800 Subject: [PATCH 17/22] Use parser_hooks --- R/document.R | 251 +++++++++++++++++++++++++-------------------------- R/zzz.R | 8 +- 2 files changed, 128 insertions(+), 131 deletions(-) diff --git a/R/document.R b/R/document.R index 6b7675cd..ed110c81 100644 --- a/R/document.R +++ b/R/document.R @@ -223,43 +223,96 @@ is_top_level <- function(arg_env, ...) { any(vapply(top_level_envs, identical, x = arg_env, FUN.VALUE = logical(1L))) } -parse_config <- new.env() -parse_config$unscoped_functions <- list( - system.time = "expr", - try = "expr", - tryCatch = c("expr", "finally"), - withCallingHandlers = "expr", - withRestarts = "expr", - allowInterrupts = "expr", - suspendInterrupts = "expr", - suppressPackageStartupMessages = "expr", - suppressMessages = "expr", - suppressWarnings = "expr" -) - -parse_config$library_functions <- list( - "library" = function(call) { - call <- match.call(base::library, call) +parser_hooks <- list( + "{" = function(expr, action) { + action$recall(as.list(expr)[-1L]) + }, + "(" = function(expr, action) { + action$recall(as.list(expr)[-1L]) + }, + "if" = function(expr, action) { + action$recall(as.list(expr)[2:4]) + }, + "for" = function(expr, action) { + if (is.symbol(e <- expr[[2L]])) { + action$update(nonfuncts = as.character(e)) + } + action$recall(expr[[4L]]) + }, + "while" = function(expr, action) { + action$recall(as.list(expr)[2:3]) + }, + "repeat" = function(expr, action) { + action$recall(expr[[2L]]) + }, + "<-" = function(expr, action) { + if (length(expr) == 3L && is.symbol(expr[[2L]])) { + action$assign(symbol = as.character(expr[[2L]]), value = expr[[3L]]) + action$recall(expr[[3L]]) + } + }, + "=" = function(expr, action) { + if (length(expr) == 3L && is.symbol(expr[[2L]])) { + action$assign(symbol = as.character(expr[[2L]]), value = expr[[3L]]) + action$recall(expr[[3L]]) + } + }, + "assign" = function(expr, action) { + call <- match.call(base::assign, expr) + if (is.character(call$x) && is_top_level(call$pos, -1L, -1) && is_top_level(call$envir)) { + action$assign(symbol = call$x, value = call$value) + action$recall(call$value) + } + }, + "delayedAssign" = function(expr, action) { + call <- match.call(base::delayedAssign, expr) + if (is.character(call$x) && is_top_level(call$assign.env)) { + action$assign(symbol = call$x, value = call$value) + action$recall(call$value) + } + }, + "makeActiveBinding" = function(expr, action) { + call <- match.call(base::makeActiveBinding, expr) + if (is.character(call$sym) && is_top_level(call$env)) { + action$assign(symbol = call$sym, value = call$fun, type = "variable") + } + }, + "library" = function(expr, action) { + call <- match.call(base::library, expr) if (!isTRUE(call$character.only)) { - as.character(call$package) + action$update(packages = as.character(call$package)) } + NULL }, - "require" = function(call) { - call <- match.call(base::require, call) + "require" = function(expr, action) { + call <- match.call(base::require, expr) if (!isTRUE(call$character.only)) { - as.character(call$package) + action$update(packages = as.character(call$package)) } + NULL }, - "pacman::p_load" = function(call) { + "pacman::p_load" = function(expr, action) { fun <- if (requireNamespace("pacman")) pacman::p_load else function(..., char, install = TRUE, update = getOption("pac_update"), character.only = FALSE) NULL - call <- match.call(fun, call, expand.dots = FALSE) + call <- match.call(fun, expr, expand.dots = FALSE) if (!isTRUE(call$character.only)) { - vapply(call[["..."]], as.character, character(1L)) + packages <- vapply(call[["..."]], as.character, character(1L)) + action$update(packages = packages) } - } + NULL + }, + "system.time" = function(expr, action) action$recall("expr"), + "try" = function(expr, action) action$recall("expr"), + "tryCatch" = function(expr, action) action$recall(c("expr", "finally")), + "withCallingHandlers" = function(expr, action) action$recall("expr"), + "withRestarts" = function(expr, action) action$recall("expr"), + "allowInterrupts" = function(expr, action) action$recall("expr"), + "suspendInterrupts" = function(expr, action) action$recall("expr"), + "suppressPackageStartupMessages" = function(expr, action) action$recall("expr"), + "suppressMessages" = function(expr, action) action$recall("expr"), + "suppressWarnings" = function(expr, action) action$recall("expr") ) parse_expr <- function(content, expr, env, srcref = attr(expr, "srcref")) { @@ -278,111 +331,57 @@ parse_expr <- function(content, expr, env, srcref = attr(expr, "srcref")) { Recall(content, e, env, srcref) } } else if (is_simple_call(expr)) { - e <- expr - f <- fun_string(e[[1]]) - if (f %in% c("{", "(")) { - Recall(content, as.list(e)[-1L], env, srcref) - } else if (f == "if") { - Recall(content, e[[2L]], env, srcref) - Recall(content, e[[3L]], env, srcref) - if (length(e) == 4L) { - Recall(content, e[[4L]], env, srcref) - } - } else if (f == "for") { - if (is.symbol(e[[2L]])) { - env$nonfuncts <- c(env$nonfuncts, as.character(e[[2L]])) - } - Recall(content, e[[4L]], env, srcref) - } else if (f == "while") { - Recall(content, e[[2L]], env, srcref) - Recall(content, e[[3L]], env, srcref) - } else if (f == "repeat") { - Recall(content, e[[2L]], env, srcref) - } else if (f %in% names(parse_config$unscoped_functions)) { - if (length(e) >= 2L) { - fun <- tryCatch(eval(e[[1L]], globalenv()), error = function(e) NULL) - if (is.function(fun)) { - call <- match.call(fun, e, expand.dots = FALSE) - captures <- parse_config$unscoped_functions[[f]] - for (capture in captures) { - if (is.call(call[[capture]])) { - Recall(content, call[[capture]], env, srcref) + f <- fun_string(expr[[1L]]) + fun <- parser_hooks[[f]] + if (is.function(fun)) { + action <- list( + update = function(...) { + updates <- list(...) + for (name in names(updates)) { + env[[name]] <- union(env[[name]], updates[[name]]) + } + }, + assign = function(symbol, value, type = get_expr_type(value)) { + env$objects <- c(env$objects, symbol) + + expr_range <- expr_range(srcref) + env$definitions[[symbol]] <- list( + name = symbol, + type = type, + range = expr_range + ) + + doc_line1 <- detect_comments(content, expr_range$start$line) + 1 + if (doc_line1 <= expr_range$start$line) { + comment <- content[seq.int(doc_line1, expr_range$start$line)] + env$documentation[[symbol]] <- convert_comment_to_documentation(comment) + } + + if (type == "function") { + env$functs <- c(env$functs, symbol) + env$formals[[symbol]] <- value[[2L]] + env$signatures[[symbol]] <- get_signature(symbol, value) + } else { + env$nonfuncts <- c(env$nonfuncts, symbol) + } + }, + recall = function(value) { + if (is.character(value)) { + fn <- tryCatch(eval(expr[[1L]], globalenv()), error = function(e) NULL) + if (is.function(fn)) { + call <- match.call(fn, expr, expand.dots = FALSE) + for (arg in value) { + if (is.call(call[[arg]])) { + parse_expr(content, call[[arg]], env, srcref) + } + } } + } else { + parse_expr(content, value, env, srcref) } } - } - } else if (f %in% c("<-", "=", "delayedAssign", "makeActiveBinding", "assign")) { - type <- NULL - recall <- FALSE - - if (f %in% c("<-", "=")) { - if (length(e) != 3L || !is.symbol(e[[2L]])) return(env) - symbol <- as.character(e[[2L]]) - value <- e[[3L]] - recall <- TRUE - } else if (f == "delayedAssign") { - call <- match.call(base::delayedAssign, as.call(e)) - if (!is.character(call$x)) return(env) - if (!is_top_level(call$assign.env)) return(env) - symbol <- call$x - value <- call$value - recall <- TRUE - } else if (f == "assign") { - call <- match.call(base::assign, as.call(e)) - if (!is.character(call$x)) return(env) - if (!is_top_level(call$pos, -1L, -1)) return(env) # -1 is the default - if (!is_top_level(call$envir)) return(env) - symbol <- call$x - value <- call$value - recall <- TRUE - } else if (f == "makeActiveBinding") { - call <- match.call(base::makeActiveBinding, as.call(e)) - if (!is.character(call$sym)) return(env) - if (!is_top_level(call$env)) return(env) - symbol <- call$sym - value <- call$fun - type <- "variable" - } - - if (is.null(type)) { - type <- get_expr_type(value) - } - - env$objects <- c(env$objects, symbol) - - expr_range <- expr_range(srcref) - env$definitions[[symbol]] <- list( - name = symbol, - type = type, - range = expr_range - ) - - doc_line1 <- detect_comments(content, expr_range$start$line) + 1 - if (doc_line1 <= expr_range$start$line) { - comment <- content[seq.int(doc_line1, expr_range$start$line)] - env$documentation[[symbol]] <- convert_comment_to_documentation(comment) - } - - if (type == "function") { - env$functs <- c(env$functs, symbol) - env$formals[[symbol]] <- value[[2L]] - env$signatures[[symbol]] <- get_signature(symbol, value) - } else { - env$nonfuncts <- c(env$nonfuncts, symbol) - } - - if (recall && is.call(value)) { - Recall(content, value, env, srcref) - } - } else if (f %in% names(parse_config$library_functions) && length(e) >= 2L) { - pkgs <- tryCatch( - parse_config$library_functions[[f]](e), - error = function(e) NULL ) - pkgs <- pkgs[nzchar(pkgs)] - if (length(pkgs)) { - env$packages <- union(env$packages, pkgs) - } + fun(expr, action) } } env diff --git a/R/zzz.R b/R/zzz.R index 30a9ae6c..755d697e 100644 --- a/R/zzz.R +++ b/R/zzz.R @@ -1,7 +1,5 @@ .onLoad <- function(...) { - unscoped_functions <- getOption("languageserver.unscoped_functions") - parse_config$unscoped_functions[names(unscoped_functions)] <- unscoped_functions - - library_functions <- getOption("languageserver.library_functions") - parse_config$library_functions[names(library_functions)] <- library_functions + user_parser_hooks <- getOption("languageserver.parser_hooks") + parser_hooks[names(user_parser_hooks)] <<- user_parser_hooks + invisible() } From 11c6e2b90ea6b3e7948daf77396708c93b21c68a Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Sat, 10 Jul 2021 17:26:57 +0800 Subject: [PATCH 18/22] Make update and assign more robust --- R/document.R | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/R/document.R b/R/document.R index ed110c81..7ab38587 100644 --- a/R/document.R +++ b/R/document.R @@ -338,10 +338,16 @@ parse_expr <- function(content, expr, env, srcref = attr(expr, "srcref")) { update = function(...) { updates <- list(...) for (name in names(updates)) { - env[[name]] <- union(env[[name]], updates[[name]]) + values <- updates[[name]] + values <- values[nzchar(values)] + if (length(values)) { + env[[name]] <- union(env[[name]], values) + } } }, assign = function(symbol, value, type = get_expr_type(value)) { + if (!nzchar(symbol)) return(NULL) + env$objects <- c(env$objects, symbol) expr_range <- expr_range(srcref) From 4af7226f8cd61b257df2fbb311af02e4e2ccae1a Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Sat, 10 Jul 2021 18:11:23 +0800 Subject: [PATCH 19/22] More robust --- R/document.R | 2 +- tests/testthat/test-completion.R | 27 +++++++++++++++++++++++++++ tests/testthat/test-symbol.R | 28 +++++++++++++++++++++++++++- 3 files changed, 55 insertions(+), 2 deletions(-) diff --git a/R/document.R b/R/document.R index 7ab38587..7e34e00e 100644 --- a/R/document.R +++ b/R/document.R @@ -387,7 +387,7 @@ parse_expr <- function(content, expr, env, srcref = attr(expr, "srcref")) { } } ) - fun(expr, action) + tryCatch(fun(expr, action), error = function(e) NULL) } } env diff --git a/tests/testthat/test-completion.R b/tests/testthat/test-completion.R index 199051ea..13beff09 100644 --- a/tests/testthat/test-completion.R +++ b/tests/testthat/test-completion.R @@ -202,6 +202,33 @@ test_that("Completion of package functions attached in unscoped functions works" expect_length(result$items %>% keep(~ .$label == "read_xml"), 1) }) +test_that("Completion is robust to invalid source", { + skip_on_cran() + client <- language_client() + + temp_file <- withr::local_tempfile(fileext = ".R") + writeLines( + c( + "library(jsonlite)", + "require('xml2')", + "require('')", + "require('xml2', nonexist_arg = 0)", + "fromJS", + "read_xm" + ), + temp_file) + + client %>% did_save(temp_file) + + result <- client %>% respond_completion(temp_file, c(4, 6), + retry_when = function(result) result$items %>% keep(~ .$label == "fromJSON") %>% length() == 0) + expect_length(result$items %>% keep(~ .$label == "fromJSON"), 1) + + result <- client %>% respond_completion(temp_file, c(5, 7), + retry_when = function(result) result$items %>% keep(~ .$label == "read_xml") %>% length() == 0) + expect_length(result$items %>% keep(~ .$label == "read_xml"), 1) +}) + test_that("Completion of function arguments works", { skip_on_cran() client <- language_client() diff --git a/tests/testthat/test-symbol.R b/tests/testthat/test-symbol.R index 568b3b6e..aa378dc5 100644 --- a/tests/testthat/test-symbol.R +++ b/tests/testthat/test-symbol.R @@ -36,7 +36,7 @@ test_that("Document Symbol works", { ) }) -test_that("Recognize symbols created by delayedAssign()/assign()/makeActiveBinding()", { +test_that("Recognize symbols created by delayedAssign/assign/makeActiveBinding", { skip_on_cran() client <- language_client() @@ -73,6 +73,32 @@ test_that("Recognize symbols created by delayedAssign()/assign()/makeActiveBindi ) }) +test_that("Document symbols are robust to invalid source", { + skip_on_cran() + client <- language_client() + + defn_file <- withr::local_tempfile(fileext = ".R") + writeLines(c( + "'' <- 2", + "assign('', 3)", + "delayedAssign('', 1)", + "makeActiveBinding('', 2)", + "makeActiveBinding(x, 1)", + "assign('var1', 1, nonexist_arg = TRUE)", + "delayedAssign('var1', 1, nonexist_arg = TRUE)", + "makeActiveBinding('var1', 1, nonexist_arg = TRUE)", + "d1 <- 1" + ), defn_file) + + client %>% did_save(defn_file) + result <- client %>% respond_document_symbol(defn_file) + + expect_setequal( + result %>% map_chr(~ .$name), + c("d1") + ) +}) + test_that("Document section symbol works", { skip_on_cran() client <- language_client(capabilities = list( From 2a6f02ecdce56ea52797bbcc4dac0c3df19b330f Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Sat, 10 Jul 2021 18:56:07 +0800 Subject: [PATCH 20/22] Update parser_hooks --- R/document.R | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/R/document.R b/R/document.R index 7e34e00e..7c68ccca 100644 --- a/R/document.R +++ b/R/document.R @@ -231,7 +231,7 @@ parser_hooks <- list( action$recall(as.list(expr)[-1L]) }, "if" = function(expr, action) { - action$recall(as.list(expr)[2:4]) + action$recall(as.list(expr)[-1L]) }, "for" = function(expr, action) { if (is.symbol(e <- expr[[2L]])) { @@ -240,7 +240,7 @@ parser_hooks <- list( action$recall(expr[[4L]]) }, "while" = function(expr, action) { - action$recall(as.list(expr)[2:3]) + action$recall(as.list(expr)[-1L]) }, "repeat" = function(expr, action) { action$recall(expr[[2L]]) From a76d01876e80cde6517bc6176ecaa8e3ffc92a21 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Sat, 10 Jul 2021 19:34:20 +0800 Subject: [PATCH 21/22] Rename functions in parser_hooks --- R/document.R | 61 ++++++++++++++++++++++++++-------------------------- 1 file changed, 30 insertions(+), 31 deletions(-) diff --git a/R/document.R b/R/document.R index 7c68ccca..6729c553 100644 --- a/R/document.R +++ b/R/document.R @@ -225,50 +225,50 @@ is_top_level <- function(arg_env, ...) { parser_hooks <- list( "{" = function(expr, action) { - action$recall(as.list(expr)[-1L]) + action$parse(as.list(expr)[-1L]) }, "(" = function(expr, action) { - action$recall(as.list(expr)[-1L]) + action$parse(as.list(expr)[-1L]) }, "if" = function(expr, action) { - action$recall(as.list(expr)[-1L]) + action$parse(as.list(expr)[-1L]) }, "for" = function(expr, action) { if (is.symbol(e <- expr[[2L]])) { action$update(nonfuncts = as.character(e)) } - action$recall(expr[[4L]]) + action$parse(expr[[4L]]) }, "while" = function(expr, action) { - action$recall(as.list(expr)[-1L]) + action$parse(as.list(expr)[-1L]) }, "repeat" = function(expr, action) { - action$recall(expr[[2L]]) + action$parse(expr[[2L]]) }, "<-" = function(expr, action) { if (length(expr) == 3L && is.symbol(expr[[2L]])) { action$assign(symbol = as.character(expr[[2L]]), value = expr[[3L]]) - action$recall(expr[[3L]]) + action$parse(expr[[3L]]) } }, "=" = function(expr, action) { if (length(expr) == 3L && is.symbol(expr[[2L]])) { action$assign(symbol = as.character(expr[[2L]]), value = expr[[3L]]) - action$recall(expr[[3L]]) + action$parse(expr[[3L]]) } }, "assign" = function(expr, action) { call <- match.call(base::assign, expr) if (is.character(call$x) && is_top_level(call$pos, -1L, -1) && is_top_level(call$envir)) { action$assign(symbol = call$x, value = call$value) - action$recall(call$value) + action$parse(call$value) } }, "delayedAssign" = function(expr, action) { call <- match.call(base::delayedAssign, expr) if (is.character(call$x) && is_top_level(call$assign.env)) { action$assign(symbol = call$x, value = call$value) - action$recall(call$value) + action$parse(call$value) } }, "makeActiveBinding" = function(expr, action) { @@ -303,16 +303,16 @@ parser_hooks <- list( } NULL }, - "system.time" = function(expr, action) action$recall("expr"), - "try" = function(expr, action) action$recall("expr"), - "tryCatch" = function(expr, action) action$recall(c("expr", "finally")), - "withCallingHandlers" = function(expr, action) action$recall("expr"), - "withRestarts" = function(expr, action) action$recall("expr"), - "allowInterrupts" = function(expr, action) action$recall("expr"), - "suspendInterrupts" = function(expr, action) action$recall("expr"), - "suppressPackageStartupMessages" = function(expr, action) action$recall("expr"), - "suppressMessages" = function(expr, action) action$recall("expr"), - "suppressWarnings" = function(expr, action) action$recall("expr") + "system.time" = function(expr, action) action$parse_args("expr"), + "try" = function(expr, action) action$parse_args("expr"), + "tryCatch" = function(expr, action) action$parse_args(c("expr", "finally")), + "withCallingHandlers" = function(expr, action) action$parse_args("expr"), + "withRestarts" = function(expr, action) action$parse_args("expr"), + "allowInterrupts" = function(expr, action) action$parse_args("expr"), + "suspendInterrupts" = function(expr, action) action$parse_args("expr"), + "suppressPackageStartupMessages" = function(expr, action) action$parse_args("expr"), + "suppressMessages" = function(expr, action) action$parse_args("expr"), + "suppressWarnings" = function(expr, action) action$parse_args("expr") ) parse_expr <- function(content, expr, env, srcref = attr(expr, "srcref")) { @@ -371,19 +371,18 @@ parse_expr <- function(content, expr, env, srcref = attr(expr, "srcref")) { env$nonfuncts <- c(env$nonfuncts, symbol) } }, - recall = function(value) { - if (is.character(value)) { - fn <- tryCatch(eval(expr[[1L]], globalenv()), error = function(e) NULL) - if (is.function(fn)) { - call <- match.call(fn, expr, expand.dots = FALSE) - for (arg in value) { - if (is.call(call[[arg]])) { - parse_expr(content, call[[arg]], env, srcref) - } + parse = function(expr) { + parse_expr(content, expr, env, srcref) + }, + parse_args = function(args) { + fn <- tryCatch(eval(expr[[1L]], globalenv()), error = function(e) NULL) + if (is.function(fn)) { + call <- match.call(fn, expr, expand.dots = FALSE) + for (arg in args) { + if (is.call(call[[arg]])) { + parse_expr(content, call[[arg]], env, srcref) } } - } else { - parse_expr(content, value, env, srcref) } } ) From ee90170f0af25f9b2a313ff32809c04014c7e584 Mon Sep 17 00:00:00 2001 From: Kun Ren Date: Sat, 10 Jul 2021 21:32:52 +0800 Subject: [PATCH 22/22] Clean up --- R/document.R | 3 --- 1 file changed, 3 deletions(-) diff --git a/R/document.R b/R/document.R index 6729c553..48a29580 100644 --- a/R/document.R +++ b/R/document.R @@ -282,14 +282,12 @@ parser_hooks <- list( if (!isTRUE(call$character.only)) { action$update(packages = as.character(call$package)) } - NULL }, "require" = function(expr, action) { call <- match.call(base::require, expr) if (!isTRUE(call$character.only)) { action$update(packages = as.character(call$package)) } - NULL }, "pacman::p_load" = function(expr, action) { fun <- if (requireNamespace("pacman")) pacman::p_load else @@ -301,7 +299,6 @@ parser_hooks <- list( packages <- vapply(call[["..."]], as.character, character(1L)) action$update(packages = packages) } - NULL }, "system.time" = function(expr, action) action$parse_args("expr"), "try" = function(expr, action) action$parse_args("expr"),