[shaders] More graceful error handling (#584)

The transition of the vello crate to use the vello_shaders crate
regressed developer ergonomics around compilation failures, in which
compilation failures always caused a panic and the displayed
debug-formatted error message wasn't pretty printed. The panic was
particularly bad with hot-reload since it effectively undid its
usefulness by crashing the whole process on shader misspellings.

This change alleviates those regressions by adding better display
formatting to WGSL parse errors returned from the shaders crate and
propagating errors up to clients instead of always panicking. The Debug
formatting is still a little wonky so call-sites are currently
responsible for formatting errors with Display to get a more readable
compiler message. This PR only updates `with_winit` with
pretty-printing.

examples/with_winit/src/lib.rs

@@ -131,7 +131,12 @@ fn run(
                 num_init_threads: NonZeroUsize::new(1),
             },
         )
-        .expect("Could create renderer");
+        .map_err(|e| {
+            // Pretty-print any renderer creation error using Display formatting before unwrapping.
+            eprintln!("{e}");
+            e
+        })
+        .expect("Failed to create renderer");
         #[cfg(feature = "wgpu-profiler")]
         renderer
             .profiler
@@ -571,7 +576,7 @@ fn run(
                     // We know that the only async here (`pop_error_scope`) is actually sync, so blocking is fine
                     match pollster::block_on(result) {
                         Ok(_) => log::info!("Reloading took {:?}", start.elapsed()),
-                        Err(e) => log::warn!("Failed to reload shaders because of {e}"),
+                        Err(e) => log::error!("Failed to reload shaders: {e}"),
                     }
                 }
             },
@@ -624,7 +629,11 @@ fn run(
                                     num_init_threads: NonZeroUsize::new(args.num_init_threads),
                                 },
                             )
-                            .expect("Could create renderer");
+                            .map_err(|e| {
+                                // Pretty-print any renderer creation error using Display formatting before unwrapping.
+                                anyhow::format_err!("{e}")
+                            })
+                            .expect("Failed to create renderer");
                             log::info!("Creating renderer {id} took {:?}", start.elapsed());
                             #[cfg(feature = "wgpu-profiler")]
                             renderer

vello/src/lib.rs

