Add optional types to JS exceptions (#206)

* Add types to JS exceptions * Remove thiserror dependency * Update crates/quickjs-wasm-rs/src/js_binding/error.rs Co-authored-by: Surma <surma@surma.dev> * Update crates/core/src/globals.rs Co-authored-by: Surma <surma@surma.dev> * Fix CI failures Co-authored-by: Surma <surma@surma.dev>
bytecodealliance · Jan 23, 2023 · 066dbb0 · 066dbb0
1 parent f789d96
commit 066dbb0
Show file tree

Hide file tree

Showing 7 changed files with 115 additions and 17 deletions.
diff --git a/crates/cli/tests/integration_test.rs b/crates/cli/tests/integration_test.rs
@@ -45,7 +45,7 @@ fn test_encoding() {
     assert_eq!("true".as_bytes(), output);
 
     let (output, _) = run(&mut runner, "invalid_fatal".as_bytes());
-    assert_eq!("The encoded data was not valid".as_bytes(), output);
+    assert_eq!("The encoded data was not valid utf-8".as_bytes(), output);
 
     let (output, _) = run(&mut runner, "test".as_bytes());
     assert_eq!("test2".as_bytes(), output);

diff --git a/crates/core/src/globals.rs b/crates/core/src/globals.rs
@@ -1,5 +1,5 @@
 use anyhow::anyhow;
-use quickjs_wasm_rs::{Context, Value};
+use quickjs_wasm_rs::{Context, JSError, Value};
 use std::borrow::Cow;
 use std::io::{Read, Write};
 use std::str;
@@ -124,11 +124,14 @@ fn decode_utf8_buffer_to_js_string(
             };
         }
 
-        let str = if fatal {
-            Cow::from(str::from_utf8(view).map_err(|_| anyhow!("The encoded data was not valid"))?)
-        } else {
-            String::from_utf8_lossy(view)
-        };
+        let str =
+            if fatal {
+                Cow::from(str::from_utf8(view).map_err(|_| {
+                    JSError::Type("The encoded data was not valid utf-8".to_string())
+                })?)
+            } else {
+                String::from_utf8_lossy(view)
+            };
         ctx.value_from_str(&str)
     }
 }

diff --git a/crates/quickjs-wasm-rs/src/js_binding/context.rs b/crates/quickjs-wasm-rs/src/js_binding/context.rs
@@ -1,4 +1,5 @@
 use super::constants::{MAX_SAFE_INTEGER, MIN_SAFE_INTEGER};
+use super::error::JSError;
 use super::exception::Exception;
 use super::value::Value;
 use anyhow::Result;
@@ -9,7 +10,8 @@ use quickjs_wasm_sys::{
     JS_NewArray, JS_NewArrayBufferCopy, JS_NewBigInt64, JS_NewBool_Ext, JS_NewCFunctionData,
     JS_NewClass, JS_NewClassID, JS_NewContext, JS_NewFloat64_Ext, JS_NewInt32_Ext, JS_NewInt64_Ext,
     JS_NewObject, JS_NewObjectClass, JS_NewRuntime, JS_NewStringLen, JS_NewUint32_Ext,
-    JS_ReadObject, JS_SetOpaque, JS_ThrowInternalError, JS_WriteObject, JS_EVAL_FLAG_COMPILE_ONLY,
+    JS_ReadObject, JS_SetOpaque, JS_ThrowInternalError, JS_ThrowRangeError, JS_ThrowReferenceError,
+    JS_ThrowSyntaxError, JS_ThrowTypeError, JS_WriteObject, JS_EVAL_FLAG_COMPILE_ONLY,
     JS_EVAL_TYPE_GLOBAL, JS_READ_OBJ_BYTECODE, JS_WRITE_OBJ_BYTECODE,
 };
 use std::any::TypeId;
