fix: Runtime ternaries should require non-alias arguments - #2834
Conversation
|
pkg.pr.new packages benchmark commit |
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 0.02%) | ❔ Unknown |
|---|---|---|---|
| 0 | 303 | 21 | 0 |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 270.74 kB ( |
| tgpu_initFromDevice.ts | 270.19 kB ( |
| tgpu_resolve.ts | 170.98 kB ( |
| tgpu_resolveWithContext.ts | 170.92 kB ( |
| tgpu_bindGroupLayout.ts | 73.93 kB ( |
| tgpu_mutableAccessor.ts | 68.66 kB ( |
| tgpu_accessor.ts | 68.66 kB ( |
| tgpu_privateVar.ts | 67.35 kB ( |
| tgpu_workgroupVar.ts | 67.35 kB ( |
| tgpu_const.ts | 66.77 kB ( |
| tgpu_lazy.ts | 66.56 kB ( |
| tgpu_fragmentFn.ts | 38.92 kB ( |
| tgpu_fn.ts | 38.87 kB ( |
| tgpu_vertexFn.ts | 38.74 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.
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.80, 1.58, 3.70, 5.94, 6.57, 12.15, 20.62, 20.00]
line [0.80, 1.68, 3.63, 6.10, 6.27, 10.70, 18.66, 21.16]
line [0.82, 1.68, 3.71, 5.76, 6.36, 10.65, 18.49, 22.19]
---
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.27, 0.45, 0.59, 0.75, 0.93, 1.01, 1.18, 1.34]
line [0.32, 0.50, 0.64, 0.75, 1.04, 1.03, 1.18, 1.32]
line [0.33, 0.48, 0.64, 0.76, 1.02, 1.02, 1.26, 1.43]
---
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.70, 1.85, 3.39, 6.30, 11.02, 21.95, 48.41, 98.34]
line [0.88, 1.90, 3.93, 5.59, 10.34, 21.70, 47.74, 94.65]
line [0.79, 1.89, 3.96, 5.43, 10.95, 22.85, 49.16, 100.02]
|
7a07fe6 to
3e83c9f
Compare
There was a problem hiding this comment.
Pull request overview
This PR tightens TGSL runtime ternary handling to prevent JS-vs-WGSL semantic mismatches by rejecting runtime ternaries whose branches are non-ephemeral aliases (references), and adds a regression test to lock the behavior in.
Changes:
- Add a new test asserting that runtime ternaries selecting between struct field aliases throw during resolution.
- Extend the WGSL generator’s runtime-ternary validation to reject alias branches unless the type is “naturally ephemeral” (scalar-like).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/typegpu/tests/tgsl/ternaryOperator.test.ts | Adds coverage for rejecting runtime ternaries that would alias in JS but copy in WGSL. |
| packages/typegpu/src/tgsl/wgslGenerator.ts | Adds alias-branch invalidation logic for runtime ternary lowering to select(...). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| (isAlias(consequent) && !wgsl.isNaturallyEphemeral(consequent.dataType)) || | ||
| (isAlias(alternative) && !wgsl.isNaturallyEphemeral(alternative.dataType)) | ||
| ) { | ||
| throw new Error( | ||
| `Ternary operator '${stringifyNode(expression)}' is invalid. For more complex branching, please use 'std.select' or if/else statements.`, |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ The fix is correct, well-scoped, and covered by a genuine regression test. One minor suggestion inline about the error guidance.
Reviewed changes
wgslGenerator.ts— runtime ternary guard extended to non-ephemeral aliases: a runtime ternary now throws when either branch is an alias whose data type isn't naturally ephemeral in JS (i.e. vectors/structs/arrays), preserving JS reference semantics thatselect()would silently downgrade to a copy. Scalar aliases remain allowed via theisNaturallyEphemeralcarve-out.ternaryOperator.test.ts— regression test:runtimeBool ? boid.pos : boid.vel(both branches struct-member aliases ofvec3f) is asserted to throw. I verified this test genuinely pins the fix — it fails with the guard removed and passes with it.
The isAlias(x) && !isNaturallyEphemeral(x.dataType) idiom matches existing precedents (generationHelpers.ts:118, ref.ts:67, wgslGenerator.ts:522), and no new imports are required. I ran the full typegpu suite (2311 tests), the docs example snapshot tests, and the gl/three/sort/noise/react suites — all pass, so no existing code regressed.
Technical details
# Runtime ternaries with vector-alias branches
## Confirmed behavior
- Guard fires only when the condition is runtime (`isKnownAtComptime(test)` is false) and the branch stays an alias after `convertToCommonType`. Existing scalar-member/uniform ternaries (u32) bypass it via `isNaturallyEphemeral`.
- `validSelectBranchTypes` (`std/boolean.ts:406`) limits branches to scalars + vectors, so the non-ephemeral cases blocked here are exactly vector aliases.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| (isAlias(consequent) && !wgsl.isNaturallyEphemeral(consequent.dataType)) || | ||
| (isAlias(alternative) && !wgsl.isNaturallyEphemeral(alternative.dataType)) |
There was a problem hiding this comment.
When this new guard fires for a vector alias, the shared error message advises std.select or if/else — but neither restores the JS reference semantics this check is protecting: std.select(boid.pos, boid.vel, cond) (whose codegenImpl in std/boolean.ts has no alias guard) reproduces the exact reference-vs-copy mismatch, and an if/else on the JS side still aliases. The test comments document the real remedy (d.vec3f(boid.pos)). Consider adding a copy hint to the message for this case (like the ArrayExpression error does), or applying the same guard to std.select.
No description provided.