fix: Reject decorated element types in arrayOf - #2781
Open
vende11s wants to merge 2 commits into
Open
Conversation
|
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.46, 1.01, 2.23, 3.35, 4.04, 6.73, 12.52, 12.43]
line [0.51, 1.04, 2.37, 3.82, 4.54, 6.51, 12.02, 13.19]
line [0.55, 1.05, 2.36, 3.50, 3.98, 5.56, 11.65, 13.04]
---
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.16, 0.34, 0.41, 0.46, 0.67, 0.66, 0.78, 0.86]
line [0.20, 0.33, 0.38, 0.44, 0.62, 0.68, 0.80, 0.84]
line [0.18, 0.28, 0.40, 0.49, 0.67, 0.68, 0.79, 0.85]
---
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.71, 1.48, 2.59, 4.19, 6.36, 13.63, 29.77, 60.81]
line [0.54, 1.33, 2.15, 3.59, 6.39, 13.53, 29.18, 59.57]
line [0.56, 1.34, 2.59, 3.72, 6.42, 13.69, 29.66, 60.54]
|
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 0.92%) | ❔ Unknown |
|---|---|---|---|
| 0 | 296 | 26 | 0 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
| Test | tsdown |
|---|---|
| d_arrayOf.ts | 26.84 kB ( |
| d_builtin.ts | 29.30 kB ( |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 260.24 kB ( |
| tgpu_initFromDevice.ts | 259.71 kB ( |
| tgpu_resolve.ts | 165.61 kB ( |
| tgpu_resolveWithContext.ts | 165.55 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 schema correctness gap in d.arrayOf by rejecting array element types that carry non-@location decorations (e.g. @align, @size), since those decorations have no effect on WGSL array element layout. @location remains allowed to support patterns used with vertexLayout.
Changes:
- Add a runtime guard and type-level error overload to
arrayOfto reject decorated element types except@location. - Extend array-related tests to validate the allowed (
@location) and rejected (non-@location) cases. - Update documentation to warn that
d.align/d.sizemust be wrapped in a struct when used with arrays, and adjust an affected buffer test schema.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/typegpu/src/data/array.ts | Enforces the “only @location allowed” rule for array element types (runtime + type-level) and documents the constraint. |
| packages/typegpu/tests/array.test.ts | Adds assertions for allowed @location decorated elements and for throwing on forbidden decorations. |
| packages/typegpu/tests/buffer.test.ts | Updates a schema in a usage-validity test to avoid now-forbidden element decorations. |
| apps/typegpu-docs/src/content/docs/apis/data-schemas.mdx | Documents the new restriction and the recommended struct-wrapping workaround. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
60
to
+65
| ) => { | ||
| if (isDecorated(elementType) && !elementType.attribs.every(isLocationAttrib)) { | ||
| throw new Error( | ||
| 'Arrays cannot hold decorated types other than @location. Wrap it in a struct instead, e.g. d.arrayOf(d.struct({ value: d.align(16, d.u32) }), n).', | ||
| ); | ||
| } |
Comment on lines
+569
to
+574
| it('throws when a non-location decorated element type is passed', () => { | ||
| expect(() => d.arrayOf(d.align(16, d.u32), 4)).toThrowErrorMatchingInlineSnapshot( | ||
| `[Error: Arrays cannot hold decorated types other than @location. Wrap it in a struct instead, e.g. d.arrayOf(d.struct({ value: d.align(16, d.u32) }), n).]`, | ||
| ); | ||
| expect(() => d.arrayOf(d.size(16, d.u32), 3)).toThrow(); | ||
| expect(() => d.arrayOf(d.location(0, d.align(16, d.u32)), 3)).toThrow(); |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
ℹ️ No critical issues — one minor inconsistency inline.
Reviewed changes
ForbiddenDecoratedArrayElementtype + overload — adds a type-level guard toWgslArrayConstructorthat resolves to a descriptive error string whenarrayOfreceives a decorated element type with anything other than@location.- Runtime
throwinarrayOfimplementation — throws if the element type isisDecoratedwith any non-@locationattribute, matching the type-level guard. - Tests — verifies
@locationis allowed, generic helper passthrough still works, runtime throw ford.align/d.size/mixed attributes, and type-level error return type. - Buffer test fix — replaces
d.arrayOf(d.align(16, d.u32), 32)withd.arrayOf(d.u32, 32)in the "ignore decorated types" test, since decorated element types are now rejected. - Docs caution — adds a
:::cautioncallout on the data-schemas page noting the restriction and showing the struct workaround.
DeepSeek Pro (free via Pullfrog for OSS) (Kimi K2 not used — the program covers this model; add its provider key to run your pick) ｜ 𝕏
| */ | ||
| <TElement extends AnyWgslData>( | ||
| elementType: ForbiddenDecoratedArrayElement<TElement>, | ||
| count?: number, |
Contributor
There was a problem hiding this comment.
The parameter name count in this error overload differs from elementCount used in the two productive overloads below and in the JSDoc @param. Consider renaming to elementCount for consistency.
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.
Fixes #2754
Throws when a non-
@locationdecorated element type is passed toarrayOf.@locationon array elements remains allowed, since it's used effectively withvertexLayout.