Skip to content

fix: validate unresolved declaration initializers - #2946

Open
huytdps13400 wants to merge 6 commits into
software-mansion:mainfrom
huytdps13400:fix/2934-unknown-data-diagnostic
Open

fix: validate unresolved declaration initializers#2946
huytdps13400 wants to merge 6 commits into
software-mansion:mainfrom
huytdps13400:fix/2934-unknown-data-diagnostic

Conversation

@huytdps13400

@huytdps13400 huytdps13400 commented Aug 30, 2026

Copy link
Copy Markdown

Summary

  • reject let and const declarations whose initializer resolves to void, including void-returning function calls and bare undefined
  • route those declarations through the existing cannot-determine-WGSL-type diagnostic
  • show schema-wrapping guidance only when the unresolved runtime value can actually be wrapped by a schema
  • retain valid object/array guidance while suppressing invalid Schema(null), Schema(undefined), and string suggestions
  • cover let/const, null/string diagnostics, bare undefined, and void-call results

Closes #2934

Verification

  • focused declaration/type-inference tests: 39 passed
  • pnpm --filter typegpu test:types: passed
  • pnpm test:style: passed
  • pnpm test:fast-unit: 216 files passed, 2,815 tests passed, 2 skipped

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.

Pull request overview

This PR improves diagnostics during TGSL let/const type inference by treating a bare undefined initializer (represented as wgsl.Void) the same way as other “untyped/unknown” RHS values, emitting the actionable “wrap with Schema(...)” error message and adding regression tests.

Changes:

  • Detect const/let x = undefined initializers represented as Void and raise the same schema-wrapping diagnostic as for UnknownData.
  • Add snapshot regressions for null/undefined inference errors in const declarations.
  • Add a snapshot regression for undefined inference errors in let declarations.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
packages/typegpu/src/tgsl/wgslGenerator.ts Extends UnknownData initializer checks to also cover bare undefined (Void) initializers for let and const.
packages/typegpu/tests/tgsl/typeInference.test.ts Adds snapshot tests asserting schema-wrapping suggestions for const a = null and const a = undefined.
packages/typegpu/tests/tgsl/letDeclaration.test.ts Adds snapshot test asserting schema-wrapping suggestion for let a = undefined.

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

@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

  • wgslGenerator.ts — bare undefined initializers now reported as untyped. Both _letStatement (L1337) and _constStatement (L1429) now enter the existing schema-wrapping WgslTypeError path when eq.value === undefined && wgsl.isVoid(definitionDataType), alongside the UnknownData check. Previously let/const a = undefined surfaced the cryptic Value undefined is not resolvable to type void.
  • Regression tests for undefined in both declaration forms (letDeclaration.test.ts, typeInference.test.ts) plus a guard for the already-working null case.

I verified the fix empirically by reverting both conditions to base form: the pre-fix failure is Value undefined is not resolvable to type void, and the two undefined tests genuinely fail without the change. Worth noting the null test passes pre-fix too — null already resolves to UnknownData, so that snapshot guards existing behavior rather than pinning this fix. The check mirrors the bare-return idiom at _returnStatement (L1245), the only Void/undefined snippet producers are the undefined identifier and the void unary operator, let/const are the only declaration statement types (var is rejected at parse), and GlslGenerator extends WgslGenerator inherits the fix. Well-scoped, minimal, and consistent — thanks.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@cieplypolar

Copy link
Copy Markdown
Collaborator

Appreciate the effort, but issue is not solved. WgslGenerator still advices wrapping undefined, null or any string with an appropriate schema.

@huytdps13400

Copy link
Copy Markdown
Author

@cieplypolar Fixed in 11511f3f. WgslGenerator now keeps the schema-wrapping hint only for values that can actually be wrapped; null, undefined, and strings report the inference failure without suggesting invalid Schema(...) calls. I updated both let and const regressions, added const-string coverage, and retained the existing hint coverage for wrappable struct/array values. Verification: focused 36 tests, TypeGPU type tests, style gate, and the fast unit suite (216 files, 2,812 passed, 2 skipped) all pass.

@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 — delta since the last Pullfrog review of 2e80d3c8, i.e. commit 11511f3f:

  • New schemaWrappingSuggestion helper — the schema-wrapping hint was extracted out of the let/const WgslTypeError messages so it can be emitted conditionally instead of unconditionally.
  • Hint suppressed for non-wrappable RHS valuesnull, undefined, and string RHS values now only report cannot determine WGSL type of '...'; the previously suggested Schema(null) / Schema(undefined) / Schema("...") wrappings are dropped since none of those can produce a valid schema.
  • Snapshot updates + new tests — existing string/null snapshots in letDeclaration, typeInference, and tgslFn were updated, and new tests pin that the hint is dropped for undefined, null, and string literals while object-backed RHS values (e.g. structValue, arrayValue at typeInference.test.ts:628/644) keep it.

I checked the other throw sites that reference the same cannot determine WGSL type of wording — those two _letStatement/_constStatement branches are the only producers, and GlslGenerator extends WgslGenerator without overriding them, so GLSL inherits the change. All three touched test files pass (100/100).

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@cieplypolar

