feat: Name collision avoidance in raw-WGSL implemented functions - #2939
feat: Name collision avoidance in raw-WGSL implemented functions#2939iwoplaza wants to merge 1 commit into
Conversation
|
pkg.pr.new packages benchmark commit |
Bundle size comparison (
|
| 🟢 Decreased (max -53.64%) | ➖ Unchanged | 🔴 Increased (max 1.31%) | ❔ Unknown |
|---|---|---|---|
| 65 | 234 | 26 | 0 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
Click to reveal the results table (88 entries).
| Test | tsdown |
|---|---|
| common_fullScreenTriangle.ts | 42.87 kB ( |
| STATIC_common.ts | 59.63 kB ( |
| STATIC_tgpu.ts | 275.22 kB ( |
| tgpu_fn.ts | 275.22 kB ( |
| tgpu_init.ts | 275.22 kB ( |
| tgpu_lazy.ts | 275.22 kB ( |
| tgpu_slot.ts | 275.22 kB ( |
| tgpu_const.ts | 275.22 kB ( |
| tgpu_unroll.ts | 275.22 kB ( |
| tgpu_resolve.ts | 275.23 kB ( |
| tgpu_accessor.ts | 275.23 kB ( |
| tgpu_comptime.ts | 275.23 kB ( |
| tgpu_vertexFn.ts | 275.23 kB ( |
| tgpu_computeFn.ts | 275.23 kB ( |
| tgpu_fragmentFn.ts | 275.23 kB ( |
| tgpu_privateVar.ts | 275.23 kB ( |
| tgpu_vertexLayout.ts | 275.23 kB ( |
| tgpu_workgroupVar.ts | 275.23 kB ( |
| tgpu_initFromDevice.ts | 275.23 kB ( |
| tgpu_bindGroupLayout.ts | 275.23 kB ( |
| tgpu_mutableAccessor.ts | 275.23 kB ( |
| tgpu_resolveWithContext.ts | 275.24 kB ( |
| STATIC_allImports.ts | 301.47 kB ( |
| STATIC_std.ts | 95.54 kB ( |
| STATIC_d.ts | 75.79 kB ( |
| std_abs.ts | 54.27 kB ( |
| std_acos.ts | 54.27 kB ( |
| std_acosh.ts | 54.27 kB ( |
| std_asin.ts | 54.27 kB ( |
| std_asinh.ts | 54.27 kB ( |
| std_atan.ts | 54.27 kB ( |
| std_atan2.ts | 54.27 kB ( |
| std_atanh.ts | 54.27 kB ( |
| std_ceil.ts | 54.27 kB ( |
| std_clamp.ts | 54.27 kB ( |
| std_cos.ts | 54.27 kB ( |
| std_cosh.ts | 54.27 kB ( |
| std_countLeadingZeros.ts | 54.27 kB ( |
| std_countOneBits.ts | 54.27 kB ( |
| std_countTrailingZeros.ts | 54.27 kB ( |
| std_cross.ts | 54.27 kB ( |
| std_degrees.ts | 54.27 kB ( |
| std_determinant.ts | 54.27 kB ( |
| std_dot4I8Packed.ts | 54.27 kB ( |
| std_exp.ts | 54.27 kB ( |
| std_exp2.ts | 54.27 kB ( |
| std_extractBits.ts | 54.27 kB ( |
| std_faceForward.ts | 54.27 kB ( |
| std_firstLeadingBit.ts | 54.27 kB ( |
| std_firstTrailingBit.ts | 54.27 kB ( |
| std_floor.ts | 54.27 kB ( |
| std_fma.ts | 54.27 kB ( |
| std_insertBits.ts | 54.27 kB ( |
| std_intdiv.ts | 54.27 kB ( |
| std_inverseSqrt.ts | 54.27 kB ( |
| std_ldexp.ts | 54.27 kB ( |
| std_log.ts | 54.27 kB ( |
| std_log2.ts | 54.27 kB ( |
| std_max.ts | 54.27 kB ( |
| std_min.ts | 54.27 kB ( |
| std_normalize.ts | 54.27 kB ( |
| std_pow.ts | 54.27 kB ( |
| std_quantizeToF16.ts | 54.27 kB ( |
| std_radians.ts | 54.27 kB ( |
| std_reflect.ts | 54.27 kB ( |
| std_refract.ts | 54.27 kB ( |
| std_reverseBits.ts | 54.27 kB ( |
| std_round.ts | 54.27 kB ( |
| std_saturate.ts | 54.27 kB ( |
| std_sign.ts | 54.27 kB ( |
| std_sin.ts | 54.27 kB ( |
| std_sinh.ts | 54.27 kB ( |
| std_smoothstep.ts | 54.27 kB ( |
| std_sqrt.ts | 54.27 kB ( |
| std_step.ts | 54.27 kB ( |
| std_tan.ts | 54.27 kB ( |
| std_tanh.ts | 54.27 kB ( |
| std_transpose.ts | 54.27 kB ( |
| std_trunc.ts | 54.27 kB ( |
| std_distance.ts | 54.26 kB ( |
| std_dot4U8Packed.ts | 54.26 kB ( |
| std_fract.ts | 54.26 kB ( |
| std_frexp.ts | 54.26 kB ( |
| std_mix.ts | 54.26 kB ( |
| std_modf.ts | 54.26 kB ( |
| std_dot.ts | 54.26 kB ( |
| std_length.ts | 54.26 kB ( |
| d_struct.ts | 1.72 kB ( |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 265.10 kB ( |
| tgpu_initFromDevice.ts | 264.56 kB ( |
| tgpu_resolve.ts | 164.27 kB ( |
| tgpu_resolveWithContext.ts | 164.21 kB ( |
| tgpu_bindGroupLayout.ts | 60.24 kB ( |
| tgpu_mutableAccessor.ts | 54.96 kB ( |
| tgpu_accessor.ts | 54.95 kB ( |
| tgpu_privateVar.ts | 53.64 kB ( |
| tgpu_workgroupVar.ts | 53.64 kB ( |
| tgpu_const.ts | 53.06 kB ( |
| tgpu_lazy.ts | 52.86 kB ( |
| tgpu_fn.ts | 42.19 kB ( |
| tgpu_fragmentFn.ts | 40.23 kB ( |
| tgpu_vertexFn.ts | 40.04 kB ( |
| tgpu_computeFn.ts | 39.75 kB ( |
| tgpu_vertexLayout.ts | 28.33 kB ( |
| tgpu_comptime.ts | 15.93 kB ( |
| tgpu_unroll.ts | 1.75 kB ( |
| tgpu_slot.ts | 1.70 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
Resolution Time Benchmark---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Random Branching (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [1.00, 2.10, 4.72, 6.89, 8.56, 11.30, 26.29, 28.19]
line [1.06, 2.13, 4.49, 6.79, 7.80, 13.11, 24.32, 25.97]
line [1.00, 2.03, 4.21, 6.72, 7.63, 13.04, 25.03, 25.21]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Linear Recursion (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.29, 0.51, 0.68, 0.83, 1.12, 1.19, 1.42, 1.63]
line [0.28, 0.49, 0.70, 0.86, 1.14, 1.22, 1.44, 1.64]
line [0.34, 0.55, 0.74, 0.86, 1.15, 1.24, 1.47, 1.61]
---
config:
themeVariables:
xyChart:
plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
title "Full Tree (🔴 PR | 🔵 main | 🟢 release)"
x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
y-axis "time (ms)"
line [0.81, 2.13, 3.52, 7.77, 13.60, 26.54, 59.04, 119.52]
line [0.87, 2.21, 3.81, 7.80, 13.19, 27.16, 58.70, 118.46]
line [0.89, 2.16, 4.35, 7.48, 12.69, 26.83, 56.71, 116.50]
|
b4f2910 to
824c2b2
Compare
There was a problem hiding this comment.
Reviewed changes
fnCore.ts— raw-WGSL string implementations now extract identifier-like tokens, rename identifiers that would clash with already-taken names (recording renames inscope.localRenames), reserve the rest, andrenameIdentifiers/replaceExternalsInWgslthe body inside a pushed function+block scope.rawShaderCodeUtils.ts— new module consolidatingblankSpaces/lineBreaks,stripWGSLComments/blankOutWGSLComments,anyIdent,extractIdentifierLikeTokens, andrenameIdentifiers;core/whitespaces.tsand the oldminify.tscopy removed.tgpuRawCodeSnippet.resolve— now rewrites its expression withctx.topFunctionScope.localRenamesbefore external substitution.nameUtils.ts/namespace.ts/wgslGenerator.ts/resolutionCtx.ts—bannedTokens/builtinsmoved out of global seeding intoWgslGenerator.isBannedToken/isBuiltinGlobal, with builtins reserved via_reserveGlobals();isIdentifierTakenconsultsgen.isBannedToken.validatePropnow takesctx; struct-prop validation moved to resolve time inresolveData.ts.- Tests:
blankOutWGSLComments,extractIdentifiers,renameIdentifiersinternals,rawFnname-clash cases,rawCodeSnippetrename-through-owner, plus updated struct/renderPipeline/minify snapshots.
The raw-WGSL side of this is coherent and well-tested, but the JS-snippet extension has a correctness gap that depends on resolution order. One functional issue below.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
| let expression = this.#expression; | ||
| if (ctx.topFunctionScope) { | ||
| expression = renameIdentifiers(expression, ctx.topFunctionScope.localRenames); | ||
| } |
There was a problem hiding this comment.
"should rename references to local variables" only holds when another raw-WGSL function in the same resolution already produced the exact {ARG: newName} entry. On its own, a JS-implemented function whose argument clashes with a global and that uses a snippet referencing that argument emits the wrong references.
localRenames is populated only in fnCore.ts's raw-string branch (line ~170). The tinyest AST path (resolveFunction in resolutionCtx.ts) renames function arguments via makeUniqueIdentifier but never records them into scope.localRenames, so a snippet resolved inside that JS function reads an empty rename map.
Reproduced (this is exactly the intended bullet-4 use case minus the sibling raw fn): with const constant = tgpu.const(d.f32,123).$name('a') and jsFn = tgpu.fn([d.f32], d.f32)((a) => { 'use gpu'; return raw('a * 2 + constant').$uses({constant}) }) in a tree with only jsFn, resolution yields:
fn jsFn(a_1: f32) -> f32 {
return a * 2 + a; // first `a` silently binds to the global constant, not the arg (renamed to `a_1`)
}Adding a sibling raw-WGSL fn that consumes the same snippet concurrently flips jsFn to the intended return a_1 * 2 + a; — so the emitted shader is order-dependent and, in the standalone case, wrong by silently reading a same-named global instead of the function's argument. The PR's added test only exercises the favorable ordering.
There was a problem hiding this comment.
Pull request overview
This PR improves WGSL name-collision handling for raw-WGSL implementations and raw code snippets by tracking/avoiding identifier clashes (including reserved/builtin names), and it updates error reporting snapshots to reflect the newer resolution trace format.
Changes:
- Reserve/rename identifiers in raw-WGSL function bodies to avoid collisions with existing declarations and WGSL reserved/builtin identifiers.
- Introduce
rawShaderCodeUtilsutilities (comment stripping/blanking, identifier extraction, identifier renaming) and reuse them in minification/externals parsing. - Update/extend tests for name-clash avoidance, raw snippet behavior, and updated error trace formatting.
Reviewed changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/tests/tgsl/wgslGenerator.test.ts | Updates error snapshots to include resolution trace context. |
| packages/typegpu/tests/tgsl/rawCodeSnippet.test.ts | Adds coverage for raw snippet identifier renaming across owner-function renames (WGSL + JS). |
| packages/typegpu/tests/struct.test.ts | Adjusts struct invalid-name tests to go through tgpu.resolve and new trace formatting. |
| packages/typegpu/tests/renderPipeline.test.ts | Updates error trace snapshots to reflect deeper struct/auto-struct trace. |
| packages/typegpu/tests/rawFn.test.ts | Adds name-clash avoidance tests and normalizes some formatting. |
| packages/typegpu/tests/minification.test.ts | Updates minification error snapshots to match new error/trace behavior. |
| packages/typegpu/tests/internal/renameIdentifiers.test.ts | New unit tests for identifier renaming logic. |
| packages/typegpu/tests/internal/extractIdentifiers.test.ts | New unit tests for identifier-like token extraction. |
| packages/typegpu/tests/internal/externals.test.ts | Adjusts externals parsing tests and relocates anyIdent usage. |
| packages/typegpu/tests/internal/blankOutWGSLComments.test.ts | New unit tests for comment blanking utility. |
| packages/typegpu/src/types.ts | Adds localRenames tracking to function scope; removes isIdentifierBanned from ResolutionCtx. |
| packages/typegpu/src/tgsl/wgslGenerator.ts | Reserves builtin globals, centralizes banned/builtin token sets, and exposes generator queries. |
| packages/typegpu/src/tgsl/shaderGenerator.ts | Extends ShaderGenerator interface with isBannedToken / isBuiltinGlobal. |
| packages/typegpu/src/resolutionCtx.ts | Routes banned-token checks through the generator and initializes localRenames in function scopes. |
| packages/typegpu/src/rawShaderCodeUtils.ts | New shared utilities for WGSL comment handling and identifier extraction/renaming. |
| packages/typegpu/src/nameUtils.ts | Moves reserved-keyword checks to generator and updates validateProp to require ctx. |
| packages/typegpu/src/minify.ts | Reuses shared comment stripping/whitespace definitions from rawShaderCodeUtils. |
| packages/typegpu/src/data/struct.ts | Removes eager property-name validation at struct creation time. |
| packages/typegpu/src/data/autoStruct.ts | Removes eager property-name validation from auto-struct property allocation. |
| packages/typegpu/src/core/whitespaces.ts | Removes old whitespace constants (moved to rawShaderCodeUtils). |
| packages/typegpu/src/core/resolve/resolveData.ts | Validates struct property names during resolution (now has access to ctx). |
| packages/typegpu/src/core/resolve/namespace.ts | Stops pre-seeding global namespace with builtins/keywords (delegated to generator + checks). |
| packages/typegpu/src/core/resolve/externals.ts | Reuses shared anyIdent and keeps bounded prop-chain logic consistent. |
| packages/typegpu/src/core/rawCodeSnippet/tgpuRawCodeSnippet.ts | Applies localRenames to raw snippet expressions before external replacement. |
| packages/typegpu/src/core/function/fnCore.ts | Implements raw-WGSL identifier reservation/renaming and records localRenames. |
| packages/typegpu/src/core/function/extractArgs.ts | Reuses shared whitespace constants from rawShaderCodeUtils. |
Suppressed comments (1)
packages/typegpu/src/rawShaderCodeUtils.ts:169
- Typo in comment: “preceeding” should be “preceding”.
export function renameIdentifiers(_source: string, renames: Map<string, string>) {
// Adding a space at the beginning of `source` so all potential identifiers
// have a preceeding character (see the regex for more context).
const source = ' ' + _source;
const noCommentsSource = blankOutWGSLComments(source);
// Capturing the preceeding character (irrespective of whitespace) to make sure that it's not a
// chained member access, nor a typed numeric literal (e.g. 1f)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| resolve(ctx) { | ||
| const replacedExpression = replaceExternalsInWgsl( | ||
| ctx, | ||
| this.#externals ?? {}, | ||
| this.#expression, | ||
| ); | ||
| let expression = this.#expression; | ||
| if (ctx.topFunctionScope) { | ||
| expression = renameIdentifiers(expression, ctx.topFunctionScope.localRenames); | ||
| } | ||
|
|
||
| const replacedExpression = replaceExternalsInWgsl(ctx, this.#externals ?? {}, expression); |
| /** | ||
| * Reserves all builtin WGSL globals. Can be overriden to change behavior. | ||
| */ |
| export function extractIdentifierLikeTokens(source: string): string[] { | ||
| // Adding a space at the beginning of `source` so all potential identifiers | ||
| // have a preceeding character (see the regex for more context). | ||
| const noCommentsSource = stripWGSLComments(' ' + source); | ||
| // Capturing the preceeding character (irrespective of whitespace) to make sure that it's not a | ||
| // chained member access, nor a typed numeric literal (e.g. 1f) |
824c2b2 to
a8d4034
Compare
There was a problem hiding this comment.
Important
This delta brings the same token-renaming machinery to templates, but it renames WGSL grammar tokens (address spaces, access modes, attribute args, unprotected type names) into invalid WGSL on re-resolution, and the committed namespace.test.ts snapshot is red against the current head. Separately, the JS-implemented-function snippet path — which the PR aims to fix — is now asserted in a test to produce the wrong output.
Reviewed changes
- Split memoized raw-snippet resolutions per owner function by reading a new
parentFunctionNameSlot(internalSlots.ts), set aroundreplaceExternalsInWgslinfnCore.tsand aroundgen.functionDefinitioninresolutionCtx.ts, and consumed viavoid parentFunctionNameSlot.$intgpuRawCodeSnippet.resolve. - Extended name-collision avoidance to
tgpu.resolve({ template, externals })(resolveFromTemplateintgpuResolve.ts): pushes a scratch function + block scope, renames identifiers that clash at global scope, and permanently reserves every other identifier it finds. - Deduplicated
identifiers([...new Set(...)]) and switchedclashingIdentifiers/uniqueIdentifiersfrom sets to arrays infnCore.ts. - Test updates: reordered the rawCodeSnippet rename-through-owner test (
jsFnnow first) and changed itsjsFnsnapshot to bind the snippet's local reference to the global constant; updated thenamespace.test.tsre-resolve snapshot tofoo_1; added rawFn "name clash avoidance" cases.
The parentFunctionNameSlot change is a genuine fix — it stops memoized snippet results from leaking between owner functions. The two issues below regress the delta itself.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
| const renamed = ctx.makeUniqueIdentifier(ident, 'global'); | ||
| scope.localRenames.set(ident, renamed); | ||
| } | ||
| const renamedImpl = renameIdentifiers(template ?? '', scope.localRenames); | ||
| for (const ident of uniqueIdentifiers) { | ||
| ctx.reserveIdentifier(ident, 'global'); | ||
| } |
There was a problem hiding this comment.
Renaming every clashing identifier at permanent global scope mangles WGSL grammar tokens inside templates on re-resolution — the committed namespace.test.ts snapshot (var<private> foo: Boid resolved twice) currently fails because the address-space token itself is rewritten to var<private_1> foo_1: Boid, which is invalid WGSL. private, function, workgroup, read/write/read_write, @location/@builtin(position)/@interpolate(flat) args, and vec3u/ptr/texture_2d/sampler_comparison are in neither bannedTokens nor builtinGlobals, so on any second resolve of the same template in a shared namespace they all get rewritten.
|
|
||
| // Should be just the template, as Boid was already defined in the namespace | ||
| expect(code2).toMatchInlineSnapshot(`"var<private> foo: Boid"`); | ||
| expect(code2).toMatchInlineSnapshot(`"var<private> foo_1: Boid"`); |
There was a problem hiding this comment.
This snapshot is red on the current head: the second tgpu.resolve actually emits var<private_1> foo_1: Boid (run npx vitest run tests/namespace.test.ts to confirm), i.e. the address-space qualifier got renamed too. Updating the expectation to the raw output would paper over invalid WGSL, so the rename logic needs to be fixed first.
| fn jsFn(a_1: f32) -> f32 { | ||
| return a * 2 + a; |
There was a problem hiding this comment.
This snapshot asserts the JS-implemented-function snippet path is broken: jsFn(a_1: f32) returns a * 2 + a, where a silently binds to the global const a: f32 = 123f instead of the renamed argument a_1 — the function ignores its input. The resolveFunction path still doesn't record argument renames into scope.localRenames, so the snippet's a is never rewritten; the parentFunctionNameSlot change removed the accidental cross-function cache correlation that previously masked this, and this test now converts deterministic wrong output into the expected snapshot rather than failing on it.
aleksanderkatan
left a comment
There was a problem hiding this comment.
Nice!
I didn't review the entire thing yet. I leave some cosmetic & complexity requests.
| /** | ||
| * Same as `stripWGSLComments`, but keeps all non-comment code at | ||
| * the same location where it originally was, and replaces the comments | ||
| * with whitespace. | ||
| */ |
There was a problem hiding this comment.
Now there's no need for stripWgslComments, as minification removes whitespace anyways. Please remove it to reduce code duplication
| result = swapChars(result, offset, ' '); | ||
| offset += 2; | ||
| } else { | ||
| result = swapChars(result, offset, ' '); |
There was a problem hiding this comment.
Swapping character by character this way puts way too much trust in JS strings IMO (I would be seriously surprised if this wasn't O(n^2)), can we optimize this? We could either reduce the number of replaces to how many comments they are (instead of how many chars they take), or we could use e.g. TextEncoder & TextDecoder to make this truly linear
| let result: RegExpExecArray | null; | ||
| while ((result = expr.exec(noCommentsSource)) !== null) { | ||
| if (result[0][0] === '.') { | ||
| // Skipping member accesses. | ||
| continue; | ||
| } | ||
|
|
||
| if (result[1]) { | ||
| identifiers.push(result[1]); | ||
| } | ||
| } |
There was a problem hiding this comment.
Wouldn't this be cleaner than a while with an assignment in its test? Am I missing something?
| let result: RegExpExecArray | null; | |
| while ((result = expr.exec(noCommentsSource)) !== null) { | |
| if (result[0][0] === '.') { | |
| // Skipping member accesses. | |
| continue; | |
| } | |
| if (result[1]) { | |
| identifiers.push(result[1]); | |
| } | |
| } | |
| for (const result of noCommentsSource.matchAll(expr)) { | |
| if (result[0][0] === '.') { | |
| // Skipping member accesses. | |
| continue; | |
| } | |
| if (result[1]) { | |
| identifiers.push(result[1]); | |
| } | |
| } |
| const noCommentsSource = blankOutWGSLComments(source); | ||
| // Capturing the preceeding character (irrespective of whitespace) to make sure that it's not a | ||
| // chained member access, nor a typed numeric literal (e.g. 1f) | ||
| const expr = new RegExp(`[^\\d]\\s*${anyIdent.source}`, 'ug'); |
There was a problem hiding this comment.
nit: we don't need to recreate this regex each call, especially when it's used in multiple functions
| let result: RegExpExecArray | null; | ||
| while ((result = expr.exec(noCommentsSource)) !== null) { | ||
| if (result[0][0] === '.') { | ||
| // Skipping member accesses. | ||
| continue; | ||
| } | ||
|
|
||
| const identifier = result[1]?.trim(); | ||
| if (!identifier) { | ||
| continue; | ||
| } | ||
|
|
||
| const end = result.index + result[0].length; | ||
| // counting back from the end to keep any extra whitespace that was there | ||
| const start = end - identifier.length; | ||
| replaced += source.slice(copiedUpTo, start); | ||
| replaced += renames.get(identifier) ?? identifier; | ||
| copiedUpTo = end; | ||
| } | ||
|
|
||
| // Removing the first space we added at the beginning | ||
| return (replaced + source.slice(copiedUpTo)).slice(1); |
There was a problem hiding this comment.
I think this could be done with one singular string.replace.
We already once do something like this in our repo, see the top of functionDefinition in wgslGenerator. Can you check this out?
|
@aleksanderkatan Thanks for the initial review! I'll put it back into draft, I want to approach some things differently to be more robust, and address your comments. |

Changes:
TODO:
localRenamesmapglobalRenamesmap to be used by templated tgpu.resolveitemStateStackpublic, not a part of the internals of the resolution ctx (perhaps another PR)