feat(sess): resolve socket hover and completion without evaluating requests

sess/R/handlers.R

@@ -684,27 +684,17 @@ handle_listview_view <- function(view_id, index, path = list()) {
   workspace_show_view(location$data, location$title, state$owner %||% state$title)
 }
 
-handle_hover <- function(expr_str) {
-  tryCatch(
-    {
-      expr <- parse(text = expr_str, keep.source = FALSE)[[1]]
-      obj <- eval(expr, .GlobalEnv)
-      list(str = capture_str(obj))
-    },
-    error = function(e) NULL
-  )
-}
+# Hover and completion for the socket session. Requests are resolved with the
+# shared inspection parser and resolver (inspection.R) without evaluating the
+# request text. Hover and completion are shown without an explicit user
+# action, so any failure, including invalid requests and errors from methods
+# such as `.DollarNames()`, is reported to the editor as no result rather than
+# as an error with a console warning. Interrupts are not caught, so that the
+# dispatcher can reply that the request was interrupted.
+handle_hover <- function(expr_str) tryCatch(inspection_hover(expr_str), error = function(e) NULL)
 
 handle_complete <- function(expr_str, trigger = NULL) {
-  obj <- tryCatch(
-    {
-      expr <- parse(text = expr_str, keep.source = FALSE)[[1]]
-      eval(expr, .GlobalEnv)
-    },
-    error = function(e) NULL
-  )
-
-  completion_items(obj, trigger)
+  tryCatch(inspection_completion(expr_str, trigger), error = function(e) NULL)
 }
 
 # Completion items for the members of `obj` after the `trigger` operator.

sess/inst/tinytest/test-hover-completion.R

