Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More flexible chained api #26

Closed
NikVolf opened this issue Jan 28, 2018 · 6 comments
Closed

More flexible chained api #26

NikVolf opened this issue Jan 28, 2018 · 6 comments
Labels
help wanted Extra attention is needed legacy-wasmi A work item for the legacy wasmi implementation. (Version `0.13.0` and older.)

Comments

@NikVolf
Copy link
Contributor

NikVolf commented Jan 28, 2018

in addition to:

https://pepyakin.github.io/wasmi/wasmi/struct.ModuleRef.html#method.invoke_export

used in the following way

        instance.invoke_export(
            "test",
            &[],
            &mut NopExternals,
        ).expect("failed to execute export")

something like

instance.export("test").with_externals(&mut NopExternals).with_args(&[]).invoke()

?

@pepyakin
Copy link
Collaborator

pepyakin commented Jan 28, 2018

That is very good idea actually!

Why?
Because result of invoking of a function might yield only two outcomes:

  • return a result
  • trap

However, the docs at the invoke_export actually give a glimpse of a problem:

Returns `Err` if:

- there are no export with a given name or this export is not a function,
- given arguments doesn't match to function signature,
- trap occured at the execution time,

So in other words, today, we have invoke_index that returns an wasmi::Error which includes various variants that just can't happen in result of invoking a function.

@pepyakin pepyakin added the help wanted Extra attention is needed label Jan 28, 2018
@pepyakin
Copy link
Collaborator

First step to implement this is in #29

@bddap
Copy link
Contributor

bddap commented Oct 26, 2018

How does this look for a start?

instance
    .export_by_name(func_name)?
    .as_func()?
    .with_externals(&mut NopExternals)
    .with_args(&args)?
    .invoke()
    .ok()?;

export_by_name() -> Option<ExternVal> and as_func() -> Option<&FuncRef> already exist.

The rest could be implemented by adding the following to func.rs:

impl FuncRef {
    pub fn with_externals<'a, E: Externals>(
        &'a self,
        externals: &'a mut E,
    ) -> BoundExternalsFunc<'a, E> {
        BoundExternalsFunc {
            func_instance: self,
            externals,
        }
    }
}

/// First step in chained function calling pattern.                                                                                  
pub struct BoundExternalsFunc<'a, E: Externals + 'a> {
    func_instance: &'a FuncRef,
    externals: &'a mut E,
}

impl<'a, E: Externals> BoundExternalsFunc<'a, E> {
    pub fn with_args(self, args: &'a [RuntimeValue]) -> Option<BoundFunc<'a, E>> {
        BoundFunc::new(self.func_instance, self.externals, args)
    }
}

/// FuncRef with verified arguments.                                                                                                 
pub struct BoundFunc<'a, E: Externals + 'a> {
    func_instance: &'a FuncRef,
    externals: &'a mut E,
    args: &'a [RuntimeValue],
}

impl<'a, E: Externals> BoundFunc<'a, E> {
    fn new(
        func_instance: &'a FuncRef,
        externals: &'a mut E,
        args: &'a [RuntimeValue],
    ) -> Option<BoundFunc<'a, E>> {
        check_function_args(func_instance.signature(), &args).ok()?;
        Some(BoundFunc {
            func_instance,
            externals,
            args,
        })
    }

    pub fn invoke(&mut self) -> Result<Option<RuntimeValue>, Trap> {
        FuncInstance::invoke(&self.func_instance, self.args, self.externals)
    }
}

@bddap
Copy link
Contributor

bddap commented Oct 31, 2018

I can submit a pr, just checking first to see if this is approach it useful.

@pepyakin
Copy link
Collaborator

Hello, @bddap ! I'm very sorry for the delay and for that I missed your comment.

Yeah, it is a good start. But we can go beyond that. For example, there is a related problem with limits (#51). And we might want to add other configuration options.

Can you see an approach that can incorporate such changes?

@Robbepop Robbepop added the legacy-wasmi A work item for the legacy wasmi implementation. (Version `0.13.0` and older.) label Jan 6, 2022
@Robbepop
Copy link
Member

This issue is no longer relevant due to the fact that the current wasmi version is using Wasmtime based API which mostly is pretty well designed already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed legacy-wasmi A work item for the legacy wasmi implementation. (Version `0.13.0` and older.)
Projects
None yet
Development

No branches or pull requests

4 participants