Repository navigation
Conversation
Stacking two decorators of one kind, e.g. d.align(16, d.align(8, d.u32)), kept both and generated invalid WGSL (@align(16) @align(8)). The new decorator now replaces the existing one, at runtime and in the Decorate type. Closes software-mansion#3250.
There was a problem hiding this comment.
🟢 Approval recommended
The focused runtime and type changes agree, regression coverage verifies the intended behavior, and no blocking issues were identified.
No open issues 🎉
What changed in this PR
Fixes #3250 by making decorators replace existing attributes of the same kind instead of generating duplicate WGSL attributes.
Changes:
- Applies replacement at runtime while preserving unrelated attribute order.
- Aligns tuple types with runtime behavior.
- Adds regression checks for generated WGSL and inferred types.
| File | Description |
|---|---|
| packages/typegpu/tests/attributes.test.ts | Tests replacement behavior, attribute order, and inferred types. |
| packages/typegpu/src/data/attributes.ts | Implements runtime and type-level attribute replacement. |
Review effort: Balanced
There was a problem hiding this comment.
✅ No new issues found.
( ◦°^°◦) 🏴☠️ Avast! I read the whole diff, ran the typegpu test fleet (2931 tests, all passin') and the package type-check, and found this ship sound — nary a leak nor a broken plank. Replacin' same-kind decorators be workin' as promised.
Reviewed changes
Fixes #3250 by making a new decorator replace an existing one of the same kind, instead of stacking same-type attributes that resolve to invalid WGSL.
- Runtime replacement —
attribute()now routes through the newwithAttribute(), which puts the new attribute first and drops any existing attribute sharing itstype, for bothDecoratedandLooseDecorated. - Type-level parity —
Decoratedrops same-type attributes from the tuple through the newWithoutAttributehelper, leaving non-tuple attribute arrays untouched. - Regression test — a new
attributes.test.tscase pins the resolved WGSL and the exact inferred type for nestedalign/size.
I verified the runtime choke point is the only place DecoratedImpl/LooseDecoratedImpl are constructed, so every public decorator (including builtin via defineBuiltin) inherits the replacement, and every runtime consumer of .attribs uses .find/.some/.every, so the dedup is safe. Full non-browser suite (196 files / 2931 tests) passes and tsc -p packages/typegpu/tsconfig.json is clean.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
cieplypolar
left a comment
There was a problem hiding this comment.
Looks good. The repository’s main branch now has schemasEqualStructurally and schemasEqualNominally. After syncing with main, could you simplify them? They currently use effectiveAttribs to deduplicate attributes, which is no longer necessary after your changes.
|
synced with main and simplified it in bc74fdd: |
There was a problem hiding this comment.
✅ No new issues found.
( ◦°^°◦) 🏴☠️ Avast! Since me last look, the cap'n merged main and swabbed the deck o' schemasEqual.ts — the effectiveAttribs dedup bucket be gone, the attributes be compared straight by type now. Me checked the seas: the only port that builds decorated types be attribute(), and deserializin' sails through the same decorators, so no duplicate same-kind attribute can stow away. All 52 tests float, the type-check be clean, and this ship be ready to sail! ⚓
Reviewed changes
Since the prior pullfrog review, the branch merged main (bringing in the new schemasEqualStructurally/schemasEqualNominally helpers and their tests) and then simplified the decorated-attribute comparison:
- Merged
main— picked up the schema-equality helpers, which onmainused aneffectiveAttribshelper to collapse each attribute type to its first (outermost) occurrence. - Simplified the decorated-attribute comparison —
schemasEqual.ts's decorated branch now compares attribute arrays order-independently viaattribsA.length !== attribsB.lengthplus a per-typefind, andeffectiveAttribsis deleted. This relies on the new guarantee that a decorator replaces a same-kind one, so eachtypeappears at most once.
I verified the "one attribute per type" invariant holds end-to-end: attribute() in attributes.ts is the only runtime site that constructs DecoratedImpl/LooseDecoratedImpl, and deserialization (serial/schema.ts#applyAttrib) reverse-applies serialized attributes through the public decorators, so duplicates cannot be reintroduced. The stricter comparison is therefore reachable only with inputs that can no longer be produced. Targeted tests (attributes, align, both schemasEqual suites) pass 52/52 and tsc --noEmit on packages/typegpu is clean.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

Closes #3250
As proposed in the issue, a new decorator now replaces an existing one of the same kind. Before,
attribute()always prepended, sod.align(16, d.align(8, d.u32))kept both and resolved to@align(16) @align(8) a: u32, which isn't valid WGSL.attribute()builds the new list withwithAttribute(): the new attribute first, then the existing ones without any attribute of the sametype. This applies to bothDecoratedandLooseDecorated.Decoratedrops the same-type attribute from the tuple through a newWithoutAttributehelper. Sod.align(16, d.align(8, d.u32))is typedDecorated<U32, [Align<16>]>, matching what it resolves to. An attribute array that isn't a tuple is left as it is.The order of the other attributes doesn't change:
d.size(16, d.align(8, d.size(8, d.u32)))resolves to@size(16) @align(8) b: u32.Tests: a new case in
packages/typegpu/tests/attributes.test.tschecks the resolved WGSL and the struct's type for both of those examples.mainthe WGSL assertion fails (@align(16) @align(8) a…), andtsc -p packages/typegpu/tsconfig.jsonreports theexpectTypeOfmismatch ([Align<16>, Align<8>]).vitest run --project=!browsergives 3704 passed and 2 skipped (310 files),tscforpackages/typegpuis clean, andoxlintandoxfmt --checkare clean on the two files.I didn't run the browser project or the built-package tests.
Note: I used Claude Code while working on this.