Skip to content

fix: Restrict using slot for strings#2700

Open
aleksanderkatan wants to merge 52 commits into
mainfrom
fix/restrict-using-slot-for-strings
Open

fix: Restrict using slot for strings#2700
aleksanderkatan wants to merge 52 commits into
mainfrom
fix/restrict-using-slot-for-strings

Conversation

@aleksanderkatan

@aleksanderkatan aleksanderkatan commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Includes changes from #2703 by @iwoplaza

@github-actions

github-actions Bot commented Jul 9, 2026

Copy link
Copy Markdown

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

🟢 Decreased (max -0.20%) ➖ Unchanged 🔴 Increased (max 0.21%) ❔ Unknown
262 36 23 0

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

Test tsdown
tgpu_init.ts 256.72 kB ($${\color{green}-3.2\%}$$)
tgpu_initFromDevice.ts 256.21 kB ($${\color{green}-3.4\%}$$)
tgpu_resolve.ts 187.44 kB ($${\color{green}-29.3\%}$$)
tgpu_resolveWithContext.ts 187.37 kB ($${\color{green}-29.3\%}$$)
tgpu_bindGroupLayout.ts 68.97 kB ($${\color{green}-74.0\%}$$)
tgpu_mutableAccessor.ts 65.90 kB ($${\color{green}-75.1\%}$$)
tgpu_accessor.ts 65.88 kB ($${\color{green}-75.2\%}$$)
tgpu_privateVar.ts 65.26 kB ($${\color{green}-75.4\%}$$)
tgpu_workgroupVar.ts 65.26 kB ($${\color{green}-75.4\%}$$)
tgpu_const.ts 64.49 kB ($${\color{green}-75.7\%}$$)
tgpu_fn.ts 38.06 kB ($${\color{green}-85.6\%}$$)
tgpu_fragmentFn.ts 37.99 kB ($${\color{green}-85.7\%}$$)
tgpu_vertexFn.ts 37.81 kB ($${\color{green}-85.7\%}$$)
tgpu_computeFn.ts 37.51 kB ($${\color{green}-85.9\%}$$)
tgpu_vertexLayout.ts 26.60 kB ($${\color{green}-90.0\%}$$)
tgpu_comptime.ts 14.40 kB ($${\color{green}-94.6\%}$$)
tgpu_unroll.ts 2.59 kB ($${\color{green}-99.0\%}$$)
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 Jul 9, 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.69, 1.33, 2.65, 4.26, 5.15, 7.37, 13.87, 15.06]
  line [0.67, 1.33, 2.90, 4.29, 4.61, 7.40, 14.18, 17.30]
  line [0.70, 1.33, 2.78, 3.80, 4.64, 7.47, 13.82, 15.07]
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.20, 0.34, 0.46, 0.54, 0.77, 0.76, 0.92, 1.06]
  line [0.21, 0.46, 0.50, 0.62, 0.83, 0.87, 1.00, 1.09]
  line [0.20, 0.39, 0.45, 0.57, 0.74, 0.81, 1.02, 1.07]
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.73, 1.58, 2.66, 4.15, 8.14, 17.05, 36.32, 74.97]
  line [0.69, 1.48, 3.15, 4.26, 8.02, 16.86, 36.96, 75.77]
  line [0.76, 1.57, 3.35, 4.08, 8.18, 16.72, 36.58, 73.72]
Loading

@aleksanderkatan aleksanderkatan force-pushed the fix/restrict-using-slot-for-strings branch from 1d3e3cd to 7ee8f2d Compare July 9, 2026 09:23
@aleksanderkatan

Copy link
Copy Markdown
Contributor Author

@pullfrog review

@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 — This PR prevents raw WGSL string injection through external maps, slots and TGSL expressions by requiring strings to carry a known schema type or be wrapped in a dedicated ResolvableString marker used for argument names.

  • Adds ResolvableString to wrap raw-WGSL argument identifiers used during entry-function header generation.
  • Tightens ResolutionCtx.resolve so that a bare string without a concrete schema throws a clear error instead of being injected.
  • Replaces ctx.resolve(value, dataType).value calls with ctx.resolveSnippet across generation code for consistency.
  • Removes strings from the isWgsl / Wgsl type union, since they are no longer first-class resolvable values.
  • Adds tests covering direct, indirect, slot-based and TGSL string-injection cases, and updates existing slot tests to use d.v3f values instead of WGSL string snippets.

