[naga wgsl-in] Handle automatic type conversions for switch selector and case expressions #7250
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jamienicol
force-pushed
the
abstract-switch-case
branch
3 times, most recently
from
fe422ea to
b941b89
Compare
jamienicol
changed the title
sagudev
reviewed
Mar 2, 2025
ErichDonGubler
approved these changes
Mar 5, 2025
ErichDonGubler
added
type: bug
jamienicol
force-pushed
the
abstract-switch-case
branch
3 times, most recently
from
41a08d1 to
b705494
Compare
…and case expressions This allows abstract-typed expressions to be used for some or all of the switch selector and case selectors. If these are all not convertible to the same concrete scalar integer type we return an error. If all the selector expressions are abstract then they are concretized to i32. The note previously provided by the relevant error message, suggesting adding or removing the `u` suffix from case values, has been removed. While useful for simple literal values, it was comically incorrect for more complex case expressions. The error message should still be useful enough to allow the user to easily identify the problem.
ErichDonGubler
force-pushed
the
abstract-switch-case
branch
from
b705494 to
3206a35
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Connections
Fixes #7225
Impacts #7226
Description
When lowering switch statements we weren't handling abstract types at all - we were simply concretizing them to i32. This PR makes us handle them correctly - find the (possibly abstract) type of the switch selector and all case selectors, find a consensus type they can be converted to, then attempt to convert them all to to that type. (concretizing to i32 if they are all abstract).
I've reworked the error handling slightly, differentiating between an invalid switch selector type (must be an int scalar), an invalid case expression (must be an int scalar constexpr), and a type mismatch between cases and the selector.
Note that this removed the note about adding/removing the
usuffix, which we have #7226 filed about. We can keep that ticket open to potentially reinstate something similar. But for now I think it's better with no note at all rather than the misleading one. Especially as allowing abstract case expressions should greatly reduce the occurrence of this error in the first place: we will now accept shaders where the selector is a u32 and the case values are abstract.Testing
Ran tests. Added new snapshot tests for code that should parse, and new wgsl_errors tests for code that should be rejected.
Squash or Rebase?
Surprise me
Checklist
cargo fmt.- [ ] Runtaplo format.cargo clippy. If applicable, add:- [ ]--target wasm32-unknown-unknowncargo xtask testto run tests.CHANGELOG.md. See simple instructions inside file.