POC for r_lock! enforcement on the R API

crates/ark/src/interface.rs

@@ -26,6 +26,7 @@ use crossbeam::channel::Sender;
 use harp::interrupts::RInterruptsSuspendedScope;
 use harp::lock::R_RUNTIME_LOCK;
 use harp::lock::R_RUNTIME_LOCK_COUNT;
+use harp::r_lock;
 use harp::routines::r_register_routines;
 use harp::utils::r_get_option;
 use libR_sys::*;
@@ -135,7 +136,7 @@ extern "C" fn handle_interrupt(_signal: libc::c_int) {
 /// The global R kernel state.
 pub static mut KERNEL: Option<Arc<Mutex<Kernel>>> = None;
 
-/// A lock guard, used to manage access to the R runtime.  The main thread holds
+/// A lock guard, used to manage access to the R runtime. The main thread holds
 /// the lock by default, but releases it at opportune times to allow the LSP to
 /// access the R runtime where appropriate.
 pub static mut R_RUNTIME_LOCK_GUARD: Option<MutexGuard<()>> = None;
@@ -264,6 +265,7 @@ pub extern "C" fn r_read_console(
             },
 
             Err(error) => {
+                // Take back the lock after we've timed out.
                 unsafe { R_RUNTIME_LOCK_GUARD = Some(R_RUNTIME_LOCK.lock()) };
 
                 use RecvTimeoutError::*;
@@ -482,8 +484,11 @@ fn complete_execute_request(req: &Request, prompt_recv: &Receiver<String>) {
     // Signal prompt
     EVENTS.console_prompt.emit(());
 
+    // Get necessary prompts
+    let (continue_prompt, default_prompt) =
+        r_lock! { (r_get_option::<String>("continue"), r_get_option::<String>("prompt")) };
+
     // 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();
@@ -495,7 +500,6 @@ fn complete_execute_request(req: &Request, prompt_recv: &Receiver<String>) {
     }
 
     // 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();

crates/ark/src/kernel.rs

@@ -229,17 +229,19 @@ impl Kernel {
         let mut data = serde_json::Map::new();
         data.insert("text/plain".to_string(), json!(""));
 
-        // Include HTML representation of data.frame
-        // TODO: Do we need to hold the R lock here?
-        let value = unsafe { Rf_findVarInFrame(R_GlobalEnv, r_symbol!(".Last.value")) };
-        if r_is_data_frame(value) {
-            match Kernel::to_html(value) {
-                Ok(html) => data.insert("text/html".to_string(), json!(html)),
-                Err(error) => {
-                    error!("{:?}", error);
-                    None
-                },
-            };
+        r_lock! {
+            let value = Rf_findVarInFrame(R_GlobalEnv, r_symbol!(".Last.value"));
+
+            // Include HTML representation of data.frame
+            if r_is_data_frame(value) {
+                match Kernel::to_html(value) {
+                    Ok(html) => data.insert("text/html".to_string(), json!(html)),
+                    Err(error) => {
+                        error!("{:?}", error);
+                        None
+                    },
+                };
+            }
         }
 
         if let Err(err) = self

crates/ark/src/lsp/backend.rs

@@ -436,12 +436,10 @@ impl LanguageServer for Backend {
             return Ok(item);
         });
 
-        unsafe {
-            unwrap!(resolve_completion_item(&mut item, &data), Err(error) => {
-                error!("{:?}", error);
-                return Ok(item);
-            });
-        }
+        unwrap!(r_lock! { resolve_completion_item(&mut item, &data) }, Err(error) => {
+            error!("{:?}", error);
+            return Ok(item);
+        });
 
         Ok(item)
     }
@@ -464,7 +462,9 @@ impl LanguageServer for Backend {
         });
 
         // request hover information
-        let result = unsafe { hover(&document, &context) };
+        let result = r_lock! {
+            hover(&document, &context)
+        };
 
         // unwrap errors
         let result = unwrap!(result, Err(error) => {
@@ -494,7 +494,7 @@ impl LanguageServer for Backend {
 
         // request signature help
         let position = params.text_document_position_params.position;
-        let result = unsafe { signature_help(document.value(), &position) };
+        let result = r_lock! { signature_help(document.value(), &position) };
 
         // unwrap errors
         let result = unwrap!(result, Err(error) => {

crates/ark/tests/environment.rs

@@ -59,10 +59,12 @@ fn test_environment_list() {
 
     // Create a new environment handler and give it a view of the test
     // environment we created.
-    let test_env_view = RObject::view(test_env.sexp);
     let incoming_tx = comm.incoming_tx.clone();
     let outgoing_rx = comm.outgoing_rx.clone();
-    REnvironment::start(test_env_view, comm);
+    r_lock! {
+        let test_env_view = RObject::view(test_env.sexp);
+        REnvironment::start(test_env_view, comm.clone())
+    }
 
     // Ensure we get a list of variables after initialization
     let msg = outgoing_rx.recv().unwrap();

crates/harp/harp-macros/src/lib.rs

@@ -149,6 +149,36 @@ pub fn vector(_attr: TokenStream, item: TokenStream) -> TokenStream {
     all.into()
 }
 
+#[proc_macro_attribute]
+pub fn lock(_attr: TokenStream, item: TokenStream) -> TokenStream {
+    // Get the pieces of the function being wrapped
+    let syn::ItemFn {
+        attrs,
+        vis,
+        sig,
+        block,
+    } = syn::parse(item).unwrap();
+
+    let stmts = &block.stmts;
+
+    // Generate error message
+    let message = format!("Can't use `{}()` without holding the R lock.", sig.ident);
+
+    let out = quote! {
+        #(#attrs)* #vis #sig {
+
+            #[cfg(debug_assertions)]
+            unsafe {
+                std::assert!(crate::lock::R_RUNTIME_LOCK.is_locked(), #message);
+            }
+
+            #(#stmts)*
+        }
+    };
+
+    out.into()
+}
+
 #[proc_macro_attribute]
 pub fn register(_attr: TokenStream, item: TokenStream) -> TokenStream {
     // Get metadata about the function being registered.

crates/harp/src/lib.rs

@@ -13,6 +13,7 @@ pub mod interrupts;
 pub mod lock;
 pub mod object;
 pub mod protect;
+pub mod r_api;
 pub mod routines;
 pub mod string;
 pub mod symbol;

crates/harp/src/protect.rs

@@ -5,7 +5,9 @@
 //
 //
 
-use libR_sys::*;
+use crate::r_api::Rf_protect;
+use crate::r_api::Rf_unprotect;
+use crate::r_api::SEXP;
 
 // NOTE: The RProtect struct uses R's stack-based object protection, and so is
 // only appropriate for R objects with 'automatic' lifetime. In general, this
@@ -16,21 +18,20 @@ pub struct RProtect {
 }
 
 impl RProtect {
-    /// SAFETY: Assumes that the R lock is held.
-    pub unsafe fn new() -> Self {
+    pub fn new() -> Self {
         Self { count: 0 }
     }
 
-    /// SAFETY: Assumes that the R lock is held.
-    pub unsafe fn add(&mut self, object: SEXP) -> SEXP {
+    /// SAFETY: Requires that the R lock is held.
+    pub fn add(&mut self, object: SEXP) -> SEXP {
         self.count += 1;
         return Rf_protect(object);
     }
 }
 
 impl Drop for RProtect {
-    /// SAFETY: Assumes that the R lock is held.
+    /// SAFETY: Requires that the R lock is held.
     fn drop(&mut self) {
-        unsafe { Rf_unprotect(self.count) }
+        Rf_unprotect(self.count)
     }
 }

crates/harp/src/r_api.rs

@@ -0,0 +1,30 @@
+//
+// r_api.rs
+//
+// Copyright (C) 2023 Posit Software, PBC. All rights reserved.
+//
+//
+
+#![allow(non_snake_case)]
+
+pub use libR_sys::SEXP;
+
+#[harp_macros::lock]
+pub fn Rf_xlength(x: SEXP) -> isize {
+    unsafe { libR_sys::Rf_xlength(x) }
+}
+
+#[harp_macros::lock]
+pub fn TYPEOF(x: SEXP) -> std::os::raw::c_int {
+    unsafe { libR_sys::TYPEOF(x) }
+}
+
+#[harp_macros::lock]
+pub fn Rf_protect(x: SEXP) -> SEXP {
+    unsafe { libR_sys::Rf_protect(x) }
+}
+
+#[harp_macros::lock]
+pub fn Rf_unprotect(count: i32) {
+    unsafe { libR_sys::Rf_unprotect(count) }
+}

crates/harp/src/utils.rs

@@ -23,6 +23,9 @@ use crate::exec::RFunction;
 use crate::exec::RFunctionExt;
 use crate::object::RObject;
 use crate::protect::RProtect;
+use crate::r_api::Rf_xlength;
+use crate::r_api::SEXP;
+use crate::r_api::TYPEOF;
 use crate::r_symbol;
 use crate::symbol::RSymbol;
 use crate::vector::CharacterVector;
@@ -87,6 +90,14 @@ impl Sxpinfo {
     }
 }
 
+pub fn r_length(x: SEXP) -> isize {
+    Rf_xlength(x)
+}
+
+pub fn r_typeof(x: SEXP) -> u32 {
+    TYPEOF(x) as u32
+}
+
 pub fn r_assert_type(object: SEXP, expected: &[u32]) -> Result<u32> {
     let actual = r_typeof(object);
 
@@ -107,7 +118,7 @@ pub unsafe fn r_assert_capacity(object: SEXP, required: usize) -> Result<usize>
 }
 
 pub fn r_assert_length(object: SEXP, expected: usize) -> Result<usize> {
-    let actual = unsafe { Rf_xlength(object) as usize };
+    let actual = r_length(object) as usize;
     if actual != expected {
         return Err(Error::UnexpectedLength(actual, expected));
     }
@@ -207,7 +218,7 @@ pub fn r_vec_shape(value: SEXP) -> String {
         let dim = RObject::new(Rf_getAttrib(value, R_DimSymbol));
 
         if r_is_null(*dim) {
-            format!("{}", Rf_xlength(value))
+            format!("{}", r_length(value))
         } else {
             let dim = IntegerVector::new_unchecked(*dim);
             dim.iter().map(|d| d.unwrap()).join(", ")
@@ -224,13 +235,6 @@ pub fn r_altrep_class(object: SEXP) -> String {
     format!("{}::{}", pkg, klass)
 }
 
-pub fn r_typeof(object: SEXP) -> u32 {
-    // SAFETY: The type of an R object is typically considered constant,
-    // and TYPEOF merely queries the R type directly from the SEXPREC struct.
-    let object = object.into();
-    unsafe { TYPEOF(object) as u32 }
-}
-
 pub unsafe fn r_type2char<T: Into<u32>>(kind: T) -> String {
     let kind = Rf_type2char(kind.into());
     let cstr = CStr::from_ptr(kind);

crates/harp/src/vector/mod.rs

@@ -10,6 +10,7 @@ use libR_sys::*;
 use crate::error::Result;
 use crate::utils::r_assert_capacity;
 use crate::utils::r_assert_type;
+use crate::utils::r_length;
 
 pub mod character_vector;
 pub use character_vector::CharacterVector;
@@ -87,7 +88,7 @@ pub trait Vector {
         <T as IntoIterator>::Item: AsRef<Self::Item>;
 
     unsafe fn len(&self) -> usize {
-        Rf_xlength(self.data()) as usize
+        r_length(self.data()) as usize
     }
 
     fn format_one(&self, x: Self::Type) -> String;