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 (349 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
from
4828d22 to
18d3088
Compare
iwoplaza
force-pushed
the
refactor/naming-cleanup
branch
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
+424
to
+431
| makeUniqueIdentifier(primer: string = 'item', scope: 'global' | 'block'): string { | ||
| if (scope === 'block' && isValidIdentifier(primer) && !this.isIdentifierTaken(primer)) { | ||
| // Preserving local definitions as they are, provided they are valid and not already taken. | ||
| this.reserveIdentifier(primer, 'block'); | ||
| return primer; | ||
| } | ||
|
|
||
| const base = sanitizePrimer(primer); |
There was a problem hiding this comment.
ResolutionCtx.makeUniqueIdentifier is declared in the interface as primer: string | undefined, but the implementation currently narrows it to string (primer: string = 'item'). This will break type compatibility and also prevents passing getName(...) if it can be undefined. Make the parameter type accept undefined (e.g. primer: string | undefined = 'item') and keep the defaulting logic inside the function.
Suggested change
| makeUniqueIdentifier(primer: string = 'item', scope: 'global' | 'block'): string { | |
| if (scope === 'block' && isValidIdentifier(primer) && !this.isIdentifierTaken(primer)) { | |
| // Preserving local definitions as they are, provided they are valid and not already taken. | |
| this.reserveIdentifier(primer, 'block'); | |
| return primer; | |
| } | |
| const base = sanitizePrimer(primer); | |
| makeUniqueIdentifier(primer: string | undefined, scope: 'global' | 'block'): string { | |
| const effectivePrimer = primer ?? 'item'; | |
| if ( | |
| scope === 'block' && | |
| isValidIdentifier(effectivePrimer) && | |
| !this.isIdentifierTaken(effectivePrimer) | |
| ) { | |
| // Preserving local definitions as they are, provided they are valid and not already taken. | |
| this.reserveIdentifier(effectivePrimer, 'block'); | |
| return effectivePrimer; | |
| } | |
| const base = sanitizePrimer(effectivePrimer); |
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.
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).