@@ -0,0 +1,118 @@
+# The socket session's hover and completion handlers use the inspection
+# resolver: requests they used to answer give the same results, and other
+# requests give no result without being evaluated.
+local({
+  hover <- sess:::handle_hover
+  complete <- sess:::handle_complete
+  capture_str <- sess:::capture_str
+  completion_items <- sess:::completion_items
+  prefix <- "sess_hover_"
+  masked <- c("$.sess_hover_s3", ".DollarNames.sess_hover_s3")
+  saved <- mget(intersect(masked, ls(.GlobalEnv, all.names = TRUE)), envir = .GlobalEnv)
+  on.exit({
+    rm(list = ls(.GlobalEnv, all.names = TRUE, pattern = paste0("^", prefix)), envir = .GlobalEnv)
+    rm(list = intersect(masked, ls(.GlobalEnv, all.names = TRUE)), envir = .GlobalEnv)
+    list2env(saved, envir = .GlobalEnv)
+  })
+  global <- function(name, value) assign(paste0(prefix, name), value, envir = .GlobalEnv)
+
+  global("df", data.frame(x = 1:3, y = letters[1:3]))
+  global("list", list(a = list(b = "leaf"), c = 1))
+  global("env", list2env(list(one = 1, two = "2")))
+  global("s3", structure(list(), class = "sess_hover_s3"))
+  assign("$.sess_hover_s3", function(x, name) paste("member", name), envir = .GlobalEnv)
+  assign(".DollarNames.sess_hover_s3", function(x, pattern = "") c("p", "q"), envir = .GlobalEnv)
+  methods::setClass(
+    "sess_hover_s4",
+    representation(items = "list", label = "character"),
+    where = .GlobalEnv
+  )
+  on.exit(methods::removeClass("sess_hover_s4", where = .GlobalEnv), add = TRUE)
+  global("s4", methods::new("sess_hover_s4", items = list(a = 1), label = "s4"))
+
+  # These requests previously went through eval(); the results are unchanged.
+  previous <- function(expr) eval(parse(text = expr, keep.source = FALSE)[[1L]], .GlobalEnv)
+  same <- function(expr, trigger = NULL) {
+    expect_identical(hover(expr), list(str = capture_str(previous(expr))), info = expr)
+    if (!is.null(trigger)) {
+      expect_identical(
+        complete(expr, trigger),
+        completion_items(previous(expr), trigger),
+        info = expr
+      )
+    }
+  }
+  same("sess_hover_df", "$")
+  same("sess_hover_df$x")
+  same("sess_hover_list", "$")
+  same("sess_hover_list $ a", "$")
+  same("sess_hover_list$a$b")
+  same("sess_hover_env", "$")
+  same("sess_hover_env$two")
+  same("sess_hover_s3", "$")
+  same("sess_hover_s3$p")
+  same("sess_hover_s4", "@")
+  same("sess_hover_s4@items", "$")
+  same("pi")
+  for (constant in c("1", "1.5", "1L", "TRUE", "NULL", "NA", "0x10", "\"text\"")) {
+    same(constant)
+  }
+  if (requireNamespace("data.table", quietly = TRUE)) {
+    global("dt", data.table::data.table(id = 1:2, value = c("a", "b")))
+    same("sess_hover_dt", "$")
+    same("sess_hover_dt$value")
+  }
+  if (requireNamespace("R6", quietly = TRUE)) {
+    Counter <- R6::R6Class(
+      "SessHoverCounter",
+      public = list(count = 1, add = function() invisible(self)),
+      active = list(double = function() self$count * 2)
+    )
+    global("r6", Counter$new())
+    same("sess_hover_r6", "$")
+    same("sess_hover_r6$double")
+  }
+
+  # Missing names and members give no result, as before.
+  expect_null(hover("sess_hover_missing"))
+  expect_null(complete("sess_hover_missing", "$"))
+  expect_null(complete("sess_hover_df", NULL))
+
+  # Other requests give no result, and the request text is not evaluated.
+  # Control flow is rejected: hovering `x` in `repeat x <- x + 1` sends
+  # `repeat x`, which used to loop forever. `repeat break` fails instead of
+  # hanging if it is evaluated again.
+  rejected <- c(
+    "repeat break",
+    "sess_hover_called <- TRUE",
+    "assign(\"sess_hover_called\", TRUE)",
+    "sess_hover_list; sess_hover_called <- TRUE",
+    "(sess_hover_called <- TRUE)$a",
+    "sess_hover_list$a("
+  )
+  for (expr in rejected) {
+    expect_null(hover(expr), info = expr)
+    expect_null(complete(expr, "$"), info = expr)
+  }
+  expect_false(exists("sess_hover_called", envir = .GlobalEnv))
+  expect_null(complete("get(\"sess_hover_list\")", "$"))
+
+  # Errors from the session's own code give no result; interrupts propagate.
+  makeActiveBinding("sess_hover_failing", function() stop("binding failed"), .GlobalEnv)
+  expect_null(hover("sess_hover_failing"))
+  assign(".DollarNames.sess_hover_s3", function(x, pattern = "") stop("names failed"), .GlobalEnv)
+  expect_null(complete("sess_hover_s3", "$"))
+  makeActiveBinding(
+    "sess_hover_interrupted",
+    function() signalCondition(structure(class = c("interrupt", "condition"), list())),
+    .GlobalEnv
+  )
+  expect_identical(
+    tryCatch(hover("sess_hover_interrupted"), interrupt = function(e) "interrupted"),
+    "interrupted"
+  )
+  expect_identical(
+    tryCatch(complete("sess_hover_interrupted", "$"), interrupt = function(e) "interrupted"),
+    "interrupted"
+  )
+})

src/test/node/interactiveRuntime.test.ts

@@ -411,12 +411,16 @@ sess::display("display-${cell}", "text/plain"); cat("stderr-${cell}\\n", file=st
     test('interrupts a slow inspection and continues serving R requests', async () => {
         for (const afterTimeout of [false, true]) {
             const marker = `inspection-started-${String(afterTimeout)}`;
+            // Hover does not evaluate its request, so make reading the binding slow.
+            const binding = `slow_inspection_${String(afterTimeout)}`;
+            const setup = await submit(
+                `makeActiveBinding("${binding}", function() { cat("${marker}"); Sys.sleep(30); 42 }, .GlobalEnv)`,
+            );
+            assert.strictEqual((await finished(setup)).state, 'success');
             const inspection = client
                 .request('inspect', {
                     method: 'hover',
-                    params: {
-                        expr: `local({ cat("${marker}"); Sys.sleep(30); 42 })`,
-                    },
+                    params: { expr: binding },
                 })
                 .then(
                     () => '',