feat: Allow tgpu.resolve to be nested - #2811
Conversation
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.63, 1.27, 2.66, 3.67, 4.49, 7.13, 13.60, 15.50]
line [0.65, 1.24, 2.44, 3.87, 4.76, 7.79, 13.66, 15.96]
line [0.70, 1.29, 2.77, 4.20, 4.75, 7.17, 14.03, 14.95]
---
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.20, 0.39, 0.46, 0.61, 0.82, 0.81, 1.03, 1.11]
line [0.20, 0.35, 0.53, 0.60, 0.85, 0.82, 0.98, 1.05]
line [0.19, 0.37, 0.47, 0.55, 0.77, 0.77, 1.01, 1.04]
---
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.61, 1.51, 2.78, 4.44, 7.91, 15.93, 34.48, 70.07]
line [0.65, 1.53, 2.83, 4.16, 7.37, 15.83, 35.13, 71.51]
line [0.63, 1.60, 2.83, 4.21, 7.93, 16.63, 35.31, 75.52]
|
There was a problem hiding this comment.
Pull request overview
Enables tgpu.resolve (and related context-providing mechanisms) to be invoked from within other resolution-driven code paths by making resolution context providers properly nestable, and adds tests to validate nested resolution scenarios.
Changes:
- Updated
provideCtxto support nested contexts by restoring the previous context on exit. - Added test coverage for nested
tgpu.resolveusage, including a shared namespace scenario. - Exposed
ResolvableObjectas a public type export to support the new tests and user typing.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/typegpu/tests/resolve.test.ts | Adds new tests asserting nested tgpu.resolve behavior and shared-namespace name generation. |
| packages/typegpu/src/indexNamedExports.ts | Exports ResolvableObject from the public barrel for external typing. |
| packages/typegpu/src/execMode.ts | Changes provideCtx to behave like a stack (save/restore previous context) to allow nesting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
Removing the only use of invariant in provideCtx leaves an unused import that fails the test:style lint gate. Drop the import before this merges.
Reviewed changes — Allow tgpu.resolve to be nested by making provideCtx restore the previous resolution context instead of resetting it to undefined, and dropping the "Cannot nest context providers" invariant. Resolves inside an outer resolve (e.g. via tgpu.comptime) now run with their own context and hand control back cleanly afterward. Exports ResolvableObject as a public type so comptime helpers can type a shared-namespace resource.
The context-stacking logic itself looks correct: the same-context short-circuit is preserved, provideCtx callers (resolutionCtx.ts top-level resolve, tgpuSimulate.ts) are unaffected since they never nest a different context, and the new tests genuinely exercise nesting (they would have thrown the old invariant). The one blocking issue is the now-unused invariant import, which oxlint --max-warnings=0 reports as a hard failure.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
88e9dd2 to
9d86c96
Compare
|
Run failed. View the logs →
|
9d86c96 to
40d03ff
Compare
|
pkg.pr.new packages benchmark commit |
Bundle size comparison (
|
| 🟢 Decreased (max -0.02%) | ➖ Unchanged | 🔴 Increased (max 0.03%) | ❔ Unknown |
|---|---|---|---|
| 21 | 136 | 165 | 0 |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 267.80 kB ( |
| tgpu_initFromDevice.ts | 267.27 kB ( |
| tgpu_resolve.ts | 168.37 kB ( |
| tgpu_resolveWithContext.ts | 168.30 kB ( |
| tgpu_bindGroupLayout.ts | 73.79 kB ( |
| tgpu_mutableAccessor.ts | 68.52 kB ( |
| tgpu_accessor.ts | 68.51 kB ( |
| tgpu_privateVar.ts | 67.20 kB ( |
| tgpu_workgroupVar.ts | 67.20 kB ( |
| tgpu_const.ts | 66.62 kB ( |
| tgpu_lazy.ts | 66.42 kB ( |
| tgpu_fragmentFn.ts | 38.92 kB ( |
| tgpu_fn.ts | 38.86 kB ( |
| tgpu_vertexFn.ts | 38.73 kB ( |
| tgpu_computeFn.ts | 38.44 kB ( |
| tgpu_vertexLayout.ts | 27.57 kB ( |
| tgpu_comptime.ts | 15.18 kB ( |
| tgpu_unroll.ts | 1.75 kB ( |
| tgpu_slot.ts | 1.70 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
40d03ff to
c074033
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — delta on top of the prior Pullfrog review (40d03ff → c074033):
- Removed the now-unused
import { invariant } from './errors.ts'fromexecMode.ts. This addresses the prior review's only blocking finding (unused import failing thetest:styleoxlint--max-warnings=0gate); the thread is resolved and oxlint reports 0 warnings/errors on all three changed files. - Reworked the first
tgpu resolve - nestingtest to usetgpu.resolveWithContext([getPi2]).declarations.length(renaming the comptime helper togetDeclarationsOfGetPi2) instead oftgpu.resolve([getPi2]).length, updating the snapshot from+ 79fto+ 2f. The new assertion (piconst +getPi2fn = 2 declarations) is more meaningful than a brittle resolved-code string-length count, and I confirmed both nesting tests pass.
The context-stacking change itself remains sound: the same-context short-circuit is preserved, nesting restores the prior context (true LIFO, no cross-context state pollution), and the tests genuinely exercise nesting — they would have thrown the old "Cannot nest context providers" invariant.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
c074033 to
b7aa8c4
Compare
b7aa8c4 to
df0fe67
Compare
df0fe67 to
8b446b4
Compare
| it('should allow for nested use with a shared namespace', () => { | ||
| const namespace = tgpu['~unstable'].namespace(); | ||
|
|
||
| const getGeneratedName = tgpu.comptime((resource: ResolvableObject) => { |
8b446b4 to
5817871
Compare
5817871 to
a0244d2
Compare
a0244d2 to
a897929
Compare
a897929 to
d706555
Compare
Nesting resolutions is essential for @typegpu/three (and similar integrations) to work correctly, as nested resolves can happen when a TSL node calls a TypeGPU function, which references a TSL node that calls a TypeGPU function again.