Skip to content

fix: validate comparison operand schemas - #2928

Open
huytdps13400 wants to merge 1 commit into
software-mansion:mainfrom
huytdps13400:fix/2924-strict-comparison-signatures
Open

fix: validate comparison operand schemas#2928
huytdps13400 wants to merge 1 commit into
software-mansion:mainfrom
huytdps13400:fix/2924-strict-comparison-signatures

Conversation

@huytdps13400

Copy link
Copy Markdown

Summary

  • reject mismatched vector kinds before CPU comparison execution
  • apply the same invariant to WGSL signature resolution for allEq, eq, ne, lt, le, gt, and ge
  • add a regression covering both CPU and generated-code paths

The TypeScript overloads already require matching vector schemas, but the runtime and generator paths accepted mixed kinds and could emit invalid WGSL such as vec3u < vec3f.

TDD evidence

RED: the focused test failed because neither the direct lt call nor tgpu.resolve threw for vec3u versus vec3f.

GREEN: the focused test passes and both paths now return the same descriptive unsupported-signature error.

Verification

  • focused lt regression: 3/3 passed
  • boolean std suite: 14 files, 49 tests passed
  • source suite with attest: 216 files, 2,794 passed, 2 skipped
  • built-artifact suite with attest: 184 files, 2,371 passed, 2 skipped
  • all checked-out workspace package typechecks passed
  • Oxlint and Oxfmt passed
  • all packages built successfully
  • circular dependency scan passed
  • git diff --check passed

Fixes #2924

Reject mismatched vector kinds consistently in CPU execution and WGSL signature resolution so comparisons cannot emit invalid mixed-schema expressions.

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.

Pull request overview

This PR tightens runtime and WGSL generator validation for boolean comparison helpers so mixed vector kinds (e.g., vec3u vs vec3f) are rejected consistently before CPU execution or WGSL emission, aligning behavior with existing TypeScript overload constraints.

Changes:

  • Added shared signature validation to reject mismatched operand vector kinds for allEq, eq, ne, lt, le, gt, and ge.
  • Added CPU-path guards for eq and lt to prevent executing comparisons on mixed-kinds vector instances.
  • Added a regression test ensuring both direct execution and tgpu.resolve fail with the same unsupported-signature error for mismatched vector schemas.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
packages/typegpu/src/std/boolean.ts Adds kind-matching validation in both signature resolution and CPU implementations for comparisons to prevent invalid mixed-kind comparisons/WGSL.
packages/typegpu/tests/std/boolean/lt.test.ts Adds a regression test covering mismatched vector schemas for both CPU execution and generator resolution paths.

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

@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.

Reviewed changes

  • Removed the type-promiscuous (...argTypes) signatures from eq/ne/lt/le/gt/ge and allEq in std/boolean.ts, replacing them with a shared comparisonSignature/matchingArgTypes helper backed by assertMatchingKinds, which throws on mismatched operand types at WGSL signature-resolution time.
  • Added the same assertMatchingKinds(lhs.kind, rhs.kind) guard to the CPU paths cpuEq/cpuLt (shared by allEq, eq, ne, lt, le, gt, ge), since the direct runtime call does not go through signature resolution.
  • Added a lt regression asserting both the CPU call and the tgpu.resolve([f]) GPU path throw the same "Unsupported signature…" error for a vec3u vs vec3f comparison.

Notes

  • The GPU path compares lhs.type (schema string) while the CPU path compares .kind; both resolve to the same strings for vector instances, so the two paths stay consistent — the test pins the shared message.
  • I checked existing callers (typegpu-radiance-cascades, typegpu-noise, std/boolean, core/root/init): all use same-kind operands and wrap scalar literal comparisons explicitly (e.g. d.vec2f(0)), so the stricter check breaks nothing. WGSL comparison operators require matching operand types, so rejecting scalar↔vector mixes aligns with valid WGSL.
  • The thrown error is a plain Error rather than SignatureNotSupportedError; this matches the file's sibling CPU impls (cpuNot, select), so I'm not flagging it as a change.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

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.

chore: More strict std generator path signatures

2 participants