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

fix: 修复load .wasm文件对importObject的处理 #1705

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

YufJi
Copy link

@YufJi YufJi commented Nov 27, 2024

close #1422

Summary by CodeRabbit

  • 新功能
    • 添加了对 WebAssembly 模块的支持,允许应用程序使用新的减法功能。
    • 新增 load 方法,增强了 WasmRuntimePlugin 的功能,支持动态解析和生成 JavaScript 导入语句。
    • 新增 greetgreet2 函数,提供额外的功能接口。
  • 文档
    • 新增 TypeScript 声明文件,定义了多个导出函数及其参数类型。
  • 其他
    • 添加了新的依赖项 wasmparser

Copy link
Contributor

coderabbitai bot commented Nov 27, 2024

Walkthrough

该拉取请求引入了对Cargo.toml文件的一个新依赖wasmparser,版本为"0.207.0"。此外,在wasm_runtime.rs文件中,为WasmRuntimePlugin实现添加了一个新方法load,用于处理WebAssembly文件并生成相应的JavaScript代码。还新增了多个TypeScript声明和JavaScript文件,以支持新的WebAssembly模块和相关功能的集成。

Changes

文件路径 更改摘要
crates/mako/Cargo.toml 添加新依赖:wasmparser = "0.207.0"
crates/mako/src/plugins/wasm_runtime.rs 添加方法:fn load(&self, param: &PluginLoadParam, _context: &Arc<Context>) -> anyhow::Result<Option<Content>>
examples/import-resources/index.tsx 新增导入:import * as wasm from './minus-wasm-pack';,集成wasm.minus功能
examples/import-resources/minus-wasm-pack/index.d.ts 新增导出函数:greet, greet2, minus
examples/import-resources/minus-wasm-pack/index.js 引入WASM资源,调用__wbg_set_wasm函数,导出所有实体
examples/import-resources/minus-wasm-pack/index_bg.js 定义WASM集成层,提供多种函数以支持WASM与JavaScript的交互
examples/import-resources/minus-wasm-pack/index_bg.wasm.d.ts 定义WASM模块的TypeScript声明,包含memory常量及多个导出函数

Assessment against linked issues

