don't panic if naga parsing of shader source fails (#5034)

* naga: glsl parser should return singular ParseError similar to wgsl * wgpu: treat glsl the same as wgsl when creating ShaderModule * naga: update glsl parser tests to use new ParseError type * naga: glsl ParseError errors field should be public * wgpu-core: add 'glsl' feature * fix some minor bugs in glsl parse error refactor * naga/wgpu/wgpu-core: improve spirv parse error handling * wgpu-core: feature gate use of glsl and spv naga modules * wgpu: enable wgpu-core glsl and spirv features when appropriate * obey clippy * naga: derive Clone in Type * naga: don't feature gate Clone derivation for Type * obey cargo fmt * wgpu-core: use bytemuck instead of zerocopy * wgpu-core: apply suggested edit * wgpu-core: no need to borrow spirv code * Update wgpu/src/backend/wgpu_core.rs Co-authored-by: Alphyr <[email protected]> --------- Co-authored-by: Alphyr <[email protected]>
gfx-rs · Jan 23, 2024 · c4b5cc9 · c4b5cc9
1 parent 60a5739
commit c4b5cc9
Show file tree

Hide file tree

Showing 14 changed files with 221 additions and 80 deletions.
diff --git a/Cargo.lock b/Cargo.lock
diff --git a/naga-cli/src/bin/naga.rs b/naga-cli/src/bin/naga.rs
@@ -488,9 +488,10 @@ fn parse_input(
                         },
                         &input,
                     )
