attr: disallow multiple attributes that are identical or in the same category.

crates/rustc_codegen_spirv/src/attr.rs

@@ -14,7 +14,7 @@ use rustc_hir::{HirId, MethodKind, Target, CRATE_HIR_ID};
 use rustc_middle::hir::map::Map;
 use rustc_middle::ty::query::Providers;
 use rustc_middle::ty::TyCtxt;
-use rustc_span::symbol::Symbol;
+use rustc_span::{Span, Symbol};
 use std::rc::Rc;
 
 // FIXME(eddyb) replace with `ArrayVec<[Word; 3]>`.
@@ -95,6 +95,86 @@ pub enum SpirvAttribute {
     UnrollLoops,
 }
 
+// HACK(eddyb) this is similar to `rustc_span::Spanned` but with `value` as the
+// field name instead of `node` (which feels inadequate in this context).
+#[derive(Copy, Clone)]
+pub struct Spanned<T> {
+    pub value: T,
+    pub span: Span,
+}
+
+/// Condensed version of a `SpirvAttribute` list, but only keeping one value per
+/// variant of `SpirvAttribute`, and treating multiple such attributes an error.
+// FIXME(eddyb) should this and `fn try_insert_attr` below be generated by a macro?
+#[derive(Default)]
+pub struct AggregatedSpirvAttributes {
+    // `struct` attributes:
+    pub storage_class: Option<Spanned<StorageClass>>,
+    pub intrinsic_type: Option<Spanned<IntrinsicType>>,
+    pub block: Option<Spanned<()>>,
+
+    // `fn` attributes:
+    pub entry: Option<Spanned<Entry>>,
+
+    // (entry) `fn` parameter attributes:
+    pub builtin: Option<Spanned<BuiltIn>>,
+    pub descriptor_set: Option<Spanned<u32>>,
+    pub binding: Option<Spanned<u32>>,
+    pub flat: Option<Spanned<()>>,
+
+    // `fn`/closure attributes:
+    pub unroll_loops: Option<Spanned<()>>,
+}
+
+struct MultipleAttrs {
+    prev_span: Span,
+    category: &'static str,
+}
+
+impl AggregatedSpirvAttributes {
+    fn try_insert_attr(&mut self, attr: SpirvAttribute, span: Span) -> Result<(), MultipleAttrs> {
+        fn try_insert<T>(
+            slot: &mut Option<Spanned<T>>,
+            value: T,
+            span: Span,
+            category: &'static str,
+        ) -> Result<(), MultipleAttrs> {
+            match slot {
+                Some(prev) => Err(MultipleAttrs {
+                    prev_span: prev.span,
+                    category,
+                }),
+                None => {
+                    *slot = Some(Spanned { value, span });
+                    Ok(())
+                }
+            }
+        }
+
+        use SpirvAttribute::*;
+        match attr {
+            StorageClass(value) => {
+                try_insert(&mut self.storage_class, value, span, "storage class")
+            }
+            IntrinsicType(value) => {
+                try_insert(&mut self.intrinsic_type, value, span, "intrinsic type")
+            }
+            Block => try_insert(&mut self.block, (), span, "#[spirv(block)]"),
+            Entry(value) => try_insert(&mut self.entry, value, span, "entry-point"),
+            Builtin(value) => try_insert(&mut self.builtin, value, span, "builtin"),
+            DescriptorSet(value) => try_insert(
+                &mut self.descriptor_set,
+                value,
+                span,
+                "#[spirv(descriptor_set)]",
+            ),
+            Binding(value) => try_insert(&mut self.binding, value, span, "#[spirv(binding)]"),
+            Flat => try_insert(&mut self.flat, (), span, "#[spirv(flat)]"),
+            UnrollLoops => try_insert(&mut self.unroll_loops, (), span, "#[spirv(unroll_loops)]"),
+        }
+    }
+}
+
 // FIXME(eddyb) make this reusable from somewhere in `rustc`.
 fn target_from_impl_item<'tcx>(tcx: TyCtxt<'tcx>, impl_item: &hir::ImplItem<'_>) -> Target {
     match impl_item.kind {
@@ -123,6 +203,8 @@ struct CheckSpirvAttrVisitor<'tcx> {
 
 impl CheckSpirvAttrVisitor<'_> {
     fn check_spirv_attributes(&self, hir_id: HirId, target: Target) {
+        let mut aggregated_attrs = AggregatedSpirvAttributes::default();
+
         let parse_attrs = |attrs| crate::symbols::parse_attrs_for_checking(&self.sym, attrs);
 
         let attrs = self.tcx.hir().attrs(hir_id);
@@ -199,7 +281,6 @@ impl CheckSpirvAttrVisitor<'_> {
                 },
             };
             match valid_target {
-                Ok(()) => {}
                 Err(Expected(expected_target)) => self.tcx.sess.span_err(
                     span,
                     &format!(
@@ -207,6 +288,21 @@ impl CheckSpirvAttrVisitor<'_> {
                         expected_target, target
                     ),
                 ),
+                Ok(()) => match aggregated_attrs.try_insert_attr(parsed_attr, span) {
+                    Ok(()) => {}
+                    Err(MultipleAttrs {
+                        prev_span,
+                        category,
+                    }) => self
+                        .tcx
+                        .sess
+                        .struct_span_err(
+                            span,
+                            &format!("only one {} attribute is allowed on a {}", category, target),
+                        )
+                        .span_note(prev_span, &format!("previous {} attribute", category))
+                        .emit(),
+                },
             }
         }
     }

