From c93bf695b0db8707cd2a40ea877063d7fd362dcc Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 24 Oct 2025 15:42:33 +0200 Subject: [PATCH 01/44] Take charge of parsing and evaluation --- crates/amalthea/src/error.rs | 2 +- crates/ark/src/interface.rs | 276 +++++++++++++++++++++---------- crates/ark/tests/kernel.rs | 105 +++++++++++- crates/harp/src/object.rs | 13 ++ crates/harp/src/parser/srcref.rs | 36 ++++ crates/libr/src/r.rs | 12 +- 6 files changed, 344 insertions(+), 100 deletions(-) diff --git a/crates/amalthea/src/error.rs b/crates/amalthea/src/error.rs index 99fdf15fa..c71a9aa59 100644 --- a/crates/amalthea/src/error.rs +++ b/crates/amalthea/src/error.rs @@ -228,6 +228,6 @@ impl From> for Error { macro_rules! anyhow { ($($rest: expr),*) => {{ let message = anyhow::anyhow!($($rest, )*); - crate::error::Error::Anyhow(message) + $crate::error::Error::Anyhow(message) }} } diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 7c6f7e3d8..56ec1b34e 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -73,6 +73,8 @@ use harp::object::RObject; use harp::r_symbol; use harp::routines::r_register_routines; use harp::session::r_traceback; +use harp::srcref::get_block_srcrefs; +use harp::srcref::get_srcref; use harp::utils::r_is_data_frame; use harp::utils::r_typeof; use harp::R_MAIN_THREAD_ID; @@ -127,7 +129,6 @@ use crate::srcref::ark_uri; use crate::srcref::ns_populate_srcref; use crate::srcref::resource_loaded_namespaces; use crate::startup; -use crate::strings::lines; use crate::sys::console::console_to_utf8; use crate::ui::UiCommMessage; use crate::ui::UiCommSender; @@ -235,7 +236,7 @@ pub struct RMain { pub positron_ns: Option, - pending_lines: Vec, + pending_inputs: Option, /// Banner output accumulated during startup, but set to `None` after we complete /// the initialization procedure and forward the banner on @@ -269,6 +270,52 @@ pub struct RMain { debug_session_index: u32, } +struct PendingInputs { + exprs: RObject, + srcrefs: RObject, + len: isize, + index: isize, +} + +impl PendingInputs { + pub(crate) fn new(exprs: RObject, srcrefs: RObject) -> Option { + let len = exprs.length(); + let index = 0; + + if len == 0 { + return None; + } + + Some(Self { + exprs, + srcrefs, + len, + index, + }) + } + + pub(crate) fn is_empty(&self) -> bool { + self.index >= self.len + } + + pub(crate) fn pop(&mut self) -> Option { + if self.index >= self.len { + return None; + } + + let srcref = get_srcref(self.srcrefs.sexp, self.index); + let expr = harp::r_list_get(self.exprs.sexp, self.index); + + self.index += 1; + Some(PendingInput { expr, srcref }) + } +} + +pub(crate) struct PendingInput { + expr: RObject, + srcref: RObject, +} + /// Represents the currently active execution request from the frontend. It /// resolves at the next invocation of the `ReadConsole()` frontend method. struct ActiveReadConsoleRequest { @@ -323,8 +370,9 @@ pub enum ConsoleInput { Input(String), } -pub enum ConsoleResult { +pub(crate) enum ConsoleResult { NewInput, + NewPendingInput(PendingInput), Interrupt, Disconnected, Error(amalthea::Error), @@ -616,7 +664,6 @@ impl RMain { pending_futures: HashMap::new(), session_mode, positron_ns: None, - pending_lines: Vec::new(), banner: None, r_error_buffer: None, captured_output: String::new(), @@ -624,6 +671,7 @@ impl RMain { debug_last_stack: vec![], debug_env: None, debug_session_index: 1, + pending_inputs: None, } } @@ -746,6 +794,13 @@ impl RMain { let info = self.prompt_info(prompt); log::trace!("R prompt: {}", info.input_prompt); + // An incomplete prompt when we no longer have any inputs to send should + // never happen because we check for incomplete inputs ahead of time and + // respond to the frontend with an error. + if info.incomplete { + unreachable!("Incomplete input in `ReadConsole` handler"); + } + // Upon entering read-console, finalize any debug call text that we were capturing. // At this point, the user can either advance the debugger, causing us to capture // a new expression, or execute arbitrary code, where we will reuse a finalized @@ -1020,18 +1075,19 @@ impl RMain { } } - // An incomplete prompt when we no longer have any inputs to send should - // never happen because we check for incomplete inputs ahead of time and - // respond to the frontend with an error. - if info.incomplete && self.pending_lines.is_empty() { - unreachable!("Incomplete input in `ReadConsole` handler"); - } - - // Next check if we have any pending lines. If we do, we are in the middle of - // evaluating a multi line selection, so immediately write the next line into R's buffer. - // The active request remains active. - if let Some(console_result) = self.handle_pending_line(buf, buflen) { - return Some(console_result); + if let Some(input) = self.pop_pending() { + if info.browser { + if let Ok(sym) = harp::RSymbol::new(input.expr.sexp) { + let sym = String::from(sym); + let debug_commands = + vec!["c", "cont", "f", "help", "n", "s", "where", "r", "Q"]; + if debug_commands.contains(&&sym[..]) { + Self::on_console_input(buf, buflen, sym).unwrap(); + } + return Some(ConsoleResult::NewInput); + } + } + return Some(ConsoleResult::NewPendingInput(input)); } // Finally, check if we have an active request from a previous `read_console()` @@ -1124,29 +1180,107 @@ impl RMain { } } - // If the input is invalid (e.g. incomplete), don't send it to R - // at all, reply with an error right away - if let Err(err) = Self::check_console_input(code.as_str()) { - return Some(ConsoleResult::Error(err)); + if let Err(err) = self.read(&code) { + return Some(ConsoleResult::Error(amalthea::anyhow!("{err:?}"))); } - // Split input by lines, retrieve first line, and store - // remaining lines in a buffer. This helps with long inputs - // because R has a fixed input buffer size of 4096 bytes at the - // time of writing. - let code = self.buffer_console_input(code.as_str()); - - // Store input in R's buffer and return sentinel indicating some - // new input is ready - match Self::on_console_input(buf, buflen, code) { - Ok(()) => Some(ConsoleResult::NewInput), - Err(err) => Some(ConsoleResult::Error(err)), - } + self.handle_active_request(info, buf, buflen) }, + ConsoleInput::EOF => Some(ConsoleResult::Disconnected), } } + fn read(&mut self, input: &str) -> anyhow::Result<()> { + let status = match harp::parse_status(&harp::ParseInput::Text(input)) { + Err(err) => { + // Failed to even attempt to parse the input, something is seriously wrong + // FIXME: There are some valid syntax errors going through here, e.g. `identity |> _(1)`. + return Err(anyhow!("Failed to parse input: {err:?}")); + }, + Ok(status) => status, + }; + + // - Incomplete inputs put R into a state where it expects more input that will never come, so we + // immediately reject them. Positron should never send us these, but Jupyter Notebooks may. + // - Complete statements are obviously fine. + // - Syntax errors will cause R to throw an error, which is expected. + let exprs = match status { + harp::ParseResult::Complete(exprs) => exprs, + harp::ParseResult::Incomplete => { + return Err(anyhow!("Can't execute incomplete input:\n{input}")); + }, + harp::ParseResult::SyntaxError { message, .. } => { + return Err(anyhow!("Syntax error: {message}")); + }, + }; + + let srcrefs = get_block_srcrefs(exprs.sexp); + + self.pending_inputs = PendingInputs::new(exprs, srcrefs); + Ok(()) + } + + fn pop_pending(&mut self) -> Option { + let Some(pending_inputs) = self.pending_inputs.as_mut() else { + return None; + }; + + let Some(input) = pending_inputs.pop() else { + // TODO! Don't like this + self.pending_inputs = None; + return None; + }; + + if pending_inputs.is_empty() { + self.pending_inputs = None; + } + + Some(input) + } + + // SAFETY: Call this from a POD frame. Inputs must be protected. + unsafe fn eval_pending( + &mut self, + expr: libr::SEXP, + srcref: libr::SEXP, + buf: *mut c_uchar, + buflen: c_int, + ) -> Option<()> { + // SAFETY: This may jump in case of error, keep this POD + unsafe { + // The global source reference is stored in this global variable by + // the R REPL before evaluation. We do the same here. + libr::set(libr::R_Srcref, srcref); + + // Evaluate the expression. Beware: this may throw an R longjump. + let value = libr::Rf_eval(expr, R_ENVS.global); + libr::Rf_protect(value); + + // Store in the base environment for robust access from (almost) any + // evaluation environment. We only require the presence of `::` so + // we can reach into base. Note that unlike regular environments + // which are stored in pairlists or hash tables, the base environment + // is stored in the `value` field of symbols, i.e. their "CDR". + libr::SETCDR(r_symbol!(".ark_last_value"), value); + + libr::Rf_unprotect(1); + value + }; + + // Back in business, Rust away + let code = if unsafe { libr::get(libr::R_Visible) == 1 } { + String::from("base::.ark_last_value") + } else { + String::from("base::invisible(base::.ark_last_value)") + }; + + // Unwrap safety: The input always fits in the buffer + Self::on_console_input(buf, buflen, code).unwrap(); + + Some(()) + } + /// Handle an `input_request` received outside of an `execute_request` context /// /// We believe it is always invalid to receive an `input_request` that isn't @@ -1544,63 +1678,6 @@ impl RMain { self.get_ui_comm_tx().is_some() } - fn handle_pending_line(&mut self, buf: *mut c_uchar, buflen: c_int) -> Option { - if self.error_occurred { - // If an error has occurred, we've already sent a complete expression that resulted in - // an error. Flush the remaining lines and return to `read_console()`, who will handle - // that error. - self.pending_lines.clear(); - return None; - } - - let Some(input) = self.pending_lines.pop() else { - // No pending lines - return None; - }; - - match Self::on_console_input(buf, buflen, input) { - Ok(()) => Some(ConsoleResult::NewInput), - Err(err) => Some(ConsoleResult::Error(err)), - } - } - - fn check_console_input(input: &str) -> amalthea::Result<()> { - let status = unwrap!(harp::parse_status(&harp::ParseInput::Text(input)), Err(err) => { - // Failed to even attempt to parse the input, something is seriously wrong - return Err(Error::InvalidConsoleInput(format!( - "Failed to parse input: {err:?}" - ))); - }); - - // - Incomplete inputs put R into a state where it expects more input that will never come, so we - // immediately reject them. Positron should never send us these, but Jupyter Notebooks may. - // - Complete statements are obviously fine. - // - Syntax errors will cause R to throw an error, which is expected. - match status { - harp::ParseResult::Incomplete => Err(Error::InvalidConsoleInput(format!( - "Can't execute incomplete input:\n{input}" - ))), - harp::ParseResult::Complete(_) => Ok(()), - harp::ParseResult::SyntaxError { .. } => Ok(()), - } - } - - fn buffer_console_input(&mut self, input: &str) -> String { - // Split into lines and reverse them to be able to `pop()` from the front - let mut lines: Vec = lines(input).rev().map(String::from).collect(); - - // SAFETY: There is always at least one line because: - // - `lines("")` returns 1 element containing `""` - // - `lines("\n")` returns 2 elements containing `""` - let first = lines.pop().unwrap(); - - // No-op if `lines` is empty - assert!(self.pending_lines.is_empty()); - self.pending_lines.append(&mut lines); - - first - } - /// Copy console input into R's internal input buffer /// /// Supposedly `buflen` is "the maximum length, in bytes, including the @@ -1917,7 +1994,7 @@ impl RMain { // https://github.com/posit-dev/positron/issues/1881 // Handle last expression - if r_main.pending_lines.is_empty() { + if r_main.pending_inputs.is_none() { r_main.autoprint_output.push_str(&content); return; } @@ -2299,8 +2376,26 @@ pub extern "C-unwind" fn r_read_console( // destructors. We're longjumping from here in case of interrupt. match result { + ConsoleResult::NewPendingInput(input) => { + let PendingInput { expr, srcref } = input; + + unsafe { + let expr = expr.into_protected(); + let srcref = srcref.into_protected(); + + match main.eval_pending(expr, srcref, buf, buflen) { + None => todo!(), + Some(()) => Some(ConsoleResult::NewInput), + }; + + libr::Rf_unprotect(2); + return 1; + } + }, + ConsoleResult::NewInput => return 1, ConsoleResult::Disconnected => return 0, + ConsoleResult::Interrupt => { log::trace!("Interrupting `ReadConsole()`"); unsafe { @@ -2311,6 +2406,7 @@ pub extern "C-unwind" fn r_read_console( log::error!("`Rf_onintr()` did not longjump"); return 0; }, + ConsoleResult::Error(err) => { main.propagate_error(anyhow::anyhow!("{err}")); }, diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 042142cec..450f86b89 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -418,6 +418,9 @@ fn test_execute_request_multiple_expressions() { fn test_execute_request_single_line_buffer_overflow() { let frontend = DummyArkFrontend::lock(); + // This used to fail back when we were passing inputs down to the REPL from + // our `ReadConsole` handler. Below is the old test description for posterity. + // The newlines do matter for what we are testing here, // due to how we internally split by newlines. We want // to test that the `aaa`s result in an immediate R error, @@ -430,16 +433,10 @@ fn test_execute_request_single_line_buffer_overflow() { let input = frontend.recv_iopub_execute_input(); assert_eq!(input.code, code); - assert!(frontend - .recv_iopub_execute_error() - .contains("Can't pass console input on to R")); + assert!(frontend.recv_iopub_execute_result().contains(&aaa)); frontend.recv_iopub_idle(); - - assert_eq!( - frontend.recv_shell_execute_reply_exception(), - input.execution_count - ); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } #[test] @@ -590,6 +587,98 @@ fn test_stdin_from_menu() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } +// Can debug the base environment (parent is the empty environment) +#[test] +fn test_browser_in_base_env() { + let frontend = DummyArkFrontend::lock(); + + let code = "evalq(browser(), baseenv())"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + // Inside `evalq()` we aren't at top level, so this comes as an iopub stream + // and not an execute result + frontend.recv_iopub_stream_stdout("Called from: evalq(browser(), baseenv())\n"); + + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // While paused in the debugger, evaluate a simple expression + let code = "1 + 1"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_stream_stdout("[1] 2\n"); + + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + let code = "Q"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + +// The minimal environment we can debug in: access to base via `::`. This might +// be a problem for very specialised sandboxing environment, but they can +// temporarily add `::` while debugging. +#[test] +fn test_browser_in_sandboxing_environment() { + let frontend = DummyArkFrontend::lock(); + + let code = " +env <- new.env(parent = emptyenv()) +env$`::` <- `::` +evalq(base::browser(), env)"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + // Inside `evalq()` we aren't at top level, so this comes as an iopub stream + // and not an execute result + frontend.recv_iopub_stream_stdout("Called from: evalq(base::browser(), env)\n"); + + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // While paused in the debugger, evaluate a simple expression that only + // requires `::` + let code = "base::list"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_stream_stdout("function (...) .Primitive(\"list\")\n"); + + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + let code = "Q"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + #[test] fn test_env_vars() { // These environment variables are set by R's shell script frontend. diff --git a/crates/harp/src/object.rs b/crates/harp/src/object.rs index 3daa1e0f1..5ce34a236 100644 --- a/crates/harp/src/object.rs +++ b/crates/harp/src/object.rs @@ -238,6 +238,10 @@ pub fn r_list_poke(x: SEXP, i: R_xlen_t, value: SEXP) { } } +pub fn r_list_get(x: SEXP, i: R_xlen_t) -> RObject { + unsafe { RObject::new(VECTOR_ELT(x, i)) } +} + pub fn r_lgl_begin(x: SEXP) -> *mut i32 { unsafe { LOGICAL(x) } } @@ -321,6 +325,15 @@ impl RObject { } } + /// Consume the `RObject` and return the underlying `SEXP`, protected via + /// `Rf_protect`. + pub unsafe fn into_protected(self) -> SEXP { + unsafe { + libr::Rf_protect(self.sexp); + } + self.sexp + } + pub fn view(data: SEXP) -> Self { RObject { sexp: data, diff --git a/crates/harp/src/parser/srcref.rs b/crates/harp/src/parser/srcref.rs index 91562fd94..65e7f58c0 100644 --- a/crates/harp/src/parser/srcref.rs +++ b/crates/harp/src/parser/srcref.rs @@ -153,6 +153,42 @@ impl TryFrom<&harp::CharacterVector> for SrcFile { } } +pub fn get_srcref(srcrefs: libr::SEXP, ind: isize) -> RObject { + if crate::r_is_null(srcrefs) { + return RObject::null(); + } + + if harp::r_length(srcrefs) <= ind { + return RObject::null(); + } + + let result = harp::list_get(srcrefs, ind); + + if crate::r_is_null(result) { + return RObject::null(); + } + + if unsafe { libr::TYPEOF(result) as u32 } != libr::INTSXP { + return RObject::null(); + } + + if harp::r_length(result) < 6 { + return RObject::null(); + } + + RObject::new(result) +} + +pub fn get_block_srcrefs(call: libr::SEXP) -> RObject { + let srcrefs = unsafe { libr::Rf_getAttrib(call, libr::R_SrcrefSymbol) }; + + if unsafe { libr::TYPEOF(srcrefs) as u32 } == libr::VECSXP { + return RObject::new(srcrefs); + } + + RObject::null() +} + #[cfg(test)] mod tests { use std::ops::Range; diff --git a/crates/libr/src/r.rs b/crates/libr/src/r.rs index eb927e48c..1c1b15663 100644 --- a/crates/libr/src/r.rs +++ b/crates/libr/src/r.rs @@ -135,7 +135,7 @@ functions::generate! { pub fn Rf_cons(arg1: SEXP, arg2: SEXP) -> SEXP; - pub fn Rf_defineVar(arg1: SEXP, arg2: SEXP, arg3: SEXP); + pub fn Rf_defineVar(sym: SEXP, value: SEXP, env: SEXP); pub fn Rf_eval(arg1: SEXP, arg2: SEXP) -> SEXP; @@ -617,6 +617,14 @@ constant_globals::generate! { #[default = std::ptr::null_mut()] pub static R_TripleColonSymbol: SEXP; + #[doc = "\"srcfile\""] + #[default = std::ptr::null_mut()] + pub static R_SrcfileSymbol: SEXP; + + #[doc = "\"srcref\""] + #[default = std::ptr::null_mut()] + pub static R_SrcrefSymbol: SEXP; + #[doc = "\"tsp\""] #[default = std::ptr::null_mut()] pub static R_TspSymbol: SEXP; @@ -689,6 +697,8 @@ mutable_globals::generate! { pub static mut R_Srcref: SEXP; + pub static mut R_Visible: Rboolean; + // ----------------------------------------------------------------------------------- // Unix From 5df36e922dfb1767847fbd8c12029d07835b67e1 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 29 Oct 2025 12:27:14 +0100 Subject: [PATCH 02/44] More caller tracking --- crates/amalthea/src/fixtures/dummy_frontend.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/amalthea/src/fixtures/dummy_frontend.rs b/crates/amalthea/src/fixtures/dummy_frontend.rs index 137e453a2..8d427a9d9 100644 --- a/crates/amalthea/src/fixtures/dummy_frontend.rs +++ b/crates/amalthea/src/fixtures/dummy_frontend.rs @@ -236,6 +236,7 @@ impl DummyFrontend { id } + #[track_caller] pub fn recv(socket: &Socket) -> Message { // It's important to wait with a timeout because the kernel thread might have // panicked, preventing it from sending the expected message. The tests would then @@ -254,16 +255,19 @@ impl DummyFrontend { } /// Receives a Jupyter message from the Shell socket + #[track_caller] pub fn recv_shell(&self) -> Message { Self::recv(&self.shell_socket) } /// Receives a Jupyter message from the IOPub socket + #[track_caller] pub fn recv_iopub(&self) -> Message { Self::recv(&self.iopub_socket) } /// Receives a Jupyter message from the Stdin socket + #[track_caller] pub fn recv_stdin(&self) -> Message { Self::recv(&self.stdin_socket) } From 7dd0f4ec65d9821b113d0afd2ca9143f7ffa9c4b Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 30 Oct 2025 09:51:10 +0100 Subject: [PATCH 03/44] Consolidate debugger states --- crates/ark/src/interface.rs | 108 ++++++++++++++++++++---------------- 1 file changed, 61 insertions(+), 47 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 56ec1b34e..5ce26ed04 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -791,21 +791,30 @@ impl RMain { buflen: c_int, _hist: c_int, ) -> ConsoleResult { + // Upon entering read-console, finalize any debug call text that we were capturing. + // At this point, the user can either advance the debugger, causing us to capture + // a new expression, or execute arbitrary code, where we will reuse a finalized + // debug call text to maintain the debug state. + self.dap.finalize_call_text(); + let info = self.prompt_info(prompt); log::trace!("R prompt: {}", info.input_prompt); - // An incomplete prompt when we no longer have any inputs to send should - // never happen because we check for incomplete inputs ahead of time and - // respond to the frontend with an error. + // Since we parse expressions ourselves and only send complete inputs to + // the base REPL, we never should be asked for completing input if info.incomplete { unreachable!("Incomplete input in `ReadConsole` handler"); } - // Upon entering read-console, finalize any debug call text that we were capturing. - // At this point, the user can either advance the debugger, causing us to capture - // a new expression, or execute arbitrary code, where we will reuse a finalized - // debug call text to maintain the debug state. - self.dap.finalize_call_text(); + // Invariant: If we detect a browser prompt, `self.dap.is_debugging()` + // is true. Otherwise it is false. + if info.browser { + // Start or continue debugging with the `debug_preserve_focus` hint + // from the last expression we evaluated + self.start_debug(self.debug_preserve_focus); + } else if self.dap.is_debugging() { + self.stop_debug(); + } // We get called here everytime R needs more input. This handler // represents the driving event of a small state machine that manages @@ -855,21 +864,13 @@ impl RMain { // often. We'd still push a `DidChangeConsoleInputs` notification from // here, but only containing high-level information such as `search()` // contents and `ls(rho)`. - if !info.browser && !info.incomplete && !info.input_request { + if !self.dap.is_debugging() && !info.input_request { self.refresh_lsp(); } // Signal prompt EVENTS.console_prompt.emit(()); - if info.browser { - self.start_debug(); - } else { - if self.dap.is_debugging() { - self.stop_debug(); - } - } - let mut select = crossbeam::channel::Select::new(); // Cloning is necessary to avoid a double mutable borrow error @@ -1076,24 +1077,53 @@ impl RMain { } if let Some(input) = self.pop_pending() { - if info.browser { + // Default: preserve current focus for evaluated expressions. + // This only has an effect if we're debugging. + // https://github.com/posit-dev/positron/issues/3151 + self.debug_preserve_focus = true; + + if self.dap.is_debugging() { + // Try to interpret this pending input as a symbol (debug commands + // are entered as symbols). Whether or not it parses as a symbol, + // if we're currently debugging we must set `debug_preserve_focus`. if let Ok(sym) = harp::RSymbol::new(input.expr.sexp) { + // All debug commands as documented in `?browser` + const DEBUG_COMMANDS: &[&str] = + &["c", "cont", "f", "help", "n", "s", "where", "r", "Q"]; + + // The subset of debug commands that continue execution + const DEBUG_COMMANDS_CONTINUE: &[&str] = &["n", "f", "c", "cont"]; + let sym = String::from(sym); - let debug_commands = - vec!["c", "cont", "f", "help", "n", "s", "where", "r", "Q"]; - if debug_commands.contains(&&sym[..]) { + + if DEBUG_COMMANDS.contains(&&sym[..]) { + if DEBUG_COMMANDS_CONTINUE.contains(&&sym[..]) { + // For continue-like commands, we do not preserve focus, + // i.e. we let the cursor jump to the stopped + // position. Set the preserve focus hint for the + // next iteration of ReadConsole. + self.debug_preserve_focus = false; + + // Let the DAP client know that execution is now continuing + self.dap.send_dap(DapBackendEvent::Continued); + } + + // All debug commands are forwarded to the base REPL as + // is so that R can interpret them. + // Unwrap safety: A debug command fits in the buffer. Self::on_console_input(buf, buflen, sym).unwrap(); + return Some(ConsoleResult::NewInput); } - return Some(ConsoleResult::NewInput); } } + return Some(ConsoleResult::NewPendingInput(input)); } - // Finally, check if we have an active request from a previous `read_console()` - // iteration. If so, we `take()` and clear the `active_request` as we're about - // to complete it and send a reply to unblock the active Shell - // request. + // If we get here we finished evaluating all pending inputs. Check if we + // have an active request from a previous `read_console()` iteration. If + // so, we `take()` and clear the `active_request` as we're about to + // complete it and send a reply to unblock the active Shell request. if let Some(req) = std::mem::take(&mut self.active_request) { // FIXME: Race condition between the comm and shell socket threads. // @@ -1116,7 +1146,8 @@ impl RMain { self.reply_execute_request(req, &info); } - // Prepare for the next user input + // Fall through Ark's ReadConsole event loop while waiting for the next + // execution request None } @@ -1166,20 +1197,6 @@ impl RMain { match input { ConsoleInput::Input(code) => { - // Handle commands for the debug interpreter - if self.dap.is_debugging() { - let continue_cmds = vec!["n", "f", "c", "cont"]; - if continue_cmds.contains(&&code[..]) { - // We're stepping so we want to focus the next location we stop at - self.debug_preserve_focus = false; - self.dap.send_dap(DapBackendEvent::Continued); - } else { - // The user is evaluating some other expression so preserve current focus - // https://github.com/posit-dev/positron/issues/3151 - self.debug_preserve_focus = true; - } - } - if let Err(err) = self.read(&code) { return Some(ConsoleResult::Error(amalthea::anyhow!("{err:?}"))); } @@ -1323,7 +1340,7 @@ impl RMain { return ConsoleResult::Error(Error::InvalidInputRequest(message)); } - fn start_debug(&mut self) { + fn start_debug(&mut self, debug_preserve_focus: bool) { match self.dap.stack_info() { Ok(stack) => { if let Some(frame) = stack.first() { @@ -1350,11 +1367,8 @@ impl RMain { let fallback_sources = self.load_fallback_sources(&stack); self.debug_last_stack = stack_id; - self.dap.start_debug( - stack, - same_stack && self.debug_preserve_focus, - fallback_sources, - ); + self.dap + .start_debug(stack, same_stack && debug_preserve_focus, fallback_sources); }, Err(err) => log::error!("ReadConsole: Can't get stack info: {err}"), }; From 2f078468133f9794bde0eeddf302e5c0eab1b2fb Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 30 Oct 2025 11:15:42 +0100 Subject: [PATCH 04/44] Extract `handle_input_request()` --- crates/ark/src/interface.rs | 102 +++++++++++++----------------------- 1 file changed, 35 insertions(+), 67 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 5ce26ed04..4f99a100b 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -816,43 +816,11 @@ impl RMain { self.stop_debug(); } - // We get called here everytime R needs more input. This handler - // represents the driving event of a small state machine that manages - // communication between R and the frontend. In the following order: - // - // - If we detect an input request prompt, then we forward the request - // on to the frontend and then fall through to the event loop to wait - // on the input reply. - // - // - If the vector of pending lines is not empty, R might be waiting for - // us to complete an incomplete expression, or we might just have - // completed an intermediate expression (e.g. from an ExecuteRequest - // like `foo\nbar` where `foo` is intermediate and `bar` is final). - // Send the next line to R. - // - // - If the vector of pending lines is empty, and if the prompt is for - // new R code, we close the active ExecuteRequest and send an - // ExecuteReply to the frontend. We then fall through to the event - // loop to wait for more input. - // - // This state machine depends on being able to reliably distinguish - // between readline prompts (from `readline()`, `scan()`, or `menu()`), - // and actual R code prompts (either top-level or from a nested debug - // REPL). A readline prompt should never change our state (in - // particular our vector of pending inputs). We think we are making this - // distinction sufficiently robustly but ideally R would let us know the - // prompt type so there is no ambiguity at all. - // - // R might throw an error at any time while we are working on our vector - // of pending lines, either from a syntax error or from an evaluation - // error. When this happens, we abort evaluation and clear the pending - // lines. - // - // If the vector of pending lines is empty and we detect an incomplete - // prompt, this is a panic. We check ahead of time for complete - // expressions before breaking up an ExecuteRequest in multiple lines, - // so this should not happen. - if let Some(console_result) = self.handle_active_request(&info, buf, buflen) { + if info.input_request { + if let Some(input) = self.handle_input_request(&info.input_prompt, buf, buflen) { + return input; + } + } else if let Some(console_result) = self.handle_active_request(&info, buf, buflen) { return console_result; }; @@ -1046,36 +1014,6 @@ impl RMain { buf: *mut c_uchar, buflen: c_int, ) -> Option { - // TODO: Can we remove this below code? - // If the prompt begins with "Save workspace", respond with (n) - // and allow R to immediately exit. - // - // NOTE: Should be able to overwrite the `Cleanup` frontend method. - // This would also help with detecting normal exits versus crashes. - if info.input_prompt.starts_with("Save workspace") { - match Self::on_console_input(buf, buflen, String::from("n")) { - Ok(()) => return Some(ConsoleResult::NewInput), - Err(err) => return Some(ConsoleResult::Error(err)), - } - } - - // First check if we are inside request for user input, like a `readline()` or `menu()`. - // It's entirely possible that we still have more pending lines, but an intermediate line - // put us into an `input_request` state. We must respond to that request before processing - // the rest of the pending lines. - if info.input_request { - if let Some(req) = &self.active_request { - // Send request to frontend. We'll wait for an `input_reply` - // from the frontend in the event loop in `read_console()`. - // The active request remains active. - self.request_input(req.originator.clone(), info.input_prompt.to_string()); - return None; - } else { - // Invalid input request, propagate error to R - return Some(self.handle_invalid_input_request(buf, buflen)); - } - } - if let Some(input) = self.pop_pending() { // Default: preserve current focus for evaluated expressions. // This only has an effect if we're debugging. @@ -1238,6 +1176,36 @@ impl RMain { Ok(()) } + /// Handles user input requests (e.g., readline, menu) and special prompts. + /// Returns `Some()` if this handler needs to return to the base R REPL, or + /// `None` if it needs to run Ark's `ReadConsole` event loop. + fn handle_input_request( + &mut self, + input_prompt: &str, + buf: *mut c_uchar, + buflen: c_int, + ) -> Option { + // If the prompt begins with "Save workspace", respond with (n) + // and allow R to immediately exit. + if input_prompt.starts_with("Save workspace") { + match Self::on_console_input(buf, buflen, String::from("n")) { + Ok(()) => return Some(ConsoleResult::NewInput), + Err(err) => return Some(ConsoleResult::Error(err)), + } + } + + if let Some(req) = &self.active_request { + // Send request to frontend. We'll wait for an `input_reply` + // from the frontend in the event loop in `read_console()`. + // The active request remains active. + self.request_input(req.originator.clone(), String::from(input_prompt)); + None + } else { + // Invalid input request, propagate error to R + Some(self.handle_invalid_input_request(buf, buflen)) + } + } + fn pop_pending(&mut self) -> Option { let Some(pending_inputs) = self.pending_inputs.as_mut() else { return None; From 06500400db844480baa137a50a7142dfd18a31cf Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 30 Oct 2025 11:47:04 +0100 Subject: [PATCH 05/44] Extract `handle_pending_input()` --- crates/ark/src/interface.rs | 127 ++++++++++++++++++++---------------- 1 file changed, 69 insertions(+), 58 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 4f99a100b..0a13709bf 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -820,9 +820,13 @@ impl RMain { if let Some(input) = self.handle_input_request(&info.input_prompt, buf, buflen) { return input; } - } else if let Some(console_result) = self.handle_active_request(&info, buf, buflen) { - return console_result; - }; + } else if let Some(input) = self.pop_pending() { + // Evaluate pending expression if there is any remaining + return self.handle_pending_input(input, buf, buflen); + } else { + // Otherwise close active request + self.handle_active_request(&info); + } // In the future we'll also send browser information, see // https://github.com/posit-dev/positron/issues/3001. Currently this is @@ -1008,56 +1012,7 @@ impl RMain { /// Returns: /// - `None` if we should fall through to the event loop to wait for more user input /// - `Some(ConsoleResult)` if we should immediately exit `read_console()` - fn handle_active_request( - &mut self, - info: &PromptInfo, - buf: *mut c_uchar, - buflen: c_int, - ) -> Option { - if let Some(input) = self.pop_pending() { - // Default: preserve current focus for evaluated expressions. - // This only has an effect if we're debugging. - // https://github.com/posit-dev/positron/issues/3151 - self.debug_preserve_focus = true; - - if self.dap.is_debugging() { - // Try to interpret this pending input as a symbol (debug commands - // are entered as symbols). Whether or not it parses as a symbol, - // if we're currently debugging we must set `debug_preserve_focus`. - if let Ok(sym) = harp::RSymbol::new(input.expr.sexp) { - // All debug commands as documented in `?browser` - const DEBUG_COMMANDS: &[&str] = - &["c", "cont", "f", "help", "n", "s", "where", "r", "Q"]; - - // The subset of debug commands that continue execution - const DEBUG_COMMANDS_CONTINUE: &[&str] = &["n", "f", "c", "cont"]; - - let sym = String::from(sym); - - if DEBUG_COMMANDS.contains(&&sym[..]) { - if DEBUG_COMMANDS_CONTINUE.contains(&&sym[..]) { - // For continue-like commands, we do not preserve focus, - // i.e. we let the cursor jump to the stopped - // position. Set the preserve focus hint for the - // next iteration of ReadConsole. - self.debug_preserve_focus = false; - - // Let the DAP client know that execution is now continuing - self.dap.send_dap(DapBackendEvent::Continued); - } - - // All debug commands are forwarded to the base REPL as - // is so that R can interpret them. - // Unwrap safety: A debug command fits in the buffer. - Self::on_console_input(buf, buflen, sym).unwrap(); - return Some(ConsoleResult::NewInput); - } - } - } - - return Some(ConsoleResult::NewPendingInput(input)); - } - + fn handle_active_request(&mut self, info: &PromptInfo) { // If we get here we finished evaluating all pending inputs. Check if we // have an active request from a previous `read_console()` iteration. If // so, we `take()` and clear the `active_request` as we're about to @@ -1083,10 +1038,6 @@ impl RMain { // back to Idle. self.reply_execute_request(req, &info); } - - // Fall through Ark's ReadConsole event loop while waiting for the next - // execution request - None } fn handle_execute_request( @@ -1135,11 +1086,22 @@ impl RMain { match input { ConsoleInput::Input(code) => { + // Parse input into pending expressions if let Err(err) = self.read(&code) { return Some(ConsoleResult::Error(amalthea::anyhow!("{err:?}"))); } - self.handle_active_request(info, buf, buflen) + // Evaluate first expression if we got one + if let Some(input) = self.pop_pending() { + return Some(self.handle_pending_input(input, buf, buflen)); + } + + // Otherwise we got an empty input, e.g. `""` and there's + // nothing to do. Close active request. + self.handle_active_request(info); + + // And return to event loop + None }, ConsoleInput::EOF => Some(ConsoleResult::Disconnected), @@ -1206,6 +1168,55 @@ impl RMain { } } + fn handle_pending_input( + &mut self, + input: PendingInput, + buf: *mut c_uchar, + buflen: c_int, + ) -> ConsoleResult { + // Default: preserve current focus for evaluated expressions. + // This only has an effect if we're debugging. + // https://github.com/posit-dev/positron/issues/3151 + self.debug_preserve_focus = true; + + if self.dap.is_debugging() { + // Try to interpret this pending input as a symbol (debug commands + // are entered as symbols). Whether or not it parses as a symbol, + // if we're currently debugging we must set `debug_preserve_focus`. + if let Ok(sym) = harp::RSymbol::new(input.expr.sexp) { + // All debug commands as documented in `?browser` + const DEBUG_COMMANDS: &[&str] = + &["c", "cont", "f", "help", "n", "s", "where", "r", "Q"]; + + // The subset of debug commands that continue execution + const DEBUG_COMMANDS_CONTINUE: &[&str] = &["n", "f", "c", "cont"]; + + let sym = String::from(sym); + + if DEBUG_COMMANDS.contains(&&sym[..]) { + if DEBUG_COMMANDS_CONTINUE.contains(&&sym[..]) { + // For continue-like commands, we do not preserve focus, + // i.e. we let the cursor jump to the stopped + // position. Set the preserve focus hint for the + // next iteration of ReadConsole. + self.debug_preserve_focus = false; + + // Let the DAP client know that execution is now continuing + self.dap.send_dap(DapBackendEvent::Continued); + } + + // All debug commands are forwarded to the base REPL as + // is so that R can interpret them. + // Unwrap safety: A debug command fits in the buffer. + Self::on_console_input(buf, buflen, sym).unwrap(); + return ConsoleResult::NewInput; + } + } + } + + ConsoleResult::NewPendingInput(input) + } + fn pop_pending(&mut self) -> Option { let Some(pending_inputs) = self.pending_inputs.as_mut() else { return None; From 3367930ab3e45e5e464a5429ba9195094e14aa3a Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 30 Oct 2025 11:59:17 +0100 Subject: [PATCH 06/44] Rename `finalize_call_text()` to `handle_read_console()` --- crates/ark/src/dap/dap_r_main.rs | 8 ++++++-- crates/ark/src/interface.rs | 8 ++------ 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/ark/src/dap/dap_r_main.rs b/crates/ark/src/dap/dap_r_main.rs index 2943bde85..cd436e78e 100644 --- a/crates/ark/src/dap/dap_r_main.rs +++ b/crates/ark/src/dap/dap_r_main.rs @@ -141,7 +141,7 @@ impl RMainDap { self.debugging = false; } - pub fn handle_stdout(&mut self, content: &str) { + pub fn handle_write_console(&mut self, content: &str) { if let DebugCallText::Capturing(ref mut call_text) = self.call_text { // Append to current expression if we are currently capturing stdout call_text.push_str(content); @@ -164,7 +164,11 @@ impl RMainDap { } } - pub fn finalize_call_text(&mut self) { + pub fn handle_read_console(&mut self) { + // Upon entering read-console, finalize any debug call text that we were capturing. + // At this point, the user can either advance the debugger, causing us to capture + // a new expression, or execute arbitrary code, where we will reuse a finalized + // debug call text to maintain the debug state. match &self.call_text { // If not debugging, nothing to do. DebugCallText::None => (), diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 0a13709bf..333134fc3 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -791,11 +791,7 @@ impl RMain { buflen: c_int, _hist: c_int, ) -> ConsoleResult { - // Upon entering read-console, finalize any debug call text that we were capturing. - // At this point, the user can either advance the debugger, causing us to capture - // a new expression, or execute arbitrary code, where we will reuse a finalized - // debug call text to maintain the debug state. - self.dap.finalize_call_text(); + self.dap.handle_read_console(); let info = self.prompt_info(prompt); log::trace!("R prompt: {}", info.input_prompt); @@ -1944,7 +1940,7 @@ impl RMain { // To capture the current `debug: ` output, for use in the debugger's // match based fallback - r_main.dap.handle_stdout(&content); + r_main.dap.handle_write_console(&content); let stream = if otype == 0 { Stream::Stdout From dccf144f8af45043a7eeff84552c8c6a5a40253a Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 30 Oct 2025 12:11:58 +0100 Subject: [PATCH 07/44] Make `read()` a constructor method on `PendingInputs` --- crates/ark/src/interface.rs | 73 ++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 38 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 333134fc3..a310f0be1 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -278,20 +278,45 @@ struct PendingInputs { } impl PendingInputs { - pub(crate) fn new(exprs: RObject, srcrefs: RObject) -> Option { + pub(crate) fn read(input: &str) -> anyhow::Result> { + let status = match harp::parse_status(&harp::ParseInput::Text(input)) { + Err(err) => { + // Failed to even attempt to parse the input, something is seriously wrong + // FIXME: There are some valid syntax errors going through here, e.g. `identity |> _(1)`. + return Err(anyhow!("Failed to parse input: {err:?}")); + }, + Ok(status) => status, + }; + + // - Incomplete inputs put R into a state where it expects more input that will never come, so we + // immediately reject them. Positron should never send us these, but Jupyter Notebooks may. + // - Complete statements are obviously fine. + // - Syntax errors will cause R to throw an error, which is expected. + let exprs = match status { + harp::ParseResult::Complete(exprs) => exprs, + harp::ParseResult::Incomplete => { + return Err(anyhow!("Can't execute incomplete input:\n{input}")); + }, + harp::ParseResult::SyntaxError { message, .. } => { + return Err(anyhow!("Syntax error: {message}")); + }, + }; + + let srcrefs = get_block_srcrefs(exprs.sexp); + let len = exprs.length(); let index = 0; if len == 0 { - return None; + return Ok(None); } - Some(Self { + Ok(Some(Self { exprs, srcrefs, len, index, - }) + })) } pub(crate) fn is_empty(&self) -> bool { @@ -1083,11 +1108,14 @@ impl RMain { match input { ConsoleInput::Input(code) => { // Parse input into pending expressions - if let Err(err) = self.read(&code) { - return Some(ConsoleResult::Error(amalthea::anyhow!("{err:?}"))); + match PendingInputs::read(&code) { + Ok(inputs) => { + self.pending_inputs = inputs; + }, + Err(err) => return Some(ConsoleResult::Error(amalthea::anyhow!("{err:?}"))), } - // Evaluate first expression if we got one + // Evaluate first expression if there is one if let Some(input) = self.pop_pending() { return Some(self.handle_pending_input(input, buf, buflen)); } @@ -1104,36 +1132,6 @@ impl RMain { } } - fn read(&mut self, input: &str) -> anyhow::Result<()> { - let status = match harp::parse_status(&harp::ParseInput::Text(input)) { - Err(err) => { - // Failed to even attempt to parse the input, something is seriously wrong - // FIXME: There are some valid syntax errors going through here, e.g. `identity |> _(1)`. - return Err(anyhow!("Failed to parse input: {err:?}")); - }, - Ok(status) => status, - }; - - // - Incomplete inputs put R into a state where it expects more input that will never come, so we - // immediately reject them. Positron should never send us these, but Jupyter Notebooks may. - // - Complete statements are obviously fine. - // - Syntax errors will cause R to throw an error, which is expected. - let exprs = match status { - harp::ParseResult::Complete(exprs) => exprs, - harp::ParseResult::Incomplete => { - return Err(anyhow!("Can't execute incomplete input:\n{input}")); - }, - harp::ParseResult::SyntaxError { message, .. } => { - return Err(anyhow!("Syntax error: {message}")); - }, - }; - - let srcrefs = get_block_srcrefs(exprs.sexp); - - self.pending_inputs = PendingInputs::new(exprs, srcrefs); - Ok(()) - } - /// Handles user input requests (e.g., readline, menu) and special prompts. /// Returns `Some()` if this handler needs to return to the base R REPL, or /// `None` if it needs to run Ark's `ReadConsole` event loop. @@ -1219,7 +1217,6 @@ impl RMain { }; let Some(input) = pending_inputs.pop() else { - // TODO! Don't like this self.pending_inputs = None; return None; }; From fc697b789066181d3e440ed1135f8c3d9fa5f53b Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 30 Oct 2025 14:58:20 +0100 Subject: [PATCH 08/44] Refactor console error and result handling --- crates/ark/src/errors.rs | 11 ++ crates/ark/src/interface.rs | 300 +++++++++++++++++------------------- crates/ark/tests/kernel.rs | 6 +- 3 files changed, 160 insertions(+), 157 deletions(-) diff --git a/crates/ark/src/errors.rs b/crates/ark/src/errors.rs index ba36a76b2..0b98aee32 100644 --- a/crates/ark/src/errors.rs +++ b/crates/ark/src/errors.rs @@ -5,6 +5,8 @@ // // +use harp::exec::r_peek_error_buffer; +use harp::exec::RE_STACK_OVERFLOW; use harp::object::RObject; use harp::r_symbol; use harp::session::r_format_traceback; @@ -67,3 +69,12 @@ unsafe extern "C-unwind" fn ps_rust_backtrace() -> anyhow::Result { let trace = format!("{trace}"); Ok(*RObject::from(trace)) } + +pub(crate) fn stack_overflow_occurred() -> bool { + // Error handlers are not called on stack overflow so the error flag + // isn't set. Instead we detect stack overflows by peeking at the error + // buffer. The message is explicitly not translated to save stack space + // so the matching should be reliable. + let err_buf = r_peek_error_buffer(); + RE_STACK_OVERFLOW.is_match(&err_buf) +} diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index a310f0be1..47a91cba7 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -64,7 +64,6 @@ use harp::exec::r_peek_error_buffer; use harp::exec::r_sandbox; use harp::exec::RFunction; use harp::exec::RFunctionExt; -use harp::exec::RE_STACK_OVERFLOW; use harp::library::RLibraries; use harp::line_ending::convert_line_endings; use harp::line_ending::LineEnding; @@ -99,6 +98,7 @@ use crate::dap::dap_r_main::FrameInfoId; use crate::dap::dap_r_main::RMainDap; use crate::dap::Dap; use crate::errors; +use crate::errors::stack_overflow_occurred; use crate::help::message::HelpEvent; use crate::help::r_help::RHelp; use crate::lsp::events::EVENTS; @@ -341,6 +341,12 @@ pub(crate) struct PendingInput { srcref: RObject, } +#[derive(Debug, Clone)] +enum ConsoleValue { + Success(serde_json::Map), + Error(Exception), +} + /// Represents the currently active execution request from the frontend. It /// resolves at the next invocation of the `ReadConsole()` frontend method. struct ActiveReadConsoleRequest { @@ -837,7 +843,13 @@ impl RMain { self.stop_debug(); } - if info.input_request { + if let Some(exception) = self.take_exception() { + // Clear any pending inputs, if any + self.pending_inputs = None; + + // Reply to active request with error + self.handle_active_request(&info, ConsoleValue::Error(exception)); + } else if info.input_request { if let Some(input) = self.handle_input_request(&info.input_prompt, buf, buflen) { return input; } @@ -846,7 +858,8 @@ impl RMain { return self.handle_pending_input(input, buf, buflen); } else { // Otherwise close active request - self.handle_active_request(&info); + let result = self.take_result(); + self.handle_active_request(&info, ConsoleValue::Success(result)); } // In the future we'll also send browser information, see @@ -1025,6 +1038,92 @@ impl RMain { }; } + /// Take result from `self.autoprint_output` and R's `.Last.value` object + fn take_result(&mut self) -> serde_json::Map { + // TODO: Implement rich printing of certain outputs. + // Will we need something similar to the RStudio model, + // where we implement custom print() methods? Or can + // we make the stub below behave sensibly even when + // streaming R output? + let mut data = serde_json::Map::new(); + + // The output generated by autoprint is emitted as an + // `execute_result` message. + let mut autoprint = std::mem::take(&mut self.autoprint_output); + + if autoprint.ends_with('\n') { + // Remove the trailing newlines that R adds to outputs but that + // Jupyter frontends are not expecting. Is it worth taking a + // mutable self ref across calling methods to avoid the clone? + autoprint.pop(); + } + if autoprint.len() != 0 { + data.insert("text/plain".to_string(), json!(autoprint)); + } + + // Include HTML representation of data.frame + unsafe { + let value = Rf_findVarInFrame(R_GlobalEnv, r_symbol!(".Last.value")); + if r_is_data_frame(value) { + match to_html(value) { + Ok(html) => { + data.insert("text/html".to_string(), json!(html)); + }, + Err(err) => { + log::error!("{:?}", err); + }, + }; + } + } + + data + } + + fn take_exception(&mut self) -> Option { + let mut exception = if self.error_occurred { + // Reset flag + self.error_occurred = false; + + // We don't fill out `ename` with anything meaningful because typically + // R errors don't have names. We could consider using the condition class + // here, which r-lib/tidyverse packages have been using more heavily. + Exception { + ename: String::from(""), + evalue: self.error_message.clone(), + traceback: self.error_traceback.clone(), + } + } else if stack_overflow_occurred() { + // Call `base::traceback()` since we don't have a handled error + // object carrying a backtrace. This won't be formatted as a + // tree which is just as well since the recursive calls would + // push a tree too far to the right. + let traceback = r_traceback(); + + // Reset error buffer so we don't display this message again + let _ = RFunction::new("base", "stop").call(); + + Exception { + ename: String::from(""), + evalue: r_peek_error_buffer(), + traceback, + } + } else { + return None; + }; + + // Jupyter clients typically discard the `evalue` when a `traceback` is + // present. Jupyter-Console even disregards `evalue` in all cases. So + // include it here if we are in Notebook mode. But should Positron + // implement similar behaviour as the other frontends eventually? The + // first component of `traceback` could be compared to `evalue` and + // discarded from the traceback if the same. + if let SessionMode::Notebook = self.session_mode { + exception.traceback.insert(0, exception.evalue.clone()); + } + + Some(exception) + } + fn read_console_cleanup(&mut self) { // The debug environment is only valid while R is idle self.debug_env = None; @@ -1033,7 +1132,7 @@ impl RMain { /// Returns: /// - `None` if we should fall through to the event loop to wait for more user input /// - `Some(ConsoleResult)` if we should immediately exit `read_console()` - fn handle_active_request(&mut self, info: &PromptInfo) { + fn handle_active_request(&mut self, info: &PromptInfo, value: ConsoleValue) { // If we get here we finished evaluating all pending inputs. Check if we // have an active request from a previous `read_console()` iteration. If // so, we `take()` and clear the `active_request` as we're about to @@ -1057,7 +1156,9 @@ impl RMain { // Let frontend know the last request is complete. This turns us // back to Idle. - self.reply_execute_request(req, &info); + self.reply_execute_request(req, &info, value); + } else { + log::info!("No active request to handle, discarding: {value:?}"); } } @@ -1102,9 +1203,6 @@ impl RMain { }, }; - // Clear error flag - self.error_occurred = false; - match input { ConsoleInput::Input(code) => { // Parse input into pending expressions @@ -1122,7 +1220,7 @@ impl RMain { // Otherwise we got an empty input, e.g. `""` and there's // nothing to do. Close active request. - self.handle_active_request(info); + self.handle_active_request(info, ConsoleValue::Success(Default::default())); // And return to event loop None @@ -1722,142 +1820,56 @@ impl RMain { // Reply to the previously active request. The current prompt type and // whether an error has occurred defines the reply kind. - fn reply_execute_request(&mut self, req: ActiveReadConsoleRequest, prompt_info: &PromptInfo) { + fn reply_execute_request( + &mut self, + req: ActiveReadConsoleRequest, + prompt_info: &PromptInfo, + value: ConsoleValue, + ) { let prompt = &prompt_info.input_prompt; - let (reply, result) = if prompt_info.incomplete { - log::trace!("Got prompt {} signaling incomplete request", prompt); - (new_incomplete_reply(&req.request, req.exec_count), None) - } else if prompt_info.input_request { - unreachable!(); - } else { - log::trace!("Got R prompt '{}', completing execution", prompt); + log::trace!("Got R prompt '{}', completing execution", prompt); - self.make_execute_reply_error(req.exec_count) - .unwrap_or_else(|| self.make_execute_reply(req.exec_count)) - }; + let exec_count = req.exec_count; - if let Some(result) = result { - self.iopub_tx.send(result).unwrap(); - } + let (reply, result) = match value { + ConsoleValue::Success(data) => { + let reply = Ok(ExecuteReply { + status: Status::Ok, + execution_count: exec_count, + user_expressions: json!({}), + }); - log::trace!("Sending `execute_reply`: {reply:?}"); - req.reply_tx.send(reply).unwrap(); - } + let result = if data.len() > 0 { + Some(IOPubMessage::ExecuteResult(ExecuteResult { + execution_count: exec_count, + data: serde_json::Value::Object(data), + metadata: json!({}), + })) + } else { + None + }; - fn make_execute_reply_error( - &mut self, - exec_count: u32, - ) -> Option<(amalthea::Result, Option)> { - // Save and reset error occurred flag - let error_occurred = self.error_occurred; - self.error_occurred = false; - - // Error handlers are not called on stack overflow so the error flag - // isn't set. Instead we detect stack overflows by peeking at the error - // buffer. The message is explicitly not translated to save stack space - // so the matching should be reliable. - let err_buf = r_peek_error_buffer(); - let stack_overflow_occurred = RE_STACK_OVERFLOW.is_match(&err_buf); - - // Reset error buffer so we don't display this message again - if stack_overflow_occurred { - let _ = RFunction::new("base", "stop").call(); - } + (reply, result) + }, - // Send the reply to the frontend - if !error_occurred && !stack_overflow_occurred { - return None; - } + ConsoleValue::Error(exception) => { + let reply = Err(amalthea::Error::ShellErrorExecuteReply( + exception.clone(), + exec_count, + )); + let result = IOPubMessage::ExecuteError(ExecuteError { exception }); - // We don't fill out `ename` with anything meaningful because typically - // R errors don't have names. We could consider using the condition class - // here, which r-lib/tidyverse packages have been using more heavily. - let mut exception = if error_occurred { - Exception { - ename: String::from(""), - evalue: self.error_message.clone(), - traceback: self.error_traceback.clone(), - } - } else { - // Call `base::traceback()` since we don't have a handled error - // object carrying a backtrace. This won't be formatted as a - // tree which is just as well since the recursive calls would - // push a tree too far to the right. - let traceback = r_traceback(); - Exception { - ename: String::from(""), - evalue: err_buf.clone(), - traceback, - } + (reply, Some(result)) + }, }; - // Jupyter clients typically discard the `evalue` when a `traceback` is - // present. Jupyter-Console even disregards `evalue` in all cases. So - // include it here if we are in Notebook mode. But should Positron - // implement similar behaviour as the other frontends eventually? The - // first component of `traceback` could be compared to `evalue` and - // discarded from the traceback if the same. - if let SessionMode::Notebook = self.session_mode { - exception.traceback.insert(0, exception.evalue.clone()) - } - - let reply = new_execute_reply_error(exception.clone(), exec_count); - let result = IOPubMessage::ExecuteError(ExecuteError { exception }); - - Some((reply, Some(result))) - } - - fn make_execute_reply( - &mut self, - exec_count: u32, - ) -> (amalthea::Result, Option) { - // TODO: Implement rich printing of certain outputs. - // Will we need something similar to the RStudio model, - // where we implement custom print() methods? Or can - // we make the stub below behave sensibly even when - // streaming R output? - let mut data = serde_json::Map::new(); - - // The output generated by autoprint is emitted as an - // `execute_result` message. - let mut autoprint = std::mem::take(&mut self.autoprint_output); - - if autoprint.ends_with('\n') { - // Remove the trailing newlines that R adds to outputs but that - // Jupyter frontends are not expecting. Is it worth taking a - // mutable self ref across calling methods to avoid the clone? - autoprint.pop(); - } - if autoprint.len() != 0 { - data.insert("text/plain".to_string(), json!(autoprint)); - } - - // Include HTML representation of data.frame - unsafe { - let value = Rf_findVarInFrame(R_GlobalEnv, r_symbol!(".Last.value")); - if r_is_data_frame(value) { - match to_html(value) { - Ok(html) => data.insert("text/html".to_string(), json!(html)), - Err(err) => { - log::error!("{:?}", err); - None - }, - }; - } + if let Some(result) = result { + self.iopub_tx.send(result).unwrap(); } - let reply = new_execute_reply(exec_count); - - let result = (data.len() > 0).then(|| { - IOPubMessage::ExecuteResult(ExecuteResult { - execution_count: exec_count, - data: serde_json::Value::Object(data), - metadata: json!({}), - }) - }); - - (reply, result) + log::trace!("Sending `execute_reply`: {reply:?}"); + req.reply_tx.send(reply).unwrap(); } /// Sends a `Wait` message to IOPub, which responds when the IOPub thread @@ -2287,28 +2299,6 @@ impl RMain { } } -/// Report an incomplete request to the frontend -fn new_incomplete_reply(req: &ExecuteRequest, exec_count: u32) -> amalthea::Result { - let error = Exception { - ename: "IncompleteInput".to_string(), - evalue: format!("Code fragment is not complete: {}", req.code), - traceback: vec![], - }; - Err(amalthea::Error::ShellErrorExecuteReply(error, exec_count)) -} - -fn new_execute_reply(exec_count: u32) -> amalthea::Result { - Ok(ExecuteReply { - status: Status::Ok, - execution_count: exec_count, - user_expressions: json!({}), - }) -} - -fn new_execute_reply_error(error: Exception, exec_count: u32) -> amalthea::Result { - Err(amalthea::Error::ShellErrorExecuteReply(error, exec_count)) -} - /// Converts a data frame to HTML fn to_html(frame: SEXP) -> Result { unsafe { diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 450f86b89..45cef3b58 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -373,11 +373,13 @@ fn test_execute_request_error() { fn test_execute_request_error_multiple_expressions() { let frontend = DummyArkFrontend::lock(); - frontend.send_execute_request("1\nstop('foobar')\n2", ExecuteRequestOptions::default()); + // `print(2)` and `3` are never evaluated + let code = "1\nstop('foobar')\nprint(2)\n3"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); frontend.recv_iopub_busy(); let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, "1\nstop('foobar')\n2"); + assert_eq!(input.code, code); frontend.recv_iopub_stream_stdout("[1] 1\n"); assert!(frontend.recv_iopub_execute_error().contains("foobar")); From 2040e086bb7da59f65b697e0b165eb8cd03c5e98 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 30 Oct 2025 15:22:48 +0100 Subject: [PATCH 09/44] Cancel pending inputs when we get in the debugger --- crates/ark/src/interface.rs | 6 ++++++ crates/ark/tests/kernel.rs | 30 ++++++++++++++++++++++++++++++ 2 files changed, 36 insertions(+) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 47a91cba7..3bf4694b1 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -839,6 +839,12 @@ impl RMain { // Start or continue debugging with the `debug_preserve_focus` hint // from the last expression we evaluated self.start_debug(self.debug_preserve_focus); + + // Clear any pending inputs, if any. Ideally we'd preserve them and + // run them once the debugging session is over, but that'd require + // keeping a stack of pending expressions and accurately tracking + // the lifetime of nested debug sessions. + self.pending_inputs = None; } else if self.dap.is_debugging() { self.stop_debug(); } diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 45cef3b58..b113081ca 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -350,6 +350,36 @@ fn test_execute_request_browser_stdin() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } +#[test] +fn test_execute_request_browser_pending_cancelled() { + let frontend = DummyArkFrontend::lock(); + + // The `print()` call should be cancelled when we get in the debugger + let code = "browser()\nprint('hello')"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + // We don't get any output for "hello" + frontend.recv_iopub_stream_stdout("Called from: top level \n"); + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + let code = "Q"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + #[test] fn test_execute_request_error() { let frontend = DummyArkFrontend::lock(); From 4bae7706d1049556718b0fbf23bead7ff339b690 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 30 Oct 2025 15:37:59 +0100 Subject: [PATCH 10/44] Add test for invalid syntax --- crates/ark/tests/kernel.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index b113081ca..063fabeb5 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -129,6 +129,30 @@ fn test_execute_request_incomplete_multiple_lines() { ) } +#[test] +fn test_execute_request_invalid() { + let frontend = DummyArkFrontend::lock(); + + let code = "1 + )"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert_eq!( + frontend.recv_iopub_execute_error(), + "Error:\n\nSyntax error: unexpected ')'" + ); + + frontend.recv_iopub_idle(); + + assert_eq!( + frontend.recv_shell_execute_reply_exception(), + input.execution_count + ) +} + #[test] fn test_execute_request_browser() { let frontend = DummyArkFrontend::lock(); From 019cba0dfc05e421871ab9afd3c1b0b10a97073d Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Oct 2025 11:32:05 +0100 Subject: [PATCH 11/44] Tweak documentation --- .../amalthea/src/fixtures/dummy_frontend.rs | 2 ++ crates/ark/src/interface.rs | 34 +++++++++++-------- crates/ark/tests/kernel.rs | 4 +-- 3 files changed, 24 insertions(+), 16 deletions(-) diff --git a/crates/amalthea/src/fixtures/dummy_frontend.rs b/crates/amalthea/src/fixtures/dummy_frontend.rs index 8d427a9d9..b6520ab7b 100644 --- a/crates/amalthea/src/fixtures/dummy_frontend.rs +++ b/crates/amalthea/src/fixtures/dummy_frontend.rs @@ -247,6 +247,8 @@ impl DummyFrontend { // // Note that the panic hook will still have run to record the panic, so we'll get // expected panic information in the test output. + // + // If you're debugging tests, you'll need to bump this timeout to a large value. if socket.poll_incoming(10000).unwrap() { return Message::read_from_socket(socket).unwrap(); } diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 3bf4694b1..d0a4b0028 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -270,10 +270,15 @@ pub struct RMain { debug_session_index: u32, } +/// Stack of pending inputs struct PendingInputs { + /// EXPRSXP vector of parsed expressions exprs: RObject, + /// List of srcrefs, the same length as `exprs` srcrefs: RObject, + /// Length of `exprs` and `srcrefs` len: isize, + /// Index into the stack index: isize, } @@ -883,6 +888,8 @@ impl RMain { // Signal prompt EVENTS.console_prompt.emit(()); + // --- Event loop part of ReadConsole + let mut select = crossbeam::channel::Select::new(); // Cloning is necessary to avoid a double mutable borrow error @@ -1144,10 +1151,8 @@ impl RMain { // so, we `take()` and clear the `active_request` as we're about to // complete it and send a reply to unblock the active Shell request. if let Some(req) = std::mem::take(&mut self.active_request) { - // FIXME: Race condition between the comm and shell socket threads. - // - // Perform a refresh of the frontend state - // (Prompts, working directory, etc) + // Perform a refresh of the frontend state (Prompts, working + // directory, etc) self.with_mut_ui_comm_tx(|ui_comm_tx| { let input_prompt = info.input_prompt.clone(); let continuation_prompt = info.continuation_prompt.clone(); @@ -1168,6 +1173,9 @@ impl RMain { } } + // Called from Ark's ReadConsole event loop when we get a new execute + // request. It's not possible to get one while an active request is ongoing + // because of Jupyter's queueing of Shell messages. fn handle_execute_request( &mut self, req: RRequest, @@ -1339,7 +1347,7 @@ impl RMain { srcref: libr::SEXP, buf: *mut c_uchar, buflen: c_int, - ) -> Option<()> { + ) { // SAFETY: This may jump in case of error, keep this POD unsafe { // The global source reference is stored in this global variable by @@ -1370,8 +1378,6 @@ impl RMain { // Unwrap safety: The input always fits in the buffer Self::on_console_input(buf, buflen, code).unwrap(); - - Some(()) } /// Handle an `input_request` received outside of an `execute_request` context @@ -2291,11 +2297,11 @@ impl RMain { } } - fn propagate_error(&mut self, err: anyhow::Error) -> ! { + fn propagate_error(&mut self, message: String) -> ! { // Save error message to `RMain`'s buffer to avoid leaking memory when `Rf_error()` jumps. // Some gymnastics are required to deal with the possibility of `CString` conversion failure // since the error message comes from the frontend and might be corrupted. - self.r_error_buffer = Some(new_cstring(format!("\n{err}"))); + self.r_error_buffer = Some(new_cstring(message)); unsafe { Rf_error(self.r_error_buffer.as_ref().unwrap().as_ptr()) } } @@ -2362,13 +2368,13 @@ pub extern "C-unwind" fn r_read_console( let PendingInput { expr, srcref } = input; unsafe { + // The pointer protection stack is restored by `run_Rmainloop()` + // after a longjump to top-level, so it's safe to protect here + // even if the evaluation throws let expr = expr.into_protected(); let srcref = srcref.into_protected(); - match main.eval_pending(expr, srcref, buf, buflen) { - None => todo!(), - Some(()) => Some(ConsoleResult::NewInput), - }; + main.eval_pending(expr, srcref, buf, buflen); libr::Rf_unprotect(2); return 1; @@ -2390,7 +2396,7 @@ pub extern "C-unwind" fn r_read_console( }, ConsoleResult::Error(err) => { - main.propagate_error(anyhow::anyhow!("{err}")); + main.propagate_error(format!("{err}")); }, }; } diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 063fabeb5..b25aa3d31 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -142,7 +142,7 @@ fn test_execute_request_invalid() { assert_eq!( frontend.recv_iopub_execute_error(), - "Error:\n\nSyntax error: unexpected ')'" + "Error:\nSyntax error: unexpected ')'" ); frontend.recv_iopub_idle(); @@ -257,7 +257,7 @@ fn test_execute_request_browser_incomplete() { let input = frontend.recv_iopub_execute_input(); assert_eq!(input.code, code); - frontend.recv_iopub_stream_stderr("Error: \nCan't execute incomplete input:\n1 +\n"); + frontend.recv_iopub_stream_stderr("Error: Can't execute incomplete input:\n1 +\n"); frontend.recv_iopub_idle(); assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); From 7c3c16851e635669b9cffb351481194f5b409c3d Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Oct 2025 12:05:43 +0100 Subject: [PATCH 12/44] Remove `into_protected()` method --- crates/ark/src/interface.rs | 4 ++-- crates/harp/src/object.rs | 9 --------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index d0a4b0028..c7eb97ef1 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -2371,8 +2371,8 @@ pub extern "C-unwind" fn r_read_console( // The pointer protection stack is restored by `run_Rmainloop()` // after a longjump to top-level, so it's safe to protect here // even if the evaluation throws - let expr = expr.into_protected(); - let srcref = srcref.into_protected(); + let expr = libr::Rf_protect(expr.into()); + let srcref = libr::Rf_protect(srcref.into()); main.eval_pending(expr, srcref, buf, buflen); diff --git a/crates/harp/src/object.rs b/crates/harp/src/object.rs index 5ce34a236..8d4f3eb8e 100644 --- a/crates/harp/src/object.rs +++ b/crates/harp/src/object.rs @@ -325,15 +325,6 @@ impl RObject { } } - /// Consume the `RObject` and return the underlying `SEXP`, protected via - /// `Rf_protect`. - pub unsafe fn into_protected(self) -> SEXP { - unsafe { - libr::Rf_protect(self.sexp); - } - self.sexp - } - pub fn view(data: SEXP) -> Self { RObject { sexp: data, From 69ca0dceaf9e7cd37d95e63b4abc90a2cf564805 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Oct 2025 12:18:09 +0100 Subject: [PATCH 13/44] Make `reply_execute_request()` a free function --- crates/ark/src/interface.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index c7eb97ef1..52b748426 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -1167,7 +1167,7 @@ impl RMain { // Let frontend know the last request is complete. This turns us // back to Idle. - self.reply_execute_request(req, &info, value); + Self::reply_execute_request(&self.iopub_tx, req, &info, value); } else { log::info!("No active request to handle, discarding: {value:?}"); } @@ -1833,7 +1833,7 @@ impl RMain { // Reply to the previously active request. The current prompt type and // whether an error has occurred defines the reply kind. fn reply_execute_request( - &mut self, + iopub_tx: &Sender, req: ActiveReadConsoleRequest, prompt_info: &PromptInfo, value: ConsoleValue, @@ -1877,7 +1877,7 @@ impl RMain { }; if let Some(result) = result { - self.iopub_tx.send(result).unwrap(); + iopub_tx.send(result).unwrap(); } log::trace!("Sending `execute_reply`: {reply:?}"); From cad9a3fcca7e86ca8f894a86bd2581e6265106b0 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Oct 2025 12:26:32 +0100 Subject: [PATCH 14/44] Create Jupyter exception in the global condition handler --- crates/ark/src/errors.rs | 14 +++++++++++--- crates/ark/src/interface.rs | 25 ++++++------------------- 2 files changed, 17 insertions(+), 22 deletions(-) diff --git a/crates/ark/src/errors.rs b/crates/ark/src/errors.rs index 0b98aee32..960d1b296 100644 --- a/crates/ark/src/errors.rs +++ b/crates/ark/src/errors.rs @@ -5,6 +5,7 @@ // // +use amalthea::wire::exception::Exception; use harp::exec::r_peek_error_buffer; use harp::exec::RE_STACK_OVERFLOW; use harp::object::RObject; @@ -39,9 +40,16 @@ unsafe extern "C-unwind" fn ps_record_error(evalue: SEXP, traceback: SEXP) -> an Vec::::new() }); - main.error_occurred = true; - main.error_message = evalue; - main.error_traceback = traceback; + main.last_error = Some( + // We don't fill out `ename` with anything meaningful because typically + // R errors don't have names. We could consider using the condition class + // here, which r-lib/tidyverse packages have been using more heavily. + Exception { + ename: String::from(""), + evalue, + traceback, + }, + ); Ok(R_NilValue) } diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 52b748426..c0619c6fd 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -215,10 +215,9 @@ pub struct RMain { /// by forwarding them through the UI comm. Optional, and really Positron specific. ui_comm_tx: Option, - /// Represents whether an error occurred during R code execution. - pub error_occurred: bool, - pub error_message: String, // `evalue` in the Jupyter protocol - pub error_traceback: Vec, + /// Error captured by our global condition handler during the last iteration + /// of the REPL. + pub(crate) last_error: Option, /// Channel to communicate with the Help thread help_event_tx: Option>, @@ -687,9 +686,7 @@ impl RMain { execution_count: 0, autoprint_output: String::new(), ui_comm_tx: None, - error_occurred: false, - error_message: String::new(), - error_traceback: Vec::new(), + last_error: None, help_event_tx: None, help_port: None, lsp_events_tx: None, @@ -1093,18 +1090,8 @@ impl RMain { } fn take_exception(&mut self) -> Option { - let mut exception = if self.error_occurred { - // Reset flag - self.error_occurred = false; - - // We don't fill out `ename` with anything meaningful because typically - // R errors don't have names. We could consider using the condition class - // here, which r-lib/tidyverse packages have been using more heavily. - Exception { - ename: String::from(""), - evalue: self.error_message.clone(), - traceback: self.error_traceback.clone(), - } + let mut exception = if let Some(exception) = self.last_error.take() { + exception } else if stack_overflow_occurred() { // Call `base::traceback()` since we don't have a handled error // object carrying a backtrace. This won't be formatted as a From e884469a88f6847165779e7e89a8a1792a2e2520 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Oct 2025 12:40:11 +0100 Subject: [PATCH 15/44] Fully remove incomplete prompts heuristic since they are now impossible --- crates/ark/src/interface.rs | 26 +++----------------------- 1 file changed, 3 insertions(+), 23 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index c0619c6fd..2c08824f7 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -392,9 +392,6 @@ pub struct PromptInfo { /// incomplete but is never a user request. browser: bool, - /// Whether the last input didn't fully parse and R is waiting for more input - incomplete: bool, - /// Whether this is a prompt from a fresh REPL iteration (browser or /// top level) or a prompt from some user code, e.g. via `readline()` input_request: bool, @@ -829,12 +826,6 @@ impl RMain { let info = self.prompt_info(prompt); log::trace!("R prompt: {}", info.input_prompt); - // Since we parse expressions ourselves and only send complete inputs to - // the base REPL, we never should be asked for completing input - if info.incomplete { - unreachable!("Incomplete input in `ReadConsole` handler"); - } - // Invariant: If we detect a browser prompt, `self.dap.is_debugging()` // is true. Otherwise it is false. if info.browser { @@ -1013,8 +1004,9 @@ impl RMain { let prompt_slice = unsafe { CStr::from_ptr(prompt_c) }; let prompt = prompt_slice.to_string_lossy().into_owned(); + // Sent to the frontend after each top-level command so users can + // customise their prompts let continuation_prompt: String = harp::get_option("continue").try_into().unwrap(); - let matches_continuation = prompt == continuation_prompt; // Detect browser prompt by matching the prompt string // https://github.com/posit-dev/positron/issues/4742. @@ -1022,28 +1014,16 @@ impl RMain { // `options(prompt =, continue = ` to something that looks like // a browser prompt, or doing the same with `readline()`. We have // chosen to not support these edge cases. - // Additionally, we send code to R one line at a time, so even if we are debugging - // it can look like we are in a continuation state. To try and detect that, we - // detect if we matched the continuation prompt while the DAP is active. - let browser = - RE_DEBUG_PROMPT.is_match(&prompt) || (self.dap.is_debugging() && matches_continuation); + let browser = RE_DEBUG_PROMPT.is_match(&prompt); // If there are frames on the stack and we're not in a browser prompt, // this means some user code is requesting input, e.g. via `readline()` let user_request = !browser && n_frame > 0; - // The request is incomplete if we see the continue prompt, except if - // we're in a user request, e.g. `readline("+ ")`. To guard against - // this, we check that we are at top-level (call stack is empty or we - // have a debug prompt). - let top_level = n_frame == 0 || browser; - let incomplete = matches_continuation && top_level; - return PromptInfo { input_prompt: prompt, continuation_prompt, browser, - incomplete, input_request: user_request, }; } From e6ec6e41c134663440dcda776d885546bdbdb6c2 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Oct 2025 14:13:18 +0100 Subject: [PATCH 16/44] Extract ReadConsole event loop into method --- crates/ark/src/interface.rs | 93 ++++++++++++++++++++++++------------- 1 file changed, 61 insertions(+), 32 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 2c08824f7..7c5a83133 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -351,6 +351,11 @@ enum ConsoleValue { Error(Exception), } +enum WaitFor { + InputReply, + ExecuteRequest, +} + /// Represents the currently active execution request from the frontend. It /// resolves at the next invocation of the `ReadConsole()` frontend method. struct ActiveReadConsoleRequest { @@ -809,11 +814,16 @@ impl RMain { /// * `prompt` - The prompt shown to the user /// * `buf` - Pointer to buffer to receive the user's input (type `CONSOLE_BUFFER_CHAR`) /// * `buflen` - Size of the buffer to receiver user's input - /// * `hist` - Whether to add the input to the history (1) or not (0) + /// * `_hist` - Whether to add the input to the history (1) or not (0) /// - /// Returns a tuple. First value is to be passed on to `ReadConsole()` and - /// indicates whether new input is available. Second value indicates whether - /// we need to call `Rf_onintr()` to process an interrupt. + /// This does two things: + /// - Move the Console state machine to the next state: + /// - Wait for input + /// - Set an active execute request and a list of pending expressions + /// - Set `self.dap.is_debugging()` depending on presence or absence of debugger prompt + /// - Evaluate next pending expression + /// - Close active execute request if pending list is empty + /// - Run an event loop while waiting for input fn read_console( &mut self, prompt: *const c_char, @@ -823,6 +833,8 @@ impl RMain { ) -> ConsoleResult { self.dap.handle_read_console(); + // State machine part of ReadConsole + let info = self.prompt_info(prompt); log::trace!("R prompt: {}", info.input_prompt); @@ -849,14 +861,13 @@ impl RMain { // Reply to active request with error self.handle_active_request(&info, ConsoleValue::Error(exception)); } else if info.input_request { - if let Some(input) = self.handle_input_request(&info.input_prompt, buf, buflen) { - return input; - } + // Request input reply to the frontend and return it to R + return self.handle_input_request(&info, buf, buflen); } else if let Some(input) = self.pop_pending() { // Evaluate pending expression if there is any remaining return self.handle_pending_input(input, buf, buflen); } else { - // Otherwise close active request + // Otherwise reply to active request with accumulated result let result = self.take_result(); self.handle_active_request(&info, ConsoleValue::Success(result)); } @@ -876,8 +887,23 @@ impl RMain { // Signal prompt EVENTS.console_prompt.emit(()); - // --- Event loop part of ReadConsole + self.run_event_loop(&info, buf, buflen, WaitFor::ExecuteRequest) + } + /// Runs the ReadConsole event loop. + /// This handles events for: + /// - Reception of either input replies or execute requests (as determined + /// by `wait_for`) + /// - Idle-time and interrupt-time tasks + /// - Requests from the frontend (currently only used for establishing UI comm) + /// - R's polled events + fn run_event_loop( + &mut self, + info: &PromptInfo, + buf: *mut c_uchar, + buflen: c_int, + wait_for: WaitFor, + ) -> ConsoleResult { let mut select = crossbeam::channel::Select::new(); // Cloning is necessary to avoid a double mutable borrow error @@ -893,8 +919,14 @@ impl RMain { // package. 50ms seems to be more in line with RStudio (posit-dev/positron#7235). let polled_events_rx = crossbeam::channel::tick(Duration::from_millis(50)); - let r_request_index = select.recv(&r_request_rx); - let stdin_reply_index = select.recv(&stdin_reply_rx); + // This is the main kind of message from the frontend that we are expecting. + // We either wait for `input_reply` messages on StdIn, or for + // `execute_request` on Shell. + let (r_request_index, stdin_reply_index) = match wait_for { + WaitFor::ExecuteRequest => (Some(select.recv(&r_request_rx)), None), + WaitFor::InputReply => (None, Some(select.recv(&stdin_reply_rx))), + }; + let kernel_request_index = select.recv(&kernel_request_rx); let tasks_interrupt_index = select.recv(&tasks_interrupt_rx); let polled_events_index = select.recv(&polled_events_rx); @@ -929,17 +961,13 @@ impl RMain { // reset the flag set_interrupts_pending(false); - // FIXME: Race between interrupt and new code request. To fix - // this, we could manage the Shell and Control sockets on the - // common message event thread. The Control messages would need - // 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) = r_request_rx.try_recv() { - if let Some(input) = self.handle_execute_request(req, &info, buf, buflen) { - return input; + if let WaitFor::ExecuteRequest = wait_for { + if let Ok(req) = r_request_rx.try_recv() { + if let Some(input) = self.handle_execute_request(req, &info, buf, buflen) { + return input; + } } } @@ -947,7 +975,7 @@ impl RMain { match oper.index() { // We've got an execute request from the frontend - i if i == r_request_index => { + i if Some(i) == r_request_index => { let req = oper.recv(&r_request_rx); let Ok(req) = req else { // The channel is disconnected and empty @@ -960,7 +988,7 @@ impl RMain { }, // We've got a reply for readline - i if i == stdin_reply_index => { + i if Some(i) == stdin_reply_index => { let reply = oper.recv(&stdin_reply_rx).unwrap(); return self.handle_input_reply(reply, buf, buflen); }, @@ -1212,20 +1240,19 @@ impl RMain { } /// Handles user input requests (e.g., readline, menu) and special prompts. - /// Returns `Some()` if this handler needs to return to the base R REPL, or - /// `None` if it needs to run Ark's `ReadConsole` event loop. + /// Runs the ReadConsole event loop until a reply comes in. fn handle_input_request( &mut self, - input_prompt: &str, + info: &PromptInfo, buf: *mut c_uchar, buflen: c_int, - ) -> Option { + ) -> ConsoleResult { // If the prompt begins with "Save workspace", respond with (n) // and allow R to immediately exit. - if input_prompt.starts_with("Save workspace") { + if info.input_prompt.starts_with("Save workspace") { match Self::on_console_input(buf, buflen, String::from("n")) { - Ok(()) => return Some(ConsoleResult::NewInput), - Err(err) => return Some(ConsoleResult::Error(err)), + Ok(()) => return ConsoleResult::NewInput, + Err(err) => return ConsoleResult::Error(err), } } @@ -1233,11 +1260,13 @@ impl RMain { // Send request to frontend. We'll wait for an `input_reply` // from the frontend in the event loop in `read_console()`. // The active request remains active. - self.request_input(req.originator.clone(), String::from(input_prompt)); - None + self.request_input(req.originator.clone(), String::from(&info.input_prompt)); + + // Run the event loop, waiting for stdin replies but not execute requests + self.run_event_loop(info, buf, buflen, WaitFor::InputReply) } else { // Invalid input request, propagate error to R - Some(self.handle_invalid_input_request(buf, buflen)) + self.handle_invalid_input_request(buf, buflen) } } From acf96e4b0ad1e53066b577b80b34efd79b61477c Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Oct 2025 16:25:59 +0100 Subject: [PATCH 17/44] Return to base REPL in case of error I couldn't produce any issues with `R_EvalDepth` but it seems more sound to let R reset state before evaluation --- crates/ark/src/interface.rs | 32 ++++++++ crates/ark/src/modules/positron/errors.R | 8 ++ crates/ark/tests/kernel.rs | 94 ++++++++++++++++++++++++ 3 files changed, 134 insertions(+) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 7c5a83133..adbdfb67f 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -219,6 +219,8 @@ pub struct RMain { /// of the REPL. pub(crate) last_error: Option, + console_need_reset: bool, + /// Channel to communicate with the Help thread help_event_tx: Option>, /// R help port @@ -340,6 +342,7 @@ impl PendingInputs { } } +#[derive(Debug)] pub(crate) struct PendingInput { expr: RObject, srcref: RObject, @@ -407,6 +410,7 @@ pub enum ConsoleInput { Input(String), } +#[derive(Debug)] pub(crate) enum ConsoleResult { NewInput, NewPendingInput(PendingInput), @@ -560,6 +564,9 @@ impl RMain { if let Err(err) = apply_default_repos(default_repos) { log::error!("Error setting default repositories: {err:?}"); } + + // Initialise Ark's last value + libr::SETCDR(r_symbol!(".ark_last_value"), harp::r_null()); } // Now that R has started (emitting any startup messages that we capture in the @@ -707,6 +714,7 @@ impl RMain { debug_env: None, debug_session_index: 1, pending_inputs: None, + console_need_reset: false, } } @@ -2349,6 +2357,29 @@ pub extern "C-unwind" fn r_read_console( hist: c_int, ) -> i32 { let main = RMain::get_mut(); + + // In case of error, we haven't had a chance to evaluate ".ark_last_value". + // So we return to the R REPL to give R a chance to run the state + // restoration that occurs between `R_ReadConsole()` and `eval()`: + // - R_PPStackTop: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L227 + // - R_EvalDepth: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L260 + // + // Technically this also resets time limits (see `base::setTimeLimit()`) but + // these aren't supported in Ark because they cause errors when we poll R + // events. + if main.last_error.is_some() && main.console_need_reset { + main.console_need_reset = false; + + // Evaluate last value so that `base::.Last.value` remains the same + RMain::on_console_input( + buf, + buflen, + String::from("base::invisible(base::.ark_last_value)"), + ) + .unwrap(); + return 1; + } + let result = r_sandbox(|| main.read_console(prompt, buf, buflen, hist)); main.read_console_cleanup(); @@ -2370,6 +2401,7 @@ pub extern "C-unwind" fn r_read_console( let expr = libr::Rf_protect(expr.into()); let srcref = libr::Rf_protect(srcref.into()); + main.console_need_reset = true; main.eval_pending(expr, srcref, buf, buflen); libr::Rf_unprotect(2); diff --git a/crates/ark/src/modules/positron/errors.R b/crates/ark/src/modules/positron/errors.R index 11e082cb5..12263fa76 100644 --- a/crates/ark/src/modules/positron/errors.R +++ b/crates/ark/src/modules/positron/errors.R @@ -33,6 +33,14 @@ #' @export .ps.errors.globalErrorHandler <- function(cnd) { + # Unlike C stack overflow errors, expressions nested too deeply errors allow + # calling handlers. But since we run R code, we need to temporarily bump the + # threshold to give a little room while we handle the error. + if (inherits(cnd, "expressionStackOverflowError")) { + old <- options(expressions = getOption("expressions") + 500) + defer(options(old)) + } + # This reproduces the behaviour of R's default error handler: # - Invoke `getOption("error")` # - Save backtrace for `traceback()` diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index b25aa3d31..db92da3bf 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -423,6 +423,100 @@ fn test_execute_request_error() { ); } +#[test] +fn test_execute_request_error_expressions_overflow() { + let frontend = DummyArkFrontend::lock(); + // Deterministically produce an "evaluation too deeply nested" error + let code = "options(expressions = 100); f <- function(x) if (x > 0 ) f(x - 1); f(100)"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend + .recv_iopub_execute_error() + .contains("evaluation nested too deeply")); + + frontend.recv_iopub_idle(); + + assert_eq!( + frontend.recv_shell_execute_reply_exception(), + input.execution_count + ); + + // Check we can still evaluate without causing another too deeply nested error + let code = "f(10)"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + +#[test] +fn test_execute_request_error_expressions_overflow_last_value() { + let frontend = DummyArkFrontend::lock(); + + // Set state and last value + let code = + "options(expressions = 100); f <- function(x) if (x > 0 ) f(x - 1); invisible('hello')"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Check last value is set + let code = ".Last.value"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"hello\""); + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Deterministically produce an "evaluation too deeply nested" error + let code = "f(100)"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend + .recv_iopub_execute_error() + .contains("evaluation nested too deeply")); + + frontend.recv_iopub_idle(); + + assert_eq!( + frontend.recv_shell_execute_reply_exception(), + input.execution_count + ); + + // Check last value is still set + let code = ".Last.value"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"hello\""); + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + #[test] fn test_execute_request_error_multiple_expressions() { let frontend = DummyArkFrontend::lock(); From 30d244b3608d49803ed61cab4e35e0ce9e5b12aa Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Oct 2025 17:33:05 +0100 Subject: [PATCH 18/44] Rename `eval_pending()` to `eval()` --- crates/ark/src/interface.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index adbdfb67f..833dff7e7 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -1345,7 +1345,7 @@ impl RMain { } // SAFETY: Call this from a POD frame. Inputs must be protected. - unsafe fn eval_pending( + unsafe fn eval( &mut self, expr: libr::SEXP, srcref: libr::SEXP, @@ -2402,7 +2402,7 @@ pub extern "C-unwind" fn r_read_console( let srcref = libr::Rf_protect(srcref.into()); main.console_need_reset = true; - main.eval_pending(expr, srcref, buf, buflen); + main.eval(expr, srcref, buf, buflen); libr::Rf_unprotect(2); return 1; From fde428eeec0f738141d7a472ae9311cc3a98d493 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 31 Oct 2025 18:00:28 +0100 Subject: [PATCH 19/44] Don't clear pending expressions in browser sessions Would be hard to get right in the case of nested browser sessions --- crates/ark/src/interface.rs | 6 ------ crates/ark/tests/kernel.rs | 39 +++++++++++++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 10 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 833dff7e7..c6177afeb 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -852,12 +852,6 @@ impl RMain { // Start or continue debugging with the `debug_preserve_focus` hint // from the last expression we evaluated self.start_debug(self.debug_preserve_focus); - - // Clear any pending inputs, if any. Ideally we'd preserve them and - // run them once the debugging session is over, but that'd require - // keeping a stack of pending expressions and accurately tracking - // the lifetime of nested debug sessions. - self.pending_inputs = None; } else if self.dap.is_debugging() { self.stop_debug(); } diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index db92da3bf..547be5637 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -375,23 +375,54 @@ fn test_execute_request_browser_stdin() { } #[test] -fn test_execute_request_browser_pending_cancelled() { +fn test_execute_request_browser_multiple_expressions() { let frontend = DummyArkFrontend::lock(); - // The `print()` call should be cancelled when we get in the debugger - let code = "browser()\nprint('hello')"; + // Ideally the evaluation of `1` would be cancelled + let code = "browser()\n1"; frontend.send_execute_request(code, ExecuteRequestOptions::default()); frontend.recv_iopub_busy(); let input = frontend.recv_iopub_execute_input(); assert_eq!(input.code, code); - // We don't get any output for "hello" frontend.recv_iopub_stream_stdout("Called from: top level \n"); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] 1"); + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Even if we could cancel pending expressions, it would still be possible + // to run multiple expressions in a debugger prompt + let code = "1\n2"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_stream_stdout("[1] 1\n"); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] 2"); frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // But getting in a nested browser session with a pending expression would + // cancel it (not the case currently) + let code = "browser()\n1"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_stream_stdout("Called from: top level \n"); + + assert_eq!(frontend.recv_iopub_execute_result(), "[1] 1"); + frontend.recv_iopub_idle(); assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + // Quit session let code = "Q"; frontend.send_execute_request(code, ExecuteRequestOptions::default()); frontend.recv_iopub_busy(); From 13f1fd01a750625334adb047d27a9c6fe035b26b Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 4 Nov 2025 10:41:35 +0100 Subject: [PATCH 20/44] Tweak docs --- crates/ark/src/interface.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index c6177afeb..784c554ed 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -388,7 +388,7 @@ pub struct PromptInfo { input_prompt: String, /// The continuation prompt string when user supplies incomplete - /// inputs. This always corresponds to `getOption("continue"). We send + /// inputs. This always corresponds to `getOption("continue")`. We send /// it to frontends along with `prompt` because some frontends such as /// Positron do not send incomplete inputs to Ark and take charge of /// continuation prompts themselves. For frontends that can send @@ -396,8 +396,8 @@ pub struct PromptInfo { /// error on them rather than requesting that this be shown. continuation_prompt: String, - /// Whether this is a `browser()` prompt. A browser prompt can be - /// incomplete but is never a user request. + /// Whether this is a `browser()` prompt. A browser prompt is never a user + /// request. browser: bool, /// Whether this is a prompt from a fresh REPL iteration (browser or From 666a727eeb18e29c267d64748cce59e5bfa61aee Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 4 Nov 2025 10:42:26 +0100 Subject: [PATCH 21/44] Add failing test --- crates/ark/tests/kernel.rs | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 547be5637..d6dc903e2 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -184,6 +184,37 @@ fn test_execute_request_browser() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } +#[test] +fn test_execute_request_browser_continue() { + let frontend = DummyArkFrontend::lock(); + + let code = "browser()"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend + .recv_iopub_execute_result() + .contains("Called from: top level")); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + let code = "n"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + #[test] fn test_execute_request_browser_error() { // The behaviour for errors is different in browsers than at top-level From fb69d6bbaa47cf5f4f12d42a1421b89931661b87 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 4 Nov 2025 13:02:28 +0100 Subject: [PATCH 22/44] `RMain::eval()` doesn't need self ref --- crates/ark/src/interface.rs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 784c554ed..32d76ee2e 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -1339,13 +1339,7 @@ impl RMain { } // SAFETY: Call this from a POD frame. Inputs must be protected. - unsafe fn eval( - &mut self, - expr: libr::SEXP, - srcref: libr::SEXP, - buf: *mut c_uchar, - buflen: c_int, - ) { + unsafe fn eval(expr: libr::SEXP, srcref: libr::SEXP, buf: *mut c_uchar, buflen: c_int) { // SAFETY: This may jump in case of error, keep this POD unsafe { // The global source reference is stored in this global variable by @@ -2396,7 +2390,7 @@ pub extern "C-unwind" fn r_read_console( let srcref = libr::Rf_protect(srcref.into()); main.console_need_reset = true; - main.eval(expr, srcref, buf, buflen); + RMain::eval(expr, srcref, buf, buflen); libr::Rf_unprotect(2); return 1; From 02a8aa403de86c3506916eb4ec69e43e9251add1 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 4 Nov 2025 13:55:57 +0100 Subject: [PATCH 23/44] Add `harp::exec_with_cleanup()` --- crates/harp/src/exec.rs | 145 ++++++++++++++++++++++++++++++++++++++++ crates/libr/src/r.rs | 7 ++ 2 files changed, 152 insertions(+) diff --git a/crates/harp/src/exec.rs b/crates/harp/src/exec.rs index 808a954da..672fcaec8 100644 --- a/crates/harp/src/exec.rs +++ b/crates/harp/src/exec.rs @@ -367,6 +367,101 @@ where } } +/// Execute a function with a cleanup handler using R's cleanup mechanism. +/// +/// This wraps `R_ExecWithCleanup` to provide execution with guaranteed cleanup, +/// even in case of an R longjump. +/// +/// In case of longjump, `cleanup()` runs but `exec_with_cleanup()` does not +/// return, the lonjump propagates. +/// +/// Note that `fun` and `cleanup` must be longjump-safe: +/// - Only POD types without Drop destructors on the stack +/// - Or protects itself from longjumps via e.g. `try_catch()` +/// ``` +pub fn exec_with_cleanup<'env, F, C, T>(fun: F, cleanup: C) -> T +where + F: FnOnce() -> T, + F: 'env, + C: FnOnce(), + C: 'env, + T: 'env, +{ + struct CleanupData<'a, F, C, T> + where + F: FnOnce() -> T + 'a, + C: FnOnce() + 'a, + { + // slot for the result of the closure + result: &'a mut Option, + closure: Option, + cleanup: Option, + } + + // Allocate stack memory for the result + let mut result: Option = None; + + // Move closures to the payload + let mut callback_data = CleanupData { + result: &mut result, + closure: Some(fun), + cleanup: Some(cleanup), + }; + let payload = &mut callback_data as *mut _ as *mut c_void; + + extern "C-unwind" fn exec_callback<'env, F, C, T>(data: *mut c_void) -> SEXP + where + F: FnOnce() -> T, + F: 'env, + C: FnOnce(), + C: 'env, + T: 'env, + { + // SAFETY: `data` points to a `CleanupData` allocated on the caller's stack. + let data: &mut CleanupData = unsafe { &mut *(data as *mut CleanupData) }; + + // Move closure here so it can be called. Required since that's an `FnOnce`. + let closure = take(&mut data.closure).unwrap(); + + // Call closure and store the result in the payload + let result = closure(); + *(data.result) = Some(result); + + // Always return R_NilValue to R_ExecWithCleanup; the real result is in `payload`. + unsafe { R_NilValue } + } + + extern "C-unwind" fn cleanup_callback<'env, F, C, T>(data: *mut c_void) + where + F: FnOnce() -> T, + F: 'env, + C: FnOnce(), + C: 'env, + T: 'env, + { + // SAFETY: `data` points to a `CleanupData` allocated on the caller's stack. + let data: &mut CleanupData = unsafe { &mut *(data as *mut CleanupData) }; + + // Move cleanup closure here so it can be called + if let Some(cleanup) = take(&mut data.cleanup) { + cleanup(); + } + } + + // Call into R; the callbacks will populate `res` and always return R_NilValue. + unsafe { + R_ExecWithCleanup( + Some(exec_callback::), + payload, + Some(cleanup_callback::), + payload, + ) + }; + + // Unwrap Safety: If we get here, we're in the happy path and the result is Some + result.unwrap() +} + pub fn r_peek_error_buffer() -> String { // SAFETY: Returns pointer to static memory buffer owned by R. let buffer = unsafe { R_curErrorBuf() }; @@ -495,6 +590,8 @@ pub fn r_check_stack(size: Option) -> Result<()> { #[cfg(test)] mod tests { use std::ffi::CString; + use std::sync::Arc; + use std::sync::Mutex; use stdext::assert_match; @@ -644,4 +741,52 @@ mod tests { }); }) } + + #[test] + fn test_exec_with_cleanup() { + crate::r_task(|| { + let cleanup_called = Arc::new(Mutex::new(false)); + let cleanup_called_clone = cleanup_called.clone(); + + let result = exec_with_cleanup( + || { + // Create a simple R object and return it directly (T = RObject) + let obj = RObject::from(unsafe { Rf_ScalarInteger(42) }); + obj + }, + || { + *cleanup_called_clone.lock().unwrap() = true; + }, + ); + + assert_eq!(unsafe { Rf_asInteger(*result) }, 42); + assert!( + *cleanup_called.lock().unwrap(), + "Cleanup should have been called" + ); + + // Test error case - cleanup should still be called. + let cleanup_called_error = Arc::new(Mutex::new(false)); + let cleanup_called_error_clone = cleanup_called_error.clone(); + + let result = try_catch(|| { + exec_with_cleanup( + || -> RObject { + let msg = CString::new("ouch").unwrap(); // This leaks + unsafe { Rf_error(msg.as_ptr()) }; + }, + || { + *cleanup_called_error_clone.lock().unwrap() = true; + }, + ) + }); + + assert!(result.is_err()); + + assert!( + *cleanup_called_error.lock().unwrap(), + "Cleanup should have been called on error" + ); + }) + } } diff --git a/crates/libr/src/r.rs b/crates/libr/src/r.rs index 1c1b15663..8fb8b4e1f 100644 --- a/crates/libr/src/r.rs +++ b/crates/libr/src/r.rs @@ -98,6 +98,13 @@ functions::generate! { data: *mut std::ffi::c_void ) -> Rboolean; + pub fn R_ExecWithCleanup( + fun: Option SEXP>, + data: *mut std::ffi::c_void, + cleanfun: Option, + cleandata: *mut std::ffi::c_void + ) -> SEXP; + pub fn R_withCallingErrorHandler( body: Option SEXP>, bdata: *mut std::ffi::c_void, From acf7965e07427e8dce3e67a5d46af39519512915 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 4 Nov 2025 22:31:08 +0100 Subject: [PATCH 24/44] Keep track of nested REPLs and clean up R's state when needed --- crates/ark/src/interface.rs | 161 ++++++++++++++++++++++++++++++------ crates/ark/tests/kernel.rs | 155 ++++++++++++++++++++++++++++++++++ 2 files changed, 289 insertions(+), 27 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 32d76ee2e..a36d82f5e 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -10,6 +10,7 @@ // The frontend methods called by R are forwarded to the corresponding // `RMain` methods via `R_MAIN`. +use std::cell::Cell; use std::cell::RefCell; use std::cell::UnsafeCell; use std::collections::HashMap; @@ -59,6 +60,7 @@ use harp::command::r_home_setup; use harp::environment::r_ns_env; use harp::environment::Environment; use harp::environment::R_ENVS; +use harp::exec::exec_with_cleanup; use harp::exec::r_check_stack; use harp::exec::r_peek_error_buffer; use harp::exec::r_sandbox; @@ -219,8 +221,6 @@ pub struct RMain { /// of the REPL. pub(crate) last_error: Option, - console_need_reset: bool, - /// Channel to communicate with the Help thread help_event_tx: Option>, /// R help port @@ -269,6 +269,24 @@ pub struct RMain { /// Ever increasing debug session index. Used to create URIs that are only /// valid for a single session. debug_session_index: u32, + + /// Tracks how many nested `r_read_console()` calls are on the stack. + /// Incremented when entering `r_read_console(),` decremented on exit. + read_console_depth: Cell, + + /// Set to true when `r_read_console()` exits. Reset to false at the start + /// of each `r_read_console()` call. Used to detect if `eval()` returned + /// from a nested REPL (the flag will be true when the evaluation returns). + nested_read_console_returned: Cell, + + /// Set to true `r_read_console()` exits via an error longjump. Used to + /// detect if we need to go return from `r_read_console()` with a dummy + /// evaluation to reset things like `R_EvalDepth`. + read_console_threw_error: Cell, + + /// Used to track an input to evaluate upon returning to `r_read_console()`, + /// after having returned a dummy input to reset `R_ConsoleIob` in R's REPL. + next_read_console_input: Cell>, } /// Stack of pending inputs @@ -714,7 +732,10 @@ impl RMain { debug_env: None, debug_session_index: 1, pending_inputs: None, - console_need_reset: false, + read_console_depth: Cell::new(0), + nested_read_console_returned: Cell::new(false), + read_console_threw_error: Cell::new(false), + next_read_console_input: Cell::new(None), } } @@ -1815,6 +1836,13 @@ impl RMain { Ok(()) } + fn console_input(buf: *mut c_uchar, _buflen: c_int) -> String { + unsafe { + let cstr = CStr::from_ptr(buf as *const c_char); + cstr.to_string_lossy().into_owned() + } + } + // Hitting this means a SINGLE line from the user was longer than the buffer size (>4000 characters) fn buffer_overflow_error() -> amalthea::Error { Error::InvalidConsoleInput(String::from( @@ -2344,30 +2372,90 @@ pub extern "C-unwind" fn r_read_console( buflen: c_int, hist: c_int, ) -> i32 { - let main = RMain::get_mut(); - - // In case of error, we haven't had a chance to evaluate ".ark_last_value". - // So we return to the R REPL to give R a chance to run the state - // restoration that occurs between `R_ReadConsole()` and `eval()`: - // - R_PPStackTop: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L227 - // - R_EvalDepth: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L260 + // In this entry point we handle two kinds of state: + // - The number of nested REPLs `read_console_depth` + // - A bunch of flags that help us reset the calling R REPL // - // Technically this also resets time limits (see `base::setTimeLimit()`) but - // these aren't supported in Ark because they cause errors when we poll R - // events. - if main.last_error.is_some() && main.console_need_reset { - main.console_need_reset = false; - - // Evaluate last value so that `base::.Last.value` remains the same - RMain::on_console_input( - buf, - buflen, - String::from("base::invisible(base::.ark_last_value)"), - ) - .unwrap(); - return 1; + // The second kind is unfortunate and due to us taking charge of parsing and + // evaluation. Ideally R would extend their frontend API so that this would + // only be necessary for backward compatibility with old versions of R. + + { + let main = RMain::get_mut(); + + // We've finished evaluating a dummy value to reset state in R's REPL, + // and are now ready to evaluate the actual input + if let Some(next_input) = main.next_read_console_input.take() { + RMain::on_console_input(buf, buflen, next_input).unwrap(); + return 1; + } + + // In case of error, we haven't had a chance to evaluate ".ark_last_value". + // So we return to the R REPL to give R a chance to run the state + // restoration that occurs between `R_ReadConsole()` and `eval()`: + // - R_PPStackTop: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L227 + // - R_EvalDepth: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L260 + // + // Technically this also resets time limits (see `base::setTimeLimit()`) but + // these aren't supported in Ark because they cause errors when we poll R + // events. + if main.last_error.is_some() && main.read_console_threw_error.get() { + main.read_console_threw_error.set(false); + + // Evaluate last value so that `base::.Last.value` remains the same + RMain::on_console_input( + buf, + buflen, + String::from("base::invisible(base::.Last.value)"), + ) + .unwrap(); + return 1; + } + + // Track nesting depth of ReadConsole REPLs + main.read_console_depth + .set(main.read_console_depth.get() + 1); + + // Reset flag that helps us figure out when a nested REPL returns + main.nested_read_console_returned.set(false); + + // Reset flag that helps us figure out when an error occurred and needs a + // reset of `R_EvalDepth` and friends + main.read_console_threw_error.set(true); } + exec_with_cleanup( + || { + let main = RMain::get_mut(); + let result = r_read_console_impl(main, prompt, buf, buflen, hist); + + // If we get here, there was no error + main.read_console_threw_error.set(false); + + result + }, + || { + let main = RMain::get_mut(); + + // We're exiting, decrease depth of nested consoles + main.read_console_depth + .set(main.read_console_depth.get() - 1); + + // Set flag so that parent read console, if any, can detect that a + // nested console returned (if it indeed returns instead of looping + // for another iteration) + main.nested_read_console_returned.set(true); + }, + ) +} + +fn r_read_console_impl( + main: &mut RMain, + prompt: *const c_char, + buf: *mut c_uchar, + buflen: c_int, + hist: c_int, +) -> i32 { let result = r_sandbox(|| main.read_console(prompt, buf, buflen, hist)); main.read_console_cleanup(); @@ -2389,16 +2477,35 @@ pub extern "C-unwind" fn r_read_console( let expr = libr::Rf_protect(expr.into()); let srcref = libr::Rf_protect(srcref.into()); - main.console_need_reset = true; RMain::eval(expr, srcref, buf, buflen); + // Check if a nested read_console() just returned. If that's the + // case, we need to reset the `R_ConsoleIob` by first returning + // a dummy value causing a `PARSE_NULL` event. + if main.nested_read_console_returned.get() { + let next_input = RMain::console_input(buf, buflen); + main.next_read_console_input.set(Some(next_input)); + + // Evaluating a space causes a `PARSE_NULL` event. Don't + // evaluate a newline, that would cause a parent debug REPL + // to interpret it as `n`, causing it to exit instead of + // being a no-op. + RMain::on_console_input(buf, buflen, String::from(" ")).unwrap(); + main.nested_read_console_returned.set(false); + } + libr::Rf_unprotect(2); return 1; } }, - ConsoleResult::NewInput => return 1, - ConsoleResult::Disconnected => return 0, + ConsoleResult::NewInput => { + return 1; + }, + + ConsoleResult::Disconnected => { + return 0; + }, ConsoleResult::Interrupt => { log::trace!("Interrupting `ReadConsole()`"); diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index d6dc903e2..7cff6e7e8 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -215,6 +215,161 @@ fn test_execute_request_browser_continue() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } +#[test] +fn test_execute_request_browser_nested() { + // Test nested browser() calls - entering a browser within a browser + let frontend = DummyArkFrontend::lock(); + + // Start first browser + let code = "browser()"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend + .recv_iopub_execute_result() + .contains("Called from: top level")); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Evaluate a value in the outer browser + let code = "42"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend.recv_iopub_execute_result().contains("[1] 42")); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Start nested browser from within the first browser + let code = "browser()"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + // Nested browser() produces execute_result output + frontend.recv_iopub_execute_result(); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Evaluate a command in the nested browser + let code = "1"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend.recv_iopub_execute_result().contains("[1] 1")); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Evaluate another value in the nested browser + let code = "\"hello\""; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend.recv_iopub_execute_result().contains("hello")); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Throw an error in the nested browser + let code = "stop('error in nested')"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_stream_stderr("Error: error in nested\n"); + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Continue to exit the nested browser and return to parent + let code = "c"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Back in the parent browser, evaluate another value + let code = "3.14"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend.recv_iopub_execute_result().contains("[1] 3.14")); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + // Throw an error in the outer browser + let code = "stop('error in parent')"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_stream_stderr("Error: error in parent\n"); + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + let code = "NA"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend.recv_iopub_execute_result().contains("[1] NA")); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + // Quit the outer browser + let code = "Q"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + #[test] fn test_execute_request_browser_error() { // The behaviour for errors is different in browsers than at top-level From 22e21bbae6cd77b971b488790ab7b9ad50ef4f41 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 5 Nov 2025 08:59:13 +0100 Subject: [PATCH 25/44] Flush autoprint buffer in case of error --- crates/ark/src/interface.rs | 11 +++++++++++ crates/ark/tests/kernel.rs | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 44 insertions(+) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index a36d82f5e..214873b6f 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -1142,6 +1142,17 @@ impl RMain { return None; }; + // Flush any accumulated output to StdOut. This can happen if + // the last input errors out during autoprint. + let autoprint = std::mem::take(&mut self.autoprint_output); + if !autoprint.is_empty() { + let message = IOPubMessage::Stream(StreamOutput { + name: Stream::Stdout, + text: autoprint, + }); + self.iopub_tx.send(message).unwrap(); + } + // Jupyter clients typically discard the `evalue` when a `traceback` is // present. Jupyter-Console even disregards `evalue` in all cases. So // include it here if we are in Notebook mode. But should Positron diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 7cff6e7e8..6e9f38780 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -640,6 +640,39 @@ fn test_execute_request_error() { ); } +#[test] +fn test_execute_request_error_with_accumulated_output() { + // Test that when the very last input output and then throws an error, + // the accumulated output is flushed before the error is reported. + // This tests the autoprint buffer flush logic in error handling. + let frontend = DummyArkFrontend::lock(); + + let code = "{ + print.foo <- function(x) { + print(unclass(x)) + stop(\"foo\") + } + structure(42, class = \"foo\") + }"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + // The output from print(1) should be flushed to stdout + frontend.recv_iopub_stream_stdout("[1] 42\n"); + + // Then the error should be reported on stderr + assert!(frontend.recv_iopub_execute_error().contains("foo")); + frontend.recv_iopub_idle(); + + assert_eq!( + frontend.recv_shell_execute_reply_exception(), + input.execution_count + ); +} + #[test] fn test_execute_request_error_expressions_overflow() { let frontend = DummyArkFrontend::lock(); From 6bb6f3b65590f8a81131ea4b9bdc37d7c4a47436 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 5 Nov 2025 09:28:43 +0100 Subject: [PATCH 26/44] Use a `PromptKind` enum to discriminate prompts --- crates/ark/src/interface.rs | 58 +++++++++++++++++++++++-------------- 1 file changed, 36 insertions(+), 22 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 214873b6f..a67cb4ea5 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -395,6 +395,19 @@ pub struct KernelInfo { pub continuation_prompt: Option, } +/// The kind of prompt we're handling in the REPL. +#[derive(Clone, Debug, PartialEq)] +pub enum PromptKind { + /// A top-level REPL prompt + TopLevel, + + /// A `browser()` debugging prompt + Browser, + + /// A user input request from code, e.g., via `readline()` + InputRequest, +} + /// This struct represents the data that we wish R would pass to /// `ReadConsole()` methods. We need this information to determine what kind /// of prompt we are dealing with. @@ -414,13 +427,8 @@ pub struct PromptInfo { /// error on them rather than requesting that this be shown. continuation_prompt: String, - /// Whether this is a `browser()` prompt. A browser prompt is never a user - /// request. - browser: bool, - - /// Whether this is a prompt from a fresh REPL iteration (browser or - /// top level) or a prompt from some user code, e.g. via `readline()` - input_request: bool, + /// The kind of prompt we're handling. + kind: PromptKind, } pub enum ConsoleInput { @@ -869,7 +877,7 @@ impl RMain { // Invariant: If we detect a browser prompt, `self.dap.is_debugging()` // is true. Otherwise it is false. - if info.browser { + if matches!(info.kind, PromptKind::Browser) { // Start or continue debugging with the `debug_preserve_focus` hint // from the last expression we evaluated self.start_debug(self.debug_preserve_focus); @@ -883,7 +891,7 @@ impl RMain { // Reply to active request with error self.handle_active_request(&info, ConsoleValue::Error(exception)); - } else if info.input_request { + } else if matches!(info.kind, PromptKind::InputRequest) { // Request input reply to the frontend and return it to R return self.handle_input_request(&info, buf, buflen); } else if let Some(input) = self.pop_pending() { @@ -903,7 +911,7 @@ impl RMain { // often. We'd still push a `DidChangeConsoleInputs` notification from // here, but only containing high-level information such as `search()` // contents and `ls(rho)`. - if !self.dap.is_debugging() && !info.input_request { + if !self.dap.is_debugging() && !matches!(info.kind, PromptKind::InputRequest) { self.refresh_lsp(); } @@ -954,16 +962,16 @@ impl RMain { let tasks_interrupt_index = select.recv(&tasks_interrupt_rx); let polled_events_index = select.recv(&polled_events_rx); - // Don't process idle tasks in browser prompts. We currently don't want + // Don't process idle tasks unless at top level. 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 + // not 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 { + let tasks_idle_index = if matches!(info.kind, PromptKind::TopLevel) { Some(select.recv(&tasks_idle_rx)) + } else { + None }; loop { @@ -975,7 +983,7 @@ impl RMain { // `UserBreak`, but won't actually fire the interrupt b/c // we have them disabled, so it would end up swallowing the // user interrupt request. - if info.input_request && interrupts_pending() { + if matches!(info.kind, PromptKind::InputRequest) && interrupts_pending() { return ConsoleResult::Interrupt; } @@ -1067,15 +1075,21 @@ impl RMain { // chosen to not support these edge cases. let browser = RE_DEBUG_PROMPT.is_match(&prompt); - // If there are frames on the stack and we're not in a browser prompt, - // this means some user code is requesting input, e.g. via `readline()` - let user_request = !browser && n_frame > 0; + // Determine the prompt kind based on context + let kind = if browser { + PromptKind::Browser + } else if n_frame > 0 { + // If there are frames on the stack and we're not in a browser prompt, + // this means some user code is requesting input, e.g. via `readline()` + PromptKind::InputRequest + } else { + PromptKind::TopLevel + }; return PromptInfo { input_prompt: prompt, continuation_prompt, - browser, - input_request: user_request, + kind, }; } @@ -1212,7 +1226,7 @@ impl RMain { buf: *mut c_uchar, buflen: c_int, ) -> Option { - if info.input_request { + if matches!(info.kind, PromptKind::InputRequest) { panic!("Unexpected `execute_request` while waiting for `input_reply`."); } From 94229c62d4f5247f9b2549c505e0c5bd5cf59c4b Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 5 Nov 2025 11:12:21 +0100 Subject: [PATCH 27/44] Evaluate in current environment --- crates/ark/src/interface.rs | 13 ++++++++--- crates/ark/tests/kernel.rs | 44 +++++++++++++++++++++++++++++++++++++ crates/harp/src/lib.rs | 1 + crates/harp/src/session.rs | 17 ++++++++++++++ 4 files changed, 72 insertions(+), 3 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index a67cb4ea5..6f0e69324 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -1386,14 +1386,18 @@ impl RMain { // SAFETY: Call this from a POD frame. Inputs must be protected. unsafe fn eval(expr: libr::SEXP, srcref: libr::SEXP, buf: *mut c_uchar, buflen: c_int) { + let frame = harp::r_current_frame(); + // SAFETY: This may jump in case of error, keep this POD unsafe { + let frame = libr::Rf_protect(frame.into()); + // The global source reference is stored in this global variable by // the R REPL before evaluation. We do the same here. libr::set(libr::R_Srcref, srcref); // Evaluate the expression. Beware: this may throw an R longjump. - let value = libr::Rf_eval(expr, R_ENVS.global); + let value = libr::Rf_eval(expr, frame); libr::Rf_protect(value); // Store in the base environment for robust access from (almost) any @@ -1403,7 +1407,7 @@ impl RMain { // is stored in the `value` field of symbols, i.e. their "CDR". libr::SETCDR(r_symbol!(".ark_last_value"), value); - libr::Rf_unprotect(1); + libr::Rf_unprotect(2); value }; @@ -2642,7 +2646,10 @@ fn do_resource_namespaces() -> bool { fn is_auto_printing() -> bool { let n_frame = harp::session::r_n_frame().unwrap(); - // The call-stack is empty so this must be R auto-printing an unclassed object + // The call-stack is empty so this must be R auto-printing an unclassed + // object. Note that this might wrongly return true in debug REPLs. Ideally + // we'd take note of the number of frames on the stack when we enter + // `r_read_console()`, and compare against that. if n_frame == 0 { return true; } diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 6e9f38780..d53fcaa46 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -621,6 +621,50 @@ fn test_execute_request_browser_multiple_expressions() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } +#[test] +fn test_execute_request_browser_local_variable() { + let frontend = DummyArkFrontend::lock(); + + let code = "local({\n local_foo <- 1\n browser()\n})"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_stream_stdout( + "Called from: eval(quote({\n local_foo <- 1\n browser()\n}), new.env())\n", + ); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + let code = "local_foo"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + // Should ideally be `recv_iopub_execute_result()`, but auto-printing + // detection currently does not work reliably in debug REPLs + frontend.recv_iopub_stream_stdout("[1] 1\n"); + frontend.recv_iopub_idle(); + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + let code = "Q"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); +} + #[test] fn test_execute_request_error() { let frontend = DummyArkFrontend::lock(); diff --git a/crates/harp/src/lib.rs b/crates/harp/src/lib.rs index ea4d66e9f..2e5d17cc1 100644 --- a/crates/harp/src/lib.rs +++ b/crates/harp/src/lib.rs @@ -70,6 +70,7 @@ pub(crate) use harp::fixtures::r_task; pub use harp::object::list_get; pub use harp::object::list_poke; pub use harp::object::RObject; +pub use harp::session::*; pub use harp::symbol::RSymbol; pub use harp::utils::get_option; pub use harp::weak_ref::RWeakRef; diff --git a/crates/harp/src/session.rs b/crates/harp/src/session.rs index 6d75e53b0..261ab0164 100644 --- a/crates/harp/src/session.rs +++ b/crates/harp/src/session.rs @@ -27,6 +27,7 @@ static SESSION_INIT: Once = Once::new(); static mut NFRAME_CALL: Option = None; static mut SYS_CALLS_CALL: Option = None; static mut SYS_FRAMES_CALL: Option = None; +static mut CURRENT_ENV_CALL: Option = None; pub fn r_n_frame() -> crate::Result { SESSION_INIT.call_once(init_interface); @@ -60,6 +61,11 @@ pub fn r_sys_frames() -> crate::Result { } } +pub fn r_current_frame() -> RObject { + SESSION_INIT.call_once(init_interface); + unsafe { libr::Rf_eval(CURRENT_ENV_CALL.unwrap_unchecked(), R_BaseEnv) }.into() +} + pub fn r_sys_functions() -> crate::Result { unsafe { let mut protect = RProtect::new(); @@ -150,5 +156,16 @@ fn init_interface() { let sys_frames_call = r_lang!(r_symbol!("sys.frames")); R_PreserveObject(sys_frames_call); SYS_FRAMES_CALL = Some(sys_frames_call); + + // Create a closure that calls `sys.frame(-1)` to get the current + // evaluation environment. We use `sys.frame(-1)` from within a closure + // because `sys.nframe()` returns the frame number where evaluation + // occurs, not the number of frames on the stack. By calling from a + // closure, we push a new frame and use negative indexing to get the + // previous frame (the actual current environment). + let closure = harp::parse_eval_base("function() sys.frame(-1)").unwrap(); + let current_env_call = r_lang!(closure.sexp); + R_PreserveObject(current_env_call); + CURRENT_ENV_CALL = Some(current_env_call); } } From 88101978afd12b0832e7bce4adec42d98af22e28 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 5 Nov 2025 11:33:04 +0100 Subject: [PATCH 28/44] Keep track of console frame via `r_read_console()` --- crates/ark/src/interface.rs | 105 ++++++++---------- .../sources/composite/search_path.rs | 18 +-- 2 files changed, 52 insertions(+), 71 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 6f0e69324..ba188f70e 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -261,11 +261,6 @@ pub struct RMain { /// reliable indication of whether we moved since last time. debug_last_stack: Vec, - /// Current topmost environment on the stack while waiting for input in the - /// debugger. This is `Some()` only when R is idle and in a `browser()` - /// prompt. - debug_env: Option, - /// Ever increasing debug session index. Used to create URIs that are only /// valid for a single session. debug_session_index: u32, @@ -287,6 +282,9 @@ pub struct RMain { /// Used to track an input to evaluate upon returning to `r_read_console()`, /// after having returned a dummy input to reset `R_ConsoleIob` in R's REPL. next_read_console_input: Cell>, + + /// Current topmost environment on the stack while waiting for input in ReadConsole + read_console_frame: RefCell, } /// Stack of pending inputs @@ -737,13 +735,13 @@ impl RMain { captured_output: String::new(), debug_preserve_focus: false, debug_last_stack: vec![], - debug_env: None, debug_session_index: 1, pending_inputs: None, read_console_depth: Cell::new(0), nested_read_console_returned: Cell::new(false), read_console_threw_error: Cell::new(false), next_read_console_input: Cell::new(None), + read_console_frame: RefCell::new(RObject::new(unsafe { libr::R_GlobalEnv })), } } @@ -1180,11 +1178,6 @@ impl RMain { Some(exception) } - fn read_console_cleanup(&mut self) { - // The debug environment is only valid while R is idle - self.debug_env = None; - } - /// Returns: /// - `None` if we should fall through to the event loop to wait for more user input /// - `Some(ConsoleResult)` if we should immediately exit `read_console()` @@ -1467,13 +1460,6 @@ impl RMain { fn start_debug(&mut self, debug_preserve_focus: bool) { match self.dap.stack_info() { Ok(stack) => { - if let Some(frame) = stack.first() { - if let Some(ref env) = frame.environment { - // This is reset on exit in the cleanup phase, see `r_read_console()` - self.debug_env = Some(env.get().clone()); - } - } - // Figure out whether we changed location since last time, // e.g. because the user evaluated an expression that hit // another breakpoint. In that case we do want to move @@ -2355,8 +2341,8 @@ impl RMain { } #[cfg(not(test))] // Avoid warnings in unit test - pub(crate) fn debug_env(&self) -> Option { - self.debug_env.clone() + pub(crate) fn read_console_frame(&self) -> RObject { + self.read_console_frame.borrow().clone() } } @@ -2409,49 +2395,50 @@ pub extern "C-unwind" fn r_read_console( // evaluation. Ideally R would extend their frontend API so that this would // only be necessary for backward compatibility with old versions of R. - { - let main = RMain::get_mut(); + let main = RMain::get_mut(); - // We've finished evaluating a dummy value to reset state in R's REPL, - // and are now ready to evaluate the actual input - if let Some(next_input) = main.next_read_console_input.take() { - RMain::on_console_input(buf, buflen, next_input).unwrap(); - return 1; - } + // We've finished evaluating a dummy value to reset state in R's REPL, + // and are now ready to evaluate the actual input + if let Some(next_input) = main.next_read_console_input.take() { + RMain::on_console_input(buf, buflen, next_input).unwrap(); + return 1; + } - // In case of error, we haven't had a chance to evaluate ".ark_last_value". - // So we return to the R REPL to give R a chance to run the state - // restoration that occurs between `R_ReadConsole()` and `eval()`: - // - R_PPStackTop: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L227 - // - R_EvalDepth: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L260 - // - // Technically this also resets time limits (see `base::setTimeLimit()`) but - // these aren't supported in Ark because they cause errors when we poll R - // events. - if main.last_error.is_some() && main.read_console_threw_error.get() { - main.read_console_threw_error.set(false); + // In case of error, we haven't had a chance to evaluate ".ark_last_value". + // So we return to the R REPL to give R a chance to run the state + // restoration that occurs between `R_ReadConsole()` and `eval()`: + // - R_PPStackTop: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L227 + // - R_EvalDepth: https://github.com/r-devel/r-svn/blob/74cd0af4/src/main/main.c#L260 + // + // Technically this also resets time limits (see `base::setTimeLimit()`) but + // these aren't supported in Ark because they cause errors when we poll R + // events. + if main.last_error.is_some() && main.read_console_threw_error.get() { + main.read_console_threw_error.set(false); + + // Evaluate last value so that `base::.Last.value` remains the same + RMain::on_console_input( + buf, + buflen, + String::from("base::invisible(base::.Last.value)"), + ) + .unwrap(); + return 1; + } - // Evaluate last value so that `base::.Last.value` remains the same - RMain::on_console_input( - buf, - buflen, - String::from("base::invisible(base::.Last.value)"), - ) - .unwrap(); - return 1; - } + // Track nesting depth of ReadConsole REPLs + main.read_console_depth + .set(main.read_console_depth.get() + 1); - // Track nesting depth of ReadConsole REPLs - main.read_console_depth - .set(main.read_console_depth.get() + 1); + // Reset flag that helps us figure out when a nested REPL returns + main.nested_read_console_returned.set(false); - // Reset flag that helps us figure out when a nested REPL returns - main.nested_read_console_returned.set(false); + // Reset flag that helps us figure out when an error occurred and needs a + // reset of `R_EvalDepth` and friends + main.read_console_threw_error.set(true); - // Reset flag that helps us figure out when an error occurred and needs a - // reset of `R_EvalDepth` and friends - main.read_console_threw_error.set(true); - } + // Set current frame environment + let current_frame = main.read_console_frame.replace(harp::r_current_frame()); exec_with_cleanup( || { @@ -2474,6 +2461,9 @@ pub extern "C-unwind" fn r_read_console( // nested console returned (if it indeed returns instead of looping // for another iteration) main.nested_read_console_returned.set(true); + + // Restore current frame + main.read_console_frame.replace(current_frame); }, ) } @@ -2486,7 +2476,6 @@ fn r_read_console_impl( hist: c_int, ) -> i32 { let result = r_sandbox(|| main.read_console(prompt, buf, buflen, hist)); - main.read_console_cleanup(); let result = unwrap!(result, Err(err) => { panic!("Unexpected longjump while reading from console: {err:?}"); diff --git a/crates/ark/src/lsp/completions/sources/composite/search_path.rs b/crates/ark/src/lsp/completions/sources/composite/search_path.rs index 0adfd8004..273cd7951 100644 --- a/crates/ark/src/lsp/completions/sources/composite/search_path.rs +++ b/crates/ark/src/lsp/completions/sources/composite/search_path.rs @@ -13,7 +13,6 @@ use harp::vector::CharacterVector; use harp::vector::Vector; use harp::RObject; use libr::R_EmptyEnv; -use libr::R_GlobalEnv; use libr::R_lsInternal; use libr::ENCLOS; use tower_lsp::lsp_types::CompletionItem; @@ -51,19 +50,12 @@ fn completions_from_search_path( ]; unsafe { - // Iterate through environments starting from the global environment. - let mut env = R_GlobalEnv; - - // If we're waiting for input in `read_console()` with a debugger - // prompt, start from current environment + // Iterate through environments starting from the current frame environment. #[cfg(not(test))] // Unit tests do not have an `RMain` - { - use crate::interface::RMain; - if let Some(debug_env) = &RMain::get().debug_env() { - // Mem-Safety: Object protected by `RMain` for the duration of the `r_task()` - env = debug_env.sexp; - } - } + // Mem-Safety: Object protected by `RMain` for the duration of the `r_task()` + let mut env = crate::interface::RMain::get().read_console_frame().sexp; + #[cfg(test)] + let mut env = libr::R_GlobalEnv; while env != R_EmptyEnv { let is_pkg_env = r_env_is_pkg_env(env); From 83ba148a57687a60a84b4651d574958ac69197f4 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 5 Nov 2025 18:37:57 +0100 Subject: [PATCH 29/44] Clearer documentation and variable name --- crates/ark/src/interface.rs | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index ba188f70e..d5d4e9f98 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -2426,20 +2426,25 @@ pub extern "C-unwind" fn r_read_console( return 1; } - // Track nesting depth of ReadConsole REPLs + // Keep track of state that we care about + + // - Track nesting depth of ReadConsole REPLs main.read_console_depth .set(main.read_console_depth.get() + 1); - // Reset flag that helps us figure out when a nested REPL returns + // - Set current frame environment + let old_current_frame = main.read_console_frame.replace(harp::r_current_frame()); + + // Keep track of state that we use for workarounds while interacting + // with the R REPL and force it to reset state + + // - Reset flag that helps us figure out when a nested REPL returns main.nested_read_console_returned.set(false); - // Reset flag that helps us figure out when an error occurred and needs a - // reset of `R_EvalDepth` and friends + // - Reset flag that helps us figure out when an error occurred and needs a + // reset of `R_EvalDepth` and friends main.read_console_threw_error.set(true); - // Set current frame environment - let current_frame = main.read_console_frame.replace(harp::r_current_frame()); - exec_with_cleanup( || { let main = RMain::get_mut(); @@ -2463,7 +2468,7 @@ pub extern "C-unwind" fn r_read_console( main.nested_read_console_returned.set(true); // Restore current frame - main.read_console_frame.replace(current_frame); + main.read_console_frame.replace(old_current_frame); }, ) } From 5780f6d4018aa60e5276dc21876de84bea8c39f8 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 5 Nov 2025 13:12:41 +0100 Subject: [PATCH 30/44] Add shutdown handling to dummy frontend --- .../amalthea/src/fixtures/dummy_frontend.rs | 33 +++++++++++++++++-- crates/amalthea/src/wire/jupyter_message.rs | 6 ++++ crates/amalthea/tests/client.rs | 29 ++++++++++++++++ 3 files changed, 65 insertions(+), 3 deletions(-) diff --git a/crates/amalthea/src/fixtures/dummy_frontend.rs b/crates/amalthea/src/fixtures/dummy_frontend.rs index b6520ab7b..768523601 100644 --- a/crates/amalthea/src/fixtures/dummy_frontend.rs +++ b/crates/amalthea/src/fixtures/dummy_frontend.rs @@ -21,6 +21,8 @@ use crate::wire::jupyter_message::JupyterMessage; use crate::wire::jupyter_message::Message; use crate::wire::jupyter_message::ProtocolMessage; use crate::wire::jupyter_message::Status; +use crate::wire::shutdown_reply::ShutdownReply; +use crate::wire::shutdown_request::ShutdownRequest; use crate::wire::status::ExecutionState; use crate::wire::stream::Stream; use crate::wire::wire_message::WireMessage; @@ -36,7 +38,7 @@ pub struct DummyConnection { } pub struct DummyFrontend { - pub _control_socket: Socket, + pub control_socket: Socket, pub shell_socket: Socket, pub iopub_socket: Socket, pub stdin_socket: Socket, @@ -132,7 +134,7 @@ impl DummyFrontend { // the Jupyter specification, these must share a ZeroMQ identity. let shell_id = rand::thread_rng().gen::<[u8; 16]>(); - let _control_socket = Socket::new( + let control_socket = Socket::new( connection.session.clone(), connection.ctx.clone(), String::from("Control"), @@ -198,7 +200,7 @@ impl DummyFrontend { }); Self { - _control_socket, + control_socket, shell_socket, iopub_socket, stdin_socket, @@ -207,12 +209,22 @@ impl DummyFrontend { } } + /// Sends a Jupyter message on the Control socket; returns the ID of the newly + /// created message + pub fn send_control(&self, msg: T) -> String { + Self::send(&self.control_socket, &self.session, msg) + } + /// Sends a Jupyter message on the Shell socket; returns the ID of the newly /// created message pub fn send_shell(&self, msg: T) -> String { Self::send(&self.shell_socket, &self.session, msg) } + pub fn send_shutdown_request(&self, restart: bool) -> String { + self.send_control(ShutdownRequest { restart }) + } + pub fn send_execute_request(&self, code: &str, options: ExecuteRequestOptions) -> String { self.send_shell(ExecuteRequest { code: String::from(code), @@ -256,6 +268,12 @@ impl DummyFrontend { panic!("Timeout while expecting message on socket {}", socket.name); } + /// Receives a Jupyter message from the Control socket + #[track_caller] + pub fn recv_control(&self) -> Message { + Self::recv(&self.control_socket) + } + /// Receives a Jupyter message from the Shell socket #[track_caller] pub fn recv_shell(&self) -> Message { @@ -274,6 +292,15 @@ impl DummyFrontend { Self::recv(&self.stdin_socket) } + /// Receive from Control and assert `ShutdownReply` message. + #[track_caller] + pub fn recv_control_shutdown_reply(&self) -> ShutdownReply { + let message = self.recv_control(); + assert_matches!(message, Message::ShutdownReply(message) => { + message.content + }) + } + /// Receive from Shell and assert `ExecuteReply` message. /// Returns `execution_count`. #[track_caller] diff --git a/crates/amalthea/src/wire/jupyter_message.rs b/crates/amalthea/src/wire/jupyter_message.rs index 76d835605..b60ae002f 100644 --- a/crates/amalthea/src/wire/jupyter_message.rs +++ b/crates/amalthea/src/wire/jupyter_message.rs @@ -46,6 +46,7 @@ use crate::wire::is_complete_reply::IsCompleteReply; use crate::wire::is_complete_request::IsCompleteRequest; use crate::wire::kernel_info_request::KernelInfoRequest; use crate::wire::originator::Originator; +use crate::wire::shutdown_reply::ShutdownReply; use crate::wire::shutdown_request::ShutdownRequest; use crate::wire::status::KernelStatus; use crate::wire::wire_message::WireMessage; @@ -101,6 +102,7 @@ pub enum Message { // Control InterruptReply(JupyterMessage), InterruptRequest(JupyterMessage), + ShutdownReply(JupyterMessage), ShutdownRequest(JupyterMessage), // Registration HandshakeRequest(JupyterMessage), @@ -163,6 +165,7 @@ impl TryFrom<&Message> for WireMessage { Message::IsCompleteRequest(msg) => WireMessage::try_from(msg), Message::KernelInfoReply(msg) => WireMessage::try_from(msg), Message::KernelInfoRequest(msg) => WireMessage::try_from(msg), + Message::ShutdownReply(msg) => WireMessage::try_from(msg), Message::ShutdownRequest(msg) => WireMessage::try_from(msg), Message::Status(msg) => WireMessage::try_from(msg), Message::CommInfoReply(msg) => WireMessage::try_from(msg), @@ -245,6 +248,9 @@ impl TryFrom<&WireMessage> for Message { if kind == UpdateDisplayData::message_type() { return Ok(Message::UpdateDisplayData(JupyterMessage::try_from(msg)?)); } + if kind == ShutdownReply::message_type() { + return Ok(Message::ShutdownReply(JupyterMessage::try_from(msg)?)); + } if kind == ShutdownRequest::message_type() { return Ok(Message::ShutdownRequest(JupyterMessage::try_from(msg)?)); } diff --git a/crates/amalthea/tests/client.rs b/crates/amalthea/tests/client.rs index e20f27fdb..26f904093 100644 --- a/crates/amalthea/tests/client.rs +++ b/crates/amalthea/tests/client.rs @@ -19,6 +19,7 @@ use amalthea::wire::comm_info_request::CommInfoRequest; use amalthea::wire::comm_msg::CommWireMsg; use amalthea::wire::comm_open::CommOpen; use amalthea::wire::jupyter_message::Message; +use amalthea::wire::jupyter_message::Status; use amalthea::wire::kernel_info_request::KernelInfoRequest; use amalthea::wire::status::ExecutionState; use assert_matches::assert_matches; @@ -63,6 +64,34 @@ fn test_amalthea_execute_request() { frontend.recv_iopub_idle(); } +#[test] +fn test_amalthea_shutdown_request() { + let frontend = DummyAmaltheaFrontend::lock(); + + // Send a shutdown request with restart = false + frontend.send_shutdown_request(false); + + // Shutdown requests generate busy/idle status messages on IOPub + frontend.recv_iopub_busy(); + + // Receive the shutdown reply + let reply = frontend.recv_control_shutdown_reply(); + assert_eq!(reply.status, Status::Ok); + assert_eq!(reply.restart, false); + + frontend.recv_iopub_idle(); + + // Test with restart = true + frontend.send_shutdown_request(true); + frontend.recv_iopub_busy(); + + let reply = frontend.recv_control_shutdown_reply(); + assert_eq!(reply.status, Status::Ok); + assert_eq!(reply.restart, true); + + frontend.recv_iopub_idle(); +} + #[test] fn test_amalthea_input_request() { let frontend = DummyAmaltheaFrontend::lock(); From d257e0573864133c4eb366632bb82383fc220e0c Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 5 Nov 2025 19:41:15 +0100 Subject: [PATCH 31/44] Add integration tests for shutdown --- crates/amalthea/src/socket/control.rs | 5 ++++ crates/ark/src/fixtures/dummy_frontend.rs | 18 ++++++++++++ crates/ark/src/interface.rs | 22 ++++++++++++++ crates/ark/src/sys/unix/interface.rs | 8 ++++++ crates/ark/tests/kernel.rs | 35 +++++++++++++++++++++++ crates/libr/src/r.rs | 3 ++ 6 files changed, 91 insertions(+) diff --git a/crates/amalthea/src/socket/control.rs b/crates/amalthea/src/socket/control.rs index 8e7d05716..829732da8 100644 --- a/crates/amalthea/src/socket/control.rs +++ b/crates/amalthea/src/socket/control.rs @@ -102,6 +102,11 @@ impl Control { H: FnOnce(JupyterMessage) -> Result<(), Error>, { // Enter the kernel-busy state in preparation for handling the message. + // The protocol specification does not mandate status messages for + // Control, but we emit them for compatibility with ipykernel: + // https://github.com/ipython/ipykernel/pull/585. These status messages + // can be discriminated from those on Shell by examining the parent + // header. if let Err(err) = self.send_state(req.clone(), ExecutionState::Busy) { warn!("Failed to change kernel status to busy: {err}"); } diff --git a/crates/ark/src/fixtures/dummy_frontend.rs b/crates/ark/src/fixtures/dummy_frontend.rs index 5823dd16c..74a951b7d 100644 --- a/crates/ark/src/fixtures/dummy_frontend.rs +++ b/crates/ark/src/fixtures/dummy_frontend.rs @@ -4,11 +4,13 @@ use std::sync::Arc; use std::sync::Mutex; use std::sync::MutexGuard; use std::sync::OnceLock; +use std::time::Duration; use amalthea::fixtures::dummy_frontend::DummyConnection; use amalthea::fixtures::dummy_frontend::DummyFrontend; use crate::interface::SessionMode; +use crate::interface::CLEANUP_SIGNAL; use crate::repos::DefaultRepos; // There can be only one frontend per process. Needs to be in a mutex because @@ -62,6 +64,22 @@ impl DummyArkFrontend { } } + /// Wait for R cleanup to start (indicating shutdown has been initiated). + /// Panics if cleanup doesn't start within the timeout. + #[track_caller] + pub fn wait_for_cleanup() { + let (lock, cvar) = &CLEANUP_SIGNAL; + let result = cvar + .wait_timeout_while(lock.lock().unwrap(), Duration::from_secs(3), |started| { + !*started + }) + .unwrap(); + + if !*result.0 { + panic!("Cleanup did not start within timeout"); + } + } + fn get_frontend() -> &'static Arc> { // These are the hard-coded defaults. Call `init()` explicitly to // override. diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index d5d4e9f98..783f95074 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -2584,6 +2584,28 @@ pub unsafe extern "C-unwind" fn r_polled_events() { }; } +// For integration tests +use std::sync::Condvar; +pub static CLEANUP_SIGNAL: (Mutex, Condvar) = (Mutex::new(false), Condvar::new()); + +#[no_mangle] +pub extern "C-unwind" fn r_cleanup_for_tests(_save_act: i32, _status: i32, _run_last: i32) { + // Signal that cleanup has started + let (lock, cvar) = &CLEANUP_SIGNAL; + + let mut started = lock.lock().unwrap(); + *started = true; + + cvar.notify_all(); + drop(started); + + // Sleep to give tests time to complete before we panic + std::thread::sleep(std::time::Duration::from_secs(5)); + + // Fallthrough to R which will call `exit()`. Note that panicking from here + // would be UB, we can't panic over a C stack. +} + // This hook is called like a user onLoad hook but for every package to be // loaded in the session #[harp::register] diff --git a/crates/ark/src/sys/unix/interface.rs b/crates/ark/src/sys/unix/interface.rs index db5bc60ce..0748ea367 100644 --- a/crates/ark/src/sys/unix/interface.rs +++ b/crates/ark/src/sys/unix/interface.rs @@ -73,6 +73,14 @@ pub fn setup_r(args: &Vec) { libr::set(ptr_R_Busy, Some(r_busy)); libr::set(ptr_R_Suicide, Some(r_suicide)); + if stdext::IS_TESTING { + use libr::ptr_R_CleanUp; + + use crate::interface::r_cleanup_for_tests; + + libr::set(ptr_R_CleanUp, Some(r_cleanup_for_tests)); + } + // In tests R may be run from various threads. This confuses R's stack // overflow checks so we disable those. This should not make it in // production builds as it causes stack overflows to crash R instead of diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index d53fcaa46..73cde8eec 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -1,5 +1,6 @@ use amalthea::fixtures::dummy_frontend::ExecuteRequestOptions; use amalthea::wire::jupyter_message::Message; +use amalthea::wire::jupyter_message::Status; use amalthea::wire::kernel_info_request::KernelInfoRequest; use ark::fixtures::DummyArkFrontend; use stdext::assert_match; @@ -1143,3 +1144,37 @@ fn test_env_vars() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } + +// Note that because of these shutdown tests you _have_ to use `cargo nextest` +// instead of `cargo test`, so that each test has its own process and R thread. +#[test] +fn test_shutdown_request() { + let frontend = DummyArkFrontend::lock(); + + frontend.send_shutdown_request(false); + frontend.recv_iopub_busy(); + + let reply = frontend.recv_control_shutdown_reply(); + assert_eq!(reply.status, Status::Ok); + assert_eq!(reply.restart, false); + + frontend.recv_iopub_idle(); + + DummyArkFrontend::wait_for_cleanup(); +} + +#[test] +fn test_shutdown_request_with_restart() { + let frontend = DummyArkFrontend::lock(); + + frontend.send_shutdown_request(true); + frontend.recv_iopub_busy(); + + let reply = frontend.recv_control_shutdown_reply(); + assert_eq!(reply.status, Status::Ok); + assert_eq!(reply.restart, true); + + frontend.recv_iopub_idle(); + + DummyArkFrontend::wait_for_cleanup(); +} diff --git a/crates/libr/src/r.rs b/crates/libr/src/r.rs index 8fb8b4e1f..510a57dad 100644 --- a/crates/libr/src/r.rs +++ b/crates/libr/src/r.rs @@ -760,6 +760,9 @@ mutable_globals::generate! { #[cfg(target_family = "unix")] pub static mut ptr_R_Suicide: Option; + #[cfg(target_family = "unix")] + pub static mut ptr_R_CleanUp: Option; + // ----------------------------------------------------------------------------------- // Windows From 3ec41cb6fb02344b9dfbc3f62066a16e013c2532 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 5 Nov 2025 19:53:58 +0100 Subject: [PATCH 32/44] Prevent R from asking about saving workspace We've never supported it, so better be explicit about it. This allows us to simplify some special-casing for that question. Also set `--no-restore-data` by default for consistency. --- crates/ark/src/interface.rs | 9 --------- crates/ark/src/main.rs | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 783f95074..4082e4d8d 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -1288,15 +1288,6 @@ impl RMain { buf: *mut c_uchar, buflen: c_int, ) -> ConsoleResult { - // If the prompt begins with "Save workspace", respond with (n) - // and allow R to immediately exit. - if info.input_prompt.starts_with("Save workspace") { - match Self::on_console_input(buf, buflen, String::from("n")) { - Ok(()) => return ConsoleResult::NewInput, - Err(err) => return ConsoleResult::Error(err), - } - } - if let Some(req) = &self.active_request { // Send request to frontend. We'll wait for an `input_reply` // from the frontend in the event loop in `read_console()`. diff --git a/crates/ark/src/main.rs b/crates/ark/src/main.rs index b5e4d202d..746e43058 100644 --- a/crates/ark/src/main.rs +++ b/crates/ark/src/main.rs @@ -83,6 +83,9 @@ fn main() -> anyhow::Result<()> { let mut capture_streams = true; let mut default_repos = DefaultRepos::Auto; + // We don't support the asking the user whether to save the workspace data + // on exit because calling readline during shutdown puts in a precarious + // position. So effectively we're implementing "no-save" by default. // Process remaining arguments. TODO: Need an argument that can passthrough args to R while let Some(arg) = argv.next() { match arg.as_str() { @@ -325,6 +328,20 @@ fn main() -> anyhow::Result<()> { r_args.push(String::from("--interactive")); } + // Prepend the vector of arguments with our default. These can be overridden + // by user arguments (last one wins). + r_args.splice(0..0, [ + // We don't support the asking the user whether to save the workspace + // data on exit because calling readline during shutdown puts in a + // precarious position. So effectively we're implementing "no-save" by + // default. Note that there is no argument to opt into the "ask" + // behaviour, so it can't be reenabled by the user. + String::from("--no-save"), + // Since we don't save by default, we also don't restore by default for + // consistency + String::from("--no-restore-data"), + ]); + // This causes panics on background threads to propagate on the main // thread. If we don't propagate a background thread panic, the program // keeps running in an unstable state as all communications with this From 5e219c2695aeeb64946c139b1fee963698f1454d Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 5 Nov 2025 20:01:10 +0100 Subject: [PATCH 33/44] Shutdown all nested consoles in case of Shutdown request --- crates/ark/src/interface.rs | 21 +++++++++++++++++---- crates/ark/tests/kernel.rs | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 4 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 4082e4d8d..c6f878a17 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -281,7 +281,11 @@ pub struct RMain { /// Used to track an input to evaluate upon returning to `r_read_console()`, /// after having returned a dummy input to reset `R_ConsoleIob` in R's REPL. - next_read_console_input: Cell>, + read_console_next_input: Cell>, + + /// We've received a Shutdown signal and need to return EOF from all nested + /// consoles to get R to shut down + read_console_shutdown: Cell, /// Current topmost environment on the stack while waiting for input in ReadConsole read_console_frame: RefCell, @@ -740,8 +744,9 @@ impl RMain { read_console_depth: Cell::new(0), nested_read_console_returned: Cell::new(false), read_console_threw_error: Cell::new(false), - next_read_console_input: Cell::new(None), + read_console_next_input: Cell::new(None), read_console_frame: RefCell::new(RObject::new(unsafe { libr::R_GlobalEnv })), + read_console_shutdown: Cell::new(false), } } @@ -2388,9 +2393,15 @@ pub extern "C-unwind" fn r_read_console( let main = RMain::get_mut(); + // Propagate an EOF event (e.g. from a Shutdown request). We need to exit + // from all consoles on the stack to let R shut down with an `exit()`. + if main.read_console_shutdown.get() { + return 0; + } + // We've finished evaluating a dummy value to reset state in R's REPL, // and are now ready to evaluate the actual input - if let Some(next_input) = main.next_read_console_input.take() { + if let Some(next_input) = main.read_console_next_input.take() { RMain::on_console_input(buf, buflen, next_input).unwrap(); return 1; } @@ -2498,7 +2509,7 @@ fn r_read_console_impl( // a dummy value causing a `PARSE_NULL` event. if main.nested_read_console_returned.get() { let next_input = RMain::console_input(buf, buflen); - main.next_read_console_input.set(Some(next_input)); + main.read_console_next_input.set(Some(next_input)); // Evaluating a space causes a `PARSE_NULL` event. Don't // evaluate a newline, that would cause a parent debug REPL @@ -2518,6 +2529,8 @@ fn r_read_console_impl( }, ConsoleResult::Disconnected => { + // Cause parent consoles to shutdown too + main.read_console_shutdown.set(true); return 0; }, diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 73cde8eec..2c3fad113 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -1178,3 +1178,36 @@ fn test_shutdown_request_with_restart() { DummyArkFrontend::wait_for_cleanup(); } + +// Can shut down Ark when running a nested debug console +// https://github.com/posit-dev/positron/issues/6553 +#[test] +fn test_shutdown_request_browser() { + let frontend = DummyArkFrontend::lock(); + + let code = "browser()"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + assert!(frontend + .recv_iopub_execute_result() + .contains("Called from: top level")); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + + frontend.send_shutdown_request(true); + frontend.recv_iopub_busy(); + + let reply = frontend.recv_control_shutdown_reply(); + assert_eq!(reply.status, Status::Ok); + assert_eq!(reply.restart, true); + + frontend.recv_iopub_idle(); + + DummyArkFrontend::wait_for_cleanup(); +} From 621605170d2be17cf64632f345754fd377c0793b Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 6 Nov 2025 12:06:01 +0100 Subject: [PATCH 34/44] Send interrupt before shutting down --- crates/ark/src/control.rs | 6 ++++++ crates/ark/tests/kernel.rs | 44 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/crates/ark/src/control.rs b/crates/ark/src/control.rs index 93fcfbc9b..b005e8485 100644 --- a/crates/ark/src/control.rs +++ b/crates/ark/src/control.rs @@ -36,6 +36,12 @@ impl ControlHandler for Control { ) -> Result { log::info!("Received shutdown request: {msg:?}"); + // Interrupt any ongoing computation. We shut down from ReadConsole when + // R has become idle again. Note that Positron will have interrupted us + // beforehand, but another frontend might not have, and it's good to + // have this as a defensive measure in any case. + crate::sys::control::handle_interrupt_request(); + // According to the Jupyter protocol we should block here until the // shutdown is complete. However AFAICS ipykernel doesn't wait // until complete shutdown before replying and instead just signals diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 2c3fad113..e4eb4c156 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -3,6 +3,9 @@ use amalthea::wire::jupyter_message::Message; use amalthea::wire::jupyter_message::Status; use amalthea::wire::kernel_info_request::KernelInfoRequest; use ark::fixtures::DummyArkFrontend; +use nix::sys::signal::signal; +use nix::sys::signal::SigHandler; +use nix::sys::signal::Signal; use stdext::assert_match; #[test] @@ -1145,10 +1148,21 @@ fn test_env_vars() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); } +/// Install a SIGINT handler for shutdown tests. This overrides the test runner +/// handler so it doesn't cancel our test. +fn install_sigint_handler() { + extern "C" fn sigint_handler(_: libc::c_int) {} + #[cfg(unix)] + unsafe { + signal(Signal::SIGINT, SigHandler::Handler(sigint_handler)).unwrap(); + } +} + // Note that because of these shutdown tests you _have_ to use `cargo nextest` // instead of `cargo test`, so that each test has its own process and R thread. #[test] fn test_shutdown_request() { + install_sigint_handler(); let frontend = DummyArkFrontend::lock(); frontend.send_shutdown_request(false); @@ -1165,6 +1179,7 @@ fn test_shutdown_request() { #[test] fn test_shutdown_request_with_restart() { + install_sigint_handler(); let frontend = DummyArkFrontend::lock(); frontend.send_shutdown_request(true); @@ -1183,6 +1198,7 @@ fn test_shutdown_request_with_restart() { // https://github.com/posit-dev/positron/issues/6553 #[test] fn test_shutdown_request_browser() { + install_sigint_handler(); let frontend = DummyArkFrontend::lock(); let code = "browser()"; @@ -1211,3 +1227,31 @@ fn test_shutdown_request_browser() { DummyArkFrontend::wait_for_cleanup(); } + +#[test] +fn test_shutdown_request_while_busy() { + install_sigint_handler(); + let frontend = DummyArkFrontend::lock(); + + let code = "Sys.sleep(10)"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.send_shutdown_request(false); + frontend.recv_iopub_busy(); + + let reply = frontend.recv_control_shutdown_reply(); + assert_eq!(reply.status, Status::Ok); + assert_eq!(reply.restart, false); + + frontend.recv_iopub_stream_stderr("\n"); + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.recv_iopub_idle(); + + DummyArkFrontend::wait_for_cleanup(); +} From ab9a936aba84451f4c82d31b3a5e9c992d7f11fb Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 6 Nov 2025 15:11:59 +0100 Subject: [PATCH 35/44] Move signal declaration inside of Unix context --- crates/ark/tests/kernel.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index e4eb4c156..68952df3a 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -3,9 +3,6 @@ use amalthea::wire::jupyter_message::Message; use amalthea::wire::jupyter_message::Status; use amalthea::wire::kernel_info_request::KernelInfoRequest; use ark::fixtures::DummyArkFrontend; -use nix::sys::signal::signal; -use nix::sys::signal::SigHandler; -use nix::sys::signal::Signal; use stdext::assert_match; #[test] @@ -1154,6 +1151,10 @@ fn install_sigint_handler() { extern "C" fn sigint_handler(_: libc::c_int) {} #[cfg(unix)] unsafe { + use nix::sys::signal::signal; + use nix::sys::signal::SigHandler; + use nix::sys::signal::Signal; + signal(Signal::SIGINT, SigHandler::Handler(sigint_handler)).unwrap(); } } From cf63dbc890a395c00289347bf4814c3008e59a33 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 6 Nov 2025 15:15:16 +0100 Subject: [PATCH 36/44] Make the error message test less brittle --- crates/ark/tests/kernel.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 68952df3a..11a123cef 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -141,10 +141,7 @@ fn test_execute_request_invalid() { let input = frontend.recv_iopub_execute_input(); assert_eq!(input.code, code); - assert_eq!( - frontend.recv_iopub_execute_error(), - "Error:\nSyntax error: unexpected ')'" - ); + assert!(frontend.recv_iopub_execute_error().contains("Syntax error")); frontend.recv_iopub_idle(); From 42f87753d324a73d35767739af3f348317d8cf3e Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 6 Nov 2025 15:52:11 +0100 Subject: [PATCH 37/44] Disable brittle tests --- crates/ark/tests/kernel.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 11a123cef..8abfa4047 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -1192,10 +1192,16 @@ fn test_shutdown_request_with_restart() { DummyArkFrontend::wait_for_cleanup(); } +static SHUTDOWN_TESTS_ENABLED: bool = false; + // Can shut down Ark when running a nested debug console // https://github.com/posit-dev/positron/issues/6553 #[test] fn test_shutdown_request_browser() { + if !SHUTDOWN_TESTS_ENABLED { + return; + } + install_sigint_handler(); let frontend = DummyArkFrontend::lock(); @@ -1217,6 +1223,9 @@ fn test_shutdown_request_browser() { frontend.send_shutdown_request(true); frontend.recv_iopub_busy(); + // There is a race condition between the Control thread and the Shell + // threads. Ideally we'd wait for both the Shutdown reply and the IOPub Idle + // messages concurrently instead of sequentially. let reply = frontend.recv_control_shutdown_reply(); assert_eq!(reply.status, Status::Ok); assert_eq!(reply.restart, true); @@ -1228,6 +1237,10 @@ fn test_shutdown_request_browser() { #[test] fn test_shutdown_request_while_busy() { + if !SHUTDOWN_TESTS_ENABLED { + return; + } + install_sigint_handler(); let frontend = DummyArkFrontend::lock(); @@ -1245,6 +1258,7 @@ fn test_shutdown_request_while_busy() { assert_eq!(reply.status, Status::Ok); assert_eq!(reply.restart, false); + // It seems this isn't emitted on older R versions frontend.recv_iopub_stream_stderr("\n"); frontend.recv_iopub_idle(); From 6d2f1e8db2a22b8ab67c394bb9cc81add7df6986 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 6 Nov 2025 16:09:31 +0100 Subject: [PATCH 38/44] Opt out of Shutdown tests on Windows --- crates/ark/tests/kernel.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 8abfa4047..4d1e851be 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -1159,6 +1159,7 @@ fn install_sigint_handler() { // Note that because of these shutdown tests you _have_ to use `cargo nextest` // instead of `cargo test`, so that each test has its own process and R thread. #[test] +#[cfg(unix)] fn test_shutdown_request() { install_sigint_handler(); let frontend = DummyArkFrontend::lock(); @@ -1176,6 +1177,7 @@ fn test_shutdown_request() { } #[test] +#[cfg(unix)] fn test_shutdown_request_with_restart() { install_sigint_handler(); let frontend = DummyArkFrontend::lock(); @@ -1197,6 +1199,7 @@ static SHUTDOWN_TESTS_ENABLED: bool = false; // Can shut down Ark when running a nested debug console // https://github.com/posit-dev/positron/issues/6553 #[test] +#[cfg(unix)] fn test_shutdown_request_browser() { if !SHUTDOWN_TESTS_ENABLED { return; @@ -1236,6 +1239,7 @@ fn test_shutdown_request_browser() { } #[test] +#[cfg(unix)] fn test_shutdown_request_while_busy() { if !SHUTDOWN_TESTS_ENABLED { return; From 492d33c3dbe41f117d0140ce77927e31baafbe33 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 6 Nov 2025 16:15:26 +0100 Subject: [PATCH 39/44] Improve naming a bit --- crates/ark/src/interface.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index c6f878a17..5c3461287 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -272,7 +272,7 @@ pub struct RMain { /// Set to true when `r_read_console()` exits. Reset to false at the start /// of each `r_read_console()` call. Used to detect if `eval()` returned /// from a nested REPL (the flag will be true when the evaluation returns). - nested_read_console_returned: Cell, + read_console_nested_return: Cell, /// Set to true `r_read_console()` exits via an error longjump. Used to /// detect if we need to go return from `r_read_console()` with a dummy @@ -742,7 +742,7 @@ impl RMain { debug_session_index: 1, pending_inputs: None, read_console_depth: Cell::new(0), - nested_read_console_returned: Cell::new(false), + read_console_nested_return: Cell::new(false), read_console_threw_error: Cell::new(false), read_console_next_input: Cell::new(None), read_console_frame: RefCell::new(RObject::new(unsafe { libr::R_GlobalEnv })), @@ -2441,7 +2441,7 @@ pub extern "C-unwind" fn r_read_console( // with the R REPL and force it to reset state // - Reset flag that helps us figure out when a nested REPL returns - main.nested_read_console_returned.set(false); + main.read_console_nested_return.set(false); // - Reset flag that helps us figure out when an error occurred and needs a // reset of `R_EvalDepth` and friends @@ -2467,7 +2467,7 @@ pub extern "C-unwind" fn r_read_console( // Set flag so that parent read console, if any, can detect that a // nested console returned (if it indeed returns instead of looping // for another iteration) - main.nested_read_console_returned.set(true); + main.read_console_nested_return.set(true); // Restore current frame main.read_console_frame.replace(old_current_frame); @@ -2507,7 +2507,7 @@ fn r_read_console_impl( // Check if a nested read_console() just returned. If that's the // case, we need to reset the `R_ConsoleIob` by first returning // a dummy value causing a `PARSE_NULL` event. - if main.nested_read_console_returned.get() { + if main.read_console_nested_return.get() { let next_input = RMain::console_input(buf, buflen); main.read_console_next_input.set(Some(next_input)); @@ -2516,7 +2516,7 @@ fn r_read_console_impl( // to interpret it as `n`, causing it to exit instead of // being a no-op. RMain::on_console_input(buf, buflen, String::from(" ")).unwrap(); - main.nested_read_console_returned.set(false); + main.read_console_nested_return.set(false); } libr::Rf_unprotect(2); From 337efde03d4ca30c2a62a902177c4ee5985ebc7f Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Thu, 6 Nov 2025 22:41:28 +0100 Subject: [PATCH 40/44] Don't include backtrace in syntax errors --- crates/amalthea/src/error.rs | 4 --- crates/ark/src/interface.rs | 59 +++++++++++++++++++++--------------- crates/ark/tests/kernel.rs | 30 +++++++++++++++--- 3 files changed, 61 insertions(+), 32 deletions(-) diff --git a/crates/amalthea/src/error.rs b/crates/amalthea/src/error.rs index c71a9aa59..119a39949 100644 --- a/crates/amalthea/src/error.rs +++ b/crates/amalthea/src/error.rs @@ -43,7 +43,6 @@ pub enum Error { UnknownCommName(String), UnknownCommId(String), InvalidCommMessage(String, String, String), - InvalidInputRequest(String), InvalidConsoleInput(String), Anyhow(anyhow::Error), ShellErrorReply(Exception), @@ -196,9 +195,6 @@ impl fmt::Display for Error { msg, id, err ) }, - Error::InvalidInputRequest(message) => { - write!(f, "{message}") - }, Error::InvalidConsoleInput(message) => { write!(f, "{message}") }, diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 5c3461287..54706e400 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -303,13 +303,18 @@ struct PendingInputs { index: isize, } +enum ParseResult { + Success(Option), + SyntaxError(String), +} + impl PendingInputs { - pub(crate) fn read(input: &str) -> anyhow::Result> { + pub(crate) fn read(input: &str) -> anyhow::Result> { let status = match harp::parse_status(&harp::ParseInput::Text(input)) { Err(err) => { // Failed to even attempt to parse the input, something is seriously wrong // FIXME: There are some valid syntax errors going through here, e.g. `identity |> _(1)`. - return Err(anyhow!("Failed to parse input: {err:?}")); + return Ok(ParseResult::SyntaxError(format!("{err}"))); }, Ok(status) => status, }; @@ -321,10 +326,12 @@ impl PendingInputs { let exprs = match status { harp::ParseResult::Complete(exprs) => exprs, harp::ParseResult::Incomplete => { - return Err(anyhow!("Can't execute incomplete input:\n{input}")); + return Ok(ParseResult::SyntaxError(format!( + "Can't execute incomplete input:\n{input}" + ))); }, harp::ParseResult::SyntaxError { message, .. } => { - return Err(anyhow!("Syntax error: {message}")); + return Ok(ParseResult::SyntaxError(format!("Syntax error: {message}"))); }, }; @@ -334,15 +341,15 @@ impl PendingInputs { let index = 0; if len == 0 { - return Ok(None); + return Ok(ParseResult::Success(None)); } - Ok(Some(Self { + Ok(ParseResult::Success(Some(Self { exprs, srcrefs, len, index, - })) + }))) } pub(crate) fn is_empty(&self) -> bool { @@ -444,7 +451,7 @@ pub(crate) enum ConsoleResult { NewPendingInput(PendingInput), Interrupt, Disconnected, - Error(amalthea::Error), + Error(String), } impl RMain { @@ -1262,10 +1269,17 @@ impl RMain { ConsoleInput::Input(code) => { // Parse input into pending expressions match PendingInputs::read(&code) { - Ok(inputs) => { + Ok(ParseResult::Success(inputs)) => { self.pending_inputs = inputs; }, - Err(err) => return Some(ConsoleResult::Error(amalthea::anyhow!("{err:?}"))), + Ok(ParseResult::SyntaxError(message)) => { + return Some(ConsoleResult::Error(message)) + }, + Err(err) => { + return Some(ConsoleResult::Error(format!( + "Error while parsing input: {err:?}" + ))) + }, } // Evaluate first expression if there is one @@ -1439,7 +1453,7 @@ impl RMain { log::info!("Detected `readline()` call in renv autoloader. Returning `'{input}'`."); match Self::on_console_input(buf, buflen, input) { Ok(()) => return ConsoleResult::NewInput, - Err(err) => return ConsoleResult::Error(err), + Err(err) => return ConsoleResult::Error(format!("{err}")), } } @@ -1450,7 +1464,7 @@ impl RMain { "Are you calling `readline()` or `menu()` from an `.Rprofile` or `.Rprofile.site` file? If so, that is the issue and you should remove that code." ].join("\n"); - return ConsoleResult::Error(Error::InvalidInputRequest(message)); + return ConsoleResult::Error(message); } fn start_debug(&mut self, debug_preserve_focus: bool) { @@ -1607,10 +1621,10 @@ impl RMain { let input = convert_line_endings(&input.value, LineEnding::Posix); match Self::on_console_input(buf, buflen, input) { Ok(()) => ConsoleResult::NewInput, - Err(err) => ConsoleResult::Error(err), + Err(err) => ConsoleResult::Error(format!("{err:?}")), } }, - Err(err) => ConsoleResult::Error(err), + Err(err) => ConsoleResult::Error(format!("{err:?}")), } } @@ -2328,14 +2342,6 @@ impl RMain { } } - fn propagate_error(&mut self, message: String) -> ! { - // Save error message to `RMain`'s buffer to avoid leaking memory when `Rf_error()` jumps. - // Some gymnastics are required to deal with the possibility of `CString` conversion failure - // since the error message comes from the frontend and might be corrupted. - self.r_error_buffer = Some(new_cstring(message)); - unsafe { Rf_error(self.r_error_buffer.as_ref().unwrap().as_ptr()) } - } - #[cfg(not(test))] // Avoid warnings in unit test pub(crate) fn read_console_frame(&self) -> RObject { self.read_console_frame.borrow().clone() @@ -2545,8 +2551,13 @@ fn r_read_console_impl( return 0; }, - ConsoleResult::Error(err) => { - main.propagate_error(format!("{err}")); + ConsoleResult::Error(message) => { + // Save error message in `RMain` to avoid leaking memory when + // `Rf_error()` jumps. Some gymnastics are required to deal with the + // possibility of `CString` conversion failure since the error + // message comes from the frontend and might be corrupted. + main.r_error_buffer = Some(new_cstring(message)); + unsafe { Rf_error(main.r_error_buffer.as_ref().unwrap().as_ptr()) } }, }; } diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 4d1e851be..63c99ca61 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -86,6 +86,11 @@ fn test_execute_request_multiple_lines() { #[test] fn test_execute_request_incomplete() { + // Set RUST_BACKTRACE to ensure backtraces are captured. We used to leak + // backtraces in syntax error messages, and this shouldn't happen even when + // `RUST_BACKTRACE` is set. + std::env::set_var("RUST_BACKTRACE", "1"); + let frontend = DummyArkFrontend::lock(); let code = "1 +"; @@ -95,9 +100,10 @@ fn test_execute_request_incomplete() { let input = frontend.recv_iopub_execute_input(); assert_eq!(input.code, code); - assert!(frontend - .recv_iopub_execute_error() - .contains("Can't execute incomplete input")); + assert_eq!( + frontend.recv_iopub_execute_error(), + "Error:\nCan't execute incomplete input:\n1 +" + ); frontend.recv_iopub_idle(); @@ -132,6 +138,11 @@ fn test_execute_request_incomplete_multiple_lines() { #[test] fn test_execute_request_invalid() { + // Set RUST_BACKTRACE to ensure backtraces are captured. We used to leak + // backtraces in syntax error messages, and this shouldn't happen even when + // `RUST_BACKTRACE` is set. + std::env::set_var("RUST_BACKTRACE", "1"); + let frontend = DummyArkFrontend::lock(); let code = "1 + )"; @@ -141,7 +152,13 @@ fn test_execute_request_invalid() { let input = frontend.recv_iopub_execute_input(); assert_eq!(input.code, code); - assert!(frontend.recv_iopub_execute_error().contains("Syntax error")); + let error_msg = frontend.recv_iopub_execute_error(); + + // Expected error + assert!(error_msg.contains("Syntax error")); + + // Check that no Rust backtrace is injected in the error message + assert!(!error_msg.contains("Stack backtrace:") && !error_msg.contains("std::backtrace")); frontend.recv_iopub_idle(); @@ -417,6 +434,11 @@ fn test_execute_request_browser_error() { #[test] fn test_execute_request_browser_incomplete() { + // Set RUST_BACKTRACE to ensure backtraces are captured. We used to leak + // backtraces in syntax error messages, and this shouldn't happen even when + // `RUST_BACKTRACE` is set. + std::env::set_var("RUST_BACKTRACE", "1"); + let frontend = DummyArkFrontend::lock(); let code = "browser()"; From b4839c29f82c4bbf71462efa1fd27f4596e2166f Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 7 Nov 2025 09:33:27 +0100 Subject: [PATCH 41/44] Fix backtraces in special syntax errors --- crates/ark/tests/kernel.rs | 49 +++++++++++++++++++++++++++++++++++++- crates/harp/src/error.rs | 5 ++-- crates/harp/src/parse.rs | 38 ++++++++++++++++++++--------- 3 files changed, 77 insertions(+), 15 deletions(-) diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 63c99ca61..2ec81c773 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -165,7 +165,54 @@ fn test_execute_request_invalid() { assert_eq!( frontend.recv_shell_execute_reply_exception(), input.execution_count - ) + ); + + // https://github.com/posit-dev/ark/issues/598 + let code = "``"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + let error_msg = frontend.recv_iopub_execute_error(); + + // Expected error + assert!(error_msg.contains("Syntax error")); + + // Check that no Rust backtrace is injected in the error message + assert!(!error_msg.contains("Stack backtrace:") && !error_msg.contains("std::backtrace")); + + frontend.recv_iopub_idle(); + + assert_eq!( + frontend.recv_shell_execute_reply_exception(), + input.execution_count + ); + + // https://github.com/posit-dev/ark/issues/722 + + let code = "_ + _()"; + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + let error_msg = frontend.recv_iopub_execute_error(); + + // Expected error + assert!(error_msg.contains("Syntax error")); + + // Check that no Rust backtrace is injected in the error message + assert!(!error_msg.contains("Stack backtrace:") && !error_msg.contains("std::backtrace")); + + frontend.recv_iopub_idle(); + + assert_eq!( + frontend.recv_shell_execute_reply_exception(), + input.execution_count + ); } #[test] diff --git a/crates/harp/src/error.rs b/crates/harp/src/error.rs index 5b4f2a5f5..5989986f0 100644 --- a/crates/harp/src/error.rs +++ b/crates/harp/src/error.rs @@ -46,7 +46,6 @@ pub enum Error { InvalidUtf8(Utf8Error), ParseSyntaxError { message: String, - line: i32, }, MissingValueError, MissingColumnError { @@ -200,8 +199,8 @@ impl fmt::Display for Error { write!(f, "Invalid UTF-8 in string: {}", error) }, - Error::ParseSyntaxError { message, line } => { - write!(f, "Syntax error on line {} when parsing: {}", line, message) + Error::ParseSyntaxError { message } => { + write!(f, "Syntax error: {}", message) }, Error::MissingValueError => { diff --git a/crates/harp/src/parse.rs b/crates/harp/src/parse.rs index 7e819a8ca..d2426876c 100644 --- a/crates/harp/src/parse.rs +++ b/crates/harp/src/parse.rs @@ -28,7 +28,7 @@ pub struct RParseOptions { pub enum ParseResult { Complete(RObject), Incomplete, - SyntaxError { message: String, line: i32 }, + SyntaxError { message: String }, } pub enum ParseInput<'a> { @@ -79,9 +79,7 @@ pub fn parse_exprs_ext<'a>(input: &ParseInput<'a>) -> crate::Result { code: parse_input_as_string(input).unwrap_or(String::from("Conversion error")), message: String::from("Incomplete code"), }), - ParseResult::SyntaxError { message, line } => { - Err(crate::Error::ParseSyntaxError { message, line }) - }, + ParseResult::SyntaxError { message } => Err(crate::Error::ParseSyntaxError { message }), } } @@ -109,17 +107,34 @@ pub fn parse_status<'a>(input: &ParseInput<'a>) -> crate::Result { ParseInput::SrcFile(srcfile) => (srcfile.lines()?, srcfile.inner.clone()), }; - let result: RObject = - try_catch(|| libr::R_ParseVector(text.sexp, -1, &mut status, srcfile.sexp).into())?; + let result = + try_catch(|| libr::R_ParseVector(text.sexp, -1, &mut status, srcfile.sexp).into()); + + let value = match result { + Ok(value) => value, + Err(err) => match err { + // The parser sometimes throws errors instead of returning an + // error flag. Convert these errors to proper syntax errors so + // we don't leak a backtrace making it seem like an internal + // error. + // https://github.com/posit-dev/ark/issues/598 + // https://github.com/posit-dev/ark/issues/722 + crate::Error::TryCatchError { message, .. } => { + return Ok(ParseResult::SyntaxError { message }); + }, + _ => { + return Err(err); + }, + }, + }; match status { - libr::ParseStatus_PARSE_OK => Ok(ParseResult::Complete(result)), + libr::ParseStatus_PARSE_OK => Ok(ParseResult::Complete(value)), libr::ParseStatus_PARSE_INCOMPLETE => Ok(ParseResult::Incomplete), libr::ParseStatus_PARSE_ERROR => Ok(ParseResult::SyntaxError { message: CStr::from_ptr(libr::get(libr::R_ParseErrorMsg).as_ptr()) .to_string_lossy() .to_string(), - line: libr::get(libr::R_ParseError) as i32, }), _ => { // Should not get here @@ -207,15 +222,16 @@ mod tests { // Error assert_match!( parse_status(&ParseInput::Text("42 + _")), - Err(_) => {} + Ok(ParseResult::SyntaxError { message }) => { + assert!(message.contains("invalid use of pipe placeholder")); + } ); // "normal" syntax error assert_match!( parse_status(&ParseInput::Text("1+1\n*42")), - Ok(ParseResult::SyntaxError {message, line}) => { + Ok(ParseResult::SyntaxError { message }) => { assert!(message.contains("unexpected")); - assert_eq!(line, 2); } ); From a4272d157c6c24584c821b5592c11b1eb54eb6a7 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 7 Nov 2025 09:50:35 +0100 Subject: [PATCH 42/44] Disable error entracing in sensitive tests --- crates/ark/tests/kernel.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index 2ec81c773..bb7d5741c 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -5,6 +5,9 @@ use amalthea::wire::kernel_info_request::KernelInfoRequest; use ark::fixtures::DummyArkFrontend; use stdext::assert_match; +// Avoids our global calling handler from rlangifying errors. +// This causes some test instability across configs. + #[test] fn test_kernel_info() { let frontend = DummyArkFrontend::lock(); @@ -93,6 +96,18 @@ fn test_execute_request_incomplete() { let frontend = DummyArkFrontend::lock(); + let code = "options(positron.error_entrace = FALSE)"; + + frontend.send_execute_request(code, ExecuteRequestOptions::default()); + frontend.recv_iopub_busy(); + + let input = frontend.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + frontend.recv_iopub_idle(); + + assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + let code = "1 +"; frontend.send_execute_request(code, ExecuteRequestOptions::default()); frontend.recv_iopub_busy(); From 0b9052c9d12141775552b1895d36f171d5ee9624 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Fri, 7 Nov 2025 10:08:16 +0100 Subject: [PATCH 43/44] Extract `FrontendDummy::execute_request()` and variants --- .../amalthea/src/fixtures/dummy_frontend.rs | 71 +++ crates/ark/tests/kernel.rs | 460 +++--------------- 2 files changed, 131 insertions(+), 400 deletions(-) diff --git a/crates/amalthea/src/fixtures/dummy_frontend.rs b/crates/amalthea/src/fixtures/dummy_frontend.rs index 768523601..09b0bfe18 100644 --- a/crates/amalthea/src/fixtures/dummy_frontend.rs +++ b/crates/amalthea/src/fixtures/dummy_frontend.rs @@ -236,6 +236,77 @@ impl DummyFrontend { }) } + /// Sends an execute request and handles the standard message flow: + /// busy -> execute_input -> idle -> execute_reply. + /// Asserts that the input code matches and returns the execution count. + #[track_caller] + pub fn execute_request_invisibly(&self, code: &str) -> u32 { + self.send_execute_request(code, ExecuteRequestOptions::default()); + self.recv_iopub_busy(); + + let input = self.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + self.recv_iopub_idle(); + + let execution_count = self.recv_shell_execute_reply(); + assert_eq!(execution_count, input.execution_count); + + execution_count + } + + /// Sends an execute request and handles the standard message flow with a result: + /// busy -> execute_input -> execute_result -> idle -> execute_reply. + /// Asserts that the input code matches and passes the result to the callback. + /// Returns the execution count. + #[track_caller] + pub fn execute_request(&self, code: &str, result_check: F) -> u32 + where + F: FnOnce(String), + { + self.send_execute_request(code, ExecuteRequestOptions::default()); + self.recv_iopub_busy(); + + let input = self.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + let result = self.recv_iopub_execute_result(); + result_check(result); + + self.recv_iopub_idle(); + + let execution_count = self.recv_shell_execute_reply(); + assert_eq!(execution_count, input.execution_count); + + execution_count + } + + /// Sends an execute request that produces an error and handles the standard message flow: + /// busy -> execute_input -> execute_error -> idle -> execute_reply_exception. + /// Passes the error message to the callback for custom assertions. + /// Returns the execution count. + #[track_caller] + pub fn execute_request_error(&self, code: &str, error_check: F) -> u32 + where + F: FnOnce(String), + { + self.send_execute_request(code, ExecuteRequestOptions::default()); + self.recv_iopub_busy(); + + let input = self.recv_iopub_execute_input(); + assert_eq!(input.code, code); + + let error_msg = self.recv_iopub_execute_error(); + error_check(error_msg); + + self.recv_iopub_idle(); + + let execution_count = self.recv_shell_execute_reply_exception(); + assert_eq!(execution_count, input.execution_count); + + execution_count + } + /// Sends a Jupyter message on the Stdin socket pub fn send_stdin(&self, msg: T) { Self::send(&self.stdin_socket, &self.session, msg); diff --git a/crates/ark/tests/kernel.rs b/crates/ark/tests/kernel.rs index bb7d5741c..fe1988899 100644 --- a/crates/ark/tests/kernel.rs +++ b/crates/ark/tests/kernel.rs @@ -28,63 +28,24 @@ fn test_kernel_info() { #[test] fn test_execute_request() { let frontend = DummyArkFrontend::lock(); - - let code = "42"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - assert_eq!(frontend.recv_iopub_execute_result(), "[1] 42"); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request("42", |result| assert_eq!(result, "[1] 42")); } #[test] fn test_execute_request_empty() { let frontend = DummyArkFrontend::lock(); - let code = ""; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly(""); // Equivalent to invisible output - let code = "invisible(1)"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("invisible(1)"); } #[test] fn test_execute_request_multiple_lines() { let frontend = DummyArkFrontend::lock(); - let code = "1 +\n 2+\n 3"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - assert_eq!(frontend.recv_iopub_execute_result(), "[1] 6"); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count) + frontend.execute_request("1 +\n 2+\n 3", |result| assert_eq!(result, "[1] 6")); } #[test] @@ -96,59 +57,20 @@ fn test_execute_request_incomplete() { let frontend = DummyArkFrontend::lock(); - let code = "options(positron.error_entrace = FALSE)"; - - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); + frontend.execute_request_invisibly("options(positron.error_entrace = FALSE)"); - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); - - let code = "1 +"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert_eq!( - frontend.recv_iopub_execute_error(), - "Error:\nCan't execute incomplete input:\n1 +" - ); - - frontend.recv_iopub_idle(); - - assert_eq!( - frontend.recv_shell_execute_reply_exception(), - input.execution_count - ) + frontend.execute_request_error("1 +", |error_msg| { + assert_eq!(error_msg, "Error:\nCan't execute incomplete input:\n1 +"); + }); } #[test] fn test_execute_request_incomplete_multiple_lines() { let frontend = DummyArkFrontend::lock(); - let code = "1 +\n2 +"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert!(frontend - .recv_iopub_execute_error() - .contains("Can't execute incomplete input")); - - frontend.recv_iopub_idle(); - - assert_eq!( - frontend.recv_shell_execute_reply_exception(), - input.execution_count - ) + frontend.execute_request_error("1 +\n2 +", |error_msg| { + assert!(error_msg.contains("Can't execute incomplete input")); + }); } #[test] @@ -160,74 +82,22 @@ fn test_execute_request_invalid() { let frontend = DummyArkFrontend::lock(); - let code = "1 + )"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - let error_msg = frontend.recv_iopub_execute_error(); - - // Expected error - assert!(error_msg.contains("Syntax error")); - - // Check that no Rust backtrace is injected in the error message - assert!(!error_msg.contains("Stack backtrace:") && !error_msg.contains("std::backtrace")); - - frontend.recv_iopub_idle(); - - assert_eq!( - frontend.recv_shell_execute_reply_exception(), - input.execution_count - ); + frontend.execute_request_error("1 + )", |error_msg| { + assert!(error_msg.contains("Syntax error")); + assert!(!error_msg.contains("Stack backtrace:") && !error_msg.contains("std::backtrace")); + }); // https://github.com/posit-dev/ark/issues/598 - let code = "``"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - let error_msg = frontend.recv_iopub_execute_error(); - - // Expected error - assert!(error_msg.contains("Syntax error")); - - // Check that no Rust backtrace is injected in the error message - assert!(!error_msg.contains("Stack backtrace:") && !error_msg.contains("std::backtrace")); - - frontend.recv_iopub_idle(); - - assert_eq!( - frontend.recv_shell_execute_reply_exception(), - input.execution_count - ); + frontend.execute_request_error("``", |error_msg| { + assert!(error_msg.contains("Syntax error")); + assert!(!error_msg.contains("Stack backtrace:") && !error_msg.contains("std::backtrace")); + }); // https://github.com/posit-dev/ark/issues/722 - - let code = "_ + _()"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - let error_msg = frontend.recv_iopub_execute_error(); - - // Expected error - assert!(error_msg.contains("Syntax error")); - - // Check that no Rust backtrace is injected in the error message - assert!(!error_msg.contains("Stack backtrace:") && !error_msg.contains("std::backtrace")); - - frontend.recv_iopub_idle(); - - assert_eq!( - frontend.recv_shell_execute_reply_exception(), - input.execution_count - ); + frontend.execute_request_error("_ + _()", |error_msg| { + assert!(error_msg.contains("Syntax error")); + assert!(!error_msg.contains("Stack backtrace:") && !error_msg.contains("std::backtrace")); + }); } #[test] @@ -249,16 +119,7 @@ fn test_execute_request_browser() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); - let code = "Q"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("Q"); } #[test] @@ -280,16 +141,7 @@ fn test_execute_request_browser_continue() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); - let code = "n"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("n"); } #[test] @@ -314,61 +166,17 @@ fn test_execute_request_browser_nested() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); // Evaluate a value in the outer browser - let code = "42"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert!(frontend.recv_iopub_execute_result().contains("[1] 42")); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request("42", |result| assert!(result.contains("[1] 42"))); // Start nested browser from within the first browser - let code = "browser()"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - // Nested browser() produces execute_result output - frontend.recv_iopub_execute_result(); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request("browser()", |_result| {}); // Evaluate a command in the nested browser - let code = "1"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert!(frontend.recv_iopub_execute_result().contains("[1] 1")); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request("1", |result| assert!(result.contains("[1] 1"))); // Evaluate another value in the nested browser - let code = "\"hello\""; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert!(frontend.recv_iopub_execute_result().contains("hello")); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request("\"hello\"", |result| assert!(result.contains("hello"))); // Throw an error in the nested browser let code = "stop('error in nested')"; @@ -384,30 +192,10 @@ fn test_execute_request_browser_nested() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); // Continue to exit the nested browser and return to parent - let code = "c"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("c"); // Back in the parent browser, evaluate another value - let code = "3.14"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert!(frontend.recv_iopub_execute_result().contains("[1] 3.14")); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request("3.14", |result| assert!(result.contains("[1] 3.14"))); // Throw an error in the outer browser let code = "stop('error in parent')"; @@ -422,29 +210,9 @@ fn test_execute_request_browser_nested() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); - let code = "NA"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert!(frontend.recv_iopub_execute_result().contains("[1] NA")); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request("NA", |result| assert!(result.contains("[1] NA"))); // Quit the outer browser - let code = "Q"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("Q"); } #[test] @@ -482,16 +250,7 @@ fn test_execute_request_browser_error() { assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); - let code = "Q"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("Q"); } #[test] @@ -582,16 +341,7 @@ fn()"; assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); - let code = "Q"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("Q"); } #[test] @@ -630,16 +380,7 @@ fn test_execute_request_browser_stdin() { frontend.recv_iopub_idle(); assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); - let code = "Q"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("Q"); } #[test] @@ -735,35 +476,16 @@ fn test_execute_request_browser_local_variable() { frontend.recv_iopub_idle(); assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); - let code = "Q"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("Q"); } #[test] fn test_execute_request_error() { let frontend = DummyArkFrontend::lock(); - frontend.send_execute_request("stop('foobar')", ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, "stop('foobar')"); - assert!(frontend.recv_iopub_execute_error().contains("foobar")); - - frontend.recv_iopub_idle(); - - assert_eq!( - frontend.recv_shell_execute_reply_exception(), - input.execution_count - ); + frontend.execute_request_error("stop('foobar')", |error_msg| { + assert!(error_msg.contains("foobar")); + }); } #[test] @@ -802,35 +524,17 @@ fn test_execute_request_error_with_accumulated_output() { #[test] fn test_execute_request_error_expressions_overflow() { let frontend = DummyArkFrontend::lock(); - // Deterministically produce an "evaluation too deeply nested" error - let code = "options(expressions = 100); f <- function(x) if (x > 0 ) f(x - 1); f(100)"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert!(frontend - .recv_iopub_execute_error() - .contains("evaluation nested too deeply")); - - frontend.recv_iopub_idle(); - assert_eq!( - frontend.recv_shell_execute_reply_exception(), - input.execution_count + // Deterministically produce an "evaluation too deeply nested" error + frontend.execute_request_error( + "options(expressions = 100); f <- function(x) if (x > 0 ) f(x - 1); f(100)", + |error_msg| { + assert!(error_msg.contains("evaluation nested too deeply")); + }, ); // Check we can still evaluate without causing another too deeply nested error - let code = "f(10)"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly("f(10)"); } #[test] @@ -838,59 +542,24 @@ fn test_execute_request_error_expressions_overflow_last_value() { let frontend = DummyArkFrontend::lock(); // Set state and last value - let code = - "options(expressions = 100); f <- function(x) if (x > 0 ) f(x - 1); invisible('hello')"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - frontend.recv_iopub_idle(); - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request_invisibly( + "options(expressions = 100); f <- function(x) if (x > 0 ) f(x - 1); invisible('hello')", + ); // Check last value is set - let code = ".Last.value"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"hello\""); - frontend.recv_iopub_idle(); - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request(".Last.value", |result| { + assert_eq!(result, "[1] \"hello\""); + }); // Deterministically produce an "evaluation too deeply nested" error - let code = "f(100)"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert!(frontend - .recv_iopub_execute_error() - .contains("evaluation nested too deeply")); - - frontend.recv_iopub_idle(); - - assert_eq!( - frontend.recv_shell_execute_reply_exception(), - input.execution_count - ); + frontend.execute_request_error("f(100)", |error_msg| { + assert!(error_msg.contains("evaluation nested too deeply")); + }); // Check last value is still set - let code = ".Last.value"; - frontend.send_execute_request(code, ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert_eq!(frontend.recv_iopub_execute_result(), "[1] \"hello\""); - frontend.recv_iopub_idle(); - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request(".Last.value", |result| { + assert_eq!(result, "[1] \"hello\""); + }); } #[test] @@ -953,16 +622,7 @@ fn test_execute_request_single_line_buffer_overflow() { // not in text written to the R buffer that calls `stop()`. let aaa = "a".repeat(4096); let code = format!("quote(\n{aaa}\n)"); - frontend.send_execute_request(code.as_str(), ExecuteRequestOptions::default()); - frontend.recv_iopub_busy(); - - let input = frontend.recv_iopub_execute_input(); - assert_eq!(input.code, code); - - assert!(frontend.recv_iopub_execute_result().contains(&aaa)); - - frontend.recv_iopub_idle(); - assert_eq!(frontend.recv_shell_execute_reply(), input.execution_count); + frontend.execute_request(code.as_str(), |result| assert!(result.contains(&aaa))); } #[test] From 7de4a5031f8af66d11136d74652ba71b6dcb8b1f Mon Sep 17 00:00:00 2001 From: Davis Vaughan Date: Tue, 11 Nov 2025 09:34:01 -0500 Subject: [PATCH 44/44] Make it clear that `srcrefs` is not always present --- crates/ark/src/interface.rs | 25 ++++++++++++++++++++----- 1 file changed, 20 insertions(+), 5 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 54706e400..0fc04d1d8 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -71,6 +71,7 @@ use harp::line_ending::convert_line_endings; use harp::line_ending::LineEnding; use harp::object::r_null_or_try_into; use harp::object::RObject; +use harp::r_null; use harp::r_symbol; use harp::routines::r_register_routines; use harp::session::r_traceback; @@ -295,8 +296,8 @@ pub struct RMain { struct PendingInputs { /// EXPRSXP vector of parsed expressions exprs: RObject, - /// List of srcrefs, the same length as `exprs` - srcrefs: RObject, + /// Srcref of each expression. If known, this is a list the same length as `exprs`. + srcrefs: Option, /// Length of `exprs` and `srcrefs` len: isize, /// Index into the stack @@ -336,6 +337,11 @@ impl PendingInputs { }; let srcrefs = get_block_srcrefs(exprs.sexp); + let srcrefs = if srcrefs.is_null() { + None + } else { + Some(srcrefs) + }; let len = exprs.length(); let index = 0; @@ -361,7 +367,11 @@ impl PendingInputs { return None; } - let srcref = get_srcref(self.srcrefs.sexp, self.index); + let srcref = self + .srcrefs + .as_ref() + .map(|srcrefs| get_srcref(srcrefs.sexp, self.index)); + let expr = harp::r_list_get(self.exprs.sexp, self.index); self.index += 1; @@ -372,7 +382,7 @@ impl PendingInputs { #[derive(Debug)] pub(crate) struct PendingInput { expr: RObject, - srcref: RObject, + srcref: Option, } #[derive(Debug, Clone)] @@ -2506,7 +2516,12 @@ fn r_read_console_impl( // after a longjump to top-level, so it's safe to protect here // even if the evaluation throws let expr = libr::Rf_protect(expr.into()); - let srcref = libr::Rf_protect(srcref.into()); + + let srcref = match srcref { + Some(srcref) => srcref.into(), + None => r_null(), + }; + libr::Rf_protect(srcref); RMain::eval(expr, srcref, buf, buflen);