Address review

* Add note that nesting structs for the inter-stage interface can't
  happen.
* Remove new TODO notes (some addressed and some transferred to an issue
  gfx-rs#5577)
* Changed issue that regression test refers to 3748 -> 5553
* Add debug_assert that binding.is_some() in hlsl writer

naga/src/back/hlsl/writer.rs

@@ -25,8 +25,8 @@ pub(crate) const INSERT_BITS_FUNCTION: &str = "naga_insertBits";
 struct EpStructMember {
     name: String,
     ty: Handle<crate::Type>,
-    // TODO: log error if binding is none?
     // technically, this should always be `Some`
+    // (we `debug_assert!` this in `write_interface_struct`)
     binding: Option<crate::Binding>,
     index: u32,
 }
@@ -481,6 +481,10 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
         write!(self.out, "struct {struct_name}")?;
         writeln!(self.out, " {{")?;
         for m in members.iter() {
+            // Sanity check that each IO member is a built-in or is assigned a
+            // location. Also see note about nesting in `write_ep_input_struct`.
+            debug_assert!(m.binding.is_some());
+
             if is_subgroup_builtin_binding(&m.binding) {
                 continue;
             }
@@ -540,10 +544,12 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
 
         let mut fake_members = Vec::new();
         for arg in func.arguments.iter() {
+            // NOTE: We don't need to handle nesting structs. All members must
+            // be either built-ins or assigned a location. I.E. `binding` is
+            // `Some`. This is checked in `VaryingContext::validate`. See:
+            // https://gpuweb.github.io/gpuweb/wgsl/#input-output-locations
             match module.types[arg.ty].inner {
                 TypeInner::Struct { ref members, .. } => {
-                    // TODO: what about nested structs? Is that possible? Maybe try an unwrap on
-                    // the binding?
                     for member in members.iter() {
                         let name = self.namer.call_or(&member.name, "member");
                         let index = fake_members.len() as u32;
@@ -600,19 +606,15 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
         let fs_input_locs = if let (Some(frag_ep), ShaderStage::Vertex) = (frag_ep, stage) {
             let mut fs_input_locs = Vec::new();
             for arg in frag_ep.func.arguments.iter() {
-                let mut push_if_location = |binding: &Option<crate::Binding>| {
-                    match *binding {
-                        Some(crate::Binding::Location { location, .. }) => {
-                            fs_input_locs.push(location)
-                        }
-                        Some(crate::Binding::BuiltIn(_)) => {}
-                        // Log error?
-                        None => {}
-                    }
+                let mut push_if_location = |binding: &Option<crate::Binding>| match *binding {
+                    Some(crate::Binding::Location { location, .. }) => fs_input_locs.push(location),
+                    Some(crate::Binding::BuiltIn(_)) | None => {}
                 };
+
+                // NOTE: We don't need to handle struct nesting. See note in
+                // `write_ep_input_struct`.
                 match frag_ep.module.types[arg.ty].inner {
                     TypeInner::Struct { ref members, .. } => {
-                        // TODO: nesting?
                         for member in members.iter() {
                             push_if_location(&member.binding);
                         }
@@ -635,9 +637,7 @@ impl<'a, W: fmt::Write> super::Writer<'a, W> {
                             continue;
                         }
                     }
-                    Some(crate::Binding::BuiltIn(_)) => {}
-                    // Log error?
-                    None => {}
+                    Some(crate::Binding::BuiltIn(_)) | None => {}
                 }
             }
 

tests/tests/regression/issue_3748.rs → tests/tests/regression/issue_5553.rs

@@ -8,13 +8,13 @@ use wgpu::*;
 /// This requirement was removed from the WebGPU spec. Now, when generating hlsl, wgpu will
 /// automatically remove any user-defined outputs from the vertex shader that are not present in
 /// the fragment inputs. This is necessary for generating correct hlsl:
-/// https://github.com/gfx-rs/naga/issues/1945
+/// https://github.com/gfx-rs/wgpu/issues/5553
 #[gpu_test]
 static ALLOW_INPUT_NOT_CONSUMED: GpuTestConfiguration =
     GpuTestConfiguration::new().run_async(|ctx| async move {
         let module = ctx
             .device
-            .create_shader_module(include_wgsl!("issue_3748.wgsl"));
+            .create_shader_module(include_wgsl!("issue_5553.wgsl"));
 
         let pipeline_layout = ctx
             .device

tests/tests/root.rs

@@ -1,9 +1,9 @@
 mod regression {
     mod issue_3349;
     mod issue_3457;
-    mod issue_3748;
     mod issue_4024;
     mod issue_4122;
+    mod issue_5553;
 }
 
 mod bgra8unorm_storage;

wgpu-core/src/validation.rs

@@ -1189,11 +1189,6 @@ impl Interface {
                                             ));
                                         }
                                         (
-                                            // TODO: is a subtype allowed here? This isn't clear
-                                            // from the line in the spec: "For each user-defined
-                                            // input of descriptor.fragment there must be a
-                                            // user-defined output of descriptor.vertex that
-                                            // location, type, and interpolation of the input."
                                             iv.ty.is_subtype_of(&provided.ty),
                                             iv.ty.dim.num_components(),
                                         )
@@ -1219,23 +1214,14 @@ impl Interface {
                         }
                     }
                 }
-                // TODO: front_facing, sample_index, and sample_mask builtin's should all increase
-                // components count for fragment input.
                 Varying::BuiltIn(_) => {}
             }
         }
 
         if shader_stage == naga::ShaderStage::Vertex {
-            // TODO: if topology is point we should add 1 to inter_stage_components?
             for output in entry_point.outputs.iter() {
                 //TODO: count builtins towards the limit?
                 inter_stage_components += match *output {
-                    // TODO: Spec mentions "Each user-defined output of descriptor.vertex consumes
-                    // 4 scalar components". Not that it varies based on the type. So is there an
-                    // inconsistency here? Also are all these "user-defined" or is that unknown at
-                    // this stage?
-                    // https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-inter-stage-interfaces
-                    // (same applies to counting components for fragment inputs)
                     Varying::Local { ref iv, .. } => iv.ty.dim.num_components(),
                     Varying::BuiltIn(_) => 0,
                 };
@@ -1257,9 +1243,6 @@ impl Interface {
             }
         }
 
-        // TODO: spec also has a max_inter_stage_shader_variables
-        // https://gpuweb.github.io/gpuweb/#abstract-opdef-validating-inter-stage-interfaces and
-        // the location of user defined outputs(vertex)/inputs(fragment) must all be less than this
         if inter_stage_components > self.limits.max_inter_stage_shader_components {
             return Err(StageError::TooManyVaryings {
                 used: inter_stage_components,

wgpu-hal/src/dx12/device.rs

@@ -223,9 +223,10 @@ impl super::Device {
 
         let frag_ep = fragment_stage
             .map(|fs_stage| {
-                hlsl::FragmentEntryPoint::new(&fs_stage.module.naga.module, fs_stage.entry_point).ok_or(
-                    crate::PipelineError::EntryPoint(naga::ShaderStage::Fragment),
-                )
+                hlsl::FragmentEntryPoint::new(&fs_stage.module.naga.module, fs_stage.entry_point)
+                    .ok_or(crate::PipelineError::EntryPoint(
+                        naga::ShaderStage::Fragment,
+                    ))
             })
             .transpose()?;
 
@@ -1500,7 +1501,8 @@ impl crate::Device for super::Device {
         &self,
         desc: &crate::ComputePipelineDescriptor<super::Api>,
     ) -> Result<super::ComputePipeline, crate::PipelineError> {
-        let blob_cs = self.load_shader(&desc.stage, desc.layout, naga::ShaderStage::Compute, None)?;
+        let blob_cs =
+            self.load_shader(&desc.stage, desc.layout, naga::ShaderStage::Compute, None)?;
 
         let pair = {
             profiling::scope!("ID3D12Device::CreateComputePipelineState");