From 06fee84935ed0bdac47eed630dfc10f7ec786e3a Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 20 Jun 2023 15:19:13 +0200 Subject: [PATCH 1/6] Add note about implementing a `Cleanup` method --- crates/ark/src/interface.rs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 6519f200f..6e624a4df 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -217,6 +217,9 @@ pub extern "C" fn r_read_console( // TODO: Can we remove this below code? // If the prompt begins with "Save workspace", respond with (n) + // + // NOTE: Should be able to overwrite the `Cleanup` frontend method. + // This would also help with detecting normal exits versus crashes. if r_prompt.to_str().unwrap().starts_with("Save workspace") { let n = CString::new("n\n").unwrap(); unsafe { From 3c81fbcdb935dd75b9110a8339efb5f6cd9e83bc Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Tue, 20 Jun 2023 16:28:47 +0200 Subject: [PATCH 2/6] Wrap prompt in a struct with metadata --- crates/ark/src/interface.rs | 92 ++++++++++++++++++++++++------------- 1 file changed, 59 insertions(+), 33 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 6e624a4df..5e7f58885 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -144,7 +144,7 @@ pub static mut KERNEL: Option>> = None; pub static mut R_RUNTIME_LOCK_GUARD: Option> = None; /// A channel that sends prompts from R to the kernel -static mut RPROMPT_SEND: Option>> = None; +static mut RPROMPT_SEND: Option>> = None; /// A channel that receives console input from the kernel and sends it to R; /// sending empty input (None) tells R to shut down @@ -212,15 +212,15 @@ pub extern "C" fn r_read_console( buflen: c_int, _hist: c_int, ) -> i32 { - let r_prompt = unsafe { CStr::from_ptr(prompt) }; - debug!("R prompt: {}", r_prompt.to_str().unwrap()); + let info = prompt_info(prompt); + debug!("R prompt: {}", info.prompt); // TODO: Can we remove this below code? // If the prompt begins with "Save workspace", respond with (n) // // NOTE: Should be able to overwrite the `Cleanup` frontend method. // This would also help with detecting normal exits versus crashes. - if r_prompt.to_str().unwrap().starts_with("Save workspace") { + if info.prompt.starts_with("Save workspace") { let n = CString::new("n\n").unwrap(); unsafe { libc::strcpy(buf as *mut c_char, n.as_ptr()); @@ -231,9 +231,7 @@ pub extern "C" fn r_read_console( // TODO: if R prompt is +, we need to tell the user their input is incomplete let mutex = unsafe { RPROMPT_SEND.as_ref().unwrap() }; let r_prompt_tx = mutex.lock().unwrap(); - r_prompt_tx - .send(r_prompt.to_string_lossy().into_owned()) - .unwrap(); + r_prompt_tx.send(info).unwrap(); let mutex = unsafe { CONSOLE_RECV.as_ref().unwrap() }; let receiver = mutex.lock().unwrap(); @@ -291,6 +289,51 @@ pub extern "C" fn r_read_console( } } +/** + * 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. + * + * TODO: `browser` field */ +pub struct PromptInfo { + /** The prompt string to be presented to the user */ + prompt: String, + + /** 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()` */ + user_request: bool, +} + +fn prompt_info(prompt_c: *const c_char) -> PromptInfo { + let prompt_slice = unsafe { CStr::from_ptr(prompt_c) }; + let prompt = prompt_slice.to_string_lossy().into_owned(); + + // The request is incomplete if we see the continue prompt + let continue_prompt = unsafe { r_get_option::("continue").unwrap() }; + let incomplete = prompt == continue_prompt; + + // If the current prompt doesn't match the default prompt, assume that + // we're reading use input, e.g. via 'readline()'. + let default_prompt = unsafe { r_get_option::("prompt").unwrap() }; + let user_request = !incomplete && prompt != default_prompt; + + if incomplete { + trace!("Got R prompt '{}', marking request incomplete", prompt); + } else if user_request { + trace!("Got R prompt '{}', asking user for input", prompt); + } + + return PromptInfo { + prompt, + incomplete, + user_request, + }; +} + /** * Invoked by R to write output to the console. */ @@ -472,7 +515,7 @@ pub fn start_r( } } -fn handle_r_request(req: &Request, prompt_recv: &Receiver) { +fn handle_r_request(req: &Request, prompt_recv: &Receiver) { // Service the request. let mutex = unsafe { KERNEL.as_ref().unwrap() }; { @@ -487,41 +530,24 @@ fn handle_r_request(req: &Request, prompt_recv: &Receiver) { } } -fn complete_execute_request(req: &Request, prompt_recv: &Receiver) { +fn complete_execute_request(req: &Request, prompt_recv: &Receiver) { let mutex = unsafe { KERNEL.as_ref().unwrap() }; // Wait for R to prompt us again. This signals that the // execution is finished and R is ready for input again. trace!("Waiting for R prompt signaling completion of execution..."); - let prompt = prompt_recv.recv().unwrap(); + let prompt_info = prompt_recv.recv().unwrap(); + let prompt = prompt_info.prompt; let kernel = mutex.lock().unwrap(); // Signal prompt EVENTS.console_prompt.emit(()); - // The request is incomplete if we see the continue prompt - let continue_prompt = unsafe { r_get_option::("continue") }; - let continue_prompt = unwrap!(continue_prompt, Err(error) => { - warn!("Failed to read R continue prompt: {}", error); - return kernel.finish_request(); - }); - - if prompt == continue_prompt { - trace!("Got R prompt '{}', marking request incomplete", prompt); + if prompt_info.incomplete { return kernel.report_incomplete_request(&req); } - // Figure out what the default prompt looks like. - let default_prompt = unsafe { r_get_option::("prompt") }; - let default_prompt = unwrap!(default_prompt, Err(error) => { - warn!("Failed to read R prompt: {}", error); - return kernel.finish_request(); - }); - - // If the current prompt doesn't match the default prompt, assume that - // we're reading use input, e.g. via 'readline()'. - if prompt != default_prompt { - trace!("Got R prompt '{}', asking user for input", prompt); + if prompt_info.user_request { if let Request::ExecuteCode(_, originator, _) = req { kernel.request_input(originator.clone(), &prompt); } else { @@ -538,14 +564,14 @@ fn complete_execute_request(req: &Request, prompt_recv: &Receiver) { return kernel.finish_request(); } -pub fn listen(exec_recv: Receiver, prompt_recv: Receiver) { +pub fn listen(exec_recv: Receiver, prompt_recv: Receiver) { // Before accepting execution requests from the front end, wait for R to // prompt us for input. trace!("Waiting for R's initial input prompt..."); - let prompt = prompt_recv.recv().unwrap(); + let info = prompt_recv.recv().unwrap(); trace!( "Got initial R prompt '{}', ready for execution requests", - prompt + info.prompt ); // Mark kernel as initialized as soon as we get the first input prompt from R From 9cc7658c07a615669de387d550a71373342498b3 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 21 Jun 2023 12:21:08 +0200 Subject: [PATCH 3/6] Use `nframe()` to detect prompts like `readline()` --- crates/ark/src/interface.rs | 26 +++++++++++++++++++------- crates/harp/src/lib.rs | 3 ++- crates/harp/src/session.rs | 36 ++++++++++++++++++++++++++++++++++++ 3 files changed, 57 insertions(+), 8 deletions(-) create mode 100644 crates/harp/src/session.rs diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 5e7f58885..de6282589 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -309,17 +309,29 @@ pub struct PromptInfo { } fn prompt_info(prompt_c: *const c_char) -> PromptInfo { + let n_frame = unwrap!(harp::session::r_n_frame(), Err(err) => { + warn!("`r_n_frame()` failed: {}", err); + 0 + }); + trace!("prompt_info(): n_frame = '{}'", n_frame); + let prompt_slice = unsafe { CStr::from_ptr(prompt_c) }; let prompt = prompt_slice.to_string_lossy().into_owned(); - // The request is incomplete if we see the continue prompt - let continue_prompt = unsafe { r_get_option::("continue").unwrap() }; - let incomplete = prompt == continue_prompt; + // TODO: Detect with `env_is_browsed(sys.frame(sys.nframe()))` + let browser = false; - // If the current prompt doesn't match the default prompt, assume that - // we're reading use input, e.g. via 'readline()'. - let default_prompt = unsafe { r_get_option::("prompt").unwrap() }; - let user_request = !incomplete && prompt != default_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("+ ")` + let continue_prompt = unwrap!(unsafe { r_get_option::("continue") }, Err(err) => { + warn!("`r_get_option()` failed: {}", err); + String::from("+ ") + }); + let incomplete = !user_request && prompt == continue_prompt; if incomplete { trace!("Got R prompt '{}', marking request incomplete", prompt); diff --git a/crates/harp/src/lib.rs b/crates/harp/src/lib.rs index 3439a34a5..3b27953bd 100644 --- a/crates/harp/src/lib.rs +++ b/crates/harp/src/lib.rs @@ -14,6 +14,7 @@ pub mod lock; pub mod object; pub mod protect; pub mod routines; +pub mod session; pub mod string; pub mod symbol; pub mod test; @@ -162,7 +163,7 @@ macro_rules! r_lang { ($($tts:tt)*) => {{ let value = $crate::r_pairlist!($($tts)*); - libR_sys::SET_TYPEOF(value, LISTSXP as i32); + libR_sys::SET_TYPEOF(value, LANGSXP as i32); value }} diff --git a/crates/harp/src/session.rs b/crates/harp/src/session.rs new file mode 100644 index 000000000..e3c7a65bf --- /dev/null +++ b/crates/harp/src/session.rs @@ -0,0 +1,36 @@ +// +// session.rs +// +// Copyright (C) 2023 Posit Software, PBC. All rights reserved. +// +// + +use std::sync::Once; + +use libR_sys::*; + +use crate::utils::r_try_eval_silent; +use crate::vector::integer_vector::IntegerVector; +use crate::vector::Vector; + +pub fn r_n_frame() -> crate::error::Result { + SESSION_INIT.call_once(init_interface); + + unsafe { + let ffi = r_try_eval_silent(NFRAME_CALL as SEXP, R_BaseEnv)?; + let n_frame = IntegerVector::new(ffi)?; + Ok(n_frame.get_unchecked_elt(0)) + } +} + +// Globals +static SESSION_INIT: Once = Once::new(); +static mut NFRAME_CALL: usize = 0; + +fn init_interface() { + unsafe { + let nframe_call = crate::r_lang!(crate::r_symbol!("sys.nframe")); + R_PreserveObject(nframe_call); + NFRAME_CALL = nframe_call as usize; + } +} From 956a120078153951b7c029bcb154a6b3e3625606 Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Wed, 21 Jun 2023 14:45:14 +0200 Subject: [PATCH 4/6] Panic instead of log in `prompt_info()` --- crates/ark/src/interface.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index de6282589..1a09d9e90 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -308,11 +308,11 @@ pub struct PromptInfo { user_request: bool, } +// We prefer to panic if there is an error while trying to determine the +// prompt type because any confusion here is prone to put the frontend in a +// bad state (e.g. causing freezes) fn prompt_info(prompt_c: *const c_char) -> PromptInfo { - let n_frame = unwrap!(harp::session::r_n_frame(), Err(err) => { - warn!("`r_n_frame()` failed: {}", err); - 0 - }); + let n_frame = harp::session::r_n_frame().unwrap(); trace!("prompt_info(): n_frame = '{}'", n_frame); let prompt_slice = unsafe { CStr::from_ptr(prompt_c) }; @@ -327,10 +327,7 @@ fn prompt_info(prompt_c: *const c_char) -> PromptInfo { // The request is incomplete if we see the continue prompt, except if // we're in a user request, e.g. `readline("+ ")` - let continue_prompt = unwrap!(unsafe { r_get_option::("continue") }, Err(err) => { - warn!("`r_get_option()` failed: {}", err); - String::from("+ ") - }); + let continue_prompt = unsafe { r_get_option::("continue").unwrap() }; let incomplete = !user_request && prompt == continue_prompt; if incomplete { From 96925672c61825a402155b266f6796484569deaa Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 26 Jun 2023 21:46:56 +0200 Subject: [PATCH 5/6] Use conventional docstrings --- crates/ark/src/interface.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/crates/ark/src/interface.rs b/crates/ark/src/interface.rs index 1a09d9e90..c6801b78a 100644 --- a/crates/ark/src/interface.rs +++ b/crates/ark/src/interface.rs @@ -289,24 +289,21 @@ pub extern "C" fn r_read_console( } } -/** - * 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. - * - * TODO: `browser` field */ +/// 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. pub struct PromptInfo { - /** The prompt string to be presented to the user */ + /// The prompt string to be presented to the user prompt: String, - /** Whether the last input didn't fully parse and R is waiting for more - * input */ + /// 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()` */ + /// 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()` user_request: bool, } +// TODO: `browser` field // We prefer to panic if there is an error while trying to determine the // prompt type because any confusion here is prone to put the frontend in a From 91612f36121d3a0a690b71d751352cb93c05ef7d Mon Sep 17 00:00:00 2001 From: Lionel Henry Date: Mon, 26 Jun 2023 21:48:18 +0200 Subject: [PATCH 6/6] Move globals to top of file --- crates/harp/src/session.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/harp/src/session.rs b/crates/harp/src/session.rs index e3c7a65bf..9088b519f 100644 --- a/crates/harp/src/session.rs +++ b/crates/harp/src/session.rs @@ -13,6 +13,10 @@ use crate::utils::r_try_eval_silent; use crate::vector::integer_vector::IntegerVector; use crate::vector::Vector; +// Globals +static SESSION_INIT: Once = Once::new(); +static mut NFRAME_CALL: usize = 0; + pub fn r_n_frame() -> crate::error::Result { SESSION_INIT.call_once(init_interface); @@ -23,10 +27,6 @@ pub fn r_n_frame() -> crate::error::Result { } } -// Globals -static SESSION_INIT: Once = Once::new(); -static mut NFRAME_CALL: usize = 0; - fn init_interface() { unsafe { let nframe_call = crate::r_lang!(crate::r_symbol!("sys.nframe"));