Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor prompt detection #51

Merged
merged 6 commits into from
Jun 26, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
101 changes: 68 additions & 33 deletions crates/ark/src/interface.rs
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ pub static mut KERNEL: Option<Arc<Mutex<Kernel>>> = None;
pub static mut R_RUNTIME_LOCK_GUARD: Option<ReentrantMutexGuard<()>> = None;

/// A channel that sends prompts from R to the kernel
static mut RPROMPT_SEND: Option<Mutex<Sender<String>>> = None;
static mut RPROMPT_SEND: Option<Mutex<Sender<PromptInfo>>> = None;

/// A channel that receives console input from the kernel and sends it to R;
/// sending empty input (None) tells R to shut down
Expand Down Expand Up @@ -212,12 +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)
if r_prompt.to_str().unwrap().starts_with("Save workspace") {
//
// NOTE: Should be able to overwrite the `Cleanup` frontend method.
// This would also help with detecting normal exits versus crashes.
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());
Expand All @@ -228,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();
Expand Down Expand Up @@ -288,6 +289,57 @@ 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.
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,
}
// 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
// bad state (e.g. causing freezes)
fn prompt_info(prompt_c: *const c_char) -> PromptInfo {
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) };
let prompt = prompt_slice.to_string_lossy().into_owned();

// TODO: Detect with `env_is_browsed(sys.frame(sys.nframe()))`
let browser = false;

// 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 = unsafe { r_get_option::<String>("continue").unwrap() };
let incomplete = !user_request && prompt == continue_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.
*/
Expand Down Expand Up @@ -469,7 +521,7 @@ pub fn start_r(
}
}

fn handle_r_request(req: &Request, prompt_recv: &Receiver<String>) {
fn handle_r_request(req: &Request, prompt_recv: &Receiver<PromptInfo>) {
// Service the request.
let mutex = unsafe { KERNEL.as_ref().unwrap() };
{
Expand All @@ -484,41 +536,24 @@ fn handle_r_request(req: &Request, prompt_recv: &Receiver<String>) {
}
}

fn complete_execute_request(req: &Request, prompt_recv: &Receiver<String>) {
fn complete_execute_request(req: &Request, prompt_recv: &Receiver<PromptInfo>) {
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::<String>("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::<String>("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 {
Expand All @@ -535,14 +570,14 @@ fn complete_execute_request(req: &Request, prompt_recv: &Receiver<String>) {
return kernel.finish_request();
}

pub fn listen(exec_recv: Receiver<Request>, prompt_recv: Receiver<String>) {
pub fn listen(exec_recv: Receiver<Request>, prompt_recv: Receiver<PromptInfo>) {
// 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
Expand Down
3 changes: 2 additions & 1 deletion crates/harp/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
}}

Expand Down
36 changes: 36 additions & 0 deletions crates/harp/src/session.rs
Original file line number Diff line number Diff line change
@@ -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;

// Globals
static SESSION_INIT: Once = Once::new();
static mut NFRAME_CALL: usize = 0;

pub fn r_n_frame() -> crate::error::Result<i32> {
SESSION_INIT.call_once(init_interface);

unsafe {
let ffi = r_try_eval_silent(NFRAME_CALL as SEXP, R_BaseEnv)?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it worth considering putting together tools for interacting with R_GlobalContext to learn these sorts of things about the call stack? (I think that might be necessary in the future, for certain intermediate frames that R doesn't include in the user-facing R functions?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a possibility. Probably best to avoid if we can though?

let n_frame = IntegerVector::new(ffi)?;
Ok(n_frame.get_unchecked_elt(0))
}
}

fn init_interface() {
unsafe {
let nframe_call = crate::r_lang!(crate::r_symbol!("sys.nframe"));
R_PreserveObject(nframe_call);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can preserve these using our own internal mechanism (to avoid accumulating a potentially large "bag" of preserved objects)? Is that worth doing?

That is, have an internal "precious list" that we append / remove these from, which is itself preserved?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main advantage of preserving in local storage instead of the global precious list is that it makes it easier to analyse the source of leaks using r-lib/memtools.

NFRAME_CALL = nframe_call as usize;
}
}