Restore Send bound on task closures

To avoid sending objects with implementations sensitive to thread
state (id, thread-local storage, etc)

Requires dev tree-sitter which adds `Send` and `Sync` bounds to a
bunch of types.

Author: lionel-

Cargo.lock

@@ -1,5 +1,10 @@
 [workspace]
 
+# Was necessary after switching to dev tree-sitter to fix this warning:
+# > some crates are on edition 2021 which defaults to `resolver = "2"`, but
+# > virtual workspaces default to `resolver = "1"`
+resolver = "2"
+
 members = [
     "crates/amalthea",
     "crates/ark",

Cargo.toml

@@ -33,7 +33,7 @@ nix = { version = "0.26.2", features = ["signal"] }
 notify = "6.0.0"
 once_cell = "1.17.1"
 parking_lot = "0.12.1"
-regex = "1.7.1"
+regex = "1.10.0"
 reqwest = { version = "0.11.20", features = ["json"] }
 ropey = "1.6.0"
 rust-embed = "8.0.0"
@@ -43,8 +43,8 @@ serde_json = "1.0.94"
 stdext = { path = "../stdext" }
 tokio = { version = "1.26.0", features = ["full"] }
 tower-lsp = "0.19.0"
-tree-sitter = "0.20.9"
-tree-sitter-r = { git = "https://github.com/r-lib/tree-sitter-r", branch = "next" }
+tree-sitter = { git = "https://github.com/tree-sitter/tree-sitter" }
+tree-sitter-r = { git = "https://github.com/r-lib/tree-sitter-r", branch = "bugfix/update" }
 uuid = "1.3.0"
 url = "2.4.1"
 walkdir = "2"

crates/ark/Cargo.toml

@@ -26,8 +26,8 @@ type SharedOption<T> = Arc<Mutex<Option<T>>>;
 pub fn r_task<'env, F, T>(f: F) -> T
 where
     F: FnOnce() -> T,
-    F: 'env,
-    T: 'env,
+    F: 'env + Send,
+    T: 'env + Send,
 {
     // Escape hatch for unit tests
     if unsafe { R_TASK_BYPASS } {
@@ -62,7 +62,7 @@ where
         };
 
         // Move `f` to heap and erase its lifetime
-        let closure: Box<dyn FnOnce() + 'env> = Box::new(closure);
+        let closure: Box<dyn FnOnce() + 'env + Send> = Box::new(closure);
         let closure: Box<dyn FnOnce() + Send + 'static> = unsafe { std::mem::transmute(closure) };
 
         // Channel to communicate completion status of the task/closure

crates/ark/src/r_task.rs

@@ -69,7 +69,7 @@ fn test_environment_list() {
     let incoming_tx = comm.incoming_tx.clone();
     let outgoing_rx = comm.outgoing_rx.clone();
     r_task(|| {
-        let test_env_view = RObject::view(test_env.sexp);
+        let test_env_view = RObject::view(*test_env);
         REnvironment::start(test_env_view, comm.clone(), comm_manager_tx.clone());
     });
 
@@ -90,7 +90,7 @@ fn test_environment_list() {
     // variables with the new variable in it.
     r_task(|| unsafe {
         let sym = r_symbol!("everything");
-        Rf_defineVar(sym, Rf_ScalarInteger(42), test_env.sexp);
+        Rf_defineVar(sym, Rf_ScalarInteger(42), *test_env);
     });
 
     // Request that the environment be refreshed
@@ -122,8 +122,8 @@ fn test_environment_list() {
 
     // Create another variable
     r_task(|| unsafe {
-        r_envir_set("nothing", Rf_ScalarInteger(43), test_env.sexp);
-        r_envir_remove("everything", test_env.sexp);
+        r_envir_set("nothing", Rf_ScalarInteger(43), *test_env);
+        r_envir_remove("everything", *test_env);
     });
 
     // Simulate a prompt signal
@@ -173,17 +173,17 @@ fn test_environment_list() {
 
     // test the env is now empty
     r_task(|| unsafe {
-        let contents = RObject::new(R_lsInternal(test_env.sexp, Rboolean_TRUE));
+        let contents = RObject::new(R_lsInternal(*test_env, Rboolean_TRUE));
         assert_eq!(Rf_length(*contents), 0);
     });
 
     // Create some more variables
     r_task(|| unsafe {
         let sym = r_symbol!("a");
-        Rf_defineVar(sym, Rf_ScalarInteger(42), test_env.sexp);
+        Rf_defineVar(sym, Rf_ScalarInteger(42), *test_env);
 
         let sym = r_symbol!("b");
-        Rf_defineVar(sym, Rf_ScalarInteger(43), test_env.sexp);
+        Rf_defineVar(sym, Rf_ScalarInteger(43), *test_env);
     });
 
     // Simulate a prompt signal

crates/ark/tests/environment.rs

@@ -104,9 +104,11 @@ pub struct Binding {
 }
 
 // For sending to main thread as R task
-unsafe impl Send for Binding {}
 unsafe impl Sync for Binding {}
 
+// FIXME: This should only be Sync
+unsafe impl Send for Binding {}
+
 impl Binding {
     pub fn new(env: SEXP, frame: SEXP) -> Self {
         unsafe {

crates/harp/src/environment.rs

@@ -214,12 +214,14 @@ impl Drop for RObject {
     }
 }
 
-// RObjects are not inherently thread-safe since they wrap a raw pointer, but we
-// allow them to be sent across threads because we require the acquisition of a
-// lock on the outer R interpreter (see `r_task()`) before using them.
-unsafe impl Send for RObject {}
+// References to RObjects are safe to send over threads but the actual
+// object should remain on the R thread at all times. We can only `drop()`
+// them on the R thread since that calls the R API.
 unsafe impl Sync for RObject {}
 
+// FIXME: This should only be Sync
+unsafe impl Send for RObject {}
+
 impl Deref for RObject {
     type Target = SEXP;
     fn deref(&self) -> &Self::Target {