[Component] share loading entry for component and module

[dannypsnl]

I was originally want to keep component model in the branch till we complete all of them, but that will make it be hard to merge back at that time. My plan is, I will complete a loader automatically treat both of component and module binary in the same entry point in this PR, once I complete I will convert it to ready.

Then we will have a foundation for further development of component model.

The first step of #2943

[juntao]

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

---

The pull request titled "[Component] share loading entry for component and module" introduces significant changes to the existing software project. The changes include the creation of a new Component class which attributes "magic", "version", and "layer", and an addition of this Component enumeration, along with new methods in the Loader class. The Loader's logic to load the module has been modified to allow for component loading, and there is a stub implementation of a validate function for the new Component class.

Firstly, a significant potential issue is the absence of validation logic for the new Component class, which may allow invalid or malformed components to pass the validation process. The error handling in newly added methods in the Loader class could be significantly improved, more specifically, those methods return a generic unformatted error if the "loadUnit" call fails. Moreover, the concurrent access and dependencies/side-effects of components have not been addressed in this pull request. Also, new methods being added without documentation might cause difficulties to other developers in correctly utilizing these new features.

Aside from that, the PR implements some substantial changes such as the introduction of two new loading functions "loadUniversalWASM" and "loadModuleAOT" and modifications of component loading logic. There are also concerns with the PR like the lack of added test cases, need for better error handling, and possible readability issues due to cognitive complexity in some methods.

Additionally, the PR makes changes to the "validate" function, but seems to be lacking in an explanation of the purpose and functionality of changes. There's also uncertainty about the impact on other modules using this function and whether the changes themselves are tested thoroughly. Another patch introduces component handling along with error messages upon failure, but the file validity checks or alternative execution pathways on failure are lacking.

Furthermore, a worrying fact is the moved Ahead-Of-Time related methods might need adjustments across all the components relying on them. Affected unit tests should be updated to still cover these loading functions. Another significant aspect introduced is the ability to run WebAssembly files with new types of objects. However, this functionality appears to be incomplete and currently unvalidated, which could result in runtime errors.

Lastly, the patch intending to avoid variable shadowing by passing the parsing results directly might make debugging challenging. It's not clear how the error situations are handled when the Wasm unit parsing fails. These changes could confuse other developers if the necessity of such changes is not well-documented.

In conclusion, the pull request introduces a lot of new features and functions enhancing the existing capabilities but also brings several potential issues. Proper documentation, implementing full functionality, thorough testing, and clearer error handling should be the primary focus to make this PR safer and more beneficial.

Details

Commit 79db08fc0d01a259c6a43d059886433846bd2a14

Key changes:

1. Introduction of a new Component class in the AST namespace with "magic", "version", and "layer" attributes. It comes with getters for these attributes.
2. Inclusion of the Component enumeration in "include/common/enum.inc".
3. New methods added to the Loader class to parse and load different components.
4. A new validation method for AST::Component in the Validator class.
5. Addition of "ast/component.cpp" to the CMakeLists.txt file under the path "lib/loader/".
6. The implementation of the new loading and parsing methods in the Loader class.
7. A new file has been added, "lib/loader/ast/component.cpp" that contains some of the logic to load the unit.
8. In the "lib/loader/loader.cpp" file - logic to load module has been extended for component and logic to parse byte code and file paths have been updated, wrapping it under parseWasmUnit and allowing the loading of modules and components separately.
9. A stub implementation of the validate function for the AST::Component class has been added to "lib/validator/validator.cpp".

Potential problems:

1. In the current implementation, the validate function for AST::Component doesn't have any validation logic. This implies that any newly added component will pass validation, which could potentially allow invalid or malformed components to pass through validation.
2. Error handling in the new methods within the Loader class could be improved further. Particularly, methods return a generic unformatted error if the call to loadUnit fails.
3. Without a detailed review of the new Component class, it's hard to specify additional potential issues. For example, the concurrent access of components is not clear from this review alone or if it has dependencies/side-effects that are not addressed in this pull request.
4. Observed "ast/component.cpp" added to CMakeLists.txt file, but if additional files will be added in the future for other components, this could lead to a sizable file structure. Therefore, a more scalable design approach might be worth considering.
5. New methods added to the Loader class but if they are not documented, it might become challenging for other developers to understand how to use these new features correctly and efficiently.

Commit c4d345cce5cccb7752922e91ea2796a24680490b

The pull request "[Component] share loading entry for component and module" primarily makes changes to the loader code of an existing software component. The key changes in the pull request can be summarized as follows:

1. Removed the loadModule function that previously returned a unique pointer to a Module. This was replaced by a modified loadModule that accepts a reference to a Module and returns Expect<void>. This change primarily helps in reducing unnecessary memory operations.

