webview2/generator: Phase 1 — fix type-system bugs + comprehensive tests#5407
webview2/generator: Phase 1 — fix type-system bugs + comprehensive tests#5407leaanthony wants to merge 1 commit into
Conversation
Four correctness bugs in the code generator fixed: 1. String output vtable call was passing the nil *uint16 pointer directly (`uintptr(unsafe.Pointer(_name))`) instead of its address so COM could write back. Fixed to `uintptr(unsafe.Pointer(&_name))`. 2. Scalar numeric input params (UINT32, INT32 …) were passed as `uintptr(unsafe.Pointer(&val))` — a pointer to a local — instead of `uintptr(val)`. Root cause: processVtableCallInput used the IDL type name (uppercase "UINT32") to detect numerics but the check looked for lowercase "uint". Now uses GoType throughout. 3. Native int output ([out,retval] int* exitCode) was passed by value (`uintptr(exitCode)` == 0) instead of by reference. Fixed by placing pointer-depth checks before the numeric GoType check. 4. COM BOOL ([in] BOOL value) was passed as unsafe.Pointer(&bool) — a pointer to a 1-byte Go bool — instead of a 4-byte int32 value. Added inputBoolSetup.tmpl and processSetupInputs handling. New capability: [in] LPCWSTR* now maps to []string (C string array) with inputStringArraySetup.tmpl converting to []*uint16 before the vtable call. New files: types/typemap.go — TypePatterns table (17 patterns) + ResolveGoType() types/templates/inputBoolSetup.tmpl types/templates/inputStringArraySetup.tmpl generator/idl_parse_test.go — parse all 6 IDL files, check counts + inheritance chain generator/typemap_test.go — one test per pattern + ResolveGoType unit tests All 36 tests pass (go test ./generator/). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Co-authored-by: multica-agent <github@multica.ai>
WalkthroughThis PR introduces a centralized type mapping system for the WebView2 code generator, enabling proper handling of string arrays and improving parameter marshalling. A new ChangesType Mapping and Parameter Generation
🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.1)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
webview2/scripts/generator/types/param.go (1)
150-158: ⚡ Quick winPrefix-string type dispatch duplicates what
typemap.gowas meant to centralize.The new branch uses
strings.HasPrefix(goType, "int")/"uint"/"float"/== "bool"to decide pass-by-value vs pass-by-address. Functionally this is correct (anduintptrhappens to also match"uint", soHRESULTis correctly passed by value), but it re-encodes the same per-IDL-type knowledge thattypes.TypePatterns/ResolveGoTypewas introduced to own.As a follow-up phase, consider driving the vtable-arg expression from the same table — e.g. a
VtableArgresolver keyed on(idlType, pointer, direction)— so adding a new pattern is a single-table edit rather than two places to keep in sync. Not blocking for this PR, but worth doing before the table grows further.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview2/scripts/generator/types/param.go` around lines 150 - 158, The current prefix-based dispatch in param.go (using goType, strings.HasPrefix checks and setting p.VtableCallInput = "uintptr("+variableName+")") duplicates logic that belongs in the centralized type mapping (types.TypePatterns / ResolveGoType); update param handling to consult the existing type-resolution table instead of using HasPrefix logic: replace the conditional branch that inspects goType and assigns p.VtableCallInput with a call into the type-mapping layer (e.g., add/extend a resolver on types.TypePatterns or ResolveGoType to return a VtableArg kind based on (idlType, pointer, direction) and use that result to set p.VtableCallInput), keeping references to the same symbols (p.VtableCallInput, variableName, goType) so behavior is unchanged while delegating decision logic to the centralized table.webview2/scripts/generator/types/typemap.go (1)
17-24: ⚡ Quick winDocstrings overstate current integration with
param.go.Two related inconsistencies that will mislead future readers:
- Line 18:
TypePatternsis described as the "authoritative reference for param.go + template behaviour", butparam.godoes not import or consultTypePatterns— it still relies onIdlTypeToGoTypeplus inlineLPCWSTR/LPWSTRoverrides and prefix string checks.- Line 143-144:
ResolveGoTypeis described as the "single-source-of-truth called from Param.Process()" — it is not called byParam.Process()in this PR; the only caller istypemap_test.go.Additionally,
ResolveGoTypehandlesHRESULT,UINT,INT64,BYTE, andIUnknown, none of which appear inTypePatterns, so the table is not actually exhaustive of the patterns the code knows about.Either wire
ResolveGoTypeintoParam.Process()as part of this PR, or soften the docstrings to make clear this is the target state for a later phase (e.g., "will replace direction-specific logic in Param.Process() in a follow-up phase").Also applies to: 142-145
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview2/scripts/generator/types/typemap.go` around lines 17 - 24, The docstrings for TypePatterns and ResolveGoType overstate current integration: TypePatterns is not consulted by param.go (Param.Process still uses IdlTypeToGoType and inline LPCWSTR/LPWSTR logic) and ResolveGoType is not invoked by Param.Process (only used in typemap_test.go) and also covers types not listed in TypePatterns; either (A) change Param.Process to call ResolveGoType (replace its direction-specific logic and LPCWSTR/LPWSTR overrides so Param.Process uses ResolveGoType as the single source of truth) or (B) soften the comments in typemap.go to accurately state this is the intended target state (e.g., “intended to replace direction-specific logic in Param.Process in a future change”) and update the TypePatterns doc to note it is not exhaustive; reference the symbols TypePatterns, ResolveGoType, Param.Process, IdlTypeToGoType, LPCWSTR/LPWSTR when making the change.webview2/scripts/generator/idl_parse_test.go (1)
84-84: 💤 Low valueUse
assert.Lenfor length assertions.testify exposes
assert.Len/require.Len, which produces clearer failure messages (e.g., includes the slice contents) thanassert.Equal(t, N, len(x)).♻️ Suggested change
- assert.Equal(t, 58, len(iface.Methods)) + assert.Len(t, iface.Methods, 58)- assert.Equal(t, 27, len(chain), "chain should have 27 entries (_27 through base)") + assert.Len(t, chain, 27, "chain should have 27 entries (_27 through base)")Also applies to: 115-115
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview2/scripts/generator/idl_parse_test.go` at line 84, Replace length comparisons that use assert.Equal with assert.Len for clearer failure output: change assert.Equal(t, 58, len(iface.Methods)) to assert.Len(t, iface.Methods, 58) (referencing iface.Methods) and similarly update the other occurrence at the second assertion (line ~115) to use assert.Len with the slice/collection variable in that assertion. Ensure the expected value is the third argument and the collection is passed as the second argument to assert.Len.webview2/scripts/generator/typemap_test.go (1)
265-265: ⚡ Quick winRemove unnecessary loop variable capture.
The
tc := tcassignment is typically used witht.Parallel()to capture the loop variable for concurrent subtests, but this test doesn't run in parallel. You can safely remove this line.♻️ Simplify the loop
for _, tc := range cases { - tc := tc name := strings.Join([]string{tc.direction, tc.idlType, tc.pointer}, "_")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@webview2/scripts/generator/typemap_test.go` at line 265, Remove the unnecessary loop variable capture `tc := tc` in the test loop inside typemap_test.go; locate the test loop where each iteration creates `tc` (used to construct subtests) and simply use the existing loop variable `tc` directly (no reassignment) since the test does not call `t.Parallel()`. This eliminates the redundant assignment without changing test behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@webview2/scripts/generator/testfiles/ICoreWebView2CustomSchemeRegistration.go.txt`:
- Around line 25-45: The SetAllowedOrigins implementation can read out-of-bounds
because allowedOriginsCount is independent of len(allowedOrigins); add a runtime
guard at the start of ICoreWebView2CustomSchemeRegistration.SetAllowedOrigins
that verifies int(allowedOriginsCount) == len(allowedOrigins) and returns an
error (use errors.New) if they differ, before converting the slice and calling
i.Vtbl.SetAllowedOrigins.Call; this prevents mismatched count vs slice-length
and keeps the existing signature and subsequent usage of _allowedOriginsptrs and
i.Vtbl.SetAllowedOrigins.Call intact.
In `@webview2/scripts/generator/typemap_test.go`:
- Line 9: The test imports the wrong module path ("updater/generator/types");
update the import to point to the types package within this module by replacing
"updater/generator/types" with the correct module path
"github.com/wailsapp/wails/webview2/scripts/generator/types" so the test
references the local webview2 types package.
---
Nitpick comments:
In `@webview2/scripts/generator/idl_parse_test.go`:
- Line 84: Replace length comparisons that use assert.Equal with assert.Len for
clearer failure output: change assert.Equal(t, 58, len(iface.Methods)) to
assert.Len(t, iface.Methods, 58) (referencing iface.Methods) and similarly
update the other occurrence at the second assertion (line ~115) to use
assert.Len with the slice/collection variable in that assertion. Ensure the
expected value is the third argument and the collection is passed as the second
argument to assert.Len.
In `@webview2/scripts/generator/typemap_test.go`:
- Line 265: Remove the unnecessary loop variable capture `tc := tc` in the test
loop inside typemap_test.go; locate the test loop where each iteration creates
`tc` (used to construct subtests) and simply use the existing loop variable `tc`
directly (no reassignment) since the test does not call `t.Parallel()`. This
eliminates the redundant assignment without changing test behavior.
In `@webview2/scripts/generator/types/param.go`:
- Around line 150-158: The current prefix-based dispatch in param.go (using
goType, strings.HasPrefix checks and setting p.VtableCallInput =
"uintptr("+variableName+")") duplicates logic that belongs in the centralized
type mapping (types.TypePatterns / ResolveGoType); update param handling to
consult the existing type-resolution table instead of using HasPrefix logic:
replace the conditional branch that inspects goType and assigns
p.VtableCallInput with a call into the type-mapping layer (e.g., add/extend a
resolver on types.TypePatterns or ResolveGoType to return a VtableArg kind based
on (idlType, pointer, direction) and use that result to set p.VtableCallInput),
keeping references to the same symbols (p.VtableCallInput, variableName, goType)
so behavior is unchanged while delegating decision logic to the centralized
table.
In `@webview2/scripts/generator/types/typemap.go`:
- Around line 17-24: The docstrings for TypePatterns and ResolveGoType overstate
current integration: TypePatterns is not consulted by param.go (Param.Process
still uses IdlTypeToGoType and inline LPCWSTR/LPWSTR logic) and ResolveGoType is
not invoked by Param.Process (only used in typemap_test.go) and also covers
types not listed in TypePatterns; either (A) change Param.Process to call
ResolveGoType (replace its direction-specific logic and LPCWSTR/LPWSTR overrides
so Param.Process uses ResolveGoType as the single source of truth) or (B) soften
the comments in typemap.go to accurately state this is the intended target state
(e.g., “intended to replace direction-specific logic in Param.Process in a
future change”) and update the TypePatterns doc to note it is not exhaustive;
reference the symbols TypePatterns, ResolveGoType, Param.Process,
IdlTypeToGoType, LPCWSTR/LPWSTR when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12ca571a-a356-43aa-871a-bb4c88c59871
📒 Files selected for processing (9)
webview2/scripts/generator/idl_parse_test.gowebview2/scripts/generator/testfiles/ICoreWebView2CustomSchemeRegistration.go.txtwebview2/scripts/generator/testfiles/ICoreWebView2FrameInfo.go.txtwebview2/scripts/generator/testfiles/ICoreWebView2ProcessFailedEventArgs2.go.txtwebview2/scripts/generator/typemap_test.gowebview2/scripts/generator/types/param.gowebview2/scripts/generator/types/templates/inputBoolSetup.tmplwebview2/scripts/generator/types/templates/inputStringArraySetup.tmplwebview2/scripts/generator/types/typemap.go
| func (i *ICoreWebView2CustomSchemeRegistration) SetAllowedOrigins(allowedOriginsCount uint32, allowedOrigins []string) error { | ||
|
|
||
| // Convert []string to COM string array (LPCWSTR*) | ||
| _allowedOriginsptrs := make([]*uint16, len(allowedOrigins)) | ||
| for _i, _s := range allowedOrigins { | ||
| _p, err := UTF16PtrFromString(_s) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _allowedOriginsptrs[_i] = _p | ||
| } | ||
| var _allowedOrigins **uint16 | ||
| if len(_allowedOriginsptrs) > 0 { | ||
| _allowedOrigins = &_allowedOriginsptrs[0] | ||
| } | ||
|
|
||
| hr, _, err := i.Vtbl.SetAllowedOrigins.Call( | ||
| uintptr(unsafe.Pointer(i)), | ||
| uintptr(unsafe.Pointer(&allowedOriginsCount)), | ||
| uintptr(allowedOriginsCount), | ||
| uintptr(unsafe.Pointer(_allowedOrigins)), | ||
| ) |
There was a problem hiding this comment.
allowedOriginsCount is decoupled from len(allowedOrigins) — potential out-of-bounds read in COM.
The generated signature exposes both the size_is(...) count and the slice. A caller can pass allowedOriginsCount = 10 with a 3-element slice; the COM callee will then walk past the end of _allowedOriginsptrs, reading uninitialised / freed memory. Symmetrically, passing a count smaller than the slice silently truncates.
Two options for the generator (Phase 2 candidates):
- Drop the separate count parameter for
size_is(...) LPCWSTR*inputs and passuint32(len(allowedOrigins))to the vtable internally — caller-side this is the idiomatic Go API. - Or, if the count must remain in the signature, generate a runtime guard, e.g.
if int(allowedOriginsCount) != len(allowedOrigins) { return errors.New(...) }, before the call.
Not strictly a regression introduced by this PR (the parameter existed before), but the new []string mapping makes the inconsistency much easier to hit accidentally because the two halves of the array contract are now expressed in different units (count vs Go slice).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@webview2/scripts/generator/testfiles/ICoreWebView2CustomSchemeRegistration.go.txt`
around lines 25 - 45, The SetAllowedOrigins implementation can read
out-of-bounds because allowedOriginsCount is independent of len(allowedOrigins);
add a runtime guard at the start of
ICoreWebView2CustomSchemeRegistration.SetAllowedOrigins that verifies
int(allowedOriginsCount) == len(allowedOrigins) and returns an error (use
errors.New) if they differ, before converting the slice and calling
i.Vtbl.SetAllowedOrigins.Call; this prevents mismatched count vs slice-length
and keeps the existing signature and subsequent usage of _allowedOriginsptrs and
i.Vtbl.SetAllowedOrigins.Call intact.
| import ( | ||
| "strings" | ||
| "testing" | ||
| "updater/generator/types" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify go.mod and that the types package import path is correct
# Find and display the go.mod file that governs this test file
fd -t f '^go.mod$' webview2 --exec cat {} \;
echo "---"
echo "Checking if types package exists at the expected location:"
fd -t f 'typemap.go' webview2/scripts/generator/types
echo "---"
echo "Checking for other go files that import the types package:"
rg -n "import.*generator/types" webview2/scripts/generator --type goRepository: wailsapp/wails
Length of output: 1162
Fix the import path that references the wrong module.
The import "updater/generator/types" on line 9 is incorrect. The test file is part of the github.com/wailsapp/wails/webview2 module, but the types package also exists in the same webview2 module at webview2/scripts/generator/types/. The import should reference the webview2 module, not the separate updater module. Change the import to the correct module path for the types package location.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@webview2/scripts/generator/typemap_test.go` at line 9, The test imports the
wrong module path ("updater/generator/types"); update the import to point to the
types package within this module by replacing "updater/generator/types" with the
correct module path "github.com/wailsapp/wails/webview2/scripts/generator/types"
so the test references the local webview2 types package.
There was a problem hiding this comment.
Pull request overview
This PR is the first phase of a WebView2 IDL parser/type-system rewrite, focusing on correcting how IDL parameter types are mapped into Go method signatures and how vtable call arguments are constructed, while adding broad unit test coverage for both parsing and type-mapping behavior.
Changes:
- Fixes several vtable argument generation bugs (notably string
[out, retval]pointer handling, scalar inputs passed by address, and COMBOOLinput handling via a new setup template). - Adds support for mapping
[in] LPCWSTR*to[]stringvia a newinputStringArraySetup.tmpl. - Adds comprehensive tests: full-IDL parse verification across 6 cached IDL files and a pattern-driven typemap test suite.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| webview2/scripts/generator/types/typemap.go | Adds a documented typemap table and ResolveGoType() helper. |
| webview2/scripts/generator/types/templates/inputStringArraySetup.tmpl | New template to marshal []string into LPCWSTR* for vtable calls. |
| webview2/scripts/generator/types/templates/inputBoolSetup.tmpl | New template to convert Go bool into COM BOOL (int32). |
| webview2/scripts/generator/types/param.go | Updates Go type inference and vtable argument generation, plus new setup paths for []string and bool. |
| webview2/scripts/generator/typemap_test.go | Adds pattern-based generator output tests and ResolveGoType() unit tests. |
| webview2/scripts/generator/testfiles/ICoreWebView2ProcessFailedEventArgs2.go.txt | Updates expected generated output for [out,retval] int* fix (pass address). |
| webview2/scripts/generator/testfiles/ICoreWebView2FrameInfo.go.txt | Updates expected generated output for [out,retval] LPWSTR* fixes (pass address-of local). |
| webview2/scripts/generator/testfiles/ICoreWebView2CustomSchemeRegistration.go.txt | Updates expected output for [in] LPCWSTR* → []string and [in] UINT32 passing by value. |
| webview2/scripts/generator/idl_parse_test.go | Adds parsing tests for all cached IDL files and inheritance chain validation. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Scalar numeric / bool inputs: use GoType (handles uppercase IDL names like UINT32, | ||
| // INT32, BOOL that map to Go uint32, int32, bool). For bool, setup code converts to | ||
| // int32 first so the local variable is already an int32 and uintptr() is correct. | ||
| goType := p.GoType | ||
| if strings.HasPrefix(goType, "int") || strings.HasPrefix(goType, "uint") || | ||
| goType == "float32" || goType == "float64" || goType == "bool" { | ||
| p.VtableCallInput = "uintptr(" + variableName + ")" | ||
| return | ||
| } |
| // String types: direction determines whether to pass pointer-to-pointer or pointer. | ||
| // For output LPWSTR* the local var is *uint16; pass &local so COM writes the pointer back. | ||
| // For input LPWSTR/LPCWSTR (plain or array) pass the *uint16 / **uint16 directly. | ||
| switch p.Type { | ||
| case "LPCWSTR", "LPWSTR": | ||
| p.VtableCallInput = "uintptr(unsafe.Pointer(" + variableName + "))" | ||
| if p.IsOutputParam() { | ||
| p.VtableCallInput = "uintptr(unsafe.Pointer(&" + variableName + "))" | ||
| } else { | ||
| p.VtableCallInput = "uintptr(unsafe.Pointer(" + variableName + "))" | ||
| } | ||
| return |
| // The outer pointer is stripped; the Go type is *string. | ||
| assert.Contains(t, body, "origins *string") | ||
| assert.Contains(t, body, "uintptr(unsafe.Pointer(&origins))") |
| // ── Pattern 14: [in] double ────────────────────────────────────────────────── | ||
|
|
||
| func TestTypePattern_InDouble(t *testing.T) { | ||
| body := generatedBody(t, wrapIDL( | ||
| `HRESULT SetZoom([in] double factor);`, | ||
| )) | ||
| assert.Contains(t, body, "SetZoom(factor float64) error") | ||
| assert.Contains(t, body, "uintptr(factor)") | ||
| } |
| // and direction. It is the single-source-of-truth called from Param.Process(). | ||
| // direction should be "in" or "out". |
| VtableArg: "uintptr(param)", | ||
| Notes: "Scalar passed by value.", |
| IDL: "[out] LPWSTR** param", | ||
| GoType: "*string", | ||
| VtableArg: "uintptr(unsafe.Pointer(¶m))", | ||
| Notes: "Double-pointer COM string array output; caller must CoTaskMemFree each element and the array itself.", | ||
| }, |
Summary
Phase 1 of the IDL parser / type-system rewrite (WAI-290).
Bugs fixed
[out,retval] LPWSTR* nameuintptr(unsafe.Pointer(_name))— passes niluintptr(unsafe.Pointer(&_name))— passes address so COM can write back[in] UINT32 countuintptr(unsafe.Pointer(&count))— passes stack addressuintptr(count)— passes value[out,retval] int* exitCodeuintptr(exitCode)== 0uintptr(unsafe.Pointer(&exitCode))[in] BOOL valueuintptr(unsafe.Pointer(&value))— 1-byte Go bool pointeruintptr(_value)where_value int32from newinputBoolSetup.tmplRoot cause:
processVtableCallInput()used the IDL type name (uppercaseUINT32) for numeric detection, checked case-insensitively for "uint", and did not distinguish input from output direction before applying numeric shortcuts.New capability
[in] LPCWSTR*now maps to[]string(C string array) viainputStringArraySetup.tmplwhich converts
[]string → []*uint16before the vtable call.New files
types/typemap.goTypePatternstable (17 patterns) +ResolveGoType()types/templates/inputBoolSetup.tmpl[in] BOOLtypes/templates/inputStringArraySetup.tmpl[]string→**uint16for[in] LPCWSTR*generator/idl_parse_test.gogenerator/typemap_test.goResolveGoTypeunit testsTest results
Acceptance criteria
CC @leaanthony
Summary by CodeRabbit
Tests
Chores