From 7e101dbbe7db20dcad73702ac7cd1382f538f2b9 Mon Sep 17 00:00:00 2001 From: IceSentry Date: Mon, 4 Nov 2024 11:57:35 -0500 Subject: [PATCH] Report missing import error (#91) # Objective If an import is missing from the module it currently just unwraps and panics without any explanations. # Solution Don't unwrap and bubble up the error instead. The error reporting is a bit weird because it just points at the start of the file and I'm not sure how to improve it. --------- Co-authored-by: robtfm <50659922+robtfm@users.noreply.github.com> --- src/compose/mod.rs | 88 ++++++++++++++----- src/compose/test.rs | 68 ++++++++++++++ .../tests/conditional_import_fail/middle.wgsl | 9 ++ .../conditional_import_fail/mod_a_b.wgsl | 3 + .../tests/conditional_import_fail/top.wgsl | 7 ++ .../conditional_import_fail/top_nested.wgsl | 5 ++ .../expected/conditional_missing_import.txt | 8 ++ .../conditional_missing_import_nested.txt | 8 ++ 8 files changed, 176 insertions(+), 20 deletions(-) create mode 100644 src/compose/tests/conditional_import_fail/middle.wgsl create mode 100644 src/compose/tests/conditional_import_fail/mod_a_b.wgsl create mode 100644 src/compose/tests/conditional_import_fail/top.wgsl create mode 100644 src/compose/tests/conditional_import_fail/top_nested.wgsl create mode 100644 src/compose/tests/expected/conditional_missing_import.txt create mode 100644 src/compose/tests/expected/conditional_missing_import_nested.txt diff --git a/src/compose/mod.rs b/src/compose/mod.rs index af6672f..4e0333f 100644 --- a/src/compose/mod.rs +++ b/src/compose/mod.rs @@ -1318,20 +1318,22 @@ impl Composer { &mut self, module_set: &ComposableModuleDefinition, shader_defs: &HashMap, - ) -> Result { + ) -> Result { let PreprocessOutput { preprocessed_source, imports, } = self .preprocessor .preprocess(&module_set.sanitized_source, shader_defs) - .map_err(|inner| ComposerError { - inner, - source: ErrSource::Module { - name: module_set.name.to_owned(), - offset: 0, - defs: shader_defs.clone(), - }, + .map_err(|inner| { + EnsureImportsError::from(ComposerError { + inner, + source: ErrSource::Module { + name: module_set.name.to_owned(), + offset: 0, + defs: shader_defs.clone(), + }, + }) })?; self.ensure_imports(imports.iter().map(|import| &import.definition), shader_defs)?; @@ -1346,6 +1348,7 @@ impl Composer { &preprocessed_source, imports, ) + .map_err(|err| err.into()) } // build required ComposableModules for a given set of shader_defs @@ -1353,10 +1356,11 @@ impl Composer { &mut self, imports: impl IntoIterator, shader_defs: &HashMap, - ) -> Result<(), ComposerError> { + ) -> Result<(), EnsureImportsError> { for ImportDefinition { import, .. } in imports.into_iter() { - // we've already ensured imports exist when they were added - let module_set = self.module_sets.get(import).unwrap(); + let Some(module_set) = self.module_sets.get(import) else { + return Err(EnsureImportsError::MissingImport(import.to_owned())); + }; if module_set.get_module(shader_defs).is_some() { continue; } @@ -1381,6 +1385,29 @@ impl Composer { } } +pub enum EnsureImportsError { + MissingImport(String), + ComposerError(ComposerError), +} + +impl EnsureImportsError { + fn into_composer_error(self, err_source: ErrSource) -> ComposerError { + match self { + EnsureImportsError::MissingImport(import) => ComposerError { + inner: ComposerErrorInner::ImportNotFound(import.to_owned(), 0), + source: err_source, + }, + EnsureImportsError::ComposerError(err) => err, + } + } +} + +impl From for EnsureImportsError { + fn from(value: ComposerError) -> Self { + EnsureImportsError::ComposerError(value) + } +} + #[derive(Default)] pub struct ComposableModuleDescriptor<'a> { pub source: &'a str, @@ -1594,12 +1621,7 @@ impl Composer { let sanitized_source = self.sanitize_and_set_auto_bindings(source); - let PreprocessorMetaData { - name, - defines, - imports, - .. - } = self + let PreprocessorMetaData { name, defines, .. } = self .preprocessor .get_preprocessor_metadata(&sanitized_source, true) .map_err(|inner| ComposerError { @@ -1614,6 +1636,18 @@ impl Composer { let name = name.unwrap_or_default(); + let PreprocessOutput { imports, .. } = self + .preprocessor + .preprocess(&sanitized_source, &shader_defs) + .map_err(|inner| ComposerError { + inner, + source: ErrSource::Constructing { + path: file_path.to_owned(), + source: sanitized_source.to_owned(), + offset: 0, + }, + })?; + // make sure imports have been added // and gather additional defs specified at module level for (import_name, offset) in imports @@ -1643,7 +1677,7 @@ impl Composer { inner: ComposerErrorInner::ImportNotFound(import_name.clone(), offset), source: ErrSource::Constructing { path: file_path.to_owned(), - source: sanitized_source, + source: sanitized_source.to_owned(), offset: 0, }, }); @@ -1652,8 +1686,22 @@ impl Composer { self.ensure_imports( imports.iter().map(|import| &import.definition), &shader_defs, - )?; - self.ensure_imports(additional_imports, &shader_defs)?; + ) + .map_err(|err| { + err.into_composer_error(ErrSource::Constructing { + path: file_path.to_owned(), + source: sanitized_source.to_owned(), + offset: 0, + }) + })?; + self.ensure_imports(additional_imports, &shader_defs) + .map_err(|err| { + err.into_composer_error(ErrSource::Constructing { + path: file_path.to_owned(), + source: sanitized_source.to_owned(), + offset: 0, + }) + })?; let definition = ComposableModuleDefinition { name, diff --git a/src/compose/test.rs b/src/compose/test.rs index ececa3c..b65c071 100644 --- a/src/compose/test.rs +++ b/src/compose/test.rs @@ -999,6 +999,74 @@ mod test { output_eq!(wgsl, "tests/expected/conditional_import_b.txt"); } + #[test] + fn conditional_missing_import() { + let mut composer = Composer::default(); + + composer + .add_composable_module(ComposableModuleDescriptor { + source: include_str!("tests/conditional_import_fail/mod_a_b.wgsl"), + file_path: "tests/conditional_import_fail/mod_a_b.wgsl", + ..Default::default() + }) + .unwrap(); + + let error = composer + .make_naga_module(NagaModuleDescriptor { + source: include_str!("tests/conditional_import_fail/top.wgsl"), + file_path: "tests/conditional_import_fail/top.wgsl", + ..Default::default() + }) + .err() + .unwrap(); + + let text = error.emit_to_string(&composer); + + // let mut f = std::fs::File::create("conditional_missing_import.txt").unwrap(); + // f.write_all(text.as_bytes()).unwrap(); + // drop(f); + + output_eq!(text, "tests/expected/conditional_missing_import.txt"); + } + + #[test] + fn conditional_missing_import_nested() { + let mut composer = Composer::default(); + + composer + .add_composable_module(ComposableModuleDescriptor { + source: include_str!("tests/conditional_import_fail/mod_a_b.wgsl"), + file_path: "tests/conditional_import_fail/mod_a_b.wgsl", + ..Default::default() + }) + .unwrap(); + + composer + .add_composable_module(ComposableModuleDescriptor { + source: include_str!("tests/conditional_import_fail/middle.wgsl"), + file_path: "tests/conditional_import_fail/middle.wgsl", + ..Default::default() + }) + .unwrap(); + + let error = composer + .make_naga_module(NagaModuleDescriptor { + source: include_str!("tests/conditional_import_fail/top_nested.wgsl"), + file_path: "tests/conditional_import_fail/top_nested.wgsl", + ..Default::default() + }) + .err() + .unwrap(); + + let text = error.emit_to_string(&composer); + + // let mut f = std::fs::File::create("conditional_missing_import_nested.txt").unwrap(); + // f.write_all(text.as_bytes()).unwrap(); + // drop(f); + + output_eq!(text, "tests/expected/conditional_missing_import_nested.txt"); + } + #[cfg(feature = "test_shader")] #[test] fn rusty_imports() { diff --git a/src/compose/tests/conditional_import_fail/middle.wgsl b/src/compose/tests/conditional_import_fail/middle.wgsl new file mode 100644 index 0000000..e6fef1d --- /dev/null +++ b/src/compose/tests/conditional_import_fail/middle.wgsl @@ -0,0 +1,9 @@ +#define_import_path middle + +#ifdef USE_A + #import a::b +#endif + +fn mid_fn() -> u32 { + return b::C; +} diff --git a/src/compose/tests/conditional_import_fail/mod_a_b.wgsl b/src/compose/tests/conditional_import_fail/mod_a_b.wgsl new file mode 100644 index 0000000..128e553 --- /dev/null +++ b/src/compose/tests/conditional_import_fail/mod_a_b.wgsl @@ -0,0 +1,3 @@ +#define_import_path a::b + +const C: u32 = 1u; diff --git a/src/compose/tests/conditional_import_fail/top.wgsl b/src/compose/tests/conditional_import_fail/top.wgsl new file mode 100644 index 0000000..a09a266 --- /dev/null +++ b/src/compose/tests/conditional_import_fail/top.wgsl @@ -0,0 +1,7 @@ +#ifdef USE_A + #import a::b +#endif + +fn main() -> u32 { + return b::C; +} diff --git a/src/compose/tests/conditional_import_fail/top_nested.wgsl b/src/compose/tests/conditional_import_fail/top_nested.wgsl new file mode 100644 index 0000000..b726ab0 --- /dev/null +++ b/src/compose/tests/conditional_import_fail/top_nested.wgsl @@ -0,0 +1,5 @@ +#import middle + +fn main() -> u32 { + return middle::mid_fn(); +} diff --git a/src/compose/tests/expected/conditional_missing_import.txt b/src/compose/tests/expected/conditional_missing_import.txt new file mode 100644 index 0000000..2b65d95 --- /dev/null +++ b/src/compose/tests/expected/conditional_missing_import.txt @@ -0,0 +1,8 @@ +error: required import 'b' not found + ┌─ tests/conditional_import_fail/top.wgsl:6:12 + │ +6 │ return b::C; + │ ^ + │ + = missing import 'b' + diff --git a/src/compose/tests/expected/conditional_missing_import_nested.txt b/src/compose/tests/expected/conditional_missing_import_nested.txt new file mode 100644 index 0000000..e29fe67 --- /dev/null +++ b/src/compose/tests/expected/conditional_missing_import_nested.txt @@ -0,0 +1,8 @@ +error: required import 'b' not found + ┌─ tests/conditional_import_fail/top_nested.wgsl:1:1 + │ +1 │ #import middle + │ ^ + │ + = missing import 'b' +