@@ -266,6 +268,8 @@ impl Context {
     ///
     /// Since the callback signature accepts parameters as high-level `Context` and `Value` objects, it can be
     /// implemented without using `unsafe` code, unlike [new_callback] which provides a low-level API.
+    /// Returning a [JSError] from the callback will cause a JavaScript error with the appropriate
+    /// type to be thrown.
     pub fn wrap_callback<F>(&self, mut f: F) -> Result<Value>
     where
         F: (FnMut(&Self, &Value, &[Value]) -> Result<Value>) + 'static,
@@ -279,11 +283,38 @@ impl Context {
         ) {
             Ok(value) => value.value,
             Err(error) => {
-                if let Ok(message) = CString::new(format!("{error:?}")) {
-                    let format = CString::new("%s").unwrap();
-                    unsafe { JS_ThrowInternalError(inner, format.as_ptr(), message.as_ptr()) }
-                } else {
-                    unsafe { ext_js_exception }
+                let format = CString::new("%s").unwrap();
+                match error.downcast::<JSError>() {
+                    Ok(js_error) => {
+                        let message = CString::new(js_error.to_string())
+                            .unwrap_or_else(|_| CString::new("Unknown error").unwrap());
+                        match js_error {
+                            JSError::Internal(_) => unsafe {
+                                JS_ThrowInternalError(inner, format.as_ptr(), message.as_ptr())
+                            },
+                            JSError::Syntax(_) => unsafe {
+                                JS_ThrowSyntaxError(inner, format.as_ptr(), message.as_ptr())
+                            },
+                            JSError::Type(_) => unsafe {
+                                JS_ThrowTypeError(inner, format.as_ptr(), message.as_ptr())
+                            },
+                            JSError::Reference(_) => unsafe {
+                                JS_ThrowReferenceError(inner, format.as_ptr(), message.as_ptr())
+                            },
+                            JSError::Range(_) => unsafe {
+                                JS_ThrowRangeError(inner, format.as_ptr(), message.as_ptr())
+                            },
+                        }
+                    }
+                    Err(e) => {
+                        if let Ok(message) = CString::new(format!("{e:?}")) {
+                            unsafe {
+                                JS_ThrowInternalError(inner, format.as_ptr(), message.as_ptr())
+                            }
+                        } else {
+                            unsafe { ext_js_exception }
+                        }
+                    }
                 }
             }
         };
@@ -338,6 +369,7 @@ where
 #[cfg(test)]
 mod tests {
     use super::Context;
+    use crate::JSError;
     use anyhow::Result;
     use quickjs_wasm_sys::ext_js_undefined;
     use std::cell::Cell;
@@ -491,4 +523,40 @@ mod tests {
 
         Ok(())
     }
+
+    #[test]
+    fn test_wrap_callback_can_throw_typed_errors() -> Result<()> {
+        error_test_case(|| JSError::Internal("".to_string()), "InternalError")?;
+        error_test_case(|| JSError::Range("".to_string()), "RangeError")?;
+        error_test_case(|| JSError::Reference("".to_string()), "ReferenceError")?;
+        error_test_case(|| JSError::Syntax("".to_string()), "SyntaxError")?;
+        error_test_case(|| JSError::Type("".to_string()), "TypeError")?;
+        Ok(())
+    }
+
+    fn error_test_case<F>(error: F, js_type: &str) -> Result<()>
+    where
+        F: Fn() -> JSError + 'static,
+    {
+        let ctx = Context::default();
+        ctx.global_object()?.set_property(
+            "foo",
+            ctx.wrap_callback(move |_, _, _| Err(error().into()))?,
+        )?;
+        ctx.eval_global(
+            "main",
+            &format!(
+                "
+                try {{
+                    foo()
+                }} catch (e) {{
+                    if (e instanceof {js_type}) {{
+                        result = true
+                    }}
+                }}"
+            ),
+        )?;
+        assert!(ctx.global_object()?.get_property("result")?.as_bool()?);
+        Ok(())
+    }
 }
diff --git a/crates/quickjs-wasm-rs/src/js_binding/error.rs b/crates/quickjs-wasm-rs/src/js_binding/error.rs
@@ -0,0 +1,25 @@
+use std::error::Error;
+use std::fmt::{self, Display, Formatter};
+
+#[derive(Debug)]
+pub enum JSError {
+    Syntax(String),
+    Type(String),
+    Reference(String),
+    Range(String),
+    Internal(String),
+}
+
+impl Display for JSError {
+    fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
+        match self {
+            Self::Internal(msg)
+            | Self::Range(msg)
+            | Self::Reference(msg)
+            | Self::Syntax(msg)
+            | Self::Type(msg) => write!(f, "{}", msg),
+        }
+    }
+}
+
+impl Error for JSError {}
diff --git a/crates/quickjs-wasm-rs/src/js_binding/mod.rs b/crates/quickjs-wasm-rs/src/js_binding/mod.rs
@@ -1,5 +1,6 @@
 pub mod constants;
 pub mod context;
+pub mod error;
 pub mod exception;
 pub mod properties;
 pub mod value;
diff --git a/crates/quickjs-wasm-rs/src/lib.rs b/crates/quickjs-wasm-rs/src/lib.rs
@@ -2,6 +2,7 @@ mod js_binding;
 mod serialize;
 
 pub use crate::js_binding::context::Context;
+pub use crate::js_binding::error::JSError;
 pub use crate::js_binding::exception::Exception;
 pub use crate::js_binding::value::Value;
 pub use crate::serialize::de::Deserializer;

diff --git a/wpt/test_spec.js b/wpt/test_spec.js
@@ -36,10 +36,10 @@ export default [
   // { // FIXME need to fix the type of the exception thrown
   //   testFile: "custom_tests/textdecoder-fatal-streaming.any.js",
   // },
-  // { // FIXME need to fix the type of the exception thrown when fatal is set to `true`
-  //   testFile: "upstream/encoding/textdecoder-fatal.any.js",
-  //   ignoredTests: ["Fatal flag: utf-16le - truncated code unit"],
-  // },
+  {
+    testFile: "upstream/encoding/textdecoder-fatal.any.js",
+    ignoredTests: ["Fatal flag: utf-16le - truncated code unit"],
+  },
   {
     testFile: "upstream/encoding/textdecoder-ignorebom.any.js",
     ignoredTests: ["/utf-16/"]