fix: Global identifier inside nested function call - #2789
Open
cieplypolar wants to merge 3 commits into
Open
Conversation
cieplypolar
changed the title
|
pkg.pr.new packages benchmark commit |
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.82, 1.69, 3.76, 5.65, 6.49, 11.80, 21.08, 22.05]
line [0.78, 1.60, 3.62, 5.00, 6.05, 9.54, 19.92, 19.85]
line [0.83, 1.72, 3.87, 5.93, 7.34, 10.25, 19.58, 22.29]
---
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.31, 0.46, 0.63, 0.84, 1.05, 1.07, 1.26, 1.49]
line [0.28, 0.45, 0.58, 0.69, 0.96, 1.00, 1.23, 1.38]
line [0.26, 0.51, 0.64, 0.77, 1.04, 1.07, 1.29, 1.41]
---
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.88, 1.83, 3.48, 5.75, 10.71, 23.70, 48.38, 96.40]
line [0.74, 1.83, 3.42, 6.01, 10.54, 22.45, 48.36, 94.24]
line [0.83, 1.83, 4.13, 6.08, 11.52, 23.79, 49.39, 100.41]
|
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 0.09%) | ❔ Unknown |
|---|---|---|---|
| 0 | 301 | 21 | 0 |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 260.28 kB ( |
| tgpu_initFromDevice.ts | 259.75 kB ( |
| tgpu_resolve.ts | 165.65 kB ( |
| tgpu_resolveWithContext.ts | 165.59 kB ( |
| tgpu_bindGroupLayout.ts | 69.27 kB ( |
| tgpu_mutableAccessor.ts | 66.27 kB ( |
| tgpu_accessor.ts | 66.26 kB ( |
| tgpu_privateVar.ts | 65.61 kB ( |
| tgpu_workgroupVar.ts | 65.60 kB ( |
| tgpu_const.ts | 64.85 kB ( |
| tgpu_fn.ts | 38.45 kB ( |
| tgpu_fragmentFn.ts | 38.45 kB ( |
| tgpu_vertexFn.ts | 38.27 kB ( |
| tgpu_computeFn.ts | 37.97 kB ( |
| tgpu_vertexLayout.ts | 27.08 kB ( |
| tgpu_comptime.ts | 14.77 kB ( |
| tgpu_unroll.ts | 1.66 kB ( |
| tgpu_slot.ts | 1.54 kB ( |
| tgpu_lazy.ts | 1.19 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
There was a problem hiding this comment.
Pull request overview
This PR fixes a TGSL/WGSL name-collision bug where a globally-declared identifier could be generated with a name that later becomes shadowed by an identifier in a wrapping (caller) function, causing subsequent references to bind to the wrong symbol.
Changes:
- Extend identifier-collision checks to distinguish between
globalandblockscopes, so global identifiers avoid collisions with identifiers taken in any active block scope on the current resolution stack. - Add a regression test covering a global declaration accessed inside a nested function while the wrapping function has a colliding argument name.
- Update multiple docs example snapshot tests to reflect the new, collision-free global naming (e.g.,
seed→seed_1/seed_2).
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| packages/typegpu/tests/tgsl/nameClashes.test.ts | Adds regression test for global identifier collisions across nested resolution contexts. |
| packages/typegpu/src/types.ts | Updates ResolutionCtx.isIdentifierTaken signature to include an explicit scope parameter. |
| packages/typegpu/src/resolutionCtx.ts | Implements scoped identifier-taken checks and adds global-aware stack scanning for collisions. |
| apps/typegpu-docs/tests/individual-example-tests/vaporrave.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/uniformity.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/smoky-triangle.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/slime-mold.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/slime-mold-3d.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/ripple-cube.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/probability.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/perlin-noise.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/jump-flood-voronoi.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/jelly-switch.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/jelly-slider.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/game-of-life.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/fluid-double-buffering.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/clouds.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/caustics.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
| apps/typegpu-docs/tests/individual-example-tests/3d-fish.test.ts | Updates expected WGSL snapshots for new global identifier naming. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Reviewed changes
This PR fixes the name-collision bug reported in #2788: a global identifier referenced from inside a nested function was not being checked against local identifiers in enclosing function/block scopes, so it could end up shadowed by a parameter of an outer function.
resolutionCtx.tsaddsisIdentifierTakenGloballyand routesisIdentifierTakenby scope so global-scoped generation considers all enclosing block scopes.types.tsupdates the publicResolutionCtxinterface accordingly.- A focused regression test in
nameClashes.test.tsreproduces the exactCOLLISIONcase from the issue. - 16 example-test snapshots are updated because module-scope
seedvariables now correctly avoid colliding with parameters namedseed.
I ran the targeted tests, the full packages/typegpu/tests/tgsl suite, and the apps/typegpu-docs/tests/individual-example-tests suite — all passed, and npx tsc -p packages/typegpu/tsconfig.json --noEmit is clean.
✅ No new issues found.
One small polish item: isIdentifierTaken in packages/typegpu/src/types.ts (~line 363) is now part of the public ResolutionCtx contract with a new scope parameter, but it has no JSDoc. Adding a brief docstring (mirroring makeUniqueIdentifier/reserveIdentifier) would help consumers understand that 'global' checks the whole namespace plus all enclosing block scopes while 'block' stops at the function boundary.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
iwoplaza
reviewed
Aug 5, 2026
aleksanderkatan
approved these changes
Aug 5, 2026
aleksanderkatan
left a comment
aleksanderkatan
left a comment
Member
There was a problem hiding this comment.
Great!
Just address Iwo's comments before merging
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers the follow-up commits since the last Pullfrog review. The new commits address the prior JSDoc feedback and tighten the naming around the global identifier collision helper.
- Renamed and documented the call-stack collision helper.
isIdentifierTakenGloballyis nowisIdentifierTakenInCallStackinresolutionCtx.ts, with a JSDoc note explaining it is used when resolving a global identifier for the first time within a nested function. - Added JSDoc to the public
isIdentifierTakencontract. TheResolutionCtxinterface intypes.tsnow documents thenameandscopeparameters. - Added a local-variable regression test.
nameClashes.test.tsnow also covers the case where a wrapping function declaresconst COLLISION = 1;and a nested function references a global of the same name. - Verified the fix.
packages/typegpu/tests/tgsl/nameClashes.test.ts(10/10), the fullpackages/typegpu/tests/tgslsuite,apps/typegpu-docs/tests/individual-example-tests(59/59), andnpx tsc -p packages/typegpu/tsconfig.json --noEmitall pass.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
This incremental review covers the merge commit that landed after the previous review.
- Merged latest
maininto the feature branch. Commit038b32a7is a clean merge that does not touch the core fix files (resolutionCtx.ts,types.ts,nameClashes.test.ts). - Snapshot fallout is limited to
vaporrave.test.ts. The merge pulled in the updated vaporrave example frommain, which added a newsphereColorUniformvec4f binding and shifted subsequent binding numbers; the inline snapshot was regenerated accordingly. - Verified post-merge.
packages/typegpu/tests/tgsl/nameClashes.test.ts(10/10), the fullpackages/typegpu/tests/tgslsuite,apps/typegpu-docs/tests/individual-example-tests(59/59), andnpx tsc -p packages/typegpu/tsconfig.json --noEmitall pass.
Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
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.
Closes #2788.