Skip to content

fix: accept scalar-vector unions in floor and sign - #2962

Open
oxura wants to merge 2 commits into
software-mansion:mainfrom
oxura:fix/2821-std-union-signatures
Open

fix: accept scalar-vector unions in floor and sign#2962
oxura wants to merge 2 commits into
software-mansion:mainfrom
oxura:fix/2821-std-union-signatures

Conversation

@oxura

@oxura oxura commented Sep 2, 2026

Copy link
Copy Markdown

The CPU-facing overloads for std.floor and std.sign split scalars and vectors into separate signatures. TypeScript therefore rejected a value typed as number | d.v2f, even though both sides of that union are valid inputs and the implementation already handles both.

This replaces the split overloads with one identity-preserving generic over the existing scalar/vector domain. Narrow scalar and vector calls keep their precise return types, while unions now pass through as the same union. The regression exercises both functions with a number | v2f input.

Verification:

  • TypeGPU type tests
  • focused numeric runtime suite: 3 passed
  • changed-file Oxlint and Oxfmt checks
  • fast unit suite on Node 24: 2,809 passed, 2 skipped
  • typegpu package build

Closes #2821.

Copilot AI lite review requested due to automatic review settings September 2, 2026 08:31

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.

🟡 Changes recommended

The new identity-generic signatures for cpuFloor/cpuSign introduce type unsoundness for scalar numeric literals unless a scalar number -> number overload is retained.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates the TypeScript typing of the CPU-facing std.floor and std.sign implementations so that inputs typed as scalar/vector unions (e.g. number | d.v2f) are accepted and preserve union return types, and adds a regression test covering the union case.

Changes:

  • Simplifies cpuFloor/cpuSign overload structure to allow scalar-vector union inputs to type-check.
  • Adds a regression test exercising sign and floor with a number | v2f-typed value.
File summaries
File Description
packages/typegpu/src/std/numeric.ts Adjusts CPU-side function typings for floor and sign to support scalar/vector union inputs.
packages/typegpu/tests/std/numeric/sign.test.ts Adds a regression test ensuring sign and floor accept `number
Review details

Suppressed comments (1)

packages/typegpu/src/std/numeric.ts:1005

  • cpuSign is currently typed as an identity generic (<T>(e: T): T), which makes scalar numeric literals preserve their literal type (e.g. const x = 0.5 as const; sign(x) would be typed as 0.5 but runtime returns 1). Keeping a scalar overload returning number avoids this unsoundness while still allowing number | v2f-style unions via a single generic overload (same pattern as cpuAbs at numeric.ts:114-116).
function cpuSign<T extends AnySignedVecInstance | number>(e: T): T {
  return generalizeFn(Math.sign, [e]);
}
  • Files reviewed: 2/2 changed files
  • Comments generated: 2
  • Review effort level: Lite

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

Comment thread packages/typegpu/src/std/numeric.ts
Comment thread packages/typegpu/tests/std/numeric/sign.test.ts Outdated

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

Important

The union fix works, but the PR's claim that "narrow scalar and vector calls keep their precise return types" isn't fully true: the merged single generic makes scalar literal calls return the literal type instead of number. E.g. floor(1.5) is now typed 1.5 while its runtime value is 1, and sign(2.5) is typed 2.5 while returning 1. The repo already has an established pattern that keeps the scalar overload and accepts unions (cpuFract, cpuMax, cpuMin, cpuMix), so this can be fixed without giving up the new union support.

Reviewed changes

  • packages/typegpu/src/std/numeric.ts — collapsed the number / AnyFloatVecInstance overload pairs of cpuFloor and cpuSign into single identity generics over … | number, so a number | vec union is accepted while the implementation is unchanged.
  • packages/typegpu/tests/std/numeric/sign.test.ts — added a scalar-vector union regression test exercising both sign and floor with a number | v2f value (real regression guard via test:types, since the as cast is erased at runtime).

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

Comment thread packages/typegpu/src/std/numeric.ts
Comment thread packages/typegpu/src/std/numeric.ts

@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

Since the prior Pullfrog review (on 40e3649e), commit afb81e44 addresses the flagged scalar-literal precision loss:

  • packages/typegpu/src/std/numeric.ts — restored the scalar (value: number): number overload on top of the union generic for both cpuFloor and cpuSign, matching the established cpuAbs/cpuFract/cpuMax/cpuMin/cpuMix pattern. Scalar calls again widen to number, while number | vec unions still pass through the generic.
  • packages/typegpu/tests/std/numeric/sign.test.ts — added expectTypeOf(sign(2.5 as const)).toEqualTypeOf<number>() and expectTypeOf(floor(1.5 as const)).toEqualTypeOf<number>() to pin the widened scalar return type; verified these fail (TS2344) if the scalar overloads are removed, so they genuinely guard the fix.

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.

fix: Expand the signature of std.sign and std.floor to accept the union number | d.v2f | d.v3f | d.v4f

2 participants