Skip to content

impr: Strict type checking on logical and relational binary expressions operands#2647

Open
cieplypolar wants to merge 8 commits into
fix/possible-side-effects-flowfrom
fix/unify-test-in-ternary
Open

impr: Strict type checking on logical and relational binary expressions operands#2647
cieplypolar wants to merge 8 commits into
fix/possible-side-effects-flowfrom
fix/unify-test-in-ternary

Conversation

@cieplypolar

@cieplypolar cieplypolar commented Jun 23, 2026

Copy link
Copy Markdown
Collaborator

Part of the #2645.

Addressed: <, >, >=, <=, ===, !==, || and &&.

Also, forced ternary operator to check if condition is boolean.

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown

pkg.pr.new

packages
Ready to be installed by your favorite package manager ⬇️

https://pkg.pr.new/software-mansion/TypeGPU/typegpu@36dd3677f7b03f2587734702c31cad8f457c2a77
https://pkg.pr.new/software-mansion/TypeGPU/@typegpu/cli@36dd3677f7b03f2587734702c31cad8f457c2a77
https://pkg.pr.new/software-mansion/TypeGPU/@typegpu/noise@36dd3677f7b03f2587734702c31cad8f457c2a77
https://pkg.pr.new/software-mansion/TypeGPU/@typegpu/react@36dd3677f7b03f2587734702c31cad8f457c2a77
https://pkg.pr.new/software-mansion/TypeGPU/unplugin-typegpu@36dd3677f7b03f2587734702c31cad8f457c2a77

benchmark
view benchmark

commit
view commit

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown

Bundle size comparison (import * as ... in PR vs import * as ... in target):

🟢 Decreased ➖ Unchanged 🔴 Increased (max 0.24%) ❔ Unknown
0 300 21 0

import { ... } in PR vs import * as ... in PR (is the library tree-Shakeable?):

Test tsdown
tgpu_init.ts 257.48 kB ($${\color{green}-3.2\%}$$)
tgpu_initFromDevice.ts 256.97 kB ($${\color{green}-3.4\%}$$)
tgpu_resolve.ts 188.19 kB ($${\color{green}-29.3\%}$$)
tgpu_resolveWithContext.ts 188.13 kB ($${\color{green}-29.3\%}$$)
tgpu_bindGroupLayout.ts 69.09 kB ($${\color{green}-74.0\%}$$)
tgpu_mutableAccessor.ts 66.09 kB ($${\color{green}-75.2\%}$$)
tgpu_accessor.ts 66.08 kB ($${\color{green}-75.2\%}$$)
tgpu_privateVar.ts 65.38 kB ($${\color{green}-75.4\%}$$)
tgpu_workgroupVar.ts 65.38 kB ($${\color{green}-75.4\%}$$)
tgpu_const.ts 64.60 kB ($${\color{green}-75.7\%}$$)
tgpu_fragmentFn.ts 38.04 kB ($${\color{green}-85.7\%}$$)
tgpu_fn.ts 38.01 kB ($${\color{green}-85.7\%}$$)
tgpu_vertexFn.ts 37.85 kB ($${\color{green}-85.8\%}$$)
tgpu_computeFn.ts 37.56 kB ($${\color{green}-85.9\%}$$)
tgpu_vertexLayout.ts 26.78 kB ($${\color{green}-89.9\%}$$)
tgpu_comptime.ts 14.47 kB ($${\color{green}-94.6\%}$$)
tgpu_unroll.ts 1.66 kB ($${\color{green}-99.4\%}$$)
tgpu_slot.ts 1.57 kB ($${\color{green}-99.4\%}$$)
tgpu_lazy.ts 1.22 kB ($${\color{green}-99.5\%}$$)

If you wish to run a comparison for other, slower bundlers, run the 'Tree-shake test' from the GitHub Actions menu.

@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown

Resolution Time Benchmark

---
config:
  themeVariables:
    xyChart:
      plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
  title "Random Branching (🔴 PR | 🔵 main | 🟢 release)"
  x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
  y-axis "time (ms)"
  line [0.97, 2.02, 4.24, 6.77, 7.54, 12.67, 23.30, 27.69]
  line [0.99, 2.02, 4.43, 7.35, 8.47, 12.85, 23.66, 26.95]
  line [0.95, 1.92, 4.46, 6.54, 7.49, 9.92, 20.82, 21.90]
Loading
---
config:
  themeVariables:
    xyChart:
      plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
  title "Linear Recursion (🔴 PR | 🔵 main | 🟢 release)"
  x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
  y-axis "time (ms)"
  line [0.28, 0.55, 0.76, 0.92, 1.17, 1.23, 1.46, 1.64]
  line [0.31, 0.55, 0.74, 0.90, 1.19, 1.28, 1.53, 1.67]
  line [0.30, 0.58, 0.75, 0.91, 1.29, 1.22, 1.44, 1.61]
Loading
---
config:
  themeVariables:
    xyChart:
      plotColorPalette: "#E63946, #3B82F6, #059669"
---
xychart
  title "Full Tree (🔴 PR | 🔵 main | 🟢 release)"
  x-axis "max depth" [1, 2, 3, 4, 5, 6, 7, 8]
  y-axis "time (ms)"
  line [0.99, 2.09, 4.49, 6.76, 13.38, 26.58, 55.62, 115.17]
  line [0.92, 2.31, 5.02, 7.27, 13.79, 27.67, 59.08, 120.08]
  line [0.82, 2.09, 3.95, 6.55, 12.98, 26.54, 59.00, 113.66]
Loading

@cieplypolar cieplypolar changed the base branch from main to fix/possible-side-effects-flow July 7, 2026 12:14
@cieplypolar cieplypolar marked this pull request as ready for review July 7, 2026 12:14

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

Reviewed changes — strict operand type checking for relational and logical binary expressions, plus forced boolean conversion for ternary conditions.

  • Added binaryRelationalOpToStdMap and runtime type checks in wgslGenerator.ts for <, <=, >, >=, ===, !==, &&, and ||.
  • Refactored comptime relational comparisons to use the same std-function suggestion map.
  • Forced ternary conditions to bool using tryConvertSnippet.
  • Added binaryLogicalOps.test.ts and moved short-circuit evaluation tests from wgslGenerator.test.ts.
  • Added ternary non-bool condition failure test.

Important

Two concrete issues to address before merging: an unreachable error branch in the new ternary handling, and a typo in the comptime vector-comparison suggestion message.

⚠️ Unreachable ternary error branch

tryConvertSnippet either returns a Snippet or throws a WgslTypeError; it never returns a falsy value. The added if (!convertedTest) branch is dead code, so the custom "Cannot convert condition to bool" message is never shown.

Technical details
# Unreachable ternary error branch

## Affected sites
- `packages/typegpu/src/tgsl/wgslGenerator.ts:920``if (!convertedTest)` is always false.

## Required outcome
- Remove the unreachable branch, or replace `tryConvertSnippet` with a conversion helper that returns `null`/`undefined` on failure so the custom error message can be surfaced.

## Suggested approach
Drop the `if (!convertedTest)` guard and rely on `tryConvertSnippet`'s own error. If the custom message is required, wrap the conversion in a `try/catch` and rethrow with the desired text.

⚠️ Typo in comptime vector suggestion message

The comptime path appends an extra single quote to the suggested std-function name. The output reads use 'std.ge''. instead of use 'std.ge'..

Technical details
# Typo in comptime vector suggestion message

## Affected sites
- `packages/typegpu/src/tgsl/wgslGenerator.ts:412` — template ends with `''.` instead of `'.`.
- `packages/typegpu/tests/tgsl/binaryLogicalOps.test.ts:271` — inline snapshot matches the typo.

## Required outcome
- Fix the trailing quote in the error template and update the matching inline snapshot.

## Suggested approach
Change `` ` For component-wise comparison, use 'std.${stdBinaryRelationalOp}''.` `` to `` ` For component-wise comparison, use 'std.${stdBinaryRelationalOp}'.` `` in `wgslGenerator.ts`, then update the snapshot in `binaryLogicalOps.test.ts`.

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

Comment thread packages/typegpu/src/tgsl/wgslGenerator.ts Outdated
Comment thread packages/typegpu/src/tgsl/wgslGenerator.ts Outdated
@iwoplaza

iwoplaza commented Jul 7, 2026

Copy link
Copy Markdown
Collaborator

Just a few notes from what we talked about in person:

  • If the left operand is known at comptime, we don't have to require the right side to be of type boolean since it becomes the whole expression
  • If the left operand is runtime known, then we should require both sides to be of type boolean in order to not confuse devs on behavior.

@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 — the review-fix commit addresses both issues flagged in the prior review.

  • Removed the unreachable if (!convertedTest) branch in the ternary operator path; conversion failures now surface through tryConvertSnippet's own WgslTypeError.
  • Fixed the extra trailing single quote in the vector-vector comparison suggestion message (std.ge''std.ge).
  • Updated the matching inline snapshot in binaryLogicalOps.test.ts.

I also ran the affected unit tests locally (binaryLogicalOps.test.ts, ternaryOperator.test.ts, wgslGenerator.test.ts) and all 122 tests passed.

Pullfrog  | View workflow run | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@cieplypolar cieplypolar force-pushed the fix/unify-test-in-ternary branch from 7f8469e to 36dd367 Compare July 9, 2026 11:54
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.

2 participants