From e59f00399edb707d202362c8e9bf50cfce89c5b7 Mon Sep 17 00:00:00 2001 From: Erich Gubler Date: Fri, 8 Nov 2024 12:09:08 -0500 Subject: [PATCH] =?UTF-8?q?fix(wgsl-in):=20include=20user=20and=20unknown?= =?UTF-8?q?=20rules=20in=20`diagnostic(=E2=80=A6)`=20tracking?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CHANGELOG.md | 4 +- naga/src/diagnostic_filter.rs | 25 ++- naga/src/front/wgsl/diagnostic_filter.rs | 178 +++++++++++++++++- naga/src/front/wgsl/parse/mod.rs | 83 ++++---- naga/src/valid/analyzer.rs | 9 +- .../out/ir/diagnostic-filter.compact.ron | 4 +- naga/tests/out/ir/diagnostic-filter.ron | 4 +- 7 files changed, 240 insertions(+), 67 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5149ae6c4..e0d478649 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -90,7 +90,7 @@ There are some limitations to keep in mind with this new functionality: - 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., ) before we took significant effort to fix this (tracked in ). -By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148](https://github.com/gfx-rs/wgpu/pull/6148), [#6533](https://github.com/gfx-rs/wgpu/pull/6533), [#6353](https://github.com/gfx-rs/wgpu/pull/6353). +By @ErichDonGubler in [#6456](https://github.com/gfx-rs/wgpu/pull/6456), [#6148](https://github.com/gfx-rs/wgpu/pull/6148), [#6533](https://github.com/gfx-rs/wgpu/pull/6533), [#6353](https://github.com/gfx-rs/wgpu/pull/6353), [#6537](https://github.com/gfx-rs/wgpu/pull/6537). ### New Features @@ -3482,4 +3482,4 @@ DeviceDescriptor { - concept of the storage hub - basic recording of passes and command buffers - submission-based lifetime tracking and command buffer recycling -- automatic resource transitions \ No newline at end of file +- automatic resource transitions diff --git a/naga/src/diagnostic_filter.rs b/naga/src/diagnostic_filter.rs index 48fe44af3..2fa5464cd 100644 --- a/naga/src/diagnostic_filter.rs +++ b/naga/src/diagnostic_filter.rs @@ -59,22 +59,35 @@ impl Severity { #[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))] pub enum FilterableTriggeringRule { + Standard(StandardFilterableTriggeringRule), + Unknown(Box), + User(Box<[Box; 2]>), +} + +/// A filterable triggering rule in a [`DiagnosticFilter`]. +/// +/// +#[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 StandardFilterableTriggeringRule { DerivativeUniformity, } -impl FilterableTriggeringRule { +impl StandardFilterableTriggeringRule { /// The default severity associated with this triggering rule. /// /// See for a table of default /// severities. pub(crate) const fn default_severity(self) -> Severity { match self { - FilterableTriggeringRule::DerivativeUniformity => Severity::Error, + Self::DerivativeUniformity => Severity::Error, } } } -/// A filter that modifies how diagnostics are emitted for shaders. +/// A filtering rule that modifies how diagnostics are emitted for shaders. /// /// #[derive(Clone, Debug)] @@ -230,13 +243,13 @@ pub struct 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`]. + /// is found, return the value of [`StandardFilterableTriggeringRule::default_severity`]. /// /// When `triggering_rule` is not applicable to this node, its parent is consulted recursively. pub(crate) fn search( node: Option>, arena: &Arena, - triggering_rule: FilterableTriggeringRule, + triggering_rule: StandardFilterableTriggeringRule, ) -> Severity { let mut next = node; while let Some(handle) = next { @@ -247,7 +260,7 @@ impl DiagnosticFilterNode { new_severity, } = inner; - if rule == &triggering_rule { + if rule == &FilterableTriggeringRule::Standard(triggering_rule) { return new_severity; } diff --git a/naga/src/front/wgsl/diagnostic_filter.rs b/naga/src/front/wgsl/diagnostic_filter.rs index ed37ef7fe..b3cd7956b 100644 --- a/naga/src/front/wgsl/diagnostic_filter.rs +++ b/naga/src/front/wgsl/diagnostic_filter.rs @@ -1,4 +1,8 @@ -use crate::diagnostic_filter::{FilterableTriggeringRule, Severity}; +use std::fmt::{self, Display, Formatter}; + +use crate::diagnostic_filter::{ + FilterableTriggeringRule, Severity, StandardFilterableTriggeringRule, +}; impl Severity { const ERROR: &'static str = "error"; @@ -18,10 +22,34 @@ impl Severity { } } +struct DisplayFilterableTriggeringRule<'a>(&'a FilterableTriggeringRule); + +impl Display for DisplayFilterableTriggeringRule<'_> { + fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result { + let &Self(inner) = self; + match *inner { + FilterableTriggeringRule::Standard(rule) => write!(f, "{}", rule.to_wgsl_ident()), + FilterableTriggeringRule::Unknown(ref rule) => write!(f, "{rule}"), + FilterableTriggeringRule::User(ref rules) => { + let &[ref seg1, ref seg2] = rules.as_ref(); + write!(f, "{seg1}.{seg2}") + } + } + } +} + impl FilterableTriggeringRule { + /// [`Display`] this rule's identifiers in WGSL. + pub const fn display_wgsl_ident(&self) -> impl Display + '_ { + DisplayFilterableTriggeringRule(self) + } +} + +impl StandardFilterableTriggeringRule { const DERIVATIVE_UNIFORMITY: &'static str = "derivative_uniformity"; - /// Convert from a sentinel word in WGSL into its associated [`FilterableTriggeringRule`], if possible. + /// Convert from a sentinel word in WGSL into its associated + /// [`StandardFilterableTriggeringRule`], if possible. pub fn from_wgsl_ident(s: &str) -> Option { Some(match s { Self::DERIVATIVE_UNIFORMITY => Self::DerivativeUniformity, @@ -29,10 +57,152 @@ impl FilterableTriggeringRule { }) } - /// Maps this [`FilterableTriggeringRule`] into the sentinel word associated with it in WGSL. - pub const fn to_wgsl_ident(&self) -> &'static str { + /// Maps this [`StandardFilterableTriggeringRule`] into the sentinel word associated with it in + /// WGSL. + pub const fn to_wgsl_ident(self) -> &'static str { match self { Self::DerivativeUniformity => Self::DERIVATIVE_UNIFORMITY, } } } + +#[cfg(test)] +mod test { + mod parse_sites_not_yet_supported { + use crate::front::wgsl::assert_parse_err; + + #[test] + fn user_rules() { + let shader = " +fn myfunc() { + if (true) @diagnostic(off, my.lint) { + // ^^^^^^^^^^^^^^^^^^^^^^^^^ not yet supported, should report an error + } +} +"; + assert_parse_err(shader, "\ +error: `@diagnostic(…)` attribute(s) not yet implemented + ┌─ wgsl:3:15 + │ +3 │ if (true) @diagnostic(off, my.lint) { + │ ^^^^^^^^^^^^^^^^^^^^^^^^^ can't use this on compound statements (yet) + │ + = note: Let Naga maintainers know that you ran into this at , so they can prioritize it! + +"); + } + + #[test] + fn unknown_rules() { + let shader = " +fn myfunc() { + if (true) @diagnostic(off, wat_is_this) { + // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ should emit a warning + } +} +"; + assert_parse_err(shader, "\ +error: `@diagnostic(…)` attribute(s) not yet implemented + ┌─ wgsl:3:12 + │ +3 │ if (true) @diagnostic(off, wat_is_this) { + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ can't use this on compound statements (yet) + │ + = note: Let Naga maintainers know that you ran into this at , so they can prioritize it! + +"); + } + } + + mod directive_conflict { + use crate::front::wgsl::assert_parse_err; + + #[test] + fn user_rules() { + let shader = " +diagnostic(off, my.lint); +diagnostic(warning, my.lint); +"; + assert_parse_err(shader, "\ +error: found conflicting `diagnostic(…)` rule(s) + ┌─ wgsl:2:1 + │ +2 │ diagnostic(off, my.lint); + │ ^^^^^^^^^^^^^^^^^^^^^^^^ first rule +3 │ diagnostic(warning, my.lint); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ second rule + │ + = note: Multiple `diagnostic(…)` rules with the same rule name conflict unless they are directives and the severity is the same. + = note: You should delete the rule you don't want. + +"); + } + + #[test] + fn unknown_rules() { + let shader = " +diagnostic(off, wat_is_this); +diagnostic(warning, wat_is_this); +"; + assert_parse_err(shader, "\ +error: found conflicting `diagnostic(…)` rule(s) + ┌─ wgsl:2:1 + │ +2 │ diagnostic(off, wat_is_this); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ first rule +3 │ diagnostic(warning, wat_is_this); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ second rule + │ + = note: Multiple `diagnostic(…)` rules with the same rule name conflict unless they are directives and the severity is the same. + = note: You should delete the rule you don't want. + +"); + } + } + + mod attribute_conflict { + use crate::front::wgsl::assert_parse_err; + + #[test] + fn user_rules() { + let shader = " +diagnostic(off, my.lint); +diagnostic(warning, my.lint); +"; + assert_parse_err(shader, "\ +error: found conflicting `diagnostic(…)` rule(s) + ┌─ wgsl:2:1 + │ +2 │ diagnostic(off, my.lint); + │ ^^^^^^^^^^^^^^^^^^^^^^^^ first rule +3 │ diagnostic(warning, my.lint); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ second rule + │ + = note: Multiple `diagnostic(…)` rules with the same rule name conflict unless they are directives and the severity is the same. + = note: You should delete the rule you don't want. + +"); + } + + #[test] + fn unknown_rules() { + let shader = " +diagnostic(off, wat_is_this); +diagnostic(warning, wat_is_this); +"; + assert_parse_err(shader, "\ +error: found conflicting `diagnostic(…)` rule(s) + ┌─ wgsl:2:1 + │ +2 │ diagnostic(off, wat_is_this); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ first rule +3 │ diagnostic(warning, wat_is_this); + │ ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ second rule + │ + = note: Multiple `diagnostic(…)` rules with the same rule name conflict unless they are directives and the severity is the same. + = note: You should delete the rule you don't want. + +"); + } + } +} diff --git a/naga/src/front/wgsl/parse/mod.rs b/naga/src/front/wgsl/parse/mod.rs index ae22eb38e..93b934332 100644 --- a/naga/src/front/wgsl/parse/mod.rs +++ b/naga/src/front/wgsl/parse/mod.rs @@ -1,6 +1,6 @@ use crate::diagnostic_filter::{ self, DiagnosticFilter, DiagnosticFilterMap, DiagnosticFilterNode, FilterableTriggeringRule, - ShouldConflictOnFullDuplicate, + ShouldConflictOnFullDuplicate, StandardFilterableTriggeringRule, }; use crate::front::wgsl::error::{Error, ExpectedToken}; use crate::front::wgsl::parse::directive::enable_extension::{ @@ -2166,10 +2166,9 @@ impl Parser { while lexer.skip(Token::Attribute) { let (name, name_span) = lexer.next_ident_with_span()?; if let Some(DirectiveKind::Diagnostic) = DirectiveKind::from_ident(name) { - if let Some(filter) = self.diagnostic_filter(lexer)? { - let span = self.peek_rule_span(lexer); - diagnostic_filters.add(filter, span, ShouldConflictOnFullDuplicate::Yes)?; - } + let filter = self.diagnostic_filter(lexer)?; + let span = self.peek_rule_span(lexer); + diagnostic_filters.add(filter, span, ShouldConflictOnFullDuplicate::Yes)?; } else { return Err(Error::Unexpected( name_span, @@ -2370,10 +2369,9 @@ impl Parser { while lexer.skip(Token::Attribute) { let (name, name_span) = lexer.next_ident_with_span()?; if let Some(DirectiveKind::Diagnostic) = DirectiveKind::from_ident(name) { - if let Some(filter) = self.diagnostic_filter(lexer)? { - let span = self.peek_rule_span(lexer); - diagnostic_filters.add(filter, span, ShouldConflictOnFullDuplicate::Yes)?; - } + let filter = self.diagnostic_filter(lexer)?; + let span = self.peek_rule_span(lexer); + diagnostic_filters.add(filter, span, ShouldConflictOnFullDuplicate::Yes)?; continue; } match name { @@ -2603,14 +2601,13 @@ impl Parser { let _ = lexer.next_ident_with_span().unwrap(); match kind { DirectiveKind::Diagnostic => { - if let Some(diagnostic_filter) = self.diagnostic_filter(&mut lexer)? { - let span = self.peek_rule_span(&lexer); - diagnostic_filters.add( - diagnostic_filter, - span, - ShouldConflictOnFullDuplicate::No, - )?; - } + let diagnostic_filter = self.diagnostic_filter(&mut lexer)?; + let span = self.peek_rule_span(&lexer); + diagnostic_filters.add( + diagnostic_filter, + span, + ShouldConflictOnFullDuplicate::No, + )?; lexer.expect(Token::Separator(';'))?; } DirectiveKind::Enable => { @@ -2698,10 +2695,7 @@ impl Parser { Ok(brace_nesting_level + 1) } - fn diagnostic_filter<'a>( - &self, - lexer: &mut Lexer<'a>, - ) -> Result, Error<'a>> { + fn diagnostic_filter<'a>(&self, lexer: &mut Lexer<'a>) -> Result> { lexer.expect(Token::Paren('('))?; let (severity_control_name, severity_control_name_span) = lexer.next_ident_with_span()?; @@ -2713,35 +2707,28 @@ impl Parser { lexer.expect(Token::Separator(','))?; let (diagnostic_name_token, diagnostic_name_token_span) = lexer.next_ident_with_span()?; - let diagnostic_rule_name = if lexer.skip(Token::Separator('.')) { - // Don't try to validate these name tokens on two tokens, which is conventionally used - // for third-party tooling. - lexer.next_ident_with_span()?; - None + let triggering_rule = if lexer.skip(Token::Separator('.')) { + let (ident, _span) = lexer.next_ident_with_span()?; + FilterableTriggeringRule::User(Box::new([diagnostic_name_token.into(), ident.into()])) } else { - Some(diagnostic_name_token) + let diagnostic_rule_name = diagnostic_name_token; + let diagnostic_rule_name_span = diagnostic_name_token_span; + if let Some(triggering_rule) = + StandardFilterableTriggeringRule::from_wgsl_ident(diagnostic_rule_name) + { + FilterableTriggeringRule::Standard(triggering_rule) + } else { + diagnostic_filter::Severity::Warning.report_wgsl_parse_diag( + Error::UnknownDiagnosticRuleName(diagnostic_rule_name_span), + lexer.source, + )?; + FilterableTriggeringRule::Unknown(diagnostic_rule_name.into()) + } + }; + let filter = DiagnosticFilter { + triggering_rule, + new_severity, }; - let diagnostic_rule_name_span = diagnostic_name_token_span; - - let filter = diagnostic_rule_name - .and_then(|name| { - FilterableTriggeringRule::from_wgsl_ident(name) - .map(Ok) - .or_else(|| { - diagnostic_filter::Severity::Warning - .report_wgsl_parse_diag( - Error::UnknownDiagnosticRuleName(diagnostic_rule_name_span), - lexer.source, - ) - .err() - .map(Err) - }) - }) - .transpose()? - .map(|triggering_rule| DiagnosticFilter { - new_severity, - triggering_rule, - }); lexer.skip(Token::Separator(',')); lexer.expect(Token::Paren(')'))?; diff --git a/naga/src/valid/analyzer.rs b/naga/src/valid/analyzer.rs index 0ca9829d6..ac9bb9cd5 100644 --- a/naga/src/valid/analyzer.rs +++ b/naga/src/valid/analyzer.rs @@ -6,7 +6,7 @@ //! - expression reference counts use super::{ExpressionError, FunctionError, ModuleInfo, ShaderStages, ValidationFlags}; -use crate::diagnostic_filter::{DiagnosticFilterNode, FilterableTriggeringRule}; +use crate::diagnostic_filter::{DiagnosticFilterNode, StandardFilterableTriggeringRule}; use crate::span::{AddSpan as _, WithSpan}; use crate::{ arena::{Arena, Handle}, @@ -852,7 +852,7 @@ impl FunctionInfo { let severity = DiagnosticFilterNode::search( self.diagnostic_filter_leaf, diagnostic_filter_arena, - FilterableTriggeringRule::DerivativeUniformity, + StandardFilterableTriggeringRule::DerivativeUniformity, ); severity.report_diag( FunctionError::NonUniformControlFlow(req, expr, cause) @@ -1372,7 +1372,10 @@ fn uniform_control_flow() { DiagnosticFilterNode { inner: crate::diagnostic_filter::DiagnosticFilter { new_severity: crate::diagnostic_filter::Severity::Off, - triggering_rule: FilterableTriggeringRule::DerivativeUniformity, + triggering_rule: + crate::diagnostic_filter::FilterableTriggeringRule::Standard( + StandardFilterableTriggeringRule::DerivativeUniformity, + ), }, parent: None, }, diff --git a/naga/tests/out/ir/diagnostic-filter.compact.ron b/naga/tests/out/ir/diagnostic-filter.compact.ron index 4294b06e9..315c8b342 100644 --- a/naga/tests/out/ir/diagnostic-filter.compact.ron +++ b/naga/tests/out/ir/diagnostic-filter.compact.ron @@ -44,14 +44,14 @@ ( inner: ( new_severity: Off, - triggering_rule: DerivativeUniformity, + triggering_rule: Standard(DerivativeUniformity), ), parent: None, ), ( inner: ( new_severity: Warning, - triggering_rule: DerivativeUniformity, + triggering_rule: Standard(DerivativeUniformity), ), parent: Some(0), ), diff --git a/naga/tests/out/ir/diagnostic-filter.ron b/naga/tests/out/ir/diagnostic-filter.ron index 4294b06e9..315c8b342 100644 --- a/naga/tests/out/ir/diagnostic-filter.ron +++ b/naga/tests/out/ir/diagnostic-filter.ron @@ -44,14 +44,14 @@ ( inner: ( new_severity: Off, - triggering_rule: DerivativeUniformity, + triggering_rule: Standard(DerivativeUniformity), ), parent: None, ), ( inner: ( new_severity: Warning, - triggering_rule: DerivativeUniformity, + triggering_rule: Standard(DerivativeUniformity), ), parent: Some(0), ),