Repository navigation
Add WASM reference type support and fix placeholder implementations - #4
Merged
Merged
Conversation
WASM externref/funcref support: - Add FuncRef and ExternRef variants to WasmValue enum - Implement handle registry in WasmInstance for storing/retrieving funcrefs - Update to_wasmtime() to look up handles from registry - Update from_wasmtime() to store refs in registry with unique handles - Update op_wasm_call to pass instance for proper ref handling - Add comprehensive tests for funcref registry and null refs forge-weld-macro improvements: - Add type_parser.rs with comprehensive Rust-to-WeldType parsing - Support all primitive types, containers, references, tuples, arrays - Strict mode panics on unparseable types to prevent unknown types Other fixes: - Delete empty resolver.ts placeholder - Pin deno_core to 0.373 across all crates - Extract window manager code to separate module - Add Lies.md to track placeholder implementations - Update site documentation for architecture and roadmap
Reviewer's GuideImplements full WASM reference type (externref/funcref) support with a handle registry, centralizes window management logic into a reusable WindowManager abstraction used by forge-host, introduces a strict Rust→WeldType type parser shared by forge-weld macros, and cleans up placeholders while pinning deno_* dependencies and updating docs/roadmap. Sequence diagram for wasm.call with funcref and handle registrysequenceDiagram
participant JS as JsCaller
participant Ext as ext_wasm_op_wasm_call
participant Inst as WasmInstance
participant WT as wasmtime_Func
JS->>Ext: wasm.call(instance_id, func_name, [FuncRef("funcref_1")])
Ext->>Inst: lock(instance_id) -> &mut WasmInstance
Ext->>Inst: get_func_ref("funcref_1")
alt handle found
Inst-->>Ext: Some(Func)
Ext->>WT: call(store, args=[Val::FuncRef(Some(Func))])
else handle missing
Inst-->>Ext: None
Ext->>WT: call(store, args=[Val::null_func_ref()])
end
WT-->>Ext: results [Val]
loop for each Val result
Ext->>Inst: WasmValue::from_wasmtime(val, &mut WasmInstance)
alt FuncRef
Inst->>Inst: store_func_ref(Func) -> "funcref_N"
else ExternRef
Inst->>Inst: store_extern_ref(id) -> "externref_N"
end
end
Ext-->>JS: Vec<WasmValue> (with FuncRef/ExternRef handles)
Class diagram for updated WASM reference handling (WasmValue and WasmInstance)classDiagram
class WasmValue {
+I32(i32)
+I64(i64)
+F32(f32)
+F64(f64)
+FuncRef(Option~String~)
+ExternRef(Option~String~)
+to_wasmtime(instance: Option~WasmInstance~) Val
+from_wasmtime(val: Val, instance: WasmInstance) Option~WasmValue~
+valtype() ValType
}
class WasmInstance {
+store: Store~WasmStoreData~
+instance: Instance
+module_id: String
+func_refs: HashMap~String, Func~
+next_func_ref_id: u64
+extern_ref_ids: HashMap~String, u64~
+next_extern_ref_id: u64
+store_func_ref(func: Func) String
+get_func_ref(handle: str) Option~Func~
+store_extern_ref(id: u64) String
+get_extern_ref_id(handle: str) Option~u64~
}
class WasmStoreData {
<<struct>>
+... (extension state)
}
class Val {
<<from wasmtime>>
+I32(i32)
+I64(i64)
+F32(u32)
+F64(u64)
+FuncRef(Option~Func~)
+ExternRef(Option~ExternRef~)
}
class Func {
<<from wasmtime>>
}
class ValType {
<<from wasmtime>>
+I32
+I64
+F32
+F64
+FUNCREF
+EXTERNREF
}
WasmInstance "1" o-- "*" WasmValue : uses
WasmInstance "1" o-- "*" Func : stores
WasmInstance "1" --> "1" WasmStoreData : owns
WasmValue ..> Val : converts_to
WasmValue ..> ValType : type_of
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}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In the WASM externref handling,
extern_ref_ids/store_extern_ref/get_extern_ref_idare currently never used andstore_extern_refis called withnext_extern_ref_idinstead of any property of theExternRefitself; consider either wiring this up to actualExternRefidentity or removing the registry until there is a concrete use case to avoid misleading state. - The new
mime_forhelper inext_window::managerduplicates the logic already present inforge-host; it might be worth centralizing this utility to a shared module to avoid divergence in MIME handling over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the WASM externref handling, `extern_ref_ids` / `store_extern_ref` / `get_extern_ref_id` are currently never used and `store_extern_ref` is called with `next_extern_ref_id` instead of any property of the `ExternRef` itself; consider either wiring this up to actual `ExternRef` identity or removing the registry until there is a concrete use case to avoid misleading state.
- The new `mime_for` helper in `ext_window::manager` duplicates the logic already present in `forge-host`; it might be worth centralizing this utility to a shared module to avoid divergence in MIME handling over time.
## Individual Comments
### Comment 1
<location> `crates/ext_window/src/manager.rs:363-372` </location>
<code_context>
+ wv_builder = wv_builder.with_custom_protocol("app".into(), move |_ctx, request| {
</code_context>
<issue_to_address>
**issue:** AssetProvider is bypassed when assets are not embedded, which contradicts its abstraction and ForgeAssetProvider implementation.
In the protocol handler, `asset_provider.get_asset(path)` is only called when `asset_provider.is_embedded()` is true. However, `ForgeAssetProvider.get_asset` already falls back to the filesystem when `ASSET_EMBEDDED` is false. Gating on `is_embedded()` disables the provider in the non-embedded case and reimplements that fallback in `WindowManager`, weakening the abstraction and coupling it to a specific provider. Instead, always call `asset_provider.get_asset(path)` and only read from the filesystem if it returns `None`, so the provider keeps full control over how assets are resolved.
</issue_to_address>
### Comment 2
<location> `crates/ext_window/src/manager.rs:1041-172` </location>
<code_context>
+ // =========================================================================
+
+ /// Handle window close requested event
+ pub fn handle_close_requested(&mut self, tao_window_id: WindowId) -> bool {
+ if let Some(win_id) = self.windows.remove(&tao_window_id) {
+ let _ = self.to_deno_tx.try_send(IpcEvent {
+ window_id: win_id.clone(),
+ channel: "__window__".to_string(),
+ payload: serde_json::json!({}),
+ event_type: Some("close".to_string()),
+ });
+ self.webviews.remove(&win_id);
+ self.tao_windows.remove(&win_id);
+ self.window_channels.remove(&win_id);
+ tracing::info!("Window {} closed", win_id);
+ true
+ } else {
+ false
</code_context>
<issue_to_address>
**suggestion:** Window close logic is duplicated between forge-host and WindowManager, increasing the chance of inconsistent behavior.
`handle_close_requested` already encapsulates closing a window (emitting the `__window__` close event and cleaning up all maps), but `forge-host` still performs similar close logic inline in the tao event loop (`get_window_id` + `remove_window`). This duplication risks divergence if one path gains extra cleanup or events. Please consolidate close handling into a single code path (e.g., always calling `window_manager.handle_close_requested` from the host loop, or making it a thin wrapper over a shared `remove_window`), so window lifecycle changes stay consistent and easier to maintain.
Suggested implementation:
```rust
/// Handle window close requested event
}
}
=======
// Window Events (called from event loop)
// =========================================================================
/// Handle window close requested event
///
/// This encapsulates all window-close behavior so that callers (e.g. the
/// forge-host tao event loop) can simply delegate to this method to ensure
/// consistent cleanup and event emission.
pub fn handle_close_requested(&mut self, tao_window_id: WindowId) -> bool {
if let Some(win_id) = self.windows.remove(&tao_window_id) {
// Emit a __window__ "close" event to the Deno side so userland
// can react to the close.
let _ = self.to_deno_tx.try_send(IpcEvent {
window_id: win_id.clone(),
channel: "__window__".to_string(),
payload: serde_json::json!({}),
event_type: Some("close".to_string()),
});
// Ensure all internal maps are cleaned up for this window ID.
self.webviews.remove(&win_id);
self.tao_windows.remove(&win_id);
self.window_channels.remove(&win_id);
tracing::info!("Window {} closed", win_id);
true
} else {
false
}
}
}
}
```
To fully consolidate close logic and remove duplication with forge-host:
1. In the forge-host tao event loop, replace any inline close handling that:
- looks up an internal window id from a tao `WindowId` (e.g. via `get_window_id`) and
- manually calls something like `remove_window` / removes from maps
with a single call to:
```rust
if !window_manager.handle_close_requested(tao_window_id) {
tracing::warn!("Close requested for unknown tao window: {:?}", tao_window_id);
}
```
2. If there is an existing public `remove_window` (by internal window id), consider either:
- marking it `pub(crate)` or keeping it for non-tao-driven closes, but have its implementation delegate to a shared internal helper used by `handle_close_requested`, or
- updating any callers that respond to tao events to use `handle_close_requested` instead, so all tao-driven closes follow the same code path.
3. Ensure that any future cleanup or additional events on window close are added only in `handle_close_requested` (or its shared helper), so behavior remains consistent between forge-host and other call sites.
</issue_to_address>
### Comment 3
<location> `crates/forge-weld-macro/src/type_parser.rs:27-36` </location>
<code_context>
+pub fn rust_type_to_weld_type(ty: &Type) -> TokenStream {
</code_context>
<issue_to_address>
**suggestion:** Using `panic!` in the type parser for unsupported types degrades macro error UX; `compile_error!` or `syn::Error` would be clearer.
The strictness of `rust_type_to_weld_type` makes sense, but panicking inside a proc macro usually produces poor diagnostics (often pointing inside the macro). For unsupported forms (`impl Trait`, `dyn Trait`, `BareFn`, `Infer`, `Macro`, `Verbatim`, and the final catch‑all), consider emitting a `compile_error!("…")` `TokenStream` or using `syn::Error::to_compile_error()`. This preserves the “no unknown types” invariant while giving users a clear error anchored on the offending type instead of a panic backtrace.
Suggested implementation:
```rust
pub fn rust_type_to_weld_type(ty: &Type) -> syn::Result<TokenStream> {
match ty {
Type::Path(type_path) => parse_path_type(type_path),
Type::Reference(type_ref) => {
let inner = rust_type_to_weld_type(&type_ref.elem)?;
let mutable = type_ref.mutability.is_some();
Ok(quote! {
forge_weld::WeldType::Reference {
inner: Box::new(#inner),
mutable: #mutable,
}
})
```
To fully implement the “no `panic!` in the type parser” suggestion and give better macro diagnostics, the following additional edits are needed in this file:
1. **Import `syn::Error` (and `Result` if you prefer a type alias)**:
- At the top of `type_parser.rs`, add:
```rust
use syn::Error;
```
and optionally:
```rust
type Result<T> = std::result::Result<T, Error>;
```
Then you can change the signature to `pub fn rust_type_to_weld_type(ty: &Type) -> Result<TokenStream>` and adjust the `syn::Result` in the edit above accordingly.
2. **Update all callers of `rust_type_to_weld_type`**:
- Every place that currently calls `rust_type_to_weld_type(...)` and expects a `TokenStream` must now handle a `Result<TokenStream>`, typically by propagating the error (`?`) inside other parsing helpers, and only converting to tokens at the outermost proc-macro boundary:
```rust
let ty_tokens = rust_type_to_weld_type(&some_type)?;
```
- At the top-level proc macro function (the one returning `TokenStream`), convert errors to `TokenStream`:
```rust
match rust_type_to_weld_type(&ty) {
Ok(tokens) => tokens,
Err(err) => err.to_compile_error(),
}
```
3. **Change all `panic!` branches for unsupported types to `syn::Error`**:
- For each unsupported `Type` variant in the `match ty` (and any helper functions like `parse_path_type`) that currently uses `panic!` or `unimplemented!`, replace with something like:
```rust
Type::ImplTrait(t) => {
return Err(Error::new_spanned(
t,
"forge-weld: `impl Trait` is not supported in Weld type mapping",
));
}
Type::TraitObject(t) => {
return Err(Error::new_spanned(
t,
"forge-weld: `dyn Trait` is not supported in Weld type mapping",
));
}
Type::BareFn(t) => {
return Err(Error::new_spanned(
t,
"forge-weld: bare function types are not supported in Weld type mapping",
));
}
Type::Infer(t) => {
return Err(Error::new_spanned(
t,
"forge-weld: inferred types (`_`) are not supported in Weld type mapping",
));
}
Type::Macro(t) => {
return Err(Error::new_spanned(
t,
"forge-weld: type macros are not supported in Weld type mapping",
));
}
Type::Verbatim(t) => {
return Err(Error::new_spanned(
t,
"forge-weld: verbatim types are not supported in Weld type mapping",
));
}
_ => {
return Err(Error::new_spanned(
ty,
"forge-weld: this Rust type is not supported in Weld type mapping",
));
}
```
- These `Error::new_spanned(...).to_compile_error()` will automatically be turned into a `compile_error!` at the top-level when you use `err.to_compile_error()` as described above.
4. **Adjust other helper functions that return `TokenStream`**:
- Any helper like `parse_path_type`, `parse_tuple_type`, etc., that currently returns `TokenStream` and may `panic!` on unsupported constructs should be changed to return `syn::Result<TokenStream>` (or the `Result<T>` alias) and use `Error::new_spanned(...)` instead of `panic!`.
- Calls to these helpers inside `rust_type_to_weld_type` and each other should be updated to use `?` for error propagation:
```rust
Type::Path(type_path) => parse_path_type(type_path)?,
```
These changes preserve the “no unknown types” invariant, but instead of panicking, the proc macro will emit a precise `compile_error!` attached to the offending type via `syn::Error::new_spanned`, which gives much clearer diagnostics to users.
</issue_to_address>
### Comment 4
<location> `Lies.md:28` </location>
<code_context>
+
+### ~~Dependency Version Not Pinned~~ FIXED
+
+**Resolution**: All deno dependencies are now pinned:
+- `deno_core = "0.373"` (all crates)
+- `deno_ast = { version = "0.52", features = ["transpiling"] }`
</code_context>
<issue_to_address>
**nitpick (typo):** Capitalize "Deno" as a proper noun for consistency with the rest of the docs.
For consistency with the rest of the docs, update this line to `All Deno dependencies are now pinned:`.
```suggestion
**Resolution**: All Deno dependencies are now pinned:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
- Remove unused WASM externref registry (funcref registry retained) - Replace panic! with syn::Error in type_parser for better diagnostics - Add context menu timeout (30s) for dismissed menus - Simplify store borrow pattern in op_wasm_call - Optimize args conversion to single-pass validation+conversion - Change verbose info! logs to debug! for routine operations - Fix typo: "deno" -> "Deno" in Lies.md - Add crossbeam-channel dependency for menu timeout handling
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
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
Changes
WASM Reference Types (
crates/ext_wasm/src/lib.rs)FuncRef(Option<String>)andExternRef(Option<String>)variants toWasmValueenumWasmInstancewithfunc_refs: HashMap<String, Func>to_wasmtime()now looks up funcref handles from the instance registryfrom_wasmtime()stores refs in registry with unique handles likefuncref_1op_wasm_call(combined function lookup and type info)test_wasm_funcref_registry,test_wasm_null_refsType Parser (
crates/forge-weld-macro/src/type_parser.rs)Option<T>,Vec<T>,Result<T, E>,HashMap<K, V>, references, tuples, arrayssyn::Errorinstead of panic! for better compile-time diagnostics with source locationWindow Manager (
crates/ext_window/src/manager.rs)info!logs todebug!for routine operations (window/tray/menu lifecycle)Other Fixes
resolver.tsplaceholderdeno_core = "0.373"across all extension cratescrossbeam-channeldependency for menu timeout handlingLies.mdto track placeholder implementationsTest plan
cargo build- Compiles successfullycargo test- All 107 tests passcargo clippy- No warningscargo fmt --check- Properly formattedCode Review Feedback Addressed