Relax Send requirement on r_task() closure

The control flow of a task is strictly delineated in such a way that the task is necessarily
completed before control is returned to the caller. So it is safe to erase the `Send` requirement on
the objects captured by the closure.

Author: lionel-

crates/ark/src/lsp/backend.rs

@@ -13,7 +13,6 @@ use std::sync::Arc;
 
 use crossbeam::channel::Sender;
 use dashmap::DashMap;
-use harp::r_lock;
 use log::*;
 use parking_lot::Mutex;
 use regex::Regex;
@@ -373,17 +372,8 @@ impl LanguageServer for Backend {
 
         log::info!("Completion context: {:#?}", context);
 
-        // FIXME: Use `r_task()`
-        // rustc [E0277]: `*const c_void` cannot be shared between threads safely
-        // within `completions::CompletionContext<'_>`, the trait `Sync` is not implemented for `*const c_void`
-        // required for `&completions::CompletionContext<'_>` to implement `Send`
-        // rustc [E0277]: `*const tree_sitter::ffi::TSTree` cannot be shared between threads safely
-        // within `completions::CompletionContext<'_>`, the trait `Sync` is not implemented for `*const tree_sitter::ffi::TSTree`
-        // required for `&completions::CompletionContext<'_>` to implement `Send`
-        // rustc [E0277]: required because it's used within this closure
-
         // Add session completions
-        let result = r_lock! { append_session_completions(&context, &mut completions) };
+        let result = r_task(|| unsafe { append_session_completions(&context, &mut completions) });
         if let Err(error) = result {
             error!("{:?}", error);
         }
@@ -482,20 +472,8 @@ impl LanguageServer for Backend {
             return Ok(None);
         });
 
-        // FIXME: Use `r_task()`
-        // rustc [E0277]: required by a bound introduced by this call
-        // rustc [E0277]: `*const tree_sitter::ffi::TSTree` cannot be shared between threads safely
-        // within `completions::CompletionContext<'_>`, the trait `Sync` is not implemented for `*const tree_sitter::ffi::TSTree`
-        // required for `&completions::CompletionContext<'_>` to implement `Send`
-        // rustc [E0277]: `*const c_void` cannot be shared between threads safely
-        // within `completions::CompletionContext<'_>`, the trait `Sync` is not implemented for `*const c_void`
-        // required for `&completions::CompletionContext<'_>` to implement `Send`
-        // rustc [E0277]: required because it's used within this closure
-
         // request hover information
