fix(wgsl-in): include user and unknown rules in diagnostic(…) tracking

This commit is contained in:
Erich Gubler 2024-11-08 12:09:08 -05:00
parent 1fc5e4027a
commit e59f00399e
7 changed files with 240 additions and 67 deletions

View File

@ -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. - 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>). - 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), [#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 ### New Features

View File

@ -59,22 +59,35 @@ impl Severity {
#[cfg_attr(feature = "deserialize", derive(Deserialize))] #[cfg_attr(feature = "deserialize", derive(Deserialize))]
#[cfg_attr(feature = "arbitrary", derive(Arbitrary))] #[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
pub enum FilterableTriggeringRule { pub enum FilterableTriggeringRule {
Standard(StandardFilterableTriggeringRule),
Unknown(Box<str>),
User(Box<[Box<str>; 2]>),
}
/// A filterable triggering rule in a [`DiagnosticFilter`].
///
/// <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 StandardFilterableTriggeringRule {
DerivativeUniformity, DerivativeUniformity,
} }
impl FilterableTriggeringRule { impl StandardFilterableTriggeringRule {
/// The default severity associated with this triggering rule. /// The default severity associated with this triggering rule.
/// ///
/// See <https://www.w3.org/TR/WGSL/#filterable-triggering-rules> for a table of default /// See <https://www.w3.org/TR/WGSL/#filterable-triggering-rules> for a table of default
/// severities. /// severities.
pub(crate) const fn default_severity(self) -> Severity { pub(crate) const fn default_severity(self) -> Severity {
match self { 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.
/// ///
/// <https://www.w3.org/TR/WGSL/#diagnostic-filter> /// <https://www.w3.org/TR/WGSL/#diagnostic-filter>
#[derive(Clone, Debug)] #[derive(Clone, Debug)]
@ -230,13 +243,13 @@ pub struct DiagnosticFilterNode {
impl DiagnosticFilterNode { impl DiagnosticFilterNode {
/// Finds the most specific filter rule applicable to `triggering_rule` from the chain of /// 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 /// 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. /// When `triggering_rule` is not applicable to this node, its parent is consulted recursively.
pub(crate) fn search( pub(crate) fn search(
node: Option<Handle<Self>>, node: Option<Handle<Self>>,
arena: &Arena<Self>, arena: &Arena<Self>,
triggering_rule: FilterableTriggeringRule, triggering_rule: StandardFilterableTriggeringRule,
) -> Severity { ) -> Severity {
let mut next = node; let mut next = node;
while let Some(handle) = next { while let Some(handle) = next {
@ -247,7 +260,7 @@ impl DiagnosticFilterNode {
new_severity, new_severity,
} = inner; } = inner;
if rule == &triggering_rule { if rule == &FilterableTriggeringRule::Standard(triggering_rule) {
return new_severity; return new_severity;
} }

View File

@ -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 { impl Severity {
const ERROR: &'static str = "error"; 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 { 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"; 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<Self> { pub fn from_wgsl_ident(s: &str) -> Option<Self> {
Some(match s { Some(match s {
Self::DERIVATIVE_UNIFORMITY => Self::DerivativeUniformity, Self::DERIVATIVE_UNIFORMITY => Self::DerivativeUniformity,
@ -29,10 +57,152 @@ impl FilterableTriggeringRule {
}) })
} }
/// Maps this [`FilterableTriggeringRule`] into the sentinel word associated with it in WGSL. /// Maps this [`StandardFilterableTriggeringRule`] into the sentinel word associated with it in
pub const fn to_wgsl_ident(&self) -> &'static str { /// WGSL.
pub const fn to_wgsl_ident(self) -> &'static str {
match self { match self {
Self::DerivativeUniformity => Self::DERIVATIVE_UNIFORMITY, 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 <https://github.com/gfx-rs/wgpu/issues/5320>, 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 <https://github.com/gfx-rs/wgpu/issues/5320>, 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.
");
}
}
}

View File

@ -1,6 +1,6 @@
use crate::diagnostic_filter::{ use crate::diagnostic_filter::{
self, DiagnosticFilter, DiagnosticFilterMap, DiagnosticFilterNode, FilterableTriggeringRule, self, DiagnosticFilter, DiagnosticFilterMap, DiagnosticFilterNode, FilterableTriggeringRule,
ShouldConflictOnFullDuplicate, ShouldConflictOnFullDuplicate, StandardFilterableTriggeringRule,
}; };
use crate::front::wgsl::error::{Error, ExpectedToken}; use crate::front::wgsl::error::{Error, ExpectedToken};
use crate::front::wgsl::parse::directive::enable_extension::{ use crate::front::wgsl::parse::directive::enable_extension::{
@ -2166,10 +2166,9 @@ impl Parser {
while lexer.skip(Token::Attribute) { while lexer.skip(Token::Attribute) {
let (name, name_span) = lexer.next_ident_with_span()?; let (name, name_span) = lexer.next_ident_with_span()?;
if let Some(DirectiveKind::Diagnostic) = DirectiveKind::from_ident(name) { if let Some(DirectiveKind::Diagnostic) = DirectiveKind::from_ident(name) {
if let Some(filter) = self.diagnostic_filter(lexer)? { let filter = self.diagnostic_filter(lexer)?;
let span = self.peek_rule_span(lexer); let span = self.peek_rule_span(lexer);
diagnostic_filters.add(filter, span, ShouldConflictOnFullDuplicate::Yes)?; diagnostic_filters.add(filter, span, ShouldConflictOnFullDuplicate::Yes)?;
}
} else { } else {
return Err(Error::Unexpected( return Err(Error::Unexpected(
name_span, name_span,
@ -2370,10 +2369,9 @@ impl Parser {
while lexer.skip(Token::Attribute) { while lexer.skip(Token::Attribute) {
let (name, name_span) = lexer.next_ident_with_span()?; let (name, name_span) = lexer.next_ident_with_span()?;
if let Some(DirectiveKind::Diagnostic) = DirectiveKind::from_ident(name) { if let Some(DirectiveKind::Diagnostic) = DirectiveKind::from_ident(name) {
if let Some(filter) = self.diagnostic_filter(lexer)? { let filter = self.diagnostic_filter(lexer)?;
let span = self.peek_rule_span(lexer); let span = self.peek_rule_span(lexer);
diagnostic_filters.add(filter, span, ShouldConflictOnFullDuplicate::Yes)?; diagnostic_filters.add(filter, span, ShouldConflictOnFullDuplicate::Yes)?;
}
continue; continue;
} }
match name { match name {
@ -2603,14 +2601,13 @@ impl Parser {
let _ = lexer.next_ident_with_span().unwrap(); let _ = lexer.next_ident_with_span().unwrap();
match kind { match kind {
DirectiveKind::Diagnostic => { DirectiveKind::Diagnostic => {
if let Some(diagnostic_filter) = self.diagnostic_filter(&mut lexer)? { let diagnostic_filter = self.diagnostic_filter(&mut lexer)?;
let span = self.peek_rule_span(&lexer); let span = self.peek_rule_span(&lexer);
diagnostic_filters.add( diagnostic_filters.add(
diagnostic_filter, diagnostic_filter,
span, span,
ShouldConflictOnFullDuplicate::No, ShouldConflictOnFullDuplicate::No,
)?; )?;
}
lexer.expect(Token::Separator(';'))?; lexer.expect(Token::Separator(';'))?;
} }
DirectiveKind::Enable => { DirectiveKind::Enable => {
@ -2698,10 +2695,7 @@ impl Parser {
Ok(brace_nesting_level + 1) Ok(brace_nesting_level + 1)
} }
fn diagnostic_filter<'a>( fn diagnostic_filter<'a>(&self, lexer: &mut Lexer<'a>) -> Result<DiagnosticFilter, Error<'a>> {
&self,
lexer: &mut Lexer<'a>,
) -> Result<Option<DiagnosticFilter>, Error<'a>> {
lexer.expect(Token::Paren('('))?; lexer.expect(Token::Paren('('))?;
let (severity_control_name, severity_control_name_span) = lexer.next_ident_with_span()?; let (severity_control_name, severity_control_name_span) = lexer.next_ident_with_span()?;
@ -2713,35 +2707,28 @@ impl Parser {
lexer.expect(Token::Separator(','))?; lexer.expect(Token::Separator(','))?;
let (diagnostic_name_token, diagnostic_name_token_span) = lexer.next_ident_with_span()?; let (diagnostic_name_token, diagnostic_name_token_span) = lexer.next_ident_with_span()?;
let diagnostic_rule_name = if lexer.skip(Token::Separator('.')) { let triggering_rule = if lexer.skip(Token::Separator('.')) {
// Don't try to validate these name tokens on two tokens, which is conventionally used let (ident, _span) = lexer.next_ident_with_span()?;
// for third-party tooling. FilterableTriggeringRule::User(Box::new([diagnostic_name_token.into(), ident.into()]))
lexer.next_ident_with_span()?;
None
} else { } 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.skip(Token::Separator(','));
lexer.expect(Token::Paren(')'))?; lexer.expect(Token::Paren(')'))?;

View File

@ -6,7 +6,7 @@
//! - expression reference counts //! - expression reference counts
use super::{ExpressionError, FunctionError, ModuleInfo, ShaderStages, ValidationFlags}; 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::span::{AddSpan as _, WithSpan};
use crate::{ use crate::{
arena::{Arena, Handle}, arena::{Arena, Handle},
@ -852,7 +852,7 @@ impl FunctionInfo {
let severity = DiagnosticFilterNode::search( let severity = DiagnosticFilterNode::search(
self.diagnostic_filter_leaf, self.diagnostic_filter_leaf,
diagnostic_filter_arena, diagnostic_filter_arena,
FilterableTriggeringRule::DerivativeUniformity, StandardFilterableTriggeringRule::DerivativeUniformity,
); );
severity.report_diag( severity.report_diag(
FunctionError::NonUniformControlFlow(req, expr, cause) FunctionError::NonUniformControlFlow(req, expr, cause)
@ -1372,7 +1372,10 @@ fn uniform_control_flow() {
DiagnosticFilterNode { DiagnosticFilterNode {
inner: crate::diagnostic_filter::DiagnosticFilter { inner: crate::diagnostic_filter::DiagnosticFilter {
new_severity: crate::diagnostic_filter::Severity::Off, new_severity: crate::diagnostic_filter::Severity::Off,
triggering_rule: FilterableTriggeringRule::DerivativeUniformity, triggering_rule:
crate::diagnostic_filter::FilterableTriggeringRule::Standard(
StandardFilterableTriggeringRule::DerivativeUniformity,
),
}, },
parent: None, parent: None,
}, },

View File

@ -44,14 +44,14 @@
( (
inner: ( inner: (
new_severity: Off, new_severity: Off,
triggering_rule: DerivativeUniformity, triggering_rule: Standard(DerivativeUniformity),
), ),
parent: None, parent: None,
), ),
( (
inner: ( inner: (
new_severity: Warning, new_severity: Warning,
triggering_rule: DerivativeUniformity, triggering_rule: Standard(DerivativeUniformity),
), ),
parent: Some(0), parent: Some(0),
), ),

View File

@ -44,14 +44,14 @@
( (
inner: ( inner: (
new_severity: Off, new_severity: Off,
triggering_rule: DerivativeUniformity, triggering_rule: Standard(DerivativeUniformity),
), ),
parent: None, parent: None,
), ),
( (
inner: ( inner: (
new_severity: Warning, new_severity: Warning,
triggering_rule: DerivativeUniformity, triggering_rule: Standard(DerivativeUniformity),
), ),
parent: Some(0), parent: Some(0),
), ),