Skip to content

fix: accept scalar-vector unions across std helpers - #2962

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

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

Conversation

@oxura

@oxura oxura commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Closes #2821. Separate scalar and vector overloads reject a value typed as number | d.v2f, even when both members belong to the same supported unary domain.

Scope

At current head d133ad846d281cb1fcaa3aa128c075aa6669eab6, this PR changes unary helpers only:

  • Floating-point unary helpers, including floor, sign, inverse trigonometry, trigonometric/exponential/logarithmic functions and rounding helpers.
  • Corresponding integer/numeric unary helpers, length, frexp and modf.
  • Unary boolean negation, arithmetic negation, derivatives and subgroup operations.

Scalar literals widen to their scalar result type; vector shapes and supported vector domains are retained. CPU bodies and GPU-side signatures are unchanged.

Following #2962 (comment), all multi-argument signature changes have been reverted to the original base behavior, including arithmetic, selection and indexed subgroup operations. The existing clamp/step generic constraints are preserved, not redesigned. The reviewer's exact std.max<d.v2f | number>(d.vec2f(), 1) counterexample is now a compile-time rejection covered by a regression assertion, alongside min.

Verification

Fresh local verification on Node 24.15.0:

  • scalarVectorUnions.test.ts and sign.test.ts: 96 tests passed.
  • Complete TypeGPU package unit suite: 193 files, 2,825 tests passed.
  • pnpm --filter typegpu test:types: passed, including unary inference/domain/literal assertions and the explicit-union max/min rejection regressions.
  • Changed-file type-aware Oxlint: zero warnings/errors; Oxfmt check passed.

A real public-API consumer, built with the normal Bun TypeGPU plugin and executed from its emitted bundle, evaluated scalar/vector acos to 0 and [0, 1.5707963705062866], and matching-vector max to [3, 4]. Resolving a typed function shell produced:

fn shader(value: vec2f) -> vec2f {
  return max(acos(value), vec2f(0, 1));
}

No actual GPU-device execution is claimed. Earlier hosted checks and Pullfrog results belong to the previous head and are not presented as verification of this revision. AI-assisted follow-up without subagents.

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) | 𝕏

@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 afb81e44), merge commit d71b337 updated the type signatures and expanded the regression coverage:

  • cpuFloor / cpuSign union overloads — each function now keeps the scalar (value: number) => number overload, adds a vector-only overload, and adds a dedicated <T extends …VecInstance>(value: T | number): T | number overload, so number | vec unions infer the vector member and return the same union while narrow scalar/vector calls keep their precise types.
  • Regression tests — sign.test.ts now pins union return types (number | v2f), numeric-literal widening inside a union, generic vector/scalar return preservation, and rejection of unsupported vector types via @ts-expect-error plus runtime toThrow('Unsupported signature').

I verified locally: npx vitest run tests/std/numeric/sign.test.ts → 7/7 pass; pnpm --filter typegpu test:types → clean. Removing the two new union-overload lines makes tsc fail on the new assertions (TS2769/TS2344), so the tests genuinely pin the fix rather than passing vacuously. cpuFloor/cpuSign are only referenced within numeric.ts, and the GPU-side signature fields are untouched.

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

@aleksanderkatan

aleksanderkatan commented Oct 6, 2026 •

Copy link
Copy Markdown
Member

Could you generalize this PR to be a solution for all functions with similar signature, like acos, acosh, asin...?

@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 d71b337d), commit df5d5240 broadened the scalar/vector union support from floor/sign to nearly every std helper:

  • numeric.ts union overloads — added a third <T extends …VecInstance | number>(…) => WidenNumber<T> overload to ~50 helpers, keeping the existing scalar and vector overloads and widening literal scalars through the new distributive WidenNumber<T>.
  • operators.ts — add/sub/mul now return WidenNumber<Lhs | Rhs>; div/mod/neg gained union overloads covering same-shape, broadcast, and mixed forms.
  • boolean.ts, derivative.ts, subgroup.ts — not/select, the derivative signature, and the five subgroup Identity* interfaces each gained a union overload.
  • scalarVectorUnions.test.ts — new 124-test suite pinning union return types, literal widening, preserved vector/scalar domains, unsupported-type rejection, and runtime CPU behavior on both scalar and vector branches.

I verified locally: npx vitest run tests/std/numeric/scalarVectorUnions.test.ts tests/std/numeric/sign.test.ts → 131/131 pass; the full packages/typegpu unit suite → 2860/2860 pass; npx tsc --noEmit → clean; npx oxlint on all six changed files → 0 warnings/errors. The changes are type-signature only — normalImpl bodies and the GPU-side signature fields are untouched, so codegen is unaffected.

