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

[Rust] Introduce the NeverType type #2497

Merged
merged 13 commits into from
May 16, 2023

Conversation

apepkuss
Copy link
Collaborator

In this PR, the NeverType type is introduced as a workaround solution to support the generic context data argument of host functions. This solution will be used until the official never type ! in Rust is stable.

Note that the new type is only available for defining sync host functions. For async host functions, it will be used after the in-progress new async design is stable.

@apepkuss apepkuss requested a review from hydai as a code owner May 15, 2023 02:20
Copy link
Member

juntao commented May 15, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


This Pull Request titled "[Rust] Introduce the NeverType type" focuses on integrating the NeverType type to various parts of the Rust codebase, including function signatures, imports, and examples. Major changes involve introducing the new NeverType type, adding a data parameter across various functions as a *mut std::os::raw::c_void, and updating dependency versions. However, several potential issues require attention:

  1. There are concerns regarding the timespec and usage of the raw pointer *mut std::os::raw::c_void, which may lead to data races, unsafe code, or misuse of the parameter. Developers should handle raw pointers cautiously and ensure adequate safety measures.

  2. The purpose and usage of the NeverType are not clear from the changes, with no examples or test cases specifically covering its usage. Proper explanations, documentation, and tests should be provided to clarify its functionality.

  3. Panics might occur at run-time if unsupported types are provided for the data parameter in macro usage or incorrect arguments are passed. Developers should be cautious about these scenarios and provide appropriate handling mechanisms.

  4. Compatibility issues may arise with existing code as NeverType is introduced in function signatures, changes in anonymous async_host_func, and updates in dependency versions for bindgen, wasmedge-sys, and wasmedge-macro. It is crucial to ensure that these changes maintain compatibility with the rest of the codebase and do not introduce breaking changes or conflicts.

  5. The changes may affect the overall performance of the SDK, which should be carefully evaluated and tested.

It is recommended to address these issues and ensure proper testing, documentation, and compatibility before merging the pull request.

Details

Commit 94ec5c3887d500e07c2b3e0117c4f672e51d6a2c

The provided GitHub patch introduces several key changes:

  1. Adds support for context data in host function calls by updating the Function::create() method to accept an additional data parameter, which, when present, is passed to the host function as a *mut std::os::raw::c_void.
  2. Adds a new NeverType enum as a workaround for the unstable Rust ! (never) type.
  3. Updates various examples and tests to reflect the new Function::create() method signature.
  4. Modifies the wrap_fn() function to pass the new data parameter when calling the real host function.

Potential problems:

  1. In the patch, the data parameter is used with both mutable and immutable references. This could lead to potential data races if there are multiple threads trying to access shared data simultaneously.

Commit ac6f36caf327a60fcafded97826ee959df6da905

This patch updates the version of the wasmedge-sys Rust package (located in bindings/rust/wasmedge-sys/Cargo.toml) from 0.13.1 to 0.14.0. The change is straightforward and does not touch any other dependencies, files, or functionality.

As a reviewer for this specific Pull Request, I would not be able to identify any potential problems without further context or information about changes in version 0.14.0 of the wasmedge-sys package. However, it's worth investigating any breaking changes, new features, or bug fixes that have been introduced in this new version before approving the update.

Commit d95ba51a3adb1f5ea9c5fbfc308511ded35c5ead

This patch introduces a new update to the host_function macro in the wasmedge-macro crate for the Rust programming language. The following are the key changes:

  1. A new input parameter data of type *mut std::os::raw::c_void has been added to the wrapper function wrapper_fn_inputs.
  2. A new match arm for a three-argument version of the host function has been added. It handles the new data parameter passed to the macro.
  3. In this match arm, the last argument of the input function is extracted, and its type is matched accordingly. The type_ptr is constructed as required (mutable reference, Option type, or panic in unsupported cases).

Potential problems:

  1. If the user provides an unsupported type for the last data argument, the macro will panic at run-time.
  2. The macro assumes that the user will always provide three arguments, and the last argument is always of type *mut std::os::raw::c_void. If the user fails to provide the proper arguments, the macro will panic.
  3. The expand_host_func() function might need more tests to ensure correctness in different scenarios related to the new changes.

Commit 945c539fd1972b0f6e824c7f029b2d23c68afa4d

