fix(shader_file): fallback to compat. no transform if #moj_import i…

…s involved

As highlighted in issue #187, a significant number of practical shaders
use `#moj_import`s in ways that are tricky for PackSquash to handle
without import expansion capabilities. The necessary redesign to achieve
expansion will ikely take some time, but there is stakeholder pressure
to get better (and more correct) solutions working now: PackSquash has
arguably been incorrect in how it processes shaders for months, and
justifying the status quo with upcoming plans to tackle the problems at
their source is no longer a tenable proposition, leading to
relatively frequent uncomfortable situations for both me and users.

Therefore, let's play the cards very close to our chest when it comes to
transforming GLSL sources: don't do it unless we can be certain that we
have a full AST and all the preprocessor state is known. This entails
significant optimization regressions for any shader that uses
`#moj_import`, as not expanding that directive is the root of all evil,
but PackSquash will at least just work in much more cases. In
particular, these changes were tested with FancyPants, TextEffects v2.1
and objmc, which are interesting shaders from a parsing standpoint due
to their popularity and intensive usage of GLSL language features.

.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

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.

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"

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::<TranslationUnit>(
 				src,
 				&shader_parser,
@@ -108,6 +110,16 @@ impl Decoder for OptimizerDecoder {
 			.or_else(|_| {
 				process_shader_as::<Expr>(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<T: Extractable<TranslationUnit> + 'static>(
 where
 	ParsedSymbol<T>: Transpilable
 {
-	if let (
-		Some(symbol),
-		ShaderSourceTransformationStrategy::Minify | ShaderSourceTransformationStrategy::Prettify
-	) = (
-		shader_parser.parse::<T>(src, is_top_level_translation_unit)?,
+	match (
+		shader_parser.parse::<T>(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())
 	}
 }

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 <base.fsh>
-#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);
 }

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 <unresolvable.glsl>
+
+vec2 var SEMICOLON

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 <mist.glsl>
+    // (i.e., within statements or other GLSL symbols) or unresolvable
+    // #moj_import cause PackSquash to fall back to no source
+    // transformation
+    #moj_import <mist.glsl>
 }