Invoke wave enhancements#10511
Conversation
2c60577 to
e12f933
Compare
pchickey
left a comment
There was a problem hiding this comment.
Thanks for working on this.
I rebased my branch on main, and changed the base branch for this PR to be my branch.
I think if you drop the change to wave core i128, we can merge this to my branch, and then I'll have alex review my PR.
| let low = v as i64; | ||
| let high = (v >> 64) as i64; | ||
| let low = i64::try_from(v).expect("128-bit value too large to fit in i64"); | ||
| let high = i64::try_from(v >> 64).expect("Upper 64 bits of 128-bit value too large for i64"); |
There was a problem hiding this comment.
The original code is correct here - this is making a pair of i64 out of the bits in a i128, so we want those operations to discard the extra bits and not panic when they are present
There was a problem hiding this comment.
Thanks, Pat.
Sure thing. The reason I updated this code is that there was a value truncation error in the cargo clippy CI, which read:
error: casting `u128` to `i64` may truncate the value
help: if this is intentional, allow the lint with `#[allow(clippy::cast_possible_truncation)]`
or use `try_from` and handle the error accordingly
let low = i64::try_from(v);
I will change it back and run clippy locally to ensure it truncates silently and suppresses the warning/error.
Will report back here soon.
There was a problem hiding this comment.
Ok, I did a cargo clean and cargo build --release and then also performed the CI locally i.e.:
export CARGO_INCREMENTAL=0
export CARGO_PROFILE_DEV_DEBUG=0
export CARGO_PROFILE_TEST_DEBUG=0
export RUSTFLAGS="-D warnings"
export WIT_REQUIRE_SEMICOLONS=1
cargo clippy --workspace --all-targetsThe following error occurred:
Checking wiggle v32.0.0 (/Users/tpmccallum/wasmtime/crates/wiggle)
error: casting `u128` to `i64` may truncate the value
--> crates/wasmtime/src/runtime/wave/core.rs:103:19
|
103 | let low = v as i64;
| ^^^^^^^^
|
= help: if this is intentional allow the lint with `#[allow(clippy::cast_possible_truncation)]` ...
= help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#cast_possible_truncation
= note: `-D clippy::cast-possible-truncation` implied by `-D warnings`
= help: to override `-D warnings` add `#[allow(clippy::cast_possible_truncation)]`
help: ... or use `try_from` and handle the error accordingly
|
103 - let low = v as i64;
103 + let low = i64::try_from(v);
|
error: could not compile `wasmtime` (lib) due to 1 previous error
warning: build failed, waiting for other jobs to finish...Reference:
This is the initial GitHub CI error that prompted the try_from approaches:
I will revert to the original code and use the following attribute (at the function level) to see if the CI error goes away for that specific function:
#[allow(clippy::cast_possible_truncation)]
fn unwrap_tuple(&self) -> Box<dyn Iterator<Item = Cow<Self>> + '_> {
let v = unwrap_val!(self, Self::V128, "tuple").as_u128();
let low = v as i64;
let high = (v >> 64) as i64;
Box::new(
[Self::I64(low), Self::I64(high)]
.into_iter()
.map(Cow::Owned),
)
}
}Report back soon ... :)
There was a problem hiding this comment.
Ok, so that attribute (#[allow(clippy::cast_possible_truncation)] at the function level) seemed to do the trick.
Build:
$ cargo build --release
Compiling proc-macro2 v1.0.92
Compiling unicode-ident v1.0.8
-- snip --
Compiling wasmtime-wasi-tls v32.0.0 (/Users/tpmccallum/wasmtime/crates/wasi-tls)
Finished `release` profile [optimized] target(s) in 10m 59sClippy:
$ export CARGO_INCREMENTAL=0
$ export CARGO_PROFILE_DEV_DEBUG=0
$ export CARGO_PROFILE_TEST_DEBUG=0
$ export RUSTFLAGS="-D warnings"
$ export WIT_REQUIRE_SEMICOLONS=1
$ cargo clippy --workspace --all-targets
Compiling proc-macro2 v1.0.92
Compiling unicode-ident v1.0.8
Compiling serde v1.0.215
--snip--
Checking wasmtime-c-api v32.0.0 (/Users/tpmccallum/wasmtime/crates/c-api/artifact)
Finished `dev` profile [unoptimized] target(s) in 4m 34sI will commit the changes now.
…at makes 2 i64 out of 1 i128 and discards extra bits).
|
Ok, signing off for the day. The absence of parentheses will give a helpful result: $ wasmtime run --invoke "compress" compress.wasm Detecting Quotes In RustInterestingly, the absence of outer double quotes is a little trickier. The Rust code I added attempts to test for double quotes via let s = "compress(\"hello\")";
println!("starts_with: {}, ends_with: {}", s.starts_with("\""), s.ends_with("\""));One would assume the result to be starts_with: false, ends_with: falseAlas, this is not a validator that is good enough to know whether the exported function being invoked by // This `invoke` that Rust sees here does not show the outer double quotes. Only the following `func()` or `func(42)`.
let invoke: &String = self.invoke.as_ref().unwrap();
// However, I can get the raw data and see double quotes if/when they exist. For example `"func()"` or `"func(42)"`.
let raw_invoke = format!("{:?}", invoke);The overarching issue is that if a user does not surround the exported function in quotes, it fails in the shell long before the Rust even sees it. And the result is something along these lines: $ wasmtime run --invoke compress(\"hello\") compress.wasm
zsh: unknown file attribute: "wasmtime run --invoke compress("hello") compress.wasm
zsh: unknown file attribute: hThe following quoting examples do work, which is great. $ wasmtime run --invoke "compress(\"hello\")" compress.wasm
[40, 181, 47, 253, 0, 88, 41, 0, 0, 104, 101, 108, 108, 111]$ wasmtime run --invoke 'compress("hello")' compress.wasm
[40, 181, 47, 253, 0, 88, 41, 0, 0, 104, 101, 108, 108, 111]AssistI am stumped in finding a way to enforce outer double or single quotes in the (I am convinced that even something built into wasm-wave would not catch the missing outer double quotes in time either. The errors with no outer quotes are stopped in their tracks in the CLI) For this reason, I ended up being a bit verbose on the documentation on this PR Hope all of that makes sense. Happy to do more - happy reviewing. |
* wip * wasmtime::component::Component: add iterator of exports * components_rec * exports_rec gives fully qualified name * invoke works!!! * code motion * more context in errors * fix test of invoke * Finalized enhancements for --invoke: error messages * Testing and documenting --invoke * Update if else re: invoke * Updating to fix truncation possibilities in unwrap_tuple * Add clippy annotation to resolve CI error and leave original code (that makes 2 i64 out of 1 i128 and discards extra bits). * Format (rustfmt --edition 2021) Rust files in this PR. * Removing duplicate code missed from previous conflict resolution * Add more verbose documentation * Add more verbose documentation --------- Co-authored-by: Pat Hickey <pat@moreproductive.org>
* wip * invoke works!!! * code motion * more context in errors * fix test of invoke * Invoke wave enhancements (#10511) * wip * wasmtime::component::Component: add iterator of exports * components_rec * exports_rec gives fully qualified name * invoke works!!! * code motion * more context in errors * fix test of invoke * Finalized enhancements for --invoke: error messages * Testing and documenting --invoke * Update if else re: invoke * Updating to fix truncation possibilities in unwrap_tuple * Add clippy annotation to resolve CI error and leave original code (that makes 2 i64 out of 1 i128 and discards extra bits). * Format (rustfmt --edition 2021) Rust files in this PR. * Removing duplicate code missed from previous conflict resolution * Add more verbose documentation * Add more verbose documentation --------- Co-authored-by: Pat Hickey <pat@moreproductive.org> * Convert docs and error trapping to single quote approach (#10533) * Convert docs and error trapping to single quote approach * Adjust the error message a little * Respond to review (#10544) * cli: search component for export items --------- Co-authored-by: Timothy McCallum <mistermac2008@gmail.com>
This PR is in relation to the Zulip conversation at #wasmtime > wasmtime-cli: support run --invoke for components using wave.
This PR includes @pchickey's existing PR 10054. (Apologies @pchickey I could not see the
pch/invoke_wavein the dropdown when trying to create a PR that would target yourpch/invoke_wavebranch). I'm not sure whypch/invoke_wavedoes not show up. When I pushed it generated this new 10511 PR and included all of your previous changes, so I hope we are still on the right path to getting this--invokefeature implemented.There is an open issue relating to this work.
Along with the changes, the documentation has also been updated accordingly (the changes in the
docs/cli-options.mdfile of this PR).Functionality
The invoke function works great:
Compress WIT:
lib.rs:
Cargo.toml:
CLI:
/Users/tpmccallum/wasmtime/target/release/wasmtime run --invoke "compress(\"hello\")" /Users/tpmccallum/testing_components/compress/target/wasm32-wasip1/debug/compress.wasm [40, 181, 47, 253, 0, 88, 41, 0, 0, 104, 101, 108, 108, 111]