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

Error type of FunctionMiddleware::feed is private #1950

Closed
webmaster128 opened this issue Dec 17, 2020 · 3 comments · Fixed by #1962
Closed

Error type of FunctionMiddleware::feed is private #1950

webmaster128 opened this issue Dec 17, 2020 · 3 comments · Fixed by #1962
Labels
bug Something isn't working

Comments

@webmaster128
Copy link
Contributor

Describe the bug

The error type of the FunctionMiddleware::feed is BinaryReaderError:

use wasmparser::{BinaryReader, Operator, Result as WpResult, Type};

// …

/// A function middleware specialized for a single function.
pub trait FunctionMiddleware: Debug {
    /// Processes the given operator.
    fn feed<'a>(
        &mut self,
        operator: Operator<'a>,
        state: &mut MiddlewareReaderState<'a>,
    ) -> WpResult<()> {
        state.push_operator(operator);
        Ok(())
    }
}

and

pub type Result<T, E = BinaryReaderError> = result::Result<T, E>;

However, BinaryReaderError is a type in the wasmparser crate with no public constructor. So if I understand correctly, a user of Wasmer cannot implement a FunctionMiddleware that errors.

Steps to reproduce

Expected behavior

The error type is either constructible or can be defined by the user (as in Wasmer 0.17).

Actual behavior

The error type is fixed to wasmparser::BinaryReaderError, which is not constructible.

Additional context

I'm trying rewrite a the Deterministic middleware, orginally created here: https://github.com/spacemeshos/svm/blob/5df80288c8b9a5ab3665297251c283cb614ebb81/crates/svm-compiler/src/middleware/validation.rs.

@webmaster128 webmaster128 added the bug Something isn't working label Dec 17, 2020
@syrusakbary
Copy link
Member

We should improve that, so middlewares can report errors!

Would you like to work on a PR for it @webmaster128 ?

@webmaster128
Copy link
Contributor Author

I'm happy to help where I can. However, in this case I'm a bit clueless what a desired API would look like. I tried the associated error type from Wasmer 0.17, but this leads to all kind of new generic in the code that uses the middleware. So if I don't a brilliant idea magically, it would be cool if someone from the team could look into it.

@webmaster128
Copy link
Contributor Author

Okay, I got some ideas now that I realized everthing is converted to WasmError at the end of the day.

bors bot added a commit that referenced this issue Dec 22, 2020
1962: Reduce scope of wasmparser::BinaryReaderError in the codebase r=MarkMcCaskey a=webmaster128

~Based on #1963~

Closes #1950

# Description

There is an error conversion pileline `BinaryReaderError` -> `WasmError`. In this PR, the same conversion happens earlier, such that `WasmError`/`WasmResult` can be used in middlewares.

Questions:
- [ ] Should middlewares be allowed to produce all `WasmError` cases? Or should we introduce a `WasmError::MiddlewareError(String)`?
- [x] ~Should `to_wasm_error` be removed as part of this PR?~ Extracted to #1963

# Review

- [ ] Add a short description of the the change to the CHANGELOG.md file


Co-authored-by: Simon Warta <simon@warta.it>
@bors bors bot closed this as completed in fd38d73 Dec 23, 2020
@bors bors bot closed this as completed in #1962 Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants