Conversation
saulecabrera
left a comment
There was a problem hiding this comment.
Looking good, left one comment that I think we need to solve before merging this.
| match ext { | ||
| None => Ok(None), | ||
| Some(ext) => ext.wrap_wasmtime_type(&ruby, store.into()).map(Some), | ||
| Err(_) => Ok(None), |
There was a problem hiding this comment.
I think this is unsound and it would be best to surface the error instead. Wasmtime 44 gracefully handles OOM, and the public APIs reflect that change, like in this case returning an error rather than simply returning Option<T>.
There was a problem hiding this comment.
Unsure how this relates to the rest of the API surfaced by wasmtime-rb though, I think we might need to audit it.
There was a problem hiding this comment.
FWIW, the C API for the linker has a similar change. At the moment, the Rust API returns an error saying the item doesn't exist. Is the worry that the Rust API might change in the future to return an error indicating an allocation failure as an additional failure path?
There was a problem hiding this comment.
Is the worry that the Rust API might change in the future to return an error indicating an allocation failure as an additional failure path?
It already does FWIW:
pub fn get(
&self,
mut store: impl AsContextMut<Data = T>,
module: &str,
name: &str,
) -> Result<Extern>
where
T: 'static,
{
let store = store.as_context_mut().0;
match self._get(module, name) {
// Safety: `T` is connecting the linker and store.
Some(def) => Ok(unsafe { def.to_extern(store)? }),
None => bail!("missing definition for `{module}::{name}`"),
}
}If you follow the path through def.to_extern, it might error with Result<Extern, OutOfMemory>:
pub(crate) unsafe fn to_extern(&self, store: &mut StoreOpaque) -> Result<Extern, OutOfMemory> {
match self {
Definition::Extern(e, _) => Ok(e.clone()),
// SAFETY: the contract of this function is the same as what's
// required of `to_func`, that `T` of the store matches the `T` of
// this original definition.
Definition::HostFunc(func) => unsafe { Ok(func.to_func(store)?.into()) },
}
}
No description provided.