Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WGSL: Add a skeleton for diagnostic filter rules/diagnostic(…); that reports nice errors #6456

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,12 @@ Bottom level categories:

## Unreleased

## 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).

## 23.0.0 (2024-10-25)

### Themes of this release
Expand Down
97 changes: 97 additions & 0 deletions naga/src/diagnostic_filter.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
//! [`DiagnosticFilter`]s and supporting functionality.

/// A severity set on a [`DiagnosticFilter`].
///
/// <https://www.w3.org/TR/WGSL/#diagnostic-severity>
#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)]
pub enum Severity {
Off,
Info,
Warning,
Error,
}

impl Severity {
const ERROR: &'static str = "error";
const WARNING: &'static str = "warning";
const INFO: &'static str = "info";
const OFF: &'static str = "off";

/// Convert from a sentinel word in WGSL into its associated [`Severity`], if possible.
pub fn from_ident(s: &str) -> Option<Self> {
Some(match s {
Self::ERROR => Self::Error,
Self::WARNING => Self::Warning,
Self::INFO => Self::Info,
Self::OFF => Self::Off,
_ => return None,
})
}

/// Checks whether this severity is [`Self::Error`].
///
/// 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,
log_handler: impl FnOnce(E, log::Level),
) -> Result<(), E> {
let log_level = match self {
Severity::Off => return Ok(()),

// NOTE: These severities are not yet reported.
Severity::Info => log::Level::Info,
Severity::Warning => log::Level::Warn,

Severity::Error => return Err(err),
};
log_handler(err, log_level);
Ok(())
}
}

/// 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)]
pub enum FilterableTriggeringRule {
DerivativeUniformity,
}

impl FilterableTriggeringRule {
const DERIVATIVE_UNIFORMITY: &'static str = "derivative_uniformity";

/// Convert from a sentinel word in WGSL into its associated [`FilterableTriggeringRule`], if possible.
pub fn from_ident(s: &str) -> Option<Self> {
Some(match s {
Self::DERIVATIVE_UNIFORMITY => Self::DerivativeUniformity,
_ => return None,
})
}

/// Maps this [`FilterableTriggeringRule`] into the sentinel word associated with it in WGSL.
pub const fn to_ident(self) -> &'static str {
match self {
Self::DerivativeUniformity => Self::DERIVATIVE_UNIFORMITY,
}
}

#[cfg(feature = "wgsl-in")]
pub(crate) const fn tracking_issue_num(self) -> u16 {
match self {
FilterableTriggeringRule::DerivativeUniformity => 5320,
}
}
}

/// A filter that modifies how diagnostics are emitted for shaders.
///
/// <https://www.w3.org/TR/WGSL/#diagnostic-filter>
#[derive(Clone, Debug)]
pub struct DiagnosticFilter {
pub new_severity: Severity,
pub triggering_rule: FilterableTriggeringRule,
}
50 changes: 50 additions & 0 deletions naga/src/front/wgsl/error.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::diagnostic_filter::FilterableTriggeringRule;
use crate::front::wgsl::parse::directive::enable_extension::{
EnableExtension, UnimplementedEnableExtension,
};
Expand Down Expand Up @@ -194,6 +195,7 @@ pub(crate) enum Error<'a> {
UnknownConservativeDepth(Span),
UnknownEnableExtension(Span, &'a str),
UnknownLanguageExtension(Span, &'a str),
UnknownDiagnosticRuleName(Span),
SizeAttributeTooLow(Span, u32),
AlignAttributeTooLow(Span, Alignment),
NonPowerOfTwoAlignAttribute(Span),
Expand Down Expand Up @@ -295,6 +297,13 @@ pub(crate) enum Error<'a> {
kind: UnimplementedLanguageExtension,
span: Span,
},
DiagnosticInvalidSeverity {
severity_control_name_span: Span,
},
DiagnosticNotYetImplemented {
triggering_rule: FilterableTriggeringRule,
span: Span,
},
}

