diff --git a/AGENTS.md b/AGENTS.md index abdedd276..ed4a91dbb 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -168,8 +168,6 @@ If changes are needed in these files, that must happen in the separate Positron - In tests, prefer exact assertions over fuzzy ones. Use `assert_eq!(stack[0].name, "foo()")` rather than `assert!(names.contains(&"foo()"))` when ordering and completeness matter. -- In tests, if you set global state (R options, env vars), clean it up _before_ assertions so it doesn't leak when a test fails. - - When you extract code in a function (or move things around) that function goes _below_ the calling function. A general goal is to be able to read linearly from top to bottom with the relevant context and main logic first. The code should be organised like a call stack. Of course that's not always possible, use best judgement to produce the clearest code organization. - Keep the main logic as unnested as possible. Favour Rust's `let ... else` syntax to return early or continue a loop in the `else` clause, over `if let`. diff --git a/crates/ark/src/variables/r_variables.rs b/crates/ark/src/variables/r_variables.rs index 0ffa6a21e..a7bda6c9a 100644 --- a/crates/ark/src/variables/r_variables.rs +++ b/crates/ark/src/variables/r_variables.rs @@ -41,6 +41,8 @@ use libr::ENVSXP; use stdext::result::ResultExt; use stdext::spawn; +use crate::console; +use crate::console::Console; use crate::data_explorer::r_data_explorer::DataObjectEnvInfo; use crate::data_explorer::r_data_explorer::RDataExplorer; use crate::data_explorer::summary_stats::summary_stats; @@ -95,6 +97,11 @@ pub struct RVariables { /// Whether we are currently showing the .Last.value variable in the Variables /// pane. showing_last_value: bool, + + /// Whether we need to send an initial `Refresh` event to the frontend, + /// which is required for frontend initialization. Set on construction, + /// cleared after the first `update()` call. + needs_initial_refresh: bool, } impl RVariables { @@ -154,6 +161,7 @@ impl RVariables { version: 0, show_last_value, showing_last_value: false, + needs_initial_refresh: true, }; environment.execution_thread(); }); @@ -162,24 +170,23 @@ impl RVariables { pub fn execution_thread(mut self) { let (prompt_signal_tx, prompt_signal_rx) = unbounded::<()>(); + // Schedule the initial environment scan as an idle task. If R is + // already idle this runs promptly, otherwise it runs at next idle. + // This and the prompt-driven `update()` may arrive in either order, + // which is safe: the first sends a `Refresh`, the second is a no-op. + let initial_signal_tx = prompt_signal_tx.clone(); + r_task::spawn_idle(move |_| async move { + initial_signal_tx.send(()).log_err(); + }); + // Register a handler for console prompt events let listen_id = EVENTS.console_prompt.listen({ move |_| { log::info!("Got console prompt signal."); - prompt_signal_tx.send(()).unwrap(); + prompt_signal_tx.send(()).log_err(); } }); - // Perform the initial environment scan and deliver to the frontend - let variables = self.list_variables(); - let length = variables.len() as i64; - let event = VariablesFrontendEvent::Refresh(RefreshParams { - variables, - length, - version: self.version as i64, - }); - self.send_event(event); - // Flag initially set to false, but set to true if the user closes the // channel (i.e. the frontend is closed) let mut user_initiated_close = false; @@ -249,13 +256,7 @@ impl RVariables { fn list_variables(&mut self) -> Vec { let mut variables: Vec = vec![]; r_task(|| { - self.update_bindings(self.bindings()); - - // If the special .Last.value variable is enabled, add it to the - // list. This is a special R value that doesn't have its own - // binding. if let Some(last_value) = self.last_value() { - self.showing_last_value = true; variables.push(last_value.var()); } @@ -566,10 +567,47 @@ impl RVariables { #[tracing::instrument(level = "trace", skip_all)] fn update(&mut self) { - let mut assigned: Vec = vec![]; - let mut removed: Vec = vec![]; - r_task(|| { + let needs_initial_refresh = self.needs_initial_refresh; + self.needs_initial_refresh = false; + + let env_changed = self.update_env(); + + // Send a full refresh if the environment changed (e.g. entered + // or exited debug mode) or on the first update after startup. + if env_changed || needs_initial_refresh { + self.update_bindings(self.bindings()); + + let mut variables: Vec = vec![]; + + if let (Some(var), _) = self.track_last_value() { + variables.push(var); + } + + for binding in self.current_bindings.get() { + variables.push(PositronVariable::new(binding).var()); + } + + let length = variables.len() as i64; + self.send_event(VariablesFrontendEvent::Refresh(RefreshParams { + variables, + length, + version: self.version as i64, + })); + + return; + } + + // Incremental update: diff old vs new bindings + let mut assigned: Vec = vec![]; + let mut removed: Vec = vec![]; + + match self.track_last_value() { + (Some(var), _) => assigned.push(var), + (None, true) => removed.push(".Last.value".to_string()), + (None, false) => {}, + } + let new_bindings = self.bindings(); let mut old_iter = self.current_bindings.get().iter(); @@ -578,18 +616,6 @@ impl RVariables { let mut new_iter = new_bindings.get().iter(); let mut new_next = new_iter.next(); - // Track the last value if the user has requested it. Treat this - // value as assigned every time we update the Variables list. - if let Some(last_value) = self.last_value() { - self.showing_last_value = true; - assigned.push(last_value.var()); - } else if self.showing_last_value { - // If we are no longer showing the last value, remove it from - // the list of assigned variables - self.showing_last_value = false; - removed.push(".Last.value".to_string()); - } - loop { match (old_next, new_next) { // nothing more to do @@ -644,24 +670,50 @@ impl RVariables { } } - // Only update the bindings (and the version) if anything changed if assigned.len() > 0 || removed.len() > 0 { self.update_bindings(new_bindings); + self.send_event(VariablesFrontendEvent::Update(UpdateParams { + assigned, + removed, + unevaluated: vec![], + version: self.version as i64, + })); } }); + } + + // SAFETY: The following methods must be called in an `r_task()` - if assigned.len() > 0 || removed.len() > 0 { - let event = VariablesFrontendEvent::Update(UpdateParams { - assigned, - removed, - unevaluated: vec![], - version: self.version as i64, - }); - self.send_event(event); + /// Updates `self.showing_last_value` and returns the last value variable + /// if it should be shown, along with whether it was previously shown. + fn track_last_value(&mut self) -> (Option, bool) { + let was_showing = self.showing_last_value; + + if let Some(last_value) = self.last_value() { + self.showing_last_value = true; + (Some(last_value.var()), was_showing) + } else { + self.showing_last_value = false; + (None, was_showing) } } - // SAFETY: The following methods must be called in an `r_task()` + /// Updates `self.env` to the current effective environment if it changed + /// (e.g. entering or exiting debug mode). Returns `true` if switched. + fn update_env(&mut self) -> bool { + if !Console::is_initialized() { + return false; + } + + let env = console::selected_env(); + + if env.sexp == self.env.get().sexp { + return false; + } + + self.env = RThreadSafe::new(env); + true + } fn bindings(&self) -> RThreadSafe> { let env = self.env.get().clone(); diff --git a/crates/ark/tests/variables.rs b/crates/ark/tests/variables.rs index e8617952c..b13a46e62 100644 --- a/crates/ark/tests/variables.rs +++ b/crates/ark/tests/variables.rs @@ -112,6 +112,22 @@ fn test_variables_list() { Rf_defineVar(sym, Rf_ScalarInteger(42), *test_env); }); + // Simulate a prompt signal so `update()` picks up the new variable + EVENTS.console_prompt.emit(()); + let msg = iopub_rx.recv_comm_msg(); + let data = match msg { + CommMsg::Data(data) => data, + _ => panic!("Expected data message"), + }; + let evt: VariablesFrontendEvent = serde_json::from_value(data).unwrap(); + match evt { + VariablesFrontendEvent::Update(params) => { + assert_eq!(params.assigned.len(), 1); + assert_eq!(params.assigned[0].display_name, "everything"); + }, + _ => panic!("Expected update event"), + } + // Request a list of variables let request = VariablesBackendRequest::List; let data = serde_json::to_value(request).unwrap(); diff --git a/crates/ark/tests/variables_debug.rs b/crates/ark/tests/variables_debug.rs new file mode 100644 index 000000000..e5ffc7c07 --- /dev/null +++ b/crates/ark/tests/variables_debug.rs @@ -0,0 +1,135 @@ +// +// variables_debug.rs +// +// Copyright (C) 2026 Posit Software, PBC. All rights reserved. +// +// + +use amalthea::fixtures::dummy_frontend::ExecuteRequestOptions; +use ark_test::DummyArkFrontend; + +/// When entering debug mode (browser), the variables pane should switch to +/// showing the debug environment's bindings. When exiting, it should switch +/// back to the global environment. +#[test] +fn test_variables_pane_shows_debug_env() { + let frontend = DummyArkFrontend::lock(); + + // Set up a global variable before opening the variables comm + frontend.execute_request_invisibly("test_gv <- 'hello'"); + + // Open the variables comm and receive the initial Refresh + let initial = frontend.open_variables_comm(); + let names: Vec<&str> = initial + .variables + .iter() + .map(|v| v.display_name.as_str()) + .collect(); + assert!(names.contains(&"test_gv")); + + // Enter browser with a local variable + frontend.send_execute_request( + "local({ debug_var <- 42; browser() })", + ExecuteRequestOptions::default(), + ); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.assert_stream_stdout_contains("Called from:"); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // The variables pane should have sent a Refresh with the debug env + let refresh = frontend.recv_variables_refresh(); + let names: Vec<&str> = refresh + .variables + .iter() + .map(|v| v.display_name.as_str()) + .collect(); + assert_eq!(names, vec!["debug_var"]); + + // Quit the debugger + frontend.send_execute_request("Q", ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // Should be back to showing the global environment + let refresh = frontend.recv_variables_refresh(); + let names: Vec<&str> = refresh + .variables + .iter() + .map(|v| v.display_name.as_str()) + .collect(); + assert!(names.contains(&"test_gv")); + assert!(!names.contains(&"debug_var")); + + // Clean up + frontend.send_execute_request("rm(test_gv)", ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // Consume the Update triggered by the rm() + let _update = frontend.recv_variables_update(); +} + +/// When entering debug via a function call, the variables pane should show +/// the function's environment with its arguments. +#[test] +fn test_variables_pane_shows_function_debug_env() { + let frontend = DummyArkFrontend::lock(); + + // Set up a global variable before opening the variables comm + frontend.execute_request_invisibly("test_gv2 <- 'world'"); + + let _initial = frontend.open_variables_comm(); + + // Define a function and call it so browser() triggers inside + frontend.send_execute_request( + "f <- function(my_arg) { browser(); my_arg }; f(99)", + ExecuteRequestOptions::default(), + ); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.assert_stream_stdout_contains("Called from: f(99)"); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // The debug env should show `my_arg` + let refresh = frontend.recv_variables_refresh(); + let names: Vec<&str> = refresh + .variables + .iter() + .map(|v| v.display_name.as_str()) + .collect(); + assert_eq!(names, vec!["my_arg"]); + + // Quit the debugger + frontend.send_execute_request("Q", ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // Should be back to showing the global environment + let refresh = frontend.recv_variables_refresh(); + let names: Vec<&str> = refresh + .variables + .iter() + .map(|v| v.display_name.as_str()) + .collect(); + assert!(names.contains(&"test_gv2")); + assert!(!names.contains(&"my_arg")); + + // Clean up + frontend.send_execute_request("rm(test_gv2, f)", ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + frontend.recv_iopub_execute_input(); + frontend.recv_iopub_idle(); + frontend.recv_shell_execute_reply(); + + // Consume the Update triggered by the rm() + let _update = frontend.recv_variables_update(); +} diff --git a/crates/ark_test/src/dummy_frontend.rs b/crates/ark_test/src/dummy_frontend.rs index bb08afada..f1bb53f25 100644 --- a/crates/ark_test/src/dummy_frontend.rs +++ b/crates/ark_test/src/dummy_frontend.rs @@ -12,6 +12,9 @@ use std::sync::OnceLock; use std::time::Duration; use std::time::Instant; +use amalthea::comm::variables_comm::RefreshParams; +use amalthea::comm::variables_comm::UpdateParams; +use amalthea::comm::variables_comm::VariablesFrontendEvent; use amalthea::fixtures::dummy_frontend::DummyConnection; use amalthea::fixtures::dummy_frontend::DummyFrontend; use amalthea::fixtures::dummy_frontend::ExecuteRequestOptions; @@ -64,6 +67,14 @@ pub struct DummyArkFrontend { streams_handled: Cell, /// Whether we're currently in a debug context (between start_debug and stop_debug) in_debug: Cell, + /// Comm ID of the open variables comm, if any. + variables_comm_id: RefCell>, + /// Buffered variables comm events, auto-collected by `recv_iopub_next()`. + /// Buffering is needed because variables events can race with Idle on + /// IOPub. Once https://github.com/posit-dev/ark/issues/689 is resolved, + /// we should be able to assert these deterministically in the message + /// sequence instead. + variables_events: RefCell>, } /// Result of draining accumulated streams @@ -122,6 +133,8 @@ impl DummyArkFrontend { pending_iopub_messages: RefCell::new(VecDeque::new()), streams_handled: Cell::new(false), in_debug: Cell::new(false), + variables_comm_id: RefCell::new(None), + variables_events: RefCell::new(VecDeque::new()), } } @@ -173,18 +186,20 @@ impl DummyArkFrontend { } } - /// Core primitive: receive the next non-stream message from IOPub. + /// Core primitive: receive the next non-stream, non-variables-comm message + /// from IOPub. /// - /// This automatically buffers any Stream messages encountered while waiting - /// for a non-stream message. All `recv_iopub_*` methods for non-stream - /// messages should use this instead of calling `recv_iopub()` directly. + /// This automatically buffers any Stream messages and variables comm + /// messages encountered while waiting. All `recv_iopub_*` methods for + /// non-stream messages should use this instead of calling `recv_iopub()` + /// directly. fn recv_iopub_next(&self) -> Message { // Check put-back buffer first if let Some(msg) = self.pending_iopub_messages.borrow_mut().pop_front() { trace_iopub_msg(&msg); return msg; } - // Read from socket, auto-buffering any streams + // Read from socket, auto-buffering streams and comm messages loop { let msg = self.recv_iopub(); trace_iopub_msg(&msg); @@ -192,11 +207,26 @@ impl DummyArkFrontend { Message::Stream(ref data) => { self.buffer_stream(&data.content); }, + Message::CommMsg(ref data) if self.is_variables_comm(&data.content.comm_id) => { + self.buffer_variables_event(&data.content.data); + }, other => return other, } } } + fn is_variables_comm(&self, comm_id: &str) -> bool { + self.variables_comm_id + .borrow() + .as_deref() + .is_some_and(|id| id == comm_id) + } + + fn buffer_variables_event(&self, data: &serde_json::Value) { + let event: VariablesFrontendEvent = serde_json::from_value(data.clone()).unwrap(); + self.variables_events.borrow_mut().push_back(event); + } + /// Internal helper for stream assertions. #[track_caller] fn assert_stream_contains(&self, buffer: &RefCell, stream_name: &str, expected: &str) { @@ -230,13 +260,17 @@ impl DummyArkFrontend { let poll_timeout = remaining.min(Duration::from_millis(100)); if let Some(msg) = self.recv_iopub_with_timeout(poll_timeout) { - match msg { + match &msg { Message::Stream(ref data) => { trace_iopub_msg(&msg); self.buffer_stream(&data.content); }, + Message::CommMsg(ref data) if self.is_variables_comm(&data.content.comm_id) => { + trace_iopub_msg(&msg); + self.buffer_variables_event(&data.content.data); + }, // Don't trace here - will be traced when popped from pending queue - other => self.pending_iopub_messages.borrow_mut().push_back(other), + _ => self.pending_iopub_messages.borrow_mut().push_back(msg), } } } @@ -660,6 +694,104 @@ impl DummyArkFrontend { port } + /// Open a `positron.variables` comm via the Shell socket. + /// + /// Returns the initial `RefreshParams` from the variables pane. + /// After this call, variables comm messages are automatically buffered + /// by `recv_iopub_next()` (parallel to how Stream messages are handled). + #[track_caller] + pub fn open_variables_comm(&self) -> RefreshParams { + debug_assert!( + self.variables_comm_id.borrow().is_none(), + "Variables comm already open" + ); + + let comm_id = uuid::Uuid::new_v4().to_string(); + *self.variables_comm_id.borrow_mut() = Some(comm_id.clone()); + + self.send_shell(CommOpen { + comm_id: comm_id.clone(), + target_name: String::from("positron.variables"), + data: serde_json::json!({}), + }); + + // Busy + Idle from the comm_open handler. The initial Refresh from + // the variables thread may arrive interleaved or shortly after; + // `recv_iopub_next()` auto-buffers it. + self.recv_iopub_busy(); + self.recv_iopub_idle(); + + // The initial Refresh may already be buffered, or we need to wait. + self.recv_variables_refresh() + } + + /// Wait for the next variables `Refresh` event. + /// + /// Checks the internal buffer first (populated by `recv_iopub_next()`), + /// then reads more IOPub messages if needed. + #[track_caller] + pub fn recv_variables_refresh(&self) -> RefreshParams { + match self.recv_variables_event() { + VariablesFrontendEvent::Refresh(params) => params, + other => panic!("Expected variables Refresh, got {other:?}"), + } + } + + /// Wait for the next variables `Update` event. + /// + /// Checks the internal buffer first (populated by `recv_iopub_next()`), + /// then reads more IOPub messages if needed. + #[track_caller] + pub fn recv_variables_update(&self) -> UpdateParams { + match self.recv_variables_event() { + VariablesFrontendEvent::Update(params) => params, + other => panic!("Expected variables Update, got {other:?}"), + } + } + + /// Wait for the next variables comm event (Refresh or Update). + /// This polling loop exists because variables events can race with Idle + /// on IOPub. Once https://github.com/posit-dev/ark/issues/689 is + /// resolved, this can be replaced by a direct `recv_iopub_next()` call. + #[track_caller] + fn recv_variables_event(&self) -> VariablesFrontendEvent { + // Check buffer first + if let Some(event) = self.variables_events.borrow_mut().pop_front() { + return event; + } + + // Read more IOPub messages until we get a variables event + let deadline = Instant::now() + RECV_TIMEOUT; + loop { + if Instant::now() >= deadline { + panic!("Timeout waiting for variables comm event"); + } + + let remaining = deadline.saturating_duration_since(Instant::now()); + let poll_timeout = remaining.min(Duration::from_millis(100)); + + if let Some(msg) = self.recv_iopub_with_timeout(poll_timeout) { + match &msg { + Message::CommMsg(ref data) if self.is_variables_comm(&data.content.comm_id) => { + self.buffer_variables_event(&data.content.data); + return self.variables_events.borrow_mut().pop_front().unwrap(); + }, + // Buffer any incoming stream messages + Message::Stream(ref data) => { + self.buffer_stream(&data.content); + }, + // Idle can race with variables events (see #689) + Message::Status(_) => { + self.pending_iopub_messages.borrow_mut().push_back(msg); + }, + other => { + panic!("Unexpected message while waiting for variables event: {other:?}") + }, + } + } + } + } + /// Receive from IOPub and assert a `start_debug` comm message. /// Automatically skips any Stream messages. #[track_caller] @@ -1254,6 +1386,17 @@ impl Drop for DummyArkFrontend { ); } + // Fail if variables events were buffered but never consumed + let buffered_variables = self.variables_events.borrow(); + if !buffered_variables.is_empty() { + panic!( + "Test has {} unconsumed variables event(s): {:?}", + buffered_variables.len(), + *buffered_variables + ); + } + drop(buffered_variables); + // Helper to check if a message is exempt from "unexpected message" check let is_exempt = |msg: &Message| -> bool { match msg {