Copy link
Copy Markdown
Collaborator
  1. drop eq.value === undefined condition
  2. add the following test
it('...', () => {
  const noop = tgpu.fn([])(() => {});

  const f = tgpu.fn([])(() => {
    const a = noop();
  });

  expect(tgpu.resolve([f])).toMatchInlineSnapshot(`
    "fn noop() {}

    fn f() {
      let a = noop();
    }"
  `);
});
  1. add tests that verify let behavior

@huytdps13400

Copy link
Copy Markdown
Author

Addressed in 182e8c90.

  • Removed the eq.value === undefined && wgsl.isVoid(...) declaration guard so calls to void-returning functions remain valid.
  • Added the requested noop() regression; it resolves to let a = noop();.
  • Extended let coverage for string/null diagnostics without invalid schema hints, plus the bare-undefined void resolution error.

Verification:

  • focused declaration/type-inference tests: 38 passed
  • pnpm --filter typegpu test:types: passed
  • pnpm test:style: passed
  • pnpm test:fast-unit: 216 files passed, 2,814 tests passed, 2 skipped

@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 delta under review (182e8c90) reverts the bare-undefined guard from the earlier commits, so the PR no longer emits a schema-wrapping diagnostic for undefined — the delivered behavior no longer matches the PR title, the description, or the author's comment on #2934, all of which still claim that diagnostic. The code itself is in good shape and, on the merits, resolves #2934's actual complaint (bogus Schema(...) hints); the mismatch is the framing. Decide before merge whether to re-scope the description or re-implement the diagnostic in a form that skips void-call results.

Reviewed changes — delta since the Pullfrog review of 11511f3f (commit 182e8c90, "fix: preserve void declaration inference"):

  • Reverted the void-initializer guard — removed (eq.value === undefined && wgsl.isVoid(definitionDataType)) from both _letStatement and _constStatement (wgslGenerator.ts:1348/1435); bare undefined in declarations is back to the pre-PR Value undefined is not resolvable to type void, and const a = voidFn() style declarations are valid again.
  • New regression testletDeclaration.test.ts pins that assigning a void function call to a declaration still resolves (let a = noop();).
  • Repurposed undefined-hint tests — the "does not suggest wrapping undefined" tests became the null-hint test and new assertions that bare undefined reports not resolvable to void in both let (letDeclaration) and const (typeInference) forms.

⚠️ PR description, title, and linked-issue comment no longer describe the delivered behavior

At head, let/const a = undefined produces Value undefined is not resolvable to type void — the same pre-PR error the PR was opened against — and the tests now pin that as expected. Meanwhile the title ("fix: improve undefined variable diagnostics"), both summary bullets ("detect bare undefined initializers … report the same actionable schema-wrapping diagnostic"), and the author's comment on #2934 all assert the opposite.