Objective Addressed Explanation
支持WebAssembly模块的导入和使用 (#1422)

Possibly related PRs

Suggested reviewers

  • sorrycc

Poem

🐰 在月光下,我欢快跳跃,
新依赖如星星般闪耀。
WebAssembly的世界如此广阔,
减法功能,轻松搞定,
代码如花,绽放芬芳,
兔子欢唱,乐在其中! 🌟


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 7

🧹 Outside diff range and nitpick comments (14)
examples/import-resources/minus-wasm-pack/index.js (1)

1-3: 建议添加错误处理机制

当前的WebAssembly初始化过程没有错误处理机制。建议添加try-catch块来处理可能的初始化失败情况。

建议修改如下:

 import { __wbg_set_wasm } from './index_bg.js';
 import * as wasm from './index_bg.wasm';
-__wbg_set_wasm(wasm);
+try {
+  __wbg_set_wasm(wasm);
+} catch (error) {
+  console.error('WebAssembly初始化失败:', error);
+  throw error;
+}
examples/import-resources/minus-wasm-pack/index.d.ts (4)

1-2: 代码规范配置需要明确原因

建议在注释中说明禁用 lint 规则的具体原因,这样可以帮助其他开发者理解为什么需要禁用这些规则。

-/* tslint:disable */
-/* eslint-disable */
+/* tslint:disable -- 由于 WebAssembly 生成的代码可能不符合规范要求 */
+/* eslint-disable -- 由于 WebAssembly 生成的代码可能不符合规范要求 */

3-5: greet 函数缺少功能说明文档

函数的 JSDoc 注释是空的,建议添加函数的用途说明。

 /**
+ * 显示问候信息
  */

6-9: greet2 函数文档需要补充参数说明

JSDoc 注释中虽然标注了参数类型,但缺少参数用途的说明。

 /**
+ * 显示带有指定名称的问候信息
  * @param {string} name
+ * @param {string} name 要显示的用户名称
  */

10-15: minus 函数文档需要完善

建议补充函数的具体用途说明和返回值的含义。

 /**
+ * 计算两个数字的差值
  * @param {number} a
+ * @param {number} a 被减数
  * @param {number} b
+ * @param {number} b 减数
  * @returns {number}
+ * @returns {number} 返回 a - b 的结果
  */
examples/import-resources/minus-wasm-pack/index_bg.wasm.d.ts (3)

1-2: 建议添加文件级别的 JSDoc 注释

为了提高代码的可维护性,建议在禁用 lint 规则的同时添加文件说明,解释这个 WebAssembly 模块的用途。

 /* tslint:disable */
 /* eslint-disable */
+/**
+ * @file WebAssembly 模块类型声明
+ * @description 该文件定义了 WebAssembly 模块导出的内存和函数接口
+ */

4-6: 建议为公共函数添加 JSDoc 文档

公共函数缺少参数说明和使用示例,这对使用者不够友好。

+/**
+ * @param a 第一个数字
+ * @param b 第二个数字
+ */
 export function greet2(a: number, b: number): void;
+/**
+ * 计算两个数字的差值
+ * @param a 被减数
+ * @param b 减数
+ * @returns 差值
+ */
 export function minus(a: number, b: number): number;
+/**
+ * 打招呼函数
+ */
 export function greet(): void;

7-13: 注意内存分配函数的使用安全性

__wbindgen_malloc__wbindgen_realloc 是关键的内存管理函数,建议:

  1. 添加警告注释,说明这些是内部使用的函数
  2. 确保这些函数不会被外部直接调用

建议添加以下注释:

+/**
+ * @internal
+ * 仅供 wasm-bindgen 内部使用的内存分配函数
+ * @private
+ */
 export function __wbindgen_malloc(a: number, b: number): number;
+/**
+ * @internal
+ * 仅供 wasm-bindgen 内部使用的内存重分配函数
+ * @private
+ */
 export function __wbindgen_realloc(
   a: number,
   b: number,
   c: number,
   d: number,
 ): number;
examples/import-resources/index.tsx (1)

10-10: WebAssembly 模块导入方式建议优化

建议使用具名导入来明确导入的具体函数,这样可以减小打包体积并提高代码可读性。

建议修改为:

-import * as wasm from './minus-wasm-pack';
+import { minus } from './minus-wasm-pack';
examples/import-resources/minus-wasm-pack/index_bg.js (2)

8-9: 建议修改模块加载方式

使用 (0, module.require)('util').TextDecoder 可能会导致兼容性问题。建议直接使用 require('util').TextDecoder,以提高代码的可读性和可靠性。


16-16: 移除不必要的空解码操作

cachedTextDecoder.decode(); 没有传入参数,可能没有实际作用。建议移除这行代码,以简化代码。

crates/mako/src/plugins/wasm_runtime.rs (3)

42-42: 建议改进文件扩展名的检查方式

在第42行,使用file.path.to_string_lossy().ends_with(".wasm")来判断文件是否为.wasm文件,这种方式可能不够严谨。建议使用extension方法获取文件扩展名,以避免可能的误判。

建议修改如下:

- if file.path.to_string_lossy().ends_with(".wasm") {
+ if file.path.extension().and_then(|ext| ext.to_str()) == Some("wasm") {

60-66: 简化import_objs_map的更新逻辑

在处理导入对象映射时,可以使用Entry API 来简化代码,提高代码的可读性和效率。

建议修改如下:

- if let Some(import_obj) = import_objs_map.get_mut(module) {
-     import_obj.push(name.to_string());
- } else {
-     import_objs_map.insert(module, vec![name.to_string()]);
- }
+ import_objs_map.entry(module)
+     .or_insert_with(Vec::new)
+     .push(name.to_string());

54-73: 考虑处理其他类型的Payload

目前仅处理了Payload::ImportSection,可能会遗漏其他重要的段信息。建议考虑处理更多类型的Payload,以增强对WebAssembly文件的支持。

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between cb5bc5f and 8daa5bb.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • examples/import-resources/minus-wasm-pack/index_bg.wasm is excluded by !**/*.wasm
📒 Files selected for processing (7)
  • crates/mako/Cargo.toml (1 hunks)
  • crates/mako/src/plugins/wasm_runtime.rs (2 hunks)
  • examples/import-resources/index.tsx (2 hunks)
  • examples/import-resources/minus-wasm-pack/index.d.ts (1 hunks)
  • examples/import-resources/minus-wasm-pack/index.js (1 hunks)
  • examples/import-resources/minus-wasm-pack/index_bg.js (1 hunks)
  • examples/import-resources/minus-wasm-pack/index_bg.wasm.d.ts (1 hunks)
🔇 Additional comments (6)
examples/import-resources/minus-wasm-pack/index.js (2)

1-3: 导入顺序和初始化逻辑正确!

WebAssembly模块的导入和初始化顺序符合最佳实践。


4-4: 请确认导出策略的安全性

使用export *可能会暴露一些不应该公开的内部实现细节。建议明确列出需要导出的函数和变量。

运行以下脚本来检查index_bg.js中导出的所有内容:

✅ Verification successful

export * 导出策略在此场景中是安全的

经过分析,index_bg.js 中导出的内容都是 WebAssembly 绑定所必需的:

  • 公共 API 函数:greet()greet2(name)minus(a, b)
  • WebAssembly 内部绑定函数:__wbg_set_wasm__wbg_alert_*__wbg_prompt_*

这些导出都是由 wasm-bindgen 工具自动生成的标准绑定代码,没有暴露任何不应该公开的实现细节。使用 export * 在这种自动生成的绑定代码场景下是合适且安全的。

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 描述:检查index_bg.js中的所有导出内容

# 搜索所有export语句
rg "export" "examples/import-resources/minus-wasm-pack/index_bg.js"

Length of output: 309


Script:

#!/bin/bash
# 检查index_bg.js中导出函数的具体实现
ast-grep --pattern 'export function $name($$$) { $$$ }'

Length of output: 150


Script:

#!/bin/bash
# 检查index_bg.js文件的完整内容
cat examples/import-resources/minus-wasm-pack/index_bg.js

Length of output: 2971

examples/import-resources/minus-wasm-pack/index.d.ts (1)

1-15: 验证 WebAssembly 模块导出函数的完整性

需要确保声明文件中的函数与 WebAssembly 模块导出的函数完全匹配。

✅ Verification successful

Let me gather more information about the implementation of these functions to verify their completeness.


WebAssembly 模块导出函数验证完成

根据 index_bg.js 中的实现,我们可以确认声明文件中的三个函数 greet()greet2(name)minus(a, b) 都有对应的 WebAssembly 绑定实现:

  • greet() 直接调用 wasm.greet()
  • greet2(name) 通过字符串传递机制调用底层 WebAssembly 函数
  • minus(a, b) 调用 wasm.minus() 并返回计算结果

声明文件中的类型定义与实际实现完全匹配。

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 描述:验证 .wasm 文件导出的函数与声明文件是否匹配

# 使用 wasmparser 检查 .wasm 文件的导出函数
fd -e wasm . examples/import-resources/minus-wasm-pack/ -x wasmparser {} | grep -A 5 "export"

# 检查相关的 JavaScript 胶水代码
rg -t js "export function (greet|greet2|minus)" examples/import-resources/minus-wasm-pack/

Length of output: 479


Script:

#!/bin/bash
# 检查 index_bg.js 中的函数实现
rg -A 5 "export function (greet|greet2|minus)" examples/import-resources/minus-wasm-pack/index_bg.js

# 检查是否有其他相关的实现文件
fd . examples/import-resources/minus-wasm-pack/ -x file {}

# 查找可能的 Rust 源代码
fd -e rs . examples/import-resources/minus-wasm-pack/

Length of output: 753

examples/import-resources/minus-wasm-pack/index_bg.wasm.d.ts (1)

3-3: 建议添加内存配置说明

WebAssembly.Memory 的配置对性能有重要影响。建议添加注释说明内存的初始大小和最大大小限制。

运行以下脚本检查相关的 Rust 代码中的内存配置:

examples/import-resources/index.tsx (1)

36-39: 🛠️ Refactor suggestion

验证 WASM 函数调用的错误处理

当前代码直接调用 wasm.minus 函数但没有进行错误处理。WebAssembly 函数可能会抛出异常,建议添加错误处理逻辑。

建议修改为:

       <h2>
-        Test import .wasm file(generated by wasm-pack) async: {num1} - {num2} ={' '}
-        {wasm.minus(num1, num2)}
+        Test import .wasm file(generated by wasm-pack) async: {num1} - {num2} = {' '}
+        {(() => {
+          try {
+            return wasm.minus(num1, num2);
+          } catch (error) {
+            console.error('WASM 减法运算失败:', error);
+            return '计算出错';
+          }
+        })()}
       </h2>

让我验证一下这个 WASM 函数的实现:

examples/import-resources/minus-wasm-pack/index_bg.js (1)

127-133: 验证直接调用浏览器函数的安全性

直接调用 alertprompt 等浏览器函数可能存在安全风险。建议验证这些函数的使用是否符合安全最佳实践,防止可能的 XSS 攻击或恶意代码执行。

@@ -27,6 +27,7 @@ percent-encoding = { version = "2.3.1" }
serde = { workspace = true }
serde_json = { workspace = true }
url = { version = "2.5.0" }
wasmparser = "0.207.0"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

建议更新 wasmparser 依赖版本

当前使用的版本 (0.207.0) 已过时。最新的稳定版本是 0.220.0,建议:

  1. 更新到最新版本
  2. 使用版本范围声明以自动获取补丁更新:wasmparser = "^0.220.0"

安全检查显示该包没有已知的安全漏洞。

🔗 Analysis chain

建议检查 wasmparser 的版本选择

建议考虑以下几点:

  1. 使用版本范围声明(例如 ^0.207.0)来自动接收补丁版本的更新
  2. 确认当前选择的版本是否为最新的稳定版本

运行以下脚本来验证版本信息:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 检查 wasmparser 包的最新版本和安全公告

# 从 crates.io 获取最新版本信息
curl -s https://crates.io/api/v1/crates/wasmparser | jq -r '.crate.max_stable_version'

# 检查安全公告
gh api graphql -f query='
{
  securityVulnerabilities(first: 5, ecosystem: RUST, package: "wasmparser") {
    nodes {
      advisory {
        summary
        severity
        publishedAt
      }
      vulnerableVersionRange
      firstPatchedVersion {
        identifier
      }
    }
  }
}'

Length of output: 439

Comment on lines +21 to +28
if (
cachedUint8ArrayMemory0 === null ||
cachedUint8ArrayMemory0.byteLength === 0
) {
cachedUint8ArrayMemory0 = new Uint8Array(wasm.memory.buffer);
}
return cachedUint8ArrayMemory0;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

确保内存视图在内存增长时更新

getUint8ArrayMemory0() 中,缓存的 cachedUint8ArrayMemory0 可能在 WebAssembly 内存增长后失效。建议在检测到内存增长时,重新获取内存视图。

Comment on lines +65 to +103
function passStringToWasm0(arg, malloc, realloc) {
if (realloc === undefined) {
const buf = cachedTextEncoder.encode(arg);
const ptr = malloc(buf.length, 1) >>> 0;
getUint8ArrayMemory0()
.subarray(ptr, ptr + buf.length)
.set(buf);
WASM_VECTOR_LEN = buf.length;
return ptr;
}

let len = arg.length;
let ptr = malloc(len, 1) >>> 0;

const mem = getUint8ArrayMemory0();

let offset = 0;

for (; offset < len; offset++) {
const code = arg.charCodeAt(offset);
if (code > 0x7f) break;
mem[ptr + offset] = code;
}

if (offset !== len) {
if (offset !== 0) {
arg = arg.slice(offset);
}
ptr = realloc(ptr, len, (len = offset + arg.length * 3), 1) >>> 0;
const view = getUint8ArrayMemory0().subarray(ptr + offset, ptr + len);
const ret = encodeString(arg, view);

offset += ret.written;
ptr = realloc(ptr, len, offset, 1) >>> 0;
}

WASM_VECTOR_LEN = offset;
return ptr;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

优化字符串传递到 WASM 的实现

passStringToWasm0() 函数较为复杂,可能存在对多字节字符处理的不稳定性。建议使用 TextEncoder 的 encode() 方法简化字符串编码过程,并重新评估内存分配和重分配逻辑。

Comment on lines +94 to +98
const view = getUint8ArrayMemory0().subarray(ptr + offset, ptr + len);
const ret = encodeString(arg, view);

offset += ret.written;
ptr = realloc(ptr, len, offset, 1) >>> 0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

更新内存指针以防止访问错误

在内存重分配后,ptr 被更新,但 memview 仍指向旧的内存区域。应在重分配后重新获取内存视图和缓冲区,以避免可能的内存访问错误。

Comment on lines 68 to 69
println!("import error");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

改进错误处理,提供详细的错误信息

当前在解析导入时,如果发生错误,仅打印了"import error",这可能不足以帮助调试。建议在错误信息中包含具体的错误详情。

建议修改如下:

- println!("import error");
+ eprintln!("Import parsing error: {:?}", e);

Committable suggestion skipped: line range outside the PR's diff.

Comment on lines +101 to +106
"module.exports = require._interopreRequireWasm(exports, \"{}\")",
final_file_name
));
} else {
content.push_str(&format!(
"module.exports = require._interopreRequireWasm(exports, \"{}\", {{{}}})",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

检查require._interopreRequireWasm函数名是否正确

在生成的JavaScript代码中,使用了require._interopreRequireWasm函数。请确认函数名是否拼写正确,是否应为require._interopRequireWasm

如果存在拼写错误,建议修改如下:

- "module.exports = require._interopreRequireWasm(exports, \"{}\")",
+ "module.exports = require._interopRequireWasm(exports, \"{}\")",

Committable suggestion skipped: line range outside the PR's diff.


💡 Codebase verification

函数名称拼写错误

wasm_runtime.js中,函数名为requireModule._interopreRequireWasm,但在生成的代码中使用了require._interopreRequireWasm。此外,函数名中存在拼写错误:_interopreRequireWasm应为_interopRequireWasminterop而不是interopre)。

  • crates/mako/src/plugins/wasm_runtime.rs:需要修正函数调用名称为requireModule._interopRequireWasm
  • crates/mako/src/plugins/wasm_runtime/wasm_runtime.js:需要修正函数定义名称中的拼写错误
🔗 Analysis chain

验证require._interopRequireWasm函数的定义

请确认require._interopRequireWasm函数在代码库中已正确定义,以确保生成的JavaScript代码能够正常运行。

运行以下脚本以检查该函数的定义:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# 描述:在代码库中搜索`_interopRequireWasm`函数的定义

# 测试:在所有JavaScript文件中查找`_interopRequireWasm`的定义,期望能够找到函数定义的位置。
rg '_interopRequireWasm' --type js

Length of output: 34


Script:

#!/bin/bash
# 扩大搜索范围,不限制文件类型
rg -l "_interopreRequireWasm"

# 搜索可能的runtime或helper文件
fd -e js -e ts "runtime|helper" --type f

# 检查项目根目录下的配置文件,查找相关依赖或配置
fd "package.json|webpack|rollup|vite" --max-depth 2 --type f --exec cat {}

Length of output: 3456


Script:

#!/bin/bash
# 检查wasm_runtime.js文件中的_interopreRequireWasm函数定义
cat crates/mako/src/plugins/wasm_runtime/wasm_runtime.js

# 检查其他可能引用此函数的文件内容
cat crates/mako/src/build/load.rs

Length of output: 12053

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (3)
crates/mako/src/plugins/wasm_runtime.rs (3)

55-68: WASM导入解析逻辑实现合理

代码使用wasmparser库来解析WASM文件的导入部分,实现逻辑清晰。建议添加错误日志,以便调试问题。

 if let Ok(Import { module, name, ty }) = import {
+    log::debug!("处理WASM导入: module={}, name={}", module, name);
     if let Some(import_obj) = import_objs_map.get_mut(module) {

70-89: 建议优化字符串处理效率

当前实现中的字符串拼接可以通过使用String的预分配来优化性能。

- let mut js_imports_str = String::new();
- let mut import_objs_str = String::new();
+ let mut js_imports_str = String::with_capacity(import_objs_map.len() * 50);
+ let mut import_objs_str = String::with_capacity(import_objs_map.len() * 100);

106-110: 建议添加类型注释

为了提高代码可读性,建议为生成的JavaScript内容添加类型注释。

 return Ok(Some(Content::Js(JsContent {
     content,
+    // 生成的JavaScript代码包含WASM模块的导入和导出
     ..Default::default()
 })));
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 8daa5bb and 74048e2.

📒 Files selected for processing (1)
  • crates/mako/src/plugins/wasm_runtime.rs (2 hunks)
🔇 Additional comments (2)
crates/mako/src/plugins/wasm_runtime.rs (2)

1-11: 导入声明看起来不错!

代码引入了适当的依赖项来处理WebAssembly文件的解析和导入处理。使用wasmparser作为WASM解析器是个不错的选择。


94-104: ⚠️ Potential issue

函数名拼写错误

_interopreRequireWasm存在拼写错误,应该是_interopRequireWasm

- "module.exports = require._interopreRequireWasm(exports, \"{}\")",
+ "module.exports = require._interopRequireWasm(exports, \"{}\")",
- "module.exports = require._interopreRequireWasm(exports, \"{}\", {{{}}})",
+ "module.exports = require._interopRequireWasm(exports, \"{}\", {{{}}})",

Comment on lines 41 to 54
let file = param.file;
if file.path.to_string_lossy().ends_with(".wasm") {
let final_file_name = format!(
"{}.{}.{}",
file.get_file_stem(),
file.get_content_hash()?,
file.extname
);
let origin_path = file.pathname.to_string_lossy().to_string();
_context.emit_assets(origin_path, final_file_name.clone());

let mut buffer = Vec::new();
File::open(file.path.as_path())?.read_to_end(&mut buffer)?;
// Parse wasm file to get imports
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

建议改进错误处理和内存管理

当前实现存在以下需要注意的地方:

  1. 文件操作缺少详细的错误处理,建议添加具体的错误信息
  2. 将整个WASM文件读入内存可能会对大文件造成问题

建议修改如下:

- File::open(file.path.as_path())?.read_to_end(&mut buffer)?;
+ File::open(&file.path)
+     .map_err(|e| anyhow::anyhow!("无法打开WASM文件 {}: {}", file.path.display(), e))?
+     .read_to_end(&mut buffer)
+     .map_err(|e| anyhow::anyhow!("读取WASM文件失败: {}", e))?;

对于大文件,可以考虑使用流式处理或设置最大文件大小限制。

📝 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.

Suggested change
let file = param.file;
if file.path.to_string_lossy().ends_with(".wasm") {
let final_file_name = format!(
"{}.{}.{}",
file.get_file_stem(),
file.get_content_hash()?,
file.extname
);
let origin_path = file.pathname.to_string_lossy().to_string();
_context.emit_assets(origin_path, final_file_name.clone());
let mut buffer = Vec::new();
File::open(file.path.as_path())?.read_to_end(&mut buffer)?;
// Parse wasm file to get imports
let file = param.file;
if file.path.to_string_lossy().ends_with(".wasm") {
let final_file_name = format!(
"{}.{}.{}",
file.get_file_stem(),
file.get_content_hash()?,
file.extname
);
let origin_path = file.pathname.to_string_lossy().to_string();
_context.emit_assets(origin_path, final_file_name.clone());
let mut buffer = Vec::new();
File::open(&file.path)
.map_err(|e| anyhow::anyhow!("无法打开WASM文件 {}: {}", file.path.display(), e))?
.read_to_end(&mut buffer)
.map_err(|e| anyhow::anyhow!("读取WASM文件失败: {}", e))?;
// Parse wasm file to get imports

@YufJi
Copy link
Author

YufJi commented Nov 27, 2024

@sorrycc review

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
crates/mako/src/plugins/wasm_runtime.rs (1)

75-98: 优化字符串构建效率

当前代码中的字符串拼接可以通过使用 String 的预分配来优化性能。建议:

- let mut js_imports_str = String::new();
+ let mut js_imports_str = String::with_capacity(import_objs_map.len() * 50);
- let mut import_objs_str = String::new();
+ let mut import_objs_str = String::with_capacity(import_objs_map.len() * 100);

另外,考虑使用 join 来替代多次 push_str

let js_imports_str = import_objs_map
    .iter()
    .enumerate()
    .map(|(index, (key, _))| {
        format!(
            "import * as module{} from \"{}\";\n",
            index, key
        )
    })
    .collect::<String>();
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 74048e2 and 9400f58.

📒 Files selected for processing (1)
  • crates/mako/src/plugins/wasm_runtime.rs (2 hunks)
🔇 Additional comments (3)
crates/mako/src/plugins/wasm_runtime.rs (3)

1-11: 导入声明看起来不错!

导入的模块和类型都是必要的,并且组织得当。


41-54: 改进文件操作的错误处理和内存管理

当前的文件操作缺少详细的错误处理,且对大文件的处理可能存在内存问题。建议:

- File::open(&file.path)?.read_to_end(&mut buffer)?;
+ File::open(&file.path)
+     .map_err(|e| anyhow::anyhow!("无法打开WASM文件 {}: {}", file.path.display(), e))?
+     .read_to_end(&mut buffer)
+     .map_err(|e| anyhow::anyhow!("读取WASM文件失败: {}", e))?;

101-102: 修正函数名拼写错误

_interopreRequireWasm 应该修改为 _interopRequireWasm

Comment on lines 55 to 73
let mut import_objs_map: HashMap<&str, Vec<String>> = HashMap::new();
for payload in Parser::new(0).parse_all(&buffer) {
if let Ok(Payload::ImportSection(imports)) = payload {
for import in imports {
if let Ok(Import {
module,
name,
ty: _,
}) = import
{
if let Some(import_obj) = import_objs_map.get_mut(module) {
import_obj.push(name.to_string());
} else {
import_objs_map.insert(module, vec![name.to_string()]);
}
}
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

优化WASM导入解析的错误处理

当前代码在处理导入时的错误处理可以更加完善。建议:

- if let Ok(Import { module, name, ty: _ }) = import
+ match import {
+     Ok(Import { module, name, ty: _ }) => {
+         if let Some(import_obj) = import_objs_map.get_mut(module) {
+             import_obj.push(name.to_string());
+         } else {
+             import_objs_map.insert(module, vec![name.to_string()]);
+         }
+     },
+     Err(e) => log::warn!("解析WASM导入时出错: {}", e)
+ }
📝 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.

Suggested change
let mut import_objs_map: HashMap<&str, Vec<String>> = HashMap::new();
for payload in Parser::new(0).parse_all(&buffer) {
if let Ok(Payload::ImportSection(imports)) = payload {
for import in imports {
if let Ok(Import {
module,
name,
ty: _,
}) = import
{
if let Some(import_obj) = import_objs_map.get_mut(module) {
import_obj.push(name.to_string());
} else {
import_objs_map.insert(module, vec![name.to_string()]);
}
}
}
}
}
let mut import_objs_map: HashMap<&str, Vec<String>> = HashMap::new();
for payload in Parser::new(0).parse_all(&buffer) {
if let Ok(Payload::ImportSection(imports)) = payload {
for import in imports {
match import {
Ok(Import { module, name, ty: _ }) => {
if let Some(import_obj) = import_objs_map.get_mut(module) {
import_obj.push(name.to_string());
} else {
import_objs_map.insert(module, vec![name.to_string()]);
}
},
Err(e) => log::warn!("解析WASM导入时出错: {}", e)
}
}
}
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (2)
crates/mako/src/plugins/wasm_runtime.rs (2)

80-96: 建议优化字符串构建逻辑

当前的字符串构建方式使用了多个push_strformat!调用,可以考虑使用更高效的方式。

建议使用itertools来优化代码:

+ use itertools::Itertools;

- for (index, (key, value)) in wasm_import_object_map.iter().enumerate() {
-     module_import_code.push_str(&format!(/*...*/));
-     wasm_import_object_code.push_str(&format!(/*...*/));
- }
+ let (module_imports, object_entries): (Vec<_>, Vec<_>) = wasm_import_object_map
+     .iter()
+     .enumerate()
+     .map(|(idx, (key, value))| (
+         format!("import * as module{} from \"{}\";", idx, key),
+         format!("\"{}\": {{ {} }}", 
+             key,
+             value.iter()
+                 .map(|name| format!("\"{0}\": module{1}[\"{0}\"]", name, idx))
+                 .join(", ")
+         )
+     ))
+     .unzip();
+ 
+ let module_import_code = module_imports.join("\n");
+ let wasm_import_object_code = object_entries.join(", ");

