Improve error handling for toStrict

when calling `toStrict`, there was no try/catch for propagating the error back to Rust correctly. However, `call_js_function` didn't have the ability to correctly pass in the current `this` object to the function, so I made another function to handle it. In the future we should do a full cleanup of helper functions in general.
urbas · May 14, 2024 · 2bf3fd5 · 2bf3fd5
1 parent a74cdc0
commit 2bf3fd5
Show file tree

Hide file tree

Showing 3 changed files with 50 additions and 26 deletions.
diff --git a/src/eval/execution.rs b/src/eval/execution.rs
@@ -192,7 +192,7 @@ fn nix_value_from_module(
             .expect("`n.recursiveStrict` is not a function.");
     let strict_nix_value = call_js_function(scope, &to_strict_fn, nixjs_rt_obj, &[nix_value])?;
 
-    js_value_to_nix(scope, &nixrt, &strict_nix_value)
+    js_value_to_nix(scope, &nixjs_rt_obj, &strict_nix_value)
 }
 
 fn create_eval_ctx<'s>(

diff --git a/src/eval/helpers.rs b/src/eval/helpers.rs
@@ -72,15 +72,38 @@ pub fn call_js_function<'s>(
     args: &[v8::Local<v8::Value>],
 ) -> Result<v8::Local<'s, v8::Value>, NixError> {
     let try_scope = &mut v8::TryCatch::new(scope);
-    let recv = v8::undefined(try_scope).into();
-    let Some(strict_nix_value) = js_function.call(try_scope, recv, args) else {
-        // TODO: Again, the stack trace needs to be source-mapped.
-        if let Some(error) = try_scope.exception() {
-            let error = js_error_to_rust(try_scope, nixrt, error);
-            return Err(error);
-        } else {
-            return Err("Unknown evaluation error.".into());
-        }
+    let this = v8::undefined(try_scope).into();
+    let Some(strict_nix_value) = js_function.call(try_scope, this, args) else {
+        let exception = try_scope.exception();
+        return Err(map_js_exception_value_to_rust(try_scope, nixrt, exception));
     };
     Ok(strict_nix_value)
 }
+
+pub fn call_applied_js_function<'s>(
+    scope: &mut v8::HandleScope<'s>,
+    js_function: &v8::Local<v8::Function>,
+    this: v8::Local<v8::Value>,
+    nixrt: v8::Local<v8::Object>,
+    args: &[v8::Local<v8::Value>],
+) -> Result<v8::Local<'s, v8::Value>, NixError> {
+    let try_scope = &mut v8::TryCatch::new(scope);
+    let Some(strict_nix_value) = js_function.call(try_scope, this, args) else {
+        let exception = try_scope.exception();
+        return Err(map_js_exception_value_to_rust(try_scope, nixrt, exception));
+    };
+    Ok(strict_nix_value)
+}
+
+pub fn map_js_exception_value_to_rust<'s>(
+    scope: &mut v8::HandleScope<'s>,
+    nixrt: v8::Local<v8::Object>,
+    exception: Option<v8::Local<'s, v8::Value>>,
+) -> NixError {
+    // TODO: Again, the stack trace needs to be source-mapped.
+    if let Some(error) = exception {
+        js_error_to_rust(scope, nixrt, error)
+    } else {
+        "Unknown evaluation error.".into()
+    }
+}
diff --git a/src/eval/types.rs b/src/eval/types.rs
@@ -2,7 +2,7 @@ use std::collections::HashMap;
 
 use super::{
     error::NixError,
-    helpers::{is_nixrt_type, try_get_js_object_key},
+    helpers::{call_applied_js_function, is_nixrt_type, try_get_js_object_key},
 };
 
 #[derive(Debug, PartialEq)]
@@ -34,7 +34,7 @@ pub type EvalResult = Result<Value, NixError>;
 
 pub fn js_value_to_nix(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> EvalResult {
     if js_value.is_function() {
@@ -75,7 +75,7 @@ pub fn js_value_to_nix(
 
 fn from_js_int(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> Result<Option<Value>, NixError> {
     if is_nixrt_type(scope, nixrt, js_value, "NixInt")? {
@@ -92,7 +92,7 @@ fn from_js_int(
 
 fn from_js_string(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> Result<Option<Value>, NixError> {
     if is_nixrt_type(scope, nixrt, js_value, "NixString")? {
@@ -111,7 +111,7 @@ fn from_js_string(
 
 fn from_js_lazy(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> Result<Option<Value>, NixError> {
     if is_nixrt_type(scope, nixrt, js_value, "Lazy")? {
@@ -123,17 +123,18 @@ fn from_js_lazy(
                 "Expected `toStrict` to be a method on the Lazy object. Internal conversion error: {err:?}"
             )
         })?;
-        let strict_value = to_strict_method
-            .call(scope, *js_value, &[])
-            .ok_or_else(|| "Could not convert the lazy value to strict.".to_string())?;
+
+        let strict_value =
+            call_applied_js_function(scope, &to_strict_method, *js_value, *nixrt, &[])?;
+
         return Ok(Some(js_value_to_nix(scope, nixrt, &strict_value)?));
     }
     Ok(None)
 }
 
 fn from_js_bool(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> Result<Option<Value>, NixError> {
     if is_nixrt_type(scope, nixrt, js_value, "NixBool")? {
@@ -152,7 +153,7 @@ fn from_js_bool(
 
 fn from_js_float(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> Result<Option<Value>, NixError> {
     if is_nixrt_type(scope, nixrt, js_value, "NixFloat")? {
@@ -176,7 +177,7 @@ fn from_js_float(
 
 fn from_js_attrset(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> Result<Option<Value>, NixError> {
     if is_nixrt_type(scope, nixrt, js_value, "Attrset")? {
@@ -206,7 +207,7 @@ fn from_js_attrset(
 
 fn from_js_list(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> Result<Option<Value>, NixError> {
     if is_nixrt_type(scope, nixrt, js_value, "NixList")? {
@@ -226,7 +227,7 @@ fn from_js_list(
 
 fn from_js_path(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> Result<Option<Value>, NixError> {
     if !is_nixrt_type(scope, nixrt, js_value, "Path")? {
@@ -240,7 +241,7 @@ fn from_js_path(
 
 fn from_js_lambda(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_value: &v8::Local<v8::Value>,
 ) -> Result<Option<Value>, NixError> {
     if !is_nixrt_type(scope, nixrt, js_value, "Lambda")? {
@@ -251,7 +252,7 @@ fn from_js_lambda(
 
 fn js_value_as_nix_array(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_array: &v8::Local<v8::Array>,
 ) -> EvalResult {
     let length = js_array.length();
@@ -268,7 +269,7 @@ fn js_value_as_nix_array(
 
 fn js_map_as_attrset(
     scope: &mut v8::HandleScope<'_>,
-    nixrt: &v8::Local<v8::Value>,
+    nixrt: &v8::Local<v8::Object>,
     js_map: &v8::Local<v8::Map>,
 ) -> EvalResult {
     let mut map: HashMap<String, Value> = HashMap::new();