Repository navigation
feat: schemasEqualStructurally & schemasEqualNominally - #3209
Conversation
|
pkg.pr.new packages benchmark commit |
77eb7a9 to
d0d1927
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Nominal comparison accepts distinct, incompatible struct schemas, undermining accessor validation.
0 open findings
What changed in this PR
Introduces explicit structural and nominal schema comparisons in TypeGPU, with nominal matching for accessor validation.
Changes:
- Adds both comparison APIs and retains
deepEqualas a deprecated alias. - Migrates serialization tests and React schema stabilization to structural comparison.
- Adds nominal identity tests and renames existing comparison tests.
| File | Description |
|---|---|
| packages/typegpu/tests/serializeDataSchema.test.ts | Uses structural comparison for round trips. |
| packages/typegpu/tests/serial.test.ts | Updates resource schema comparisons. |
| packages/typegpu/tests/internal/schemasEqualStructurally.test.ts | Checks structural pointer comparison. |
| packages/typegpu/tests/internal/schemasEqualNominally.test.ts | Checks nominal pointer comparison. |
| packages/typegpu/tests/internal/deepEqual.test.ts | Removes replaced pointer tests. |
| packages/typegpu/tests/data/schemasEqualStructurally.test.ts | Preserves structural comparison coverage. |
| packages/typegpu/tests/data/schemasEqualNominally.test.ts | Adds nominal identity coverage. |
| packages/typegpu/tests/data/deepEqual.test.ts | Removes replaced structural tests. |
| packages/typegpu/src/data/schemasEqual.ts | Implements both comparison modes. |
| packages/typegpu/src/data/index.ts | Exports APIs and deprecated alias. |
| packages/typegpu/src/core/slot/accessor.ts | Switches validation to nominal comparison. |
| packages/typegpu-react/src/core/helper-hooks.ts | Uses structural schema stabilization. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Bundle size comparison (
|
| 🟢 Decreased | ➖ Unchanged | 🔴 Increased (max 5.16%) | ❔ Unknown |
|---|---|---|---|
| 0 | 347 | 6 | 2 |
import * as ... in PR vs import * as ... in target (did bundle size increase?):
| Test | tsdown |
|---|---|
| d_deepEqual.ts | 2.69 kB ( |
| d_schemasEqualNominally.ts | 2.69 kB |
| d_schemasEqualStructurally.ts | 2.69 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 [1.11, 2.19, 4.88, 7.62, 8.29, 13.36, 28.13, 28.27]
line [1.02, 2.12, 4.46, 7.34, 8.62, 13.55, 26.27, 27.36]
line [1.05, 2.14, 4.81, 7.26, 8.43, 13.61, 27.78, 30.56]
---
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.30, 0.53, 0.77, 0.95, 1.25, 1.30, 1.56, 1.69]
line [0.30, 0.52, 0.76, 0.94, 1.27, 1.28, 1.57, 1.70]
line [0.34, 0.59, 0.73, 0.87, 1.20, 1.26, 1.53, 1.66]
---
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.95, 2.24, 4.45, 7.77, 13.61, 29.04, 60.29, 122.21]
line [0.98, 2.18, 4.86, 8.29, 15.23, 27.30, 59.52, 122.95]
line [0.97, 2.31, 4.65, 7.82, 14.49, 28.33, 60.33, 121.16]
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
schemasEqualNominally does not compare structs/unstructs at all: it reports two distinct structs as equal. The PR's own nominal suite fails 8 assertions, and pnpm test:style also fails on schemasEqual.ts.
Reviewed changes
Reviewed the full diff introducing schemasEqualStructurally/schemasEqualNominally and the call-site migrations.
- New comparison API —
packages/typegpu/src/data/deepEqual.tsbecomesschemasEqual.ts, with structural/nominal exports over a sharedschemasEqual(a, b, structural);deepEqualis kept as a deprecated alias of the structural variant. - Call-site migration — react's
useStableSchemaand the serial/serialize tests use the structural function;packages/typegpu/src/core/slot/accessor.tsmatchesSchemauses the nominal one. - Test reorganization — the old
deepEqualsuites are removed and split into structural/nominal suites (plus internal implicit-pointer tests).
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ The new commit is a formatting pass only; it resolves the
oxfmtnit from the previous review. The remaining findings there are still open.
Reviewed changes
Re-reviewed only the delta between 77eb7a9 and d0d1927.
- Formatting —
oxfmtapplied acrossschemasEqual.ts, the two new comparison suites,serial.test.ts, andserializeDataSchema.test.ts; the extra blank line between the two exports is gone, andnpx oxfmt --checknow passes on those files. - No logic changes —
packages/typegpu/src/data/schemasEqual.ts:58still gates the struct branch onstructural, so the 8schemasEqualNominallyfailures reproduced against the previous commit still reproduce here (npx vitest run --project=!browser packages/typegpu/tests/data/schemasEqualNominally.test.ts→ 8 failed).
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
c47790e to
02acc88
Compare
There was a problem hiding this comment.
Caution
The nominal fix activates a regression at the matchesSchema call site. Now that schemasEqualNominally actually compares structs by identity, two existing accessor tests fail: pnpm test:unit reports 2 failed / 2918 passed (tests/accessor.test.ts > can provide parts of a bind group layout and > can provide a runtime-sized array).
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (c47790e → 02acc88), which is confined to packages/typegpu/src/data/schemasEqual.ts.
- Fixed the nominal struct skip — the
isWgslStruct/isUnstructbranch is no longer gated onstructural; in nominal mode it now returnsa === b, so two distinct struct instances are no longer reported equal. The 8 previously-failingschemasEqualNominallyassertions now pass. - Doc reflow — the
@example/import block in theschemasEqualNominallydoc comment was moved.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
02acc88 to
75e4e70
Compare
75e4e70 to
4fe1864
Compare
4fe1864 to
ea51187
Compare
There was a problem hiding this comment.
ℹ️ No critical issues — the accessor regression is fixed. One description mismatch noted inline.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (4fe1864 → ea51187), which touches only packages/typegpu/src/core/slot/accessor.ts.
- Accessor switched back to structural comparison —
matchesSchemausesschemasEqualStructurallyinstead ofschemasEqualNominally, restoring the historical acceptance of structurally-equal but distinct structs. The two previously failing accessor tests now pass (verified: accessor + both equality suites, 88 passed). - No other authored delta — the branch was squashed/force-pushed, but the full diff still contains just the schema-equality API, call-site migrations, and test reorganization.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
ea51187 to
0cd0729
Compare
There was a problem hiding this comment.
ℹ️ No new issues — the delta is a behavior-neutral clarity fix that addresses prior feedback.
Reviewed changes
Re-reviewed the delta since the prior pullfrog review (ea51187 → 0cd0729), which touches only packages/typegpu/src/data/schemasEqual.ts.
- Explicit nominal struct return — the
!structuralstruct branch now returnsfalseinstead ofa === b. Identity is already handled by thea === bearly return at the top ofschemasEqual, so the two are equivalent; the change makes the intent explicit and the inline comment was reworded to read correctly. - JSDoc reflow — the
@param/@returnsblock in theschemasEqualNominallydoc comment was repositioned.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
f1d2ff4 to
85dc7ed
Compare
85dc7ed to
8de223e
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Re-reviewed the delta since the last reviewed head (85dc7eda → 8de223ea), which is confined to packages/typegpu/src/data/schemasEqual.ts and the two data-level comparison suites.
- Order-independent attribute comparison — the decorated branch now compares attributes by type through a new
effectiveAttribshelper that keeps only the outermost attribute of each type, instead of walking theattribsarrays positionally. This matches TypeGPU's own resolution (attribute()prepends, and both theDecoratedImplconstructor andgetCustomAlignmentuse.find), soalign(16, align(8, f32))is correctly equivalent toalign(16, f32). - Tests updated — both suites flip
decorated6/decorated7(size(8, align(16, u32))vsalign(16, size(8, u32))) fromfalsetotrueand add outermost-wins cases.
Verified locally: full non-browser suite 3708 passed / 2 skipped, targeted equality/accessor/serial suites 103 passed, and oxfmt --check is clean on the changed files. All prior Pullfrog threads are resolved; the remaining open thread is the human question on decorator ordering, which this change answers.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
a404e4e to
71cca0e
Compare
cieplypolar
left a comment
cieplypolar
left a comment
There was a problem hiding this comment.
Looks good. Left one nit.
71cca0e to
e8f5315
Compare
e8f5315 to
e554a97
Compare
No description provided.