wgsl: Add short-circuiting validation tests #3567
Conversation
Test that the short-circuiting logical expressions are only accepted for scalar boolean types. Also test that they guard invalid expressions on the right-hand-side when the left-hand-side is a const-expression.
| overflow: 'i32(1<<31) < 0', | ||
| binary: '(1.0 / 0) == 0', | ||
| builtin: 'sqrt(-1) == 0', | ||
| array_size: 'array<bool, 3 - 4>()[0]', |
There was a problem hiding this comment.
This is the contentious case that we've been discussing internally, and likely needs clarification in the spec as to whether this is valid or not on the RHS of a short-circuited expression.
FYI @alan-baker too.
There was a problem hiding this comment.
I agree it makes sense for this to be a shader-creation error. I'd be in favour of updating the spec to reflect that.
There was a problem hiding this comment.
I've opened gpuweb/gpuweb#4551 to get clarification on this.
There was a problem hiding this comment.
We should land this test before the spec update is reviewed (if needed)
There was a problem hiding this comment.
I pulled out the array-element-count tests to their own suite(?) and made them expected-to-fail (with a control case), in anticipation of the spec clarification falling in that direction. No need to rush to land this though.
| overflow: 'i32(1<<31) < 0', | ||
| binary: '(1.0 / 0) == 0', | ||
| builtin: 'sqrt(-1) == 0', | ||
| array_size: 'array<bool, 3 - 4>()[0]', |
There was a problem hiding this comment.
We should land this test before the spec update is reviewed (if needed)
| ) | ||
| .fn(t => { | ||
| const lhs = | ||
| t.params.op === '&&' |
There was a problem hiding this comment.
It took a while to read this but yes, this is correct.
An alternative:
const lookup = { '&&': { true: false, false: true}, '||': { true: true, false: false}};
const lhs = lookup[t.params.op][t.params.skip_rhs];There was a problem hiding this comment.
Did this a slightly different way because TypeScript was being picky.
| ) | ||
| .fn(t => { | ||
| const lhs = | ||
| t.params.op === '&&' |
There was a problem hiding this comment.
Did this a slightly different way because TypeScript was being picky.
| overflow: 'i32(1<<31) < 0', | ||
| binary: '(1.0 / 0) == 0', | ||
| builtin: 'sqrt(-1) == 0', | ||
| array_size: 'array<bool, 3 - 4>()[0]', |
There was a problem hiding this comment.
I pulled out the array-element-count tests to their own suite(?) and made them expected-to-fail (with a control case), in anticipation of the spec clarification falling in that direction. No need to rush to land this though.
Test that the short-circuiting logical expressions are only accepted for scalar boolean types.
Also test that they guard invalid expressions on the right-hand-side when the left-hand-side is a const-expression.
Requirements for PR author:
.unimplemented()./** documented */and new helper files are found inhelper_index.txt.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.