From b477c66a9d89ee246bd40e0109fec3a801a89da9 Mon Sep 17 00:00:00 2001 From: Jamie Nicol Date: Mon, 10 Feb 2025 12:11:19 +0000 Subject: [PATCH] [naga wgsl-in] Do not attempt automatic type conversion for non-abstract types Attempting to convert a non-abstract type will always fail, which can result in unrelated errors being misreported as type conversion errors. For example, in #7035 we made it so that abstract types can be used as return values. This changed the final testcase in the invalid_functions() test to fail due to failing to convert a u32 to an atomic, rather than with a NonConstructibleReturnType error. This patch makes it so that we do not attempt to convert non-abstract types in the first place. This avoids the AutoConversion error for that testcase, meaning the NonConstructibleReturnType is once again reported. Various callsites of try_automatic_conversions() have been updated to ensure they still return an InitializationTypeMismatch error if the types are mismatched, even if try_automatic_conversions() succeeds (eg when conversion was not attempted due to the type being concrete). To reduce code duplication these callsites were all adapted to use the existing type_and_init() helper function. Lastly, a couple of tests that expected to result in a AutoConversion error have been adjusted to ensure this still occurs, by ensuring the type used is abstract. --- naga/src/front/wgsl/lower/conversion.rs | 16 ++++ naga/src/front/wgsl/lower/mod.rs | 97 +++++++------------------ naga/src/front/wgsl/tests.rs | 2 +- naga/tests/wgsl_errors.rs | 32 ++++---- 4 files changed, 59 insertions(+), 88 deletions(-) diff --git a/naga/src/front/wgsl/lower/conversion.rs b/naga/src/front/wgsl/lower/conversion.rs index b7a649fd19..f7e767bfb8 100644 --- a/naga/src/front/wgsl/lower/conversion.rs +++ b/naga/src/front/wgsl/lower/conversion.rs @@ -32,6 +32,22 @@ 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. + let mut base_inner = expr_inner; + while let crate::TypeInner::Array { base, .. } = *base_inner { + base_inner = &types[base].inner; + } + if !base_inner + .scalar() + .is_some_and(|scalar| scalar.is_abstract()) + { + return Ok(expr); + } + // If `expr` already has the requested type, we're done. if expr_inner.equivalent(goal_inner, types) { return Ok(expr); diff --git a/naga/src/front/wgsl/lower/mod.rs b/naga/src/front/wgsl/lower/mod.rs index a1bb65533f..8059cc5e55 100644 --- a/naga/src/front/wgsl/lower/mod.rs +++ b/naga/src/front/wgsl/lower/mod.rs @@ -1100,28 +1100,14 @@ impl<'source, 'temp> Lowerer<'source, 'temp> { } ast::GlobalDeclKind::Const(ref c) => { let mut ectx = ctx.as_const(); - let mut init = self.expression_for_abstract(c.init, &mut ectx)?; - let ty; - if let Some(explicit_ty) = c.ty { - let explicit_ty = - self.resolve_ast_type(explicit_ty, &mut ectx.as_global())?; - let explicit_ty_res = crate::proc::TypeResolution::Handle(explicit_ty); - init = ectx - .try_automatic_conversions(init, &explicit_ty_res, c.name.span) - .map_err(|error| match error { - Error::AutoConversion(e) => Error::InitializationTypeMismatch { - name: c.name.span, - expected: e.dest_type, - got: e.source_type, - }, - other => other, - })?; - ty = explicit_ty; - } else { - init = ectx.concretize(init)?; - ty = ectx.register_type(init)?; - } + let explicit_ty = + c.ty.map(|ast| self.resolve_ast_type(ast, &mut ectx.as_global())) + .transpose()?; + + let (ty, init) = + self.type_and_init(c.name, Some(c.init), explicit_ty, &mut ectx)?; + let init = init.expect("Global const must have init"); let handle = ctx.module.constants.append( crate::Constant { @@ -1233,6 +1219,17 @@ impl<'source, 'temp> Lowerer<'source, 'temp> { }, other => other, })?; + + let init_ty = ectx.register_type(init)?; + let explicit_inner = &ectx.module.types[explicit_ty].inner; + let init_inner = &ectx.module.types[init_ty].inner; + if !explicit_inner.equivalent(init_inner, &ectx.module.types) { + return Err(Error::InitializationTypeMismatch { + name: name.span, + expected: explicit_inner.to_wgsl(&ectx.module.to_ctx()).into(), + got: init_inner.to_wgsl(&ectx.module.to_ctx()).into(), + }); + } ty = explicit_ty; initializer = Some(init); } @@ -1479,37 +1476,8 @@ impl<'source, 'temp> Lowerer<'source, 'temp> { let mut emitter = Emitter::default(); emitter.start(&ctx.function.expressions); let mut ectx = ctx.as_expression(block, &mut emitter); - - let ty; - let initializer; - match (v.init, explicit_ty) { - (Some(init), Some(explicit_ty)) => { - let init = self.expression_for_abstract(init, &mut ectx)?; - let ty_res = crate::proc::TypeResolution::Handle(explicit_ty); - let init = ectx - .try_automatic_conversions(init, &ty_res, v.name.span) - .map_err(|error| match error { - Error::AutoConversion(e) => Error::InitializationTypeMismatch { - name: v.name.span, - expected: e.dest_type, - got: e.source_type, - }, - other => other, - })?; - ty = explicit_ty; - initializer = Some(init); - } - (Some(init), None) => { - let concretized = self.expression(init, &mut ectx)?; - ty = ectx.register_type(concretized)?; - initializer = Some(concretized); - } - (None, Some(explicit_ty)) => { - ty = explicit_ty; - initializer = None; - } - (None, None) => return Err(Error::DeclMissingTypeAndInit(v.name.span)), - } + let (ty, initializer) = + self.type_and_init(v.name, v.init, explicit_ty, &mut ectx)?; let (const_initializer, initializer) = { match initializer { @@ -1564,26 +1532,13 @@ impl<'source, 'temp> Lowerer<'source, 'temp> { let ectx = &mut ctx.as_const(block, &mut emitter); - let mut init = self.expression_for_abstract(c.init, ectx)?; + let explicit_ty = + c.ty.map(|ast| self.resolve_ast_type(ast, &mut ectx.as_global())) + .transpose()?; - if let Some(explicit_ty) = c.ty { - let explicit_ty = - self.resolve_ast_type(explicit_ty, &mut ectx.as_global())?; - let explicit_ty_res = crate::proc::TypeResolution::Handle(explicit_ty); - init = ectx - .try_automatic_conversions(init, &explicit_ty_res, c.name.span) - .map_err(|error| match error { - Error::AutoConversion(error) => Error::InitializationTypeMismatch { - name: c.name.span, - expected: error.dest_type, - got: error.source_type, - }, - other => other, - })?; - } else { - init = ectx.concretize(init)?; - ectx.register_type(init)?; - } + let (_ty, init) = + self.type_and_init(c.name, Some(c.init), explicit_ty, ectx)?; + let init = init.expect("Local const must have init"); block.extend(emitter.finish(&ctx.function.expressions)); ctx.local_table diff --git a/naga/src/front/wgsl/tests.rs b/naga/src/front/wgsl/tests.rs index 54d931efe2..7d14105b92 100644 --- a/naga/src/front/wgsl/tests.rs +++ b/naga/src/front/wgsl/tests.rs @@ -77,7 +77,7 @@ fn parse_type_cast() { assert!(parse_str( " fn main() { - let x: vec2 = vec2(0i, 0i); + let x: vec2 = vec2(0.0, 0.0); } ", ) diff --git a/naga/tests/wgsl_errors.rs b/naga/tests/wgsl_errors.rs index a110a37fe0..c54acf37fe 100644 --- a/naga/tests/wgsl_errors.rs +++ b/naga/tests/wgsl_errors.rs @@ -1174,7 +1174,7 @@ fn invalid_functions() { if function_name == "return_pointer" } - check( + check_validation! { " @group(0) @binding(0) var atom: atomic; @@ -1182,15 +1182,14 @@ fn invalid_functions() { fn return_atomic() -> atomic { return atom; } - ", - "error: automatic conversions cannot convert `u32` to `atomic` - ┌─ wgsl:6:19 - │ -6 │ return atom; - │ ^^^^ this expression has type u32 - -", - ); + ": + Err(naga::valid::ValidationError::Function { + name: function_name, + source: naga::valid::FunctionError::NonConstructibleReturnType, + .. + }) + if function_name == "return_atomic" + } } #[test] @@ -2139,15 +2138,16 @@ fn constructor_type_error_span() { check( " fn unfortunate() { - var i: i32; - var a: array = array(i); + var a: array = array(1.0); } ", - r###"error: automatic conversions cannot convert `i32` to `f32` - ┌─ wgsl:4:36 + r###"error: automatic conversions cannot convert `{AbstractFloat}` to `i32` + ┌─ wgsl:3:36 │ -4 │ var a: array = array(i); - │ ^^^^^^^^^^^^^ a value of type f32 is required here +3 │ var a: array = array(1.0); + │ ^^^^^^^^^^^^^ ^^^ this expression has type {AbstractFloat} + │ │ + │ a value of type i32 is required here "###, )