-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Test wasm32 without js #6283
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
Test wasm32 without js #6283
Conversation
WalkthroughThe PR replaces the broken wasm test infrastructure by relocating the wasm crate from Changes
Sequence DiagramsequenceDiagram
participant Host as Host Runtime<br>(main.rs)
participant WasmModule as WASM Module<br>(eval)
participant RustPython as RustPython VM
participant KVStore as KV Store
Host->>Host: Load WASM module from path
Host->>Host: Initialize KV store with code
Host->>WasmModule: Call eval(code_ptr, code_len)
activate WasmModule
WasmModule->>RustPython: Create Interpreter
RustPython->>RustPython: Execute Python expr "1 + 3"
RustPython-->>WasmModule: Result repr
WasmModule->>Host: Call print(msg_ptr, msg_len)
activate Host
Host->>Host: Read string from WASM memory
Host->>Host: Print to stdout
Host-->>WasmModule: return 0
deactivate Host
WasmModule-->>Host: return 0 (success)
deactivate WasmModule
Host->>Host: Report result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
555c55a to
1a92d4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 9
🧹 Nitpick comments (5)
example_projects/wasm32_without_js/wasm-runtime/.gitignore (1)
1-4: Confirm whether the!wasm/rustpython.wasmexception is still neededThis override looks like a leftover from the previous layout; the new example seems to produce
rustpython-without-js/target/.../rustpython_without_js.wasminstead. Ifwasm/rustpython.wasmis no longer used, consider removing or updating this exception to avoid confusion.example_projects/wasm32_without_js/rustpython-without-js/Cargo.toml (1)
13-15: Consider dropping empty[workspace]/[patch.crates-io]sectionsIf this crate is meant to live inside the existing top-level workspace and you’re not patching crates, these sections can be removed to reduce manifest noise and avoid accidentally creating a nested workspace.
example_projects/wasm32_without_js/wasm-runtime/README.md (1)
3-11: Clarify thewat2wasmprerequisite or remove itThe README asks to “install wat2wasm and rust”, but the example usage only shows:
cargo run --release <wasm binary>If the current flow always consumes a
.wasmproduced bycargo build(as in the other README) and no.watis involved anymore, consider either:
- Explaining where
wat2wasmfits into the workflow, or- Dropping the
wat2wasmprerequisite to keep the instructions focused.example_projects/wasm32_without_js/wasm-runtime/src/main.rs (2)
56-72: Clarify the intent of commented code.The
get_codefunction is fully commented out. Should this be removed entirely, or is it planned for future use? If it's dead code, consider removing it to improve maintainability.
95-100: Clarify hard-coded Python snippet purpose.The runtime initializes the KV store with hard-coded Python code (
"a=10;b='str';f'{a}{b}'"). For an example/demo, this might be appropriate, but consider:
- Documenting why this code is hard-coded
- Making it configurable via command-line or environment variable
- Adding a comment explaining what this snippet is testing
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/ci.yaml(1 hunks)example_projects/wasm32_without_js/.gitignore(1 hunks)example_projects/wasm32_without_js/README.md(1 hunks)example_projects/wasm32_without_js/rustpython-without-js/Cargo.toml(1 hunks)example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs(1 hunks)example_projects/wasm32_without_js/wasm-runtime/.gitignore(1 hunks)example_projects/wasm32_without_js/wasm-runtime/Cargo.toml(1 hunks)example_projects/wasm32_without_js/wasm-runtime/README.md(1 hunks)example_projects/wasm32_without_js/wasm-runtime/src/main.rs(1 hunks)wasm/wasm-unknown-test/Cargo.toml(0 hunks)wasm/wasm-unknown-test/src/lib.rs(0 hunks)
💤 Files with no reviewable changes (2)
- wasm/wasm-unknown-test/src/lib.rs
- wasm/wasm-unknown-test/Cargo.toml
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-06-28T16:31:03.991Z
Learnt from: arihant2math
Repo: RustPython/RustPython PR: 5790
File: build.rs:2-2
Timestamp: 2025-06-28T16:31:03.991Z
Learning: In Cargo build scripts (build.rs), the environment variable CARGO_CFG_TARGET_OS is guaranteed to exist and is automatically set by Cargo during the build process, making unwrap() safe to use when accessing this variable.
Applied to files:
example_projects/wasm32_without_js/.gitignore
🧬 Code graph analysis (2)
example_projects/wasm32_without_js/wasm-runtime/src/main.rs (1)
example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs (3)
kv_get(3-4)kv_put(7-7)
example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs (1)
example_projects/wasm32_without_js/wasm-runtime/src/main.rs (3)
kv_get(13-41)kv_put(44-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Run snippets and cpython tests (windows-latest)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Check Rust code with clippy
- GitHub Check: Check the WASM package and demo
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run tests under miri
- GitHub Check: Run rust tests (windows-latest)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: auto_format
🔇 Additional comments (6)
example_projects/wasm32_without_js/.gitignore (1)
1-2: Standard Rust workspace ignore patterns look appropriate.The patterns correctly ignore build artifacts (
target/) and lock files (Cargo.lock) at the crate level within the workspace, which aligns with typical Rust project conventions for example workspaces.Optional verification: Confirm that the workspace structure matches the pattern depth (one level of subdirectories). If the workspace root itself generates
target/orCargo.lockartifacts that need ignoring, add corresponding root-level patterns (target/andCargo.lockwithout the*/prefix). If those are handled by a parent.gitignorein the RustPython repository root, no further action is needed.example_projects/wasm32_without_js/rustpython-without-js/Cargo.toml (1)
1-4: Verify that usingedition = "2024"matches your toolchain and workspaceThis crate is set to Rust 2024 edition. Please confirm CI and local toolchains actually support this edition and that it’s consistent with the rest of the workspace; otherwise you may want to stick to the edition used by the main crates (commonly 2021 today) to avoid surprise compilation issues.
example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs (1)
51-60:__getrandom_v03_customhas attribute and behavioral issuesThe good news: the current
extern "Rust"ABI is correct pergetrandom0.3 documentation.The remaining issues:
#[unsafe(no_mangle)]is invalid syntax; should be#[no_mangle].- Returns
Ok(())without writing todest, leaving the buffer uninitialized (UB).Fix:
-#[unsafe(no_mangle)] -unsafe extern "Rust" fn __getrandom_v03_custom( +#[no_mangle] +unsafe extern "Rust" fn __getrandom_v03_custom( _dest: *mut u8, _len: usize, ) -> Result<(), getrandom::Error> { - // Err(getrandom::Error::UNSUPPORTED) - - // WARNING: This function **MUST** perform proper getrandom - Ok(()) + // TODO: implement proper randomness for wasm32 + Err(getrandom::Error::UNSUPPORTED) }Likely an incorrect or invalid review comment.
.github/workflows/ci.yaml (1)
412-412: Verify build profile mismatch is intentional.The wasm-runtime is built with
--releasebut loads the wasm artifact from thedebug/directory. This mismatch may be intentional for testing, but please confirm this is the desired behavior.example_projects/wasm32_without_js/wasm-runtime/src/main.rs (1)
1-9: LGTM!The structure and imports are well-organized. The
Ctxstruct appropriately usesOption<Memory>since the memory reference is established after WASM module instantiation.example_projects/wasm32_without_js/wasm-runtime/Cargo.toml (1)
7-7: wasmer 6.1.0 is current and secure.Verification confirms that version
6.1.0is the latest available version on crates.io. The only known security vulnerability (symlink sandbox bypass) affects versions<= 4.3.1and does not impact6.1.0. No action required.
| run: | | ||
| cd wasm/wasm-unknown-test | ||
| cargo build --release --verbose | ||
| if wasm-objdump -xj Import target/wasm32-unknown-unknown/release/wasm_unknown_test.wasm; then | ||
| cd example_projects/wasm32_without_js | ||
| cargo build --manifest-path rustpython-without-js/Cargo.toml | ||
| if wasm-objdump -xj Import rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm; then | ||
| echo "ERROR: wasm32-unknown module expects imports from the host environment" >2 | ||
| fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix shell redirect syntax error.
Line 410 uses >2 which will create a file named "2" instead of redirecting to stderr. The correct syntax is >&2.
Apply this diff:
if wasm-objdump -xj Import rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm; then
- echo "ERROR: wasm32-unknown module expects imports from the host environment" >2
+ echo "ERROR: wasm32-unknown module expects imports from the host environment" >&2
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| run: | | |
| cd wasm/wasm-unknown-test | |
| cargo build --release --verbose | |
| if wasm-objdump -xj Import target/wasm32-unknown-unknown/release/wasm_unknown_test.wasm; then | |
| cd example_projects/wasm32_without_js | |
| cargo build --manifest-path rustpython-without-js/Cargo.toml | |
| if wasm-objdump -xj Import rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm; then | |
| echo "ERROR: wasm32-unknown module expects imports from the host environment" >2 | |
| fi | |
| run: | | |
| cd example_projects/wasm32_without_js | |
| cargo build --manifest-path rustpython-without-js/Cargo.toml | |
| if wasm-objdump -xj Import rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm; then | |
| echo "ERROR: wasm32-unknown module expects imports from the host environment" >&2 | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/ci.yaml around lines 406 to 411: the shell redirection uses
`>2` which creates a file named "2" instead of redirecting to stderr; change the
redirection to `>&2` so the error message is written to stderr (i.e., replace
the `echo "ERROR: ... " >2` with `echo "ERROR: ... " >&2`).
| To test, build rustpython to wasm32-unknown-unknown target first. | ||
|
|
||
| ```shell | ||
| cargo build --manifest-path rustpython-without-js/Cargo.toml | ||
| ``` | ||
|
|
||
| Then there will be `rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm` file. | ||
|
|
||
| Now we can run the wasm file with wasm runtime: | ||
|
|
||
| ```shell | ||
| cargo run --release --manifest-path wasm-runtime/Cargo.toml rustpython-without-js/target/wasm32-unknown-unknown/debug/rustpython_without_js.wasm | ||
| ``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add the explicit wasm32 target to the build command
The text says “build … to wasm32-unknown-unknown target” and then references target/wasm32-unknown-unknown/..., but the sample command doesn’t pass a --target:
cargo build --manifest-path rustpython-without-js/Cargo.tomlTo match the described output path, this should be:
-cargo build --manifest-path rustpython-without-js/Cargo.toml
+cargo build --target wasm32-unknown-unknown --manifest-path rustpython-without-js/Cargo.tomlOtherwise users on a default host toolchain will not get the .wasm artifact at the documented location.
🤖 Prompt for AI Agents
In example_projects/wasm32_without_js/README.md around lines 3 to 15, the build
command omits the explicit wasm32 target so the output path shown will not be
produced on a default host toolchain; update the documented cargo build command
to include --target wasm32-unknown-unknown so the .wasm artifact appears under
target/wasm32-unknown-unknown/... (also consider adding the same --target to any
run command examples that reference that path).
| #[unsafe(no_mangle)] | ||
| pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 { | ||
| // let src = unsafe { std::slice::from_raw_parts(s, l) }; | ||
| // let src = std::str::from_utf8(src).unwrap(); | ||
| // TODO: use src | ||
| let src = "1 + 3"; | ||
|
|
||
| // 2. Execute Python code | ||
| let interpreter = Interpreter::without_stdlib(Default::default()); | ||
| let result = interpreter.enter(|vm| { | ||
| let scope = vm.new_scope_with_builtins(); | ||
| let res = match vm.run_block_expr(scope, src) { | ||
| Ok(val) => val, | ||
| Err(_) => return Err(-1), // Python execution error | ||
| }; | ||
| let repr_str = match res.repr(vm) { | ||
| Ok(repr) => repr.as_str().to_string(), | ||
| Err(_) => return Err(-1), // Failed to get string representation | ||
| }; | ||
| Ok(repr_str) | ||
| }); | ||
| let result = match result { | ||
| Ok(r) => r, | ||
| Err(code) => return code, | ||
| }; | ||
|
|
||
| let msg = format!("eval result: {result}"); | ||
|
|
||
| unsafe { | ||
| print( | ||
| result.as_str().as_ptr() as usize as i32, | ||
| result.len() as i32, | ||
| ) | ||
| }; | ||
|
|
||
| 0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix no_mangle attribute, use the eval input, and print the full message
There are a few issues in this block:
#[unsafe(no_mangle)]is not a valid attribute and will fail to compile.evalignores itss/linputs and always runs"1 + 3", which doesn’t match the documentedeval(code_ptr, code_len)contract.- You build
msg = "eval result: {result}"but passresult(without the prefix) toprint.
A minimal fix that keeps the current structure but makes it usable would look like:
-#[unsafe(no_mangle)]
-pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 {
- // let src = unsafe { std::slice::from_raw_parts(s, l) };
- // let src = std::str::from_utf8(src).unwrap();
- // TODO: use src
- let src = "1 + 3";
+#[no_mangle]
+pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 {
+ let src_bytes = std::slice::from_raw_parts(s, l);
+ let src = match std::str::from_utf8(src_bytes) {
+ Ok(s) => s,
+ Err(_) => return -1, // invalid UTF‑8 input
+ };
@@
- let msg = format!("eval result: {result}");
-
- unsafe {
- print(
- result.as_str().as_ptr() as usize as i32,
- result.len() as i32,
- )
- };
+ let msg = format!("eval result: {result}");
+
+ unsafe {
+ print(
+ msg.as_ptr() as usize as i32,
+ msg.len() as i32,
+ )
+ };This keeps the ABI, actually uses the caller‑provided code, and prints the prefixed message, while preserving your simple i32 error convention.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| #[unsafe(no_mangle)] | |
| pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 { | |
| // let src = unsafe { std::slice::from_raw_parts(s, l) }; | |
| // let src = std::str::from_utf8(src).unwrap(); | |
| // TODO: use src | |
| let src = "1 + 3"; | |
| // 2. Execute Python code | |
| let interpreter = Interpreter::without_stdlib(Default::default()); | |
| let result = interpreter.enter(|vm| { | |
| let scope = vm.new_scope_with_builtins(); | |
| let res = match vm.run_block_expr(scope, src) { | |
| Ok(val) => val, | |
| Err(_) => return Err(-1), // Python execution error | |
| }; | |
| let repr_str = match res.repr(vm) { | |
| Ok(repr) => repr.as_str().to_string(), | |
| Err(_) => return Err(-1), // Failed to get string representation | |
| }; | |
| Ok(repr_str) | |
| }); | |
| let result = match result { | |
| Ok(r) => r, | |
| Err(code) => return code, | |
| }; | |
| let msg = format!("eval result: {result}"); | |
| unsafe { | |
| print( | |
| result.as_str().as_ptr() as usize as i32, | |
| result.len() as i32, | |
| ) | |
| }; | |
| 0 | |
| } | |
| #[no_mangle] | |
| pub unsafe extern "C" fn eval(s: *const u8, l: usize) -> i32 { | |
| let src_bytes = std::slice::from_raw_parts(s, l); | |
| let src = match std::str::from_utf8(src_bytes) { | |
| Ok(s) => s, | |
| Err(_) => return -1, // invalid UTF‑8 input | |
| }; | |
| // 2. Execute Python code | |
| let interpreter = Interpreter::without_stdlib(Default::default()); | |
| let result = interpreter.enter(|vm| { | |
| let scope = vm.new_scope_with_builtins(); | |
| let res = match vm.run_block_expr(scope, src) { | |
| Ok(val) => val, | |
| Err(_) => return Err(-1), // Python execution error | |
| }; | |
| let repr_str = match res.repr(vm) { | |
| Ok(repr) => repr.as_str().to_string(), | |
| Err(_) => return Err(-1), // Failed to get string representation | |
| }; | |
| Ok(repr_str) | |
| }); | |
| let result = match result { | |
| Ok(r) => r, | |
| Err(code) => return code, | |
| }; | |
| let msg = format!("eval result: {result}"); | |
| unsafe { | |
| print( | |
| msg.as_ptr() as usize as i32, | |
| msg.len() as i32, | |
| ) | |
| }; | |
| 0 | |
| } |
🤖 Prompt for AI Agents
In example_projects/wasm32_without_js/rustpython-without-js/src/lib.rs around
lines 13 to 49, fix three issues: change the invalid attribute to #[no_mangle]
(keep the function as pub unsafe extern "C" fn eval(...)), actually read the
caller-provided code by creating a slice from_raw_parts(s, l) and converting it
to &str (return an error code on null/utf8 failure), and build the full message
using the formatted prefix (e.g. "eval result: {repr}") then pass that message's
pointer and length to print (cast pointer/len to the i32 ABI types you use) so
the printed output contains the prefix and the evaluated result.
| [package] | ||
| name = "wasm-runtime" | ||
| version = "0.1.0" | ||
| edition = "2024" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fix invalid Rust edition.
Edition 2024 does not exist. The latest stable Rust edition is 2021. This will cause a compilation failure.
Apply this diff to fix:
-edition = "2024"
+edition = "2021"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| edition = "2024" | |
| edition = "2021" |
🤖 Prompt for AI Agents
In example_projects/wasm32_without_js/wasm-runtime/Cargo.toml around line 4, the
Cargo.toml incorrectly sets edition = "2024"; change it to the latest valid Rust
edition by replacing the value with "2021" so the crate compiles successfully.
| fn kv_get(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 { | ||
| let (c, s) = ctx.data_and_store_mut(); | ||
| let mut key = vec![0u8; kl as usize]; | ||
| if c.mem | ||
| .as_ref() | ||
| .unwrap() | ||
| .view(&s) | ||
| .read(kp as u64, &mut key) | ||
| .is_err() | ||
| { | ||
| return -1; | ||
| } | ||
| match c.kv.get(&key) { | ||
| Some(val) => { | ||
| let len = val.len().min(vl as usize); | ||
| if c.mem | ||
| .as_ref() | ||
| .unwrap() | ||
| .view(&s) | ||
| .write(vp as u64, &val[..len]) | ||
| .is_err() | ||
| { | ||
| return -1; | ||
| } | ||
| len as i32 | ||
| } | ||
| None => 0, | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace unwrap() with proper error handling.
Lines 18 and 30 call unwrap() on c.mem.as_ref(), which will panic if the memory reference is None. While this should never happen if the module is correctly initialized, it creates a potential panic point.
Consider using expect() with a descriptive message or returning an error code:
fn kv_get(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 {
let (c, s) = ctx.data_and_store_mut();
let mut key = vec![0u8; kl as usize];
- if c.mem
- .as_ref()
- .unwrap()
- .view(&s)
+ let mem = c.mem.as_ref().expect("memory not initialized");
+ if mem.view(&s)
.read(kp as u64, &mut key)
.is_err()
{
return -1;
}
match c.kv.get(&key) {
Some(val) => {
let len = val.len().min(vl as usize);
- if c.mem
- .as_ref()
- .unwrap()
- .view(&s)
+ if mem.view(&s)
.write(vp as u64, &val[..len])
.is_err()
{
return -1;
}
len as i32
}
None => 0,
}
}🤖 Prompt for AI Agents
In example_projects/wasm32_without_js/wasm-runtime/src/main.rs around lines 13
to 41, replace the two uses of c.mem.as_ref().unwrap() with safe error handling:
check whether c.mem.as_ref() is Some and if not return an appropriate error code
(e.g. -1) or use expect() with a clear message if you prefer a hard failure;
then use the unwrapped reference afterward (or bind it to a variable) so the
read/write calls operate on a guaranteed memory reference and avoid potential
panics.
| fn kv_put(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 { | ||
| let (c, s) = ctx.data_and_store_mut(); | ||
| let mut key = vec![0u8; kl as usize]; | ||
| let mut val = vec![0u8; vl as usize]; | ||
| let m = c.mem.as_ref().unwrap().view(&s); | ||
| if m.read(kp as u64, &mut key).is_err() || m.read(vp as u64, &mut val).is_err() { | ||
| return -1; | ||
| } | ||
| c.kv.insert(key, val); | ||
| 0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace unwrap() with proper error handling.
Line 48 calls unwrap() on c.mem.as_ref(), which creates a potential panic point if memory is not initialized.
Apply similar fix as suggested for kv_get:
fn kv_put(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 {
let (c, s) = ctx.data_and_store_mut();
let mut key = vec![0u8; kl as usize];
let mut val = vec![0u8; vl as usize];
- let m = c.mem.as_ref().unwrap().view(&s);
+ let m = c.mem.as_ref().expect("memory not initialized").view(&s);
if m.read(kp as u64, &mut key).is_err() || m.read(vp as u64, &mut val).is_err() {
return -1;
}
c.kv.insert(key, val);
0
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn kv_put(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 { | |
| let (c, s) = ctx.data_and_store_mut(); | |
| let mut key = vec![0u8; kl as usize]; | |
| let mut val = vec![0u8; vl as usize]; | |
| let m = c.mem.as_ref().unwrap().view(&s); | |
| if m.read(kp as u64, &mut key).is_err() || m.read(vp as u64, &mut val).is_err() { | |
| return -1; | |
| } | |
| c.kv.insert(key, val); | |
| 0 | |
| } | |
| fn kv_put(mut ctx: FunctionEnvMut<Ctx>, kp: i32, kl: i32, vp: i32, vl: i32) -> i32 { | |
| let (c, s) = ctx.data_and_store_mut(); | |
| let mut key = vec![0u8; kl as usize]; | |
| let mut val = vec![0u8; vl as usize]; | |
| let m = c.mem.as_ref().expect("memory not initialized").view(&s); | |
| if m.read(kp as u64, &mut key).is_err() || m.read(vp as u64, &mut val).is_err() { | |
| return -1; | |
| } | |
| c.kv.insert(key, val); | |
| 0 | |
| } |
🤖 Prompt for AI Agents
In example_projects/wasm32_without_js/wasm-runtime/src/main.rs around lines 44
to 54, replace the direct unwrap() on c.mem.as_ref() with safe handling: check
whether c.mem is Some and if not return an error code (e.g., -1) like kv_get
does; if Some(mem) obtain the memory view (let m = mem.view(&s)) and continue
with the existing reads and inserts, preserving the current read error checks
and returning -1 on failure and 0 on success.
| fn print(mut ctx: FunctionEnvMut<Ctx>, p: i32, l: i32) -> i32 { | ||
| let (c, s) = ctx.data_and_store_mut(); | ||
| let mut msg = vec![0u8; l as usize]; | ||
| let m = c.mem.as_ref().unwrap().view(&s); | ||
| if m.read(p as u64, &mut msg).is_err() { | ||
| return -1; | ||
| } | ||
| let s = std::str::from_utf8(&msg).expect("print got non-utf8 str"); | ||
| println!("{s}"); | ||
| 0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve error handling for memory access and UTF-8 validation.
Two issues:
- Line 78:
unwrap()onc.mem.as_ref()creates a panic point - Line 82:
expect()will panic if the WASM module passes non-UTF8 data
Consider this more robust approach:
fn print(mut ctx: FunctionEnvMut<Ctx>, p: i32, l: i32) -> i32 {
let (c, s) = ctx.data_and_store_mut();
let mut msg = vec![0u8; l as usize];
- let m = c.mem.as_ref().unwrap().view(&s);
+ let m = c.mem.as_ref().expect("memory not initialized").view(&s);
if m.read(p as u64, &mut msg).is_err() {
return -1;
}
- let s = std::str::from_utf8(&msg).expect("print got non-utf8 str");
- println!("{s}");
- 0
+ match std::str::from_utf8(&msg) {
+ Ok(s) => {
+ println!("{s}");
+ 0
+ }
+ Err(_) => {
+ eprintln!("print received non-UTF8 data");
+ -1
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn print(mut ctx: FunctionEnvMut<Ctx>, p: i32, l: i32) -> i32 { | |
| let (c, s) = ctx.data_and_store_mut(); | |
| let mut msg = vec![0u8; l as usize]; | |
| let m = c.mem.as_ref().unwrap().view(&s); | |
| if m.read(p as u64, &mut msg).is_err() { | |
| return -1; | |
| } | |
| let s = std::str::from_utf8(&msg).expect("print got non-utf8 str"); | |
| println!("{s}"); | |
| 0 | |
| } | |
| fn print(mut ctx: FunctionEnvMut<Ctx>, p: i32, l: i32) -> i32 { | |
| let (c, s) = ctx.data_and_store_mut(); | |
| let mut msg = vec![0u8; l as usize]; | |
| let m = c.mem.as_ref().expect("memory not initialized").view(&s); | |
| if m.read(p as u64, &mut msg).is_err() { | |
| return -1; | |
| } | |
| match std::str::from_utf8(&msg) { | |
| Ok(s) => { | |
| println!("{s}"); | |
| 0 | |
| } | |
| Err(_) => { | |
| eprintln!("print received non-UTF8 data"); | |
| -1 | |
| } | |
| } | |
| } |
| fn main() { | ||
| let mut store = Store::default(); | ||
| let module = Module::new( | ||
| &store, | ||
| &std::fs::read(&std::env::args().nth(1).unwrap()).unwrap(), | ||
| ) | ||
| .unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add command-line argument validation.
Line 91 calls nth(1).unwrap() without checking if the argument exists, which will panic if no WASM file path is provided. Consider adding proper error handling.
Apply this diff:
fn main() {
+ let wasm_path = std::env::args()
+ .nth(1)
+ .expect("Usage: wasm-runtime <wasm_file_path>");
+
let mut store = Store::default();
- let module = Module::new(
- &store,
- &std::fs::read(&std::env::args().nth(1).unwrap()).unwrap(),
- )
- .unwrap();
+ let wasm_bytes = std::fs::read(&wasm_path)
+ .expect("Failed to read WASM file");
+ let module = Module::new(&store, &wasm_bytes)
+ .expect("Failed to create WASM module");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn main() { | |
| let mut store = Store::default(); | |
| let module = Module::new( | |
| &store, | |
| &std::fs::read(&std::env::args().nth(1).unwrap()).unwrap(), | |
| ) | |
| .unwrap(); | |
| fn main() { | |
| let wasm_path = std::env::args() | |
| .nth(1) | |
| .expect("Usage: wasm-runtime <wasm_file_path>"); | |
| let mut store = Store::default(); | |
| let wasm_bytes = std::fs::read(&wasm_path) | |
| .expect("Failed to read WASM file"); | |
| let module = Module::new(&store, &wasm_bytes) | |
| .expect("Failed to create WASM module"); |
🤖 Prompt for AI Agents
In example_projects/wasm32_without_js/wasm-runtime/src/main.rs around lines 87
to 93, the code calls std::env::args().nth(1).unwrap() which will panic if no
argument is provided; update the main function to validate the presence of the
first command-line argument, return a clear error or print a usage message and
exit when it is missing, and then use the validated path when reading the file
(e.g., check args().nth(1).ok_or_else(|| ...) or pattern-match and handle the
None case before calling std::fs::read and Module::new).
| let inst = Instance::new(&mut store, &module, &imports).unwrap(); | ||
| env.as_mut(&mut store).mem = inst.exports.get_memory("memory").ok().cloned(); | ||
| let res = inst | ||
| .exports | ||
| .get_function("eval") | ||
| .unwrap() | ||
| .call(&mut store, &[wasmer::Value::I32(0), wasmer::Value::I32(0)]) | ||
| .unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace unwrap() calls and document eval arguments.
Multiple issues:
- Lines 117, 122, 124:
unwrap()calls that could panic - Line 123: The eval function is called with
(0, 0)arguments - their meaning is undocumented
Apply this diff for better error handling:
- let inst = Instance::new(&mut store, &module, &imports).unwrap();
+ let inst = Instance::new(&mut store, &module, &imports)
+ .expect("Failed to instantiate WASM module");
env.as_mut(&mut store).mem = inst.exports.get_memory("memory").ok().cloned();
- let res = inst
- .exports
- .get_function("eval")
- .unwrap()
- .call(&mut store, &[wasmer::Value::I32(0), wasmer::Value::I32(0)])
- .unwrap();
+ let eval_fn = inst
+ .exports
+ .get_function("eval")
+ .expect("WASM module does not export 'eval' function");
+ // TODO: Document what these arguments (0, 0) represent
+ let res = eval_fn
+ .call(&mut store, &[wasmer::Value::I32(0), wasmer::Value::I32(0)])
+ .expect("Failed to call eval function");🤖 Prompt for AI Agents
In example_projects/wasm32_without_js/wasm-runtime/src/main.rs around lines 117
to 124, replace the three unwrap() calls with proper error handling and document
the eval function arguments: check and return or propagate errors when creating
the Instance, when getting the exported memory, and when locating/calling the
"eval" function (e.g., use match/if let or ? with anyhow::Context to add
descriptive messages like "failed to instantiate module", "memory export not
found", "eval function not found" and include the underlying error), and add a
brief comment above the call explaining what the two i32 arguments represent
(for example, pointer/length or args/indexes) and derive or compute the correct
argument values instead of hardcoding (0,0) or validate that (0,0) is intended
before calling.
Co-Authored-By: Valentyn Faychuk <valy@faychuk.com> Co-Authored-By: Lee Dogeon <dev.moreal@gmail.com>
1a92d4f to
9134cca
Compare
Fix #6281
Thanks @valentynfaychuk and @moreal !
Summary by CodeRabbit
New Features
Documentation
Chores