WGSL: Implement diagnostic(…); directives and derivative_uniformity triggering rule

CHANGELOG.md

@@ -40,11 +40,62 @@ Bottom level categories:
 
 ## Unreleased
 
+## Major changes
+
+### The `diagnostic(…);` directive is now supported in WGSL
+
+Naga now parses `diagnostic(…);` directives according to the WGSL spec. This allows users to control certain lints, similar to Rust's `allow`, `warn`, and `deny` attributes. For example, in standard WGSL (but, notably, not Naga yet—see <https://github.com/gfx-rs/wgpu/issues/4369>) this snippet would emit a uniformity error:
+
+```wgsl
+@group(0) @binding(0) var s : sampler;
+@group(0) @binding(2) var tex : texture_2d<f32>;
+@group(1) @binding(0) var<storage, read> ro_buffer : array<f32, 4>;
+
+@fragment
+fn main(@builtin(position) p : vec4f) -> @location(0) vec4f {
+  if ro_buffer[0] == 0 {
+    // Emits a derivative uniformity error during validation.
+    return textureSample(tex, s, vec2(0.,0.));
+  }
+
+  return vec4f(0.);
+}
+```
+
+…but we can now silence it with the `off` severity level, like so:
+
+```wgsl
+// Disable the diagnosic with this…
+diagnostic(off, derivative_uniformity);
+
+@group(0) @binding(0) var s : sampler;
+@group(0) @binding(2) var tex : texture_2d<f32>;
+@group(1) @binding(0) var<storage, read> ro_buffer : array<f32, 4>;
+
+@fragment
+fn main(@builtin(position) p : vec4f) -> @location(0) vec4f {
+  if ro_buffer[0] == 0 {
+    // Look ma, no error!
+    return textureSample(tex, s, vec2(0.,0.));
+  }
+
+  return vec4f(0.);
+}
+```
+
+There are some limitations to keep in mind with this new functionality:
+
+- We do not yet support `diagnostic(…)` rules in attribute position (i.e., `@diagnostic(…) fn my_func { … }`). This is being tracked in <https://github.com/gfx-rs/wgpu/issues/5320>. We expect that rules in `fn` attribute position will be relaxed shortly (see <https://github.com/gfx-rs/wgpu/pull/6353>), but the prioritization for statement positions is unclear. If you are blocked by not being able to parse `diagnostic(…)` rules in statement positions, please let us know in that issue, so we can determine how to prioritize it!
+- Standard WGSL specifies `error`, `warning`, `info`, and `off` severity levels. These are all technically usable now! A caveat, though: warning- and info-level are only emitted to `stderr` via the `log` façade, rather than being reported through a `Result::Err` in Naga or the `CompilationInfo` interface in `wgpu{,-core}`. This will require breaking changes in Naga to fix, and is being tracked by <https://github.com/gfx-rs/wgpu/issues/6458>.
+- Not all lints can be controlled with `diagnostic(…)` rules. In fact, only the `derivative_uniformity` triggering rule exists in the WGSL standard. That said, Naga contributors are excited to see how this level of control unlocks a new ecosystem of configurable diagnostics.
+- Finally, `diagnostic(…)` rules are not yet emitted in WGSL output. This means that `wgsl-in` → `wgsl-out` is currently a lossy process. We felt that it was important to unblock users who needed `diagnostic(…)` rules (i.e., <https://github.com/gfx-rs/wgpu/issues/3135>) before we took significant effort to fix this (tracked in <https://github.com/gfx-rs/wgpu/issues/6496>).
+
+By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148](https://github.com/gfx-rs/wgpu/pull/6148).
+
 ### New Features
 
 #### Naga
 
-- Parse `diagnostic(…)` directives, but don't implement any triggering rules yet. By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456).
 - Fix an issue where `naga` CLI would incorrectly skip the first positional argument when `--stdin-file-path` was specified. By @ErichDonGubler in [#6480](https://github.com/gfx-rs/wgpu/pull/6480).
 - Fix textureNumLevels in the GLSL backend. By @magcius in [#6483](https://github.com/gfx-rs/wgpu/pull/6483).
 - Implement `quantizeToF16()` for WGSL frontend, and WGSL, SPIR-V, HLSL, MSL, and GLSL backends. By @jamienicol in [#6519](https://github.com/gfx-rs/wgpu/pull/6519).

naga/src/diagnostic_filter.rs

@@ -1,9 +1,24 @@
 //! [`DiagnosticFilter`]s and supporting functionality.
 
+#[cfg(feature = "wgsl-in")]
+use crate::Span;
+use crate::{Arena, Handle};
+#[cfg(feature = "arbitrary")]
+use arbitrary::Arbitrary;
+#[cfg(feature = "wgsl-in")]
+use indexmap::IndexMap;
+#[cfg(feature = "deserialize")]
+use serde::Deserialize;
+#[cfg(feature = "serialize")]
+use serde::Serialize;
+
 /// A severity set on a [`DiagnosticFilter`].
 ///
 /// <https://www.w3.org/TR/WGSL/#diagnostic-severity>
 #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
+#[cfg_attr(feature = "serialize", derive(Serialize))]
+#[cfg_attr(feature = "deserialize", derive(Deserialize))]
+#[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
 pub enum Severity {
     Off,
     Info,
@@ -33,7 +48,6 @@ impl Severity {
     /// Naga does not yet support diagnostic items at lesser severities than
     /// [`Severity::Error`]. When this is implemented, this method should be deleted, and the
     /// severity should be used directly for reporting diagnostics.
-    #[cfg(feature = "wgsl-in")]
     pub(crate) fn report_diag<E>(
         self,
         err: E,
@@ -57,6 +71,9 @@ impl Severity {
 ///
 /// <https://www.w3.org/TR/WGSL/#filterable-triggering-rules>
 #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
+#[cfg_attr(feature = "serialize", derive(Serialize))]
+#[cfg_attr(feature = "deserialize", derive(Deserialize))]
+#[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
 pub enum FilterableTriggeringRule {
     DerivativeUniformity,
 }
@@ -79,10 +96,13 @@ impl FilterableTriggeringRule {
         }
     }
 
-    #[cfg(feature = "wgsl-in")]
-    pub(crate) const fn tracking_issue_num(self) -> u16 {
+    /// The default severity associated with this triggering rule.
+    ///
+    /// See <https://www.w3.org/TR/WGSL/#filterable-triggering-rules> for a table of default
+    /// severities.
+    pub(crate) const fn default_severity(self) -> Severity {
         match self {
-            FilterableTriggeringRule::DerivativeUniformity => 5320,
+            FilterableTriggeringRule::DerivativeUniformity => Severity::Error,
         }
     }
 }
@@ -91,7 +111,140 @@ impl FilterableTriggeringRule {
 ///
 /// <https://www.w3.org/TR/WGSL/#diagnostic-filter>
 #[derive(Clone, Debug)]
+#[cfg_attr(feature = "serialize", derive(Serialize))]
+#[cfg_attr(feature = "deserialize", derive(Deserialize))]
+#[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
 pub struct DiagnosticFilter {
     pub new_severity: Severity,
     pub triggering_rule: FilterableTriggeringRule,
 }
+
+/// A map of diagnostic filters to their severity and first occurrence's span.
+///
+/// Intended for front ends' first step into storing parsed [`DiagnosticFilter`]s.
+#[derive(Clone, Debug, Default)]
+#[cfg(feature = "wgsl-in")]
+pub(crate) struct DiagnosticFilterMap(IndexMap<FilterableTriggeringRule, (Severity, Span)>);
+
+#[cfg(feature = "wgsl-in")]
+impl DiagnosticFilterMap {
+    pub(crate) fn new() -> Self {
+        Self::default()
+    }
+
+    /// Add the given `diagnostic_filter` parsed at the given `span` to this map.
+    pub(crate) fn add(
+        &mut self,
+        diagnostic_filter: DiagnosticFilter,
+        span: Span,
+    ) -> Result<(), ConflictingDiagnosticRuleError> {
+        use indexmap::map::Entry;
+
+        let &mut Self(ref mut diagnostic_filters) = self;
+        let DiagnosticFilter {
+            new_severity,
+            triggering_rule,
+        } = diagnostic_filter;
+
+        match diagnostic_filters.entry(triggering_rule) {
+            Entry::Vacant(entry) => {
+                entry.insert((new_severity, span));
+            }
+            Entry::Occupied(entry) => {
+                let &(first_severity, first_span) = entry.get();
+                if first_severity != new_severity {
+                    return Err(ConflictingDiagnosticRuleError {
+                        triggering_rule,
+                        triggering_rule_spans: [first_span, span],
+                    });
+                }
+            }
+        }
+        Ok(())
+    }
+}
+
+#[cfg(feature = "wgsl-in")]
+impl IntoIterator for DiagnosticFilterMap {
+    type Item = (FilterableTriggeringRule, (Severity, Span));
+
+    type IntoIter = indexmap::map::IntoIter<FilterableTriggeringRule, (Severity, Span)>;
+
+    fn into_iter(self) -> Self::IntoIter {
+        let Self(this) = self;
+        this.into_iter()
+    }
+}
+
+/// An error returned by [`DiagnosticFilterMap::add`] when it encounters conflicting rules.
+#[cfg(feature = "wgsl-in")]
+#[derive(Clone, Debug)]
+pub(crate) struct ConflictingDiagnosticRuleError {
+    pub triggering_rule: FilterableTriggeringRule,
+    pub triggering_rule_spans: [Span; 2],
+}
+
+/// Represents a single parent-linking node in a tree of [`DiagnosticFilter`]s backed by a
+/// [`crate::Arena`].
+///
+/// A single element of a _tree_ of diagnostic filter rules stored in
+/// [`crate::Module::diagnostic_filters`]. When nodes are built by a front-end, module-applicable
+/// filter rules are chained together in runs based on parse site.  For instance, given the
+/// following:
+///
+/// - Module-applicable rules `a` and `b`.
+/// - Rules `c` and `d`, applicable to an entry point called `c_and_d_func`.
+/// - Rule `e`, applicable to an entry point called `e_func`.
+///
+/// The tree would be represented as follows:
+///
+/// ```text
+/// a <- b
+///      ^
+///      |- c <- d
+///      |
+///      \- e
+/// ```
+///
+/// ...where:
+///
+/// - `d` is the first leaf consulted by validation in `c_and_d_func`.
+/// - `e` is the first leaf consulted by validation in `e_func`.
+#[derive(Clone, Debug)]
+#[cfg_attr(feature = "serialize", derive(Serialize))]
+#[cfg_attr(feature = "deserialize", derive(Deserialize))]
+#[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
+pub struct DiagnosticFilterNode {
+    pub inner: DiagnosticFilter,
+    pub parent: Option<Handle<DiagnosticFilterNode>>,
+}
+
+impl DiagnosticFilterNode {
+    /// Finds the most specific filter rule applicable to `triggering_rule` from the chain of
+    /// diagnostic filter rules in `arena`, starting with `node`, and returns its severity. If none
+    /// is found, return the value of [`FilterableTriggeringRule::default_severity`].
+    ///
+    /// When `triggering_rule` is not applicable to this node, its parent is consulted recursively.
+    pub(crate) fn search(
+        node: Option<Handle<Self>>,
+        arena: &Arena<Self>,
+        triggering_rule: FilterableTriggeringRule,
+    ) -> Severity {
+        let mut next = node;
+        while let Some(handle) = next {
+            let node = &arena[handle];
+            let &Self { ref inner, parent } = node;
+            let &DiagnosticFilter {
+                triggering_rule: rule,
+                new_severity,
+            } = inner;
+
+            if rule == triggering_rule {
+                return new_severity;
+            }
+
+            next = parent;
+        }
+        triggering_rule.default_severity()
+    }
+}

naga/src/front/wgsl/error.rs

@@ -1,4 +1,4 @@
-use crate::diagnostic_filter::FilterableTriggeringRule;
+use crate::diagnostic_filter::ConflictingDiagnosticRuleError;
 use crate::front::wgsl::parse::directive::enable_extension::{
     EnableExtension, UnimplementedEnableExtension,
 };
@@ -295,10 +295,13 @@ pub(crate) enum Error<'a> {
     DiagnosticInvalidSeverity {
         severity_control_name_span: Span,
     },
-    DiagnosticNotYetImplemented {
-        triggering_rule: FilterableTriggeringRule,
-        span: Span,
-    },
+    DiagnosticDuplicateTriggeringRule(ConflictingDiagnosticRuleError),
+}
+
+impl<'a> From<ConflictingDiagnosticRuleError> for Error<'a> {
+    fn from(value: ConflictingDiagnosticRuleError) -> Self {
+        Self::DiagnosticDuplicateTriggeringRule(value)
+    }
 }
 
 #[derive(Clone, Debug)]
@@ -1017,24 +1020,29 @@ impl<'a> Error<'a> {
                 )
                 .into()],
             },
-            Error::DiagnosticNotYetImplemented {
+            Error::DiagnosticDuplicateTriggeringRule(ConflictingDiagnosticRuleError {
                 triggering_rule,
-                span,
-            } => ParseError {
-                message: format!(
-                    "the `{}` diagnostic filter is not yet supported",
-                    triggering_rule.to_ident()
-                ),
-                labels: vec![(span, "".into())],
-                notes: vec![format!(
-                    concat!(
-                        "Let Naga maintainers know that you ran into this at ",
-                        "<https://github.com/gfx-rs/wgpu/issues/{}>, ",
-                        "so they can prioritize it!"
+                triggering_rule_spans,
+            }) => {
+                let [first_span, second_span] = triggering_rule_spans;
+                ParseError {
+                    message: format!(
+                        "found conflicting `diagnostic(…)` rule(s) for `{}`",
+                        triggering_rule.to_ident()
                     ),
-                    triggering_rule.tracking_issue_num()
-                )],
-            },
+                    labels: vec![
+                        (first_span, "first rule".into()),
+                        (second_span, "second rule".into()),
+                    ],
+                    notes: vec![concat!(
+                        "multiple `diagnostic(…)` rules with the same rule name ",
+                        "conflict unless the severity is the same; ",
+                        "delete the rule you don't want, or ",
+                        "ensure that all severities with the same rule name match"
+                    )
+                    .into()],
+                }
+            }
         }
     }
 }

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

@@ -1013,7 +1013,11 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
         &mut self,
         tu: &'temp ast::TranslationUnit<'source>,
     ) -> Result<crate::Module, Error<'source>> {
-        let mut module = crate::Module::default();
+        let mut module = crate::Module {
+            diagnostic_filters: tu.diagnostic_filters.clone(),
+            diagnostic_filter_leaf: tu.diagnostic_filter_leaf,
+            ..Default::default()
+        };
 
         let mut ctx = GlobalContext {
             ast_expressions: &tu.expressions,
@@ -1244,7 +1248,7 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
             .arguments
             .iter()
             .enumerate()
-            .map(|(i, arg)| {
+            .map(|(i, arg)| -> Result<_, Error<'_>> {
                 let ty = self.resolve_ast_type(arg.ty, ctx)?;
                 let expr = expressions
                     .append(crate::Expression::FunctionArgument(i as u32), arg.name.span);
@@ -1263,7 +1267,7 @@ impl<'source, 'temp> Lowerer<'source, 'temp> {
         let result = f
             .result
             .as_ref()
-            .map(|res| {
+            .map(|res| -> Result<_, Error<'_>> {
                 let ty = self.resolve_ast_type(res.ty, ctx)?;
                 Ok(crate::FunctionResult {
                     ty,

naga/src/front/wgsl/parse/ast.rs

@@ -1,3 +1,4 @@
+use crate::diagnostic_filter::DiagnosticFilterNode;
 use crate::front::wgsl::parse::directive::enable_extension::EnableExtensions;
 use crate::front::wgsl::parse::number::Number;
 use crate::front::wgsl::Scalar;
@@ -26,6 +27,17 @@ pub struct TranslationUnit<'a> {
     /// These are referred to by `Handle<ast::Type<'a>>` values.
     /// User-defined types are referred to by name until lowering.
     pub types: Arena<Type<'a>>,
+
+    /// Arena for all diagnostic filter rules parsed in this module, including those in functions.
+    ///
+    /// See [`DiagnosticFilterNode`] for details on how the tree is represented and used in
+    /// validation.
+    pub diagnostic_filters: Arena<DiagnosticFilterNode>,
+    /// The leaf of all `diagnostic(…)` directives in this module.
+    ///
+    /// See [`DiagnosticFilterNode`] for details on how the tree is represented and used in
+    /// validation.
+    pub diagnostic_filter_leaf: Option<Handle<DiagnosticFilterNode>>,
 }
 
 #[derive(Debug, Clone, Copy)]