fix(naga): Improve validation of non-constructible types

Fixes #4720
Fixes the test cases in #7393, but not the issue itself

Author: andyleiserson

CHANGELOG.md

@@ -71,6 +71,7 @@ Bottom level categories:
 - Reject splat vector construction if the argument type does not match the type of the vector's scalar. Previously it would succeed. By @mooori in [#8829](https://github.com/gfx-rs/wgpu/pull/8829).
 - Fixed `workgroupUniformLoad` incorrectly returning an atomic when called on an atomic, it now returns the inner `T` as per the spec. By @cryvosh in [#8791](https://github.com/gfx-rs/wgpu/pull/8791).
 - Fixed constant evaluation for `sign()` builtin to return zero when the argument is zero. By @mandryskowski in [#8942](https://github.com/gfx-rs/wgpu/pull/8942).
+- Reject non-constructible types (runtime- and override-sized arrays, and structs containing non-constructible types) in more places where they should not be allowed. By @andyleiserson in [#8873](https://github.com/gfx-rs/wgpu/pull/8873).
 
 #### GLES
 
@@ -107,7 +108,7 @@ This has been a long time coming. See [the tracking issue](https://github.com/gf
 They are now fully supported on Vulkan, and supported on Metal and DX12 with passthrough shaders. WGSL parsing and rewriting
 is supported, meaning they can be used through WESL or naga_oil.
 
-Mesh shader pipelines replace the standard vertex shader pipelines and allow new ways to render meshes. 
+Mesh shader pipelines replace the standard vertex shader pipelines and allow new ways to render meshes.
 They are ideal for meshlet rendering, a form of rendering where small groups of triangles are handled together,
 for both culling and rendering.
 
@@ -154,7 +155,7 @@ fn ms_main(@builtin(local_invocation_index) index: u32, @builtin(global_invocati
 
     mesh_output.vertices[2].position = positions[2];
     mesh_output.vertices[2].color = colors[2] * taskPayload.colorMask;
-    
+
     // Set the vertex indices for the only primitive
     mesh_output.primitives[0].indices = vec3<u32>(0, 1, 2);
     // Cull it if the data passed by the task shader says to

cts_runner/test.lst

@@ -224,12 +224,17 @@ webgpu:shader,validation,expression,call,builtin,all:arguments:test="ptr_deref"
 webgpu:shader,validation,expression,call,builtin,distance:values:stage="constant";type="f32"
 webgpu:shader,validation,expression,call,builtin,length:scalar:stage="constant";type="f32"
 webgpu:shader,validation,expression,call,builtin,max:values:*
-// FAIL: others in `value_constructor` due to https://github.com/gfx-rs/wgpu/issues/4720, possibly more
 webgpu:shader,validation,expression,call,builtin,value_constructor:array_value:*
+webgpu:shader,validation,expression,call,builtin,value_constructor:array_zero_value:*
 webgpu:shader,validation,expression,call,builtin,value_constructor:matrix_zero_value:*
+webgpu:shader,validation,expression,call,builtin,value_constructor:partial_eval:*
 webgpu:shader,validation,expression,call,builtin,value_constructor:scalar_value:*
 webgpu:shader,validation,expression,call,builtin,value_constructor:scalar_zero_value:*
 webgpu:shader,validation,expression,call,builtin,value_constructor:struct_value:*
+webgpu:shader,validation,expression,call,builtin,value_constructor:struct_zero_value:*
+webgpu:shader,validation,expression,call,builtin,value_constructor:vector_copy:*
+webgpu:shader,validation,expression,call,builtin,value_constructor:vector_elementwise:*
+webgpu:shader,validation,expression,call,builtin,value_constructor:vector_mixed:*
 webgpu:shader,validation,expression,call,builtin,value_constructor:vector_splat:*
 webgpu:shader,validation,expression,call,builtin,value_constructor:vector_zero_value:*
 // NOTE: This is supposed to be an exhaustive listing underneath

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

@@ -164,33 +164,38 @@ impl<'source> Lowerer<'source, '_> {
 
         let expr;
         match (components, constructor) {
-            // Empty constructor
-            (Components::None, dst_ty) => match dst_ty {
-                Constructor::Type((result_ty, _)) => {
-                    expr = crate::Expression::ZeroValue(result_ty);
-                }
-                Constructor::PartialVector { size } => {
-                    // vec2(), vec3(), vec4() return vectors of abstractInts; the same
-                    // is not true of the similar constructors for matrices or arrays.
-                    // See https://www.w3.org/TR/WGSL/#vec2-builtin et seq.
-                    let result_ty = ctx.module.types.insert(
-                        crate::Type {
-                            name: None,
-                            inner: crate::TypeInner::Vector {
-                                size,
-                                scalar: crate::Scalar::ABSTRACT_INT,
-                            },
+            // Zero-value constructor with explicit type. Not everything this
+            // matches is constructible, but the non-constructible types not
+            // excluded by `!is_dynamically_sized` are rejected by the
+            // parser.
+            (Components::None, Constructor::Type((result_ty, inner)))
+                if !inner.is_dynamically_sized(&ctx.module.types) =>
+            {
+                expr = crate::Expression::ZeroValue(result_ty);
+            }
+            // Zero-value constructor, vector with type inference
+            (Components::None, Constructor::PartialVector { size }) => {
+                // vec2(), vec3(), vec4() return vectors of abstractInts; the same
+                // is not true of the similar constructors for matrices or arrays.
+                // See https://www.w3.org/TR/WGSL/#vec2-builtin et seq.
+                let result_ty = ctx.module.types.insert(
+                    crate::Type {
+                        name: None,
+                        inner: crate::TypeInner::Vector {
+                            size,
+                            scalar: crate::Scalar::ABSTRACT_INT,
                         },
-                        span,
-                    );
-                    expr = crate::Expression::ZeroValue(result_ty);
-                }
-                Constructor::PartialMatrix { .. } | Constructor::PartialArray => {
-                    // We have no arguments from which to infer the result type, so
-                    // partial constructors aren't acceptable here.
-                    return Err(Box::new(Error::TypeNotInferable(ty_span)));
-                }
-            },
+                    },
+                    span,
+                );
+                expr = crate::Expression::ZeroValue(result_ty);
+            }
+            // Zero-value constructor, matrix or array with type inference
+            (Components::None, Constructor::PartialMatrix { .. } | Constructor::PartialArray) => {
+                // We have no arguments from which to infer the result type, so
+                // partial constructors aren't acceptable here.
+                return Err(Box::new(Error::TypeNotInferable(ty_span)));
+            }
 
             // Scalar constructor & conversion (scalar -> scalar)
             (
@@ -539,8 +544,11 @@ impl<'source> Lowerer<'source, '_> {
                 expr = crate::Expression::Compose { ty, components };
             }
 
-            // Array constructor, explicit type
-            (components, Constructor::Type((ty, &crate::TypeInner::Array { base, .. }))) => {
+            // Array constructor, explicit type. Override- and runtime-sized arrays are not constructible.
+            (
+                components,
+                Constructor::Type((ty, inner @ &crate::TypeInner::Array { base, .. })),
+            ) if !inner.is_dynamically_sized(&ctx.module.types) => {
                 let mut components = components.into_components_vec();
                 ctx.try_automatic_conversions_slice(&mut components, &Tr::Handle(base), ty_span)?;
                 expr = crate::Expression::Compose { ty, components };
@@ -549,8 +557,8 @@ impl<'source> Lowerer<'source, '_> {
             // Struct constructor
             (
                 components,
-                Constructor::Type((ty, &crate::TypeInner::Struct { ref members, .. })),
-            ) => {
+                Constructor::Type((ty, inner @ &crate::TypeInner::Struct { ref members, .. })),
+            ) if !inner.is_dynamically_sized(&ctx.module.types) => {
                 let mut components = components.into_components_vec();
                 let struct_ty_span = ctx.module.types.get_span(ty);
 

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

@@ -1717,14 +1717,28 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
 
                     let mut ectx = ctx.as_expression(block, &mut emitter);
 
-                    let (_ty, initializer) = self.type_and_init(
+                    let (ty, initializer) = self.type_and_init(
                         l.name,
                         Some(l.init),
                         explicit_ty,
                         AbstractRule::Concretize,
                         &mut ectx,
                     )?;
 
+                    // We have this special check here for `let` declarations because the
+                    // validator doesn't check them (they are comingled with other things in
+                    // `named_expressions`; see <https://github.com/gfx-rs/wgpu/issues/7393>).
+                    // The check could go in `type_and_init`, but then we'd have to
+                    // distinguish whether override-sized is allowed. The error ought to use
+                    // the type's span, but `module.types.get_span(ty)` is `Span::UNDEFINED`
+                    // (see <https://github.com/gfx-rs/wgpu/issues/7951>).
+                    if ctx.module.types[ty]
+                        .inner
+                        .is_dynamically_sized(&ctx.module.types)
+                    {
+                        return Err(Box::new(Error::TypeNotConstructible(l.name.span)));
+                    }
+
                     // We passed `Some()` to `type_and_init`, so we
                     // will get a lowered initializer expression back.
                     let initializer =

naga/src/proc/type_methods.rs

@@ -344,10 +344,18 @@ impl crate::TypeInner {
         left.as_ref().unwrap_or(self) == right.as_ref().unwrap_or(rhs)
     }
 
+    /// Returns true if `self` is runtime- or override-sized.
     pub fn is_dynamically_sized(&self, types: &crate::UniqueArena<crate::Type>) -> bool {
         use crate::TypeInner as Ti;
         match *self {
-            Ti::Array { size, .. } => size == crate::ArraySize::Dynamic,
+            Ti::Array {
+                size: crate::ArraySize::Constant(_),
+                ..
+            } => false,
+            Ti::Array {
+                size: crate::ArraySize::Pending(_) | crate::ArraySize::Dynamic,
+                ..
+            } => true,
             Ti::Struct { ref members, .. } => members
                 .last()
                 .map(|last| types[last.ty].inner.is_dynamically_sized(types))

naga/src/valid/expression.rs

@@ -1,14 +1,11 @@
 use super::{compose::validate_compose, FunctionInfo, ModuleInfo, ShaderStages, TypeFlags};
 use crate::arena::UniqueArena;
-use crate::valid::expression::builtin::validate_zero_value;
 use crate::{
     arena::Handle,
     proc::OverloadSet as _,
     proc::{IndexableLengthError, ResolveError},
 };
 
-pub mod builtin;
-
 #[derive(Clone, Debug, thiserror::Error)]
 #[cfg_attr(test, derive(PartialEq))]
 pub enum ExpressionError {
@@ -38,8 +35,8 @@ pub enum ExpressionError {
     InvalidSwizzleComponent(crate::SwizzleComponent, crate::VectorSize),
     #[error(transparent)]
     Compose(#[from] super::ComposeError),
-    #[error(transparent)]
-    ZeroValue(#[from] super::ZeroValueError),
+    #[error("Cannot construct zero value of {0:?} because it is not a constructible type")]
+    InvalidZeroValue(Handle<crate::Type>),
     #[error(transparent)]
     IndexableLength(#[from] IndexableLengthError),
     #[error("Operation {0:?} can't work with {1:?}")]
@@ -387,7 +384,9 @@ impl super::Validator {
             }
             E::Constant(_) | E::Override(_) => ShaderStages::all(),
             E::ZeroValue(ty) => {
-                validate_zero_value(ty, module.to_ctx())?;
+                if !mod_info[ty].contains(TypeFlags::CONSTRUCTIBLE) {
+                    return Err(ExpressionError::InvalidZeroValue(ty));
+                }
                 ShaderStages::all()
             }
             E::Compose { ref components, ty } => {

naga/src/valid/expression/builtin.rs

@@ -27,7 +27,6 @@ use crate::{
 use crate::span::{AddSpan as _, WithSpan};
 pub use analyzer::{ExpressionInfo, FunctionInfo, GlobalUse, Uniformity, UniformityRequirements};
 pub use compose::ComposeError;
-pub use expression::builtin::ZeroValueError;
 pub use expression::{check_literal_value, LiteralError};
 pub use expression::{ConstExpressionError, ExpressionError};
 pub use function::{CallError, FunctionError, LocalVariableError, SubgroupError};