57-75: 建议添加导入数量限制

当前实现没有限制单个WASM模块可以导入的函数数量,这可能导致内存问题。

建议添加合理的限制:

+ const MAX_IMPORTS_PER_MODULE: usize = 1000;

  let mut wasm_import_object_map: HashMap<&str, Vec<String>> = HashMap::new();
+ let mut total_imports = 0;

  for payload in Parser::new(0).parse_all(&buffer) {
      if let Ok(Payload::ImportSection(imports)) = payload {
+         if total_imports >= MAX_IMPORTS_PER_MODULE {
+             return Err(anyhow::anyhow!("导入数量超过限制"));
+         }
          for import in imports {
+             total_imports += 1;
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 9400f58 and 49b470e.

📒 Files selected for processing (1)
  • crates/mako/src/plugins/wasm_runtime.rs (2 hunks)
🔇 Additional comments (3)
crates/mako/src/plugins/wasm_runtime.rs (3)

1-11: 导入声明看起来不错!

所选用的依赖项都是合适的,特别是wasmparsercrate对于解析WebAssembly文件是一个很好的选择。


54-56: 建议改进错误处理

当前的错误处理过于简单,缺少具体的上下文信息,这可能会增加调试难度。

建议修改为:

- File::open(&file.path)?.read_to_end(&mut buffer)?;
+ File::open(&file.path)
+     .map_err(|e| anyhow::anyhow!("无法打开WASM文件 {}: {}", file.path.display(), e))?
+     .read_to_end(&mut buffer)
+     .map_err(|e| anyhow::anyhow!("读取WASM文件失败: {}", e))?;

103-110: 函数名称拼写错误

_interopreRequireWasm 应该是 _interopRequireWasm

建议修改:

- "module.exports = require._interopreRequireWasm(exports, \"{}\")"
+ "module.exports = require._interopRequireWasm(exports, \"{}\")"

Comment on lines 61 to 72
if let Ok(Import {
module,
name,
ty: _,
}) = import
{
if let Some(import_object) = wasm_import_object_map.get_mut(module) {
import_object.push(name.to_string());
} else {
wasm_import_object_map.insert(module, vec![name.to_string()]);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

建议添加导入名称的安全验证

当前代码直接使用了WASM模块中的导入名称,但没有进行任何验证,这可能存在安全风险。

建议添加名称验证:

+ fn is_valid_import_name(name: &str) -> bool {
+     // 只允许字母、数字、下划线和美元符号
+     name.chars().all(|c| c.is_alphanumeric() || c == '_' || c == '$')
+ }

  if let Ok(Import { module, name, ty: _ }) = import {
+     if !is_valid_import_name(name) {
+         log::warn!("跳过不安全的导入名称: {}", name);
+         continue;
+     }
      if let Some(import_object) = wasm_import_object_map.get_mut(module) {

Committable suggestion skipped: line range outside the PR's diff.

Copy link

codecov bot commented Nov 29, 2024

Codecov Report

Attention: Patch coverage is 10.41667% with 43 lines in your changes missing coverage. Please review.

Project coverage is 55.16%. Comparing base (b6f638d) to head (49b470e).
Report is 2 commits behind head on master.

Files with missing lines Patch % Lines
crates/mako/src/plugins/wasm_runtime.rs 10.41% 43 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1705      +/-   ##
==========================================
- Coverage   55.28%   55.16%   -0.13%     
==========================================
  Files         175      175              
  Lines       17699    17747      +48     
==========================================
+ Hits         9785     9790       +5     
- Misses       7914     7957      +43     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (2)
crates/mako/src/plugins/wasm_runtime.rs (2)

134-151: 改进测试用例的健壮性

当前的测试用例存在以下问题:

  1. 使用了硬编码的文件路径
  2. 缺少错误情况的测试
  3. 对生成内容的验证不够完整

建议添加以下测试场景:

  1. 使用测试资源目录和环境变量来管理测试文件
  2. 添加对无效WASM文件的测试
  3. 添加对生成的JavaScript代码结构的详细验证

示例改进:

#[test]
fn test_wasm_runtime_load_invalid_wasm() {
    let plugin = WasmRuntimePlugin {};
    let context = Arc::new(Context::default());
    let temp_dir = tempfile::tempdir().unwrap();
    let invalid_wasm_path = temp_dir.path().join("invalid.wasm");
    std::fs::write(&invalid_wasm_path, b"invalid wasm content").unwrap();
    
    let file = File::new(invalid_wasm_path.to_string_lossy().to_string(), context.clone());
    let param = PluginLoadParam { file: &file };
    let result = plugin.load(&param, &context);
    
    assert!(result.is_err());
}

35-124: 考虑重构为更模块化的结构

当前的实现将所有逻辑都放在了load方法中,建议将功能拆分为更小的模块:

  1. WASM文件处理
  2. 导入对象解析
  3. JavaScript代码生成

建议创建以下模块结构:

mod wasm {
    pub struct WasmFile {
        buffer: Vec<u8>,
        imports: HashMap<String, Vec<String>>,
    }
    
    impl WasmFile {
        pub fn load(path: &Path) -> Result<Self>;
        pub fn parse_imports(&mut self) -> Result<()>;
    }
}

mod js {
    pub struct CodeGenerator {
        imports: HashMap<String, Vec<String>>,
    }
    
    impl CodeGenerator {
        pub fn generate_module_imports(&self) -> String;
        pub fn generate_export_statement(&self, filename: &str) -> String;
    }
}

这样的结构将:

  1. 提高代码的可维护性
  2. 便于单元测试
  3. 使功能边界更清晰
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between d0aea67 and 1a31062.

📒 Files selected for processing (1)
  • crates/mako/src/plugins/wasm_runtime.rs (2 hunks)
🔇 Additional comments (3)
crates/mako/src/plugins/wasm_runtime.rs (3)

106-107: ⚠️ Potential issue

修复函数名拼写错误

函数名_interopreRequireWasm存在拼写错误,应为_interopRequireWasm

建议修改如下:

- "module.exports = require._interopreRequireWasm(exports, \"{}\")",
+ "module.exports = require._interopRequireWasm(exports, \"{}\")",

54-55: 🛠️ Refactor suggestion

改进文件读取的错误处理和内存管理

当前的文件读取实现存在以下问题:

  1. 错误处理不够详细,无法快速定位问题
  2. 对大型WASM文件可能会造成内存压力

建议修改如下:

- let mut buffer = Vec::new();
- File::open(&file.path)?.read_to_end(&mut buffer)?;
+ let mut buffer = Vec::with_capacity(1024 * 1024); // 预分配1MB
+ File::open(&file.path)
+     .map_err(|e| anyhow::anyhow!("无法打开WASM文件 {}: {}", file.path.display(), e))?
+     .take(10 * 1024 * 1024) // 限制最大读取10MB
+     .read_to_end(&mut buffer)
+     .map_err(|e| anyhow::anyhow!("读取WASM文件失败: {}", e))?;

Likely invalid or redundant comment.


58-78: 🛠️ Refactor suggestion

优化WASM导入解析的错误处理和日志记录

当前的导入解析缺少适当的错误处理和日志记录,这可能会导致调试困难。

建议修改如下:

 Parser::new(0).parse_all(&buffer).for_each(|payload| {
-    if let Ok(Payload::ImportSection(imports)) = payload {
+    match payload {
+        Ok(Payload::ImportSection(imports)) => {
         imports.into_iter_with_offsets().for_each(|import| {
-            if let Ok((
-                _,
-                Import {
-                    module,
-                    name,
-                    ty: _,
-                },
-            )) = import
-            {
+            match import {
+                Ok((_, Import { module, name, ty })) => {
+                    log::debug!("处理WASM导入: module={}, name={}", module, name);
                 if let Some(import_object) = wasm_import_object_map.get_mut(module) {
                     import_object.push(name.to_string());
                 } else {
                     wasm_import_object_map.insert(module, vec![name.to_string()]);
                 }
+                }
+                Err(e) => log::warn!("解析WASM导入时出错: {}", e)
             }
         });
+        }
+        Ok(_) => (),
+        Err(e) => log::error!("解析WASM文件时出错: {}", e)
     }
 });

Likely invalid or redundant comment.

@Jinbao1001 Jinbao1001 requested a review from xusd320 December 4, 2024 07:46
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.

不支持 wasm?
1 participant