refactor: Naming cleanup#2363
Open
iwoplaza wants to merge 1 commit intofix/first-class-argument-usage-trackingfrom
Open
refactor: Naming cleanup#2363iwoplaza wants to merge 1 commit intofix/first-class-argument-usage-trackingfrom
iwoplaza wants to merge 1 commit intofix/first-class-argument-usage-trackingfrom
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 (350 entries).
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu. |
iwoplaza
force-pushed
the
refactor/naming-cleanup
branch
4 times, most recently
from
b5b49ac to
4828d22
Compare
iwoplaza
changed the base branch from
main
to
fix/first-class-argument-usage-tracking
iwoplaza
force-pushed
the
fix/first-class-argument-usage-tracking
branch
from
8a474d6 to
1036589
Compare
iwoplaza
force-pushed
the
refactor/naming-cleanup
branch
2 times, most recently
from
18d3088 to
d7926fa
Compare
iwoplaza
requested review from
aleksanderkatan,
cieplypolar,
Copilot and
reczkok
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Refactors the naming/identifier generation system by moving uniqueness + scoping logic into ResolutionCtx, simplifying the namespace implementation, and updating generators/resolvers/tests to use the new APIs.
Changes:
- Replaced
getUniqueName/makeNameValidwithmakeUniqueIdentifier(..., scope), plus newisIdentifierTaken/reserveIdentifierAPIs. - Merged scope-layer naming tracking into
ItemStateStack/ResolutionCtxand removednamespace.on('name', ...)event functionality. - Updated WGSL generator + core resolvers to use block/global-aware identifier allocation; adjusted tests accordingly.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/tests/resolve.test.ts | Updates tests to use makeUniqueIdentifier(getName(...), 'global'). |
| packages/typegpu/tests/namespace.test.ts | Removes namespace.on('name') test and drops unused vi import. |
| packages/typegpu/src/types.ts | Extends scope layer + ResolutionCtx interface with new identifier APIs. |
| packages/typegpu/src/tgsl/wgslGenerator.ts | Migrates local variable naming to 'block' identifiers and adds _blockStatement helper. |
| packages/typegpu/src/resolutionCtx.ts | Implements new identifier generation/reservation logic and adjusts function/block scoping behavior. |
| packages/typegpu/src/nameRegistry.ts | Removes registry classes; keeps/export identifier utilities + token sets for reuse. |
| packages/typegpu/src/data/struct.ts | Switches prop validation import to nameUtils. |
| packages/typegpu/src/data/autoStruct.ts | Switches prop validation import to nameUtils. |
| packages/typegpu/src/core/variable/tgpuVariable.ts | Uses makeUniqueIdentifier(..., 'global') for variable declarations. |
| packages/typegpu/src/core/texture/texture.ts | Uses makeUniqueIdentifier(..., 'global') for texture view declarations. |
| packages/typegpu/src/core/texture/externalTexture.ts | Uses makeUniqueIdentifier(..., 'global') for external texture declarations. |
| packages/typegpu/src/core/sampler/sampler.ts | Uses makeUniqueIdentifier(..., 'global') for sampler declarations. |
| packages/typegpu/src/core/resolve/resolveData.ts | Uses makeUniqueIdentifier(..., 'global') for struct/unstruct names. |
| packages/typegpu/src/core/resolve/namespace.ts | Simplifies namespace internals (drops NameRegistry + event listeners; tracks taken globals + strategy). |
| packages/typegpu/src/core/function/fnCore.ts | Changes entry-input argument naming for string implementations and adds identifier validation. |
| packages/typegpu/src/core/constant/tgpuConstant.ts | Uses makeUniqueIdentifier(..., 'global') for constant declarations. |
| packages/typegpu/src/core/buffer/bufferUsage.ts | Uses makeUniqueIdentifier(..., 'global') for buffer declarations. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+76
to
88
| if (entryInput) { | ||
| for (const arg of entryInput.positionalArgs) { | ||
| if (!isValidIdentifier(arg.schemaKey)) { | ||
| throw new Error(`Invalid argument name: ${arg.schemaKey}`); | ||
| } | ||
| } | ||
|
|
||
| if (validArgNames && Object.keys(validArgNames).length > 0) { | ||
| applyExternals(externalMap, { in: validArgNames }); | ||
| applyExternals(externalMap, { | ||
| in: Object.fromEntries( | ||
| entryInput.positionalArgs.map((a) => [a.schemaKey, a.schemaKey]), | ||
| ), | ||
| }); | ||
| } |
There was a problem hiding this comment.
isValidIdentifier can throw (e.g. whitespace / leading underscores), so if (!isValidIdentifier(...)) will not reliably produce the intended Invalid argument name: ... error; it will often throw a different error message instead. Also, this is a behavior change from the previous ctx.makeNameValid(...) approach: previously, schema keys that weren't safe WGSL identifiers could be mapped/renamed; now they hard-error. If the intent is only to improve error reporting, wrap validation in a try/catch and rethrow with a consistent message. If the intent is to preserve prior behavior, reintroduce a mapping step (generate safe identifiers and pass that mapping to applyExternals) rather than requiring schemaKey to already be usable as a WGSL parameter name.
| } | ||
| } | ||
|
|
||
| public _blockStatement(block: tinyest.Block, externalMap?: ExternalMap): string { |
There was a problem hiding this comment.
This introduces a new public method with a leading underscore (_blockStatement), which typically signals a non-public API. If this is an internal helper, make it private/protected (or remove the underscore and treat it as part of the public surface). Keeping it public makes it harder to evolve without breaking external consumers.
Suggested change
| public _blockStatement(block: tinyest.Block, externalMap?: ExternalMap): string { | |
| protected _blockStatement(block: tinyest.Block, externalMap?: ExternalMap): string { |
Comment on lines
+417
to
+424
| isIdentifierTaken(name: string): boolean { | ||
| return ( | ||
| this.#namespaceInternal.takenGlobalIdentifiers.has(name) || | ||
| !!this._itemStateStack.isIdentifierTakenLocally(name) | ||
| ); | ||
| } | ||
|
|
||
| makeNameValid(name: string): string { | ||
| return this.#namespaceInternal.nameRegistry.makeValid(name); | ||
| makeUniqueIdentifier(primer: string = 'item', scope: 'global' | 'block'): string { |
There was a problem hiding this comment.
The new identifier API (isIdentifierTaken, makeUniqueIdentifier, reserveIdentifier) adds important scoping semantics ('global' vs 'block', and visibility across nested blocks/functions). There are test updates for call sites, but there doesn't appear to be coverage specifically asserting the new scoping rules (e.g., block identifiers not shadowing visible names, identifiers becoming available after popping a block scope, and global uniqueness across the resolution). Add focused unit tests around the new naming behavior to prevent regressions.
aleksanderkatan
approved these changes
Apr 15, 2026
| * defined within it are no longer visible. | ||
| * @returns an identifier that is unique within the given scope | ||
| */ | ||
| makeUniqueIdentifier(primer: string | undefined, scope: 'global' | 'block'): string; |
Contributor
There was a problem hiding this comment.
I'd add a note in docs that this reserves the identifier. Actually, do we need to expose the reserveIdentifier? It is only used internally.
Collaborator
Author
There was a problem hiding this comment.
.reserveIdentifier is used by a PR that's stacked on top of this one, #2360
Contributor
|
I'm not sure if I like going away from |
iwoplaza
force-pushed
the
fix/first-class-argument-usage-tracking
branch
from
1036589 to
88dfb64
Compare
iwoplaza
force-pushed
the
refactor/naming-cleanup
branch
from
d7926fa to
5b7b6eb
Compare
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.83, 1.78, 3.96, 5.96, 7.26, 11.36, 20.83, 23.96]
line [0.88, 1.83, 4.06, 6.27, 7.01, 10.08, 19.98, 24.00]
line [0.90, 1.89, 3.95, 6.07, 6.97, 10.11, 20.63, 22.74]
---
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.28, 0.56, 0.69, 0.80, 1.03, 1.07, 1.31, 1.46]
line [0.32, 0.59, 0.63, 0.77, 1.02, 1.13, 1.37, 1.50]
line [0.28, 0.53, 0.64, 0.80, 1.08, 1.16, 1.38, 1.57]
---
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.82, 1.80, 3.81, 5.86, 12.07, 24.34, 53.01, 107.68]
line [0.88, 1.97, 4.44, 6.09, 12.26, 25.63, 56.45, 110.57]
line [0.85, 2.13, 3.40, 5.93, 12.03, 24.34, 53.62, 108.59]
|
Collaborator
Author
@aleksanderkatan It was more modular that way, and I wrote it with the assumption that there were going to be more ways to name things, but deciding what to name something now requires much more context than before, and we don't foresee any new naming strategy. Imo we can revisit modularizing it if we find that we need to introduce more name strategies. |
iwoplaza
force-pushed
the
refactor/naming-cleanup
branch
from
5b7b6eb to
2afcfb8
Compare
iwoplaza
force-pushed
the
fix/first-class-argument-usage-tracking
branch
from
88dfb64 to
6a12e46
Compare
iwoplaza
force-pushed
the
refactor/naming-cleanup
branch
from
2afcfb8 to
d5d4be5
Compare
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.
Blocked by #2359
@typegpu/gl)getUniqueName->makeUniqueIdentifier(..., 'global')makeNameValid->makeUniqueIdentifier(..., 'block')namespace.onfunctionality (previously used by@typegpu/three).