feat: 新增 Rust binding by AI #891
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's Guide添加一个实验性的、由 AI 生成的 MaaFramework Rust 绑定 crate，包括动态 FFI 层、安全的高级抽象（resource/controller/tasker/context/toolkit）、任务与通知模型、自定义动作/识别钩子、示例用法，以及更新文档以引用 Rust 集成并将其标记为实验性功能。 使用新绑定执行 Rust 任务的时序图sequenceDiagram
%% Flow: Rust app uses maa-framework crate to run a task on a device
actor User
participant App as RustApp
participant Crate as MaaFrameworkCrate
participant Toolkit as ToolkitModule
participant Ctrl as AdbController
participant Res as Resource
participant Tasker as Tasker
participant FFI as ffi_rs
participant CApi as Maa_C_API
User->>App: run()
App->>Crate: load_library(path_to_framework)
Crate->>FFI: load_symbol MaaVersion
FFI->>CApi: dlopen + dlsym
App->>Crate: load_toolkit(path_to_toolkit)
Crate->>FFI: load_symbol MaaToolkitAdbDeviceListCreate
FFI->>CApi: dlopen + dlsym
App->>Toolkit: find_adb_devices()
Toolkit->>FFI: maa_toolkit_adb_device_list_create
FFI->>CApi: MaaToolkitAdbDeviceListCreate
Toolkit->>FFI: maa_toolkit_adb_device_find
FFI->>CApi: MaaToolkitAdbDeviceFind
Toolkit-->>App: Vec<AdbDevice>
App->>Ctrl: AdbController::new(device_info)
Ctrl->>FFI: maa_adb_controller_create
FFI->>CApi: MaaAdbControllerCreate
Ctrl-->>App: AdbController
App->>Res: Resource::new()
Res->>FFI: maa_resource_create
FFI->>CApi: MaaResourceCreate
Res-->>App: Resource
App->>Tasker: Tasker::new()
Tasker->>FFI: maa_tasker_create
FFI->>CApi: MaaTaskerCreate
Tasker-->>App: Tasker
App->>Tasker: bind(resource, controller)
Tasker->>FFI: maa_tasker_bind_resource
FFI->>CApi: MaaTaskerBindResource
Tasker->>FFI: maa_tasker_bind_controller
FFI->>CApi: MaaTaskerBindController
App->>Tasker: post_task("StartUp", override_json)
Tasker->>FFI: maa_tasker_post_task
FFI->>CApi: MaaTaskerPostTask
Tasker-->>App: TaskJob
App->>Tasker: job.wait()
Tasker->>FFI: maa_tasker_wait
FFI->>CApi: MaaTaskerWait
Tasker-->>App: Status::Succeeded/Failed
App-->>User: print result and task detail
缓冲区、工具集、通知和自定义钩子的支持类图classDiagram
%% Supporting types: buffers, toolkit, notifications, custom hooks, errors
class StringBuffer {
- MaaStringBufferHandle handle
- bool own
+ new() StringBuffer
+ from_handle(handle MaaStringBufferHandle) StringBuffer
+ handle() MaaStringBufferHandle
+ into_raw() MaaStringBufferHandle
+ get() Result~String~
+ set(value &str) Result~bool~
+ size() u64
}
class ImageBuffer {
- MaaImageBufferHandle handle
- bool own
+ new() ImageBuffer
+ from_handle(handle MaaImageBufferHandle) ImageBuffer
+ handle() MaaImageBufferHandle
+ get_raw_data() *mut u8
+ width() i32
+ height() i32
+ channels() i32
+ image_type() i32
+ set_raw_data(data &[u8], width i32, height i32, img_type i32) bool
+ to_vec() Vec~u8~
}
class RectBuffer {
- MaaRectHandle handle
+ new() RectBuffer
+ handle() MaaRectHandle
+ get() Rect
+ set(rect &Rect) bool
}
class StringListBuffer {
- MaaStringListBufferHandle handle
+ new() StringListBuffer
+ handle() MaaStringListBufferHandle
+ size() u64
+ get() Result~Vec~String~~
+ set(values &[&str]) Result~bool~
}
class ImageListBuffer {
- MaaImageListBufferHandle handle
+ new() ImageListBuffer
+ handle() MaaImageListBufferHandle
+ size() u64
+ get() Vec~ImageBuffer~
}
class Rect {
+ i32 x
+ i32 y
+ i32 width
+ i32 height
+ new(x i32, y i32, width i32, height i32) Rect
}
class Toolkit {
+ init_option(user_path impl AsRef~Path~, default_config &Value) Result~bool~
+ find_adb_devices() Vec~AdbDevice~
+ find_adb_devices_specified(adb_path impl AsRef~Path~) Result~Vec~AdbDevice~~
+ find_desktop_windows() Vec~DesktopWindow~
}
class AdbDevice {
+ String name
+ PathBuf adb_path
+ String address
+ MaaAdbScreencapMethod screencap_methods
+ MaaAdbInputMethod input_methods
+ Value config
}
class DesktopWindow {
+ *mut c_void hwnd
+ String class_name
+ String window_name
}
class NotificationType {
<<enum>>
+ Starting
+ Succeeded
+ Failed
+ Unknown
}
class ResourceLoadingDetail {
+ i64 res_id
+ String hash
+ String path
}
class ControllerActionDetail {
+ i64 ctrl_id
+ String uuid
+ String action
+ Value param
}
class TaskerTaskDetail {
+ i64 task_id
+ String entry
+ String uuid
+ String hash
}
class NextListItem {
+ String name
+ bool jump_back
+ bool anchor
}
class NodeNextListDetail {
+ i64 task_id
+ String name
+ Vec~NextListItem~ list
+ Value focus
}
class NodeRecognitionDetail {
+ i64 task_id
+ i64 reco_id
+ String name
+ Value focus
}
class NodeActionDetail {
+ i64 task_id
+ i64 action_id
+ String name
+ Value focus
}
class NodePipelineNodeDetail {
+ i64 task_id
+ i64 node_id
+ String name
+ Value focus
}
class ResourceEventSink {
<<interface>>
+ on_resource_loading(noti_type NotificationType, detail ResourceLoadingDetail) void
+ on_unknown_notification(msg &str, details &Value) void
}
class ControllerEventSink {
<<interface>>
+ on_controller_action(noti_type NotificationType, detail ControllerActionDetail) void
+ on_unknown_notification(msg &str, details &Value) void
}
class TaskerEventSink {
<<interface>>
+ on_tasker_task(noti_type NotificationType, detail TaskerTaskDetail) void
+ on_unknown_notification(msg &str, details &Value) void
}
class ContextEventSink {
<<interface>>
+ on_node_next_list(noti_type NotificationType, detail NodeNextListDetail) void
+ on_node_recognition(noti_type NotificationType, detail NodeRecognitionDetail) void
+ on_node_action(noti_type NotificationType, detail NodeActionDetail) void
+ on_node_pipeline_node(noti_type NotificationType, detail NodePipelineNodeDetail) void
+ on_unknown_notification(msg &str, details &Value) void
}
class CustomRecognitionArgs {
+ &Context context
+ MaaTaskId task_id
+ &str node_name
+ &str custom_recognition_name
+ &str custom_recognition_param
+ &ImageBuffer image
+ Rect roi
}
class CustomRecognitionResult {
+ bool hit
+ Rect box_
+ String detail
}
class CustomRecognition {
<<interface>>
+ run(args CustomRecognitionArgs) Option~CustomRecognitionResult~
}
class CustomActionArgs {
+ &Context context
+ MaaTaskId task_id
+ &str node_name
+ &str custom_action_name
+ &str custom_action_param
+ MaaRecoId reco_id
+ Rect box_
}
class CustomAction {
<<interface>>
+ run(args CustomActionArgs) bool
}
class FnRecognition~F~ {
- F func
+ new(func F) FnRecognition~F~
+ run(args CustomRecognitionArgs) Option~CustomRecognitionResult~
}
class FnAction~F~ {
- F func
+ new(func F) FnAction~F~
+ run(args CustomActionArgs) bool
}
class MaaError {
<<enum>>
+ LibraryLoad
+ CreateFailed
+ OperationFailed
+ InvalidHandle
+ BufferError
+ JsonError
+ Utf8Error
+ NullPointer
+ TaskFailed
+ ConnectionFailed
+ ResourceLoadFailed
+ InvalidId
}
StringListBuffer --> StringBuffer : uses
ImageListBuffer --> ImageBuffer : uses
Toolkit --> AdbDevice : returns
Toolkit --> DesktopWindow : returns
ResourceEventSink <|.. ControllerEventSink
ResourceEventSink <|.. TaskerEventSink
ResourceEventSink <|.. ContextEventSink
FnRecognition ..|> CustomRecognition
FnAction ..|> CustomAction
文件级改动
Tips and commandsInteracting with Sourcery
Customizing Your Experience访问你的 dashboard 来：
Getting HelpOriginal review guide in EnglishReviewer's GuideAdd an experimental, AI-generated Rust binding crate for MaaFramework, including a dynamic FFI layer, safe high-level abstractions (resource/controller/tasker/context/toolkit), job and notification models, custom action/recognition hooks, example usage, and documentation updates to reference the Rust integration and mark it as experimental. Sequence diagram for a Rust task execution using the new bindingsequenceDiagram
%% Flow: Rust app uses maa-framework crate to run a task on a device
actor User
participant App as RustApp
participant Crate as MaaFrameworkCrate
participant Toolkit as ToolkitModule
participant Ctrl as AdbController
participant Res as Resource
participant Tasker as Tasker
participant FFI as ffi_rs
participant CApi as Maa_C_API
User->>App: run()
App->>Crate: load_library(path_to_framework)
Crate->>FFI: load_symbol MaaVersion
FFI->>CApi: dlopen + dlsym
App->>Crate: load_toolkit(path_to_toolkit)
Crate->>FFI: load_symbol MaaToolkitAdbDeviceListCreate
FFI->>CApi: dlopen + dlsym
App->>Toolkit: find_adb_devices()
Toolkit->>FFI: maa_toolkit_adb_device_list_create
FFI->>CApi: MaaToolkitAdbDeviceListCreate
Toolkit->>FFI: maa_toolkit_adb_device_find
FFI->>CApi: MaaToolkitAdbDeviceFind
Toolkit-->>App: Vec<AdbDevice>
App->>Ctrl: AdbController::new(device_info)
Ctrl->>FFI: maa_adb_controller_create
FFI->>CApi: MaaAdbControllerCreate
Ctrl-->>App: AdbController
App->>Res: Resource::new()
Res->>FFI: maa_resource_create
FFI->>CApi: MaaResourceCreate
Res-->>App: Resource
App->>Tasker: Tasker::new()
Tasker->>FFI: maa_tasker_create
FFI->>CApi: MaaTaskerCreate
Tasker-->>App: Tasker
App->>Tasker: bind(resource, controller)
Tasker->>FFI: maa_tasker_bind_resource
FFI->>CApi: MaaTaskerBindResource
Tasker->>FFI: maa_tasker_bind_controller
FFI->>CApi: MaaTaskerBindController
App->>Tasker: post_task("StartUp", override_json)
Tasker->>FFI: maa_tasker_post_task
FFI->>CApi: MaaTaskerPostTask
Tasker-->>App: TaskJob
App->>Tasker: job.wait()
Tasker->>FFI: maa_tasker_wait
FFI->>CApi: MaaTaskerWait
Tasker-->>App: Status::Succeeded/Failed
App-->>User: print result and task detail
Supporting class diagram for buffers, toolkit, notifications, and custom hooksclassDiagram
%% Supporting types: buffers, toolkit, notifications, custom hooks, errors
class StringBuffer {
- MaaStringBufferHandle handle
- bool own
+ new() StringBuffer
+ from_handle(handle MaaStringBufferHandle) StringBuffer
+ handle() MaaStringBufferHandle
+ into_raw() MaaStringBufferHandle
+ get() Result~String~
+ set(value &str) Result~bool~
+ size() u64
}
class ImageBuffer {
- MaaImageBufferHandle handle
- bool own
+ new() ImageBuffer
+ from_handle(handle MaaImageBufferHandle) ImageBuffer
+ handle() MaaImageBufferHandle
+ get_raw_data() *mut u8
+ width() i32
+ height() i32
+ channels() i32
+ image_type() i32
+ set_raw_data(data &[u8], width i32, height i32, img_type i32) bool
+ to_vec() Vec~u8~
}
class RectBuffer {
- MaaRectHandle handle
+ new() RectBuffer
+ handle() MaaRectHandle
+ get() Rect
+ set(rect &Rect) bool
}
class StringListBuffer {
- MaaStringListBufferHandle handle
+ new() StringListBuffer
+ handle() MaaStringListBufferHandle
+ size() u64
+ get() Result~Vec~String~~
+ set(values &[&str]) Result~bool~
}
class ImageListBuffer {
- MaaImageListBufferHandle handle
+ new() ImageListBuffer
+ handle() MaaImageListBufferHandle
+ size() u64
+ get() Vec~ImageBuffer~
}
class Rect {
+ i32 x
+ i32 y
+ i32 width
+ i32 height
+ new(x i32, y i32, width i32, height i32) Rect
}
class Toolkit {
+ init_option(user_path impl AsRef~Path~, default_config &Value) Result~bool~
+ find_adb_devices() Vec~AdbDevice~
+ find_adb_devices_specified(adb_path impl AsRef~Path~) Result~Vec~AdbDevice~~
+ find_desktop_windows() Vec~DesktopWindow~
}
class AdbDevice {
+ String name
+ PathBuf adb_path
+ String address
+ MaaAdbScreencapMethod screencap_methods
+ MaaAdbInputMethod input_methods
+ Value config
}
class DesktopWindow {
+ *mut c_void hwnd
+ String class_name
+ String window_name
}
class NotificationType {
<<enum>>
+ Starting
+ Succeeded
+ Failed
+ Unknown
}
class ResourceLoadingDetail {
+ i64 res_id
+ String hash
+ String path
}
class ControllerActionDetail {
+ i64 ctrl_id
+ String uuid
+ String action
+ Value param
}
class TaskerTaskDetail {
+ i64 task_id
+ String entry
+ String uuid
+ String hash
}
class NextListItem {
+ String name
+ bool jump_back
+ bool anchor
}
class NodeNextListDetail {
+ i64 task_id
+ String name
+ Vec~NextListItem~ list
+ Value focus
}
class NodeRecognitionDetail {
+ i64 task_id
+ i64 reco_id
+ String name
+ Value focus
}
class NodeActionDetail {
+ i64 task_id
+ i64 action_id
+ String name
+ Value focus
}
class NodePipelineNodeDetail {
+ i64 task_id
+ i64 node_id
+ String name
+ Value focus
}
class ResourceEventSink {
<<interface>>
+ on_resource_loading(noti_type NotificationType, detail ResourceLoadingDetail) void
+ on_unknown_notification(msg &str, details &Value) void
}
class ControllerEventSink {
<<interface>>
+ on_controller_action(noti_type NotificationType, detail ControllerActionDetail) void
+ on_unknown_notification(msg &str, details &Value) void
}
class TaskerEventSink {
<<interface>>
+ on_tasker_task(noti_type NotificationType, detail TaskerTaskDetail) void
+ on_unknown_notification(msg &str, details &Value) void
}
class ContextEventSink {
<<interface>>
+ on_node_next_list(noti_type NotificationType, detail NodeNextListDetail) void
+ on_node_recognition(noti_type NotificationType, detail NodeRecognitionDetail) void
+ on_node_action(noti_type NotificationType, detail NodeActionDetail) void
+ on_node_pipeline_node(noti_type NotificationType, detail NodePipelineNodeDetail) void
+ on_unknown_notification(msg &str, details &Value) void
}
class CustomRecognitionArgs {
+ &Context context
+ MaaTaskId task_id
+ &str node_name
+ &str custom_recognition_name
+ &str custom_recognition_param
+ &ImageBuffer image
+ Rect roi
}
class CustomRecognitionResult {
+ bool hit
+ Rect box_
+ String detail
}
class CustomRecognition {
<<interface>>
+ run(args CustomRecognitionArgs) Option~CustomRecognitionResult~
}
class CustomActionArgs {
+ &Context context
+ MaaTaskId task_id
+ &str node_name
+ &str custom_action_name
+ &str custom_action_param
+ MaaRecoId reco_id
+ Rect box_
}
class CustomAction {
<<interface>>
+ run(args CustomActionArgs) bool
}
class FnRecognition~F~ {
- F func
+ new(func F) FnRecognition~F~
+ run(args CustomRecognitionArgs) Option~CustomRecognitionResult~
}
class FnAction~F~ {
- F func
+ new(func F) FnAction~F~
+ run(args CustomActionArgs) bool
}
class MaaError {
<<enum>>
+ LibraryLoad
+ CreateFailed
+ OperationFailed
+ InvalidHandle
+ BufferError
+ JsonError
+ Utf8Error
+ NullPointer
+ TaskFailed
+ ConnectionFailed
+ ResourceLoadFailed
+ InvalidId
}
StringListBuffer --> StringBuffer : uses
ImageListBuffer --> ImageBuffer : uses
Toolkit --> AdbDevice : returns
Toolkit --> DesktopWindow : returns
ResourceEventSink <|.. ControllerEventSink
ResourceEventSink <|.. TaskerEventSink
ResourceEventSink <|.. ContextEventSink
FnRecognition ..|> CustomRecognition
FnAction ..|> CustomAction
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - 我已经审查了你的修改，并发现了一些需要处理的问题。
- 有几条对外暴露的调用路径可能会触发 panic（
load_symbol!中使用expect，在resource.rs、tasker.rs、toolkit.rs以及示例代码中多处CString::new(...).unwrap()等）。建议从这些调用点返回Result，或者把这些 panic 转换成MaaError，这样在遇到不合法输入或缺失符号时，绑定层的行为更像一个库，而不是直接让整个进程崩溃。 - 像
Tasker、Controller、Resource和DesktopWindow这类类型在持有原始 FFI 句柄的情况下被标记为unsafe impl Send/Sync；最好能在某个地方（比如注释）对这些并发安全保证进行说明，或者在 C API 没有明确声明线程安全时，在封装的调用外层加上合适的同步措施。 Resource里的custom_recognitions和custom_actions字段目前完全没有被使用；如果后续计划支持自定义回调注册，建议现在就把这些字段接到底层 FFI 上；如果短期内不会支持，考虑先移除这些字段，以免造成困惑。
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several public-facing paths can panic (`load_symbol!` using `expect`, multiple `CString::new(...).unwrap()` in `resource.rs`, `tasker.rs`, `toolkit.rs`, examples, etc.); consider returning `Result` from these call sites or converting the panics into `MaaError` so the binding behaves like a library rather than aborting the process on bad input or missing symbols.
- Types like `Tasker`, `Controller`, `Resource`, and `DesktopWindow` are marked `unsafe impl Send/Sync` while holding raw FFI handles; it would be good to either justify these guarantees (e.g. in comments) or wrap underlying calls with appropriate synchronization if the C API is not explicitly thread-safe.
- The `custom_recognitions` and `custom_actions` fields in `Resource` are currently never used; if registration of custom callbacks is planned, consider wiring these through to the underlying FFI now or removing the fields to avoid confusion until that support exists.
## Individual Comments
### Comment 1
<location> `source/binding/Rust/src/buffer.rs:206-210` </location>
<code_context>
+ Ok(result)
+ }
+
+ pub fn set(&self, values: &[&str]) -> Result<bool> {
+ for value in values {
+ let str_buf = StringBuffer::new();
+ str_buf.set(value)?;
+ if ffi::maa_string_list_buffer_append(self.handle, str_buf.handle()) == 0 {
+ return Ok(false);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** StringListBuffer::set drops StringBuffers after appending, likely leaving the underlying list with dangling pointers.
Each `StringBuffer` is created with `own: true` and its handle appended to the list, then `str_buf` is dropped at end of the loop, calling `maa_string_buffer_destroy` while the list still stores the pointer. That’s a likely use-after-free on the C side.
You’ll need to change the ownership model here, e.g.:
- use an `into_raw` / `from_raw` pattern so ownership is transferred to the C list, or
- build the list fully on the C side so Rust never owns the child buffers.
At minimum, construct these via `StringBuffer::from_handle` with `own = false`, or otherwise ensure list elements are not dropped by Rust.
</issue_to_address>
### Comment 2
<location> `source/binding/Rust/src/resource.rs:103-104` </location>
<code_context>
+ self.set_inference(InferenceExecutionProvider::Cpu, InferenceDevice::Cpu)
+ }
+
+ pub fn use_directml(&self, device_id: i32) -> bool {
+ self.set_inference(InferenceExecutionProvider::DirectMl, unsafe {
+ std::mem::transmute(device_id)
+ })
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting `i32` to `InferenceDevice` with `transmute` is unsafe and unnecessary here.
`use_directml` (and `use_coreml`) build `InferenceDevice` with `unsafe { std::mem::transmute(device_id) }`. This is UB for any `device_id` that doesn’t match a valid enum discriminant, and it isn’t needed since the C API ultimately wants an `i32`.
Prefer either:
- keeping `ep`/`device` as plain `i32` at this FFI boundary, or
- adding a safe `InferenceDevice::from_i32 -> Option<Self>` and handling invalid values.
This avoids `transmute` while keeping the API explicit and safe.
</issue_to_address>
### Comment 3
<location> `source/binding/Rust/src/tasker.rs:126-132` </location>
<code_context>
+ ffi::maa_tasker_inited(self.handle) != 0
+ }
+
+ pub fn post_task(&self, entry: &str, pipeline_override: &Value) -> Result<TaskJob> {
+ let entry_cstr = CString::new(entry).map_err(|e| MaaError::BufferError(e.to_string()))?;
+ let override_str = serde_json::to_string(pipeline_override)?;
+ let override_cstr =
+ CString::new(override_str).map_err(|e| MaaError::BufferError(e.to_string()))?;
+
+ let id =
+ ffi::maa_tasker_post_task(self.handle, entry_cstr.as_ptr(), override_cstr.as_ptr());
+ Ok(TaskJob::new(id, self.handle))
</code_context>
<issue_to_address>
**suggestion (bug_risk):** No handling for invalid task IDs (MAA_INVALID_ID) returned from FFI when creating TaskJob.
The C API uses `MAA_INVALID_ID == 0`, but `post_task` always constructs a `TaskJob` from whatever ID is returned. If the FFI call fails and returns `MAA_INVALID_ID`, subsequent `wait`/`status` calls may misbehave or even hit UB depending on the C side.
Consider checking the result and returning an error when it equals `MAA_INVALID_ID`, similar to your null-handle checks elsewhere, e.g.:
```rust
let id = ffi::maa_tasker_post_task(self.handle, entry_cstr.as_ptr(), override_cstr.as_ptr());
if id == MAA_INVALID_ID {
return Err(MaaError::TaskFailed); // or a more specific error
}
Ok(TaskJob::new(id, self.handle))
```
You may also want to apply the same pattern to other FFI calls that return task/controller/resource IDs if they use `MAA_INVALID_ID` as well.
Suggested implementation:
```rust
pub fn post_task(&self, entry: &str, pipeline_override: &Value) -> Result<TaskJob> {
let entry_cstr = CString::new(entry).map_err(|e| MaaError::BufferError(e.to_string()))?;
let override_str = serde_json::to_string(pipeline_override)?;
let override_cstr =
CString::new(override_str).map_err(|e| MaaError::BufferError(e.to_string()))?;
let id =
ffi::maa_tasker_post_task(self.handle, entry_cstr.as_ptr(), override_cstr.as_ptr());
if id == ffi::MAA_INVALID_ID {
// FFI reported a failure to create the task; surface this as a Rust error
return Err(MaaError::TaskFailed("failed to post task".to_string()));
}
Ok(TaskJob::new(id, self.handle))
}
```
1. Ensure that the FFI constant `MAA_INVALID_ID` is exposed under `ffi::MAA_INVALID_ID`.
- If it is defined elsewhere (e.g., as a top-level constant), adjust the comparison accordingly (for example, `if id == MAA_INVALID_ID`).
2. Confirm that `MaaError::TaskFailed(String)` (or the constructor used here) exists in your error type definition.
- If it does not, add an appropriate variant to your `MaaError` enum (e.g. `TaskFailed(String)` or a more general FFI error) and update its `Display`/`Error` implementations as needed.
- If your project uses a different error style (e.g. `MaaError::TaskFailed` without payload, or `MaaError::TaskError { reason: String }`), adjust the `return Err(...)` line to match.
3. Consider applying the same `MAA_INVALID_ID` check to other FFI functions that return IDs (e.g. `maa_tasker_post_stop`) and, if appropriate, changing their return type from `TaskJob` to `Result<TaskJob>` to propagate errors instead of constructing a `TaskJob` for an invalid ID.
</issue_to_address>
### Comment 4
<location> `source/binding/Rust/src/tasker.rs:84-89` </location>
<code_context>
+ self.handle
+ }
+
+ pub fn bind(&mut self, resource: Resource, controller: Controller) -> bool {
+ let res_ret = ffi::maa_tasker_bind_resource(self.handle, resource.handle());
+ let ctrl_ret = ffi::maa_tasker_bind_controller(self.handle, controller.handle());
+ self._resource_holder = Some(resource);
+ self._controller_holder = Some(controller);
+ res_ret != 0 && ctrl_ret != 0
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** bind/bind_resource/bind_controller store holders even when the underlying bind call fails.
Here the `Resource` and `Controller` are stored in `_resource_holder` / `_controller_holder` even if the FFI bind calls fail. That means callers can see `false` but the `Tasker` still holds the handles, which may be surprising and could mask failed bindings.
Either only store the holders when both FFI calls succeed, or keep the current behavior but document that `Tasker` may retain resources/controllers purely for lifetime management after a failed bind, and consider exposing an explicit "is bound" check (e.g., via `maa_tasker_inited`).
Suggested implementation:
```rust
pub fn bind(&mut self, resource: Resource, controller: Controller) -> bool {
let res_ret = ffi::maa_tasker_bind_resource(self.handle, resource.handle());
let ctrl_ret = ffi::maa_tasker_bind_controller(self.handle, controller.handle());
let ok = res_ret != 0 && ctrl_ret != 0;
if ok {
// Only hold the resource/controller when both bindings succeed
self._resource_holder = Some(resource);
self._controller_holder = Some(controller);
}
ok
}
```
From your comment, it sounds like there might also be separate `bind_resource` and `bind_controller` methods that similarly store holders even when the FFI calls fail. You should apply the same pattern to those methods: compute an `ok` flag from the FFI return value, update the corresponding holder only when `ok` is `true`, and return `ok`. If you decide to keep holders even on failure for some reason, add Rust doc comments on those methods documenting that behavior and consider exposing an explicit "is bound" method that checks the actual FFI state (e.g. via `maa_tasker_inited` or an equivalent function).
</issue_to_address>
### Comment 5
<location> `source/binding/Rust/src/toolkit.rs:50-60` </location>
<code_context>
+ fn find_adb_devices_impl(specified_adb: Option<&Path>) -> Vec<AdbDevice> {
+ let list_handle = ffi::maa_toolkit_adb_device_list_create();
+
+ if let Some(adb_path) = specified_adb {
+ let path_str = adb_path.to_string_lossy();
+ let path_cstr = CString::new(path_str.as_ref()).unwrap();
+ ffi::maa_toolkit_adb_device_find_specified(path_cstr.as_ptr(), list_handle);
+ } else {
+ ffi::maa_toolkit_adb_device_find(list_handle);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The return value of adb device discovery functions is ignored, which can hide discovery failures.
Since both `maa_toolkit_adb_device_find_specified` and `maa_toolkit_adb_device_find` return `MaaBool`, it would be safer to branch on that result and avoid using the list when discovery fails. For example:
```rust
let ok = if let Some(adb_path) = specified_adb { /* call _find_specified */ } else { /* call _find */ };
if ok == 0 {
ffi::maa_toolkit_adb_device_list_destroy(list_handle);
return Vec::new(); // or a Result
}
```
That way you only read the list when discovery succeeded and you keep error handling explicit.
```suggestion
fn find_adb_devices_impl(specified_adb: Option<&Path>) -> Vec<AdbDevice> {
let list_handle = ffi::maa_toolkit_adb_device_list_create();
let ok = if let Some(adb_path) = specified_adb {
let path_str = adb_path.to_string_lossy();
let path_cstr = CString::new(path_str.as_ref()).unwrap();
ffi::maa_toolkit_adb_device_find_specified(path_cstr.as_ptr(), list_handle)
} else {
ffi::maa_toolkit_adb_device_find(list_handle)
};
if ok == 0 {
ffi::maa_toolkit_adb_device_list_destroy(list_handle);
return Vec::new();
}
```
</issue_to_address>
### Comment 6
<location> `source/binding/Rust/src/tasker.rs:355-356` </location>
<code_context>
+ }
+}
+
+unsafe impl Send for Tasker {}
+unsafe impl Sync for Tasker {}
+
+// Global options
</code_context>
<issue_to_address>
**question (bug_risk):** Unsafe Send/Sync impls assume the underlying C tasker handle is thread-safe.
These `unsafe impl Send` / `Sync` blocks for `Tasker` (and the similar ones for `Controller`/`Resource`) assume `MaaTaskerHandle` is fully thread-safe to move and share. If that’s not guaranteed by the C API, this can introduce data races and undefined behavior that Rust’s type system can’t prevent.
If thread-safety is guaranteed, please document that in a single obvious place (e.g., near the handle/type definition or via a dedicated wrapper type) so the assumption is explicit. If not, narrow the auto traits (e.g., only `Send`, or neither) and expose any required synchronization at a higher level in Rust instead.
</issue_to_address>Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Original comment in English
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Several public-facing paths can panic (
load_symbol!usingexpect, multipleCString::new(...).unwrap()inresource.rs,tasker.rs,toolkit.rs, examples, etc.); consider returningResultfrom these call sites or converting the panics intoMaaErrorso the binding behaves like a library rather than aborting the process on bad input or missing symbols. - Types like
Tasker,Controller,Resource, andDesktopWindoware markedunsafe impl Send/Syncwhile holding raw FFI handles; it would be good to either justify these guarantees (e.g. in comments) or wrap underlying calls with appropriate synchronization if the C API is not explicitly thread-safe. - The
custom_recognitionsandcustom_actionsfields inResourceare currently never used; if registration of custom callbacks is planned, consider wiring these through to the underlying FFI now or removing the fields to avoid confusion until that support exists.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several public-facing paths can panic (`load_symbol!` using `expect`, multiple `CString::new(...).unwrap()` in `resource.rs`, `tasker.rs`, `toolkit.rs`, examples, etc.); consider returning `Result` from these call sites or converting the panics into `MaaError` so the binding behaves like a library rather than aborting the process on bad input or missing symbols.
- Types like `Tasker`, `Controller`, `Resource`, and `DesktopWindow` are marked `unsafe impl Send/Sync` while holding raw FFI handles; it would be good to either justify these guarantees (e.g. in comments) or wrap underlying calls with appropriate synchronization if the C API is not explicitly thread-safe.
- The `custom_recognitions` and `custom_actions` fields in `Resource` are currently never used; if registration of custom callbacks is planned, consider wiring these through to the underlying FFI now or removing the fields to avoid confusion until that support exists.
## Individual Comments
### Comment 1
<location> `source/binding/Rust/src/buffer.rs:206-210` </location>
<code_context>
+ Ok(result)
+ }
+
+ pub fn set(&self, values: &[&str]) -> Result<bool> {
+ for value in values {
+ let str_buf = StringBuffer::new();
+ str_buf.set(value)?;
+ if ffi::maa_string_list_buffer_append(self.handle, str_buf.handle()) == 0 {
+ return Ok(false);
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** StringListBuffer::set drops StringBuffers after appending, likely leaving the underlying list with dangling pointers.
Each `StringBuffer` is created with `own: true` and its handle appended to the list, then `str_buf` is dropped at end of the loop, calling `maa_string_buffer_destroy` while the list still stores the pointer. That’s a likely use-after-free on the C side.
You’ll need to change the ownership model here, e.g.:
- use an `into_raw` / `from_raw` pattern so ownership is transferred to the C list, or
- build the list fully on the C side so Rust never owns the child buffers.
At minimum, construct these via `StringBuffer::from_handle` with `own = false`, or otherwise ensure list elements are not dropped by Rust.
</issue_to_address>
### Comment 2
<location> `source/binding/Rust/src/resource.rs:103-104` </location>
<code_context>
+ self.set_inference(InferenceExecutionProvider::Cpu, InferenceDevice::Cpu)
+ }
+
+ pub fn use_directml(&self, device_id: i32) -> bool {
+ self.set_inference(InferenceExecutionProvider::DirectMl, unsafe {
+ std::mem::transmute(device_id)
+ })
</code_context>
<issue_to_address>
**issue (bug_risk):** Casting `i32` to `InferenceDevice` with `transmute` is unsafe and unnecessary here.
`use_directml` (and `use_coreml`) build `InferenceDevice` with `unsafe { std::mem::transmute(device_id) }`. This is UB for any `device_id` that doesn’t match a valid enum discriminant, and it isn’t needed since the C API ultimately wants an `i32`.
Prefer either:
- keeping `ep`/`device` as plain `i32` at this FFI boundary, or
- adding a safe `InferenceDevice::from_i32 -> Option<Self>` and handling invalid values.
This avoids `transmute` while keeping the API explicit and safe.
</issue_to_address>
### Comment 3
<location> `source/binding/Rust/src/tasker.rs:126-132` </location>
<code_context>
+ ffi::maa_tasker_inited(self.handle) != 0
+ }
+
+ pub fn post_task(&self, entry: &str, pipeline_override: &Value) -> Result<TaskJob> {
+ let entry_cstr = CString::new(entry).map_err(|e| MaaError::BufferError(e.to_string()))?;
+ let override_str = serde_json::to_string(pipeline_override)?;
+ let override_cstr =
+ CString::new(override_str).map_err(|e| MaaError::BufferError(e.to_string()))?;
+
+ let id =
+ ffi::maa_tasker_post_task(self.handle, entry_cstr.as_ptr(), override_cstr.as_ptr());
+ Ok(TaskJob::new(id, self.handle))
</code_context>
<issue_to_address>
**suggestion (bug_risk):** No handling for invalid task IDs (MAA_INVALID_ID) returned from FFI when creating TaskJob.
The C API uses `MAA_INVALID_ID == 0`, but `post_task` always constructs a `TaskJob` from whatever ID is returned. If the FFI call fails and returns `MAA_INVALID_ID`, subsequent `wait`/`status` calls may misbehave or even hit UB depending on the C side.
Consider checking the result and returning an error when it equals `MAA_INVALID_ID`, similar to your null-handle checks elsewhere, e.g.:
```rust
let id = ffi::maa_tasker_post_task(self.handle, entry_cstr.as_ptr(), override_cstr.as_ptr());
if id == MAA_INVALID_ID {
return Err(MaaError::TaskFailed); // or a more specific error
}
Ok(TaskJob::new(id, self.handle))
```
You may also want to apply the same pattern to other FFI calls that return task/controller/resource IDs if they use `MAA_INVALID_ID` as well.
Suggested implementation:
```rust
pub fn post_task(&self, entry: &str, pipeline_override: &Value) -> Result<TaskJob> {
let entry_cstr = CString::new(entry).map_err(|e| MaaError::BufferError(e.to_string()))?;
let override_str = serde_json::to_string(pipeline_override)?;
let override_cstr =
CString::new(override_str).map_err(|e| MaaError::BufferError(e.to_string()))?;
let id =
ffi::maa_tasker_post_task(self.handle, entry_cstr.as_ptr(), override_cstr.as_ptr());
if id == ffi::MAA_INVALID_ID {
// FFI reported a failure to create the task; surface this as a Rust error
return Err(MaaError::TaskFailed("failed to post task".to_string()));
}
Ok(TaskJob::new(id, self.handle))
}
```
1. Ensure that the FFI constant `MAA_INVALID_ID` is exposed under `ffi::MAA_INVALID_ID`.
- If it is defined elsewhere (e.g., as a top-level constant), adjust the comparison accordingly (for example, `if id == MAA_INVALID_ID`).
2. Confirm that `MaaError::TaskFailed(String)` (or the constructor used here) exists in your error type definition.
- If it does not, add an appropriate variant to your `MaaError` enum (e.g. `TaskFailed(String)` or a more general FFI error) and update its `Display`/`Error` implementations as needed.
- If your project uses a different error style (e.g. `MaaError::TaskFailed` without payload, or `MaaError::TaskError { reason: String }`), adjust the `return Err(...)` line to match.
3. Consider applying the same `MAA_INVALID_ID` check to other FFI functions that return IDs (e.g. `maa_tasker_post_stop`) and, if appropriate, changing their return type from `TaskJob` to `Result<TaskJob>` to propagate errors instead of constructing a `TaskJob` for an invalid ID.
</issue_to_address>
### Comment 4
<location> `source/binding/Rust/src/tasker.rs:84-89` </location>
<code_context>
+ self.handle
+ }
+
+ pub fn bind(&mut self, resource: Resource, controller: Controller) -> bool {
+ let res_ret = ffi::maa_tasker_bind_resource(self.handle, resource.handle());
+ let ctrl_ret = ffi::maa_tasker_bind_controller(self.handle, controller.handle());
+ self._resource_holder = Some(resource);
+ self._controller_holder = Some(controller);
+ res_ret != 0 && ctrl_ret != 0
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** bind/bind_resource/bind_controller store holders even when the underlying bind call fails.
Here the `Resource` and `Controller` are stored in `_resource_holder` / `_controller_holder` even if the FFI bind calls fail. That means callers can see `false` but the `Tasker` still holds the handles, which may be surprising and could mask failed bindings.
Either only store the holders when both FFI calls succeed, or keep the current behavior but document that `Tasker` may retain resources/controllers purely for lifetime management after a failed bind, and consider exposing an explicit "is bound" check (e.g., via `maa_tasker_inited`).
Suggested implementation:
```rust
pub fn bind(&mut self, resource: Resource, controller: Controller) -> bool {
let res_ret = ffi::maa_tasker_bind_resource(self.handle, resource.handle());
let ctrl_ret = ffi::maa_tasker_bind_controller(self.handle, controller.handle());
let ok = res_ret != 0 && ctrl_ret != 0;
if ok {
// Only hold the resource/controller when both bindings succeed
self._resource_holder = Some(resource);
self._controller_holder = Some(controller);
}
ok
}
```
From your comment, it sounds like there might also be separate `bind_resource` and `bind_controller` methods that similarly store holders even when the FFI calls fail. You should apply the same pattern to those methods: compute an `ok` flag from the FFI return value, update the corresponding holder only when `ok` is `true`, and return `ok`. If you decide to keep holders even on failure for some reason, add Rust doc comments on those methods documenting that behavior and consider exposing an explicit "is bound" method that checks the actual FFI state (e.g. via `maa_tasker_inited` or an equivalent function).
</issue_to_address>
### Comment 5
<location> `source/binding/Rust/src/toolkit.rs:50-60` </location>
<code_context>
+ fn find_adb_devices_impl(specified_adb: Option<&Path>) -> Vec<AdbDevice> {
+ let list_handle = ffi::maa_toolkit_adb_device_list_create();
+
+ if let Some(adb_path) = specified_adb {
+ let path_str = adb_path.to_string_lossy();
+ let path_cstr = CString::new(path_str.as_ref()).unwrap();
+ ffi::maa_toolkit_adb_device_find_specified(path_cstr.as_ptr(), list_handle);
+ } else {
+ ffi::maa_toolkit_adb_device_find(list_handle);
+ }
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The return value of adb device discovery functions is ignored, which can hide discovery failures.
Since both `maa_toolkit_adb_device_find_specified` and `maa_toolkit_adb_device_find` return `MaaBool`, it would be safer to branch on that result and avoid using the list when discovery fails. For example:
```rust
let ok = if let Some(adb_path) = specified_adb { /* call _find_specified */ } else { /* call _find */ };
if ok == 0 {
ffi::maa_toolkit_adb_device_list_destroy(list_handle);
return Vec::new(); // or a Result
}
```
That way you only read the list when discovery succeeded and you keep error handling explicit.
```suggestion
fn find_adb_devices_impl(specified_adb: Option<&Path>) -> Vec<AdbDevice> {
let list_handle = ffi::maa_toolkit_adb_device_list_create();
let ok = if let Some(adb_path) = specified_adb {
let path_str = adb_path.to_string_lossy();
let path_cstr = CString::new(path_str.as_ref()).unwrap();
ffi::maa_toolkit_adb_device_find_specified(path_cstr.as_ptr(), list_handle)
} else {
ffi::maa_toolkit_adb_device_find(list_handle)
};
if ok == 0 {
ffi::maa_toolkit_adb_device_list_destroy(list_handle);
return Vec::new();
}
```
</issue_to_address>
### Comment 6
<location> `source/binding/Rust/src/tasker.rs:355-356` </location>
<code_context>
+ }
+}
+
+unsafe impl Send for Tasker {}
+unsafe impl Sync for Tasker {}
+
+// Global options
</code_context>
<issue_to_address>
**question (bug_risk):** Unsafe Send/Sync impls assume the underlying C tasker handle is thread-safe.
These `unsafe impl Send` / `Sync` blocks for `Tasker` (and the similar ones for `Controller`/`Resource`) assume `MaaTaskerHandle` is fully thread-safe to move and share. If that’s not guaranteed by the C API, this can introduce data races and undefined behavior that Rust’s type system can’t prevent.
If thread-safety is guaranteed, please document that in a single obvious place (e.g., near the handle/type definition or via a dedicated wrapper type) so the assumption is explicit. If not, narrow the auto traits (e.g., only `Send`, or neither) and expose any required synchronization at a higher level in Rust instead.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
你好——我已经审阅了你的更改，并发现一些需要处理的问题。
ffi.rs中的 FFI 封装使用了带有expect(...)的load_symbol!，在符号缺失或尚未调用load_library/load_toolkit时会发生 panic；建议让这些公共函数（或者framework()/toolkit()访问器）返回Result，这样调用方就可以优雅地处理库加载和符号解析失败，而不是直接让进程崩溃。- 几个核心类型（
Resource、Controller、Tasker、DesktopWindow）基于对 C API 的注释被标记为unsafe impl Send/Sync；更安全的做法是将这些保证进行集中和收窄（例如通过专门的封装或特性开关），并清晰地文档化哪些操作实际上是线程安全的，哪些需要外部同步，以避免在 C 端保证发生变化或被误解时出现意外的数据竞争。 lib.rs中的全局库句柄（FRAMEWORK_LIB/TOOLKIT_LIB）是写一次后就不会被清除的，但当前 API 并不能阻止在同一进程的不同路径中多次调用load_library/load_toolkit；你可能需要让后续调用具备幂等性（例如比较路径），或者返回包含已加载路径的更具描述性的错误，以帮助诊断配置错误。
面向 AI Agent 的提示
Please address the comments from this code review:
## Overall Comments
- The FFI wrappers in `ffi.rs` use `load_symbol!` with `expect(...)`, which will panic on missing symbols or if `load_library`/`load_toolkit` has not been called; consider returning a `Result` from these public functions (or from the `framework()/toolkit()` accessors) so library load and symbol resolution failures can be handled gracefully by callers instead of aborting the process.
- Several core types (`Resource`, `Controller`, `Tasker`, `DesktopWindow`) are marked `unsafe impl Send/Sync` based only on comments about the C API; it would be safer to centralize and narrow these guarantees (e.g., through a dedicated wrapper or feature flag) and document clearly which operations are actually thread-safe vs. require external synchronization, to avoid accidental data races if the C side's guarantees change or are misunderstood.
- The global library handles in `lib.rs` (`FRAMEWORK_LIB`/`TOOLKIT_LIB`) are write-once and never cleared, but the API does not prevent multiple `load_library`/`load_toolkit` calls from different paths within the same process; you may want to either make subsequent calls idempotent (e.g., compare paths) or return a more descriptive error that includes the already-loaded path to help diagnose misconfiguration.
## Individual Comments
### Comment 1
<location> `source/binding/Rust/src/buffer.rs:111-112` </location>
<code_context>
+ ffi::maa_image_buffer_set_raw_data(self.handle, data.as_ptr(), width, height, img_type) != 0
+ }
+
+ pub fn to_vec(&self) -> Vec<u8> {
+ let size = (self.width() * self.height() * self.channels()) as usize;
+ if size == 0 {
+ return Vec::new();
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against negative or overflowing dimensions when computing image buffer size.
`size` is computed as `(self.width() * self.height() * self.channels()) as usize`. Because this multiplication happens in `i32`, negative values or overflow will wrap before being cast to `usize`, which can cause out-of-bounds reads when creating the slice. Please promote to a wider unsigned type before multiplying, and validate that width/height/channels are non-negative and that the product fits in `usize` before calling `from_raw_parts`.
</issue_to_address>
### Comment 2
<location> `source/binding/Rust/src/tasker.rs:335-344` </location>
<code_context>
+ }))
+ }
+
+ pub fn get_task_detail(&self, task_id: MaaTaskId) -> Result<Option<TaskDetail>> {
+ let entry_buf = StringBuffer::new();
+ let mut size: MaaSize = 0;
+ let mut status: MaaStatus = 0;
+
+ if ffi::maa_tasker_get_task_detail(
+ self.handle,
+ task_id,
+ entry_buf.handle(),
+ std::ptr::null_mut(),
+ &mut size,
+ &mut status,
+ ) == 0
+ {
+ return Ok(None);
+ }
+
+ let mut node_ids: Vec<MaaNodeId> = vec![0; size as usize];
+ if ffi::maa_tasker_get_task_detail(
+ self.handle,
</code_context>
<issue_to_address>
**nitpick (performance):** Handle the zero-node case explicitly to avoid an unnecessary second FFI call.
Right now, when `size` is 0 after the first `maa_tasker_get_task_detail` call, we still allocate an empty `Vec` and make a second FFI call with `node_ids.as_mut_ptr()`. Instead, consider an early return for `size == 0` that constructs a `TaskDetail` with an empty `nodes` vector, which keeps the logic clearer and skips the extra FFI call.
</issue_to_address>帮我变得更有用！请对每条评论点 👍 或 👎，我会根据你的反馈改进后续的评审。
Original comment in English
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- The FFI wrappers in
ffi.rsuseload_symbol!withexpect(...), which will panic on missing symbols or ifload_library/load_toolkithas not been called; consider returning aResultfrom these public functions (or from theframework()/toolkit()accessors) so library load and symbol resolution failures can be handled gracefully by callers instead of aborting the process. - Several core types (
Resource,Controller,Tasker,DesktopWindow) are markedunsafe impl Send/Syncbased only on comments about the C API; it would be safer to centralize and narrow these guarantees (e.g., through a dedicated wrapper or feature flag) and document clearly which operations are actually thread-safe vs. require external synchronization, to avoid accidental data races if the C side's guarantees change or are misunderstood. - The global library handles in
lib.rs(FRAMEWORK_LIB/TOOLKIT_LIB) are write-once and never cleared, but the API does not prevent multipleload_library/load_toolkitcalls from different paths within the same process; you may want to either make subsequent calls idempotent (e.g., compare paths) or return a more descriptive error that includes the already-loaded path to help diagnose misconfiguration.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The FFI wrappers in `ffi.rs` use `load_symbol!` with `expect(...)`, which will panic on missing symbols or if `load_library`/`load_toolkit` has not been called; consider returning a `Result` from these public functions (or from the `framework()/toolkit()` accessors) so library load and symbol resolution failures can be handled gracefully by callers instead of aborting the process.
- Several core types (`Resource`, `Controller`, `Tasker`, `DesktopWindow`) are marked `unsafe impl Send/Sync` based only on comments about the C API; it would be safer to centralize and narrow these guarantees (e.g., through a dedicated wrapper or feature flag) and document clearly which operations are actually thread-safe vs. require external synchronization, to avoid accidental data races if the C side's guarantees change or are misunderstood.
- The global library handles in `lib.rs` (`FRAMEWORK_LIB`/`TOOLKIT_LIB`) are write-once and never cleared, but the API does not prevent multiple `load_library`/`load_toolkit` calls from different paths within the same process; you may want to either make subsequent calls idempotent (e.g., compare paths) or return a more descriptive error that includes the already-loaded path to help diagnose misconfiguration.
## Individual Comments
### Comment 1
<location> `source/binding/Rust/src/buffer.rs:111-112` </location>
<code_context>
+ ffi::maa_image_buffer_set_raw_data(self.handle, data.as_ptr(), width, height, img_type) != 0
+ }
+
+ pub fn to_vec(&self) -> Vec<u8> {
+ let size = (self.width() * self.height() * self.channels()) as usize;
+ if size == 0 {
+ return Vec::new();
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against negative or overflowing dimensions when computing image buffer size.
`size` is computed as `(self.width() * self.height() * self.channels()) as usize`. Because this multiplication happens in `i32`, negative values or overflow will wrap before being cast to `usize`, which can cause out-of-bounds reads when creating the slice. Please promote to a wider unsigned type before multiplying, and validate that width/height/channels are non-negative and that the product fits in `usize` before calling `from_raw_parts`.
</issue_to_address>
### Comment 2
<location> `source/binding/Rust/src/tasker.rs:335-344` </location>
<code_context>
+ }))
+ }
+
+ pub fn get_task_detail(&self, task_id: MaaTaskId) -> Result<Option<TaskDetail>> {
+ let entry_buf = StringBuffer::new();
+ let mut size: MaaSize = 0;
+ let mut status: MaaStatus = 0;
+
+ if ffi::maa_tasker_get_task_detail(
+ self.handle,
+ task_id,
+ entry_buf.handle(),
+ std::ptr::null_mut(),
+ &mut size,
+ &mut status,
+ ) == 0
+ {
+ return Ok(None);
+ }
+
+ let mut node_ids: Vec<MaaNodeId> = vec![0; size as usize];
+ if ffi::maa_tasker_get_task_detail(
+ self.handle,
</code_context>
<issue_to_address>
**nitpick (performance):** Handle the zero-node case explicitly to avoid an unnecessary second FFI call.
Right now, when `size` is 0 after the first `maa_tasker_get_task_detail` call, we still allocate an empty `Vec` and make a second FFI call with `node_ids.as_mut_ptr()`. Instead, consider an early return for `size == 0` that constructs a `TaskDetail` with an empty `nodes` vector, which keeps the logic clearer and skips the extra FFI call.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an experimental Rust binding for MaaFramework, providing a type-safe, idiomatic Rust API over the C FFI. The binding is AI-generated and explicitly marked as not fully tested, requiring developer validation before production use. It follows standard interface design patterns with async job support, event notifications, and custom recognition/action capabilities.
Key changes:
- Complete Rust binding with FFI layer wrapping all MaaFramework C APIs
- High-level abstractions for Resource, Controller (ADB/Win32/Debug), Tasker, Context, and toolkit functions
- Job-based async pattern for all operations (TaskJob, CtrlJob, ResJob)
- Documentation in both English and Chinese with usage examples
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| source/binding/Rust/src/lib.rs | Library root with module exports and dynamic library loading |
| source/binding/Rust/src/ffi.rs | Raw FFI bindings to C API using libloading for dynamic symbol resolution |
| source/binding/Rust/src/error.rs | Error types using thiserror for Result-based error handling |
| source/binding/Rust/src/define.rs | Type definitions, enums, and constants matching C API |
| source/binding/Rust/src/buffer.rs | Safe wrappers for string, image, rect, and list buffers |
| source/binding/Rust/src/job.rs | Async job abstractions for tracking operation status |
| source/binding/Rust/src/resource.rs | Resource management with pipeline and inference configuration |
| source/binding/Rust/src/controller.rs | Device controllers for ADB, Win32, and debug modes |
| source/binding/Rust/src/tasker.rs | Task execution with detail retrieval for recognition/actions |
| source/binding/Rust/src/context.rs | Task context for running tasks and managing state |
| source/binding/Rust/src/toolkit.rs | Device discovery utilities for ADB and desktop windows |
| source/binding/Rust/src/custom.rs | Traits for custom recognition and action implementations |
| source/binding/Rust/src/notification.rs | Event sink traits and notification message parsing |
| source/binding/Rust/examples/basic.rs | Example demonstrating device connection and resource loading |
| source/binding/Rust/README.md | English documentation with API reference and examples |
| source/binding/Rust/README.zh_cn.md | Chinese documentation with quick start guide |
| source/binding/Rust/Cargo.toml | Package manifest with dependencies (libloading, serde, thiserror) |
| docs/en_us/2.1-Integration.md | Updated integration guide noting AI-generated experimental status |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+220
to
+224
| return Ok(false); | ||
| } | ||
| // Transfer ownership to the C list - don't let Rust destroy this buffer | ||
| // The C StringListBuffer takes ownership of appended buffers | ||
| str_buf.into_raw(); |
There was a problem hiding this comment.
The StringListBuffer::set method has a memory management issue. After calling str_buf.into_raw() to transfer ownership to the C API, the code doesn't verify whether the C API actually takes ownership. If maa_string_list_buffer_append fails (returns 0), the buffer ownership has already been transferred via into_raw(), causing a memory leak.
The code should transfer ownership only after confirming the append operation succeeded:
pub fn set(&self, values: &[&str]) -> Result<bool> {
for value in values {
let str_buf = StringBuffer::new();
str_buf.set(value)?;
if ffi::maa_string_list_buffer_append(self.handle, str_buf.handle()) == 0 {
// Don't call into_raw() here - let Drop clean up
return Ok(false);
}
// Transfer ownership only after successful append
str_buf.into_raw();
}
Ok(true)
}
Suggested change
| return Ok(false); | |
| } | |
| // Transfer ownership to the C list - don't let Rust destroy this buffer | |
| // The C StringListBuffer takes ownership of appended buffers | |
| str_buf.into_raw(); | |
| // Don't transfer ownership; let Drop clean up | |
| return Ok(false); | |
| } else { | |
| // Transfer ownership to the C list - don't let Rust destroy this buffer | |
| // The C StringListBuffer takes ownership of appended buffers | |
| str_buf.into_raw(); | |
| } |
Comment on lines
+94
to
+102
| let ctrl_ret = ffi::maa_tasker_bind_controller(self.handle, controller.handle()); | ||
| let ok = res_ret != 0 && ctrl_ret != 0; | ||
|
|
||
| if ok { | ||
| self._resource_holder = Some(resource); | ||
| self._controller_holder = Some(controller); | ||
| } | ||
|
|
||
| ok |
There was a problem hiding this comment.
The Tasker::bind method has inconsistent error handling. When either resource or controller binding fails, the method returns false, but the successful binding is not rolled back. This creates an inconsistent state where one is bound but not the other, and the successful binding's holder is stored even though the overall operation failed.
Consider rolling back the successful binding or ensuring both succeed atomically:
pub fn bind(&mut self, resource: Resource, controller: Controller) -> bool {
let res_ret = ffi::maa_tasker_bind_resource(self.handle, resource.handle());
if res_ret == 0 {
return false;
}
let ctrl_ret = ffi::maa_tasker_bind_controller(self.handle, controller.handle());
if ctrl_ret == 0 {
// Could potentially unbind resource here if API supports it
return false;
}
self._resource_holder = Some(resource);
self._controller_holder = Some(controller);
true
}
Suggested change
| let ctrl_ret = ffi::maa_tasker_bind_controller(self.handle, controller.handle()); | |
| let ok = res_ret != 0 && ctrl_ret != 0; | |
| if ok { | |
| self._resource_holder = Some(resource); | |
| self._controller_holder = Some(controller); | |
| } | |
| ok | |
| if res_ret == 0 { | |
| return false; | |
| } | |
| let ctrl_ret = ffi::maa_tasker_bind_controller(self.handle, controller.handle()); | |
| if ctrl_ret == 0 { | |
| // Roll back resource binding if controller binding fails | |
| // If the unbind function is available, call it; otherwise, document the limitation. | |
| #[allow(unused_unsafe)] | |
| unsafe { | |
| // If ffi::maa_tasker_unbind_resource exists, call it. | |
| #[cfg(feature = "has_unbind_resource")] | |
| { | |
| ffi::maa_tasker_unbind_resource(self.handle); | |
| } | |
| } | |
| return false; | |
| } | |
| self._resource_holder = Some(resource); | |
| self._controller_holder = Some(controller); | |
| true |
Comment on lines
+50
to
+62
| controller.post_connection().wait(); | ||
|
|
||
| // 5. 加载资源 | ||
| let resource = Resource::new()?; | ||
| resource.post_bundle("./resource").wait(); | ||
|
|
||
| // 6. 创建任务器 | ||
| let mut tasker = Tasker::new()?; | ||
| tasker.bind(resource, controller); | ||
|
|
||
| // 7. 执行任务 | ||
| let job = tasker.post_task("StartUp", &json!({}))?; | ||
| job.wait(); |
There was a problem hiding this comment.
The documentation claims that .wait() can be called without error handling (line 50, 54, 64, etc.), but this is misleading. Looking at the implementation, post_connection(), post_bundle(), and post_task() all return Result<Job>, not Job. The .wait() call would need to be chained after unwrapping the Result:
controller.post_connection()?.wait();
// or
resource.post_bundle("./resource")?.wait();This is a documentation error that would prevent the code from compiling.
Suggested change
| controller.post_connection().wait(); | |
| // 5. 加载资源 | |
| let resource = Resource::new()?; | |
| resource.post_bundle("./resource").wait(); | |
| // 6. 创建任务器 | |
| let mut tasker = Tasker::new()?; | |
| tasker.bind(resource, controller); | |
| // 7. 执行任务 | |
| let job = tasker.post_task("StartUp", &json!({}))?; | |
| job.wait(); | |
| controller.post_connection()?.wait(); | |
| // 5. 加载资源 | |
| let resource = Resource::new()?; | |
| resource.post_bundle("./resource")?.wait(); | |
| // 6. 创建任务器 | |
| let mut tasker = Tasker::new()?; | |
| tasker.bind(resource, controller); | |
| // 7. 执行任务 | |
| let job = tasker.post_task("StartUp", &json!({}))?.wait(); |
Comment on lines
+16
to
+28
| pub fn from_handle(handle: MaaContextHandle) -> Result<Self> { | ||
| if handle.is_null() { | ||
| return Err(MaaError::InvalidHandle); | ||
| } | ||
| let tasker_handle = ffi::maa_context_get_tasker(handle); | ||
| if tasker_handle.is_null() { | ||
| return Err(MaaError::InvalidHandle); | ||
| } | ||
| Ok(Self { | ||
| handle, | ||
| tasker: Tasker::from_handle(tasker_handle), | ||
| }) | ||
| } |
There was a problem hiding this comment.
[nitpick] Missing validation for null handle in Context::from_handle. While the function checks if the handle is null (line 17-19) and if tasker_handle is null (lines 21-23), there's no check to ensure the context handle is actually valid. The C API might return a non-null but invalid handle in some error cases. Consider adding additional validation or documenting the assumption that non-null handles from the C API are always valid.
|
|
||
| // 创建任务器 | ||
| let mut tasker = Tasker::new()?; | ||
| tasker.bind(resource, controller.inner.clone()); |
There was a problem hiding this comment.
The example code at line 70 references controller.inner.clone() but the Controller struct doesn't implement Clone, and the binding design intentionally avoids cloning handles (only one owner should drop the handle). This code will not compile.
The example should pass the controller directly or use a reference pattern:
tasker.bind(resource, controller);Note: This assumes bind takes ownership of the controller, which appears to be the intended design based on the bind method signature.
Suggested change
| tasker.bind(resource, controller.inner.clone()); | |
| tasker.bind(resource, controller); |
Comment on lines
+113
to
+114
| if !tasker.bind_resource(resource) { | ||
| println!("绑定资源失败"); |
There was a problem hiding this comment.
Potential memory leak in the example code. The controller is created but then passed to tasker.bind() which takes ownership. After the bind, attempting to use controller later would fail. However, the example only creates the controller and binds it, so this is actually not an issue in the provided code.
Looking more carefully, there's a different issue: the comment on line 112 mentions "需要将 controller 的内部 handle 传递，这里简化处理" (need to pass the controller's internal handle, simplified here), but then the code only binds the resource and doesn't bind the controller at all. This leaves the tasker in an incomplete state.
The code should bind both:
if !tasker.bind(resource, controller) {
println!("绑定失败");
}Or bind them separately if the controller was already used:
// If controller needs to be used elsewhere, bind separately
if !tasker.bind_resource(resource) {
println!("绑定资源失败");
}
Suggested change
| if !tasker.bind_resource(resource) { | |
| println!("绑定资源失败"); | |
| if !tasker.bind(resource, controller) { | |
| println!("绑定失败"); |
Comment on lines
+56
to
+63
| controller.post_connection().wait(); | ||
| if !controller.connected() { | ||
| return Err(MaaError::ConnectionFailed); | ||
| } | ||
|
|
||
| // 创建资源 | ||
| let resource = Resource::new()?; | ||
| resource.post_bundle("path/to/resource").wait(); |
There was a problem hiding this comment.
Missing error propagation operator in the example. The same compilation issue exists here - post_connection() returns Result<CtrlJob>, not CtrlJob, so .wait() cannot be called directly.
Should be:
controller.post_connection()?.wait();This pattern is repeated throughout the examples in lines 56, 63, 116, 130-147.
Suggested change
| controller.post_connection().wait(); | |
| if !controller.connected() { | |
| return Err(MaaError::ConnectionFailed); | |
| } | |
| // 创建资源 | |
| let resource = Resource::new()?; | |
| resource.post_bundle("path/to/resource").wait(); | |
| controller.post_connection()?.wait(); | |
| if !controller.connected() { | |
| return Err(MaaError::ConnectionFailed); | |
| } | |
| // 创建资源 | |
| let resource = Resource::new()?; | |
| resource.post_bundle("path/to/resource")?.wait(); |
Comment on lines
+17
to
+28
| pub box_: Option<Rect>, | ||
| pub raw_detail: Value, | ||
| pub raw_image: Vec<u8>, | ||
| pub draw_images: Vec<Vec<u8>>, | ||
| } | ||
|
|
||
| #[derive(Debug, Clone)] | ||
| pub struct ActionDetail { | ||
| pub action_id: MaaActId, | ||
| pub name: String, | ||
| pub action: Action, | ||
| pub box_: Rect, |
There was a problem hiding this comment.
[nitpick] Inconsistent naming: the field is named box_ (with trailing underscore) throughout the codebase because box is a Rust keyword. However, this creates an inconsistent API surface. Consider using a more idiomatic name like bounding_box, bbox, or rect instead of box_.
This affects multiple structs:
RecognitionDetail::box_(line 17)ActionDetail::box_(line 28)
While box_ is valid Rust, it's unconventional and may confuse users.
Comment on lines
+10
to
+13
| pub struct Context { | ||
| handle: MaaContextHandle, | ||
| tasker: Tasker, | ||
| } |
There was a problem hiding this comment.
The Context struct stores a Tasker instance (line 12) that is created from a non-owned handle (via Tasker::from_handle). However, the Context doesn't manage the lifetime of this tasker handle, and if the original tasker is dropped, this reference would become invalid. The code should document that the Context must not outlive the original Tasker, or it should ensure proper lifetime management.
Consider adding a lifetime parameter or documenting the constraint:
/// Context must not outlive the Tasker it was created from
pub struct Context {
handle: MaaContextHandle,
tasker: Tasker,
}
MistEO
force-pushed
the
main
branch
2 times, most recently
from
bef9648 to
dcee917
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
为 MaaFramework 新增一个实验性的 Rust 绑定，包括 FFI 层、高层 Rust API 和文档，并在集成文档中添加链接，说明这是一个由 AI 生成、尚未经过完整测试的接口。
新功能：
文档：
Original summary in English
Summary by Sourcery
Add an experimental Rust binding for MaaFramework, including FFI layer, high-level Rust API, and documentation, and link it from the integration docs as an AI-generated, not-yet-fully-tested interface.
New Features:
Documentation:
Original summary in English
Summary by Sourcery
为 MaaFramework 新增一个实验性的 Rust 绑定，包括 FFI 层、高层 Rust API 和文档，并在集成文档中添加链接，说明这是一个由 AI 生成、尚未经过完整测试的接口。
新功能：
文档：
Original summary in English
Summary by Sourcery
Add an experimental Rust binding for MaaFramework, including FFI layer, high-level Rust API, and documentation, and link it from the integration docs as an AI-generated, not-yet-fully-tested interface.
New Features:
Documentation: