fix(@typegpu/three): Allow TgpuFnNode to transitively nest - #2815
Conversation
|
pkg.pr.new packages benchmark commit |
There was a problem hiding this comment.
Pull request overview
Enables @typegpu/three’s TgpuFnNode generation context to be safely nested transitively (including failure paths), and adds a Vitest-based regression suite in the package to prevent context-leak regressions.
Changes:
- Restore outer
TgpuFnNodegeneration context after nested traversal via a stack-like helper (withGeneratingFnNodeCtx). - Add Vitest setup + tests in
packages/typegpu-threecovering nested analyze/generate and nested-throw/caught scenarios. - Update package scripts and lockfile to include the new test tooling dependencies.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pnpm-lock.yaml | Adds lockfile entries for new test tooling deps used by @typegpu/three. |
| packages/typegpu-three/vitest.config.mts | Introduces Vitest config (aligned with other workspace packages) including TypeGPU plugin + aliases. |
| packages/typegpu-three/tsconfig.test.json | Adds a dedicated TS config to typecheck tests and Vitest config. |
| packages/typegpu-three/tests/typegpu-node.test.ts | Adds regression tests for nested context restoration across analyze/generate and exception scenarios. |
| packages/typegpu-three/src/typegpu-node.ts | Refactors context handling to restore previous context instead of clearing it, enabling safe transitive nesting. |
| packages/typegpu-three/package.json | Adds vitest/jiti/testing utility dev deps and a test script; expands test:types to include test TS config. |
Files not reviewed (1)
- pnpm-lock.yaml: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.95, 1.95, 4.53, 6.90, 7.48, 12.26, 23.32, 23.45]
line [0.95, 1.91, 4.20, 6.01, 7.26, 12.95, 21.94, 26.01]
line [0.94, 1.99, 4.28, 6.25, 7.66, 12.24, 23.56, 24.52]
---
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.55, 0.73, 0.87, 1.08, 1.16, 1.36, 1.52]
line [0.37, 0.47, 0.69, 0.80, 1.06, 1.18, 1.40, 1.54]
line [0.33, 0.59, 0.72, 0.92, 1.28, 1.30, 1.49, 1.70]
---
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.83, 1.96, 3.98, 6.82, 12.96, 25.89, 53.50, 110.66]
line [0.79, 2.08, 4.06, 7.14, 13.05, 25.55, 53.91, 109.90]
line [0.85, 2.24, 4.24, 6.40, 13.46, 26.81, 57.56, 115.70]
|
Bundle size comparison (
|
| 🟢 Decreased (max -0.02%) | ➖ Unchanged | 🔴 Increased (max 0.03%) | ❔ Unknown |
|---|---|---|---|
| 21 | 137 | 165 | 0 |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 270.23 kB ( |
| tgpu_initFromDevice.ts | 269.68 kB ( |
| tgpu_resolve.ts | 170.33 kB ( |
| tgpu_resolveWithContext.ts | 170.27 kB ( |
| tgpu_bindGroupLayout.ts | 73.81 kB ( |
| tgpu_mutableAccessor.ts | 68.54 kB ( |
| tgpu_accessor.ts | 68.54 kB ( |
| tgpu_privateVar.ts | 67.23 kB ( |
| tgpu_workgroupVar.ts | 67.23 kB ( |
| tgpu_const.ts | 66.65 kB ( |
| tgpu_lazy.ts | 66.44 kB ( |
| tgpu_fragmentFn.ts | 38.92 kB ( |
| tgpu_fn.ts | 38.86 kB ( |
| tgpu_vertexFn.ts | 38.74 kB ( |
| tgpu_computeFn.ts | 38.45 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.
10a50b4 to
cc033a5
Compare
cc033a5 to
64888e9
Compare
There was a problem hiding this comment.
Important
Nesting a TgpuFnNode inside another no longer throws, but the enclosing node's nodeFunction is parsed from a contaminated slice of the shared generation buffer, silently returning the inner function's getNodeType()/inputs whenever the two functions have different signatures. The mechanism and a verified repro are in the inline comment; the added tests won't catch it.
Reviewed changes — One commit on fix/nested-tgpu-resolve making TgpuFnNode generation re-entrant: withGeneratingFnNodeCtx now saves and restores the previous currentlyGeneratingFnNodeCtx with finally stack semantics (used in #getNodeFunction and #analyzeFunction), and the obsolete "Nested function generation detected" warning is removed. Also introduces the package's first unit tests and vitest harness.
- Re-entrant generation context — stacked ctx restore lets a
fromTSLaccessor (or a nestedTgpuFnNode) resolve inside anotherTgpuFnNode's impl without stranding the outer context. All three new tests fail against the pre-fix behavior, so the regression coverage is real. - Test infrastructure —
tests/typegpu-node.test.ts,vitest.config.mts,tsconfig.test.json,testscript, plusjiti/typegpu-testing-utility/vitestdevDeps, mirroring the other packages.vitest run,test:types, lint, and format all pass.
⚠️ The tests can't catch the wrong-type regression they should pin
All three tests use a single f32 signature for every node, so they pin "doesn't throw / context restored" but not the PR's headline claim — that nested nodes are correct. With the PR's own nesting shape I reproduced the enclosing node reporting getNodeType() = 'float' instead of 'vec3'; a test with distinct inner/outer return types asserting the outer node's getNodeType (and ideally the emitted WGSL) would actually pin the transitively-nested contract.
Technical details
# Test-only coverage gap masks the wrong-type bug
## Affected sites
- packages/typegpu-three/tests/typegpu-node.test.ts:42-72 — both tests exercise only f32-typed nodes, so the enclosing-node mis-parse (see inline comment on typegpu-node.ts:147) is invisible to CI.
## Required outcome
- A new test where the enclosing `toTSL` returns a different type (e.g. `vec3f`) than a nested `f32` `TgpuFnNode`, asserting `outerNode.getNodeType(builder)` is `'vec3'` and, if feasible, that the generated source yields exactly one `fn` per node.
## Suggested approach (optional)
- This test stays red until the parse extraction is fixed; it is the correct regression pin for "transitively nest".ℹ️ Nitpicks
viis imported attests/typegpu-node.test.ts:4but never used. Test files haveeslint/no-unused-varsdisabled inoxlint.config.ts, so it won't fail CI, but it's a dead import — drop it.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
64888e9 to
cbfaf97
Compare
9ff9260 to
b211d8c
Compare
There was a problem hiding this comment.
ℹ️ No critical issues — the prior IMPORTANT mis-parse finding is resolved by the head rewrite; one optional hardening suggestion inline.
Reviewed changes — Since the prior pullfrog review (10a50b43, IMPORTANT), the head commit b211d8c9 rewrites how TgpuFnNode#getNodeFunction extracts the function declaration:
- Declaration-based extraction replaced shared-buffer slicing —
GenerateStageDatadrops thecodeGeneratedThusFarstring accumulator;#getNodeFunctionnow resolves viatgpu.resolveWithContext([this.#impl]), re-resolves the impl in the same namespace to get its identifier, and pulls the exact fn declaration from an accumulatedexistingDeclarations: ResolvedDeclaration[]. This removes theindexOf('\nfn ' + id)byte-slicing whose prefix-collision (itemvsitem_1) made the enclosing node mis-parse its type under transitive nesting. ResolvedDeclarationexported fromtypegpu(indexNamedExports.ts), andGenerateStageDataaccumulates declarations across the builder/stage so a fn used transitively and then passed directly intotoTSLis still found.- Unused
viimport removed from the test file (addressed the prior nitpick).
The prior IMPORTANT thread (typegpu-node.ts:147) is resolved. I re-verified on this head with scratch tests (removed after):
- Depth-2 transitive nesting (outer
d.vec3f(mid.$)→ midd.vec2f(leaf.$, leaf.$)→ leafd.f32) now returns the correct enclosing type: outergetNodeType()='vec3', mid'vec2', leaf'float'— previously'vec2'. - The emitted WGSL (
builder.getCodes('fragment')after a fullouter.build) containsfn item/item_1/item_2exactly once each, in correct nesting order — no double-emission from the accumulated-declarations lookup. - Direct reuse on the same builder of a
tgpu.fnthat was already resolved transitively builds without throwing and reports the right type. pnpm --filter @typegpu/three test:types, oxlint, and all three PR tests pass.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
b211d8c to
32f3de2
Compare
32f3de2 to
3f7abea
Compare
cieplypolar
left a comment
There was a problem hiding this comment.
Looks good. Left one nit.
3f7abea to
1cb767d
Compare
a3166dc to
e7951d4
Compare
e7951d4 to
ed23268
Compare
ed23268 to
78cbd66
Compare

This can happen when a TSL node calls a TypeGPU function which references a TSL node that calls another TypeGPU function.