#[derive(Clone, Debug)]
Expand Down Expand Up @@ -562,6 +571,15 @@ impl<'a> Error<'a> {
)
.into()],
},
Error::UnknownDiagnosticRuleName(span) => ParseError {
message: format!("unknown `diagnostic(…)` rule name `{}`", &source[span]),
labels: vec![(span, "not a valid diagnostic rule name".into())],
notes: vec![concat!(
"See available trigger rules at ",
"<https://www.w3.org/TR/WGSL/#filterable-triggering-rules>."
)
.into()],
},
Error::SizeAttributeTooLow(bad_span, min_size) => ParseError {
message: format!("struct member size must be at least {min_size}"),
labels: vec![(bad_span, format!("must be at least {min_size}").into())],
Expand Down Expand Up @@ -1008,6 +1026,38 @@ impl<'a> Error<'a> {
kind.tracking_issue_num()
)],
},
Error::DiagnosticInvalidSeverity {
severity_control_name_span,
} => ParseError {
message: "invalid `diagnostic(…)` severity".into(),
labels: vec![(
severity_control_name_span,
"not a valid severity level".into(),
)],
notes: vec![concat!(
"See available severities at ",
"<https://www.w3.org/TR/WGSL/#diagnostic-severity>."
)
.into()],
},
Error::DiagnosticNotYetImplemented {
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.tracking_issue_num()
)],
},
}
}
}
Expand Down
55 changes: 28 additions & 27 deletions naga/src/front/wgsl/parse/directive.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@ pub(crate) mod language_extension;
/// A parsed sentinel word indicating the type of directive to be parsed next.
#[derive(Clone, Copy, Debug, Hash, Eq, PartialEq)]
pub(crate) enum DirectiveKind {
/// A [`crate::diagnostic_filter`].
Diagnostic,
/// An [`enable_extension`].
Enable,
/// A [`language_extension`].
Expand All @@ -23,7 +25,7 @@ impl DirectiveKind {
/// Convert from a sentinel word in WGSL into its associated [`DirectiveKind`], if possible.
pub fn from_ident(s: &str) -> Option<Self> {
Some(match s {
Self::DIAGNOSTIC => Self::Unimplemented(UnimplementedDirectiveKind::Diagnostic),
Self::DIAGNOSTIC => Self::Diagnostic,
Self::ENABLE => Self::Enable,
Self::REQUIRES => Self::Requires,
_ => return None,
Expand All @@ -33,34 +35,45 @@ impl DirectiveKind {
/// Maps this [`DirectiveKind`] into the sentinel word associated with it in WGSL.
pub const fn to_ident(self) -> &'static str {
match self {
Self::Diagnostic => Self::DIAGNOSTIC,
Self::Enable => Self::ENABLE,
Self::Requires => Self::REQUIRES,
Self::Unimplemented(kind) => match kind {
UnimplementedDirectiveKind::Diagnostic => Self::DIAGNOSTIC,
},
Self::Unimplemented(kind) => match kind {},
}
}

#[cfg(test)]
fn iter() -> impl Iterator<Item = Self> {
use strum::IntoEnumIterator;

UnimplementedDirectiveKind::iter().map(Self::Unimplemented)
[Self::Diagnostic, Self::Enable, Self::Requires]
.into_iter()
.chain(UnimplementedDirectiveKind::iter().map(Self::Unimplemented))
}
}

/// A [`DirectiveKind`] that is not yet implemented. See [`DirectiveKind::Unimplemented`].
#[derive(Clone, Copy, Debug, Hash, Eq, PartialEq)]
#[cfg_attr(test, derive(strum::EnumIter))]
pub(crate) enum UnimplementedDirectiveKind {
Diagnostic,
}
pub(crate) enum UnimplementedDirectiveKind {}

impl UnimplementedDirectiveKind {
pub const fn tracking_issue_num(self) -> u16 {
match self {
Self::Diagnostic => 5320,
}
match self {}
}
}

impl crate::diagnostic_filter::Severity {
#[cfg(feature = "wgsl-in")]
pub(crate) fn report_wgsl_parse_diag<'a>(
self,
err: crate::front::wgsl::error::Error<'a>,
source: &str,
) -> Result<(), crate::front::wgsl::error::Error<'a>> {
self.report_diag(err, |e, level| {
let e = e.as_parse_error(source);
log::log!(level, "{}", e.emit_to_string(source));
})
}
}

Expand All @@ -73,25 +86,12 @@ mod test {
use super::{DirectiveKind, UnimplementedDirectiveKind};

#[test]
#[allow(clippy::never_loop, unreachable_code, unused_variables)]
fn unimplemented_directives() {
for unsupported_shader in UnimplementedDirectiveKind::iter() {
let shader;
let expected_msg;
match unsupported_shader {
UnimplementedDirectiveKind::Diagnostic => {
shader = "diagnostic(off,derivative_uniformity);";
expected_msg = "\
error: the `diagnostic` directive is not yet implemented
┌─ wgsl:1:1
1 │ diagnostic(off,derivative_uniformity);
│ ^^^^^^^^^^ this global directive is standard, but not yet implemented
= note: Let Naga maintainers know that you ran into this at <https://github.com/gfx-rs/wgpu/issues/5320>, so they can prioritize it!

";
}
};
match unsupported_shader {};

assert_parse_err(shader, expected_msg);
}
Expand All @@ -103,7 +103,7 @@ error: the `diagnostic` directive is not yet implemented
let directive;
let expected_msg;
match unsupported_shader {
DirectiveKind::Unimplemented(UnimplementedDirectiveKind::Diagnostic) => {
DirectiveKind::Diagnostic => {
directive = "diagnostic(off,derivative_uniformity)";
expected_msg = "\
error: expected global declaration, but found a global directive
Expand Down Expand Up @@ -142,6 +142,7 @@ error: expected global declaration, but found a global directive

";
}
DirectiveKind::Unimplemented(kind) => match kind {},
ErichDonGubler marked this conversation as resolved.
Show resolved Hide resolved
}

let shader = format!(
Expand Down
63 changes: 63 additions & 0 deletions naga/src/front/wgsl/parse/mod.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
use crate::diagnostic_filter::{self, DiagnosticFilter, FilterableTriggeringRule};
use crate::front::wgsl::error::{Error, ExpectedToken};
use crate::front::wgsl::parse::directive::enable_extension::{
EnableExtension, EnableExtensions, UnimplementedEnableExtension,
Expand Down Expand Up @@ -2529,6 +2530,17 @@ impl Parser {
self.push_rule_span(Rule::Directive, &mut lexer);
let _ = lexer.next_ident_with_span().unwrap();
match kind {
DirectiveKind::Diagnostic => {
if let Some(diagnostic_filter) = self.diagnostic_filter(&mut lexer)? {
let triggering_rule = diagnostic_filter.triggering_rule;
let span = self.peek_rule_span(&lexer);
Err(Error::DiagnosticNotYetImplemented {
triggering_rule,
span,
})?;
}
lexer.expect(Token::Separator(';'))?;
}
DirectiveKind::Enable => {
self.directive_ident_list(&mut lexer, |ident, span| {
let kind = EnableExtension::from_ident(ident, span)?;
Expand Down Expand Up @@ -2614,4 +2626,55 @@ impl Parser {
}
Ok(brace_nesting_level + 1)
}

fn diagnostic_filter<'a>(
&self,
lexer: &mut Lexer<'a>,
) -> Result<Option<DiagnosticFilter>, Error<'a>> {
lexer.expect(Token::Paren('('))?;

let (severity_control_name, severity_control_name_span) = lexer.next_ident_with_span()?;
let new_severity = diagnostic_filter::Severity::from_ident(severity_control_name).ok_or(
Error::DiagnosticInvalidSeverity {
severity_control_name_span,
},
)?;

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
} else {
Some(diagnostic_name_token)
};
let diagnostic_rule_name_span = diagnostic_name_token_span;

let filter = diagnostic_rule_name
.and_then(|name| {
FilterableTriggeringRule::from_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(')'))?;

Ok(filter)
}
}
1 change: 1 addition & 0 deletions naga/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ pub mod back;
mod block;
#[cfg(feature = "compact")]
pub mod compact;
pub mod diagnostic_filter;
pub mod error;
pub mod front;
pub mod keywords;
Expand Down