impl From<anyhow::Error> for Trap (bytecodealliance#1753)

* From<anyhow::Error> for Trap * Add TrapReason::Error * wasmtime: Improve Error to Trap conversion * Remove Trap::message
kevlarr · May 29, 2020 · 0b3b9c2 · 0b3b9c2
1 parent 8fce8dd
commit 0b3b9c2
Show file tree

Hide file tree

Showing 11 changed files with 74 additions and 65 deletions.
diff --git a/crates/c-api/src/error.rs b/crates/c-api/src/error.rs
@@ -10,8 +10,8 @@ pub struct wasmtime_error_t {
 wasmtime_c_api_macros::declare_own!(wasmtime_error_t);
 
 impl wasmtime_error_t {
-    pub(crate) fn to_trap(&self) -> Box<wasm_trap_t> {
-        Box::new(wasm_trap_t::new(Trap::new(format!("{:?}", self.error))))
+    pub(crate) fn to_trap(self) -> Box<wasm_trap_t> {
+        Box::new(wasm_trap_t::new(Trap::from(self.error)))
     }
 }
 

diff --git a/crates/c-api/src/trap.rs b/crates/c-api/src/trap.rs
@@ -53,7 +53,7 @@ pub extern "C" fn wasm_trap_new(
 #[no_mangle]
 pub extern "C" fn wasm_trap_message(trap: &wasm_trap_t, out: &mut wasm_message_t) {
     let mut buffer = Vec::new();
-    buffer.extend_from_slice(trap.trap.borrow().message().as_bytes());
+    buffer.extend_from_slice(trap.trap.borrow().to_string().as_bytes());
     buffer.reserve_exact(1);
     buffer.push(0);
     out.set_buffer(buffer);

diff --git a/crates/wasmtime/src/linker.rs b/crates/wasmtime/src/linker.rs
@@ -404,11 +404,7 @@ impl Linker {
                 let export_name = export.name().to_owned();
                 let func = Func::new(&self.store, func_ty.clone(), move |_, params, results| {
                     // Create a new instance for this command execution.
-                    let instance = Instance::new(&module, &imports).map_err(|error| {
-                        error
-                            .downcast::<Trap>()
-                            .unwrap_or_else(|error| Trap::new(format!("{:?}", error)))
-                    })?;
+                    let instance = Instance::new(&module, &imports)?;
 
                     // `unwrap()` everything here because we know the instance contains a
                     // function export with the given name and signature because we're

diff --git a/crates/wasmtime/src/runtime.rs b/crates/wasmtime/src/runtime.rs
@@ -905,7 +905,7 @@ impl Store {
     /// });
     ///
     /// let trap = run().unwrap_err();
-    /// assert!(trap.message().contains("wasm trap: interrupt"));
+    /// assert!(trap.to_string().contains("wasm trap: interrupt"));
     /// # Ok(())
     /// # }
     /// ```

diff --git a/crates/wasmtime/src/trap.rs b/crates/wasmtime/src/trap.rs
@@ -20,13 +20,17 @@ enum TrapReason {
 
     /// An `i32` exit status describing an explicit program exit.
     I32Exit(i32),
+
+    /// A structured error describing a trap.
+    Error(Box<dyn std::error::Error + Send + Sync>),
 }
 
 impl fmt::Display for TrapReason {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
         match self {
             TrapReason::Message(s) => write!(f, "{}", s),
             TrapReason::I32Exit(status) => write!(f, "Exited with i32 exit status {}", status),
+            TrapReason::Error(e) => write!(f, "{}", e),
         }
     }
 }
@@ -46,11 +50,12 @@ impl Trap {
     /// # Example
     /// ```
     /// let trap = wasmtime::Trap::new("unexpected error");
-    /// assert_eq!("unexpected error", trap.message());
+    /// assert!(trap.to_string().contains("unexpected error"));
     /// ```
     pub fn new<I: Into<String>>(message: I) -> Self {
         let info = FRAME_INFO.read().unwrap();
-        Trap::new_with_trace(&info, None, message.into(), Backtrace::new_unresolved())
+        let reason = TrapReason::Message(message.into());
+        Trap::new_with_trace(&info, None, reason, Backtrace::new_unresolved())
     }
 
     /// Creates a new `Trap` representing an explicit program exit with a classic `i32`
@@ -68,19 +73,7 @@ impl Trap {
     pub(crate) fn from_runtime(runtime_trap: wasmtime_runtime::Trap) -> Self {
         let info = FRAME_INFO.read().unwrap();
         match runtime_trap {
-            wasmtime_runtime::Trap::User(error) => {
-                // Since we're the only one using the wasmtime internals (in
-                // theory) we should only see user errors which were originally
-                // created from our own `Trap` type (see the trampoline module
-                // with functions).
-                //
-                // If this unwrap trips for someone we'll need to tweak the
-                // return type of this function to probably be `anyhow::Error`
-                // or something like that.
-                *error
-                    .downcast()
-                    .expect("only `Trap` user errors are supported")
-            }
+            wasmtime_runtime::Trap::User(error) => Trap::from(error),
             wasmtime_runtime::Trap::Jit {
                 pc,
                 backtrace,
@@ -100,7 +93,8 @@ impl Trap {
                 backtrace,
             } => Trap::new_wasm(&info, None, trap_code, backtrace),
             wasmtime_runtime::Trap::OOM { backtrace } => {
-                Trap::new_with_trace(&info, None, "out of memory".to_string(), backtrace)
+                let reason = TrapReason::Message("out of memory".to_string());
+                Trap::new_with_trace(&info, None, reason, backtrace)
             }
         }
     }
@@ -125,14 +119,14 @@ impl Trap {
             Interrupt => "interrupt",
             User(_) => unreachable!(),
         };
-        let msg = format!("wasm trap: {}", desc);
+        let msg = TrapReason::Message(format!("wasm trap: {}", desc));
         Trap::new_with_trace(info, trap_pc, msg, backtrace)
     }
 
     fn new_with_trace(
         info: &GlobalFrameInfo,
         trap_pc: Option<usize>,
-        message: String,
+        reason: TrapReason,
         native_trace: Backtrace,
     ) -> Self {
         let mut wasm_trace = Vec::new();
@@ -158,24 +152,13 @@ impl Trap {
         }
         Trap {
             inner: Arc::new(TrapInner {
-                reason: TrapReason::Message(message),
+                reason,
                 wasm_trace,
                 native_trace,
             }),
         }
     }
 
-    /// Returns a reference the `message` stored in `Trap`.
-    ///
-    /// In the case of an explicit exit, the exit status can be obtained by
-    /// calling `i32_exit_status`.
-    pub fn message(&self) -> &str {
-        match &self.inner.reason {
-            TrapReason::Message(message) => message,
-            TrapReason::I32Exit(_) => "explicitly exited",
-        }
-    }
-
     /// If the trap was the result of an explicit program exit with a classic
     /// `i32` exit status value, return the value, otherwise return `None`.
     pub fn i32_exit_status(&self) -> Option<i32> {
@@ -226,4 +209,30 @@ impl fmt::Display for Trap {
     }
 }
 
-impl std::error::Error for Trap {}
+impl std::error::Error for Trap {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        match &self.inner.reason {
+            TrapReason::Error(e) => e.source(),
+            TrapReason::I32Exit(_) | TrapReason::Message(_) => None,
+        }
+    }
+}
+
+impl From<anyhow::Error> for Trap {
+    fn from(e: anyhow::Error) -> Trap {
+        Box::<dyn std::error::Error + Send + Sync>::from(e).into()
+    }
+}
+
+impl From<Box<dyn std::error::Error + Send + Sync>> for Trap {
+    fn from(e: Box<dyn std::error::Error + Send + Sync>) -> Trap {
+        // If the top-level error is already a trap, don't be redundant and just return it.
+        if let Some(trap) = e.downcast_ref::<Trap>() {
+            trap.clone()
+        } else {
+            let info = FRAME_INFO.read().unwrap();
+            let reason = TrapReason::Error(e.into());
+            Trap::new_with_trace(&info, None, reason, Backtrace::new_unresolved())
+        }
+    }
+}
diff --git a/crates/wast/src/wast.rs b/crates/wast/src/wast.rs
@@ -123,7 +123,7 @@ impl WastContext {
     fn module(&mut self, instance_name: Option<&str>, module: &[u8]) -> Result<()> {
         let instance = match self.instantiate(module)? {
             Outcome::Ok(i) => i,
-            Outcome::Trap(e) => bail!("instantiation failed: {}", e.message()),
+            Outcome::Trap(e) => return Err(e).context("instantiation failed"),
         };
         if let Some(name) = instance_name {
             self.linker.instance(name, &instance)?;
@@ -189,7 +189,7 @@ impl WastContext {
             Outcome::Ok(values) => bail!("expected trap, got {:?}", values),
             Outcome::Trap(t) => t,
         };
-        let actual = trap.message();
+        let actual = trap.to_string();
         if actual.contains(expected)
             // `bulk-memory-operations/bulk.wast` checks for a message that
             // specifies which element is uninitialized, but our traps don't

diff --git a/examples/interrupt.rs b/examples/interrupt.rs
@@ -32,7 +32,7 @@ fn main() -> Result<()> {
     let trap = run().unwrap_err();
 
     println!("trap received...");
-    assert!(trap.message().contains("wasm trap: interrupt"));
+    assert!(trap.to_string().contains("wasm trap: interrupt"));
 
     Ok(())
 }
diff --git a/tests/all/custom_signal_handler.rs b/tests/all/custom_signal_handler.rs
@@ -113,10 +113,10 @@ mod tests {
                 .unwrap_err()
                 .downcast::<Trap>()?;
             assert!(
-                trap.message()
-                    .starts_with("wasm trap: out of bounds memory access"),
+                trap.to_string()
+                    .contains("wasm trap: out of bounds memory access"),
                 "bad trap message: {:?}",
-                trap.message()
+                trap.to_string()
             );
         }
 
@@ -140,8 +140,8 @@ mod tests {
                 .unwrap_err()
                 .downcast::<Trap>()?;
             assert!(trap
-                .message()
-                .starts_with("wasm trap: out of bounds memory access"));
+                .to_string()
+                .contains("wasm trap: out of bounds memory access"));
         }
         Ok(())
     }

diff --git a/tests/all/func.rs b/tests/all/func.rs
@@ -181,7 +181,7 @@ fn trap_smoke() -> Result<()> {
     let store = Store::default();
     let f = Func::wrap(&store, || -> Result<(), Trap> { Err(Trap::new("test")) });
     let err = f.call(&[]).unwrap_err().downcast::<Trap>()?;
-    assert_eq!(err.message(), "test");
+    assert!(err.to_string().contains("test"));
     assert!(err.i32_exit_status().is_none());
     Ok(())
 }
@@ -203,7 +203,7 @@ fn trap_import() -> Result<()> {
     .err()
     .unwrap()
     .downcast::<Trap>()?;
-    assert_eq!(trap.message(), "foo");
+    assert!(trap.to_string().contains("foo"));
     Ok(())
 }
 
@@ -397,9 +397,8 @@ fn func_write_nothing() -> anyhow::Result<()> {
     let ty = FuncType::new(Box::new([]), Box::new([ValType::I32]));
     let f = Func::new(&store, ty, |_, _, _| Ok(()));
     let err = f.call(&[]).unwrap_err().downcast::<Trap>()?;
-    assert_eq!(
-        err.message(),
-        "function attempted to return an incompatible value"
-    );
+    assert!(err
+        .to_string()
+        .contains("function attempted to return an incompatible value"));
     Ok(())
 }
diff --git a/tests/all/import_calling_export.rs b/tests/all/import_calling_export.rs
@@ -90,9 +90,8 @@ fn test_returns_incorrect_type() -> Result<()> {
         .call(&[])
         .expect_err("the execution should fail")
         .downcast::<Trap>()?;
-    assert_eq!(
-        trap.message(),
-        "function attempted to return an incompatible value"
-    );
+    assert!(trap
+        .to_string()
+        .contains("function attempted to return an incompatible value"));
     Ok(())
 }
diff --git a/tests/all/traps.rs b/tests/all/traps.rs
@@ -25,7 +25,7 @@ fn test_trap_return() -> Result<()> {
         .expect("error calling function")
         .downcast::<Trap>()?;
 
-    assert_eq!(e.message(), "test 123");
+    assert!(e.to_string().contains("test 123"));
 
     Ok(())
 }
@@ -64,9 +64,9 @@ fn test_trap_trace() -> Result<()> {
     assert_eq!(trace[1].func_offset(), 1);
     assert_eq!(trace[1].module_offset(), 0x21);
     assert!(
-        e.message().contains("unreachable"),
+        e.to_string().contains("unreachable"),
         "wrong message: {}",
-        e.message()
+        e.to_string()
     );
 
     Ok(())
@@ -103,7 +103,7 @@ fn test_trap_trace_cb() -> Result<()> {
     assert_eq!(trace[0].func_index(), 2);
     assert_eq!(trace[1].module_name().unwrap(), "hello_mod");
     assert_eq!(trace[1].func_index(), 1);
-    assert_eq!(e.message(), "cb throw");
+    assert!(e.to_string().contains("cb throw"));
 
     Ok(())
 }
@@ -135,7 +135,7 @@ fn test_trap_stack_overflow() -> Result<()> {
         assert_eq!(trace[i].func_index(), 0);
         assert_eq!(trace[i].func_name(), Some("run"));
     }
-    assert!(e.message().contains("call stack exhausted"));
+    assert!(e.to_string().contains("call stack exhausted"));
 
     Ok(())
 }
@@ -234,7 +234,11 @@ fn trap_start_function_import() -> Result<()> {
     let sig = FuncType::new(Box::new([]), Box::new([]));
     let func = Func::new(&store, sig, |_, _, _| Err(Trap::new("user trap")));
     let err = Instance::new(&module, &[func.into()]).err().unwrap();
-    assert_eq!(err.downcast_ref::<Trap>().unwrap().message(), "user trap");
+    assert!(err
+        .downcast_ref::<Trap>()
+        .unwrap()
+        .to_string()
+        .contains("user trap"));
     Ok(())
 }
 
@@ -373,7 +377,9 @@ fn call_signature_mismatch() -> Result<()> {
         .unwrap()
         .downcast::<Trap>()
         .unwrap();
-    assert_eq!(err.message(), "wasm trap: indirect call type mismatch");
+    assert!(err
+        .to_string()
+        .contains("wasm trap: indirect call type mismatch"));
     Ok(())
 }