Fix argument completion after member access

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.
 

R/completion.R

@@ -427,7 +427,7 @@
                 data = list(
                     type = "function",
                     package = nsname
             ))
         })
     } else {
         completions <- lapply(functs, function(object) {
@@ -439,7 +439,7 @@
                 data = list(
                     type = "function",
                     package = nsname
             ))
             if (!identical(inserted, object)) item$insertText <- inserted
             item
         })
@@ -471,7 +471,7 @@
                     data = list(
                         type = "function",
                         package = nsname
                 ))
             } else {
                 inserted <- quote_completion_name(object)
                 item <- list(label = object,
@@ -481,7 +481,7 @@
                     data = list(
                         type = "function",
                         package = nsname
                 ))
                 if (!identical(inserted, object)) item$insertText <- inserted
             }
             idx <- idx + 1L
@@ -556,7 +556,7 @@
                 value
             }
         }
         candidates[[length(candidates) + 1L]] <<- list(
             label = objects,
             kind = recycle(kind),
             detail = recycle(detail),
@@ -1296,7 +1296,7 @@
     nmax <- lsp_settings$get("max_completions")
     if (isTRUE(lsp_settings$get("member_completion"))) {
         members <- tryCatch(member_completion(uri, workspace, document, point,
             snippet_support, nmax), error = function(e) {
                 logger$info("static member completion:", e)
                 NULL
             })
@@ -1339,7 +1339,16 @@
 
     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)
             }

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},

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");

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__ */

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))))
+})

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))