[x/programs/rust] Use clippy::pedantic in the SDK #395
Conversation
richardpringle
requested review from
hexfusion,
samliok and
patrick-ogrady
as code owners
| // always deallocate the full capacity, initialize vs uninitialized memory is irrelevant here | ||
| let data = Vec::from_raw_parts(ptr, capacity, capacity); | ||
| std::mem::drop(data); | ||
| } |
There was a problem hiding this comment.
Clippy didn't like the usage of #[no_mangle] here. It doesn't make sense to have the #[no_mangle] attribute if the function isn't being exported with extern "C" fn // ....
So instead of exposing a function that isn't being called from the wasm-host, I figured it's probably better to delete these. Let me know if you (the reviewer) thinks otherwise.
There was a problem hiding this comment.
you need #[no_mange] when dealing with wazero and the rust compiler. Otherwise, the compiler can you change the function signature which causes an error in the host.
There was a problem hiding this comment.
you should also have extern "C" in that case (at least that's what clippy says)
|
If @hexfusion or @samliok have any Rust work-in-progress, that should definitely be merged first. It would be hard to redo this stuff. Please pay close attention to the |
| @@ -83,7 +87,7 @@ pub fn host_program_invoke(call_ctx: &Context, method_name: &str, args: &[u8]) - | |||
| /// Allocate memory into the module's linear memory | |||
| /// and return the offset to the start of the block. | |||
| #[no_mangle] | |||
| pub fn alloc(len: usize) -> *mut u8 { | |||
| pub extern "C" fn alloc(len: usize) -> *mut u8 { | |||
There was a problem hiding this comment.
Rust does not have a stable ABI, and while it's unlikely to change for function signatures, there are no guarantees. What that means is external calls could break when calling the same "binary" (WASM in this case) compiled with different versions of Rust. extern "C" uses the C ABI, which is stable.
| @@ -1,40 +1,48 @@ | |||
| use crate::errors::StorageError; | |||
| use crate::host::host_program_invoke; | |||
| use crate::host::program_invoke; | |||
There was a problem hiding this comment.
No stutters like host::host_*
| use crate::host::{get_bytes, get_bytes_len, store_bytes}; | ||
| use crate::types::Argument; | ||
| use serde::de::DeserializeOwned; | ||
| use serde::{Deserialize, Serialize}; | ||
| use serde_bare::{from_slice, to_vec}; | ||
| use std::str; | ||
|
|
||
| pub enum StoreResult<'a> { | ||
| Ok(&'a Context), | ||
| #[allow(clippy::module_name_repetitions)] |
There was a problem hiding this comment.
This is another stutter, but I think it's fine to ignore here since causing a collision with std::result::Result would be a pain.
| Ok(&'a Context), | ||
| #[allow(clippy::module_name_repetitions)] | ||
| pub enum StoreResult { | ||
| Ok(Context), |
There was a problem hiding this comment.
Changed this since the Context type is Copy... same reasoning as explained above.
| pub fn store_value<T>(self, key: &str, value: &T) -> Self | ||
| where | ||
| T: Serialize + ?Sized, | ||
| { | ||
| match self { | ||
| Self::Ok(ctx) => ctx.store_value(key, value), | ||
| err => err, | ||
| err @ Self::Err(_) => err, |
There was a problem hiding this comment.
Now if we are to add new variants to StoreResult, we have to handle the behaviour explicitly.
|
|
||
| /// Returns the value of the field `name` from the host. | ||
| /// # Errors | ||
| /// Returns a `StorageError` if the field does not exist or if the bytes are invalid. |
There was a problem hiding this comment.
@samliok, are these comments accurate?
|
|
||
| /// Returns the value of the field `key` from the map `map_name` from the host. | ||
| /// # Errors | ||
| /// Returns a `StorageError` if the field does not exist or if the bytes are invalid. |
There was a problem hiding this comment.
@samliok, are these comments accurate?
| @@ -102,7 +120,7 @@ impl From<i64> for Context { | |||
| } | |||
| } | |||
|
|
|||
| fn store_key_value<T>(ctx: &Context, key_bytes: Vec<u8>, value: &T) -> Result<(), StorageError> | |||
| fn store_key_value<T>(ctx: Context, key_bytes: &[u8], value: &T) -> Result<(), StorageError> | |||
There was a problem hiding this comment.
This was an interesting one... passing in a Vec<u8> here assures that the memory is deallocated after this function exits, which might be desired. However, I did find one place where we were calling to_vec to pass in a value to store_key_value, so taking in a slice requires one less allocation.
I think this is better, but I'm open to an argument for the contrary.
There was a problem hiding this comment.
passed in a Vec since we don't need the memory after, but if its faster I'm all for it.
| bytes_len.try_into().unwrap(), | ||
| bytes_len.try_into().unwrap(), |
There was a problem hiding this comment.
I know there's a check above assuring that the cast will succeed, but generally it's better to avoid using as casts with numbers unless you're doing something that's guaranteed to succeed (like usize as u64).
I think there are some other improvements that can be made to this function (like calling into_boxed_slice so that you can't expand the Vec) and possible changing the return type for get_bytes_len, but that's for another PR.
| Err(StorageError), | ||
| } | ||
|
|
||
| impl StoreResult<'_> { | ||
| impl StoreResult { | ||
| #[must_use] |
There was a problem hiding this comment.
🤷 clippy --fix.
Means you have to use the return value. I think it makes sense.
| @@ -102,7 +120,7 @@ impl From<i64> for Context { | |||
| } | |||
| } | |||
|
|
|||
| fn store_key_value<T>(ctx: &Context, key_bytes: Vec<u8>, value: &T) -> Result<(), StorageError> | |||
| fn store_key_value<T>(ctx: Context, key_bytes: &[u8], value: &T) -> Result<(), StorageError> | |||
There was a problem hiding this comment.
passed in a Vec since we don't need the memory after, but if its faster I'm all for it.
| where | ||
| T: DeserializeOwned, | ||
| { | ||
| let bytes = get_field_as_bytes(ctx, name.as_bytes())?; | ||
| from_slice(&bytes).map_err(|_| StorageError::InvalidBytes) | ||
| } | ||
|
|
||
| /// Gets the correct key to in the host storage for a [map_name] and [key] within that map | ||
| /// Gets the correct key to in the host storage for a [`map_name`] and [key] within that map |
There was a problem hiding this comment.
clippy --fix... but good call out, these docs are actually not formatted properly.
Co-authored-by: Sam Liokumovich <[email protected]> Signed-off-by: Richard Pringle <[email protected]>
Co-authored-by: Sam Liokumovich <[email protected]> Signed-off-by: Richard Pringle <[email protected]>
Clippy::pendanticsaves you from yourself. I recommend this for any open-source Rust (then ignore rules where it's overkill).