The result is nonetheless coherent and arguably the better outcome: #2934's complaint is the bogus Schema(null) / Schema("...") hints, and suppressing them for null/string/undefined is exactly what head does (object/array-backed RHS values still get the hint). Two ways forward — pick one so the merged record matches reality:

  • Re-scope the description (and the comment on #2934) to say the change suppresses invalid schema-wrapping hints for null, strings, and undefined, and preserves void-call inference — and reconsider whether "Closes #2934" is the right framing.
  • Re-implement the bare-undefined diagnostic restricted to the literal identifier: _identifier('undefined') (wgslGenerator.ts:370-371) yields origin: 'constant' with value === undefined and dataType Void, while a void function call's result does not — so a guard can discriminate. The new void-call regression test covers the false-positive side.
Technical details
# Align PR scope/description with head behavior

## Affected sites
- packages/typegpu/src/tgsl/wgslGenerator.ts:1348, 1435 — UnknownData-only guards; the void/undefined check from 11511f3f is removed at head
- packages/typegpu/tests/tgsl/letDeclaration.test.ts:87-98 — new test pins `Value undefined is not resolvable to type void`
- packages/typegpu/tests/tgsl/typeInference.test.ts:334-346 — same, const form
- packages/typegpu/src/tgsl/wgslGenerator.ts:370-371 — `_identifier('undefined')` → origin `'constant'`; the discriminator available if re-adding the fix

## Required outcome
- The PR title/description and the #2934 comment must describe what the merged branch actually does (suppress bogus hints; void-call declarations supported), OR the undefined diagnostic must be re-added in a way that does not fire for void-call results.

## Open questions for the human
- Is "Closes #2934" still accurate once the undefined diagnostic is dropped? (#2934's complaint was the bogus hint, which is fixed — but the issue text also names `undefined`.)
- Should bare `undefined` keep the cryptic `not resolvable to type void` message, or get a dedicated one?

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

@huytdps13400 huytdps13400 changed the title fix: improve undefined variable diagnostics fix: avoid invalid schema wrapping hints Aug 31, 2026
@huytdps13400

Copy link
Copy Markdown
Author

Updated the PR title/description and corrected my earlier #2934 comment so the record now matches 182e8c90: invalid schema-wrapping hints are suppressed, valid object/array hints remain, and void-call declaration inference is preserved. No code change was needed.

@cieplypolar

cieplypolar commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

The following test should fail

it('supports assigning the result of a void function', () => {
    const noop = tgpu.fn([])(() => {});

    const f = tgpu.fn([])(() => {
      const a = noop();
    });

    expect(tgpu.resolve([f])).toMatchInlineSnapshot(`
      "fn noop() {}

      fn f() {
        let a = noop();
      }"
    `);
  });

Sorry for being unclear.

@huytdps13400

Copy link
Copy Markdown
Author

The following test should fail

it('supports assigning the result of a void function', () => {

    const noop = tgpu.fn([])(() => {});



    const f = tgpu.fn([])(() => {

      const a = noop();

    });



    expect(tgpu.resolve([f])).toMatchInlineSnapshot(`

      "fn noop() {}



      fn f() {

        let a = noop();

      }"

    `);

  });

Sorry for being unclear.

I got it

@huytdps13400 huytdps13400 changed the title fix: avoid invalid schema wrapping hints fix: validate unresolved declaration initializers Sep 1, 2026
@huytdps13400

Copy link
Copy Markdown
Author

Fixed in 080c9cec. Both const a = noop() and let a = noop() now fail through the unresolved-declaration diagnostic, and the invalid schema hint remains suppressed. Bare undefined follows the same path.

Verification:

  • focused declaration/type-inference tests: 39 passed
  • TypeGPU type tests: passed
  • style: passed
  • fast unit suite: 216 files, 2,815 passed, 2 skipped

@cieplypolar

Copy link
Copy Markdown
Collaborator

This test shouldn't be in letDeclaration.test.ts file

it('rejects assigning the result of a void function', () => {
  const noop = tgpu.fn([])(() => {});

  const f = tgpu.fn([])(() => {
    const a = noop();
  });

  expect(() => tgpu.resolve([f])).toThrowErrorMatchingInlineSnapshot(`
    [Error: Resolution of the following tree failed:
    - <root>
    - fn:f: 'const a = noop()' is invalid, cannot determine WGSL type of 'noop()']
  `);
});

@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 — delta since the Pullfrog review of 182e8c90 (080c9cec, "fix: reject void declaration initializers"), which re-adds void-initializer rejection in the broader form the earlier commits lacked:

  • Void-typed declaration initializers are now rejected. Both _letStatement (wgslGenerator.ts:1348) and _constStatement (wgslGenerator.ts:1435) throw the cannot determine WGSL type of '...' WgslTypeError when eq.dataType is UnknownData or void, covering bare undefined, the void unary, and void-returning function calls. A variable cannot be void-typed in WGSL, so the previous resolution of const a = noop() to let a = noop(); produced unusable output — rejecting it is the more principled outcome.
  • Regression tests updated to pin the new behavior. The former supports assigning the result of a void function test is now two rejection tests (const a = noop() and let a = noop()), and the bare-undefined tests assert the new diagnostic message for both let and const forms. The tests genuinely fail without the void guard.
  • PR framing re-aligned with head behavior. The title ("fix: validate unresolved declaration initializers") and description now describe rejecting void-valued initializers and suppressing bogus Schema(null) / Schema(undefined) / string hints, resolving the narrative mismatch flagged in the last review.

The rejection is well-scoped: wgsl.isVoid is strict, var is rejected at parse, the only Void snippet producers (undefined identifier, void unary, void-returning calls, and a few side-effect std builtins) are all non-assignable in valid WGSL, and no existing tests, docs, or examples assign a void-returning call. I re-ran the modified test files plus the full tests/tgsl suite (420 tests) — all green. GlslGenerator inherits both statement paths unchanged.

Pullfrog  | View workflow run | Using DeepSeek Flash (free via Pullfrog for OSS) | 𝕏

@huytdps13400

Copy link
Copy Markdown
Author

Fixed in c84e186d. The const a = noop() regression now lives with the unresolved const initializer cases in typeInference.test.ts; the direct let a = noop() regression remains in letDeclaration.test.ts. Test behavior and snapshots are unchanged.

Verification:

  • focused tests: 39 passed
  • TypeGPU type tests: passed
  • style: passed
  • fast unit suite: 216 files, 2,815 passed, 2 skipped

@cieplypolar

Copy link
Copy Markdown
Collaborator

Almost there, just delete duplicated test: tgslFn.test.ts line 1315

@huytdps13400

Copy link
Copy Markdown
Author

Fixed in 4fa3c88e. Removed the duplicate null-assignment regression from tgslFn.test.ts; the focused const coverage remains in typeInference.test.ts, and let coverage remains in letDeclaration.test.ts.

Verification:

  • focused three-file suite: 102 passed
  • TypeGPU type tests: passed
  • style: passed
  • fast unit suite: 216 files, 2,814 passed, 2 skipped

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.

chore: UnknownData rhs sometimes cannot be wrapped with appropriate schema

4 participants