Skip to content

feat: Name collision avoidance in raw-WGSL implemented functions - #2939

Draft
iwoplaza wants to merge 1 commit into
mainfrom
feat/unique-names-in-wgsl
Draft

feat: Name collision avoidance in raw-WGSL implemented functions#2939
iwoplaza wants to merge 1 commit into
mainfrom
feat/unique-names-in-wgsl

Conversation

@iwoplaza

@iwoplaza iwoplaza commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator

Changes:

  • Reserving local identifiers in raw-WGSL implementations so that subsequent declarations get a different name
  • Renaming local definitions is they clash with something already declared.
  • Non-external identifiers in raw code snippets should be treated as referring to the nearest scope, and they should get renamed if that's what happened in the raw-WGSL implemented function that used the snippet
  • Extend the renaming behavior to uses of raw code snippets in JS-implemented functions
  • Moved prop verification to struct resolution
  • Removed prop verification from auto-structs, they're already verified by the generated struct

TODO:

  • Register renamed arguments and local definitions in localRenames map
  • Consider adding a globalRenames map to be used by templated tgpu.resolve
  • Consider making itemStateStack public, not a part of the internals of the resolution ctx (perhaps another PR)

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

pkg.pr.new

packages
Ready to be installed by your favorite package manager ⬇️

https://pkg.pr.new/software-mansion/TypeGPU/tinyest@a8d4034c194c520f3305a4c8c3a85f399d14e0fc
https://pkg.pr.new/software-mansion/TypeGPU/tinyest-for-wgsl@a8d4034c194c520f3305a4c8c3a85f399d14e0fc
https://pkg.pr.new/software-mansion/TypeGPU/typegpu@a8d4034c194c520f3305a4c8c3a85f399d14e0fc
https://pkg.pr.new/software-mansion/TypeGPU/unplugin-typegpu@a8d4034c194c520f3305a4c8c3a85f399d14e0fc

benchmark
view benchmark

commit
view commit

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

Bundle size comparison (import * as ... in PR vs import * as ... in target):

🟢 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 ($${\color{red}+1.3\%}$$)
STATIC_common.ts 59.63 kB ($${\color{red}+0.9\%}$$)
STATIC_tgpu.ts 275.22 kB ($${\color{red}+0.9\%}$$)
tgpu_fn.ts 275.22 kB ($${\color{red}+0.9\%}$$)
tgpu_init.ts 275.22 kB ($${\color{red}+0.9\%}$$)
tgpu_lazy.ts 275.22 kB ($${\color{red}+0.9\%}$$)
tgpu_slot.ts 275.22 kB ($${\color{red}+0.9\%}$$)
tgpu_const.ts 275.22 kB ($${\color{red}+0.9\%}$$)
tgpu_unroll.ts 275.22 kB ($${\color{red}+0.9\%}$$)
tgpu_resolve.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_accessor.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_comptime.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_vertexFn.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_computeFn.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_fragmentFn.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_privateVar.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_vertexLayout.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_workgroupVar.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_initFromDevice.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_bindGroupLayout.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_mutableAccessor.ts 275.23 kB ($${\color{red}+0.9\%}$$)
tgpu_resolveWithContext.ts 275.24 kB ($${\color{red}+0.9\%}$$)
STATIC_allImports.ts 301.47 kB ($${\color{red}+0.8\%}$$)
STATIC_std.ts 95.54 kB ($${\color{green}-2.0\%}$$)
STATIC_d.ts 75.79 kB ($${\color{green}-2.6\%}$$)
std_abs.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_acos.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_acosh.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_asin.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_asinh.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_atan.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_atan2.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_atanh.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_ceil.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_clamp.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_cos.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_cosh.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_countLeadingZeros.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_countOneBits.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_countTrailingZeros.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_cross.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_degrees.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_determinant.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_dot4I8Packed.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_exp.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_exp2.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_extractBits.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_faceForward.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_firstLeadingBit.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_firstTrailingBit.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_floor.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_fma.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_insertBits.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_intdiv.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_inverseSqrt.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_ldexp.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_log.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_log2.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_max.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_min.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_normalize.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_pow.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_quantizeToF16.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_radians.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_reflect.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_refract.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_reverseBits.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_round.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_saturate.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_sign.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_sin.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_sinh.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_smoothstep.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_sqrt.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_step.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_tan.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_tanh.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_transpose.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_trunc.ts 54.27 kB ($${\color{green}-3.5\%}$$)
std_distance.ts 54.26 kB ($${\color{green}-3.5\%}$$)
std_dot4U8Packed.ts 54.26 kB ($${\color{green}-3.5\%}$$)
std_fract.ts 54.26 kB ($${\color{green}-3.5\%}$$)
std_frexp.ts 54.26 kB ($${\color{green}-3.5\%}$$)
std_mix.ts 54.26 kB ($${\color{green}-3.5\%}$$)
std_modf.ts 54.26 kB ($${\color{green}-3.5\%}$$)
std_dot.ts 54.26 kB ($${\color{green}-3.5\%}$$)
std_length.ts 54.26 kB ($${\color{green}-3.5\%}$$)
d_struct.ts 1.72 kB ($${\color{green}-53.6\%}$$)

import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):