Pullfrog  | View workflow run | Using Kimi K2 (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 — This PR prevents raw WGSL string injection through external maps, slots and TGSL expressions by requiring strings to carry a known schema type or be wrapped in a dedicated ResolvableString marker used for argument names.

  • Adds ResolvableString to wrap raw-WGSL argument identifiers used during entry-function header generation.
  • Tightens ResolutionCtx.resolve so that a bare string without a concrete schema throws a clear error instead of being injected.
  • Replaces ctx.resolve(value, dataType).value calls with ctx.resolveSnippet across generation code for consistency.
  • Removes strings from the isWgsl / Wgsl type union, since they are no longer first-class resolvable values.
  • Adds tests covering direct, indirect, slot-based and TGSL string-injection cases, and updates existing slot tests to use d.v3f values instead of WGSL string snippets.

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

@aleksanderkatan aleksanderkatan marked this pull request as ready for review July 9, 2026 15:52
@pullfrog

pullfrog Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Run failed. View the logs →

Pullfrog  | Rerun failed job ➔View workflow run | via Pullfrog | Using Kimi K2 (free via Pullfrog for OSS) | 𝕏

@pullfrog

pullfrog Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Important

The runtime guard now rejects strings, but the Wgsl type still advertises them. Tightening the type would keep string-injection errors compile-time instead of runtime.

Reviewed changes — restrict raw string injection into WGSL and route internal identifier strings through a new ResolvableString wrapper.

  • Introduced internal ResolvableString so raw-function positional argument names can still be emitted as valid WGSL identifiers.
  • Updated ResolutionCtxImpl.resolve to throw when a string is resolved without a concrete schema, blocking string injection via externals, slots, and TGSL identifiers.
  • Converted generator/accessor sites from ctx.resolve(value, dataType) to ctx.resolveSnippet(snippet).
  • Updated tests to stop relying on string-valued slots and added coverage for the new restrictions plus a comptime-string-slot test.

⚠️ Wgsl type/runtime mismatch

packages/typegpu/src/types.ts:75 still defines Wgsl as Eventual<string | number | boolean | ResolvableObject>, but isWgsl at types.ts:425 no longer accepts strings. The mismatch means users can pass a string to resolveWithContext(...).externals or the low-level resolve(item: Wgsl, ...) and have it type-check but throw at runtime. Removing string from the Wgsl type would make this restriction compile-time.

⚠️ String injection still possible with a concrete schema

packages/typegpu/src/resolutionCtx.ts:961 throws for strings only when no schema (or UnknownData) is supplied, but the branch at resolutionCtx.ts:968 still returns snip(item, schema, 'runtime') when a concrete schema is provided. This is an internal hole in the "no raw strings" rule. If no internal call site depends on it, the branch should be removed; otherwise it warrants a comment explaining the legitimate use case.


Unable to submit via the review API in this environment, so posting the review as a comment.

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

@pullfrog

pullfrog Bot commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

Technical note: I attempted to submit the above feedback via create_pull_request_review multiple times, but the MCP tool consistently skipped because it did not receive the body/comments parameters from my calls. I was unable to get that tool to create an actual PR review in this run.

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

Base automatically changed from fix/external-traversal-for-raw-wgsl-implemented-functions to main July 10, 2026 08:24
Copilot AI review requested due to automatic review settings July 10, 2026 10:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@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 latest commit aligns the type system with the runtime string-injection restriction.

  • Removed string from Wgsl and isWgsl() so plain JS strings no longer type-check as resolvable values, matching the runtime rejection added earlier.
  • Clarified the scoped string-identifer path in ResolutionCtx.resolve() with an inline example showing how identifier snippets carry their concrete schema.

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

@aleksanderkatan aleksanderkatan linked an issue Jul 10, 2026 that may be closed by this pull request
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: Restrict using tgpu.slot for injecting raw shader code

3 participants