This pull request introduces the NeverType type to the Rust WasmEdge SDK. The key changes made include:

  1. Added NeverType as an additional generic parameter in various function signatures such as:

    • Func::new
    • Func::wrap
    • ImportObjectBuilder::with_func
  2. The real_func closure in Func::new and Func::wrap now accepts an additional parameter of type *mut std::os::raw::c_void. This corresponds to the additional data object passed to the host function context.

  3. Implemented the host functions host_layer1 and host_layer2 with the new additional argument.

  4. Modified example files to accommodate the new changes.

Potential problems:

  1. The introduction of NeverType in function signatures may break compatibility with existing code that calls these functions.

  2. Possible misuse or misunderstanding of the new *mut std::os::raw::c_void parameter in the real_func closure.

  3. The changes may affect the overall performance of the SDK.

It is recommended to carefully evaluate the potential problems and their implications on existing code, and to properly test and document the new features to ensure smooth integration.

Commit 93c7aa6b965e867cc345dc276f7330a22bb22357

This patch updates the version of the wasmedge-macro crate from 0.3.0 to 0.4.0. The changes are applied to the following files:

  1. bindings/rust/Cargo.toml
  2. bindings/rust/wasmedge-macro/Cargo.toml

Key changes:

  • In bindings/rust/Cargo.toml, the version of wasmedge-macro is updated from 0.3 to 0.4.
  • In bindings/rust/wasmedge-macro/Cargo.toml, the version of wasmedge-macro is updated from 0.3.0 to 0.4.0.

There are no significant issues or major problems with this patch. However, it's essential to ensure that the new version 0.4.0 of wasmedge-macro maintains compatibility with the rest of the codebase and doesn't introduce any breaking changes or dependency conflicts.

Commit ef845e7631ba3a7a4f61f9f4718db65863d0fbaf

This patch introduces changes to the bindings/rust/wasmedge-macro/src/lib.rs file, focusing on the update of async macros. Here's a summary of key changes:

  1. In the expand_async_host_func_with_two_args function, an additional argument (_data: *mut std::os::raw::c_void) is inserted into fn_inputs. This also implies adding the necessary import, use std::os::raw.

  2. The same change is applied in the sys_expand_async_host_func_with_two_args function, where the new _data parameter is added to fn_inputs.

Potential problems:

  1. The use of raw pointers (*mut) could lead to unsafe code, and as a Rust developer, you should be cautious when using them. There might be a better alternative to using raw pointers to ensure memory safety.

  2. The inserted argument has a name _data, which starts with an underscore. It's a convention in Rust that names starting with an underscore indicate unused variables. If _data is meant to be used, it might create confusion.

Commit 07f4e98bf4913b31c6d7a746c73420a4a3305acf

Summary of key changes:

  1. A new example file context_data_to_host_func.rs has been created with an implementation of real_add function and its usage through a Data struct.
  2. The Function struct's create() method has been updated in function.rs to include generic type NeverType.
  3. The real_fn closure function signature has been updated to include the new data parameter.

Potential problems:

  1. The data parameter is not passed to the Function::create() call in main() in the newly added example file context_data_to_host_func.rs. This might prevent the data from being correctly passed to the real_add function.
  2. The usage of the NeverType type is not clear from this patch, and there's no test or example illustrating how this type might be used. This may make it difficult for other developers to understand its purpose and functionality.

Commit 4b6b552e2ace2abcb0dae70d8226d99cfa1db9eb

