[wgsl-in] Report concrete type mismatches in WGSL terms

CHANGELOG.md

@@ -86,6 +86,7 @@ Bottom level categories:
 
 - Fix panics when shader `var<immediate>` size is larger than 256 bytes. By @beicause in [#9725](https://github.com/gfx-rs/wgpu/pull/9725).
 - Fix a panic in the SPIR-V frontend when a subgroup collective operation (e.g. `OpGroupNonUniformUMin`) or `OpGroupNonUniformBallot` used an argument whose value needed to be spilled to a temporary variable, such as when the argument was computed inside a loop. By @nazar-pc in [#9957](https://github.com/gfx-rs/wgpu/issues/9957).
+- Report WGSL type mismatches in `return` statements, function call arguments and composite constructors as WGSL errors naming both types, instead of IR validation errors that could only name the operands by handle index (such as "The \`return\` expression Some([1]) does not match the declared return type Some([1])"). By @emilk in [#9973](https://github.com/gfx-rs/wgpu/pull/9973).
 
 #### Validation
 

naga/src/front/wgsl/error.rs

@@ -404,6 +404,7 @@ pub(crate) enum Error<'a> {
     ExpectedPositiveArrayLength(Span),
     MissingWorkgroupSize(Span),
     ConstantEvaluatorError(Box<ConstantEvaluatorError>, Span),
+    TypeMismatch(Box<TypeMismatchError>),
     AutoConversion(Box<AutoConversionError>),
     AutoConversionLeafScalar(Box<AutoConversionLeafScalarError>),
     ConcretizationFailed(Box<ConcretizationFailedError>),
@@ -499,6 +500,17 @@ impl From<&'static str> for DiagnosticAttributeNotSupportedPosition {
     }
 }
 
+/// A value's concrete type differs from the type required by its context.
+#[derive(Clone, Debug)]
+pub(crate) struct TypeMismatchError {
+    /// Where the required type comes from.
+    pub dest_span: Span,
+    pub dest_type: String,
+    /// The value whose type is wrong.
+    pub source_span: Span,
+    pub source_type: String,
+}
+
 #[derive(Clone, Debug)]
 pub(crate) struct AutoConversionError {
     pub dest_span: Span,
@@ -1164,6 +1176,29 @@ impl<'a> Error<'a> {
                 )],
                 notes: vec![],
             },
