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

added downcast functionality to Box<dyn HostError> #236

Merged
merged 4 commits into from Jul 27, 2020

Conversation

reuvenpo
Copy link
Contributor

@reuvenpo reuvenpo commented Jul 22, 2020

@pepyakin Sorry for so many small PRs one after the other, this time I made sure the code does what i need it to, and added a documentation test.
The last missing piece was adding by-value downcasting functionality (as opposed to the by-reference downcast). I use the downcast-rs crate here which provides a handy shortcut to implementing this but there's no magic happening there, as you can see in its docs. It also supports no_std as you can see.
Now I can write code like this:

let wasmi_error = /* Something that returns an instance of wasmi::Error */;
match wasmi_error.try_into_host_error() {
  // host_error: Box<dyn HostError>
  Ok(host_error) => match host_error.downcast::<MyError>() {
    Ok(my_error) => /* my_error is a Box<MyError>. I can get my error object by unboxing it */,
    Err(host_error) => /* I got the host error back */,
}
  Err(wasmi_error) => /* I got the original error object back */,
}

@reuvenpo
Copy link
Contributor Author

Also, which commands do you run to see the full test suite?
Running cargo test fails because of missing .wast files.
I checked the doc test locally running:

cargo test --doc
cargo test --doc --no-default-features --features=core

@pepyakin
Copy link
Collaborator

Hey, thanks for the PR!

yeah cargo test should run the full suite. However, not all checks are performed. One of the reasons why travis fails is that the code is not properly formatted. Running cargo fmt should fix that.

@reuvenpo
Copy link
Contributor Author

Should i run it and push another commit?

@pepyakin
Copy link
Collaborator

Yeah, please! I also yanked the wabt that might fix the test failure we are seeing here.

@reuvenpo
Copy link
Contributor Author

reuvenpo commented Jul 27, 2020

Do you mean that you modified master? I don't see any new commits there. Should i pull in any other branches?

@reuvenpo
Copy link
Contributor Author

well that was easier than expected :D

@pepyakin pepyakin merged commit 84d2764 into wasmi-labs:master Jul 27, 2020
@pepyakin
Copy link
Collaborator

yeah, the problem was coming from another crate we use for testing: wabt. It had minor version bump, however, it had some changes to wat format that prevented us to run the tests.

@reuvenpo
Copy link
Contributor Author

Thanks for the merge!

yeah, the problem was coming from another crate we use for testing: wabt

Ah, cool, I see.

I will comment though that i still can't get the test suite to pass locally, even after running cargo clean (at least, from the top project directory)

@pepyakin
Copy link
Collaborator

have you tried to execute git submodule update?

@reuvenpo
Copy link
Contributor Author

ohhh yeah that was what i was missing, thanks for that - i didn't notice the submodule definition.
So now i'm getting this set of failing tests:

failures:

---- spec::wasm_conversions stdout ----
running test: conversions
thread 'spec::wasm_conversions' panicked at 'Can't read spec script: WabtError(Error(Parse("conversions.wast:347:1: error: unexpected token (, expected EOF.\n(assert_return_canonical_nan (invoke \"f64.promote_f32\" (f32.const nan)))\n^\n")))', tests/spec/run.rs:357:22
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- spec::wasm_f32 stdout ----
running test: f32
thread 'spec::wasm_f32' panicked at 'Can't read spec script: WabtError(Error(Parse("f32.wast:51:1: error: unexpected token (, expected EOF.\n(assert_return_canonical_nan (invoke \"add\" (f32.const -0x0p+0) (f32.const -na...\n^\n")))', tests/spec/run.rs:357:22

---- spec::wasm_f64 stdout ----
running test: f64
thread 'spec::wasm_f64' panicked at 'Can't read spec script: WabtError(Error(Parse("f64.wast:51:1: error: unexpected token (, expected EOF.\n(assert_return_canonical_nan (invoke \"add\" (f64.const -0x0p+0) (f64.const -na...\n^\n")))', tests/spec/run.rs:357:22

---- spec::wasm_float_exprs stdout ----
running test: float_exprs
thread 'spec::wasm_float_exprs' panicked at 'Can't read spec script: WabtError(Error(Parse("float_exprs.wast:49:1: error: unexpected token (, expected EOF.\n(assert_return_arithmetic_nan (invoke \"f32.no_fold_add_zero\" (f32.const nan:0...\n^\n")))', tests/spec/run.rs:357:22

---- spec::wasm_float_misc stdout ----
running test: float_misc
thread 'spec::wasm_float_misc' panicked at 'Can't read spec script: WabtError(Error(Parse("float_misc.wast:572:1: error: unexpected token (, expected EOF.\n(assert_return_canonical_nan (invoke \"f64.sqrt\" (f64.const -0x1.360e8d0032adp...\n^\n")))', tests/spec/run.rs:357:22


failures:
    spec::wasm_conversions
    spec::wasm_f32
    spec::wasm_f64
    spec::wasm_float_exprs
    spec::wasm_float_misc

test result: FAILED. 67 passed; 5 failed; 0 ignored; 0 measured; 0 filtered out

@pepyakin
Copy link
Collaborator

Yeah, this looks like the error in wabt. Try to remove the Cargo.lock file and re run the tests.

@reuvenpo
Copy link
Contributor Author

Ah, awesome, should've tried that :)
Now all tests manage to pass locally 💯
In the mean time we integrated this feature in our repository, which you can check out here: scrtlabs/SecretNetwork#432

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

Successfully merging this pull request may close these issues.

None yet

2 participants