-        let result = r_lock! {
-            hover(&document, &context)
-        };
+        let result = r_task(|| unsafe { hover(&document, &context) });
 
         // unwrap errors
         let result = unwrap!(result, Err(error) => {

crates/ark/src/lsp/diagnostics.rs

@@ -17,11 +17,13 @@ use harp::external_ptr::ExternalPointer;
 use harp::object::RObject;
 use harp::protect::RProtect;
 use harp::r_symbol;
+use harp::utils::r_is_null;
 use harp::utils::r_symbol_quote_invalid;
 use harp::utils::r_symbol_valid;
 use harp::vector::CharacterVector;
 use harp::vector::Vector;
 use libR_sys::*;
+use libc::c_void;
 use stdext::*;
 use tower_lsp::lsp_types::Diagnostic;
 use tower_lsp::lsp_types::DiagnosticSeverity;
@@ -635,78 +637,72 @@ impl<'a> TreeSitterCall<'a> {
     }
 }
 
-// FIXME: Can't directly switch to `r_task()`
-// rustc [E0277]: `*const tree_sitter::ffi::TSTree` cannot be shared between threads safely
-// within `tree_sitter::Node<'_>`, the trait `Sync` is not implemented for `*const tree_sitter::ffi::TSTree`
-// required for `&tree_sitter::Node<'_>` to implement `Send`
-// rustc [E0277]: `*const c_void` cannot be shared between threads safely
-
-// fn recurse_call_arguments_custom(
-//     node: Node,
-//     context: &mut DiagnosticContext,
-//     diagnostics: &mut Vec<Diagnostic>,
-//     function: &str,
-//     diagnostic_function: &str,
-// ) -> Result<()> {
-//     r_lock! {
-//         // Build a call that mixes treesitter nodes (as external pointers)
-//         // library(foo, pos = 2 + 2)
-//         //    ->
-//         // library([0, <node0>], pos = [1, <node1>])
-//         // where:
-//         //   - node0 is an external pointer to a treesitter Node for the identifier `foo`
-//         //   - node1 is an external pointer to a treesitter Node for the call `2 + 2`
-//         //
-//         // The TreeSitterCall object holds on to the nodes, so that they can be
-//         // safely passed down to the R side as external pointers
-//         let call = TreeSitterCall::new(node, function, context)?;
-
-//         let custom_diagnostics = RFunction::from(diagnostic_function)
-//             .add(&call)
-//             .add(ExternalPointer::new(&context.source))
-//             .call()?;
-
-//         if !r_is_null(*custom_diagnostics) {
-//             let n = XLENGTH(*custom_diagnostics);
-//             for i in 0..n {
-//                 // diag is a list with:
-//                 //   - The kind of diagnostic: skip, default, simple
-//                 //   - The node external pointer, i.e. the ones made in TreeSitterCall::new
-//                 //   - The message, when kind is "simple"
-//                 let diag = VECTOR_ELT(*custom_diagnostics, i);
-
-//                 let kind: String = RObject::view(VECTOR_ELT(diag, 0)).try_into()?;
-
-//                 if kind == "skip" {
-//                     // skip the diagnostic entirely, e.g.
-//                     // library(foo)
-//                     //         ^^^
-//                     continue;
-//                 }
-
-//                 let ptr = VECTOR_ELT(diag, 1);
-//                 let value = *(R_ExternalPtrAddr(ptr) as *const c_void as *const Node);
-
-//                 if kind == "default" {
-//                     // the R side gives up, so proceed as normal, e.g.
-//                     // library(foo, pos = ...)
-//                     //                    ^^^
-//                     recurse(value, context, diagnostics)?;
-//                 } else if kind == "simple" {
-//                     // Simple diagnostic from R, e.g.
-//                     // library("ggplot3")
-//                     //          ^^^^^^^   Package 'ggplot3' is not installed
-//                     let message: String = RObject::view(VECTOR_ELT(diag, 2)).try_into()?;
-//                     let range: Range = value.range().into();
-//                     let diagnostic = Diagnostic::new_simple(range.into(), message.into());
-//                     diagnostics.push(diagnostic);
-//                 }
-//             }
-//         }
-
-//         ().ok()
-//     }
-// }
+fn recurse_call_arguments_custom(
+    node: Node,
+    context: &mut DiagnosticContext,
+    diagnostics: &mut Vec<Diagnostic>,
+    function: &str,
+    diagnostic_function: &str,
+) -> Result<()> {
+    r_task(|| unsafe {
+        // Build a call that mixes treesitter nodes (as external pointers)
+        // library(foo, pos = 2 + 2)
+        //    ->
+        // library([0, <node0>], pos = [1, <node1>])
+        // where:
+        //   - node0 is an external pointer to a treesitter Node for the identifier `foo`
+        //   - node1 is an external pointer to a treesitter Node for the call `2 + 2`
+        //
+        // The TreeSitterCall object holds on to the nodes, so that they can be
+        // safely passed down to the R side as external pointers
+        let call = TreeSitterCall::new(node, function, context)?;
+
+        let custom_diagnostics = RFunction::from(diagnostic_function)
+            .add(&call)
+            .add(ExternalPointer::new(&context.source))
+            .call()?;
+
+        if !r_is_null(*custom_diagnostics) {
+            let n = XLENGTH(*custom_diagnostics);
+            for i in 0..n {
+                // diag is a list with:
+                //   - The kind of diagnostic: skip, default, simple
+                //   - The node external pointer, i.e. the ones made in TreeSitterCall::new
+                //   - The message, when kind is "simple"
+                let diag = VECTOR_ELT(*custom_diagnostics, i);
+
+                let kind: String = RObject::view(VECTOR_ELT(diag, 0)).try_into()?;
+
+                if kind == "skip" {
+                    // skip the diagnostic entirely, e.g.
+                    // library(foo)
+                    //         ^^^
+                    continue;
+                }
+
+                let ptr = VECTOR_ELT(diag, 1);
+                let value = *(R_ExternalPtrAddr(ptr) as *const c_void as *const Node);
+
+                if kind == "default" {
+                    // the R side gives up, so proceed as normal, e.g.
+                    // library(foo, pos = ...)
+                    //                    ^^^
+                    recurse(value, context, diagnostics)?;
+                } else if kind == "simple" {
+                    // Simple diagnostic from R, e.g.
+                    // library("ggplot3")
+                    //          ^^^^^^^   Package 'ggplot3' is not installed
+                    let message: String = RObject::view(VECTOR_ELT(diag, 2)).try_into()?;
+                    let range: Range = value.range().into();
+                    let diagnostic = Diagnostic::new_simple(range.into(), message.into());
+                    diagnostics.push(diagnostic);
+                }
+            }
+        }
+
+        ().ok()
+    })
+}
 
 fn recurse_call(
     node: Node,
@@ -726,30 +722,25 @@ fn recurse_call(
     // things like 'local({ ... })'.
     let fun = callee.utf8_text(context.source.as_bytes())?;
     match fun {
-        // FIXME: These are currently disabled because they use the R
-        // runtime to match arguments with an approach that meshes R and
-        // treesitter objects. This is not compatible with `r_task()`
-        // because treesitter objects are not Send/Sync.
-
-        // // TODO: there should be some sort of registration mechanism so
-        // //       that functions can declare that they know how to generate
-        // //       diagnostics, i.e. ggplot2::aes() would skip diagnostics
-
-        // // special case to deal with library() and require() nse
-        // "library" => recurse_call_arguments_custom(
-        //     node,
-        //     context,
-        //     diagnostics,
-        //     "library",
-        //     ".ps.diagnostics.custom.library",
-        // )?,
-        // "require" => recurse_call_arguments_custom(
-        //     node,
-        //     context,
-        //     diagnostics,
-        //     "require",
-        //     ".ps.diagnostics.custom.require",
-        // )?,
+        // TODO: there should be some sort of registration mechanism so
+        //       that functions can declare that they know how to generate
+        //       diagnostics, i.e. ggplot2::aes() would skip diagnostics
+
+        // special case to deal with library() and require() nse
+        "library" => recurse_call_arguments_custom(
+            node,
+            context,
+            diagnostics,
+            "library",
+            ".ps.diagnostics.custom.library",
+        )?,
+        "require" => recurse_call_arguments_custom(
+            node,
+            context,
+            diagnostics,
+            "require",
+            ".ps.diagnostics.custom.require",
+        )?,
 
         // default case: recurse into each argument
         _ => recurse_call_arguments_default(node, context, diagnostics)?,

crates/ark/src/r_task.rs

@@ -25,8 +25,8 @@ type SharedOption<T> = Arc<Mutex<Option<T>>>;
 pub fn r_task<'env, F, T>(f: F) -> T
 where
     F: FnOnce() -> T,
-    F: 'env + Send,
-    T: 'env + Send,
+    F: 'env,
+    T: 'env,
 {
     // 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() + Send + 'env> = Box::new(closure);
+        let closure: Box<dyn FnOnce() + 'env> = Box::new(closure);
         let closure: Box<dyn FnOnce() + Send + 'static> = unsafe { std::mem::transmute(closure) };
 
         // Channel to communicate completion status of the task/closure