fix: Reject decorated element types in arrayOf - #2781
Open
vende11s wants to merge 4 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.89, 1.95, 4.13, 6.06, 7.20, 10.41, 21.66, 24.17]
line [0.90, 1.79, 3.91, 6.10, 7.30, 11.69, 22.11, 24.87]
line [0.93, 1.89, 4.55, 6.92, 7.73, 12.59, 22.27, 25.25]
---
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.28, 0.48, 0.66, 0.78, 1.06, 1.12, 1.35, 1.50]
line [0.26, 0.49, 0.64, 0.77, 1.09, 1.18, 1.30, 1.45]
line [0.35, 0.52, 0.66, 0.83, 1.11, 1.17, 1.39, 1.58]
---
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.90, 2.13, 3.98, 6.64, 11.70, 24.32, 53.80, 108.69]
line [0.69, 2.01, 3.60, 6.57, 11.72, 25.23, 55.43, 109.60]
line [0.83, 2.06, 3.98, 6.50, 12.80, 26.06, 57.96, 112.56]
|
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 0.91%) | ❔ Unknown |
|---|---|---|---|
| 0 | 296 | 26 | 0 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
| Test | tsdown |
|---|---|
| d_arrayOf.ts | 27.14 kB ( |
| d_builtin.ts | 29.61 kB ( |
import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):
| Test | tsdown |
|---|---|
| tgpu_init.ts | 264.69 kB ( |
| tgpu_initFromDevice.ts | 264.16 kB ( |
| tgpu_resolve.ts | 167.75 kB ( |
| tgpu_resolveWithContext.ts | 167.69 kB ( |
| tgpu_bindGroupLayout.ts | 71.83 kB ( |
| tgpu_mutableAccessor.ts | 68.22 kB ( |
| tgpu_accessor.ts | 68.22 kB ( |
| tgpu_privateVar.ts | 66.96 kB ( |
| tgpu_workgroupVar.ts | 66.96 kB ( |
| tgpu_const.ts | 66.44 kB ( |
| tgpu_lazy.ts | 66.32 kB ( |
| tgpu_fn.ts | 38.75 kB ( |
| tgpu_fragmentFn.ts | 38.75 kB ( |
| tgpu_vertexFn.ts | 38.57 kB ( |
| tgpu_computeFn.ts | 38.27 kB ( |
| tgpu_vertexLayout.ts | 27.39 kB ( |
| tgpu_comptime.ts | 15.07 kB ( |
| tgpu_unroll.ts | 1.66 kB ( |
| tgpu_slot.ts | 1.54 kB ( |
If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.
Contributor
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.
![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) ｜ 𝕏
reczkok
reviewed
Aug 5, 2026
Contributor
There was a problem hiding this comment.
Caution
This PR currently commits the .pnpm-store/ directory (~19 MB, 382 k+ lines, 300 files). This is the pnpm content-addressable cache and must not be in source control. Remove these files from the branch and add .pnpm-store to .gitignore before merging.
Reviewed changes
Reviewed the delta since the prior Pullfrog review at d11f072 (new commit 4519833).
- Renamed error-overload parameter from
counttoelementCountinpackages/typegpu/src/data/array.tsto match the productive overloads and JSDoc. - Tightened error message punctuation — added a trailing period to the decorated-element error string and updated the matching inline snapshots/type assertions.
- Added partial-application throw test in
packages/typegpu/tests/array.test.tsford.arrayOf(d.align(16, d.u32)). - Removed the misleading buffer usage test in
packages/typegpu/tests/buffer.test.tsper reviewer feedback; the test had been repurposed with a now-forbidden decorated array element and no longer tested its original intent.
🚨 .pnpm-store/ cache committed to the repo
The 4519833 commit adds ~300 .pnpm-store/v11/files/... entries. This is not source code; it is pnpm's local package cache. Committing it bloats the repository and the PR diff, and it is not present on main.
Technical details
# Remove committed pnpm store and prevent recurrence
## Affected sites
- Entire `.pnpm-store/` directory added in commit `4519833`.
- Root `.gitignore` currently lacks an entry for `.pnpm-store`.
## Required outcome
- Remove all `.pnpm-store/` entries from the PR branch history.
- Add `.pnpm-store` to the root `.gitignore`.
## Suggested approach
- Run `git rm -r --cached .pnpm-store` and commit the removal.
- Add `.pnpm-store` to `.gitignore` (e.g. under the `# dependencies` section alongside `node_modules`).
- If the files have already been pushed, consider an interactive rebase or a clean revert of the pnpm-store files to keep history tidy.Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Contributor
There was a problem hiding this comment.
✅ Prior critical issue resolved — one follow-up suggestion below.
Reviewed changes
Reviewed the new commit 0b881a2 pushed after the previous review.
- Removed accidentally committed
.pnpm-store/from the branch history.
ℹ️ Add .pnpm-store to .gitignore to prevent recurrence
The pnpm store is kept inside the repo (~/.pnpm-store is not being used here). Now that the directory has been removed from the branch, it will reappear as untracked on the next pnpm install and could be committed again. Add .pnpm-store to the root .gitignore to close the loop.
Technical details
# Prevent the pnpm store from being re-committed
## Affected sites
- Root `.gitignore` currently lacks an entry for `.pnpm-store`.
## Required outcome
- Add `.pnpm-store` to the root `.gitignore` (e.g. under `# dependencies` alongside `node_modules`).
## Suggested approach
- Edit `.gitignore`:
```diff
# dependencies
node_modules
+.pnpm-store
.pnp
.pnp.js
```
- Commit the change as part of this PR so the fix and the prevention travel together.Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
reczkok
approved these changes
Aug 11, 2026
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.