Merge pull request #719 from posit-dev/task/reparse-lazily

Generate srcrefs lazily

Author: lionel-

.zed/settings.json

@@ -0,0 +1,8 @@
+{
+    "languages": {
+        "R": {
+            "hard_tabs": false,
+            "tab_size": 4
+        }
+    }
+}

crates/ark/src/interface.rs

@@ -53,7 +53,6 @@ use crossbeam::channel::bounded;
 use crossbeam::channel::unbounded;
 use crossbeam::channel::Receiver;
 use crossbeam::channel::Sender;
-use crossbeam::select;
 use harp::command::r_command;
 use harp::command::r_home_setup;
 use harp::environment::r_ns_env;
@@ -103,6 +102,7 @@ use crate::lsp::main_loop::KernelNotification;
 use crate::lsp::main_loop::TokioUnboundedSender;
 use crate::lsp::state_handlers::ConsoleInputs;
 use crate::modules;
+use crate::modules::ARK_ENVS;
 use crate::plots::graphics_device;
 use crate::r_task;
 use crate::r_task::BoxFuture;
@@ -443,7 +443,7 @@ impl RMain {
             }
 
             // Register all hooks once all modules have been imported
-            let hook_result = RFunction::from(".ps.register_all_hooks").call();
+            let hook_result = RFunction::from("register_hooks").call_in(ARK_ENVS.positron_ns);
             if let Err(err) = hook_result {
                 log::error!("Error registering some hooks: {err:?}");
             }
@@ -753,6 +753,32 @@ impl RMain {
             }
         }
 
