diff --git a/NEWS.md b/NEWS.md index 936bab73..43bf8dc0 100644 --- a/NEWS.md +++ b/NEWS.md @@ -1,5 +1,9 @@ # languageserver 0.3.20 +- Avoid suggesting function arguments after `$` or `@` inside a call when + member or slot completion cannot resolve the receiver, including names + enclosed in backticks, single quotes or double quotes. + - Keep signature help visible while editing quoted and raw string arguments, including strings spanning multiple lines. diff --git a/R/completion.R b/R/completion.R index 665ccb8b..00508dec 100644 --- a/R/completion.R +++ b/R/completion.R @@ -1339,7 +1339,16 @@ completion_reply <- function(id, uri, workspace, document, point, capabilities) if (token_result$accessor == "") { call_result <- document$detect_call(point) + # An unresolved member access must not fall back to arguments of its + # enclosing call. Check before a quoted name so that operators within + # the name do not hide an accessor on the preceding line. + member_point <- if (isTRUE(token_result$quoted)) token_result$range$start else point if (!is.null(call_result$opening)) { + string <- .Call("scan_closed_string_completion_c", document$line0(point$row), + as.integer(point$col), PACKAGE = "languageserver") + if (!is.null(string)) member_point <- list(row = point$row, col = string$start) + } + if (!is.null(call_result$opening) && is.null(member_cursor(document, member_point))) { constructor_args <- if (isTRUE(lsp_settings$get("member_completion"))) { member_constructor_arguments(uri, workspace, document, point, token, call_result) } diff --git a/src/languageserver.c b/src/languageserver.c index 0182cae4..5ffd64e1 100644 --- a/src/languageserver.c +++ b/src/languageserver.c @@ -57,6 +57,7 @@ static const R_CallMethodDef CallEntries[] = { {"code_point_from_unit_c", (DL_FUNC) &code_point_from_unit_c, 2}, {"scan_token_c", (DL_FUNC) &scan_token_c, 3}, {"scan_backtick_completion_c", (DL_FUNC) &scan_backtick_completion_c, 2}, + {"scan_closed_string_completion_c", (DL_FUNC) &scan_closed_string_completion_c, 2}, {"match_with_c", (DL_FUNC) &match_with_c, 2}, {"fuzzy_find_c", (DL_FUNC) &fuzzy_find_c, 2}, {"completion_parse_index_c", (DL_FUNC) &completion_parse_index_c, 8}, diff --git a/src/token.c b/src/token.c index ee2185b3..c816ca4a 100644 --- a/src/token.c +++ b/src/token.c @@ -105,7 +105,11 @@ static SEXP make_str(const char *text, int start, int len) { } /* Keep source offsets separate from the decoded name used for completion. */ -SEXP scan_backtick_completion_c(SEXP line, SEXP col) { +static int completion_quote_state(const fsm_state *state, int strings) { + return strings ? state->single_quoted || state->double_quoted : state->backticked; +} + +static SEXP scan_quoted_completion(SEXP line, SEXP col, int strings) { if (!Rf_isString(line) || Rf_length(line) != 1) { Rf_error("line must be a single character string"); } @@ -115,7 +119,12 @@ SEXP scan_backtick_completion_c(SEXP line, SEXP col) { const char *text = Rf_translateCharUTF8(STRING_ELT(line, 0)); int len = (int) strlen(text); int cursor = byte_index_for_cp(text, len, INTEGER(col)[0]); - if (memchr(text, '`', cursor) == NULL) return R_NilValue; + if (strings) { + if (memchr(text, '\'', cursor) == NULL && + memchr(text, '"', cursor) == NULL) return R_NilValue; + } else if (memchr(text, '`', cursor) == NULL) { + return R_NilValue; + } fsm_state state; fsm_initialize(&state); @@ -126,22 +135,25 @@ SEXP scan_backtick_completion_c(SEXP line, SEXP col) { !state.backticked && !state.escaped && text[i] == '#') { return R_NilValue; } - int backticked = state.backticked; + int quoted = completion_quote_state(&state, strings); fsm_feed(&state, text[i]); - if (!backticked && state.backticked) start = i; - if (backticked && !state.backticked) closing = i; + int next_quoted = completion_quote_state(&state, strings); + if (!quoted && next_quoted) start = i; + if (quoted && !next_quoted) closing = i; } - if (start < 0 || (!state.backticked && closing + 1 != cursor)) { + int quoted = completion_quote_state(&state, strings); + /* Strings only need a context check immediately after their closing quote. */ + if (start < 0 || (strings && quoted) || (!quoted && closing + 1 != cursor)) { return R_NilValue; } - int prefix_end = state.backticked ? cursor : closing; + int prefix_end = quoted ? cursor : closing; int end = cursor; - if (state.backticked) { + if (quoted) { end = len; for (int i = cursor; i < len; i++) { fsm_feed(&state, text[i]); - if (!state.backticked) { + if (!completion_quote_state(&state, strings)) { end = i + 1; break; } @@ -168,6 +180,14 @@ SEXP scan_backtick_completion_c(SEXP line, SEXP col) { return out; } +SEXP scan_backtick_completion_c(SEXP line, SEXP col) { + return scan_quoted_completion(line, col, 0); +} + +SEXP scan_closed_string_completion_c(SEXP line, SEXP col) { + return scan_quoted_completion(line, col, 1); +} + SEXP scan_token_c(SEXP line, SEXP col, SEXP forward) { if (!Rf_isString(line) || Rf_length(line) != 1) { Rf_error("line must be a single character string"); diff --git a/src/token.h b/src/token.h index a3382c7c..bad98683 100644 --- a/src/token.h +++ b/src/token.h @@ -7,5 +7,6 @@ /* Scan a line for tokens around a column index (ASCII fast-path). */ SEXP scan_token_c(SEXP line, SEXP col, SEXP forward); SEXP scan_backtick_completion_c(SEXP line, SEXP col); +SEXP scan_closed_string_completion_c(SEXP line, SEXP col); #endif /* end of include guard: TOKEN_H__ */ diff --git a/tests/testthat/test-completion-member-context.R b/tests/testthat/test-completion-member-context.R new file mode 100644 index 00000000..2560a36b --- /dev/null +++ b/tests/testthat/test-completion-member-context.R @@ -0,0 +1,190 @@ +member_context_fixture <- function(lines) { + fixture <- provider_fixture(lines, formals_resolver = function(...) { + list(`a$pos` = NULL, `a@pos` = NULL, pos = 1, mode = quote(c("auto", "manual"))) + }) + fixture$workspace$guess_namespace <- function(...) "base" + fixture$workspace$loaded_packages <- character() + fixture$workspace$imported_objects <- collections::dict() + fixture$workspace$get_namespace <- function(...) NULL + fixture +} + +member_context_items <- function(fixture, row, col) { + completion_reply(1L, fixture$uri, fixture$workspace, fixture$document, + list(row = row, col = col), list())$result$items +} + +test_that("Member access inside calls does not offer arguments or argument values", { + old <- lsp_settings$get("member_completion") + withr::defer(lsp_settings$set("member_completion", old)) + cases <- list( + list(lines = "get($)", row = 0L, col = 5L), + list(lines = "get(@)", row = 0L, col = 5L), + list(lines = "get(unknown$)", row = 0L, col = 12L), + list(lines = "get(unknown@)", row = 0L, col = 12L), + list(lines = "get(unknown$pos)", row = 0L, col = 15L), + list(lines = "get(unknown@pos)", row = 0L, col = 15L), + list(lines = "get(unknown$`pos`)", row = 0L, col = 17L), + list(lines = "get(unknown@`pos`)", row = 0L, col = 17L), + list(lines = "get(unknown $ pos)", row = 0L, col = 18L), + list(lines = "get(unknown$\tpos)", row = 0L, col = 16L), + list(lines = c("get(unknown$", "# comment", " pos)"), row = 2L, col = 5L), + list(lines = c("get(unknown$", " `pos`)"), row = 1L, col = 7L), + list(lines = c("get(unknown@", " `pos`)"), row = 1L, col = 7L), + list(lines = c("get(unknown$", " `a$po`)"), row = 1L, col = 7L), + list(lines = c("get(unknown@", " `a@po`)"), row = 1L, col = 7L) + ) + for (enabled in c(TRUE, FALSE)) { + lsp_settings$set("member_completion", enabled) + for (case in cases) { + fixture <- member_context_fixture(case$lines) + items <- member_context_items(fixture, case$row, case$col) + types <- vapply(items, function(item) { + if (is.null(item$data$type)) "" else item$data$type + }, "") + expect_false(any(types %in% c("parameter", "argument_value", "s3_parameter")), + info = paste(enabled, paste(case$lines, collapse = " "))) + expect_false(any(vapply(items, function(item) !is.null(item$data$parameter), logical(1L))), + info = paste(enabled, paste(case$lines, collapse = " "))) + } + } +}) + +test_that("Ordinary and quoted argument prefixes still complete", { + for (case in list( + list(text = "get()", col = 4L, label = "pos"), + list(text = "get(pos)", col = 7L, label = "pos"), + list(text = "get(`a$po`)", col = 9L, label = "a$pos"), + list(text = "get(`a@po`)", col = 9L, label = "a@pos") + )) { + fixture <- member_context_fixture(case$text) + items <- member_context_items(fixture, 0L, case$col) + parameters <- Filter(function(item) identical(item$data$type, "parameter"), items) + expect_true(case$label %in% vapply(parameters, `[[`, "", "label"), info = case$text) + } + fixture <- member_context_fixture("get()") + items <- member_context_items(fixture, 0L, 4L) + values <- Filter(function(item) identical(item$data$type, "argument_value"), items) + expect_setequal(vapply(values, `[[`, "", "label"), c("auto", "manual")) +}) + +test_that("String-quoted member names do not offer enclosing call arguments", { + old <- lsp_settings$get("member_completion") + withr::defer(lsp_settings$set("member_completion", old)) + calls <- c( + 'get(unknown$"pos")', "get(unknown$'pos')", + 'get(unknown@"pos")', "get(unknown@'pos')", + 'get(unknown $ "pos")', 'get(unknown$"a\\\"pos")', + "get(unknown$'a\\'pos')", 'get(unknown$\n# comment\n "a$pos")', + "get(unknown@\n 'a@pos')", 'get("unknown$pos', "get('unknown@pos" + ) + for (enabled in c(TRUE, FALSE)) { + lsp_settings$set("member_completion", enabled) + for (call in calls) { + lines <- strsplit(call, "\n", fixed = TRUE)[[1L]] + fixture <- member_context_fixture(lines) + col <- nchar(tail(lines, 1L)) - as.integer(endsWith(call, ")")) + items <- member_context_items(fixture, length(lines) - 1L, col) + expect_false(any(vapply(items, function(item) { + isTRUE(item$data$type %in% c("parameter", "argument_value", "s3_parameter")) || + !is.null(item$data$parameter) + }, logical(1L))), info = paste(enabled, call)) + } + for (call in c('get("unknown$pos")', "get('unknown@pos')")) { + fixture <- member_context_fixture(call) + items <- member_context_items(fixture, 0L, nchar(call) - 1L) + expect_true(any(vapply(items, function(item) identical(item$data$type, "parameter"), logical(1L))), + info = paste(enabled, call)) + } + } +}) + +test_that("Known members and slots still complete inside calls", { + cases <- list( + list(lines = c("x <- list(alpha = 1)", "get(x$)"), col = 6L, label = "alpha"), + list(lines = c("setClass('Widget', slots=c(value='numeric'))", "x <- new('Widget')", "get(x@)"), + col = 6L, label = "value") + ) + for (case in cases) { + fixture <- member_context_fixture(case$lines) + items <- member_context_items(fixture, length(case$lines) - 1L, case$col) + expect_identical(vapply(items, `[[`, "", "label"), case$label) + expect_identical(items[[1L]]$data$type, "member") + } +}) + +test_that("Member triggers after edits do not offer enclosing call arguments", { + skip_on_cran() + root <- withr::local_tempdir() + path <- file.path(root, "completion.R") + uri <- path_to_uri(path) + client <- language_client(working_dir = root) + did_open(client, path, text = "get()") + + ordinary <- respond_completion(client, path, c(0L, 4L), + retry_when = function(result) { + !any(vapply(result$items, function(item) { + identical(item$data$type, "parameter") && identical(item$label, "pos") + }, logical(1L))) + }) + expect_true(any(vapply(ordinary$items, function(item) identical(item$label, "pos"), logical(1L)))) + notify(client, "workspace/didChangeConfiguration", list(settings = list(parse_delay = 0.5))) + + for (i in seq_along(c("$", "@"))) { + operator <- c("$", "@")[[i]] + start <- list(line = 0L, character = 4L) + after <- list(line = 0L, character = 5L) + notify(client, "textDocument/didChange", list( + textDocument = list(uri = uri, version = 2L * i), + contentChanges = list(list(range = list(start = start, end = start), text = operator)) + )) + result <- respond(client, "textDocument/completion", list( + textDocument = list(uri = uri), position = after, + context = list(triggerKind = 2L, triggerCharacter = operator) + ), retry = FALSE) + expect_false(any(vapply(result$items, function(item) { + identical(item$data$type, "parameter") || + identical(item$data$type, "argument_value") || !is.null(item$data$parameter) + }, logical(1L))), info = operator) + notify(client, "textDocument/didChange", list( + textDocument = list(uri = uri, version = 2L * i + 1L), + contentChanges = list(list(range = list(start = start, end = after), text = "")) + )) + } + + quoted_calls <- c('get(unknown$"pos")', "get(unknown$'pos')", + 'get(unknown@"pos")', "get(unknown@'pos')") + for (i in seq_along(quoted_calls)) { + call <- quoted_calls[[i]] + notify(client, "textDocument/didChange", list( + textDocument = list(uri = uri, version = 5L + i), + contentChanges = list(list(text = call)) + )) + result <- respond(client, "textDocument/completion", list( + textDocument = list(uri = uri), + position = list(line = 0L, character = nchar(call) - 1L), + context = list(triggerKind = 1L) + ), retry = FALSE) + expect_false(any(vapply(result$items, function(item) { + isTRUE(item$data$type %in% c("parameter", "argument_value", "s3_parameter")) || + !is.null(item$data$parameter) + }, logical(1L))), info = call) + } + + member_path <- file.path(root, "member.R") + member_uri <- path_to_uri(member_path) + did_open(client, member_path, text = "x <- list(alpha = 1)\nget(x)") + start <- list(line = 1L, character = 5L) + after <- list(line = 1L, character = 6L) + notify(client, "textDocument/didChange", list( + textDocument = list(uri = member_uri, version = 2L), + contentChanges = list(list(range = list(start = start, end = start), text = "$")) + )) + result <- respond(client, "textDocument/completion", list( + textDocument = list(uri = member_uri), position = after, + context = list(triggerKind = 2L, triggerCharacter = "$") + ), retry = FALSE) + expect_true(any(vapply(result$items, function(item) { + identical(item$label, "alpha") && identical(item$data$type, "member") + }, logical(1L)))) +}) diff --git a/tests/testthat/test-native-utilities.R b/tests/testthat/test-native-utilities.R index 8f652b34..7b8f41a3 100644 --- a/tests/testthat/test-native-utilities.R +++ b/tests/testthat/test-native-utilities.R @@ -141,6 +141,27 @@ test_that("Backtick completion scanning respects escapes and source offsets", { PACKAGE = "languageserver"), "col must be a single integer") }) +test_that("Closed-string scanning locates quoted names without confusing their contents", { + scan <- function(text, col = nchar(text)) { + .Call("scan_closed_string_completion_c", text, as.integer(col), + PACKAGE = "languageserver") + } + expect_identical(scan('x$"pos"'), list(token = "pos", start = 2L, end = 7L)) + expect_identical(scan("x$'pos'"), list(token = "pos", start = 2L, end = 7L)) + expect_null(scan('x$"pos"', 5L)) + expect_identical(scan('x$"a\\\"pos"'), list(token = 'a\\"pos', start = 2L, end = 10L)) + expect_identical(scan("x$'a\\'pos'"), list(token = "a\\'pos", start = 2L, end = 10L)) + text <- paste0('"', intToUtf8(0x1f680), '"; x$"', intToUtf8(0xe9), '"') + expect_identical(scan(text), list(token = intToUtf8(0xe9), start = 7L, end = 10L)) + for (text in c("plain", 'x$"closed" + ', '# x$"pos"', '`x$"pos"`')) { + expect_null(scan(text)) + } + expect_error(.Call("scan_closed_string_completion_c", c("x", "y"), 0L, + PACKAGE = "languageserver"), "line must be a single character string") + expect_error(.Call("scan_closed_string_completion_c", "x", 0, + PACKAGE = "languageserver"), "col must be a single integer") +}) + test_that("UTF-16 conversion handles every UTF-8 width and boundary", { text <- intToUtf8(c(0x61, 0xe9, 0x4f62, 0x10400, 0x7a))