fix: Make numeric names valid#2598
Open
aleksanderkatan wants to merge 5 commits into
Open
Conversation
|
pkg.pr.new packages benchmark commit |
📊 Bundle Size Comparison
👀 Notable resultsStatic test results:No major changes. Dynamic test results:
📋 All resultsClick to reveal the results table (355 entries).
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 [0.92, 1.92, 3.78, 5.98, 7.31, 11.36, 21.96, 22.85]
line [0.89, 1.78, 4.40, 6.66, 7.37, 10.47, 20.44, 22.91]
line [0.89, 1.89, 4.19, 6.47, 7.85, 10.99, 21.95, 23.59]
---
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.30, 0.50, 0.66, 0.85, 1.11, 1.15, 1.40, 1.61]
line [0.28, 0.48, 0.67, 0.79, 1.11, 1.16, 1.34, 1.51]
line [0.29, 0.58, 0.72, 0.84, 1.18, 1.19, 1.45, 1.54]
---
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.75, 1.97, 4.26, 6.34, 11.77, 24.96, 53.08, 107.72]
line [0.80, 2.09, 3.71, 6.11, 12.06, 25.30, 54.13, 109.36]
line [0.91, 2.18, 4.01, 7.42, 12.74, 25.67, 54.54, 109.82]
|
There was a problem hiding this comment.
Pull request overview
This PR tightens identifier sanitization/validation to ensure generated WGSL identifiers don’t start from invalid “primers” (e.g. empty strings or numeric names), and updates/extends WGSL generator tests to cover these cases.
Changes:
- Extend
sanitizePrimer/validateIdentifierso invalid primers (e.g.'','0','__') fall back toitem. - Add WGSL generator tests for item renaming and for invalid struct property keys; remove an obsolete test that relied on numeric struct keys.
- Refactor
validatePropto reusevalidateIdentifierbefore applying reserved-word checks.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/typegpu/tests/tgsl/wgslGenerator.test.ts | Adds coverage for invalid item names and invalid struct property identifiers; removes a now-obsolete numeric-key test. |
| packages/typegpu/src/nameUtils.ts | Updates identifier sanitization/validation to align with WGSL identifier rules and handle numeric/empty primers. |
Comments suppressed due to low confidence (1)
packages/typegpu/src/nameUtils.ts:446
validatePropchecksbannedTokens.has(ident)but the error message says identifiers cannot start with reserved keywords. As written, names likestruct_1/loop_1would be allowed even though the message (and previous behavior) implies they should be rejected. Either update the message/tests to match exact-keyword-only behavior, or restore the prefix-based check here.
if (bannedTokens.has(ident)) {
return {
success: false,
error: `Identifiers cannot start with reserved keywords.`,
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
cieplypolar
reviewed
Jun 12, 2026
| if (primer) { | ||
| const base = primer | ||
| .replaceAll(/\s/g, '_') // whitespaces | ||
| .replaceAll(/[^\w\d]/g, ''); // removing illegal characters |
Collaborator
There was a problem hiding this comment.
Suggested change
| .replaceAll(/[^\w\d]/g, ''); // removing illegal characters | |
| .replaceAll(/[^\w]/g, ''); // removing illegal characters |
\w includes digits
cieplypolar
reviewed
Jun 12, 2026
| }; | ||
| } | ||
| const prefix = ident.split('_')[0] as string; | ||
| if (bannedTokens.has(prefix) || builtins.has(prefix)) { |
Collaborator
There was a problem hiding this comment.
We can't simply delete this check.
import { expect } from 'vitest';
import { it } from 'typegpu-testing-utility';
import tgpu, { d } from '../../src/index.js';
it('reserved keyword in compute fn', () => {
const f = tgpu.computeFn({
in: { loop: d.builtin.globalInvocationId },
workgroupSize: [1],
})`{
let x = loop.x;
}`;
expect(tgpu.resolve([f])).toMatchInlineSnapshot(`
"@compute @workgroup_size(1) fn f(@builtin(global_invocation_id) loop: vec3u) {
let x = loop.x;
}"
`);
});
it('reserved keyword in fragment fn', () => {
const f = tgpu.fragmentFn({
in: { loop: d.builtin.position },
out: d.vec4f,
})`{
return vec4f(loop.x);
}`;
expect(tgpu.resolve([f])).toMatchInlineSnapshot(`
"@fragment fn f(@builtin(position) loop: vec4f) -> @location(0) vec4f {
return vec4f(loop.x);
}"
`);
});The unused argument pruning will save as, but we should validate input props with more attention.
Contributor
Author
There was a problem hiding this comment.
I added an explicit check for this, see if you see this solution fit
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.
No description provided.