Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[naga wgsl-in] vecN() constructors and let type conversions #7367

Open
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

andyleiserson
Copy link
Contributor

@andyleiserson andyleiserson commented Mar 18, 2025

Connections
Fix for #7356.

This pull request is on top of #7304, due to overlapping effects on naga output snapshots.

Description
Naga did not previously allow use of vec4() as a constructor, because it has no type information. However, the spec states that this constructor (and vec2/vec3) should return a vector of abstract ints. Depending on the context (e.g. the case of var v: vec4f = vec() from the associated issue), the vector of abstract ints may then be converted to another type. While writing test cases for this, I noticed that we weren't applying automatic conversions to the initializer of let declarations, so I fixed that as well, changing the code for handling let declarations to operate similarly to the code for var and const, which already handled conversions.

Testing
Added several snapshot tests to exercise various related cases, plus a few cases where a conversion is not possible in wgsl_errors. Type conversion for let declarations also fixes the CTS test webgpu:shader,validation,decl,let:type:case="array_sized".

Squash or Rebase? Squash

Checklist

  • Run cargo fmt.
  • Run taplo format.
  • Run cargo clippy --tests. If applicable, add:
    • --target wasm32-unknown-unknown
  • Run cargo xtask test to run tests.
  • Needs a CHANGELOG.md entry.

Copy link
Contributor

@jamienicol jamienicol left a comment

Choose a reason for hiding this comment

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

Love the links to the spec and the thorough test coverage 🥇

Copy link
Contributor

@jamienicol jamienicol left a comment

Choose a reason for hiding this comment

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

Great, LGTM!

@ErichDonGubler
Copy link
Member

Rebased to resolve conflicts.

@ErichDonGubler
Copy link
Member

@andyleiserson, @sagudev: Should we just merge this, and close #7304? Or do we need to merge that first?

@andyleiserson
Copy link
Contributor Author

I don't know if @jamienicol reviewed all the changes or just the ones that I added in this PR. And @jimblandy mentioned documenting ImplConst, which made me question whether there is some issue removing it.

@jamienicol
Copy link
Contributor

I only reviewed your changes @andyleiserson

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.

Initialiazing var foo: vec4f = vec4() fails with "type can't be inferred"
4 participants