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

CHANGELOG.md

@@ -28,8 +28,8 @@ Top level categories:
 
 Bottom level categories:
 
-- naga
 - General
+- naga
 - DX12
 - Vulkan
 - Metal
@@ -58,17 +58,16 @@ Bottom level categories:
 
 - The validator checks that override-sized arrays have a positive size, if overrides have been resolved. By @andyleiserson in [#8822](https://github.com/gfx-rs/wgpu/pull/8822).
 - Fix some cases where f16 constants were not working. By @andyleiserson in [#8816](https://github.com/gfx-rs/wgpu/pull/8816).
+- Reject zero-value construction of a runtime-sized array with a validation error. Previously it would crash in the HLSL backend. By @mooori in [#8741](https://github.com/gfx-rs/wgpu/pull/8741).
+- 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).
+- 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
 
 - `DisplayHandle` should now be passed to `InstanceDescriptor` for correct EGL initialization on Wayland. By @MarijnS95 in [#8012](https://github.com/gfx-rs/wgpu/pull/8012)
   Note that the existing workaround to create surfaces before the adapter is no longer valid.
 
-#### naga
-
-- Reject zero-value construction of a runtime-sized array with a validation error. Previously it would crash in the HLSL backend. By @mooori in [#8741](https://github.com/gfx-rs/wgpu/pull/8741).
-- 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).
 
 ### Documentation
 

cts_runner/test.lst

@@ -246,12 +246,17 @@ webgpu:shader,validation,expression,binary,short_circuiting_and_or:invalid_types
 webgpu:shader,validation,expression,binary,short_circuiting_and_or:scalar_vector:op="%26%26";lhs="bool";rhs="bool"
 webgpu:shader,validation,expression,call,builtin,all:arguments:test="ptr_deref"
 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:scalar_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/mod.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};