Repository navigation
Conversation
There was a problem hiding this comment.
🟡 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/cpuSignoverload structure to allow scalar-vector union inputs to type-check. - Adds a regression test exercising
signandfloorwith anumber | 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
cpuSignis 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 as0.5but runtime returns1). Keeping a scalar overload returningnumberavoids this unsoundness while still allowingnumber | v2f-style unions via a single generic overload (same pattern ascpuAbsat 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.
There was a problem hiding this comment.
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 thenumber/AnyFloatVecInstanceoverload pairs ofcpuFloorandcpuSigninto single identity generics over… | number, so anumber | vecunion is accepted while the implementation is unchanged.packages/typegpu/tests/std/numeric/sign.test.ts— added a scalar-vector union regression test exercising bothsignandfloorwith anumber | v2fvalue (real regression guard viatest:types, since theascast is erased at runtime).
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ 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): numberoverload on top of the union generic for bothcpuFloorandcpuSign, matching the establishedcpuAbs/cpuFract/cpuMax/cpuMin/cpuMixpattern. Scalar calls again widen tonumber, whilenumber | vecunions still pass through the generic.packages/typegpu/tests/std/numeric/sign.test.ts— addedexpectTypeOf(sign(2.5 as const)).toEqualTypeOf<number>()andexpectTypeOf(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.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
There was a problem hiding this comment.
✅ 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/cpuSignunion overloads — each function now keeps the scalar(value: number) => numberoverload, adds a vector-only overload, and adds a dedicated<T extends …VecInstance>(value: T | number): T | numberoverload, sonumber | vecunions infer the vector member and return the same union while narrow scalar/vector calls keep their precise types.- Regression tests —
sign.test.tsnow 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-errorplus runtimetoThrow('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.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
|
Could you generalize this PR to be a solution for all functions with similar signature, like |
There was a problem hiding this comment.
✅ 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.tsunion 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 distributiveWidenNumber<T>.operators.ts—add/sub/mulnow returnWidenNumber<Lhs | Rhs>;div/mod/neggained union overloads covering same-shape, broadcast, and mixed forms.boolean.ts,derivative.ts,subgroup.ts—not/select, the derivative signature, and the five subgroupIdentity*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.deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏
|
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. |
|
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. |
| 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>; |
There was a problem hiding this comment.
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();
});There was a problem hiding this comment.
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.
There was a problem hiding this comment.
✅ 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/modand their overloads,select, and the indexed subgroupIdentity*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.tshelpers,length/frexp/modf,cpuNot, the derivative signature, the two unary subgroup interfaces, andcpuNegretain the<T extends Vec | number>(value: T): WidenNumber<T>overload;floor/signkeep their extra<T extends Vec>(value: T | number): T | numberoverload. - Rewrote
scalarVectorUnions.test.tsto unary coverage — compile-timeexpectTypeOf(...).returnsassertions plusif (false)blocks for@ts-expect-errorcases, 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.
deepseek-v4.1-flash (free via Pullfrog for OSS) | 𝕏

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:floor,sign, inverse trigonometry, trigonometric/exponential/logarithmic functions and rounding helpers.length,frexpandmodf.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/stepgeneric constraints are preserved, not redesigned. The reviewer's exactstd.max<d.v2f | number>(d.vec2f(), 1)counterexample is now a compile-time rejection covered by a regression assertion, alongsidemin.Verification
Fresh local verification on Node 24.15.0:
scalarVectorUnions.test.tsandsign.test.ts: 96 tests passed.pnpm --filter typegpu test:types: passed, including unary inference/domain/literal assertions and the explicit-unionmax/minrejection regressions.A real public-API consumer, built with the normal Bun TypeGPU plugin and executed from its emitted bundle, evaluated scalar/vector
acosto0and[0, 1.5707963705062866], and matching-vectormaxto[3, 4]. Resolving a typed function shell produced: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.