tests/ui/spirv-attr/multiple.rs

@@ -0,0 +1,47 @@
+// Tests that multiple `#[spirv(...)]` attributes that are either identical, or
+// part of the same mutually exclusive category, are properly disallowed.
+
+// build-fail
+
+use spirv_std as _;
+
+#[spirv(uniform, uniform)]
+struct _SameStorageClass {}
+
+#[spirv(uniform, push_constant)]
+struct _DiffStorageClass {}
+
+#[spirv(sampler, sampler)]
+struct _SameIntrinsicType {}
+
+#[spirv(
+    sampler,
+    image_type(dim = "Dim2D", depth = 0, arrayed = 0, multisampled = 0, sampled = 1, image_format = "Unknown"),
+)]
+struct _DiffIntrinsicType {}
+
+#[spirv(block, block)]
+struct _Block {}
+
+#[spirv(vertex, vertex)]
+fn _same_entry() {}
+
+#[spirv(vertex, fragment)]
+fn _diff_entry() {}
+
+#[spirv(vertex)]
+fn _entry(
+    #[spirv(position, position)] _same_builtin: (),
+    #[spirv(position, vertex_index)] _diff_builtin: (),
+
+    #[spirv(descriptor_set = 0, descriptor_set = 0)] _same_descriptor_set: (),
+    #[spirv(descriptor_set = 0, descriptor_set = 1)] _diff_descriptor_set: (),
+
+    #[spirv(binding = 0, binding = 0)] _same_binding: (),
+    #[spirv(binding = 0, binding = 1)] _diff_binding: (),
+
+    #[spirv(flat, flat)] _flat: (),
+) {}
+
+#[spirv(unroll_loops, unroll_loops)]
+fn _unroll_loops() {}

tests/ui/spirv-attr/multiple.stderr

