fix: External traversal for raw WGSL implemented functions#2648
fix: External traversal for raw WGSL implemented functions#2648aleksanderkatan wants to merge 40 commits into
Conversation
|
@pullfrog review |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new in externalMap check on packages/typegpu/src/core/resolve/externals.ts:98 walks the prototype chain, so WGSL identifiers like toString, constructor, or valueOf are incorrectly treated as missing externals and trigger spurious warnings. See the inline comment for the one-line fix.
Reviewed changes — a rewrite of replaceExternalsInWgsl that resolves external property chains in a single regex pass instead of recursing per external key.
- Single-pass external resolution —
replaceExternalsInWgslnow finds all bounded identifier chains, walks them until a resolvable value is found, and caches resolved values. - Removed unused-external warning — the call to warn when an external key isn't used has been removed.
- Test and snapshot updates —
declare.test.ts,resolve.test.ts, and several console-log snapshots were updated for the new resolution ordering.
openrouter/moonshotai/kimi-k2.7-code (free via Pullfrog for OSS) ｜ 𝕏
Co-authored-by: pullfrog[bot] <226033991+pullfrog[bot]@users.noreply.github.com>
There was a problem hiding this comment.
ℹ️ One behavioral change worth confirming — details inline.
Reviewed changes — rewrote raw-WGSL external resolution to traverse identifier chains in a single scan and resolve at the first resolvable link.
- Rewrite
replaceExternalsInWgsl— uses one regex scan over WGSL, walks each identifier chain through the external map, and resolves at the first resolvable value or warns when the chain ends without one. - Remove the unused-external warning — the old
The external 'X' wasn't used...console warning is dropped because externals may be applied broadly across functions. - Emit resolved WGSL in source order — inline snapshots updated to reflect declarations appearing in the order they are referenced rather than in
$usesinsertion order. - Update
tgpu.declaretests — declarations are now explicitly referenced in templates; unreferenced declarations are no longer injected.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
…l-for-raw-wgsl-implemented-functions
…l-for-raw-wgsl-implemented-functions
There was a problem hiding this comment.
Pull request overview
This PR updates how tgpu.resolve traverses and replaces externals in raw WGSL/template code by detecting full property chains that start with external keys, then following the chain until a resolvable is found (or the chain ends), which also changes the ordering of resolved WGSL snippets to match first appearance in code.
Changes:
- Reworked external replacement to scan the WGSL once for bounded identifier/property chains and resolve externals based on chain traversal.
- Adjusted warning behavior/tests to cover “invalid external” and “chain ends without resolvable” scenarios, and removed warnings for unused externals.
- Updated multiple inline snapshots due to the new resolution/injection ordering.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/src/core/resolve/externals.ts | Implements new property-chain-based external traversal and replacement logic. |
| packages/typegpu/tests/tgsl/consoleLog.test.ts | Updates expected WGSL ordering in snapshots (e.g., dataBlockIndex placement). |
| packages/typegpu/tests/tgpuGenericFn.test.ts | Updates expected WGSL ordering in snapshot output. |
| packages/typegpu/tests/resolve.test.ts | Updates ordering snapshot and rewrites warning tests for new traversal behavior. |
| packages/typegpu/tests/declare.test.ts | Updates declaration tests to explicitly reference declarations in templates and adjusts nested-declare behavior. |
| apps/typegpu-docs/tests/individual-example-tests/log-test.test.ts | Updates expected WGSL ordering in snapshots for docs examples. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (typeof currentItem !== 'object' || currentItem === null || i === chain.length - 1) { | ||
| console.warn( | ||
| `During resolution, the external '${chain.slice(0, i + 1).join('.')}' has been omitted. Only TGPU resources, 'use gpu' functions, primitives, and plain JS objects can be used as externals.`, | ||
| ); | ||
| return match; |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — removed the redundant local resolution cache in replaceExternalsInWgsl and synced the branch with origin/main.
- Removed the local resolution cache —
replaceExternalsInWgslno longer maintains its ownMap<unknown, string>becausectx.resolvealready memoizes results throughResolutionCtx._getOrInstantiate/NamespaceInternal.memoizedResolves. - Synced with
origin/main— merged the latest base branch changes into the PR branch.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
…l-for-raw-wgsl-implemented-functions
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — since the last review, the identifier-boundary regex was updated to use Unicode character classes and external keys are now validated to exclude dots.
- Fixed Unicode identifier boundaries in
boundedPropChain— boundary assertions now use\p{XID_Continue}instead of ASCII\w, matching the Unicode identifier body used byanyIdent. - Added validation for external keys —
replaceExternalsInWgslnow throws when an external map key contains., enforcing the assumption documented in the JSDoc. - Synced with
origin/main— merged the latest base branch changes into the PR branch.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
| const anyIdent = /([$_\p{XID_Start}][$\p{XID_Continue}]*)/u; // WGSL ident, modified to include $ | ||
| const anyPropChain = new RegExp(`(${anyIdent.source})(\\.${anyIdent.source})*`, 'ug'); | ||
| const boundedPropChain = new RegExp( | ||
| `(?<![\\p{XID_Continue}\\$.])${anyPropChain.source}(?![\\p{XID_Continue}\\$])`, | ||
| 'ug', | ||
| ); |
There was a problem hiding this comment.
This seems to be a hallucination. Nevertheless, I would add some internal tests for this Regex to verify that it in fact works.
| if (typeof currentItem !== 'object' || currentItem === null || i === chain.length - 1) { | ||
| console.warn( | ||
| `During resolution, the external '${chain.slice(0, i + 1).join('.')}' has been omitted. Only TGPU resources, 'use gpu' functions, primitives, and plain JS objects can be used as externals.`, | ||
| ); | ||
| return match; | ||
| } |
…l-for-raw-wgsl-implemented-functions
| expect(matches(' 1.0 ', boundedPropChain)).toMatchInlineSnapshot(`""`); | ||
| expect(matches(' .5f ', boundedPropChain)).toMatchInlineSnapshot(`""`); | ||
| expect(matches(' .prop ', boundedPropChain)).toMatchInlineSnapshot(`""`); | ||
| expect(matches(' p1 . p2 ', boundedPropChain)).toMatchInlineSnapshot(`"p1 p2"`); |
There was a problem hiding this comment.
Is this how it should be?
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — exported the WGSL identifier/property-chain regexes for direct unit testing and added focused regex coverage in packages/typegpu/tests/internal/externals.test.ts.
- Export
anyIdentandboundedPropChainfrompackages/typegpu/src/core/resolve/externals.tsso they can be asserted without going through full resolution. - Add regex unit tests covering WGSL identifiers (Unicode start/continue, dollar signs, boundary stops) and bounded property chains (deep chains, numeric literals, call/array expressions, comma-separated identifiers).
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Before: given an external, if resolvable then replace occurrences with resolved, otherwise look which keys appear in the code with a prop following them, and call recursively with
external.prop.Now: given externals, look for prop chains starting with external keys, then follow them until either a resolvable appears, or the chain ends.
Other changes: