Skip to content

fix: Let a decorator replace an existing one of the same kind - #3252

Open
bercedev wants to merge 3 commits into
software-mansion:mainfrom
bercedev:fix/override-same-kind-decorator
Open

bercedev wants to merge 3 commits into
software-mansion:mainfrom
bercedev:fix/override-same-kind-decorator

Conversation

@bercedev

Copy link
Copy Markdown

Closes #3250

As proposed in the issue, a new decorator now replaces an existing one of the same kind. Before, attribute() always prepended, so d.align(16, d.align(8, d.u32)) kept both and resolved to @align(16) @align(8) a: u32, which isn't valid WGSL.

  • Runtime: attribute() builds the new list with withAttribute(): the new attribute first, then the existing ones without any attribute of the same type. This applies to both Decorated and LooseDecorated.
  • Types: Decorate drops the same-type attribute from the tuple through a new WithoutAttribute helper. So d.align(16, d.align(8, d.u32)) is typed Decorated<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.ts checks the resolved WGSL and the struct's type for both of those examples.

  • On main the WGSL assertion fails (@align(16) @align(8) a…), and tsc -p packages/typegpu/tsconfig.json reports the expectTypeOf mismatch ([Align<16>, Align<8>]).
  • With the change, vitest run --project=!browser gives 3704 passed and 2 skipped (310 files), tsc for packages/typegpu is clean, and oxlint and oxfmt --check are 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.

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.
Copilot AI balanced review requested due to automatic review settings October 21, 2026 11:38

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 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

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 new withAttribute(), which puts the new attribute first and drops any existing attribute sharing its type, for both Decorated and LooseDecorated.
  • Type-level parity — Decorate drops same-type attributes from the tuple through the new WithoutAttribute helper, leaving non-tuple attribute arrays untouched.
  • Regression test — a new attributes.test.ts case pins the resolved WGSL and the exact inferred type for nested align/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.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

@cieplypolar cieplypolar left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@bercedev

Copy link
Copy Markdown
Author

synced with main and simplified it in bc74fdd: effectiveAttribs is gone, the decorated branch now checks that both sides have the same number of attributes and that each one has a same-type match with equal params. since a decorator now replaces one of the same kind, each type shows up once. the schemasEqual tests and the full typegpu suite pass (2981), and tsc on the package is clean.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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 on main used an effectiveAttribs helper 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 via attribsA.length !== attribsB.length plus a per-type find, and effectiveAttribs is deleted. This relies on the new guarantee that a decorator replaces a same-kind one, so each type appears 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.

Pullfrog  | View workflow run | Using deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: Wrapping a schema with multiple decorators of the same type generates invalid WGSL

3 participants