@@ -0,0 +1,182 @@
+error: only one storage class attribute is allowed on a struct
+ --> $DIR/multiple.rs:8:18
+  |
+8 | #[spirv(uniform, uniform)]
+  |                  ^^^^^^^
+  |
+note: previous storage class attribute
+ --> $DIR/multiple.rs:8:9
+  |
+8 | #[spirv(uniform, uniform)]
+  |         ^^^^^^^
+
+error: only one storage class attribute is allowed on a struct
+  --> $DIR/multiple.rs:11:18
+   |
+11 | #[spirv(uniform, push_constant)]
+   |                  ^^^^^^^^^^^^^
+   |
+note: previous storage class attribute
+  --> $DIR/multiple.rs:11:9
+   |
+11 | #[spirv(uniform, push_constant)]
+   |         ^^^^^^^
+
+error: only one intrinsic type attribute is allowed on a struct
+  --> $DIR/multiple.rs:14:18
+   |
+14 | #[spirv(sampler, sampler)]
+   |                  ^^^^^^^
+   |
+note: previous intrinsic type attribute
+  --> $DIR/multiple.rs:14:9
+   |
+14 | #[spirv(sampler, sampler)]
+   |         ^^^^^^^
+
+error: only one intrinsic type attribute is allowed on a struct
+  --> $DIR/multiple.rs:19:5
+   |
+19 |     image_type(dim = "Dim2D", depth = 0, arrayed = 0, multisampled = 0, sampled = 1, image_format = "Unknown"),
+   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+   |
+note: previous intrinsic type attribute
+  --> $DIR/multiple.rs:18:5
+   |
+18 |     sampler,
+   |     ^^^^^^^
+
+error: only one #[spirv(block)] attribute is allowed on a struct
+  --> $DIR/multiple.rs:23:16
+   |
+23 | #[spirv(block, block)]
+   |                ^^^^^
+   |
+note: previous #[spirv(block)] attribute
+  --> $DIR/multiple.rs:23:9
+   |
+23 | #[spirv(block, block)]
+   |         ^^^^^
+
+error: only one entry-point attribute is allowed on a function
+  --> $DIR/multiple.rs:26:17
+   |
+26 | #[spirv(vertex, vertex)]
+   |                 ^^^^^^
+   |
+note: previous entry-point attribute
+  --> $DIR/multiple.rs:26:9
+   |
+26 | #[spirv(vertex, vertex)]
+   |         ^^^^^^
+
+error: only one entry-point attribute is allowed on a function
+  --> $DIR/multiple.rs:29:17
+   |
+29 | #[spirv(vertex, fragment)]
+   |                 ^^^^^^^^
+   |
+note: previous entry-point attribute
+  --> $DIR/multiple.rs:29:9
+   |
+29 | #[spirv(vertex, fragment)]
+   |         ^^^^^^
+
+error: only one builtin attribute is allowed on a function param
+  --> $DIR/multiple.rs:34:23
+   |
+34 |     #[spirv(position, position)] _same_builtin: (),
+   |                       ^^^^^^^^
+   |
+note: previous builtin attribute
+  --> $DIR/multiple.rs:34:13
+   |
+34 |     #[spirv(position, position)] _same_builtin: (),
+   |             ^^^^^^^^
+
+error: only one builtin attribute is allowed on a function param
+  --> $DIR/multiple.rs:35:23
+   |
+35 |     #[spirv(position, vertex_index)] _diff_builtin: (),
+   |                       ^^^^^^^^^^^^
+   |
+note: previous builtin attribute
+  --> $DIR/multiple.rs:35:13
+   |
+35 |     #[spirv(position, vertex_index)] _diff_builtin: (),
+   |             ^^^^^^^^
+
+error: only one #[spirv(descriptor_set)] attribute is allowed on a function param
+  --> $DIR/multiple.rs:37:33
+   |
+37 |     #[spirv(descriptor_set = 0, descriptor_set = 0)] _same_descriptor_set: (),
+   |                                 ^^^^^^^^^^^^^^^^^^
+   |
+note: previous #[spirv(descriptor_set)] attribute
+  --> $DIR/multiple.rs:37:13
+   |
+37 |     #[spirv(descriptor_set = 0, descriptor_set = 0)] _same_descriptor_set: (),
+   |             ^^^^^^^^^^^^^^^^^^
+
+error: only one #[spirv(descriptor_set)] attribute is allowed on a function param
+  --> $DIR/multiple.rs:38:33
+   |
+38 |     #[spirv(descriptor_set = 0, descriptor_set = 1)] _diff_descriptor_set: (),
+   |                                 ^^^^^^^^^^^^^^^^^^
+   |
+note: previous #[spirv(descriptor_set)] attribute
+  --> $DIR/multiple.rs:38:13
+   |
+38 |     #[spirv(descriptor_set = 0, descriptor_set = 1)] _diff_descriptor_set: (),
+   |             ^^^^^^^^^^^^^^^^^^
+
+error: only one #[spirv(binding)] attribute is allowed on a function param
+  --> $DIR/multiple.rs:40:26
+   |
+40 |     #[spirv(binding = 0, binding = 0)] _same_binding: (),
+   |                          ^^^^^^^^^^^
+   |
+note: previous #[spirv(binding)] attribute
+  --> $DIR/multiple.rs:40:13
+   |
+40 |     #[spirv(binding = 0, binding = 0)] _same_binding: (),
+   |             ^^^^^^^^^^^
+
+error: only one #[spirv(binding)] attribute is allowed on a function param
+  --> $DIR/multiple.rs:41:26
+   |
+41 |     #[spirv(binding = 0, binding = 1)] _diff_binding: (),
+   |                          ^^^^^^^^^^^
+   |
+note: previous #[spirv(binding)] attribute
+  --> $DIR/multiple.rs:41:13
+   |
+41 |     #[spirv(binding = 0, binding = 1)] _diff_binding: (),
+   |             ^^^^^^^^^^^
+
+error: only one #[spirv(flat)] attribute is allowed on a function param
+  --> $DIR/multiple.rs:43:19
+   |
+43 |     #[spirv(flat, flat)] _flat: (),
+   |                   ^^^^
+   |
+note: previous #[spirv(flat)] attribute
+  --> $DIR/multiple.rs:43:13
+   |
+43 |     #[spirv(flat, flat)] _flat: (),
+   |             ^^^^
+
+error: only one #[spirv(unroll_loops)] attribute is allowed on a function
+  --> $DIR/multiple.rs:46:23
+   |
+46 | #[spirv(unroll_loops, unroll_loops)]
+   |                       ^^^^^^^^^^^^
+   |
+note: previous #[spirv(unroll_loops)] attribute
+  --> $DIR/multiple.rs:46:9
+   |
+46 | #[spirv(unroll_loops, unroll_loops)]
+   |         ^^^^^^^^^^^^
+
+error: aborting due to 15 previous errors
+