+            Error::TypeMismatch(ref error) => {
+                let TypeMismatchError {
+                    dest_span,
+                    ref dest_type,
+                    source_span,
+                    ref source_type,
+                } = **error;
+                let mut labels = vec![(
+                    source_span,
+                    format!("this expression has type `{source_type}`").into(),
+                )];
+                if dest_span != source_span {
+                    labels.push((
+                        dest_span,
+                        format!("a value of type `{dest_type}` is required here").into(),
+                    ));
+                }
+                ParseError {
+                    message: format!("expected `{dest_type}`, found `{source_type}`"),
+                    labels,
+                    notes: vec![],
+                }
+            }
             Error::AutoConversion(ref error) => {
                 // destructuring ensures all fields are handled
                 let AutoConversionError {

naga/src/front/wgsl/lower/conversion.rs

@@ -5,6 +5,7 @@ use alloc::{boxed::Box, string::String, vec::Vec};
 use crate::common::wgsl::{TryToWgsl, TypeContext};
 use crate::front::wgsl::error::{
     AutoConversionError, AutoConversionLeafScalarError, ConcretizationFailedError,
+    TypeMismatchError,
 };
 use crate::front::wgsl::Result;
 use crate::{Handle, Span};
@@ -14,13 +15,15 @@ impl<'source> super::ExpressionContext<'source, '_, '_> {
     ///
     /// If no conversions are necessary, return `expr` unchanged.
     ///
-    /// If automatic conversions cannot convert `expr` to `goal_ty`, return an
-    /// [`AutoConversion`] error.
+    /// If `expr`'s type is concrete and differs from `goal_ty`, return a
+    /// [`TypeMismatch`] error. If it is abstract but automatic conversions
+    /// cannot convert it to `goal_ty`, return an [`AutoConversion`] error.
     ///
     /// Although the Load Rule is one of the automatic conversions, this
     /// function assumes it has already been applied if appropriate, as
     /// indicated by the fact that the Rust type of `expr` is not `Typed<_>`.
     ///
+    /// [`TypeMismatch`]: super::Error::TypeMismatch
     /// [`AutoConversion`]: super::Error::AutoConversion
     pub fn try_automatic_conversions(
         &mut self,
@@ -36,20 +39,31 @@ impl<'source> super::ExpressionContext<'source, '_, '_> {
         let expr_inner = expr_resolution.inner_with(types);
         let goal_inner = goal_ty.inner_with(types);
 
-        // We can only convert abstract types, so if `expr` is not abstract do not even
-        // attempt conversion. This allows the validator to catch type errors correctly
-        // rather than them being misreported as type conversion errors.
-        // If the type is an array (of an array, etc) then we must check whether the
-        // type of the innermost array's base type is abstract.
-        if !expr_inner.is_abstract(types) {
-            return Ok(expr);
-        }
-
         // If `expr` already has the requested type, we're done.
         if self.module.compare_types(expr_resolution, goal_ty) {
             return Ok(expr);
         }
 
+        // We can only convert abstract types, so if `expr` is not abstract then this
+        // is a plain type mismatch, not a failed conversion. Report it as such, rather
+        // than misreporting it as a conversion error, or leaving it to the IR
+        // validator, which can only name the operands by handle index.
+        // If the type is an array (of an array, etc) then we must check whether the
+        // type of the innermost array's base type is abstract.
+        if !expr_inner.is_abstract(types) {
+            let source_type = self.type_resolution_to_string(expr_resolution);
+            let dest_type = self.type_resolution_to_string(goal_ty);
+
+            return Err(Box::new(super::Error::TypeMismatch(Box::new(
+                TypeMismatchError {
+                    dest_span: goal_span,
+                    dest_type,
+                    source_span: expr_span,
+                    source_type,
+                },
+            ))));
+        }
+
         let (_expr_scalar, goal_scalar) =
             match expr_inner.automatically_converts_to(goal_inner, types) {
                 Some(scalars) => scalars,

naga/src/front/wgsl/lower/mod.rs

@@ -1521,25 +1521,22 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
                 let init = ectx
                     .try_automatic_conversions(init, &ty_res, name.span)
                     .map_err(|error| match *error {
+                        // Both of these mean the same thing to the reader of a
+                        // `var`/`let` declaration: the initializer's type isn't
+                        // the declared one.
                         Error::AutoConversion(e) => Box::new(Error::InitializationTypeMismatch {
                             name: name.span,
                             expected: e.dest_type,
                             got: e.source_type,
                         }),
+                        Error::TypeMismatch(e) => Box::new(Error::InitializationTypeMismatch {
+                            name: name.span,
+                            expected: e.dest_type,
+                            got: e.source_type,
+                        }),
                         _ => error,
                     })?;
 
-                let init_ty = ectx.register_type(init)?;
-                if !ectx.module.compare_types(
-                    &proc::TypeResolution::Handle(explicit_ty),
-                    &proc::TypeResolution::Handle(init_ty),
-                ) {
-                    return Err(Box::new(Error::InitializationTypeMismatch {
-                        name: name.span,
-                        expected: ectx.type_to_string(explicit_ty),
-                        got: ectx.type_to_string(init_ty),
-                    }));
-                }
                 ty = explicit_ty;
                 initializer = Some(init);
             }
@@ -2104,6 +2101,7 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
 
                 let value;
                 if let Some(ast_expr) = ast_value {
+                    let value_span = ctx.ast_expressions.get_span(ast_expr);
                     let result_ty = ctx.function.result.as_ref().map(|r| r.ty);
                     let mut ectx = ctx.as_expression(block, &mut emitter);
                     let expr = self.expression_for_abstract(ast_expr, &mut ectx)?;
@@ -2112,7 +2110,7 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
                         let mut ectx = ctx.as_expression(block, &mut emitter);
                         let resolution = proc::TypeResolution::Handle(result_ty);
                         let converted =
-                            ectx.try_automatic_conversions(expr, &resolution, Span::default())?;
+                            ectx.try_automatic_conversions(expr, &resolution, value_span)?;
                         value = Some(converted);
                     } else {
                         value = Some(expr);

naga/tests/naga/wgsl_errors.rs

@@ -1773,43 +1773,84 @@ fn struct_type_mismatch_in_let_decl() {
 
 #[test]
 fn struct_type_mismatch_in_return_value() {
-    check_validation!(
+    check(
         "
         struct Foo { a: u32 };
         struct Bar { a: u32 };
         fn bar() -> Bar {
             return Foo(1);
         }
-        ":
-        Err(naga::valid::ValidationError::Function {
-            handle: _,
-            name: function_name,
-            source: naga::valid::FunctionError::InvalidReturnType { .. }
-        }) if function_name == "bar"
+        ",
+        r#"error: expected `Bar`, found `Foo`
+  ┌─ wgsl:5:20
+  │
+5 │             return Foo(1);
+  │                    ^^^^^^ this expression has type `Foo`
+
+"#,
     );
 }
 
 #[test]
 fn struct_type_mismatch_in_argument() {
-    check_validation!(
+    check(
         "
         struct Foo { a: u32 };
         struct Bar { a: u32 };
         fn bar(a: Bar) {}
         fn main() {
             bar(Foo(1));
         }
-        ":
-        Err(naga::valid::ValidationError::Function {
-            name: function_name,
-            source: naga::valid::FunctionError::InvalidCall {
-                function: _,
-                error: naga::valid::CallError::ArgumentType { index, .. },
-            },
-            ..
-        })
-        // The validation error is reported at the call, i.e., in `main`
-        if function_name == "main" && *index == 0
+        ",
+        r#"error: expected `Bar`, found `Foo`
+  ┌─ wgsl:6:17
+  │
+6 │             bar(Foo(1));
+  │                 ^^^^^^ this expression has type `Foo`
+
+"#,
+    );
+}
+
+/// Regression test for <https://github.com/gfx-rs/wgpu/issues/7419>: a
+/// constructor component of the wrong concrete type used to be reported by the
+/// IR validator as `Composing 0's component type is not expected`.
+#[test]
+fn type_mismatch_in_composite_constructor() {
+    check(
+        "
+        fn main() {
+            var a = array<vec2<u32>, 2>(1u, 2u);
+        }
+        ",
+        r#"error: expected `vec2<u32>`, found `u32`
+  ┌─ wgsl:3:21
+  │
+3 │             var a = array<vec2<u32>, 2>(1u, 2u);
+  │                     ^^^^^^^^^^^^^^^^^^^ ^^ this expression has type `u32`
+  │                     │                    
+  │                     a value of type `vec2<u32>` is required here
+
+"#,
+    );
+
+    check(
+        "
+        struct S { inner: array<u32, 4> }
+        fn main() {
+            var s = S(1u);
+        }
+        ",
+        r#"error: expected `array<u32, 4>`, found `u32`
+  ┌─ wgsl:2:9
+  │
+2 │         struct S { inner: array<u32, 4> }
+  │         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ a value of type `array<u32, 4>` is required here
+3 │         fn main() {
+4 │             var s = S(1u);
+  │                       ^^ this expression has type `u32`
+
+"#,
     );
 }
 
@@ -1970,13 +2011,16 @@ fn invalid_functions() {
 
 #[test]
 fn invalid_return_type() {
-    check_validation! {
-        "fn invalid_return_type() -> i32 { return 0u; }":
-        Err(naga::valid::ValidationError::Function {
-            source: naga::valid::FunctionError::InvalidReturnType { .. },
-            ..
-        })
-    };
+    check(
+        "fn invalid_return_type() -> i32 { return 0u; }",
+        r#"error: expected `i32`, found `u32`
+  ┌─ wgsl:1:42
+  │
+1 │ fn invalid_return_type() -> i32 { return 0u; }
+  │                                          ^^ this expression has type `u32`
+
+"#,
+    );
 }
 
 #[test]
@@ -4045,9 +4089,10 @@ fn vector_logical_ops() {
 #[test]
 fn issue7165() {
     // Regression test for https://github.com/gfx-rs/wgpu/issues/7165
+    // Any shader that parses but fails validation with a span will do.
     let shader = "
-        struct Struct { a: u32 }
-        fn invalid_return_type(a: Struct) -> i32 { return a; }
+        struct Atom { a: atomic<u32> }
+        fn non_constructible_return_type(a: Atom) -> Atom { return a; }
     ";
 
     // We need the span for the error, so have to invoke manually.