fix: Reuse callbacks with matching function shells by oxura · Pull Request #3200 · software-mansion/TypeGPU

oxura · 2026-10-15T08:35:11Z

Closes #2453.

Reuse an already-shelled callback instead of treating its wrapper as a new JavaScript implementation. Signature checks now use schemasEqualNominally from #3209: matching arrays/pointers work, while distinct struct identities and pointer spaces stay distinct.

Merged main (7c853fa3) and dropped the old helper move. Shellless cache lookup uses the shared utility too.

Checked the TypeGPU source suite: 198 files / 2,960 tests passed. Package typecheck, focused type-aware Oxlint and Oxfmt pass; the core import graph has no circular dependencies. The current public consumer returns 42 on the CPU and rejects seven incompatible signatures. Its fresh WGSL also ran on RX 6600 / RADV / Vulkan (Mesa 26.2.4) and read back 42. No current hosted CI or cross-platform result claimed.

Code, tests and prose are AI-assisted (GPT-6.1 Sol, openai-codex/gpt-6.1-sol).

Copilot

🟡 Changes recommended

Reference equality rejects independently constructed pointer and array schemas with matching signatures.

1 open finding

Compare compound schemas structurally instead of by reference · New

What changed in this PR

Addresses #2453 by reusing already-shelled callbacks rather than interpreting their wrappers as new implementations.

Changes:

Adds callback reuse and signature validation.
Tests CPU execution, identity, WGSL preservation, externals, and incompatible signatures.

File	Description
packages/typegpu/tests/function.test.ts	Adds callback reuse and signature rejection tests.
packages/typegpu/src/core/function/tgpuFn.ts	Implements and documents reuse for matching shells.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

oxura · 2026-10-15T09:10:42Z

+      innerShell.returnType !== shell.returnType ||
+      innerShell.argTypes.length !== shell.argTypes.length ||
+      !innerShell.argTypes.every((type, index) => type === shell.argTypes[index])


Addressed in 6284ddb using the existing nominal-aware shallowEqualSchemas, now shared. Equivalent array arguments/returns and independently constructed pointers reuse correctly; different lengths, element types, nominal structs and pointer spaces reject. Full Node 24 suite: 2,840 tests passed. Generated array WGSL dispatched and read back 42 on RX 6600/Vulkan. AI-assisted: GPT-6.1 Sol, openai-codex/gpt-6.1-sol.

pullfrog

Important

The new signature check compares schemas by reference, so a wrapper whose signature is structurally identical but built from distinct schema objects still throws instead of reusing. Details inline.

Reviewed changes

Reuse matching shelled callbacks — createFn early-returns the already-shelled implementation when its shell's returnType/argTypes match the wrapping shell, preserving the original core, raw-WGSL body, externals and slot bindings; mismatches throw at construction time, before WGSL generation.
Tests — packages/typegpu/tests/function.test.ts gains three cases: JS-fn reuse (identity + CPU result + WGSL), raw-WGSL reuse preserving $uses externals, and rejection of mismatched arg/return arity and types.

^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

pullfrog

✅ No new issues found.

Reviewed changes

Changes since the prior pullfrog review (0fe5360 → 6284ddb) replace reference-equality signature matching with structural comparison and add coverage for it.

Structural schema matching — createFn now rejects a mismatched shell via shallowEqualSchemas instead of ===, so independently constructed but equivalent array and pointer schemas are reused rather than throwing.
Shared helper — shallowEqualSchemas moved from tgsl/shellless.ts into data/deepEqual.ts and is reused by both the callback-reuse path and the shellless cache, removing the duplicate.
Tests — added cases for structural array/pointer matching, mismatched array/pointer rejection, and nominal struct identity preservation; the prior review's open thread is addressed and resolved.