2. Introduced two new loading functions loadUniversalWASM and loadModuleAOT that take reference to a Module or AST::AOTSection respectively as parameters and return Expect<void>. These might be intended for cases where the WASM code to be loaded can come from different places (native WASM bytecode or an ahead-of-time (AOT) compiled WASM bytecode).

3. The code for loading the AOT and custom sections, which used to be part of the loadModule function, has been moved to the new loadModuleAOT function.

4. The logic for loading the WASM code and symbols was added to the loadUniversalWASM function.

5. Changes were made to the component loading logic to differentiate between loading a WASM Module and loading a Component.

Also, the pull request results in a total reduction of 6 lines of code, suggesting refactoring and possible optimization.

Potential Problems:

1. There are error messages which notify the developer when the function falls back to interpreter mode instead of AOT. However, they might need more context for better understanding by developers.

2. The functionality of the new loadModuleAOT and loadUniversalWASM methods depends on the error handling and fallback mechanism, as indicated by error messages in logging. It's necessary to test the robustness of these mechanisms.

3. Some error messages are logged without being passed up the call stack. Functions return unexpect(Res) which might not contain the detailed error information. This could make debugging more difficult in case of failures.

4. The Component loading logic now errors out with "Component model is not fully parsed yet!" which indicates the feature might not be complete, and could potentially lead to issues if used.

5. Some of the methods have large cognitive complexity due to many nested conditions and loops. Further refactoring may be needed to ensure maintainability and readability of the code.

6. There are no added test cases to validate the new functionality.

7. The pull request lacks a detailed description, which might make code review and understanding the context of these changes difficult.

Commit 20852ae3b3263b5705debb61f99d0a5c48ab4cd2

The key change in this patch that is titled "[PATCH 3/8] the missing validate for prototype" is in the file lib/validator/validator.cpp. The change is within the Validator namespace and involves the modification of the prototype for a function named validate.

The original line read as follows:

Expect<void> validate(const AST::Component &) { return {}; }

The modified line reads as follows:

Expect<void> Validator::validate(const AST::Component &) { return {}; }

The main change here is that the validate function is now clearly within the scope of Validator class, which was implicitly the case in the earlier version.

Potential Issues:

1. Using class scoping implicitly could lead to issues in larger complex projects where there may be function name clashes. It is not an issue in this pull request, but the developer should make it a practice to be explicit in function scopes.
2. There is no context or comments provided about what the validate function does. Future refactorings or bug fixes could be difficult without this documentation. It's also unclear what Expect<void> does and why the function is expected to return an empty object {}.
3. It seems that the change has wider implications on other modules that use this function, but the PR has only updated a single usage of this function. The maintainers should ensure that this change does not break the application elsewhere. Additionally, it is not clear whether this change has been properly tested.
4. The commit doesn't describe why this change is made, and why it is necessary which makes the review process harder for the reviewer and understanding the impact of changes difficult.

Commit ac218b23eb738a7458fe48963cdd28de03ba59e7

The changes in this pull request primarily involve replacing the "loadModule" method with the "loadWasmUnit" method in several places in the "vm.cpp" file. The replaced methods are part of conditional statements that check if the resources are correctly loaded. After replacing "loadModule" with "loadWasmUnit", the author also adds a new condition to check if the loaded resource is an instance of the AST::Module class. If the condition is true, the code continues the previously established flow; however, if it's false, it logs an error ("component executable is not done yet" or "component load is not done yet") and returns an unexpected result.

Key Changes:

1. Replaced the method used for loading a Wasm unit, from "parseModule" to "parseWasmUnit".
2. Added conditional statements to check if the loaded resource is an instance of AST::Module.
3. Added error logging messages if the loaded resource is not an instance of the AST::Module.

Potential Problems:

1. There are no checks for the existence or validity of the files being loaded. A missing or invalid file would likely result in a runtime error.
2. If the "parseWasmUnit" method does not return an AST::Module, it just logs an error and moves on. However, it doesn't handle the error or provide any alternative execution pathway. This could result in unexpected behavior or unhandled exceptions.
3. Testing is not mentioned or included in this pull request. Given the nature of the changes, tests would be necessary to ensure proper operation. Without them, we risk introducing breaking changes.
4. Detailed error messaging and handling is missing. Simply logging that "component executable is not done yet" or "component load is not done yet" may not be sufficient for troubleshooting potential issues.
5. Thread-safety may be a concern. If these methods are accessed by multiple threads, synchronization issues might occur, leading to faulty behavior.

Commit 536cd4c0be31c82e75dfd8aef3328c25ea0a3d9a

The patch essentially moves some AOT (Ahead-Of-Time) related methods from component.cpp to module.cpp. It could be for better code organization or bigger design decision. The two methods are Loader::loadUniversalWASM() and Loader::loadModuleAOT(), both functioning as loaders for AOT sections.

Even though there is no modification in the methods' implementation, this change could have potential impacts on the codebase.