ℹ️ PR description still scopes the change to floor/sign

The title and body describe replacing the split overloads of std.floor/std.sign, but df5d5240 extends the same pattern to roughly fifty std helpers and adds a 1051-line test file. The description no longer reflects what a reviewer or future reader sees in the diff.

Technical details
# Update the PR title and body to match the broadened scope

## Required outcome
- The title/body should state that the union fix now spans the scalar/vector `std` helpers generally, not only `floor` and `sign`.
- The body's "replaces the split overloads with one identity-preserving generic" wording is outdated: the final shape is additive (scalar overload + vector overload + union overload), not a replacement.

## Open questions for the human (optional)
- If this broadening was intended to be a separate PR, consider splitting it; otherwise update the description before merge.

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

@oxura

oxura commented Oct 8, 2026

Copy link
Copy Markdown
Contributor Author

Yes — I’ll generalize this to the other unary functions with the same scalar/vector signature, including the inverse trigonometric functions, and verify union inference plus actual CPU/WGSL consumers. I’ll preserve each function’s existing scalar/vector restrictions rather than broadening unrelated inputs.

@oxura oxura changed the title fix: accept scalar-vector unions in floor and sign fix: accept scalar-vector unions across std helpers Oct 9, 2026
@oxura

oxura commented Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Updated the title/body to match the full implementation rather than floor/sign only. The existing df5d524 head covers the unary float family including acos/asin, matching numeric/arithmetic and boolean/derivative/subgroup signatures, while retaining the existing scalar/vector/matrix restrictions. Fresh Node 24.15.0 validation: 131 focused tests passed and the package typecheck passed. A separate consumer using the normal Bun TypeGPU plugin computed acos(1)=0 and vector acos([1,0])=[0,1.5707963705062866], then resolved an actual WGSL vec2f function returning acos(value). No source rewrite was needed, no hardware execution is claimed, and the independently reported 2,860-test run is clearly attributed rather than presented as mine.

Comment thread packages/typegpu/src/std/numeric.ts Outdated
type VariadicOverload = {
(fst: number, ...rest: number[]): number;
<T extends NumVec>(fst: T, ...rest: T[]): T;
<T extends NumVec | number>(fst: T, ...rest: T[]): WidenNumber<T>;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This allows for an invalid signature. I would refrain from updating multi-argument functions.

it('works', () => {
  const main = () => {
    'use gpu';
    return std.max<d.v2f | number>(d.vec2f(), 1);
  };

  expect(tgpu.resolve([main])).toMatchInlineSnapshot();
});

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you are right: a shared union type parameter does not correlate multiple operands. Fixed in d133ad8. I reverted all multi-argument signature changes to the PR base (including arithmetic, select and indexed subgroup operations), while retaining only unary union support and the original clamp/step constraints. Your exact max counterexample and the corresponding min case are now compile-time rejection regressions. Fresh local checks: 96 focused tests, all 2,825 TypeGPU package tests, package typecheck, type-aware changed-file lint and formatting passed on Node 24.15.0. A normal Bun-plugin public-API consumer also executed scalar/vector acos and matching-vector max, and resolved valid vec2f WGSL. No GPU-device execution is claimed. Updated the PR body to describe the narrower scope and current-head evidence.

@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 df5d5240), commit d133ad84 narrows the union work to unary helpers in response to human review 5469644229:

  • Reverted all multi-argument signature changes to base — add/sub/mul/div/mod and their overloads, select, and the indexed subgroup Identity* interfaces are back to their original shapes, so a shared union type parameter can no longer accept mismatched operands (std.max<d.v2f | number>(d.vec2f(), 1) is again a compile error, now pinned by a regression).
  • Kept unary union overloads — the ~40 unary numeric.ts helpers, length/frexp/modf, cpuNot, the derivative signature, the two unary subgroup interfaces, and cpuNeg retain the <T extends Vec | number>(value: T): WidenNumber<T> overload; floor/sign keep their extra <T extends Vec>(value: T | number): T | number overload.
  • Rewrote scalarVectorUnions.test.ts to unary coverage — compile-time expectTypeOf(...).returns assertions plus if (false) blocks for @ts-expect-error cases, and runtime CPU checks on implemented scalar/vector branches.

I verified locally at d133ad84: npx vitest run tests/std/numeric/scalarVectorUnions.test.ts tests/std/numeric/sign.test.ts → 96/96 pass; the full packages/typegpu suite → 193 files, 2825/2825 pass; npx tsc --noEmit → clean. Reverting the multi-arg overloads genuinely restores the reviewer's rejection while the unary helpers still accept number | vec unions, so the fix for #2821's unary surface is intact. The PR description now matches the delivered scope.

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.

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

3 participants