^{｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

…n' into sol50/typegpu-2453

oxura · 2026-10-15T12:10:52Z

Normally merged current main (no rebase/force push), now 9e2f0f2d. Reverified 194 test files / 2856 tests on Node24 and the package typecheck. The actual refreshed-source WGSL ran on RX6600 / RADV / Vulkan and read back 42; separately constructed equivalent arrays remain accepted and nominal struct mismatches remain rejected. No new semantic change to the reviewed fix.

iwoplaza · 2026-10-18T10:23:24Z

This PR should wait until #3209 is merged, and use those utilities

oxura · 2026-10-18T10:49:30Z

Agreed — holding this for #3209 so I can reuse its equality utilities.

iwoplaza · 2026-10-21T11:55:11Z

@oxura I merged #3209, you can go ahead and update your implementation to use schemasEqualNominally

oxura · 2026-10-21T18:34:18Z

Updated to schemasEqualNominally and removed the old helper move. Main is merged; typecheck, lint/format and 2,960 TypeGPU tests pass. The current consumer also ran on RX 6600 / Vulkan and read back 42.

pullfrog

ℹ️ No critical issues — one minor coverage suggestion inline.

Reviewed changes

Changes since the prior pullfrog review (9e2f0f2 → c6bd12c), which is a merge of main (7c853fa) into the branch:

Merged main and reconciled the PR's matching helper with main's newer utility.
Switched createFn and ShelllessRepository.get from the PR-local shallowEqualSchemas to main's shared schemasEqualNominally, and removed the data/deepEqual.ts helper the PR had added (that file was removed on main).
Dropped three expect(wrapped).toBe(...) identity assertions from the reuse tests.

I verified on this head that npx vitest run tests/function.test.ts tests/data/schemasEqualNominally.test.ts tests/tgsl passes (577 tests) and that reuse identity still holds for all three removed assertions.

^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏}

pullfrog · 2026-10-21T18:38:01Z

+    });
+    const wrapped = tgpu.fn([d.u32], d.u32)(callback);
+
+    expect(wrapped(41)).toBe(42);


The merge commit c6bd12c dropped the expect(wrapped).toBe(callback) assertions here and at the array case (line 42) and pointer case (wrapped/increment, line 64), but reuse still preserves identity for all three — I re-added them locally and they pass. Since direct identity reuse is the PR's core behavior, keeping these guards against a future regression that re-wraps while still emitting equivalent WGSL; the resolve/CPU assertions do not distinguish the two.

Keeping the checks on callback behaviour here: CPU/WGSL results, raw WGSL externals, and incompatible signatures. I removed the identity assertions intentionally so the tests do not constrain which JavaScript object implements that behaviour.

oxura added 2 commits October 15, 2026 14:22

fix: reuse callbacks with matching function shells

31099ab

test: assert callback WGSL and shell error transitions

0fe5360

Copilot AI balanced review requested due to automatic review settings October 15, 2026 08:35

Copilot started reviewing on behalf of oxura October 15, 2026 08:35

View session

Copilot AI reviewed Oct 15, 2026

View reviewed changes

pullfrog Bot reviewed Oct 15, 2026

View reviewed changes

Comment thread packages/typegpu/src/core/function/tgpuFn.ts Outdated

fix: match callback shells by nominal WGSL type

6284ddb

pullfrog Bot reviewed Oct 15, 2026

View reviewed changes

Merge remote-tracking branch 'refs/remotes/sol50-refresh-20261015/mai…

9e2f0f2

…n' into sol50/typegpu-2453

iwoplaza changed the title ~~fix: reuse callbacks with matching function shells~~ fix: Reuse callbacks with matching function shells Oct 15, 2026

fix: reuse shared nominal schema equality for callbacks

c6bd12c

pullfrog Bot reviewed Oct 21, 2026

View reviewed changes

Navigation Menu

Repository navigation

fix: Reuse callbacks with matching function shells - #3200

fix: Reuse callbacks with matching function shells#3200
oxura wants to merge 5 commits into
software-mansion:mainfrom
oxura:sol50/typegpu-2453

oxura commented Oct 15, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

oxura Oct 15, 2026

Uh oh!

pullfrog Bot left a comment

Uh oh!

Uh oh!

pullfrog Bot left a comment

Uh oh!

oxura commented Oct 15, 2026

Uh oh!

iwoplaza commented Oct 18, 2026

Uh oh!

oxura commented Oct 18, 2026

Uh oh!

iwoplaza commented Oct 21, 2026

Uh oh!

oxura commented Oct 21, 2026

Uh oh!

pullfrog Bot left a comment

Uh oh!

pullfrog Bot Oct 21, 2026

Uh oh!

oxura Oct 21, 2026

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

oxura commented Oct 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

oxura Oct 15, 2026

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

oxura commented Oct 15, 2026

Uh oh!

iwoplaza commented Oct 18, 2026

Uh oh!

oxura commented Oct 18, 2026

Uh oh!

iwoplaza commented Oct 21, 2026

Uh oh!

oxura commented Oct 21, 2026

Uh oh!

pullfrog Bot left a comment

Choose a reason for hiding this comment

Uh oh!

pullfrog Bot Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

oxura Oct 21, 2026

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

oxura commented Oct 15, 2026 •

edited

Loading