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

Implement FromWasmAbi for Result<i32|u32, JsValue> #1528

Closed
ibaryshnikov opened this issue May 12, 2019 · 6 comments · Fixed by #1550
Closed

Implement FromWasmAbi for Result<i32|u32, JsValue> #1528

ibaryshnikov opened this issue May 12, 2019 · 6 comments · Fixed by #1550

Comments

@ibaryshnikov
Copy link
Member

It looks like there's no FromWasmAbi implementation for Result<u32, JsValue> and Result<i32, JsValue>
The feature is required by #1379, since there are return types like Result<i32, JsValue> in Atomics
https://github.com/rustwasm/wasm-bindgen/blob/master/crates/js-sys/src/lib.rs#L528

@alexcrichton
Copy link
Contributor

It's intended that Result can be returned from functions but for FromWasmAbi that typically means it's an argument I think? Are you trying to take a Result as an argument? Or is there a different situation where the requirement for Result: FromWasmAbi is coming up?

@ibaryshnikov
Copy link
Member Author

Here is an example

pub mod Atomics {
    use super::*;

    #[wasm_bindgen]
    extern "C" {
        #[wasm_bindgen(js_namespace = Atomics, catch)]
        pub fn add(typed_array: &JsValue, index: u32, value: i32) -> Result<JsValue, JsValue>;

When I run wasm-pack test --headless --safari in crates/futures folder, it fails with an error

error[E0277]: the trait bound `std::result::Result<wasm_bindgen::JsValue, wasm_bindgen::JsValue>: wasm_bindgen::convert::traits::FromWasmAbi` is not satisfied
   --> crates/js-sys/src/lib.rs:522:5
    |
522 |     #[wasm_bindgen]
    |     ^^^^^^^^^^^^^^^ the trait `wasm_bindgen::convert::traits::FromWasmAbi` is not implemented for `std::result::Result<wasm_bindgen::JsValue, wasm_bindgen::JsValue>`

The error points here https://github.com/rustwasm/wasm-bindgen/blob/master/crates/js-sys/src/lib.rs#L519
Originally the return type was Result<i32, JsValue>, but changing i32 to JsValue didn't help. I wonder, what could cause the error

@alexcrichton
Copy link
Contributor

Sorry I think I still may not be quite following, could you perhaps gist a standalone example that exhibits the error?

@ibaryshnikov
Copy link
Member Author

ibaryshnikov commented May 15, 2019

Found it! I was confused by error message.

use wasm_bindgen::prelude::*;

pub mod Namespace {
    use super::*;

    #[wasm_bindgen]
    extern "C" {
        #[wasm_bindgen(js_namespace = Namespace)]
        pub fn example() -> Result<JsValue, JsValue>;
    }
}

If we try to run wasm-pack build, the error will be

error[E0277]: the trait bound `std::result::Result<wasm_bindgen::JsValue, wasm_bindgen::JsValue>: wasm_bindgen::convert::traits::FromWasmAbi` is not satisfied
 --> src/lib.rs:6:5
  |
6 |     #[wasm_bindgen]
  |     ^^^^^^^^^^^^^^^ the trait `wasm_bindgen::convert::traits::FromWasmAbi` is not implemented for `std::result::Result<wasm_bindgen::JsValue, wasm_bindgen::JsValue>

The mistake in code is missing catch here:

#[wasm_bindgen(js_namespace = Namespace)]
pub fn example() -> Result<JsValue, JsValue>;

and the following code compiles:

#[wasm_bindgen(js_namespace = Namespace, catch)]
pub fn example() -> Result<JsValue, JsValue>;

@alexcrichton
Copy link
Contributor

Hm I'm unfortunately still having difficulty reproducing this and I don't know why, @ibaryshnikov can you perhaps send a PR adding a test to crates/macro/ui-tests/*.rs which produces the desired error?

@ibaryshnikov
Copy link
Member Author

@alexcrichton I tried to add the code to crates/macro/ui-tests/ but it doesn't trigger an error
I created a repo with minimal example, you can checkout it and run wasm-pack build to see the error
https://github.com/ibaryshnikov/fromwasmabi

alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue May 20, 2019
This commit tweaks the codegen for imported functions and such (anything
that relies on some imported intrinsic or function filled in by the CLI)
to share as much code as possible on non-wasm32 platforms. This should
help us catch more errors before compiling to wasm and also just make it
easier to write UI tests!

For example a UI test previously couldn't be written for rustwasm#1528 but now
it can be, and one is include (although the error message is quite bad).
alexcrichton added a commit to alexcrichton/wasm-bindgen that referenced this issue May 20, 2019
Rejigger a few spans, work around an odd rustc issue, and hopefully
produce higher quality error messages!

Closes rustwasm#1528
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants