Safer passing of R functions to JS (#41)

* Safer passing of R functions to JS * Tidy * Bump version * Fix initialiser
andrjohns · May 31, 2024 · e395871 · e395871
1 parent 78c6c27
commit e395871
Show file tree

Hide file tree

Showing 6 changed files with 30 additions and 21 deletions.
diff --git a/DESCRIPTION b/DESCRIPTION
@@ -1,6 +1,6 @@
 Package: QuickJSR
 Title: Interface for the 'QuickJS' Lightweight 'JavaScript' Engine
-Version: 1.2.0
+Version: 1.2.0.9000
 Authors@R: c(
     person(c("Andrew", "R."), "Johnson", , "andrew.johnson@arjohnsonau.com", role = c("aut", "cre"),
            comment = c(ORCID = "0000-0001-7000-8065")),

diff --git a/NEWS.md b/NEWS.md
@@ -1,7 +1,7 @@
 # QuickJSR 1.2.0
   - `Rcpp` dependency replaced with vendored `cpp11` headers
   - `R6` dependency removed
-  - `R` and `JS` interopability added, removing `jsonlite` dependency
+  - `R` and `JS` interoperability added, removing `jsonlite` dependency
   - Fixes for libatomic linking on 32-bit systems
   - Added `to_json` and `from_json` functions for testing `R`/`JS` interop
 

diff --git a/inst/include/quickjsr.hpp b/inst/include/quickjsr.hpp
@@ -1,6 +1,7 @@
 #ifndef QUICKJSR_HPP
 #define QUICKJSR_HPP
 
+#include <quickjsr/JS_SEXP.hpp>
 #include <quickjsr/SEXP_to_JSValue.hpp>
 #include <quickjsr/JSValue_to_SEXP.hpp>
 #include <quickjsr/JSValue_to_JSON.hpp>

diff --git a/inst/include/quickjsr/JS_SEXP.hpp b/inst/include/quickjsr/JS_SEXP.hpp
@@ -0,0 +1,14 @@
+#ifndef QUICKJSR_JS_SEXP_HPP
+#define QUICKJSR_JS_SEXP_HPP
+
+#include <quickjs-libc.h>
+
+namespace quickjsr {
+  JSClassID js_sexp_class_id;
+  JSClassDef js_sexp_class_def = {
+    "SEXP",
+    nullptr // finalized
+  };
+}
+
+#endif
diff --git a/inst/include/quickjsr/SEXP_to_JSValue.hpp b/inst/include/quickjsr/SEXP_to_JSValue.hpp
@@ -3,14 +3,11 @@
 
 #include <quickjsr/JSValue_Date.hpp>
 #include <quickjsr/JSValue_to_SEXP.hpp>
+#include <quickjsr/JS_SEXP.hpp>
 #include <cpp11.hpp>
 #include <quickjs-libc.h>
 
 namespace quickjsr {
-  // Global tape to store JSValue objects that are created during conversion but
-  // but can't be immediately freed because they are needed
-  std::vector<JSValue> global_tape;
-
   // Forward declaration to allow for recursive calls
   inline JSValue SEXP_to_JSValue(JSContext* ctx, const SEXP& x, bool auto_unbox, bool auto_unbox_curr);
   inline JSValue SEXP_to_JSValue(JSContext* ctx, const SEXP& x, bool auto_unbox, bool auto_unbox_curr, int index);
@@ -83,9 +80,9 @@ namespace quickjsr {
 
   static JSValue js_fun_static(JSContext* ctx, JSValueConst this_val, int argc,
                                 JSValueConst* argv, int magic, JSValue* data) {
-    int64_t ptr;
-    JS_ToBigInt64(ctx, &ptr, *data);
-    SEXP x = reinterpret_cast<SEXP>(ptr);
+    JSValue data_val = data[0];
+    SEXP x = reinterpret_cast<SEXP>(JS_GetOpaque(data_val, js_sexp_class_id));
+    JS_FreeValue(ctx, data_val);
     cpp11::writable::list args(argc);
     for (int i = 0; i < argc; i++) {
       args[i] = JSValue_to_SEXP(ctx, argv[i]);
@@ -97,11 +94,10 @@ namespace quickjsr {
   inline JSValue SEXP_to_JSValue_function(JSContext* ctx, const SEXP& x,
                                           bool auto_unbox_inp = false,
                                           bool auto_unbox = false) {
-    // Store the SEXP pointer as a 64-bit integer so that it can be
-    // passed to the JS C function
-    global_tape.push_back(JS_NewBigInt64(ctx, reinterpret_cast<int64_t>(x)));
+    JSValue obj = JS_NewObjectClass(ctx, js_sexp_class_id);
+    JS_SetOpaque(obj, reinterpret_cast<void*>(x));
     return JS_NewCFunctionData(ctx, js_fun_static, Rf_length(FORMALS(x)),
-                                JS_CFUNC_generic, 1, &global_tape[global_tape.size() - 1]);
+                                JS_CFUNC_generic, 1, &obj);
   }
 
   inline JSValue SEXP_to_JSValue_matrix(JSContext* ctx, const SEXP& x, bool auto_unbox_inp = false, bool auto_unbox = false) {

diff --git a/src/quickjsr.cpp b/src/quickjsr.cpp
@@ -3,20 +3,13 @@
 #include <quickjs-libc.h>
 #include <quickjsr.hpp>
 
-void JS_FreeJSContextandTape(JSContext* ctx) {
-  for (auto&& val : quickjsr::global_tape) {
-    JS_FreeValue(ctx, val);
-  }
-  JS_FreeContext(ctx);
-}
-
 void JS_FreeRuntimeStdHandlers(JSRuntime* rt) {
   js_std_free_handlers(rt);
   JS_FreeRuntime(rt);
 }
 
 // Register the cpp11 external pointer types with the correct cleanup/finaliser functions
-using ContextXPtr = cpp11::external_pointer<JSContext, JS_FreeJSContextandTape>;
+using ContextXPtr = cpp11::external_pointer<JSContext, JS_FreeContext>;
 using RuntimeXPtr = cpp11::external_pointer<JSRuntime, JS_FreeRuntimeStdHandlers>;
 
 extern "C" SEXP qjs_context_(SEXP stack_size_) {
@@ -29,6 +22,11 @@ extern "C" SEXP qjs_context_(SEXP stack_size_) {
     JS_SetMaxStackSize(rt.get(), 0);
   }
   js_std_init_handlers(rt.get());
+
+  // Initialise a class which can be used for passing SEXP objects to JS
+  // without needing conversion
+  JS_NewClass(rt.get(), quickjsr::js_sexp_class_id, &quickjsr::js_sexp_class_def);
+
   ContextXPtr ctx(JS_NewContext(rt.get()));
   js_std_add_helpers(ctx.get(), 0, (char**)"");