Test tsdown
tgpu_init.ts 265.10 kB ($${\color{green}-3.7\%}$$)
tgpu_initFromDevice.ts 264.56 kB ($${\color{green}-3.9\%}$$)
tgpu_resolve.ts 164.27 kB ($${\color{green}-40.3\%}$$)
tgpu_resolveWithContext.ts 164.21 kB ($${\color{green}-40.3\%}$$)
tgpu_bindGroupLayout.ts 60.24 kB ($${\color{green}-78.1\%}$$)
tgpu_mutableAccessor.ts 54.96 kB ($${\color{green}-80.0\%}$$)
tgpu_accessor.ts 54.95 kB ($${\color{green}-80.0\%}$$)
tgpu_privateVar.ts 53.64 kB ($${\color{green}-80.5\%}$$)
tgpu_workgroupVar.ts 53.64 kB ($${\color{green}-80.5\%}$$)
tgpu_const.ts 53.06 kB ($${\color{green}-80.7\%}$$)
tgpu_lazy.ts 52.86 kB ($${\color{green}-80.8\%}$$)
tgpu_fn.ts 42.19 kB ($${\color{green}-84.7\%}$$)
tgpu_fragmentFn.ts 40.23 kB ($${\color{green}-85.4\%}$$)
tgpu_vertexFn.ts 40.04 kB ($${\color{green}-85.5\%}$$)
tgpu_computeFn.ts 39.75 kB ($${\color{green}-85.6\%}$$)
tgpu_vertexLayout.ts 28.33 kB ($${\color{green}-89.7\%}$$)
tgpu_comptime.ts 15.93 kB ($${\color{green}-94.2\%}$$)
tgpu_unroll.ts 1.75 kB ($${\color{green}-99.4\%}$$)
tgpu_slot.ts 1.70 kB ($${\color{green}-99.4\%}$$)

If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.

@github-actions

github-actions Bot commented Aug 27, 2026

Copy link
Copy Markdown

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]
Loading
---
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]
Loading
---
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]
Loading

@iwoplaza
iwoplaza force-pushed the feat/unique-names-in-wgsl branch 2 times, most recently from b4f2910 to 824c2b2 Compare August 27, 2026 15:39
@iwoplaza iwoplaza linked an issue Aug 27, 2026 that may be closed by this pull request
@iwoplaza
iwoplaza marked this pull request as ready for review August 27, 2026 15:43
Copilot AI lite review requested due to automatic review settings August 27, 2026 15:43

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed changes

  • fnCore.ts — raw-WGSL string implementations now extract identifier-like tokens, rename identifiers that would clash with already-taken names (recording renames in scope.localRenames), reserve the rest, and renameIdentifiers/replaceExternalsInWgsl the body inside a pushed function+block scope.
  • rawShaderCodeUtils.ts — new module consolidating blankSpaces/lineBreaks, stripWGSLComments/blankOutWGSLComments, anyIdent, extractIdentifierLikeTokens, and renameIdentifiers; core/whitespaces.ts and the old minify.ts copy removed.
  • tgpuRawCodeSnippet.resolve — now rewrites its expression with ctx.topFunctionScope.localRenames before external substitution.
  • nameUtils.ts/namespace.ts/wgslGenerator.ts/resolutionCtx.tsbannedTokens/builtins moved out of global seeding into WgslGenerator.isBannedToken/isBuiltinGlobal, with builtins reserved via _reserveGlobals(); isIdentifierTaken consults gen.isBannedToken.
  • validateProp now takes ctx; struct-prop validation moved to resolve time in resolveData.ts.
  • Tests: blankOutWGSLComments, extractIdentifiers, renameIdentifiers internals, rawFn name-clash cases, rawCodeSnippet rename-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.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +110 to +113
let expression = this.#expression;
if (ctx.topFunctionScope) {
expression = renameIdentifiers(expression, ctx.topFunctionScope.localRenames);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 rawShaderCodeUtils utilities (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.

Comment on lines 109 to +115
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);
Comment on lines +618 to +620
/**
* Reserves all builtin WGSL globals. Can be overriden to change behavior.
*/
Comment on lines +138 to +143
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)
@iwoplaza
iwoplaza force-pushed the feat/unique-names-in-wgsl branch from 824c2b2 to a8d4034 Compare August 28, 2026 14:58

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 around replaceExternalsInWgsl in fnCore.ts and around gen.functionDefinition in resolutionCtx.ts, and consumed via void parentFunctionNameSlot.$ in tgpuRawCodeSnippet.resolve.
  • Extended name-collision avoidance to tgpu.resolve({ template, externals }) (resolveFromTemplate in tgpuResolve.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 switched clashingIdentifiers/uniqueIdentifiers from sets to arrays in fnCore.ts.
  • Test updates: reordered the rawCodeSnippet rename-through-owner test (jsFn now first) and changed its jsFn snapshot to bind the snippet's local reference to the global constant; updated the namespace.test.ts re-resolve snapshot to foo_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.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment on lines +247 to +253
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');
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"`);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +182 to +183
fn jsFn(a_1: f32) -> f32 {
return a * 2 + a;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 aleksanderkatan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!
I didn't review the entire thing yet. I leave some cosmetic & complexity requests.

Comment on lines +79 to +83
/**
* Same as `stripWGSLComments`, but keeps all non-comment code at
* the same location where it originally was, and replaces the comments
* with whitespace.
*/

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, ' ');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines +148 to +158
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]);
}
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't this be cleaner than a while with an assignment in its test? Am I missing something?

Suggested change
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');

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: we don't need to recreate this regex each call, especially when it's used in multiple functions

Comment on lines +175 to +196
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);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@iwoplaza

Copy link
Copy Markdown
Collaborator Author

@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.

@iwoplaza
iwoplaza marked this pull request as draft August 28, 2026 17:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: Unique names in raw WGSL code

3 participants