+        let mut select = crossbeam::channel::Select::new();
+
+        // Cloning is necessary to avoid a double mutable borrow error
+        let r_request_rx = self.r_request_rx.clone();
+        let stdin_reply_rx = self.stdin_reply_rx.clone();
+        let kernel_request_rx = self.kernel_request_rx.clone();
+        let tasks_interrupt_rx = self.tasks_interrupt_rx.clone();
+        let tasks_idle_rx = self.tasks_idle_rx.clone();
+
+        let r_request_index = select.recv(&r_request_rx);
+        let stdin_reply_index = select.recv(&stdin_reply_rx);
+        let kernel_request_index = select.recv(&kernel_request_rx);
+        let tasks_interrupt_index = select.recv(&tasks_interrupt_rx);
+
+        // Don't process idle tasks in browser prompts. We currently don't want
+        // idle tasks (e.g. for srcref generation) to run when the call stack is
+        // empty. We could make this configurable though if needed, i.e. some
+        // idle tasks would be able to run in the browser. Those should be sent
+        // to a dedicated channel that would always be included in the set of
+        // recv channels.
+        let tasks_idle_index = if info.browser {
+            None
+        } else {
+            Some(select.recv(&tasks_idle_rx))
+        };
+
         loop {
             // If an interrupt was signaled and we are in a user
             // request prompt, e.g. `readline()`, we need to propagate
@@ -777,17 +803,31 @@ impl RMain {
             // to be handled in a blocking way to ensure subscribers are
             // notified before the next incoming message is processed.
 
-            // First handle execute requests outside of `select!` to ensure they
-            // have priority. `select!` chooses at random.
-            if let Ok(req) = self.r_request_rx.try_recv() {
+            // First handle execute requests outside of `select` to ensure they
+            // have priority. `select` chooses at random.
+            if let Ok(req) = r_request_rx.try_recv() {
                 if let Some(input) = self.handle_execute_request(req, &info, buf, buflen) {
                     return input;
                 }
             }
 
-            select! {
-                // Wait for an execution request from the frontend.
-                recv(self.r_request_rx) -> req => {
+            let oper = select.select_timeout(Duration::from_millis(200));
+
+            let Ok(oper) = oper else {
+                // We hit a timeout. Process events because we need to
+                // pump the event loop while waiting for console input.
+                //
+                // Alternatively, we could try to figure out the file
+                // descriptors that R has open and select() on those for
+                // available data?
+                unsafe { Self::process_events() };
+                continue;
+            };
+
+            match oper.index() {
+                // We've got an execute request from the frontend
+                i if i == r_request_index => {
+                    let req = oper.recv(&r_request_rx);
                     let Ok(req) = req else {
                         // The channel is disconnected and empty
                         return ConsoleResult::Disconnected;
@@ -796,35 +836,33 @@ impl RMain {
                     if let Some(input) = self.handle_execute_request(req, &info, buf, buflen) {
                         return input;
                     }
-                }
+                },
 
                 // We've got a reply for readline
-                recv(self.stdin_reply_rx) -> reply => {
-                    return self.handle_input_reply(reply.unwrap(), buf, buflen);
-                }
+                i if i == stdin_reply_index => {
+                    let reply = oper.recv(&stdin_reply_rx).unwrap();
+                    return self.handle_input_reply(reply, buf, buflen);
+                },
 
                 // We've got a kernel request
-                recv(self.kernel_request_rx) -> req => {
-                    self.handle_kernel_request(req.unwrap(), &info);
-                }
+                i if i == kernel_request_index => {
+                    let req = oper.recv(&kernel_request_rx).unwrap();
+                    self.handle_kernel_request(req, &info);
+                },
 
-                // A task woke us up, start next loop tick to yield to it
-                recv(self.tasks_interrupt_rx) -> task => {
-                    self.handle_task_interrupt(task.unwrap());
-                }
-                recv(self.tasks_idle_rx) -> task => {
-                    self.handle_task(task.unwrap());
-                }
+                // An interrupt task woke us up
+                i if i == tasks_interrupt_index => {
+                    let task = oper.recv(&tasks_interrupt_rx).unwrap();
+                    self.handle_task_interrupt(task);
+                },
 
-                // Wait with a timeout. Necessary because we need to
-                // pump the event loop while waiting for console input.
-                //
-                // Alternatively, we could try to figure out the file
-                // descriptors that R has open and select() on those for
-                // available data?
-                default(Duration::from_millis(200)) => {
-                    unsafe { Self::process_events() };
-                }
+                // An idle task woke us up
+                i if Some(i) == tasks_idle_index => {
+                    let task = oper.recv(&tasks_idle_rx).unwrap();
+                    self.handle_task(task);
+                },
+
+                i => log::error!("Unexpected index in Select: {i}"),
             }
         }
     }
@@ -2056,7 +2094,10 @@ fn do_resource_namespaces() -> bool {
     let opt: Option<bool> = r_null_or_try_into(harp::get_option("ark.resource_namespaces"))
         .ok()
         .flatten();
-    opt.unwrap_or(true)
+
+    // By default we don't eagerly resource namespaces to avoid increased memory usage.
+    // https://github.com/posit-dev/positron/issues/5050
+    opt.unwrap_or(false)
 }
 
 /// Are we auto-printing?

crates/ark/src/modules/positron/hooks.R

@@ -1,70 +1,66 @@
 #
 # hooks.R
 #
-# Copyright (C) 2023-2024 Posit Software, PBC. All rights reserved.
+# Copyright (C) 2023-2025 Posit Software, PBC. All rights reserved.
 #
 #
 
-#' @export
-.ps.register_utils_hook <- function(name, hook, namespace = FALSE) {
-  if (namespace) {
-    hook_namespace <- hook
-  } else {
-    hook_namespace <- NULL
-  }
-
-  pkg_hook(
-    pkg = "utils",
-    name = name,
-    hook = hook,
-    hook_namespace = hook_namespace
-  )
+register_hooks <- function() {
+    rebind("utils", "View", .ps.view_data_frame, namespace = TRUE)
+    rebind("base", "debug", new_ark_debug(base::debug), namespace = TRUE)
+    rebind("base", "debugonce", new_ark_debug(base::debugonce), namespace = TRUE)
+    register_getHook_hook()
 }
 
-# Wrapper to contain the definition of all hooks we want to register
-#' @export
-.ps.register_all_hooks <- function() {
-  .ps.register_utils_hook("View", .ps.view_data_frame, namespace = TRUE)
-  register_getHook_hook()
+rebind <- function(pkg, name, value, namespace = FALSE) {
+    pkg_bind(
+        pkg = pkg,
+        name = name,
+        value = value
+    )
+
+    if (namespace) {
+        ns_bind(
+            pkg = pkg,
+            name = name,
+            value = value
+          )
+    }
 }
 
 #' Override a function within an attached package
 #'
 #' Assumes the package is attached, typically used for base packages like base or utils.
-#' - `hook` will replace the binding for unnamespaced calls.
-#' - `hook_namespace` will optionally also replace the binding for namespaced calls.
+#' - `pkg_bind()` replaces the binding in the package environment on the search
+#'   path for unnamespaced calls.
+#' - `ns_bind()` replaces the binding for namespaced calls.
 #'
 #' TODO: Will cause ark to fail to start if `option(defaultPackages = character())`
 #' or `R_DEFAULT_PACKAGES=NULL` are set! One idea is to register an `onAttach()`
 #' hook here and use that if the package is not loaded yet.
-pkg_hook <- function(pkg, name, hook, hook_namespace = NULL) {
-  env <- sprintf("package:%s", pkg)
-  env <- as.environment(env)
-
-  if (!exists(name, envir = env, mode = "function", inherits = FALSE)) {
-    msg <- sprintf("Can't register hook: function `%s::%s` not found.", pkg, name)
-    stop(msg, call. = FALSE)
-  }
-
-  # Replaces the unnamespaced, attached version of the function
-  hook_original <- env_bind_force(env, name, hook)
-
-  # If `hook_namespace` is provided, we try to replace the binding for `pkg::name` as well
-  if (is.null(hook_namespace)) {
-    hook_namespace_original <- NULL
-  } else {
+pkg_bind <- function(pkg, name, value) {
+    env <- sprintf("package:%s", pkg)
+    env <- as.environment(env)
+
+    if (!exists(name, envir = env, mode = "function", inherits = FALSE)) {
+        msg <- sprintf("Can't register hook: function `%s::%s` not found.", pkg, name)
+        stop(msg, call. = FALSE)
+    }
+
+    old <- env_bind_force(env, name, value)
+    invisible(old)
+}
+
+ns_bind <- function(pkg, name, value) {
     ns <- asNamespace(pkg)
+
     if (!exists(name, envir = ns, mode = "function", inherits = FALSE)) {
-      msg <- sprintf("Can't replace `%s` in the '%s' namespace.", name, pkg)
-      stop(msg, call. = FALSE)
+        msg <- sprintf("Can't replace `%s` in the '%s' namespace.", name, pkg)
+        stop(msg, call. = FALSE)
     }
-    hook_namespace_original <- env_bind_force(ns, name, hook_namespace)
-  }
 
-  invisible(list(
-    hook = hook_original,
-    hook_namespace = hook_namespace_original
-  ))
+    old <- env_bind_force(ns, name, value)
+    invisible(old)
 }
 
 # R only allows `onLoad` hooks for named packages, not for any package that

crates/ark/src/modules/positron/srcref.R

@@ -44,3 +44,42 @@ reparse_with_srcref <- function(x, name, uri, line) {
 zap_srcref <- function(x) {
     .ps.Call("ark_zap_srcref", x)
 }
+
+new_ark_debug <- function(fn) {
+    # Signature of `debug()` and `debugonce()`:
+    # function(fun, text = "", condition = NULL, signature = NULL)
+
+    body(fn) <- bquote({
+        local({
+            if (!.ps.internal(do_resource_namespaces(default = TRUE))) {
+                return() # from local()
+            }
+
+            pkgs <- loadedNamespaces()
+
+            # Give priority to the namespace of the debugged function
+            env <- topenv(environment(fun))
+            if (isNamespace(env)) {
+                pkgs <- unique(c(getNamespaceName(env), pkgs))
+            }
+
+            # Enable namespace resourcing for all future loaded namespaces and
+            # resource already loaded namespaces so we get virtual documents for
+            # step-debugging.
+            options(ark.resource_namespaces = TRUE)
+            .ps.internal(resource_namespaces(pkgs))
+        })
+
+        .(body(fn))
+    })
+
+    fn
+}
+
+do_resource_namespaces <- function(default) {
+    getOption("ark.resource_namespaces", default = default)
+}
+
+resource_namespaces <- function(pkgs) {
+    .ps.Call("ps_resource_namespaces", pkgs)
+}

crates/ark/src/srcref.rs

@@ -15,22 +15,33 @@ use crate::r_task;
 use crate::variables::variable::is_binding_fancy;
 use crate::variables::variable::plain_binding_force_with_rollback;
 
-#[tracing::instrument(level = "trace")]
-pub(crate) fn resource_loaded_namespaces() -> anyhow::Result<()> {
-    let loaded = RFunction::new("base", "loadedNamespaces").call()?;
-    let loaded: Vec<String> = loaded.try_into()?;
-
-    for pkg in loaded.into_iter() {
-        r_task::spawn_idle(|| async move {
+#[tracing::instrument(level = "trace", skip_all)]
+pub(crate) fn resource_namespaces(pkgs: Vec<String>) -> anyhow::Result<()> {
+    // Generate only one task and loop inside it to preserve the order of `pkgs`
+    r_task::spawn_idle(|| async move {
+        for pkg in pkgs.into_iter() {
             if let Err(err) = ns_populate_srcref(pkg.clone()).await {
                 log::error!("Can't populate srcrefs for `{pkg}`: {err:?}");
             }
-        });
-    }
+        }
+    });
 
     Ok(())
 }
 
+pub(crate) fn resource_loaded_namespaces() -> anyhow::Result<()> {
+    let loaded = RFunction::new("base", "loadedNamespaces").call()?;
+    let loaded: Vec<String> = loaded.try_into()?;
+    resource_namespaces(loaded)
+}
+
+#[harp::register]
+unsafe extern "C" fn ps_resource_namespaces(pkgs: SEXP) -> anyhow::Result<SEXP> {
+    let pkgs: Vec<String> = RObject::view(pkgs).try_into()?;
+    resource_namespaces(pkgs)?;
+    Ok(harp::r_null())
+}
+
 #[harp::register]
 unsafe extern "C" fn ps_ns_populate_srcref(ns_name: SEXP) -> anyhow::Result<SEXP> {
     let ns_name: String = RObject::view(ns_name).try_into()?;