From aa78d491b0a6ffb84439606ac4e7bd20587e056d Mon Sep 17 00:00:00 2001 From: Josh Triplett Date: Sat, 22 Feb 2020 15:56:23 -0800 Subject: [PATCH] Make Func::getN return a Result rather than an Option (#966) This allows getN to return a detailed explanation of any type signature mismatch, and makes it easy to just use `?` on the result of getN rather than constructing a (necessarily vaguer) error message in the caller. --- crates/api/src/func.rs | 79 +++++++++++++++++++++++++--------------- crates/api/tests/func.rs | 68 +++++++++++++++++----------------- crates/wasi/src/lib.rs | 4 +- 3 files changed, 86 insertions(+), 65 deletions(-) diff --git a/crates/api/src/func.rs b/crates/api/src/func.rs index 917946bd2e..bc24660afc 100644 --- a/crates/api/src/func.rs +++ b/crates/api/src/func.rs @@ -1,5 +1,6 @@ use crate::callable::{NativeCallable, WasmtimeFn, WrappedCallable}; use crate::{Callable, FuncType, Store, Trap, Val, ValType}; +use anyhow::{ensure, Context as _}; use std::fmt; use std::mem; use std::panic::{self, AssertUnwindSafe}; @@ -98,7 +99,7 @@ macro_rules! getters { $(#[$doc])* #[allow(non_snake_case)] pub fn $name<$($args,)* R>(&self) - -> Option Result> + -> anyhow::Result Result> where $($args: WasmTy,)* R: WasmTy, @@ -106,23 +107,19 @@ macro_rules! getters { // Verify all the paramers match the expected parameters, and that // there are no extra parameters... let mut params = self.ty().params().iter().cloned(); + let n = 0; $( - if !$args::matches(&mut params) { - return None; - } + let n = n + 1; + $args::matches(&mut params) + .with_context(|| format!("Type mismatch in argument {}", n))?; )* - if !params.next().is_none() { - return None; - } + ensure!(params.next().is_none(), "Type mismatch: too many arguments (expected {})", n); // ... then do the same for the results... let mut results = self.ty().results().iter().cloned(); - if !R::matches(&mut results) { - return None; - } - if !results.next().is_none() { - return None; - } + R::matches(&mut results) + .context("Type mismatch in return type")?; + ensure!(results.next().is_none(), "Type mismatch: too many return values (expected 1)"); // ... and then once we've passed the typechecks we can hand out our // object since our `transmute` below should be safe! @@ -130,9 +127,9 @@ macro_rules! getters { wasmtime_runtime::Export::Function { address, vmctx, signature: _} => { (*address, *vmctx) } - _ => return None, + _ => panic!("expected function export"), }; - Some(move |$($args: $args),*| -> Result { + Ok(move |$($args: $args),*| -> Result { unsafe { let f = mem::transmute::< *const VMFunctionBody, @@ -447,7 +444,7 @@ pub trait WasmTy { #[doc(hidden)] fn push(dst: &mut Vec); #[doc(hidden)] - fn matches(tys: impl Iterator) -> bool; + fn matches(tys: impl Iterator) -> anyhow::Result<()>; #[doc(hidden)] fn from_abi(vmctx: *mut VMContext, abi: Self::Abi) -> Self; #[doc(hidden)] @@ -457,8 +454,8 @@ pub trait WasmTy { impl WasmTy for () { type Abi = (); fn push(_dst: &mut Vec) {} - fn matches(_tys: impl Iterator) -> bool { - true + fn matches(_tys: impl Iterator) -> anyhow::Result<()> { + Ok(()) } #[inline] fn from_abi(_vmctx: *mut VMContext, abi: Self::Abi) -> Self { @@ -475,8 +472,14 @@ impl WasmTy for i32 { fn push(dst: &mut Vec) { dst.push(ValType::I32); } - fn matches(mut tys: impl Iterator) -> bool { - tys.next() == Some(ValType::I32) + fn matches(mut tys: impl Iterator) -> anyhow::Result<()> { + let next = tys.next(); + ensure!( + next == Some(ValType::I32), + "Type mismatch, expected i32, got {:?}", + next + ); + Ok(()) } #[inline] fn from_abi(_vmctx: *mut VMContext, abi: Self::Abi) -> Self { @@ -493,8 +496,14 @@ impl WasmTy for i64 { fn push(dst: &mut Vec) { dst.push(ValType::I64); } - fn matches(mut tys: impl Iterator) -> bool { - tys.next() == Some(ValType::I64) + fn matches(mut tys: impl Iterator) -> anyhow::Result<()> { + let next = tys.next(); + ensure!( + next == Some(ValType::I64), + "Type mismatch, expected i64, got {:?}", + next + ); + Ok(()) } #[inline] fn from_abi(_vmctx: *mut VMContext, abi: Self::Abi) -> Self { @@ -511,8 +520,14 @@ impl WasmTy for f32 { fn push(dst: &mut Vec) { dst.push(ValType::F32); } - fn matches(mut tys: impl Iterator) -> bool { - tys.next() == Some(ValType::F32) + fn matches(mut tys: impl Iterator) -> anyhow::Result<()> { + let next = tys.next(); + ensure!( + next == Some(ValType::F32), + "Type mismatch, expected f32, got {:?}", + next + ); + Ok(()) } #[inline] fn from_abi(_vmctx: *mut VMContext, abi: Self::Abi) -> Self { @@ -529,8 +544,14 @@ impl WasmTy for f64 { fn push(dst: &mut Vec) { dst.push(ValType::F64); } - fn matches(mut tys: impl Iterator) -> bool { - tys.next() == Some(ValType::F64) + fn matches(mut tys: impl Iterator) -> anyhow::Result<()> { + let next = tys.next(); + ensure!( + next == Some(ValType::F64), + "Type mismatch, expected f64, got {:?}", + next + ); + Ok(()) } #[inline] fn from_abi(_vmctx: *mut VMContext, abi: Self::Abi) -> Self { @@ -556,7 +577,7 @@ pub trait WasmRet { #[doc(hidden)] fn push(dst: &mut Vec); #[doc(hidden)] - fn matches(tys: impl Iterator) -> bool; + fn matches(tys: impl Iterator) -> anyhow::Result<()>; #[doc(hidden)] fn into_abi(self) -> Self::Abi; } @@ -567,7 +588,7 @@ impl WasmRet for T { T::push(dst) } - fn matches(tys: impl Iterator) -> bool { + fn matches(tys: impl Iterator) -> anyhow::Result<()> { T::matches(tys) } @@ -583,7 +604,7 @@ impl WasmRet for Result { T::push(dst) } - fn matches(tys: impl Iterator) -> bool { + fn matches(tys: impl Iterator) -> anyhow::Result<()> { T::matches(tys) } diff --git a/crates/api/tests/func.rs b/crates/api/tests/func.rs index 7ab8494b0c..dcb1a1d8c9 100644 --- a/crates/api/tests/func.rs +++ b/crates/api/tests/func.rs @@ -207,33 +207,33 @@ fn trap_import() -> Result<()> { fn get_from_wrapper() { let store = Store::default(); let f = Func::wrap0(&store, || {}); - assert!(f.get0::<()>().is_some()); - assert!(f.get0::().is_none()); - assert!(f.get1::<(), ()>().is_some()); - assert!(f.get1::().is_none()); - assert!(f.get1::().is_none()); - assert!(f.get2::<(), (), ()>().is_some()); - assert!(f.get2::().is_none()); - assert!(f.get2::().is_none()); + assert!(f.get0::<()>().is_ok()); + assert!(f.get0::().is_err()); + assert!(f.get1::<(), ()>().is_ok()); + assert!(f.get1::().is_err()); + assert!(f.get1::().is_err()); + assert!(f.get2::<(), (), ()>().is_ok()); + assert!(f.get2::().is_err()); + assert!(f.get2::().is_err()); let f = Func::wrap0(&store, || -> i32 { loop {} }); - assert!(f.get0::().is_some()); + assert!(f.get0::().is_ok()); let f = Func::wrap0(&store, || -> f32 { loop {} }); - assert!(f.get0::().is_some()); + assert!(f.get0::().is_ok()); let f = Func::wrap0(&store, || -> f64 { loop {} }); - assert!(f.get0::().is_some()); + assert!(f.get0::().is_ok()); let f = Func::wrap1(&store, |_: i32| {}); - assert!(f.get1::().is_some()); - assert!(f.get1::().is_none()); - assert!(f.get1::().is_none()); - assert!(f.get1::().is_none()); + assert!(f.get1::().is_ok()); + assert!(f.get1::().is_err()); + assert!(f.get1::().is_err()); + assert!(f.get1::().is_err()); let f = Func::wrap1(&store, |_: i64| {}); - assert!(f.get1::().is_some()); + assert!(f.get1::().is_ok()); let f = Func::wrap1(&store, |_: f32| {}); - assert!(f.get1::().is_some()); + assert!(f.get1::().is_ok()); let f = Func::wrap1(&store, |_: f64| {}); - assert!(f.get1::().is_some()); + assert!(f.get1::().is_ok()); } #[test] @@ -247,16 +247,16 @@ fn get_from_signature() { let store = Store::default(); let ty = FuncType::new(Box::new([]), Box::new([])); let f = Func::new(&store, ty, Rc::new(Foo)); - assert!(f.get0::<()>().is_some()); - assert!(f.get0::().is_none()); - assert!(f.get1::().is_none()); + assert!(f.get0::<()>().is_ok()); + assert!(f.get0::().is_err()); + assert!(f.get1::().is_err()); let ty = FuncType::new(Box::new([ValType::I32]), Box::new([ValType::F64])); let f = Func::new(&store, ty, Rc::new(Foo)); - assert!(f.get0::<()>().is_none()); - assert!(f.get0::().is_none()); - assert!(f.get1::().is_none()); - assert!(f.get1::().is_some()); + assert!(f.get0::<()>().is_err()); + assert!(f.get0::().is_err()); + assert!(f.get1::().is_err()); + assert!(f.get1::().is_ok()); } #[test] @@ -276,16 +276,16 @@ fn get_from_module() -> anyhow::Result<()> { )?; let instance = Instance::new(&module, &[])?; let f0 = instance.get_export("f0").unwrap().func().unwrap(); - assert!(f0.get0::<()>().is_some()); - assert!(f0.get0::().is_none()); + assert!(f0.get0::<()>().is_ok()); + assert!(f0.get0::().is_err()); let f1 = instance.get_export("f1").unwrap().func().unwrap(); - assert!(f1.get0::<()>().is_none()); - assert!(f1.get1::().is_some()); - assert!(f1.get1::().is_none()); + assert!(f1.get0::<()>().is_err()); + assert!(f1.get1::().is_ok()); + assert!(f1.get1::().is_err()); let f2 = instance.get_export("f2").unwrap().func().unwrap(); - assert!(f2.get0::<()>().is_none()); - assert!(f2.get0::().is_some()); - assert!(f2.get1::().is_none()); - assert!(f2.get1::().is_none()); + assert!(f2.get0::<()>().is_err()); + assert!(f2.get0::().is_ok()); + assert!(f2.get1::().is_err()); + assert!(f2.get1::().is_err()); Ok(()) } diff --git a/crates/wasi/src/lib.rs b/crates/wasi/src/lib.rs index 5f0b24b127..5a6d29c199 100644 --- a/crates/wasi/src/lib.rs +++ b/crates/wasi/src/lib.rs @@ -38,8 +38,8 @@ impl wasmtime::WasmTy for WasiCallerMemory { fn push(_dst: &mut Vec) {} - fn matches(_tys: impl Iterator) -> bool { - true + fn matches(_tys: impl Iterator) -> anyhow::Result<()> { + Ok(()) } fn from_abi(vmctx: *mut wasmtime_runtime::VMContext, _abi: ()) -> Self {