@@ -209,11 +209,20 @@ pub enum Error {
     #[error("Failed to async map a buffer")]
     BufferAsyncError(#[from] wgpu::BufferAsyncError),
 
+    #[cfg(feature = "wgpu")]
+    #[error("wgpu Error from scope")]
+    WgpuErrorFromScope(#[from] wgpu::Error),
+
     /// Failed to create [`GpuProfiler`].
     /// See [`wgpu_profiler::CreationError`] for more information.
     #[cfg(feature = "wgpu-profiler")]
     #[error("Couldn't create wgpu profiler")]
     ProfilerCreationError(#[from] wgpu_profiler::CreationError),
+
+    /// Failed to compile the shaders.
+    #[cfg(feature = "hot_reload")]
+    #[error("Failed to compile shaders:\n{0}")]
+    ShaderCompilation(#[from] vello_shaders::compile::ErrorVec),
 }
 
 #[allow(dead_code)] // this can be unused when wgpu feature is not used
@@ -293,7 +302,7 @@ impl Renderer {
             #[cfg(not(target_arch = "wasm32"))]
             engine.use_parallel_initialisation();
         }
-        let shaders = shaders::full_shaders(device, &mut engine, &options);
+        let shaders = shaders::full_shaders(device, &mut engine, &options)?;
         #[cfg(not(target_arch = "wasm32"))]
         engine.build_shaders_if_needed(device, options.num_init_threads);
         let blit = options
@@ -435,14 +444,14 @@ impl Renderer {
 
     /// Reload the shaders. This should only be used during `vello` development
     #[cfg(feature = "hot_reload")]
-    pub async fn reload_shaders(&mut self, device: &Device) -> Result<(), wgpu::Error> {
+    pub async fn reload_shaders(&mut self, device: &Device) -> Result<(), Error> {
         device.push_error_scope(wgpu::ErrorFilter::Validation);
         let mut engine = WgpuEngine::new(self.options.use_cpu);
         // We choose not to initialise these shaders in parallel, to ensure the error scope works correctly
-        let shaders = shaders::full_shaders(device, &mut engine, &self.options);
+        let shaders = shaders::full_shaders(device, &mut engine, &self.options)?;
         let error = device.pop_error_scope().await;
         if let Some(error) = error {
-            return Err(error);
+            return Err(error.into());
         }
         self.engine = engine;
         self.shaders = shaders;

vello/src/shaders.rs

@@ -12,7 +12,7 @@ use crate::ShaderId;
 use crate::{
     recording::{BindType, ImageFormat},
     wgpu_engine::WgpuEngine,
-    RendererOptions,
+    Error, RendererOptions,
 };
 
 // Shaders for the full pipeline
@@ -49,7 +49,7 @@ pub fn full_shaders(
     device: &Device,
     engine: &mut WgpuEngine,
     options: &RendererOptions,
-) -> FullShaders {
+) -> Result<FullShaders, Error> {
     use crate::wgpu_engine::CpuShaderType;
     use BindType::*;
 
@@ -60,7 +60,7 @@ pub fn full_shaders(
     //let force_gpu_from = Some("binning");
 
     #[cfg(feature = "hot_reload")]
-    let mut shaders = vello_shaders::compile::ShaderInfo::from_default();
+    let mut shaders = vello_shaders::compile::ShaderInfo::from_default()?;
     #[cfg(not(feature = "hot_reload"))]
     let shaders = vello_shaders::SHADERS;
 
@@ -247,7 +247,7 @@ pub fn full_shaders(
         None
     };
 
-    FullShaders {
+    Ok(FullShaders {
         pathtag_reduce,
         pathtag_reduce2,
         pathtag_scan,
@@ -271,5 +271,5 @@ pub fn full_shaders(
         fine_msaa8,
         fine_msaa16,
         pathtag_is_cpu: options.use_cpu,
-    }
+    })
 }

vello_shaders/build.rs

@@ -18,7 +18,13 @@ fn main() {
 
     println!("cargo:rerun-if-changed={}", compile::shader_dir().display());
 
-    let mut shaders = compile::ShaderInfo::from_default();
+    let mut shaders = match compile::ShaderInfo::from_default() {
+        Ok(s) => s,
+        Err(err) => {
+            eprintln!("{err}");
+            return;
+        }
+    };
 
     // Drop the HashMap and sort by name so that we get deterministic order.
     let mut shaders = shaders.drain().collect::<Vec<_>>();

vello_shaders/src/compile/mod.rs

@@ -5,6 +5,7 @@ use naga::front::wgsl;
 use naga::valid::{Capabilities, ModuleInfo, ValidationError, ValidationFlags};
 use naga::{AddressSpace, ArraySize, ImageClass, Module, StorageAccess, WithSpan};
 use std::collections::{HashMap, HashSet};
+use std::fmt;
 use std::path::{Path, PathBuf};
 use std::sync::OnceLock;
 use thiserror::Error;
@@ -17,18 +18,62 @@ pub mod msl;
 
 use crate::types::{BindType, BindingInfo, WorkgroupBufferInfo};
 
+pub type Result<T> = std::result::Result<T, Error>;
+pub type CoalescedResult<T> = std::result::Result<T, ErrorVec>;
+
+#[derive(Error, Debug)]
+pub struct ErrorVec(Vec<Error>);
+
+#[derive(Error, Debug)]
+#[error("{source} ({name}) {msg}")]
+pub struct Error {
+    name: String,
+    msg: String,
+    source: InnerError,
+}
+
 #[derive(Error, Debug)]
-pub enum Error {
-    #[error("failed to parse shader: {0}")]
+enum InnerError {
+    #[error("failed to parse shader")]
     Parse(#[from] wgsl::ParseError),
 
-    #[error("failed to validate shader: {0}")]
+    #[error("failed to validate shader")]
     Validate(#[from] WithSpan<ValidationError>),
 
     #[error("missing entry point function")]
     EntryPointNotFound,
 }
 
+impl fmt::Display for ErrorVec {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        for e in self.0.iter() {
+            write!(f, "{e}")?;
+        }
+        Ok(())
+    }
+}
+
+impl Error {
+    fn new(wgsl: &str, name: &str, error: impl Into<InnerError>) -> Error {
+        let source = error.into();
+        Error {
+            name: name.to_owned(),
+            msg: source.emit_msg(wgsl, &format!("({name} preprocessed)")),
+            source,
+        }
+    }
+}
+
+impl InnerError {
+    fn emit_msg(&self, wgsl: &str, name: &str) -> String {
+        match self {
+            Self::Parse(e) => e.emit_to_string_with_path(wgsl, name),
+            Self::Validate(e) => e.emit_to_string_with_path(wgsl, name),
+            _ => String::default(),
+        }
+    }
+}
+
 #[derive(Debug)]
 pub struct ShaderInfo {
     pub source: String,
@@ -40,16 +85,17 @@ pub struct ShaderInfo {
 }
 
 impl ShaderInfo {
-    pub fn new(source: String, entry_point: &str) -> Result<ShaderInfo, Error> {
-        let module = wgsl::parse_str(&source)?;
+    pub fn new(name: &str, source: String, entry_point: &str) -> Result<ShaderInfo> {
+        let module = wgsl::parse_str(&source).map_err(|error| Error::new(&source, name, error))?;
         let module_info = naga::valid::Validator::new(ValidationFlags::all(), Capabilities::all())
-            .validate(&module)?;
+            .validate(&module)
+            .map_err(|error| Error::new(&source, name, error))?;
         let (entry_index, entry) = module
             .entry_points
             .iter()
             .enumerate()
             .find(|(_, entry)| entry.name.as_str() == entry_point)
-            .ok_or(Error::EntryPointNotFound)?;
+            .ok_or(Error::new(&source, name, InnerError::EntryPointNotFound))?;
         let mut bindings = vec![];
         let mut workgroup_buffers = vec![];
         let mut wg_buffer_idx = 0;
@@ -131,11 +177,11 @@ impl ShaderInfo {
     }
 
     /// Same as [`ShaderInfo::from_dir`] but uses the default shader directory provided by [`shader_dir`].
-    pub fn from_default() -> HashMap<String, Self> {
+    pub fn from_default() -> CoalescedResult<HashMap<String, Self>> {
         ShaderInfo::from_dir(shader_dir())
     }
 
-    pub fn from_dir(shader_dir: impl AsRef<Path>) -> HashMap<String, Self> {
+    pub fn from_dir(shader_dir: impl AsRef<Path>) -> CoalescedResult<HashMap<String, Self>> {
         use std::fs;
         let shader_dir = shader_dir.as_ref();
         let permutation_map = if let Ok(permutations_source) =
@@ -147,6 +193,7 @@ impl ShaderInfo {
         };
         //println!("{permutation_map:?}");
         let imports = preprocess::get_imports(shader_dir);
+        let mut errors = vec![];
         let mut info = HashMap::default();
         let defines: HashSet<_> = if cfg!(feature = "full") {
             vec!["full".to_string()]
@@ -174,18 +221,34 @@ impl ShaderInfo {
                             let mut defines = defines.clone();
                             defines.extend(permutation.defines.iter().cloned());
                             let source = preprocess::preprocess(&contents, &defines, &imports);
-                            let shader_info = Self::new(source.clone(), "main").unwrap();
-                            info.insert(permutation.name.clone(), shader_info);
+                            match Self::new(&permutation.name, source, "main") {
+                                Ok(shader_info) => {
+                                    info.insert(permutation.name.clone(), shader_info);
+                                }
+                                Err(e) => {
+                                    errors.push(e);
+                                }
+                            }
                         }
                     } else {
                         let source = preprocess::preprocess(&contents, &defines, &imports);
-                        let shader_info = Self::new(source.clone(), "main").unwrap();
-                        info.insert(shader_name.to_string(), shader_info);
+                        match Self::new(shader_name, source, "main") {
+                            Ok(shader_info) => {
+                                info.insert(shader_name.to_string(), shader_info);
+                            }
+                            Err(e) => {
+                                errors.push(e);
+                            }
+                        }
                     }
                 }
             }
         }
-        info
+        if !errors.is_empty() {
+            Err(ErrorVec(errors))
+        } else {
+            Ok(info)
+        }
     }
 }