Open jamienicol opened 1 day ago
There may be a discussion to be had regarding how to handle conversion errors, which can either be addressed here or in a follow up. Note the change made to the parse_pointers()
test. The old code was invalid WGSL which would be caught by the validator with this error message:
error: Function [1] 'bar' is invalid
┌─ in.wgsl:2:1
│
2 │ ╭ fn bar() {
3 │ │ var x: f32 = 1.0;
4 │ │ let px = &x;
5 │ │ let py = foo(px);
│ │ ^^^^^^^ invalid function call
│ ╰─────────────────────^ naga::Function [1]
│
= Call to [0] is invalid
= Argument 0 value [1] doesn't match the type [1]
However, the test only ensures that parsing succeeds, not validation. With this patch we now report an error during parsing (hence why we had to fix the shader code for the test to still pass):
Could not parse WGSL:
error: automatic conversions cannot convert `ptr<f32>` to `ptr<f32>`
┌─ in.wgsl:5:18
│
5 │ let py = foo(px);
│ ^^ a value of type ptr<f32> is required here
Arguably the new error is more helpful as tells you the required type (though not printing the pointer's address space is unhelpful). Though of course the validator's error message could be improved. Additionally the "automatic conversions cannot convert..." line is a bit misleading, as we shouldn't really be attempting to automatically convert a ptr.
FWIW I did try making us not attempt automatic conversions for non-abstract types, but that caused this test assertion to fail. Again because it expects an error during parsing, but by not attempting to convert non-abstract types that shader parses successfully (but still fails validation)
For comparison, tint's error messages seem a bit more intuitive.
With the pointer example from above:
fn foo(a: ptr<private, f32>) -> f32 { return *a; }
fn bar() {
var x: f32 = 1.0;
let px = &x;
let py = foo(px);
}
Tint outputs:
error: type mismatch for argument 1 in call to foo, expected ptr<private, f32, read_write>, got ptr<function, f32, read_write>
let py = foo(px);
^^
But in a case where an abstract type cannot convert correctly:
fn foo(a: u32) {}
fn bar() {
foo(0.0);
}
error: cannot convert value of type abstract-float to type u32
foo(0.0);
^^^
Note the distinction between a completely mismatched type, and an abstract type that cannot be converted correctly
Connections Fixes #5523
Description When lowering arguments for a user-defined function call, avoid concretizing the argument types. Instead make use of the existing
try_automatic_conversions()
machinery to attempt to convert each argument to the type expected by the function. This is straightforward as user-defined functions only have a single overload.This additionally changes an argument type in the test parse_pointers() from
ptr<private>
toptr<function>
. The former is invalid code which is indeed caught by the validator, but the test only asserts that parsing succeeds, not validation. With this patch, this error is now caught during parsing which caused the test to fail.Testing Test suite passes. Added new tests.
Checklist
cargo fmt
.taplo format
.cargo clippy
. If applicable, add:--target wasm32-unknown-unknown
--target wasm32-unknown-emscripten
cargo xtask test
to run tests.CHANGELOG.md
. See simple instructions inside file.