Summary of key changes:

  • In the file bindings/rust/wasmedge-sys/examples/async_host_func.rs, the anonymous async_host_func has been updated.
  • The lambda function signature was changed from |_frame, _input| { to |_frame, _input, _data| {.

Potential problems:

  • The added _data parameter is not being used within the lambda function body. This could be intentional for the example, but it is important to verify that this change is as intended and won't break any existing code. If the _data parameter is meant to be used, its usage should be documented or demonstrated in the example.
  • Depending on how this lambda function is being called, the argument passed for _data may be missing or incorrect. It is essential to check all places where async_host_func is being called and ensure the new signature is correctly accommodated.
  • No accompanying updates to comments or documentation were made regarding the change in the anonymous async_host_func. If this change is significant, it may be helpful to update the comments to reflect the intended purpose and usage of the added _data parameter.

Commit 172e7e10440ea98f7d58f9cbe1ed12f42278f1d4

Summary:
This patch introduces two changes in the Rust Wasmedge SDK:

  1. In the file bindings/rust/wasmedge-sdk/src/externals/function.rs, an additional parameter of type *mut std::os::raw::c_void is added to the input parameters of the real_func function.
  2. Similarly, in the file bindings/rust/wasmedge-sdk/src/import.rs, the same parameter is added to the input parameters of the real_func function.

Potential problems:

  1. The newly introduced parameter is a raw pointer (*mut std::os::raw::c_void), which could lead to unsafe code if not handled properly. The developers should be cautious with its usage and ensure that adequate safety measures are taken.
  2. The commit message and the pull request title don't provide enough context or information about the reason behind these changes. The purpose and use case of the added parameter should be adequately documented.

Commit 7e1caa7b48046a8552537e7b9b082333a07db7b0

This patch introduces the NeverType type to the Rust code and makes related changes in the function calls.

Key changes:

  1. Import NeverType in 'func_call_across_different_modes.rs' and 'test_aot.rs'.
  2. Update Function::create calls to use NeverType and remove the last argument 0.
  3. Update the Function::create calls within the host_print_i32 and host_print_f64 functions, adding None as the third argument.
  4. Add #[sys_host_function] attribute to spec_test_print function.

Potential problems:

  1. The purpose of introducing the NeverType is not clear from the patch. A proper explanation or comment should be added to justify its usage.
  2. It would be useful to have tests or examples specifically aimed at demonstrating the usage and benefits of the NeverType.

Commit 55888ee2962fd0f701c12e80293ebd13b923fcc1

The pull request introduces the NeverType in the Rust SDK code by updating the instance.rs file. Here is the summary of key changes:

  1. Two lines are modified in the "tests" module imports, namely:

    • MemoryType, Module, Mutability, RefType is modified to MemoryType, Module, Mutability, NeverType, RefType.
    • ValType, WasmValue is modified to ValType, WasmValue, NeverType.
  2. The real_add function signature is updated to add a _data parameter of type *mut std::os::raw::c_void in the tests module.

  3. The ImportObjectBuilder is modified with a new signature for with_func function, where the previous signature is changed from with_func::<(i32, i32), i32>("add", real_add) to with_func::<(i32, i32), i32, NeverType>("add", real_add, None).

Potential Problems:

  1. It is unclear what the new NeverType is supposed to do in this context. There should be more documentation or comments to explain the purpose and usage of this new type.
  2. Although the _data parameter is added to the real_add function signature, it's not used inside the function. This can lead to confusion, and it should be either used or documented why it's necessary.
  3. There are no new test cases that specifically cover the usage of this new NeverType, which makes it hard to ensure the correctness or potential issues related to this change.

Commit ee8d55b20c5bc674ca7e896fdd12e1fb32128183

Summary of key changes:

  • This patch updates the dependency of the bindgen crate in wasmedge-sys from version 0.61 to 0.65.

Potential problems:

  • The potential issue with updating the dependency is the compatibility of the new version with the existing code. There might be breaking changes, deprecated features, or behavior differences in the new version of the bindgen crate that could affect the project.

Recommendation:

  • Before merging this pull request, it is important to ensure that the update to bindgen version 0.65 is compatible with the rest of the code in the wasmedge-sys package. This can be done by running the existing tests and potentially adding new tests to ensure that the updated dependency does not introduce any unexpected issues.

@apepkuss
Copy link
Collaborator Author

@hydai Could you please help review this PR? Thanks a lot!

@hydai hydai merged commit 54d5a51 into WasmEdge:master May 16, 2023
@apepkuss
Copy link
Collaborator Author

@hydai Thanks for the review!

@apepkuss apepkuss deleted the rust/feat-never-type branch May 17, 2023 01:22
sarrah-basta pushed a commit to sarrah-basta/WasmEdge that referenced this pull request Oct 20, 2023
* feat(rust-sys): support context data in host func

Signed-off-by: Xin Liu <[email protected]>

* version(rust-sys): bump to 0.14.0

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-macro): update host_function macro

Signed-off-by: Xin Liu <[email protected]>

* feat(rust-sdk): support context data in host func

Signed-off-by: Xin Liu <[email protected]>

* version(rust-macro): bump to 0.4.0

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-macro): update async macros

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): new example

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sdk): update code

Signed-off-by: Xin Liu <[email protected]>

* chore(rust-sys): update dependency

Signed-off-by: Xin Liu <[email protected]>

---------

Signed-off-by: Xin Liu <[email protected]>
Co-authored-by: hydai <[email protected]>
Signed-off-by: Sarrah Bastawala <[email protected]>
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.

3 participants