1. Other components relying on these methods being in component.cpp would need to be updated. Direct or indirect usage of these methods from component.cpp should be checked and updated accordingly.

2. Existing unit tests (if any) verifying these methods might be impacted. The owning source file changed from component.cpp to module.cpp. It's important to ensure unit tests are updated too, so they still cover these methods.

3. If the software development process includes any form of build optimization or the Input and Output (I/O) operations rely on the structure of the project, those aspects might require a re-configuration.

In terms of code quality, the patch looks fine. Undocumented public methods or classes may negatively impact maintainability. To help others understand the functionality more easily, it would be better if comments describing the methods were also included. The logging messages in English are good for troubleshooting, but if the software is meant to support multiple languages, consider adding support for localized error messages. Finally, it is also crucial to ensure that the changed files still adhere to the overall coding style and standards as the rest of your codebase.

Commit 65bcec5556372e35c059a2180595bd4d34effd20

The changes made introduces setup to run WebAssembly(WASM) files with components, alongside the existing method to run with modules. This would allow a shared entry point for running files using components and modules. Three main changes have been made:

1. Added method overload unsafeRunWasmFile() in vm.h to handle Component type objects in addition to already handled Module type.

2. Revised Validator class in validator.cpp to add an error log and return a RuntimeError value when validation for a Component is attempted. This shows validation for components is not currently implemented.

3. Updated unsafeRunWasmFile() methods in vm.cpp to handle Component type objects and added a new unsafeRunWasmFile() method specifically for Component type objects.

This update could cause potential issues:

1. The error message in the validate() function in validator.cpp indicates that validation for components is not implemented. This could result in runtime errors as the expectation is to handle already validated code, and since there is no provision for that validation.

2. The added unsafeRunWasmFiles() method for Component type objects is not completely implemented, and will always result in a RuntimeError. This could potentially mislead users into believing that the functionality is currently operational.

3. It's hard to see if the changes are compatible with the rest of the system as there's no unit tests included with the patch. Tests are vitally important for ensuring correct functionality without breaking existing code. Lack of tests increases the risk of introducing bugs.

Given that some of the implementation has not been completed yet and the lack of testing, it's not advisable to merge this pull request without further development and testing.

Commit 3d1eed909671bd560aa99eacb26eaae491609c2f

This patch includes changes to the lib/vm/vm.cpp file. The changes mainly aim to avoid variable shadowing in two methods VM::unsafeRunWasmFile. The methods take either a filesystem path or a binary code, a function's name, parameters, and their types, and use them to run a WebAssembly (Wasm) module or component.

More particularly, the changes ensure that instead of declaring a new variable to store the parsed Wasm unit and then use that variable to call unsafeRunWasmFile, the results of the std::get function are passed directly to the unsafeRunWasmFile function. This happens for both AST::Module and AST::Component.

In terms of potential problems, the patch seems to be quite straightforward and doesn't introduce any complex changes that could lead to issues. However, one possible concern could be the removing of intermediate variables can make debugging slightly more challenging, as it's now less straightforward to examine the exact value of the module or component after parsing.

Considerations might be made concerning error handling; more specifically, the case when parsing a Wasm unit fails. Although the Unexpect function is called, it's not clear how the error situations are handled from this patch. The developer might need to ensure that sufficient and clear error messages are provided, and appropriate action is taken when an error occurs during the Wasm unit parsing. Furthermore, the adjustment to "shadow removing" refactoring might cause confusion to other developers if there's no clear note or necessity.

[codecov]

Codecov Report

Merging #2945 (18cc6ee) into master (f7931f2) will decrease coverage by 0.14%.
The diff coverage is 65.58%.

@@            Coverage Diff             @@
##           master    #2945      +/-   ##
==========================================
- Coverage   80.89%   80.75%   -0.14%     
==========================================
  Files         150      151       +1     
  Lines       21638    21711      +73     
  Branches     4354     4394      +40     
==========================================
+ Hits        17504    17533      +29     
- Misses       2965     2999      +34     
- Partials     1169     1179      +10     

Files	Coverage Δ
include/loader/loader.h	97.26% <ø> (ø)
include/validator/validator.h	100.00% <ø> (ø)
include/vm/vm.h	80.24% <ø> (ø)
include/ast/module.h	91.89% <0.00%> (-8.11%)	⬇️
lib/validator/validator.cpp	91.78% <0.00%> (-0.79%)	⬇️
lib/loader/loader.cpp	67.78% <76.31%> (+0.30%)	⬆️
lib/loader/ast/component.cpp	70.73% <70.73%> (ø)
lib/vm/vm.cpp	78.49% <34.48%> (-5.94%)	⬇️
lib/loader/ast/module.cpp	84.18% <72.27%> (-0.99%)	⬇️

📣 Codecov offers a browser extension for seamless coverage viewing on GitHub. Try it in Chrome or Firefox today!

[hydai]

@dannypsnl Please rebase to the latest master commit.