Skip to content

Commit

Permalink
[naga wgsl-in] Do not attempt automatic type conversion for non-abstr…
Browse files Browse the repository at this point in the history 
…act 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<u32>, 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.
  • Loading branch information
@jamienicol
jamienicol committed Feb 12, 2025
1 parent a546e60 commit b477c66
Show file tree
Hide file tree
Showing 4 changed files with 61 additions and 90 deletions.
16 changes: 16 additions & 0 deletions naga/src/front/wgsl/lower/conversion.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
101 changes: 28 additions & 73 deletions naga/src/front/wgsl/lower/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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)?;

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 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, ectx)?;
let init = init.expect("Local const must have init");

block.extend(emitter.finish(&ctx.function.expressions));
ctx.local_table
Expand Down
2 changes: 1 addition & 1 deletion naga/src/front/wgsl/tests.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ fn parse_type_cast() {
assert!(parse_str(
"
fn main() {
let x: vec2<f32> = vec2<f32>(0i, 0i);
let x: vec2<i32> = vec2<i32>(0.0, 0.0);
}
",
)
Expand Down
32 changes: 16 additions & 16 deletions naga/tests/wgsl_errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1174,23 +1174,22 @@ fn invalid_functions() {
if function_name == "return_pointer"
}

check(
check_validation! {
"
@group(0) @binding(0)
var<storage> atom: atomic<u32>;
fn return_atomic() -> atomic<u32> {
return atom;
}
",
"error: automatic conversions cannot convert `u32` to `atomic<u32>`
┌─ 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]
Expand Down Expand Up @@ -2139,15 +2138,16 @@ fn constructor_type_error_span() {
check(
"
fn unfortunate() {
var i: i32;
var a: array<f32, 1> = array<f32, 1>(i);
var a: array<i32, 1> = array<i32, 1>(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<f32, 1> = array<f32, 1>(i);
│ ^^^^^^^^^^^^^ a value of type f32 is required here
3 │ var a: array<i32, 1> = array<i32, 1>(1.0);
│ ^^^^^^^^^^^^^ ^^^ this expression has type {AbstractFloat}
│ │
│ a value of type i32 is required here
"###,
)
Expand Down

0 comments on commit b477c66

Please sign in to comment.