-                    .unwrap_or_else(|errors| {
-                        let filename = input_path.file_name().and_then(std::ffi::OsStr::to_str);
-                        emit_glsl_parser_error(errors, filename.unwrap_or("glsl"), &input);
+                    .unwrap_or_else(|error| {
+                        let filename = input_path.file_name().and_then(std::ffi::OsStr::to_str).unwrap_or("glsl");
+                        let mut writer = StandardStream::stderr(ColorChoice::Auto);
+                        error.emit_to_writer_with_path(&mut writer, &input, filename);
                         std::process::exit(1);
                     }),
                 Some(input),
@@ -719,22 +720,6 @@ use codespan_reporting::{
 };
 use naga::WithSpan;
 
-pub fn emit_glsl_parser_error(errors: Vec<naga::front::glsl::Error>, filename: &str, source: &str) {
-    let files = SimpleFile::new(filename, source);
-    let config = codespan_reporting::term::Config::default();
-    let writer = StandardStream::stderr(ColorChoice::Auto);
-
-    for err in errors {
-        let mut diagnostic = Diagnostic::error().with_message(err.kind.to_string());
-
-        if let Some(range) = err.meta.to_range() {
-            diagnostic = diagnostic.with_labels(vec![Label::primary((), range)]);
-        }
-
-        term::emit(&mut writer.lock(), &config, &files, &diagnostic).expect("cannot write error");
-    }
-}
-
 pub fn emit_annotated_error<E: Error>(ann_err: &WithSpan<E>, filename: &str, source: &str) {
     let files = SimpleFile::new(filename, source);
     let config = codespan_reporting::term::Config::default();

diff --git a/naga/src/front/glsl/error.rs b/naga/src/front/glsl/error.rs
@@ -1,7 +1,11 @@
 use super::token::TokenValue;
 use crate::{proc::ConstantEvaluatorError, Span};
+use codespan_reporting::diagnostic::{Diagnostic, Label};
+use codespan_reporting::files::SimpleFile;
+use codespan_reporting::term;
 use pp_rs::token::PreprocessorError;
 use std::borrow::Cow;
+use termcolor::{NoColor, WriteColor};
 use thiserror::Error;
 
 fn join_with_comma(list: &[ExpectedToken]) -> String {
@@ -18,7 +22,7 @@ fn join_with_comma(list: &[ExpectedToken]) -> String {
 }
 
 /// One of the expected tokens returned in [`InvalidToken`](ErrorKind::InvalidToken).
-#[derive(Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq)]
 pub enum ExpectedToken {
     /// A specific token was expected.
     Token(TokenValue),
@@ -55,7 +59,7 @@ impl std::fmt::Display for ExpectedToken {
 }
 
 /// Information about the cause of an error.
-#[derive(Debug, Error)]
+#[derive(Clone, Debug, Error)]
 #[cfg_attr(test, derive(PartialEq))]
 pub enum ErrorKind {
     /// Whilst parsing as encountered an unexpected EOF.
@@ -123,7 +127,7 @@ impl From<ConstantEvaluatorError> for ErrorKind {
 }
 
 /// Error returned during shader parsing.
-#[derive(Debug, Error)]
+#[derive(Clone, Debug, Error)]
 #[error("{kind}")]
 #[cfg_attr(test, derive(PartialEq))]
 pub struct Error {
@@ -132,3 +136,56 @@ pub struct Error {
     /// Holds information about the range of the source code where the error happened.
     pub meta: Span,
 }
+
+/// A collection of errors returned during shader parsing.
+#[derive(Clone, Debug)]
+#[cfg_attr(test, derive(PartialEq))]
+pub struct ParseError {
+    pub errors: Vec<Error>,
+}
+
+impl ParseError {
+    pub fn emit_to_writer(&self, writer: &mut impl WriteColor, source: &str) {
+        self.emit_to_writer_with_path(writer, source, "glsl");
+    }
+
+    pub fn emit_to_writer_with_path(&self, writer: &mut impl WriteColor, source: &str, path: &str) {
+        let path = path.to_string();
+        let files = SimpleFile::new(path, source);
+        let config = codespan_reporting::term::Config::default();
+
+        for err in &self.errors {
+            let mut diagnostic = Diagnostic::error().with_message(err.kind.to_string());
+
+            if let Some(range) = err.meta.to_range() {
+                diagnostic = diagnostic.with_labels(vec![Label::primary((), range)]);
+            }
+
+            term::emit(writer, &config, &files, &diagnostic).expect("cannot write error");
+        }
+    }
+
+    pub fn emit_to_string(&self, source: &str) -> String {
+        let mut writer = NoColor::new(Vec::new());
+        self.emit_to_writer(&mut writer, source);
+        String::from_utf8(writer.into_inner()).unwrap()
+    }
+}
+
+impl std::fmt::Display for ParseError {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
+        self.errors.iter().try_for_each(|e| write!(f, "{e:?}"))
+    }
+}
+
+impl std::error::Error for ParseError {
+    fn source(&self) -> Option<&(dyn std::error::Error + 'static)> {
+        None
+    }
+}
+
+impl From<Vec<Error>> for ParseError {
+    fn from(errors: Vec<Error>) -> Self {
+        Self { errors }
+    }
+}
diff --git a/naga/src/front/glsl/mod.rs b/naga/src/front/glsl/mod.rs
@@ -13,7 +13,7 @@ To begin, take a look at the documentation for the [`Frontend`].
 */
 
 pub use ast::{Precision, Profile};
-pub use error::{Error, ErrorKind, ExpectedToken};
+pub use error::{Error, ErrorKind, ExpectedToken, ParseError};
 pub use token::TokenValue;
 
 use crate::{proc::Layouter, FastHashMap, FastHashSet, Handle, Module, ShaderStage, Span, Type};
@@ -196,7 +196,7 @@ impl Frontend {
         &mut self,
         options: &Options,
         source: &str,
-    ) -> std::result::Result<Module, Vec<Error>> {
+    ) -> std::result::Result<Module, ParseError> {
         self.reset(options.stage);
 
         let lexer = lex::Lexer::new(source, &options.defines);
@@ -207,12 +207,12 @@ impl Frontend {
                 if self.errors.is_empty() {
                     Ok(module)
                 } else {
-                    Err(std::mem::take(&mut self.errors))
+                    Err(std::mem::take(&mut self.errors).into())
                 }
             }
             Err(e) => {
                 self.errors.push(e);
-                Err(std::mem::take(&mut self.errors))
+                Err(std::mem::take(&mut self.errors).into())
             }
         }
     }

diff --git a/naga/src/front/glsl/parser_tests.rs b/naga/src/front/glsl/parser_tests.rs
@@ -1,7 +1,7 @@
 use super::{
     ast::Profile,
     error::ExpectedToken,
-    error::{Error, ErrorKind},
+    error::{Error, ErrorKind, ParseError},
     token::TokenValue,
     Frontend, Options, Span,
 };
@@ -21,10 +21,12 @@ fn version() {
             )
             .err()
             .unwrap(),
-        vec![Error {
-            kind: ErrorKind::InvalidVersion(99000),
-            meta: Span::new(9, 14)
-        }],
+        ParseError {
+            errors: vec![Error {
+                kind: ErrorKind::InvalidVersion(99000),
+                meta: Span::new(9, 14)
+            }],
+        },
     );
 
     assert_eq!(
@@ -35,10 +37,12 @@ fn version() {
             )
             .err()
             .unwrap(),
-        vec![Error {
-            kind: ErrorKind::InvalidVersion(449),
-            meta: Span::new(9, 12)
-        }]
+        ParseError {
+            errors: vec![Error {
+                kind: ErrorKind::InvalidVersion(449),
+                meta: Span::new(9, 12)
+            }]
+        },
     );
 
     assert_eq!(
@@ -49,10 +53,12 @@ fn version() {
             )
             .err()
             .unwrap(),
-        vec![Error {
-            kind: ErrorKind::InvalidProfile("smart".into()),
-            meta: Span::new(13, 18),
-        }]
+        ParseError {
+            errors: vec![Error {
+                kind: ErrorKind::InvalidProfile("smart".into()),
+                meta: Span::new(13, 18),
+            }]
+        },
     );
 
     assert_eq!(
@@ -63,19 +69,21 @@ fn version() {
             )
             .err()
             .unwrap(),
-        vec![
-            Error {
-                kind: ErrorKind::PreprocessorError(PreprocessorError::UnexpectedHash,),
-                meta: Span::new(27, 28),
-            },
-            Error {
-                kind: ErrorKind::InvalidToken(
-                    TokenValue::Identifier("version".into()),
-                    vec![ExpectedToken::Eof]
-                ),
-                meta: Span::new(28, 35)
-            }
-        ]
+        ParseError {
+            errors: vec![
+                Error {
+                    kind: ErrorKind::PreprocessorError(PreprocessorError::UnexpectedHash,),
+                    meta: Span::new(27, 28),
+                },
+                Error {
+                    kind: ErrorKind::InvalidToken(
+                        TokenValue::Identifier("version".into()),
+                        vec![ExpectedToken::Eof]
+                    ),
+                    meta: Span::new(28, 35)
+                }
+            ]
+        },
     );
 
     // valid versions
@@ -447,10 +455,12 @@ fn functions() {
             )
             .err()
             .unwrap(),
-        vec![Error {
-            kind: ErrorKind::SemanticError("Function already defined".into()),
-            meta: Span::new(134, 152),
-        }]
+        ParseError {
+            errors: vec![Error {
+                kind: ErrorKind::SemanticError("Function already defined".into()),
+                meta: Span::new(134, 152),
+            }]
+        },
     );
 
     println!();
@@ -626,10 +636,12 @@ fn implicit_conversions() {
             )
             .err()
             .unwrap(),
-        vec![Error {
-            kind: ErrorKind::SemanticError("Unknown function \'test\'".into()),
-            meta: Span::new(156, 165),
-        }]
+        ParseError {
+            errors: vec![Error {
+                kind: ErrorKind::SemanticError("Unknown function \'test\'".into()),
+                meta: Span::new(156, 165),
+            }]
+        },
     );
 
     assert_eq!(
@@ -648,10 +660,12 @@ fn implicit_conversions() {
             )
             .err()
             .unwrap(),
-        vec![Error {
-            kind: ErrorKind::SemanticError("Ambiguous best function for \'test\'".into()),
-            meta: Span::new(158, 165),
-        }]
+        ParseError {
+            errors: vec![Error {
+                kind: ErrorKind::SemanticError("Ambiguous best function for \'test\'".into()),
+                meta: Span::new(158, 165),
+            }]
+        }
     );
 }
 

diff --git a/naga/src/front/glsl/token.rs b/naga/src/front/glsl/token.rs
@@ -20,7 +20,7 @@ pub struct Token {
 ///
 /// This type is exported since it's returned in the
 /// [`InvalidToken`](super::ErrorKind::InvalidToken) error.
-#[derive(Debug, PartialEq)]
+#[derive(Clone, Debug, PartialEq)]
 pub enum TokenValue {
     Identifier(String),
 

diff --git a/naga/src/front/spv/error.rs b/naga/src/front/spv/error.rs
@@ -1,5 +1,9 @@
 use super::ModuleState;
 use crate::arena::Handle;
+use codespan_reporting::diagnostic::Diagnostic;
+use codespan_reporting::files::SimpleFile;
+use codespan_reporting::term;
+use termcolor::{NoColor, WriteColor};
 
 #[derive(Debug, thiserror::Error)]
 pub enum Error {
@@ -127,3 +131,24 @@ pub enum Error {
     )]
     NonBindingArrayOfImageOrSamplers,
 }
+
+impl Error {
+    pub fn emit_to_writer(&self, writer: &mut impl WriteColor, source: &str) {
+        self.emit_to_writer_with_path(writer, source, "glsl");
+    }
+
+    pub fn emit_to_writer_with_path(&self, writer: &mut impl WriteColor, source: &str, path: &str) {
+        let path = path.to_string();
+        let files = SimpleFile::new(path, source);
+        let config = codespan_reporting::term::Config::default();
+        let diagnostic = Diagnostic::error().with_message(format!("{self:?}"));
+
+        term::emit(writer, &config, &files, &diagnostic).expect("cannot write error");
+    }
+
+    pub fn emit_to_string(&self, source: &str) -> String {
+        let mut writer = NoColor::new(Vec::new());
+        self.emit_to_writer(&mut writer, source);
+        String::from_utf8(writer.into_inner()).unwrap()
+    }
+}
diff --git a/naga/src/lib.rs b/naga/src/lib.rs
@@ -687,8 +687,7 @@ pub enum ImageClass {
 }
 
 /// A data type declared in the module.
-#[derive(Debug, Eq, Hash, PartialEq)]
-#[cfg_attr(feature = "clone", derive(Clone))]
+#[derive(Clone, Debug, Eq, Hash, PartialEq)]
 #[cfg_attr(feature = "serialize", derive(Serialize))]
 #[cfg_attr(feature = "deserialize", derive(Deserialize))]
 #[cfg_attr(feature = "arbitrary", derive(Arbitrary))]
@@ -700,8 +699,7 @@ pub struct Type {
 }
 
 /// Enum with additional information, depending on the kind of type.
-#[derive(Debug, Eq, Hash, PartialEq)]
-#[cfg_attr(feature = "clone", derive(Clone))]
+#[derive(Clone, Debug, Eq, Hash, PartialEq)]
 #[cfg_attr(feature = "serialize", derive(Serialize))]
 #[cfg_attr(feature = "deserialize", derive(Deserialize))]
 #[cfg_attr(feature = "arbitrary", derive(Arbitrary))]