[naga] Two structs with the same members are not equivalent

Fixes gfx-rs#5796

Author: andyleiserson

naga/src/arena/unique_arena.rs

@@ -16,7 +16,8 @@ use crate::{FastIndexSet, Span};
 /// The element type must implement `Eq` and `Hash`. Insertions of equivalent
 /// elements, according to `Eq`, all return the same `Handle`.
 ///
-/// Once inserted, elements may not be mutated.
+/// Once inserted, elements generally may not be mutated, although a `replace`
+/// method exists to support rare cases.
 ///
 /// `UniqueArena` is similar to [`Arena`]: If `Arena` is vector-like,
 /// `UniqueArena` is `HashSet`-like.

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

@@ -46,7 +46,10 @@ impl<'source> super::ExpressionContext<'source, '_, '_> {
         }
 
         // If `expr` already has the requested type, we're done.
-        if expr_inner.non_struct_equivalent(goal_inner, types) {
+        if self
+            .module
+            .compare_types(expr_resolution.clone(), goal_ty.clone())
+        {
             return Ok(expr);
         }
 

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

@@ -1276,13 +1276,11 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
                     })?;
 
                 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.non_struct_equivalent(init_inner, &ectx.module.types) {
+                if !ectx.module.compare_types(explicit_ty, init_ty) {
                     return Err(Box::new(Error::InitializationTypeMismatch {
                         name: name.span,
-                        expected: ectx.type_inner_to_string(explicit_inner),
-                        got: ectx.type_inner_to_string(init_inner),
+                        expected: ectx.type_to_string(explicit_ty),
+                        got: ectx.type_to_string(init_ty),
                     }));
                 }
                 ty = explicit_ty;
@@ -1508,10 +1506,7 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
                     if let Some(ty) = explicit_ty {
                         let mut ctx = ctx.as_expression(block, &mut emitter);
                         let init_ty = ctx.register_type(value)?;
-                        if !ctx.module.types[ty]
-                            .inner
-                            .non_struct_equivalent(&ctx.module.types[init_ty].inner, &ctx.module.types)
-                        {
+                        if !ctx.module.compare_types(ty, init_ty) {
                             return Err(Box::new(Error::InitializationTypeMismatch {
                                 name: l.name.span,
                                 expected: ctx.type_to_string(ty),

naga/src/proc/mod.rs

@@ -21,7 +21,7 @@ pub use namer::{EntryPointIndex, NameKey, Namer};
 pub use terminator::ensure_block_returns;
 use thiserror::Error;
 pub use type_methods::min_max_float_representable_by;
-pub use typifier::{ResolveContext, ResolveError, TypeResolution};
+pub use typifier::{compare_types, ResolveContext, ResolveError, TypeResolution};
 
 impl From<super::StorageFormat> for super::Scalar {
     fn from(format: super::StorageFormat) -> Self {
@@ -399,6 +399,14 @@ impl crate::Module {
             global_expressions: &self.global_expressions,
         }
     }
+
+    pub fn compare_types<L: Into<TypeResolution>, R: Into<TypeResolution>>(
+        &self,
+        lhs: L,
+        rhs: R,
+    ) -> bool {
+        compare_types(lhs, rhs, &self.types)
+    }
 }
 
 #[derive(Debug)]
@@ -487,6 +495,14 @@ impl GlobalCtx<'_> {
             _ => get(*self, handle, arena),
         }
     }
+
+    pub fn compare_types<L: Into<TypeResolution>, R: Into<TypeResolution>>(
+        &self,
+        lhs: L,
+        rhs: R,
+    ) -> bool {
+        compare_types(lhs, rhs, self.types)
+    }
 }
 
 #[derive(Error, Debug, Clone, Copy, PartialEq)]

naga/src/proc/type_methods.rs

@@ -4,6 +4,8 @@
 //! [`Scalar`]: crate::Scalar
 //! [`ScalarKind`]: crate::ScalarKind
 
+use crate::ir;
+
 use super::TypeResolution;
 
 impl crate::ScalarKind {
@@ -224,7 +226,7 @@ impl crate::TypeInner {
         }
     }
 
-    /// Compare `self` and `rhs` as types.
+    /// Compare value type `self` and `rhs` as types.
     ///
     /// This is mostly the same as `<TypeInner as Eq>::eq`, but it treats
     /// `ValuePointer` and `Pointer` types as equivalent. This method
@@ -237,11 +239,17 @@ impl crate::TypeInner {
     /// values with `==`.
     pub fn non_struct_equivalent(
         &self,
-        rhs: &crate::TypeInner,
+        rhs: &ir::TypeInner,
         types: &crate::UniqueArena<crate::Type>,
     ) -> bool {
         let left = self.canonical_form(types);
         let right = rhs.canonical_form(types);
+
+        let left_struct = matches!(*self, ir::TypeInner::Struct { .. });
+        let right_struct = matches!(*rhs, ir::TypeInner::Struct { .. });
+
+        assert!(!left_struct || !right_struct);
+
         left.as_ref().unwrap_or(self) == right.as_ref().unwrap_or(rhs)
     }
 

naga/src/proc/typifier.rs

@@ -2,7 +2,10 @@ use alloc::{format, string::String};
 
 use thiserror::Error;
 
-use crate::arena::{Arena, Handle, UniqueArena};
+use crate::{
+    arena::{Arena, Handle, UniqueArena},
+    ir,
+};
 
 /// The result of computing an expression's type.
 ///
@@ -121,6 +124,12 @@ impl TypeResolution {
     }
 }
 
+impl From<Handle<crate::Type>> for TypeResolution {
+    fn from(value: Handle<crate::Type>) -> Self {
+        Self::Handle(value)
+    }
+}
+
 // Clone is only implemented for numeric variants of `TypeInner`.
 impl Clone for TypeResolution {
     fn clone(&self) -> Self {
@@ -916,6 +925,42 @@ impl<'a> ResolveContext<'a> {
     }
 }
 
+/// Compare two types.
+///
+/// This is the most general way of comparing two types, as it can distinguish
+/// two structs with different names but the same members. For other ways, see
+/// `TypeInner::non_struct_equivalent` or even `TypeInner::eq`.
+///
+/// This is usually called via the like-named methods on `Module` and `GlobalCtx`.
+pub fn compare_types<L: Into<TypeResolution>, R: Into<TypeResolution>>(
+    lhs: L,
+    rhs: R,
+    types: &UniqueArena<crate::Type>,
+) -> bool {
+    let lhs_tr = lhs.into();
+    let rhs_tr = rhs.into();
+
+    match lhs_tr {
+        TypeResolution::Handle(lhs_handle)
+            if matches!(
+                types[lhs_handle],
+                ir::Type {
+                    inner: ir::TypeInner::Struct { .. },
+                    ..
+                }
+            ) =>
+        {
+            // Structs can only be in the arena, not in a TypeResolution::Value
+            rhs_tr
+                .handle()
+                .is_some_and(|rhs_handle| lhs_handle == rhs_handle)
+        }
+        _ => lhs_tr
+            .inner_with(types)
+            .non_struct_equivalent(rhs_tr.inner_with(types), types),
+    }
+}
+
 #[test]
 fn test_error_size() {
     assert_eq!(size_of::<ResolveError>(), 32);

naga/src/valid/compose.rs

@@ -84,11 +84,7 @@ pub fn validate_compose(
                 });
             }
             for (index, comp_res) in component_resolutions.enumerate() {
-                let base_inner = &gctx.types[base].inner;
-                let comp_res_inner = comp_res.inner_with(gctx.types);
-                // We don't support arrays of pointers, but it seems best not to
-                // embed that assumption here, so use `TypeInner::equivalent`.
-                if !base_inner.non_struct_equivalent(comp_res_inner, gctx.types) {
+                if !gctx.compare_types(base, comp_res.clone()) {
                     log::error!("Array component[{}] type {:?}", index, comp_res);
                     return Err(ComposeError::ComponentType {
                         index: index as u32,
@@ -105,11 +101,7 @@ pub fn validate_compose(
             }
             for (index, (member, comp_res)) in members.iter().zip(component_resolutions).enumerate()
             {
-                let member_inner = &gctx.types[member.ty].inner;
-                let comp_res_inner = comp_res.inner_with(gctx.types);
-                // We don't support pointers in structs, but it seems best not to embed
-                // that assumption here, so use `TypeInner::equivalent`.
-                if !comp_res_inner.non_struct_equivalent(member_inner, gctx.types) {
+                if !gctx.compare_types(member.ty, comp_res.clone()) {
                     log::error!("Struct component[{}] type {:?}", index, comp_res);
                     return Err(ComposeError::ComponentType {
                         index: index as u32,

naga/src/valid/function.rs

@@ -120,8 +120,11 @@ pub enum FunctionError {
     ContinueOutsideOfLoop,
     #[error("The `return` is called within a `continuing` block")]
     InvalidReturnSpot,
-    #[error("The `return` value {0:?} does not match the function return value")]
-    InvalidReturnType(Option<Handle<crate::Expression>>),
+    #[error("The `return` expression {expression:?} does not match the declared return type {expected_ty:?}")]
+    InvalidReturnType {
+        expression: Option<Handle<crate::Expression>>,
+        expected_ty: Option<Handle<crate::Type>>,
+    },
     #[error("The `if` condition {0:?} is not a boolean scalar")]
     InvalidIfType(Handle<crate::Expression>),
     #[error("The `switch` value {0:?} is not an integer scalar")]
@@ -313,6 +316,14 @@ impl<'a> BlockContext<'a> {
     fn inner_type<'t>(&'t self, ty: &'t TypeResolution) -> &'t crate::TypeInner {
         ty.inner_with(self.types)
     }
+
+    fn compare_types<L: Into<TypeResolution>, R: Into<TypeResolution>>(
+        &self,
+        lhs: L,
+        rhs: R,
+    ) -> bool {
+        crate::proc::compare_types(lhs, rhs, self.types)
+    }
 }
 
 impl super::Validator {
@@ -338,8 +349,7 @@ impl super::Validator {
                     CallError::Argument { index, source }
                         .with_span_handle(expr, context.expressions)
                 })?;
-            let arg_inner = &context.types[arg.ty].inner;
-            if !ty.non_struct_equivalent(arg_inner, context.types) {
+            if !context.compare_types(arg.ty, ty.clone()) {
                 return Err(CallError::ArgumentType {
                     index,
                     required: arg.ty,
@@ -964,13 +974,12 @@ impl super::Validator {
                     let value_ty = value
                         .map(|expr| context.resolve_type(expr, &self.valid_expression_set))
                         .transpose()?;
-                    let expected_ty = context.return_type.map(|ty| &context.types[ty].inner);
                     // We can't return pointers, but it seems best not to embed that
                     // assumption here, so use `TypeInner::equivalent` for comparison.
-                    let okay = match (value_ty, expected_ty) {
+                    let okay = match (value_ty, context.return_type) {
                         (None, None) => true,
-                        (Some(value_inner), Some(expected_inner)) => {
-                            value_inner.non_struct_equivalent(expected_inner, context.types)
+                        (Some(value_inner), Some(expected_ty)) => {
+                            context.compare_types(value_inner.clone(), expected_ty)
                         }
                         (_, _) => false,
                     };
@@ -979,14 +988,20 @@ impl super::Validator {
                         log::error!(
                             "Returning {:?} where {:?} is expected",
                             value_ty,
-                            expected_ty
+                            context.return_type,
                         );
                         if let Some(handle) = value {
-                            return Err(FunctionError::InvalidReturnType(value)
-                                .with_span_handle(handle, context.expressions));
+                            return Err(FunctionError::InvalidReturnType {
+                                expression: value,
+                                expected_ty: context.return_type,
+                            }
+                            .with_span_handle(handle, context.expressions));
                         } else {
-                            return Err(FunctionError::InvalidReturnType(value)
-                                .with_span_static(span, "invalid return"));
+                            return Err(FunctionError::InvalidReturnType {
+                                expression: value,
+                                expected_ty: context.return_type,
+                            }
+                            .with_span_static(span, "invalid return"));
                         }
                     }
                     finished = true;
@@ -1640,9 +1655,7 @@ impl super::Validator {
         }
 
         if let Some(init) = var.init {
-            let decl_ty = &gctx.types[var.ty].inner;
-            let init_ty = fun_info[init].ty.inner_with(gctx.types);
-            if !decl_ty.non_struct_equivalent(init_ty, gctx.types) {
+            if !gctx.compare_types(var.ty, fun_info[init].ty.clone()) {
                 return Err(LocalVariableError::InitializerType);
             }
 

naga/src/valid/interface.rs

@@ -636,9 +636,7 @@ impl super::Validator {
                 return Err(GlobalVariableError::InitializerExprType);
             }
 
-            let decl_ty = &gctx.types[var.ty].inner;
-            let init_ty = mod_info[init].inner_with(gctx.types);
-            if !decl_ty.non_struct_equivalent(init_ty, gctx.types) {
+            if !gctx.compare_types(var.ty, mod_info[init].clone()) {
                 return Err(GlobalVariableError::InitializerType);
             }
         }

naga/src/valid/mod.rs

@@ -532,9 +532,7 @@ impl Validator {
             return Err(ConstantError::InitializerExprType);
         }
 
-        let decl_ty = &gctx.types[con.ty].inner;
-        let init_ty = mod_info[con.init].inner_with(gctx.types);
-        if !decl_ty.non_struct_equivalent(init_ty, gctx.types) {
+        if !gctx.compare_types(con.ty, mod_info[con.init].clone()) {
             return Err(ConstantError::InvalidType);
         }
 
@@ -560,9 +558,8 @@ impl Validator {
             return Err(OverrideError::NonConstructibleType);
         }
 
-        let decl_ty = &gctx.types[o.ty].inner;
-        match decl_ty {
-            &crate::TypeInner::Scalar(
+        match gctx.types[o.ty].inner {
+            crate::TypeInner::Scalar(
                 crate::Scalar::BOOL
                 | crate::Scalar::I32
                 | crate::Scalar::U32
@@ -574,8 +571,7 @@ impl Validator {
         }
 
         if let Some(init) = o.init {
-            let init_ty = mod_info[init].inner_with(gctx.types);
-            if !decl_ty.non_struct_equivalent(init_ty, gctx.types) {
+            if !gctx.compare_types(o.ty, mod_info[init].clone()) {
                 return Err(OverrideError::InvalidType);
             }
         } else if self.overrides_resolved {