diff --git a/.gitattributes b/.gitattributes index d599c9151..883160834 100644 --- a/.gitattributes +++ b/.gitattributes @@ -3,5 +3,6 @@ packages/packsquash/src/pack_file/json_file/*.json eol=lf packages/packsquash/src/pack_file/properties_file/*.properties eol=lf packages/packsquash/src/pack_file/shader_file/*.fsh eol=lf +packages/packsquash/src/pack_file/shader_file/*.glsl eol=lf packages/packsquash/src/pack_file/legacy_lang_file/*.lang eol=lf packages/packsquash/src/pack_file/command_function_file/*.mcfunction eol=lf diff --git a/CHANGELOG.md b/CHANGELOG.md index 5c96ae479..9b7fcf704 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -65,6 +65,26 @@ Versioning](https://semver.org/spec/v2.0.0.html). #### Fixed +- Shaders that depend on `#moj_import`ed or parent-defined preprocessor + variables to be syntactically correct or expand to the intended source code + will no longer cause PackSquash to fail or change their semantics. + - Since PackSquash still can't resolve `#moj_import` directives, this fix came + at the cost of falling back to not doing any source transformations + (minification, transformation) when this directive is used. Note that + PackSquash will never be able to resolve these directives in the general + case, because shaders can import shaders from other packs, including the + default game pack, which PackSquash doesn't have access to. + - If PackSquash cannot parse a shader, the parsing error is now considered + tentative, causing a fallback to no source transformation, if the shader + contains a `#moj_import` directive, as it cannot be ruled out that the + shader source would be valid if the `#moj_import` is expanded. + - In the future, we look forward to improve PackSquash `#moj_import` expansion + capabilities. At least, we should be able to better handle the common case + of imports of other shaders from the same pack. +- Include shaders now do not need to be parsable as translation units, + statements or expressions. Failure to parse them as such will result in a + tentative syntax error to be shown, but such an error will not be fatal and + PackSquash will fall back to no source transformation. - The UTF-8 BOM is no longer automatically stripped from properties files, as they should not be encoded in UTF-8 to begin with, and carrying on processing with mismatched encodings may cause mojibake. diff --git a/Cargo.lock b/Cargo.lock index 47c7d2b81..7bd90a69a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -343,26 +343,6 @@ dependencies = [ "tiny-keccak", ] -[[package]] -name = "const_format" -version = "0.2.31" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c990efc7a285731f9a4378d81aff2f0e85a2c8781a05ef0f8baa8dac54d0ff48" -dependencies = [ - "const_format_proc_macros", -] - -[[package]] -name = "const_format_proc_macros" -version = "0.2.31" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e026b6ce194a874cb9cf32cd5772d1ef9767cc8fcb5765948d74f37a9d8b2bf6" -dependencies = [ - "proc-macro2", - "quote", - "unicode-xid", -] - [[package]] name = "convert_case" version = "0.4.0" @@ -1634,7 +1614,6 @@ dependencies = [ "aho-corasick", "bytes", "const-random", - "const_format", "core-foundation", "crc32fast", "criterion", diff --git a/packages/packsquash/Cargo.toml b/packages/packsquash/Cargo.toml index fec661951..35cb7e3be 100644 --- a/packages/packsquash/Cargo.toml +++ b/packages/packsquash/Cargo.toml @@ -59,7 +59,6 @@ crc32fast = { version = "1.3.2", features = ["nightly"] } zopfli = { version = "0.8.0", default-features = false, features = ["std", "nightly"] } const-random = "0.1.15" -const_format = "0.2.31" aes = "0.8.3" fpe = "0.6.1" uuid = "1.4.1" diff --git a/packages/packsquash/src/pack_file/shader_file.rs b/packages/packsquash/src/pack_file/shader_file.rs index 7a168a50b..8fc9eaad8 100644 --- a/packages/packsquash/src/pack_file/shader_file.rs +++ b/packages/packsquash/src/pack_file/shader_file.rs @@ -88,9 +88,11 @@ impl Decoder for OptimizerDecoder { } else { // Include shaders may not necessarily be a translation unit. In fact, they technically // can be any text, as long as its inclusion in a top-level shader yields valid GLSL. As - // a compromise between rejecting technically valid inputs and allowing potential nonsensical - // text in them, which hampers validation and minification/prettifying capabilities, - // also accept standalone statements and expressions + // a compromise between requiring include shaders to conform to a known GLSL grammar symbol, + // which is required for validation and minification/prettifying capabilities, and accepting + // practical input that may not be parsable as a grammar symbol, also accept standalone + // statements and expressions, and fall back to warning about tentative syntax errors if + // parsing the source as any GLSL grammar symbol failed process_shader_as::( src, &shader_parser, @@ -108,6 +110,16 @@ impl Decoder for OptimizerDecoder { .or_else(|_| { process_shader_as::(src, &shader_parser, false, source_transformation_strategy) }) + .or_else(|err| { + if let OptimizationError::InvalidShader(err) = err { + Ok(Some(( + Cow::Owned(format!("Copied, but validation tentatively failed: {err}")), + src.split_off(0) + ))) + } else { + Err(err) + } + }) } } } @@ -167,33 +179,51 @@ fn process_shader_as + 'static>( where ParsedSymbol: Transpilable { - if let ( - Some(symbol), - ShaderSourceTransformationStrategy::Minify | ShaderSourceTransformationStrategy::Prettify - ) = ( - shader_parser.parse::(src, is_top_level_translation_unit)?, + match ( + shader_parser.parse::(src, is_top_level_translation_unit), source_transformation_strategy ) { - // The shader is valid and safe to transform - let minify = matches!( - source_transformation_strategy, - ShaderSourceTransformationStrategy::Minify - ); - - let mut buf = src.split_off(0); - buf.clear(); - - buf.extend_from_slice(symbol.transpile(minify).as_bytes()); - - Ok(Some(( - Cow::Borrowed(if minify { "Minified" } else { "Prettified" }), - buf - ))) - } else { - // The shader is valid, but not safe to transform, or we don't want to transform it - Ok(Some(( - Cow::Borrowed("Validated and copied"), + ( + Ok(Some(symbol)), + ShaderSourceTransformationStrategy::Minify | ShaderSourceTransformationStrategy::Prettify + ) => { + // The shader is valid and safe to transform, and we want to transform it + let minify = matches!( + source_transformation_strategy, + ShaderSourceTransformationStrategy::Minify + ); + + let mut buf = src.split_off(0); + buf.clear(); + + buf.extend_from_slice(symbol.transpile(minify).as_bytes()); + + Ok(Some(( + Cow::Borrowed(if minify { "Minified" } else { "Prettified" }), + buf + ))) + } + (Ok(None), _) | (Ok(Some(_)), ShaderSourceTransformationStrategy::KeepAsIs) => { + // The shader is valid, but not safe to transform, or we don't want to transform it + Ok(Some(( + Cow::Borrowed("Validated and copied"), + src.split_off(0) + ))) + } + ( + Err(ParseError::Syntax { + error, + found_unresolvable_moj_import: true + }), + _ + ) => Ok(Some(( + // The shader is not valid, but the syntax error may be a false positive due to + // missing preprocessor context + Cow::Owned(format!( + "Copied, but validation tentatively failed: {error}" + )), src.split_off(0) - ))) + ))), + (Err(err), _) => Err(err.into()) } } diff --git a/packages/packsquash/src/pack_file/shader_file/example.fsh b/packages/packsquash/src/pack_file/shader_file/example.fsh index eca299f81..5d8c9f182 100644 --- a/packages/packsquash/src/pack_file/shader_file/example.fsh +++ b/packages/packsquash/src/pack_file/shader_file/example.fsh @@ -5,12 +5,8 @@ #version 120 // Dummy variables added to check that transpilation doesn't mess with the preprocessor directives -vec2 dummy_variable; -vec3 dummy_var_2; - -// Test that the #moj_import directive added in 1.17 works fine -#moj_import -#moj_import "common.fsh" +vec2 dummyVariable; +vec3 dummyVariable2; // Dummy function added to check that it transpiles fine float rand(vec2 co, vec2 _) { @@ -21,6 +17,8 @@ varying vec4 texcoord; uniform sampler2D gcolor; uniform mat3 IViewRotMat; +#pragma optimize(on) + void main() { // Get the location of the current pixel on the screen. // point.x ranges from 0 on the left to 1 on the right. @@ -44,6 +42,13 @@ void main() { #error "Unexpected GLSL version" #endif + // The following lines would cause our current lacks_injectable_pp_directives_out_of_external_declaration_position + // implementation to return false, even though all the preprocessor directives here would be expanded, because + // TYPE and SEMICOLON would not be defined identifiers + //#define TYPE vec2 + //#define SEMICOLON ; + //TYPE dummyVar SEMICOLON + // Here's where we tell Minecraft what color we want this pixel. gl_FragColor = vec4(color, 1.0); } diff --git a/packages/packsquash/src/pack_file/shader_file/example_false_positive_parse_error.glsl b/packages/packsquash/src/pack_file/shader_file/example_false_positive_parse_error.glsl new file mode 100644 index 000000000..3e65da6d0 --- /dev/null +++ b/packages/packsquash/src/pack_file/shader_file/example_false_positive_parse_error.glsl @@ -0,0 +1,8 @@ +// unresolvable.glsl may be resolvable by the game, but +// not by us, and contain preprocessor directives that +// may render otherwise invalid GLSL source valid. In +// the example below, unresolvable.glsl may define +// SEMICOLON to be ; +#moj_import + +vec2 var SEMICOLON diff --git a/packages/packsquash/src/pack_file/shader_file/example_non_transformable.glsl b/packages/packsquash/src/pack_file/shader_file/example_non_transformable.glsl index ac9bc9a3d..362f68922 100644 --- a/packages/packsquash/src/pack_file/shader_file/example_non_transformable.glsl +++ b/packages/packsquash/src/pack_file/shader_file/example_non_transformable.glsl @@ -2,7 +2,8 @@ void main() { // Preprocessor directives outside of external declaration position - // (i.e., within statements or other GLSL symbols) cause PackSquash - // to fall back to no source transformation - #moj_import + // (i.e., within statements or other GLSL symbols) or unresolvable + // #moj_import cause PackSquash to fall back to no source + // transformation + #moj_import } diff --git a/packages/packsquash/src/pack_file/shader_file/parser.rs b/packages/packsquash/src/pack_file/shader_file/parser.rs index 9074b9524..e7cc4d468 100644 --- a/packages/packsquash/src/pack_file/shader_file/parser.rs +++ b/packages/packsquash/src/pack_file/shader_file/parser.rs @@ -2,8 +2,6 @@ use crate::pack_file::strip_utf8_bom; use aho_corasick::AhoCorasick; -use const_format::concatcp; -use const_random::const_random; use glsl_lang::ast::{ Expr, ExternalDeclarationData, FileId, Statement, TranslationUnit, TypeSpecifierNonArrayData }; @@ -18,12 +16,12 @@ use std::any::TypeId; use std::borrow::Cow; use std::cell::Cell; use std::convert::Infallible; +use std::fmt; use std::fmt::Write; use std::ops::{Deref, DerefMut}; use std::path::{Path, PathBuf}; use std::str::Utf8Error; use std::sync::LazyLock; -use std::{fmt, path}; use thiserror::Error; /// A GLSL lexer that does not preprocess its input before parsing, considering @@ -41,32 +39,19 @@ type NonPreprocessingLexer<'i> = glsl_lang_lexer::v2_min::str::Lexer<'i>; type PreprocessingLexer<'r, 'p, 'flag> = glsl_lang_lexer::v2_full::fs::Lexer<'r, 'p, ImportFileSystem<'flag>>; -/// The path prefix used in [`MOJ_IMPORT_PLACEHOLDER_PRAGMA`] to mark a given path as -/// included via `<...>` syntax (i.e., relative to the `include` folder, a.k.a. -/// "system include" in C/GLSL preprocessor literature), as opposed to `"..."` syntax. -const MOJ_IMPORT_PLACEHOLDER_PRAGMA_RELATIVE_MARKER_PATH_PREFIX: &str = - // The # character prevents this prefix from forming a valid resource location: - // Minecraft parses import paths as resource locations - concatcp!("__packsquash_internal_relative_marker#", const_random!(u16)); - -/// A placeholder, to be used only by PackSquash `#pragma` preprocessor directive that -/// signals that there was a `#moj_import` preprocessor directive at its position in -/// the original source. -/// -/// This directive is replaced by PackSquash before outputting transpiled shader code -/// with its corresponding `#moj_import`. It should not be used by any external application -/// under any circumstances. -static MOJ_IMPORT_PLACEHOLDER_PRAGMA: &str = concatcp!( - "#pragma __PACKSQUASH_INTERNAL_MOJ_IMPORT_PLACEHOLDER_", - const_random!(u16), - " " -); - /// An error that may happen while parsing a GLSL grammar symbol. #[derive(Error, Debug)] pub enum ParseError { - #[error("Syntax error: {0}")] - Syntax(#[from] glsl_lang::parse::ParseError>), + #[error("Syntax error: {error}")] + Syntax { + error: glsl_lang::parse::ParseError>, + /// Represents whether this syntax error happened when the source file contained + /// an unresolvable `#moj_import` directive. This indicates that the syntax error + /// might be a false positive, as it can be caused due to preprocessor directives + /// being inappropriately expanded or masked: imported shaders can change the + /// outcome of the preprocessing stage. + found_unresolvable_moj_import: bool + }, #[error("Non-included shaders must define a main function, but it was not defined")] MissingMainFunction, #[error("Invalid encoding: {0}")] @@ -111,17 +96,14 @@ impl Parser { // Wrap the input GLSL symbol into something that parses as a translation unit let source = T::wrap(std::str::from_utf8(strip_utf8_bom(source))?); - let inserted_moj_import_placeholder_pragma = Cell::new(false); + let found_unresolvable_moj_import = Cell::new(false); - let mut preprocessor = Processor::new_with_fs(ImportFileSystem::new( - &inserted_moj_import_placeholder_pragma - )); + let mut preprocessor = + Processor::new_with_fs(ImportFileSystem::new(&found_unresolvable_moj_import)); // Imports using the <...> syntax are known as "system imports", and the preprocessor needs // to know at least one system include directory to resolve them - preprocessor - .system_paths_mut() - .push(MOJ_IMPORT_PLACEHOLDER_PRAGMA_RELATIVE_MARKER_PATH_PREFIX.into()); + preprocessor.system_paths_mut().push(PathBuf::new()); TranslationUnit::parse_with_options::( preprocessor.open_source(&source, ""), @@ -158,19 +140,21 @@ impl Parser { // Expanding and removing preprocessor directives in top-level shaders does not alter // their semantics, as long as we are able to resolve every #moj_import to its source - // code, or no shader imported with #moj_import defines preprocessor variables that - // affect the source code compiled in the top-level shader. + // code, because shaders imported with #moj_import may define preprocessor variables + // that affect the source code compiled in the top-level shader. // // In general, we may not be able to resolve #moj_import to source files. But we can - // keep potential semantic alterations to a minimum if we check that the AST is capable - // of holding preprocessor directives that cannot be expanded in the same position as - // they originally were in the source. If that's not the case, then it's definitely not - // safe to transform this code - Ok((lacks_preprocessor_directives - || self.lacks_injectable_pp_directives_out_of_external_declaration_position( - &source - )) - .then(|| { + // keep semantics if we check that we could resolve all the #moj_import directives, and + // the AST is capable of holding preprocessor directives that cannot be expanded in the + // same position as they originally were in the source. If that's not the case, then + // it's definitely not safe to transform this code + let safe_to_transpile = lacks_preprocessor_directives + || (!found_unresolvable_moj_import.get() + && self.lacks_injectable_pp_directives_out_of_external_declaration_position( + &source + )); + + Ok(safe_to_transpile.then(|| { preprocessor_directives.inject(&mut translation_unit); // Wrapping and extracting a TU into itself is a no-op, so this always returns Some @@ -185,28 +169,31 @@ impl Parser { } }) .map_or_else( - |err| Err(err.into()), - |map_result| { - map_result.map(|symbol| { - symbol.flatten().map(|symbol| ParsedSymbol { - symbol, - inserted_moj_import_placeholder_pragma: - inserted_moj_import_placeholder_pragma.into_inner() - }) + |error| { + Err(ParseError::Syntax { + error, + found_unresolvable_moj_import: found_unresolvable_moj_import.get() }) + }, + |map_result| { + map_result.map(|symbol| symbol.flatten().map(|symbol| ParsedSymbol { symbol })) } ) } - /// Checks whether the specified GLSL source lacks injectable preprocessor directives + /// Checks whether the specified valid GLSL source lacks injectable preprocessor directives /// out of external declaration position. Currently, the injectable preprocessor directives - /// are `#version`, `#extension`, `#pragma` and `#moj_import`, which are necessary to - /// preserve in the generated GLSL code for the game to parse shaders as intended. + /// are `#version`, `#extension`, `#pragma` and `#moj_import`, which are necessary to preserve + /// (or, in the case of `#moj_import`, expand) in the generated GLSL code for the game to + /// parse shaders as intended. /// /// Currently, this method is implemented by removing preprocessor directives that do not get /// injected (i.e., are expanded) and trying to parse the result with a non-preprocessing lexer /// that only accepts preprocessor directives in external declaration position. Therefore, if /// it is best to avoid calling it if it is known that there are no preprocessor directives. + /// False negatives are returned if the source cannot be parsed when removing preprocessor + /// directives whose expansion is necessary for syntax correctness, which given the usage of + /// this function is a safe but inefficient fallback. fn lacks_injectable_pp_directives_out_of_external_declaration_position( &self, source: &str @@ -219,7 +206,7 @@ impl Parser { AhoCorasick::new(NEWLINE_ESCAPES).unwrap(), // When this function is called we know that any present preprocessor directives // are valid and follow their expected syntax, so this regex can be a bit lenient - Regex::new("(?m:^[[:space:]]*#[[:space:]]*(?:|define.*|undef.*|if.*|else.*|elif.*|endif.*|error.*|line.*)$)").unwrap() + Regex::new("(?m:^[[:space:]]*#[[:space:]]*(?:|define.*|undef.*|if.*|else.*|elif.*|endif.*|error.*|line.*|moj_import.*)$)").unwrap() ) }); @@ -242,38 +229,6 @@ impl Parser { ) .is_ok() } - - /// Replaces internal, temporary `#moj_import` placeholder pragmas with their corresponding - /// `#moj_import` directive in the provided `source`, which is assumed to have been generated - /// by the GLSL transpiler used by PackSquash. See [`MOJ_IMPORT_PLACEHOLDER_PRAGMA`] for more - /// details. - /// - /// The implementation of this method is optimized for the case where there is at least one - /// placeholder `#pragma` to replace. - fn replace_moj_import_placeholder_pragmas(source: &str) -> String { - source - .split('\n') // Our GLSL transpiler always uses LF and doesn't insert newline escapes - .map(|line| { - if let Some(import_path) = line - .trim_start_matches(|c: char| c.is_ascii_whitespace()) - .strip_prefix(MOJ_IMPORT_PLACEHOLDER_PRAGMA) - { - if let Some(import_path) = import_path.strip_prefix(concatcp!( - MOJ_IMPORT_PLACEHOLDER_PRAGMA_RELATIVE_MARKER_PATH_PREFIX, - path::MAIN_SEPARATOR - )) { - // Luckily, Minecraft does not support escaping > or " in #moj_import paths - Cow::Owned(format!("#moj_import <{import_path}>")) - } else { - Cow::Owned(format!("#moj_import \"{import_path}\"")) - } - } else { - Cow::Borrowed(line) - } - }) - .intersperse(Cow::Borrowed("\n")) - .collect() - } } /// The virtual filesystem used by [`PreprocessingLexer`] to expand `#moj_import` @@ -281,16 +236,17 @@ impl Parser { /// /// This filesystem records whether any `#moj_import` directive was expanded. struct ImportFileSystem<'flag> { - inserted_moj_import_placeholder_pragma: &'flag Cell + found_unresolvable_moj_import: &'flag Cell } impl<'flag> ImportFileSystem<'flag> { /// Creates a new virtual filesystem used by the preprocessing shader lexer. - /// `inserted_moj_import_placeholder_pragma` will be set to `true` if any - /// `#moj_import` directive is expanded. - fn new(inserted_moj_import_placeholder_pragma: &'flag Cell) -> Self { + /// `found_unresolvable_moj_import` will be set to `true` if any `#moj_import` + /// directive is found, because it is not possible to expand them at the + /// moment. + fn new(found_unresolvable_moj_import: &'flag Cell) -> Self { Self { - inserted_moj_import_placeholder_pragma + found_unresolvable_moj_import } } } @@ -298,32 +254,34 @@ impl<'flag> ImportFileSystem<'flag> { impl FileSystem for ImportFileSystem<'_> { type Error = Infallible; - fn canonicalize(&self, path: &Path) -> Result { - Ok(path.into()) + fn canonicalize(&self, _: &Path) -> Result { + Ok(PathBuf::new()) } fn exists(&self, _: &Path) -> bool { true } - fn read(&self, path: &Path) -> Result, Self::Error> { - // For now, always expand included files to placeholder #pragmas, to replace them - // back with #moj_imports when transpiling. For v0.4.0, we could try to actually - // expand the files for parsing, which should support more inputs (e.g., shaders - // that depend on included tokens to be syntactically correct, or rely on included - // preprocessor variables to select code to compile). If the referenced file does - // not exist on this pack, then we could fall back to accepting potentially invalid - // input: it could become valid after resolving imports - self.inserted_moj_import_placeholder_pragma.set(true); - Ok(format!("{MOJ_IMPORT_PLACEHOLDER_PRAGMA}{}", path.to_str().unwrap()).into()) + fn read(&self, _: &Path) -> Result, Self::Error> { + // For now, just record that a #moj_import was found. A #moj_import'ed file + // may contain preprocessor directives whose knowledge may be necessary to properly + // expand other preprocessor directives, and thus keep the GLSL source semantics + // when doing AST transformations and transpiling. We can't resolve #moj_import + // at the moment because we can't access pack files here, so if such a directive + // is found, we know we should not proceed with any transformations: doing AST + // transformations and transpiling is appropriate if and only if we can expand all + // the preprocessor directives. Also consider that not expanding can cause failure + // to correctly parse shaders that depend on imported text to be syntactically + // correct, but those should be not that common to find in practice + self.found_unresolvable_moj_import.set(true); + Ok("".into()) } } /// A parsed GLSL grammar symbol returned by a [`Parser`]. -#[derive(Debug)] +#[derive(Debug, Eq, PartialEq)] pub struct ParsedSymbol { - symbol: T, - inserted_moj_import_placeholder_pragma: bool + symbol: T } /// Represents a GLSL grammar symbol that can be transpiled back to GLSL. @@ -351,11 +309,7 @@ where ) .unwrap(); - if self.inserted_moj_import_placeholder_pragma { - Parser::replace_moj_import_placeholder_pragmas(&output) - } else { - output - } + output } } @@ -373,16 +327,7 @@ impl DerefMut for ParsedSymbol { } } -impl PartialEq for ParsedSymbol { - fn eq(&self, other: &Self) -> bool { - self.symbol.eq(&other.symbol) - } -} - -impl Eq for ParsedSymbol {} - -/// Internal trait used to mark types that can carry out a part of the -/// transpilation steps for GLSL symbols. +/// Internal trait used to encapsulate GLSL symbol-specific transpilation logic. trait TranspilableInner { fn transpile( &self, diff --git a/packages/packsquash/src/pack_file/shader_file/tests.rs b/packages/packsquash/src/pack_file/shader_file/tests.rs index 9b0fd2584..b5125eab9 100644 --- a/packages/packsquash/src/pack_file/shader_file/tests.rs +++ b/packages/packsquash/src/pack_file/shader_file/tests.rs @@ -8,15 +8,18 @@ use super::*; static FRAGMENT_SHADER_DATA: &[u8] = include_bytes!("example.fsh"); static NON_TRANSFORMABLE_SHADER_DATA: &[u8] = include_bytes!("example_non_transformable.glsl"); +static FALSE_POSITIVE_PARSE_ERROR: &[u8] = include_bytes!("example_false_positive_parse_error.glsl"); /// Processes the given input data as a [ShaderFile], using the provided settings, /// expecting a successful result that equals the expected string. +#[allow(clippy::too_many_arguments)] async fn successful_process_test + PartialEq + Debug + 'static>( input_data: &[u8], add_bom: bool, settings: ShaderFileOptions, is_vertex_or_fragment_shader: bool, is_top_level_translation_unit: bool, + expect_parseable_input_and_equal_ast_after_processing: bool, expect_smaller_file_size: bool, expect_output_equal_to_input: bool ) { @@ -24,11 +27,7 @@ async fn successful_process_test + PartialEq + D let input_data_str = std::str::from_utf8(input_data); - let input_ast = shader_parser - .parse::(input_data, is_top_level_translation_unit) - .expect("The test input data should be a valid GLSL symbol"); - - let input_data = if add_bom { + let input_data_with_maybe_bom = if add_bom { let mut input_data = input_data.to_vec(); input_data.splice(0..0, BOM_UTF8); Cow::Owned(input_data) @@ -37,8 +36,8 @@ async fn successful_process_test + PartialEq + D }; let data_stream = ShaderFile { - read: Builder::new().read(&input_data).build(), - file_length_hint: input_data.len(), + read: Builder::new().read(&input_data_with_maybe_bom).build(), + file_length_hint: input_data_with_maybe_bom.len(), is_vertex_or_fragment_shader, optimization_settings: settings } @@ -49,7 +48,7 @@ async fn successful_process_test + PartialEq + D .collect() .await; - let mut data = Vec::with_capacity(input_data.len()); + let mut data = Vec::with_capacity(input_data_with_maybe_bom.len()); for (_, partial_data) in process_result { data.extend_from_slice(&partial_data); } @@ -63,14 +62,20 @@ async fn successful_process_test + PartialEq + D assert!(!data.is_empty(), "Some data was expected for this input"); - let processed_ast = shader_parser - .parse::(&data, is_top_level_translation_unit) - .expect("The result should be a valid GLSL symbol"); + if expect_parseable_input_and_equal_ast_after_processing { + let input_ast = shader_parser + .parse::(input_data, is_top_level_translation_unit) + .expect("The test input data should be a valid GLSL symbol"); - assert_eq!( - input_ast, processed_ast, - "The processed translation unit should represent the same AST as the input" - ); + let processed_ast = shader_parser + .parse::(&data, is_top_level_translation_unit) + .expect("The result should be a valid GLSL symbol"); + + assert_eq!( + input_ast, processed_ast, + "The processed translation unit should represent the same AST as the input" + ); + } assert!( !expect_smaller_file_size || data.len() < input_data.len(), @@ -94,6 +99,7 @@ async fn minifying_works() { }, true, // Fragment shader true, // Top-level TU + true, // The output AST should match the input AST true, // Smaller size false // Different output text ) @@ -111,6 +117,7 @@ async fn minifying_with_bom_works() { }, true, // Fragment shader true, // Top-level TU + true, // The output AST should match the input AST true, // Smaller size false // Different output text ) @@ -128,6 +135,7 @@ async fn prettifying_works() { }, true, // Fragment shader true, // Top-level TU + true, // The output AST should match the input AST false, // Same/bigger size false // Different output text ) @@ -145,6 +153,7 @@ async fn passthrough_works() { }, true, // Fragment shader true, // Top-level TU + true, // The output AST should match the input AST false, // Same size true // Same output text ) @@ -169,7 +178,7 @@ async fn invalid_input_is_handled() { } #[tokio::test] -async fn minifying_is_averted_when_preprocessor_directives_would_be_lost() { +async fn minifying_is_averted_when_preprocessor_expansion_would_be_incomplete() { successful_process_test::( NON_TRANSFORMABLE_SHADER_DATA, false, // No BOM @@ -179,6 +188,25 @@ async fn minifying_is_averted_when_preprocessor_directives_would_be_lost() { }, true, // Fragment shader true, // Top-level TU + true, // The output AST should match the input AST + false, // Same size + true // Same output text + ) + .await +} + +#[tokio::test] +async fn tentative_parsing_errors_are_not_fatal() { + successful_process_test::( + FALSE_POSITIVE_PARSE_ERROR, + false, // No BOM + ShaderFileOptions { + source_transformation_strategy: ShaderSourceTransformationStrategy::Minify, + ..Default::default() + }, + true, // Fragment shader + true, // Top-level TU + false, // The input AST cannot be constructed false, // Same size true // Same output text ) @@ -196,6 +224,7 @@ async fn include_shaders_may_not_be_translation_units() { }, false, // Include shader false, // Not top-level + true, // The output AST should match the input AST true, // Smaller size false // Different output text ) @@ -213,6 +242,7 @@ async fn include_shaders_with_preprocessor_directives_are_passed_through() { }, false, // Include shader false, // Not top-level + true, // The output AST should match the input AST false, // Smaller size true // Same output text )